1551385 - [webvtt] cue display wasn't updated correctly when there are multiple cues with overlapping timestamp

Status: VERIFIED FIXED

(Core :: Audio/Video: Playback, defect, P2)

Description

[Alastor Wu [:alwu]]

STR.

1. goto https://alastor0325.github.io/htmltests/webvtt/cues_time_overlap.html

Expected.
2. "Second cue" should disappear after 2s

Actual.
2. "Second cue" is still appearing, it disappears with "First cue".

Comment 1

[Alastor Wu [:alwu]]

Attachment: Bug 1551385 - part2 : recompute display state when display cues amount is different from the one we rendered last time.

We can actually let processCue() to handle rendering cues or cleaning displayed cues, no need to use another way to clear the cue.

The advantages is to make the code cleaner and easier to read, now we just need to know JS side would handle all rendering stuffs for us. We don't need to have different behavior when there is no showing cue.

The way we clear displayed cues are intuitive, we would remove all child nodes under the overlay, which are used to display cues.

Comment 2

[Alastor Wu [:alwu]]

Attachment: Bug 1551385 - part1 : let 'processCue()' handle cleaning cues div.

We can actually let processCue() to handle rendering cues or cleaning displayed cues, no need to use another way to clear the cue.

The advantages is to make the code cleaner and easier to read, now we just need to know JS side would handle all rendering stuffs for us. We don't need to have different behavior when there is no showing cue.

The way we clear displayed cues are intuitive, we would remove all child nodes under the overlay, which are used to display cues.

Comment 3

[Alastor Wu [:alwu]]

Attachment: Bug 1551385 - part3 : add test 'test_webvtt_overlapping_time.html' and 'vtt_overlapping_time.html'.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/883151b42d88
part1 : let 'processCue()' handle cleaning cues div. r=heycam
https://hg.mozilla.org/integration/autoland/rev/fb20dcf3c072
part2 : recompute display state when display cues amount is different from the one we rendered last time. r=heycam
https://hg.mozilla.org/integration/autoland/rev/4268e661e741
part3 : add test 'test_webvtt_overlapping_time.html' and 'vtt_overlapping_time.html'. r=heycam

Comment 5

[Andreea Pavel [:apavel]]

Backed out for breaking reftests at vtt_overlapping_time.html

Push that started the failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&selectedJob=248054168&revision=4268e661e741a5ad122880f818bbcabc9135cb7d

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&selectedJob=248051339&revision=7eaaa8f90abd04fd6bedfd8e0dbd8fcba5027c0a

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=248057729&repo=autoland&lineNumber=1590

Backout: https://hg.mozilla.org/integration/autoland/rev/ca67f4681a4a6674f3ae2e1bed1bc2cfbdea3749

Comment 6

[Alastor Wu [:alwu]]

That is a known issue (bug1546133), I will add fuzzy for this test on Windows.

Comment 7

[Alastor Wu [:alwu]]

From the try result [1], although we got some orange on wpt8, but all of them are related with service work, which are impossible to be affected by my changes.

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=ba57ed9eb351b1ce92cc1aa31c1f5c70b8596df2&selectedJob=248100189

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d3159b7f0a9c
part1 : let 'processCue()' handle cleaning cues div. r=heycam
https://hg.mozilla.org/integration/autoland/rev/3ca6e66532a6
part2 : recompute display state when display cues amount is different from the one we rendered last time. r=heycam
https://hg.mozilla.org/integration/autoland/rev/08cd8019be34
part3 : add test 'test_webvtt_overlapping_time.html' and 'vtt_overlapping_time.html'. r=heycam

Comment 9

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/d3159b7f0a9c
https://hg.mozilla.org/mozilla-central/rev/3ca6e66532a6
https://hg.mozilla.org/mozilla-central/rev/08cd8019be34

Comment 10

[Alastor Wu [:alwu]]

Comment on attachment 9064932 [details]
Bug 1551385 - part1 : let 'processCue()' handle cleaning cues div.

Beta/Release Uplift Approval Request

• User impact if declined: When there are some cues having overlapping timestamp, the cue with earlier end time won't disappear correctly when the time reaches its end time.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: See comment0.
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): What my change did is to force us to recompute cue display when the number of active cues changes, so it just trigger another display update, we didn't add new functionality in this bug. In addition, we have automation tests for this issue, it would be safe to catch any unexpected regressions.
• String changes made/needed: none

Comment 11

[Alastor Wu [:alwu]]

Attachment: Bug 1551385 - part1 : introduce new state to distinguish the different situation for showing cues' display state.

Cue's display state is a DIV element with corresponding CSS style to display cue on the screen. When the cue is being displayed first time, we will compute its display state.

After that, we could reuse its state until following conditions happen.
(1) control changes : it means the rendering area changes so we should recompute cues' position.
(2) cue's hasBeenReset flag is true : it means cues' line or position property has been modified, we also need to recompute cues' position.
(3) the amount of showing cues changes : it means some cue would disappear but other cues should stay at the same place without recomputing, so we can resume their display state.

The new state includes REUSE, REUSE AND CLEAR and COMPUTE AND CLEAR, our current behavior doesn't handle `REUSE AND CLEAR, which would be implemented in next patch.

Comment 12

[Alastor Wu [:alwu]]

Attachment: Bug 1551385 - part2 : handle state `REUSE AND CLEAR`.

According to the spec [1] step 13, if we won't reset cues' display state, we should reuse them.

If the amount of displaying cues changes, which means some cues should disappear from the screen and some of them should still be showed, we have to clear the screen and resume displaying cues' display state.

