fix(text): Fix TTML render timing and line break issues #4407
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…issues
This changes the SimpleTextDisplayer to flatten text payloads
only starting at non-container cues. Not doing so poses an issue with
'rolled over' caption lines.
E.g., consider:
- TTML file N with caption line A from time [1.4, 2.1].
- TTML file N+1 with caption lines A from time [1.4, 2.4] and
B from time [2.4, 3.4].
If captions A and B are descendents of two different <p> elements, Shaka
currently flattens the text payload from A and B into a single cue that is
rendered onto the display from time [1.4, 3.4]. Therefore, caption A's text
payload is incorrectly showing during time [2.4, 3.4].\nThe fix updates the
SimpleTextDisplayer append() logic to split up individual non-container cues
into their own VTTCue object. This will not be done at the TTMLParser level to
ensure proper style inheritance still occurs for the (actual) text cues.
Closes shaka-project#4381
|
That is an excellent PR description. Very helpful! |
joeyparrish
requested changes
Aug 15, 2022
lib/text/simple_text_displayer.js
Outdated
| const getCuesToFlatten = (cues) => { | ||
| for (const cue of cues) { | ||
| if (cue.isContainer) { | ||
| flattenedCues = |
There was a problem hiding this comment.
I have a problem with the data flow here. flattenedCues is set to the result of getCuesToFlatten, which itself modifies flattenedCues in multiple ways and then returns flattenedCues. This is very hard to follow.
If getCuesToFlatten is going to be recursive, you shouldn't modify an effectively "global" variable (not truly global, but captured by this closure, so the effect is similar).
There was a problem hiding this comment.
I've updated the function to not modify and reference a variable outside of the closure by passing a placeholder result array which is returned by the function.
| @@ -127,6 +127,33 @@ describe('SimpleTextDisplayer', () => { | |||
| [shakaCue]); | |||
| }); | |||
|
|
|||
| it('flattens nested cue payloads correctly', () => { | |||
There was a problem hiding this comment.
This test looks great. So I'm not concerned about the validity of the code change, so much as the clarity and readability.
There was a problem hiding this comment.
Thanks, after the updates based on your other comments I think it should be easier to follow.
lib/text/simple_text_displayer.js
Outdated
| const flatCue = cue.clone(); | ||
| flatCue.nestedCues = []; | ||
| flatCue.payload = flattenPayload(cue); | ||
| flattenedCues = flattenedCues.concat(flatCue); |
There was a problem hiding this comment.
I was surprised to learn that Array.concat can be used for non-arrays, to append elements. For clarity when pushing a single value, I would suggest you use Array.push instead, and we save Array.concat for concatenating two arrays.
There was a problem hiding this comment.
Done. Agreed this would be more readable, thanks!
…r' variable which is also the result.
joeyparrish
approved these changes
Aug 16, 2022
joeyparrish
pushed a commit
that referenced
this pull request
Aug 16, 2022
This changes the `SimpleTextDisplayer` to flatten text payloads only starting at non-container cues. Not doing so poses an issue with 'rolled over' caption lines. E.g., consider: - TTML file `N` with caption line `A` from time `[1.4, 2.1]`. - TTML file `N+1` with caption lines `A` from time `[1.4, 2.4]` and `B` from time `[2.4, 3.4]`. If captions `A` and `B` are descendants of two different `<p>` elements, Shaka currently flattens the text payload from `A` and `B` into a single cue that is rendered onto the display from time `[1.4, 3.4]`. Therefore, caption `A`'s text payload is incorrectly showing during time `[2.4, 3.4]`. The fix updates the `SimpleTextDisplayer#append()` logic to split up individual non-container cues into their own `VTTCue` object. This will not be done at the `TTMLTextParser` level to ensure proper style inheritance still occurs for the (actual) text cues. Closes #4381
joeyparrish
pushed a commit
that referenced
this pull request
Aug 16, 2022
This changes the `SimpleTextDisplayer` to flatten text payloads only starting at non-container cues. Not doing so poses an issue with 'rolled over' caption lines. E.g., consider: - TTML file `N` with caption line `A` from time `[1.4, 2.1]`. - TTML file `N+1` with caption lines `A` from time `[1.4, 2.4]` and `B` from time `[2.4, 3.4]`. If captions `A` and `B` are descendants of two different `<p>` elements, Shaka currently flattens the text payload from `A` and `B` into a single cue that is rendered onto the display from time `[1.4, 3.4]`. Therefore, caption `A`'s text payload is incorrectly showing during time `[2.4, 3.4]`. The fix updates the `SimpleTextDisplayer#append()` logic to split up individual non-container cues into their own `VTTCue` object. This will not be done at the `TTMLTextParser` level to ensure proper style inheritance still occurs for the (actual) text cues. Closes #4381
joeyparrish
pushed a commit
that referenced
this pull request
Aug 16, 2022
This changes the `SimpleTextDisplayer` to flatten text payloads only starting at non-container cues. Not doing so poses an issue with 'rolled over' caption lines. E.g., consider: - TTML file `N` with caption line `A` from time `[1.4, 2.1]`. - TTML file `N+1` with caption lines `A` from time `[1.4, 2.4]` and `B` from time `[2.4, 3.4]`. If captions `A` and `B` are descendants of two different `<p>` elements, Shaka currently flattens the text payload from `A` and `B` into a single cue that is rendered onto the display from time `[1.4, 3.4]`. Therefore, caption `A`'s text payload is incorrectly showing during time `[2.4, 3.4]`. The fix updates the `SimpleTextDisplayer#append()` logic to split up individual non-container cues into their own `VTTCue` object. This will not be done at the `TTMLTextParser` level to ensure proper style inheritance still occurs for the (actual) text cues. Closes #4381
joeyparrish
pushed a commit
that referenced
this pull request
Aug 16, 2022
This changes the `SimpleTextDisplayer` to flatten text payloads only starting at non-container cues. Not doing so poses an issue with 'rolled over' caption lines. E.g., consider: - TTML file `N` with caption line `A` from time `[1.4, 2.1]`. - TTML file `N+1` with caption lines `A` from time `[1.4, 2.4]` and `B` from time `[2.4, 3.4]`. If captions `A` and `B` are descendants of two different `<p>` elements, Shaka currently flattens the text payload from `A` and `B` into a single cue that is rendered onto the display from time `[1.4, 3.4]`. Therefore, caption `A`'s text payload is incorrectly showing during time `[2.4, 3.4]`. The fix updates the `SimpleTextDisplayer#append()` logic to split up individual non-container cues into their own `VTTCue` object. This will not be done at the `TTMLTextParser` level to ensure proper style inheritance still occurs for the (actual) text cues. Closes #4381
github-actions
bot
added
the
status: archived
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This changes the
SimpleTextDisplayerto flatten text payloadsonly starting at non-container cues. Not doing so poses an issue with
'rolled over' caption lines.
E.g., consider:
Nwith caption lineAfrom time[1.4, 2.1].N+1with caption linesAfrom time[1.4, 2.4]andBfrom time[2.4, 3.4].If captions
AandBare descendants of two different<p>elements, Shakacurrently flattens the text payload from
AandBinto a single cue that isrendered onto the display from time
[1.4, 3.4]. Therefore, captionA's textpayload is incorrectly showing during time
[2.4, 3.4].The fix updates the
SimpleTextDisplayer#append()logic to split up individualnon-container cues into their own
VTTCueobject. This will not be done at theTTMLTextParserlevel to ensure proper style inheritance still occurs for the(actual) text cues.
Closes #4381
BEGIN_COMMIT_OVERRIDE
fix(text): Fix TTML render timing and line break issues for native display
END_COMMIT_OVERRIDE