1278151 - [webvtt] fix html/semantics/embedded-content/media-elements/interfaces/TextTrack/cues.html

Status: RESOLVED DUPLICATE of bug 871747

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

Description

[Benjamin Chen [:bechen]]

https://html.spec.whatwg.org/multipage/embedded-content.html#text-track-cue-order

Our implementation |CompareCuesByTime| is wrong about the endtime.

Comment 1

[Benjamin Chen [:bechen]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4fb52b05cf75

Comment 2

[Benjamin Chen [:bechen]]

Attachment: Bug 1278151 - Fix CompareCuesByTime for the web-platform/meta/html/semantics/embedded-content/media-elements/interfaces/TextTrack/cues.html.

Review commit: https://reviewboard.mozilla.org/r/58456/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/58456/

Comment 3

[Benjamin Chen [:bechen]]

Attachment: Bug 1278151 - Fix web-platform/meta/html/semantics/embedded-content/media-elements/interfaces/TextTrack/cues.html.

Review commit: https://reviewboard.mozilla.org/r/58458/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/58458/

Comment 4

[Benjamin Chen [:bechen]]

https://reviewboard.mozilla.org/r/58456/#review55322

::: dom/html/HTMLTrackElement.cpp:184
(Diff revision 1)
> +  if (mTrack) {
> +    mMediaParent->AddTextTrack(mTrack);
> +  }

Add the texttrack into MediaElement after the TrackElement bind into the MediElement.

::: dom/html/HTMLTrackElement.cpp
(Diff revision 1)
> -  if (!aDocument) {
> -    return NS_OK;
> -  }
> -

The TrackElement should work if there is a MediaElement but outside the document.

Comment 5

[Benjamin Chen [:bechen]]

https://reviewboard.mozilla.org/r/58458/#review55328

::: testing/web-platform/tests/html/semantics/embedded-content/media-elements/interfaces/TextTrack/cues.html:78
(Diff revision 1)
>  async_test(function(){
>      var video = document.createElement('video');
>      var t1 = video.addTextTrack('subtitles');
>      var t1_cues = t1.cues;
> -    t1.mode = 'showing';
> +    t1.mode = 'hidden';
>      var track = document.createElement('track');
>      track['default'] = true;
>      video.appendChild(track); // queues a task to "honor user preferences...", media element event task source
>      var t2 = track.track;
>      assert_equals(t2.cues, null, 't2.cues should be null');
>      // We need to wait until the "honor user preferences..." steps have run so we invoke play()
>      // which queues an event with the same task source.
>      video.onplay = this.step_func(function(){
>          assert_equals(t2.cues, t2.cues, 't2.cues should return same object');
>          assert_not_equals(t1.cues, t2.cues, 't1.cues and t2.cues should be different objects');
>          assert_not_equals(t2.cues, null, 't2.cues should not be null');
>          assert_true(t2.cues instanceof TextTrackCueList, 't2.cues instanceof TextTrackCueList');
>          assert_equals(t2.cues.length, 0, 't2.cues should have length 0');
>          this.done();
>      });
>      video.play(); // queues a task to fire 'play', media element event task source

I think the testcase is wrong.
https://html.spec.whatwg.org/multipage/embedded-content.html#perform-automatic-text-track-selection

In step 3, it says if there is a showing TextTrack, we don't need to do auto-select.
In testcase, t1 is "showing", so even the t2 set the default attribute, the t2 still remains at "disabled" mode.

Comment 7

[Ralph Giles (:rillian)]

Comment on attachment 8761118 [details]
Bug 1278151 - Fix CompareCuesByTime for the web-platform/meta/html/semantics/embedded-content/media-elements/interfaces/TextTrack/cues.html.

https://reviewboard.mozilla.org/r/58456/#review56172

Comment 8

[Ralph Giles (:rillian)]

Comment on attachment 8761119 [details]
Bug 1278151 - Fix web-platform/meta/html/semantics/embedded-content/media-elements/interfaces/TextTrack/cues.html.

https://reviewboard.mozilla.org/r/58458/#review56174

Looks like you're right.

Comment 9

[Benjamin Chen [:bechen]]

Comment on attachment 8761118 [details]
Bug 1278151 - Fix CompareCuesByTime for the web-platform/meta/html/semantics/embedded-content/media-elements/interfaces/TextTrack/cues.html.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58456/diff/1-2/

Comment 10

[Benjamin Chen [:bechen]]

Comment on attachment 8761119 [details]
Bug 1278151 - Fix web-platform/meta/html/semantics/embedded-content/media-elements/interfaces/TextTrack/cues.html.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58458/diff/1-2/

Comment 11

[Benjamin Chen [:bechen]]

https://reviewboard.mozilla.org/r/58456/#review55322

> The TrackElement should work if there is a MediaElement but outside the document.

Remove this cause test_texttrak.html fail.
When the TrackElement append to the MediaElement, there are two cases here. 1. If the MediaElement append to the domtree, we will receive 2 bintotree function calls with null and non-null |aDocument|. 2 If the MediaElement is not attached to the domtree (cues.html), we only receive null |aDocument|.
The problem is that if the aDocument is null, the RunInStableState in MediaElement won't do anythng. Once the RunInStableState doesn't work, the |mMediaParent->NotifyAddedSource();, QueueSelectResourceTask, QueueLoadFromSourceTask| doesn't work neither.
I feel risky to make the change here, so I will not enable the cues.html and restore the |aDocument| check. Land other changes and leave this bug open.

Comment 12

[Benjamin Chen [:bechen]]

Comment on attachment 8761118 [details]
Bug 1278151 - Fix CompareCuesByTime for the web-platform/meta/html/semantics/embedded-content/media-elements/interfaces/TextTrack/cues.html.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58456/diff/2-3/

Comment 13

[Benjamin Chen [:bechen]]

Comment on attachment 8761119 [details]
Bug 1278151 - Fix web-platform/meta/html/semantics/embedded-content/media-elements/interfaces/TextTrack/cues.html.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58458/diff/2-3/

Comment 14

[Benjamin Chen [:bechen]]

Comment on attachment 8761118 [details]
Bug 1278151 - Fix CompareCuesByTime for the web-platform/meta/html/semantics/embedded-content/media-elements/interfaces/TextTrack/cues.html.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58456/diff/3-4/

Comment 15

[Benjamin Chen [:bechen]]

Comment on attachment 8761119 [details]
Bug 1278151 - Fix web-platform/meta/html/semantics/embedded-content/media-elements/interfaces/TextTrack/cues.html.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58458/diff/3-4/

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7fb0f8f1d63d
https://hg.mozilla.org/integration/mozilla-inbound/rev/3a0281977c76

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7fb0f8f1d63d
Fix CompareCuesByTime for the web-platform/meta/html/semantics/embedded-content/media-elements/interfaces/TextTrack/cues.html. r=rillian
https://hg.mozilla.org/integration/mozilla-inbound/rev/3a0281977c76
Fix web-platform/meta/html/semantics/embedded-content/media-elements/interfaces/TextTrack/cues.html. r=rillian

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7fb0f8f1d63d
https://hg.mozilla.org/mozilla-central/rev/3a0281977c76

Comment 19

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Mass change P2 -> P3