As the amount of displaying cues might affect the computed result of cues' display state, if we recompute cues' display state in this situaion, the result would be different from the one we had last time.

For example, there are two cues, Cue0 [0:2], Cue1 [1:5]. When Cue0 disappears, Cue1 should stay in the same position.

[playing at 1s]
-------------------
| Cue1 |
| Cue0 |
-------------------

[playing at 3s] (correct)
-------------------
| Cue1 |
| |
-------------------

[playing at 3s] (incorrect)
-------------------
| |
| Cue1 |
-------------------

[1] https://w3c.github.io/webvtt/#rules-for-updating-the-display-of-webvtt-text-tracks

Comment 13

[Alastor Wu [:alwu]]

Attachment: Bug 1551385 - part3 : modify log.

Add and modify some log content in order to help debugging.

Comment 14

[Alastor Wu [:alwu]]

Attachment: Bug 1551385 - part4 : modify wpt 'track-webvtt-two-cue-layout-after-first-end.html' and 'track-webvtt-two-cue-layout-after-first-end-ref.html'.

For track-webvtt-two-cue-layout-after-first-end.html

(1) modify cue1's start time

According to the text track cue order [1], "cues must be sorted by their start time, earliest first; then, any cues with the same start time must be sorted by their end time, latest first".

This order also decides which cue we would display first. As this test would like show cue1 at top and show another cue at bottom, we should modify cue1's start in order to put it before cue [0:3].

[1] https://html.spec.whatwg.org/multipage/media.html#text-track-cue-order

(2) listen for cue1's exit

As this cue would like to stop when the first cue ends, it should listen exit event.

---

For track-webvtt-two-cue-layout-after-first-end-ref.html, we should call video.play() in order to clear show-poster flag [2] and run TimeMarchesOn to show the cue.

If we didn't call it to reset the flag, we won't display any cue.

[2] https://html.spec.whatwg.org/multipage/media.html#playing-the-media-resource:show-poster-flag

Comment 15

[Alastor Wu [:alwu]]

Attachment: Bug 1551385 - part5 : enable wpt 'track-webvtt-two-cue-layout-after-first-end.html'.

Comment 16

[Phabricator Automation]

Comment on attachment 9068237 [details]
Bug 1551385 - part1 : introduce new state to distinguish the different situation for showing cues' display state.

Revision D32934 was moved to bug 1555197. Setting attachment 9068237 [details] to obsolete.

Comment 17

[Phabricator Automation]

Comment on attachment 9068238 [details]
Bug 1551385 - part2 : handle state REUSE AND CLEAR.

Revision D32935 was moved to bug 1555197. Setting attachment 9068238 [details] to obsolete.

Comment 18

[Phabricator Automation]

Comment on attachment 9068239 [details]
Bug 1551385 - part3 : modify log.

Revision D32936 was moved to bug 1555197. Setting attachment 9068239 [details] to obsolete.

Comment 19

[Phabricator Automation]

Comment on attachment 9068240 [details]
Bug 1551385 - part4 : modify wpt 'track-webvtt-two-cue-layout-after-first-end.html' and 'track-webvtt-two-cue-layout-after-first-end-ref.html'.

Revision D32937 was moved to bug 1555197. Setting attachment 9068240 [details] to obsolete.

Comment 20

[Phabricator Automation]

Comment on attachment 9068241 [details]
Bug 1551385 - part5 : enable wpt 'track-webvtt-two-cue-layout-after-first-end.html'.

Revision D32938 was moved to bug 1555197. Setting attachment 9068241 [details] to obsolete.

Comment 21

[Oana Botisan, Desktop QA]

I managed to reproduce the issue using an older version of Nightly (2019-05-13) on Windows 10 x64.
I retested everything using latest Nightly 69.0a1 on Windows 10 x64, Ubuntu 18.04 x64 and macOS 10.13. The bug is not reproducing anymore.

I did notice on the other hand that the button "Picture-in-Picture" is not displayed anymore. Is that a known issue or it's something expected? Or is it a regression caused by this fix?

Comment 22

[Julien Cristau [:jcristau]]

Is this a regression? If so when was it introduced?

Comment 23

[Oana Botisan, Desktop QA]

I found the regression:
last good: 23-05-2019
first bad: 24-05-2019
But mozregression couldn't find a pushlog, but maybe this one will help: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=840b7106d8ae3158aeba8268d2ac0b40c3682bcb&tochange=f58ae8ec64c812739509de09659327bf7ea33494

I can log a separate bug for this issue.

Comment 24

[Alastor Wu [:alwu]]

No, my change is impossible to affect the video control. That must be caused by other bug.

Comment 25

[Alastor Wu [:alwu]]

(In reply to Oana Botisan, Desktop Release QA from comment #21)

I did notice on the other hand that the button "Picture-in-Picture" is not displayed anymore. Is that a known issue or it's something expected? Or is it a regression caused by this fix?

Look like Bug 1547795.

Comment 26

[Oana Botisan, Desktop QA]

According to comment 21 and comment 25 I will mark this issue as verified fixed.

Comment 27

[Julien Cristau [:jcristau]]

(In reply to Julien Cristau [:jcristau] from comment #22)

Is this a regression? If so when was it introduced?

Comment 28

[Alastor Wu [:alwu]]

This issue is an old existing bug, it's not a regression.

Comment 29

[Julien Cristau [:jcristau]]

Comment on attachment 9064931 [details]
Bug 1551385 - part2 : recompute display state when display cues amount is different from the one we rendered last time.

In that case I'd prefer to let this ride the trains to 69.