1509446 - [webvtt] cue should be removed from video's rendering area after it has been removed from text track

Status: RESOLVED FIXED

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

Description

[laszlo.bialis]

Attachment: Screen Shot 2018-11-22 at 4.48.41 PM.png

[Affected versions]:
Firefox Nightly 65.0a1 20181122220059, Beta 64.0b11 20181119162153, Release 63.0.3 20181114214635

[Affected platforms]:
All

[Steps to reproduce]:

1.Go to https://shaka-player-demo.appspot.com/demo/#asset=https://storage.googleapis.com/shaka-demo-assets/angel-one-widevine-hls/hls.m3u8;lang=en-US;build=uncompiled
2. Click on Load button
3. After the video begins, click on the CC button to show the subtitles
4. Select another codec from the drop down without deactivating the CC 
5. Click on Load and wait for the video to play 

[Expected result]:
Video should play and the subtitle should be aligned with the dialog. 

[Actual result]:
Depending on the codec selected from the drop down there are two possibilities:
1. If the secondary video contains audible text, then at first the last paragraph of the last subtitle from the previous video is shown, then the new subtitle appears but its offset from the dialog.
Ex: Angel One (multicodec, multilingual) to Angel One (HLS, MP4, multilingual, Widevine)
2.If the secondary video is without audible text and the image size is different then on the previous video the last paragraph of the last subtitle from the previous video is shown outside the video window (see image attached).
Ex: Angel One (multicodec, multilingual) to Sintel 4k (multicodec)

[Additional notes]:
The issue doesn't seem to be related to any of the codecs used.

Comment 1

[C.M.Chang[:chunmin]]

quick check for shaka-player: it supports may ways to show the subtitles[0], including WebVTT and TTML.

When I switch from "Sintel 4k (multicodec, VTT in MP4)" to "Tears of Steel (multicodec, TTML)", the last subtitle from the previous video doesn't disappear (Chorme will remove the the last subtitle after switching.). I guess it might relate the WebVTT.

Cameron, could you take a look?

[0] https://github.com/google/shaka-player#media-container-and-subtitle-support

Comment 2

[Cameron McCormack (:heycam)]

So I don't know where track changing code is -- maybe that is handled in TextTrackManager::TimeMarchesOn?  The actual insertion/removal of the WebVTT cues in the document happens in vtt.jsm, in the WebVTT.processCues function.  That's called by TextTrackManager::UpdateCueDisplay.  It might be that we need to call that function (or maybe TextTrackManager::DispatchUpdateCueDisplay?) when the text track is changed.

Comment 3

[Alastor Wu [:alwu]]

Will take a look at this one.

Comment 4

[Alastor Wu [:alwu]]

Attachment: Bug 1509446 - part1 : update active cue before returning it.

We don't need an extra flag to remind us to reset cues list in 'UpdateActiveCueList()', we can reset it every when it needs.

Comment 5

[Alastor Wu [:alwu]]

Attachment: Bug 1509446 - part6 : update cues display without checking whether cue is active or not.

We should update cue display everytime when the cues list changed.

In addition, we shouldn't check whether cue is active when we update display, because it's always inactive when the cue has been removed in 'TextTrack::RemoveCue()'.

Comment 6

[Alastor Wu [:alwu]]

Attachment: Bug 1509446 - part7 : add a reftest.

Adding a reftest to ensure we would update the cue display after removing cue from the text track, the removed cue should not display on the video's rendering area.

Comment 7

[Alastor Wu [:alwu]]

There are some incorrect implementation in TextTrack and TimeMarchesOn algorithm which made my patches not possible to be landed right now.

Will land these patches after fixing those problems.

Comment 8

[Alastor Wu [:alwu]]

Attachment: Bug 1509446 - part1 : update active cues list when cue's active state changed.

According to spec [1], activeCues should represent a subset of the text track cues whose active flag was set when the script started.

We should only depend on the TimeMarchesOn algorithm which will change cue's active state, and then add or remove cue to activeCues.

[1] https://html.spec.whatwg.org/multipage/media.html#dom-texttrack-activecues

Comment 9

[Alastor Wu [:alwu]]

Attachment: Bug 1509446 - part2 : use current cue in 'TimeMarchesOn' algorithm.

According to the spec [1], the current cue is not equal with the active cue, because it might contain non active cues.

The current cue should be a list of cues, initialized to contain all the cues of all the hidden or showing text tracks of the media element (not the disabled ones) whose start times are less than or equal to the current playback position and whose end times are greater than the current playback position.

[1] https://html.spec.whatwg.org/multipage/media.html#time-marches-on

