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

RESOLVED FIXED in Firefox 67

Status

()

defect
P2
normal
RESOLVED FIXED
5 months ago
a month ago

People

(Reporter: laszlo.bialis, Assigned: alwu)

Tracking

(Blocks 1 bug)

Trunk
mozilla67
Points:
---

Firefox Tracking Flags

(firefox65 wontfix, firefox66 wontfix, firefox67 fixed)

Details

(URL)

Attachments

(9 attachments, 1 obsolete attachment)

(Reporter)

Description

5 months ago
[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.
Priority: -- → P3
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
Flags: needinfo?(cam)
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.
Flags: needinfo?(cam)
(Assignee)

Updated

2 months ago
Blocks: webvtt
Summary: Shaka player demo, subtitles still showing after video change → [webvtt] Shaka player demo, subtitles still showing after video change
(Assignee)

Comment 3

2 months ago

Will take a look at this one.

Assignee: nobody → alwu
(Assignee)

Updated

2 months ago
Summary: [webvtt] Shaka player demo, subtitles still showing after video change → [webvtt] cue should be removed from video's rendering area after it has been removed from text track
(Assignee)

Updated

2 months ago
Priority: P3 → P2
(Assignee)

Comment 4

2 months ago

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

(Assignee)

Comment 5

2 months ago

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()'.

(Assignee)

Comment 6

2 months ago

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.

Attachment #9046569 - Attachment description: Bug 1509446 - part1 : reset active cues before update cues list. → Bug 1509446 - part1 : update active cue before returning it.
(Assignee)

Comment 7

2 months ago

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.

(Assignee)

Updated

2 months ago
Depends on: 1531863
(Assignee)

Updated

2 months ago
No longer depends on: 1531863
Attachment #9046569 - Attachment is obsolete: true
(Assignee)

Comment 8

2 months ago

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

(Assignee)

Comment 9

2 months ago

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

(Assignee)

Comment 10

2 months ago

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

(Assignee)

Comment 11

2 months ago

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.

(Assignee)

Comment 12

2 months ago

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

Attachment #9046570 - Attachment description: Bug 1509446 - part2 : update cues display without checking whether cue is active or not. → Bug 1509446 - part6 : update cues display without checking whether cue is active or not.
Attachment #9046571 - Attachment description: Bug 1509446 - part3 : add a reftest. → Bug 1509446 - part7 : add a reftest.
(Assignee)

Comment 13

2 months ago

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

2 months ago
Pushed by alwu@mozilla.com:
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
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/15789 for changes under testing/web-platform/tests
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)
(Assignee)

Comment 18

a month ago

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

Flags: needinfo?(james)

Yeah, I can override. Thanks for investigating.

Flags: needinfo?(james)
You need to log in before you can comment on or make changes to this bug.