Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: don't trim line breaks #5380

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

PopoSensei
Copy link

This PR will...

Allow empty line break lines as subtitle rows.

Why is this Pull Request needed?

In SRT they have no means to define line height. So they use line breaks to define where subtitles should be in video.

Are there any points in the code the reviewer needs to double check?

I am unsure how row 106 .trim() will effect overall, but in this case it allows for empty line breaks to exist. The other trim()relocation trims the subtitle row for extra white spaces.

Resolves issues:

#5337

Checklist

  • changes have been done against master branch, and PR does not conflict
  • new unit / functional tests have been added (whenever applicable)
  • API or design changes are documented in API.md

@PopoSensei
Copy link
Author

@robwalch asked to be cc:d with @OrenMe, the author #3431.

@OrenMe
Copy link
Collaborator

OrenMe commented Apr 11, 2023

It is a bit old and I'm not active at the moment on the recent project but IIRC it fixed an issue with line break at end of VTT cue especially when it is cut to segment
If you cut a cue on segment boundary and it crosses it it will appear at end of vtt file of one segment as last one and as first one in the other segment and the extra line break is really up to how the segment logic is implemented.
The main issue happens in logic that checks if this is same cue and it is a hash function afaicr so the cues don't match cause one has trimmed empty line and one doesn't so it shows cue twice at the same time.
I think this fox will break it again
The fix you need if I understand correctly is if line break is in middle of vtt cue text while my fix was handling extra line break at end of string which is a by product of the greedy evaluation of string borders we had that didn't trim empty line between discrete cues

@mtoczko
Copy link
Collaborator

mtoczko commented Apr 11, 2023

Hi @OrenMe
Do you have a stream that can be used to create tests based on it? The example from #3431 will now show duplicates because the cue id handling has been changed.

@OrenMe
Copy link
Collaborator

OrenMe commented Apr 11, 2023

@mtoczko not sure I follow, the example case I added in original ticket is what you can use for tests and if you already see it is broken with your change for this example so it means this change is breaking functionality
Or you mean that already now the example is broken from main branch?

@robwalch robwalch added the Needs Triage If there is a suspected stream issue, apply this label to triage if it is something we should fix. label Feb 7, 2024
@robwalch robwalch added Stale and removed Needs Triage If there is a suspected stream issue, apply this label to triage if it is something we should fix. labels Aug 9, 2024
@stale stale bot removed the Stale label Aug 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants