fix(WebVTT): Add support to voice tag styles #4845
Conversation
avelad
added
type: enhancement
|
Incremental code coverage: 91.26% |
lib/text/vtt_text_parser.js
Outdated
| for (let i = 0; i < payload.length; i++) { | ||
| if (payload[i] === '/') { | ||
| const end = payload.indexOf('>', i); | ||
| if (end <= i) { |
There was a problem hiding this comment.
In what condition do we expect end <= i? We start searching at i, so end will only be less than i if it's -1. And we know that payload[i] is equal to '/', so end is guaranteed to not equal i.
If this is just meant to handle end being -1, then it'd be more informative to write that out.
| * @return {string} processed payload | ||
| * @private | ||
| */ | ||
| static replaceVoiceStylePayload_(payload) { |
There was a problem hiding this comment.
I don't know if this method is correct?
I tried running it on this simple content:
'WEBVTT\n\n' +
'00:00:20.000 --> 00:00:40.000\n' +
'<v A>Test\n\n' +
'00:00:40.000 --> 00:00:50.000\n' +
'<v B>Test'
And I got this output (broken into multiple lines for readability):
'WEBVTT\n\n' +
'00:00:20.000 --> 00:00:40.000\n' +
'<v.voice-A>Test\n\n' +
'00:00:40.000 --> 00:00:50.000\n' +
'<v.voice-B>Test\n' +
'</v.voice-A></v.voice-B>'
Note that both of the end tags are at the end of the file. Also, the end tags are in the wrong order; this ends up being like <A><B></A></B>, which is incorrectly nested.
Incidentally, if one of the tags already has an end tag, the other isn't given one:
'WEBVTT\n\n' +
'00:00:20.000 --> 00:00:40.000\n' +
'<v A>Test</v>\n\n' +
'00:00:40.000 --> 00:00:50.000\n' +
'<v B>Test'
Results in:
'WEBVTT\n\n' +
'00:00:20.000 --> 00:00:40.000\n' +
'<v.voice-A>Test</v.voice-A>\n\n' +
'00:00:40.000 --> 00:00:50.000\n' +
'<v.voice-B>Test\n'
There was a problem hiding this comment.
You are wrong! The payload parameter is <v A>Test</v> or <v B>Test. I retested with this input and it works well!
There was a problem hiding this comment.
Oh, I see, I had misread the method. I was mistakenly thinking it was parseCueStyles as in parse the styles of all cues, not parse all of the styles of one cue.
I guess if the expectation is that there will be at most one voice tag per invocation of this method, this should be fine.
|
@theodab can you review again? Thanks! |
|
Thanks @theodab !! |
|
Good news: This fixed one of the test cases I'm adding for VTT layout. 😁 |
joeyparrish
changed the title
github-actions
bot
added
the
status: archived
closes #4844
fixes #4843
fixes #4479
BEGIN_COMMIT_OVERRIDE
fix(WebVTT): Fix voice tag styles
END_COMMIT_OVERRIDE