Comment 10

[Alastor Wu [:alwu]]

Attachment: Bug 1509446 - part3 : run 'TimeMarchesOn' directly, instead of queuing a task.

According to the spec [1], we should run TimeMarchesOn algorithm directly when the specific situation happens, it doesn't say that we need to queue a task for it.

In addition, all the call sites for TimeMarchesOn are on the main thread, so we don't need to worry about race condition.

[1]
https://html.spec.whatwg.org/multipage/media.html#playing-the-media-resource:time-marches-on-2
https://html.spec.whatwg.org/multipage/media.html#playing-the-media-resource:time-marches-on-3
https://html.spec.whatwg.org/multipage/media.html#playing-the-media-resource:time-marches-on-4

Comment 11

[Alastor Wu [:alwu]]

Attachment: Bug 1509446 - part4 : remove set dirty.

As the active cues list would be automatically contruct when there are any active cues being added or inactive cues being removed, we have no need to use dirty to reset the active cues list.

Comment 12

[Alastor Wu [:alwu]]

Attachment: Bug 1509446 - part5 : enable wpt test 'track-cues-enter-exit.html'.

Fix the missing parameter in cueEntered() and enable this wpt.

Comment 13

[Alastor Wu [:alwu]]

Attachment: Bug 1509446 - part8 : modify wpt 'activeCues.html'.

According to the spec [1], activeCues attribute must return a live TextTrackCueList object, so we should get correct cue list immediately, no need to wait until the next event loop.

[1] https://html.spec.whatwg.org/multipage/media.html#dom-texttrack-activecues

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3944dae3dabd
part1 : update active cues list when cue's active state changed. r=jya
https://hg.mozilla.org/integration/autoland/rev/fa26e1068b50
part2 : use current cue in 'TimeMarchesOn' algorithm. r=jya
https://hg.mozilla.org/integration/autoland/rev/d1046aea2e0e
part3 : run 'TimeMarchesOn' directly, instead of queuing a task. r=jya
https://hg.mozilla.org/integration/autoland/rev/ad7bfe5bbc3a
part4 : remove set dirty. r=jya
https://hg.mozilla.org/integration/autoland/rev/057318480b0e
part5 : enable wpt test 'track-cues-enter-exit.html'. r=jya
https://hg.mozilla.org/integration/autoland/rev/a0d8bc1dab63
part6 : update cues display without checking whether cue is active or not. r=jya
https://hg.mozilla.org/integration/autoland/rev/b2ad34980053
part7 : add a reftest. r=jya
https://hg.mozilla.org/integration/autoland/rev/2f913db74eb5
part8 : modify wpt 'activeCues.html'. r=jya

Comment 15

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/3944dae3dabd
https://hg.mozilla.org/mozilla-central/rev/fa26e1068b50
https://hg.mozilla.org/mozilla-central/rev/d1046aea2e0e
https://hg.mozilla.org/mozilla-central/rev/ad7bfe5bbc3a
https://hg.mozilla.org/mozilla-central/rev/057318480b0e
https://hg.mozilla.org/mozilla-central/rev/a0d8bc1dab63
https://hg.mozilla.org/mozilla-central/rev/b2ad34980053
https://hg.mozilla.org/mozilla-central/rev/2f913db74eb5

Comment 16

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/15789 for changes under testing/web-platform/tests

Comment 17

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Can't merge web-platform-tests PR due to failing upstream checks:
Github PR https://github.com/web-platform-tests/wpt/pull/15789
* Taskcluster (pull_request) (https://tools.taskcluster.net/task-group-inspector/#/OYhu-QGtRIegKtIl5lQ8CA)

Comment 18

[Alastor Wu [:alwu]]

Hi, James,

I heard that you might be the right person to ask about how to handle wpt test merging fail.

The comment17 shows that there are some upstream checks fail on Chrome which resulted in merging fail. After checking the fail log [1], it failed on the activeCues.html. However, that test has already failed on Chrome before merging my changes, so I think these fails should be skipped and shouldn't block us landing the change.

Do you have any idea how to deal with this kinds of error?

Thank you.

[1] https://tools.taskcluster.net/groups/OYhu-QGtRIegKtIl5lQ8CA/tasks/RvXM_a30ScqSdbxIhU40sg/runs/0/logs/public%2Flogs%2Flive.log
[2] http://w3c-test.org/html/semantics/embedded-content/media-elements/interfaces/TextTrack/activeCues.html

Comment 19

[James Graham [:jgraham]]

Yeah, I can override. Thanks for investigating.