implement cuechange event

RESOLVED FIXED in Firefox 47

Status

()

defect
RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: rillian, Assigned: ruxton, Mentored)

Tracking

(Blocks 1 bug, {dev-doc-needed})

Trunk
mozilla47
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox47 fixed, relnote-firefox 47+)

Details

Attachments

(3 attachments, 9 obsolete attachments)

2.33 KB, patch
rillian
: review+
bzbarsky
: review+
Details | Diff | Splinter Review
3.41 KB, patch
rillian
: review+
Details | Diff | Splinter Review
1.60 KB, patch
rillian
: review+
Details | Diff | Splinter Review
When the list of active cues changes as a result of running the 'time marches on' algorithm, a TextTrack object should queue a 'cuechange' event.

http://www.whatwg.org/specs/web-apps/current-work/multipage/the-video-element.html#event-media-cuechange

[Carrying over implementation work from bug 996331]
Blocks: webvtt, 882718
Depends on: 996311
Depends on: 996331
No longer depends on: 996311
Any progress on the quick implementation you mentioned, Rick?
Nope, not yet. I didn't start immediately since I saw your patch for removal. I should be able to find sometime this week to work on it though, shouldn't be too hard.
Assignee: nobody → rick.eyre
Ok. The removal patch was quick and an obvious improvement so I went ahead and did it. If we can get a small patch to dispatch the event working in nightly we can try to approve it for aurora as well.
Sounds good. I'll take a look at it tomorrow or Friday.
Work so far, haven't gotten the tests to work yet. Will work more on it Wednesday.
Attachment #8452079 - Attachment is obsolete: true
Attachment #8452078 - Flags: review?(giles)
Attachment #8452078 - Flags: review?(bzbarsky)
Attachment #8460690 - Flags: review?(giles)
Comment on attachment 8452078 [details] [diff] [review]
Part 1: Dispatch 'cuechange' event on TextTracks.

Review of attachment 8452078 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with comments addressed. Thanks for working on this!

::: content/media/TextTrack.cpp
@@ +158,5 @@
>    if (mDirty) {
>      mCuePos = 0;
>      mDirty = false;
>      mActiveCueList->RemoveAll();
> +    hasChanged = true;

Hmm. I guess you need this because you're removing all Cues and won't necessarily add one back. Is it correct to fire the event if you end up adding back exactly the same cues?

@@ +163,3 @@
>    }
>  
> +

I don't think you need this extra blank line.
Attachment #8452078 - Flags: review?(giles) → review+
Comment on attachment 8460690 [details] [diff] [review]
Part 2: Add tests for TextTrack cuechange event.

Review of attachment 8460690 [details] [diff] [review]:
-----------------------------------------------------------------

The general approach looks good. r- for issues.

::: content/media/test/test_texttrackevents.html
@@ +32,5 @@
> +      if (trackElement.readyState == 1) {
> +        return setTimeout(run_tests, 0);
> +      }
> +      is(trackElement.readyState, 2, "Track::ReadyState should be set to LOADED.");
> +      is(trackElement.track.oncuechange, null, "Track::OnCueChange should exist.");

By comparing with null, you're verifying it's not undefined, which is what you get when the event handler isn't present, and you're verifying that no handler is set, which isn't what the failure message says.

This is brittle, because:

null === undefined => false
null ==  undefined => true

And in fact, it looks like is() uses == so this won't verify the presence of the attribute.

Better to ok(foo === null) with a comment explaining the difference, or find a more clear way to verify this. I haven't figured out a better way. hasOwnProperty is close, but our implementation in in the prototype so that doesn't work. :/

@@ +42,5 @@
> +        // 12 all together. Since we're listening for the events on the TextTrack
> +        // as well as the TrackElement that should be 24 total, however, the last
> +        // exit event doesn't fire for both since it's end time is too close to
> +        // the end of the video for the fire time update event to catch it. Hence,
> +        // 24 - 2 == 22.

I'm worried this will cause intermittent failures later. Is the count reliable if you use a longer video, like vp9cake.webm? Should we be doing an extra check when playback finishes to make sure the final cues stop showing?
Attachment #8460690 - Flags: review?(giles) → review-
(In reply to Ralph Giles (:rillian) from comment #10)

Thanks for review Ralph!

> ::: content/media/TextTrack.cpp
> @@ +158,5 @@
> >    if (mDirty) {
> >      mCuePos = 0;
> >      mDirty = false;
> >      mActiveCueList->RemoveAll();
> > +    hasChanged = true;
> 
> Hmm. I guess you need this because you're removing all Cues and won't
> necessarily add one back. Is it correct to fire the event if you end up
> adding back exactly the same cues?

If that happened I don't think it would be since the track hasn't actually changed... but I'm having a hard time thinking of a case where that would happen. That would mean that the cue end time <= current play back time and its start time is also >= current play back time and that its already been added to the list, which it wouldn't be since its start time has just become valid for the current play back time.
(In reply to Ralph Giles (:rillian) from comment #11)

> ::: content/media/test/test_texttrackevents.html
> @@ +32,5 @@
> > +      if (trackElement.readyState == 1) {
> > +        return setTimeout(run_tests, 0);
> > +      }
> > +      is(trackElement.readyState, 2, "Track::ReadyState should be set to LOADED.");
> > +      is(trackElement.track.oncuechange, null, "Track::OnCueChange should exist.");
> 
> By comparing with null, you're verifying it's not undefined, which is what
> you get when the event handler isn't present, and you're verifying that no
> handler is set, which isn't what the failure message says.
> 
> This is brittle, because:
> 
> null === undefined => false
> null ==  undefined => true
> 
> And in fact, it looks like is() uses == so this won't verify the presence of
> the attribute.
> 
> Better to ok(foo === null) with a comment explaining the difference, or find
> a more clear way to verify this. I haven't figured out a better way.
> hasOwnProperty is close, but our implementation in in the prototype so that
> doesn't work. :/

Ah, good catch. I'm thinking it might be better to do okay('oncuechange' in track) as the in operator also checks the object's prototype.

> @@ +42,5 @@
> > +        // 12 all together. Since we're listening for the events on the TextTrack
> > +        // as well as the TrackElement that should be 24 total, however, the last
> > +        // exit event doesn't fire for both since it's end time is too close to
> > +        // the end of the video for the fire time update event to catch it. Hence,
> > +        // 24 - 2 == 22.
> 
> I'm worried this will cause intermittent failures later. Is the count
> reliable if you use a longer video, like vp9cake.webm? Should we be doing an
> extra check when playback finishes to make sure the final cues stop showing?

I think it would be more reliable if we used a longer video and stopped showing cues at least 1 second before the video ended. I think that would give us more than enough time to have a fire time update event trigger oncuechange. I'll update the patch to do that.
Comment on attachment 8452078 [details] [diff] [review]
Part 1: Dispatch 'cuechange' event on TextTracks.

Doesn't the spec say to queue a task to fire this event?
Flags: needinfo?(rick.eyre)
And if so, can we add a test that would catch the issue?
(In reply to Boris Zbarsky [:bz] from comment #14)

> Doesn't the spec say to queue a task to fire this event?

So it does. Rick, I asked on irc, and smaug suggested

> (new AsyncEventDispatcher())->PostDOMEvent();

There's an already-implemented example of 'queue a task to fire a simple event' here: http://dxr.mozilla.org/mozilla-central/source/content/base/src/Element.cpp#2853

As for writing a test, I'm not sure how to do that. Apparently DispatchTrustedEvent() will call any registered handlers _before_ returning from whatever triggered oncuechange, so you could register one handler before and one after and check that both get it, but you'd have to find some reliable way to make the event fire inside a call.
(In reply to Boris Zbarsky [:bz] from comment #14)

> Doesn't the spec say to queue a task to fire this event?

Yup, you're right.

(In reply to Ralph Giles (:rillian) from comment #16)

> So it does. Rick, I asked on irc, and smaug suggested
> 
> > (new AsyncEventDispatcher())->PostDOMEvent();
> 
> There's an already-implemented example of 'queue a task to fire a simple
> event' here:
> http://dxr.mozilla.org/mozilla-central/source/content/base/src/Element.
> cpp#2853

Great, thanks for the help. I'll update the patch to do it that way.

> As for writing a test, I'm not sure how to do that. Apparently
> DispatchTrustedEvent() will call any registered handlers _before_ returning
> from whatever triggered oncuechange, so you could register one handler
> before and one after and check that both get it, but you'd have to find some
> reliable way to make the event fire inside a call.

Yeah, that might be a bit tricky. I'll see if I can get it done.
Flags: needinfo?(rick.eyre)
Comment on attachment 8452078 [details] [diff] [review]
Part 1: Dispatch 'cuechange' event on TextTracks.

OK.  r=me on the rest, but I'd like to see the patch updated with the event async.
Attachment #8452078 - Flags: review?(bzbarsky) → review+
Thanks for review Boris.

Carrying forward r=rillian,bz.

I've updated the event to be on the TextTrack to be fired asynchronously using the method
that Ralph suggested. I've also removed the setting of 'hasChanged' to true when the TextTrack
became dirty. This is because if the TextTrack gets dirty it's going to either remove cues
or add them anyways and set the hasChanged flag anyways.
Attachment #8452078 - Attachment is obsolete: true
I've updated the test to use vp9cake.webm to give the events a little more time. I've
also started using a different VTT file that is more sequential so we can be sure that
the events will be regular and will have a single event for each enter/exit on a cue.

I'm not quite sure how to test the async here. Or even if it's possible. Can you explain
a bit more about the approach you were thinking Ralph?
Attachment #8460690 - Attachment is obsolete: true
Attachment #8462303 - Flags: review?(giles)
Sorry(In reply to Rick Eyre (:reyre) from comment #19)

> that Ralph suggested. I've also removed the setting of 'hasChanged' to true
> when the TextTrack
> became dirty. This is because if the TextTrack gets dirty it's going to
> either remove cues
> or add them anyways and set the hasChanged flag anyways.

I don't think this is correct. I was worried about removing all cues, then adding the same ones back firing cuechanged. The more common case is removing them all and adding different ones, but you should still fire cuechanged if you remove them all and don't end up adding any.

Sorry for the review delay on the test patch. I'll get to it tomorrow.
Comment on attachment 8462303 [details] [diff] [review]
Part 2: Add tests for TextTrack cuechange event r=rillian

Review of attachment 8462303 [details] [diff] [review]:
-----------------------------------------------------------------

This is better, but I'd like to see it again with comments addressed.

Re testing the async dispatch, my understanding is if you do:

> var e = new Element();
> var count = 0;
> e.addEventListener('onsomevent', function foo() { ++count; });
> e.doCall();
> e.addEventListner('onsomeevent', function bar() { ++count; });

then if doCall() dispatches 'someevent' synchronously, count will be 1, and if it dispatches it asynchronously count will be 2.

So this gives you a way to test when the event is dispatched: if it's inside doCall() only foo() will be called. If doCall() instead queues a task, it won't be dispatched until the event loop spins again, e.g. after the script finishes executing, and at that time foo() and bar() will both be registered.

However, I can't think of any equivalent to doCall() for TextTracks which wouldn't already by async. That is, seeking updates the playback position asynchronously; addCue is synchronous, but it looks like cuechange only fires inside 'time marches on' which is triggered asynchronously. So any test like I've just described would always pass, because there's no way to trigger cuechange that doesn't already queue a task long before it gets to actually updating the active cue list.

::: content/media/test/test_texttrackevents.html
@@ +14,5 @@
> +<pre id="test">
> +<script class="testbody" type="text/javascript">
> +SimpleTest.waitForExplicitFinish();
> +SpecialPowers.pushPrefEnv({"set": [["media.webvtt.enabled", true],
> +                                   ["media.webvtt.regions.enabled", true]]},

Don't need region support. :)

@@ +42,5 @@
> +        // for when it is activated/deactivated (6 events total), and we are
> +        // listening on both the track and track element, 12 total.
> +        if (++count === 12) {
> +          ok(true, "cuechange should have been fired 12 times.");
> +          SimpleTest.finish();

This is better, but won't detect if extra events are fired. Increment count here, and attach a separate 'onended' listener to video which verifies the count is 12 and finishes the test. That's also less likely to time out if fewer than 12 events fire.
Attachment #8462303 - Flags: review?(giles) → review-
While you're at it, you should also check the onload event, which isn't raised either.
Example: http://html5videoguide.net/NEW/CH4/code_c4_8.html
Works in Chrome 36.
Spec: http://www.w3.org/html/wg/drafts/html/master/embedded-content.html#event-media-cuechange
Sorry, the example has moved to http://html5videoguide.net/NEW/CH4/code_c4_9.html
Sorry Silvia, I don't see an 'onload' event in the link you posted?
Sorry for the _super_ late reply Ralph. Life got busy for a while and I didn't have anytime to finish this patch.

(In reply to Ralph Giles (:rillian) from comment #21)

> I don't think this is correct. I was worried about removing all cues, then
> adding the same ones back firing cuechanged. The more common case is
> removing them all and adding different ones, but you should still fire
> cuechanged if you remove them all and don't end up adding any.

I'm not quite sure what the best approach here would be to solve the case where we remove all the cues and then add the same ones back. My first idea would be to just track what cues were active at time of removal and then check that against the ones that are added back. It might be the simplest approach. Do you have any suggestions?
http://html5videoguide.net/NEW/CH4/code_c4_10.html is the right demo now.
track.onload fires in Chrome.
But I can't find it in the spec any more...
(In reply to Rick Eyre (:reyre) from comment #26)

> I'm not quite sure what the best approach here would be to solve the case
> where we remove all the cues and then add the same ones back. My first idea
> would be to just track what cues were active at time of removal and then
> check that against the ones that are added back. It might be the simplest
> approach. Do you have any suggestions?

Looking more at this I think I'm just going to refactor the UpdateActiveCueList function to make it easier to keep track of things like this. There are some improvements we can make here I think...
Component: Audio/Video → Audio/Video: Playback
I just wanted to check the status of this issue as it is still not resolved. Even as of Firefox 42 Beta I can't get a cuechange event. As MSE is released "to the wild" in FF42, adaptive streaming player's, like bitmovin's bitdash player (http://www.dash-player.com/), the HTML5 playback will be used. But not having cuechange events is kind of a blocker for us as we use cues for subtitles as well for other information. As you can see at http://www.dash-player.com/demo/multi-language-and-multi-audio/ in FF42 (with MediaSource enabled for everyone), no subtitles are shown due to cuechange event not firing.
It definitely won't get fixed in Firefox 42. We don't have any work on text tracks scheduled at this point in time.
(In reply to Anthony Jones (:kentuckyfriedtakahe, :k17e) from comment #30)
> It definitely won't get fixed in Firefox 42. We don't have any work on text
> tracks scheduled at this point in time.

That's a shame, using capability from this and/or bug 996333 / bug 865395 would be the native API way to manually workaround bug 992664.
Ralph - any thoughts?
Flags: needinfo?(giles)
I agree it's a shame. These remaining event bugs aren't too hard, but we need someone to work on them.
Assignee: rick.eyre → nobody
Mentor: giles
Flags: needinfo?(giles)
Aligns reyre's Part 1 with current development
Aligns reyre's previous work with current development and updates the test slightly.
Ok so I've gone and aligned the patches work with the current master on github.  I've also adjusted the test slightly to individually test they're firing as 6, rather than as a group of 12 and only pass upon end. 

I feel like there should be some kind of "timing out" so if the ended event doesn't fire off, the test will fail.  I was just going to base that on the VTT's length, but that makes the test brittle, so I avoided that.

I have some another test and commit that hang off this work to make the cuechange fire for audio tags properly too.  Right now, this only makes video tags work.
accidentally uploaded the wrong patch.
Attachment #8714160 - Attachment is obsolete: true
uploaded the wrong patch.
Attachment #8714161 - Attachment is obsolete: true
Ralph - are you following this?
Flags: needinfo?(giles)
Thanks for the patched, ruxton. Sorry for not responding yesterday. Let me try them out.
Flags: needinfo?(giles)
Attachment #8462302 - Attachment is obsolete: true
Attachment #8462303 - Attachment is obsolete: true
Comment on attachment 8714165 [details] [diff] [review]
0002-Bug-1033144-Part-2-Add-tests-for-TextTrack-cuechange.patch

Review of attachment 8714165 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the update. Looks resonable to move this forward r=me with comments addressed.

(That means update the patch or explain why review suggestions are wrong. :)

Not having a timeout is unfortunate. You do have a listener on 'ended', so it should terminate as long as playback completes, which is probably good enough. The test harness itself has a timeout which will trigger eventually, but it's minutes away and relying on it slows down the test run. This file is seekable, so after metadataloaded is available, video.duration should be available, so you could set a timeout based on that, but you have to worry about it racing with the successful end of the test.

Dan's been doing some work to clean up test flakiness lately. Dan, do you have any commets on this patch?

::: dom/media/test/test_texttrackevents_video.html
@@ +14,5 @@
> +<pre id="test">
> +<script class="testbody" type="text/javascript">
> +SimpleTest.waitForExplicitFinish();
> +SpecialPowers.pushPrefEnv({"set": [["media.webvtt.enabled", true],
> +                                   ["media.webvtt.regions.enabled", true]]},

This test doesn't use regions, which we're unlikely to implement anyway. No reason to keep propagating the preference for it. So just:

  SpecialPowers.pushPrefEnv({"set": [["media.webvtt.enabled", true]]},

should work fine.

@@ +31,5 @@
> +    var trackElementCueChangeCount = 0;
> +    var trackCueChangeCount = 0;
> +
> +    video.addEventListener("loadedmetadata", function run_tests() {
> +      // Re-que run_tests() at the end of the event loop until the track

// Re-queue

@@ +48,5 @@
> +        ++trackCueChangeCount;
> +      }
> +
> +      trackElement.track.oncuechange = countTrackCueChange;
> +      trackElement.addEventListener("cuechange", countCueChange);

FWIW, anonymous functions would be fine here. Up to you though.

@@ +57,5 @@
> +    video.addEventListener('ended', function() {
> +      // Should be fired 6 times, as there are 3 cues, with a change event
> +      // for when it is activated/deactivated (6 events total)
> +      ok((trackElementCueChangeCount === 6),"TrackElement should fire cue change 6 times.");
> +      ok(trackCueChangeCount === 6,"TrackElement.track should fire cue change 6 times.");

Mochitest has an is() method which is easier to read than the boolean. e.g.

  is(trackCueChangeCount, 6, "TrackElement.track should fire cue change 6 times.");

See https://developer.mozilla.org/en-US/docs/Mochitest#Test_functions for a full list. It also now claims to use `===` contrary to my comment #11. But `==` works for integers, I think.

Either way, please put a space after the comma separating arguments.
Attachment #8714165 - Flags: review+
Attachment #8714165 - Flags: feedback?(dglastonbury)
Attachment #8714164 - Flags: review?(giles)
Comment on attachment 8714165 [details] [diff] [review]
0002-Bug-1033144-Part-2-Add-tests-for-TextTrack-cuechange.patch

Review of attachment 8714165 [details] [diff] [review]:
-----------------------------------------------------------------

Ralph, I've been looking at test flakiness caused by my own changes to C++ code.

LGTM.
Attachment #8714165 - Flags: feedback?(dglastonbury) → feedback+
Thanks for taking a look.
Comment on attachment 8714164 [details] [diff] [review]
0001-Bug-1033144-Part-1-Dispatch-cuechange-event-on-TextT.patch

Review of attachment 8714164 [details] [diff] [review]:
-----------------------------------------------------------------

Looks ok to me. bz, you said you wanted to check the async dispatch again.
Attachment #8714164 - Flags: review?(giles)
Attachment #8714164 - Flags: review?(bzbarsky)
Attachment #8714164 - Flags: review+
Comment on attachment 8714164 [details] [diff] [review]
0001-Bug-1033144-Part-1-Dispatch-cuechange-event-on-TextT.patch

r=me
Attachment #8714164 - Flags: review?(bzbarsky) → review+
Need to un-exfail the corresponding web platform test.

TEST-UNEXPECTED-PASS | /html/dom/interfaces.html | TextTrack interface: attribute oncuechange - expected FAIL

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8521df7b7e3c&selectedJob=16454881
Attachment updates the web-platform-tests to expect PASS for oncuechange on TextTrack r=rillian
Comment on attachment 8719644 [details] [diff] [review]
0003-1033144-Part-3-Update-web-platform-tests-to-expect-P.patch

Review of attachment 8719644 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM, but let's wait for confirmation from the try server builds.
Attachment #8719644 - Flags: review+
Comment on attachment 8719630 [details] [diff] [review]
0002-Bug-1033144-Part-2-Add-tests-for-TextTrack-cuechange.patch

Review of attachment 8719630 [details] [diff] [review]:
-----------------------------------------------------------------

r=me Thanks for following up, and sorry for the long delay.

I worry having waitForExplicitFinish() outside the pushPrefEnv might cause problems on e10s, but we have several other tests like that, so we can fix it separately if it's an issue.
Attachment #8719630 - Flags: review+
Green on try. I'll fix up the commit messages to conform to style and land. Thanks for contributing!

NB if you do more patches in the future, setting an explicity r? flag on the the attachment is a good way to make sure it gets attention in bugzilla. We don't track r= or r? in comments of the commit message like github does.
Rebase over yesterday's additions which moved the changes outside the fuzz window. Carrying r=me.
Attachment #8719644 - Attachment is obsolete: true
Attachment #8725781 - Flags: review+
I've pushed your changes to the inbound queue. They'll go through a full set of tests there and if all goes well, will be merged to the central repo later today and appear in a subsequent nightly build.
Assignee: nobody → ruxton
Blocks: 1253905
Ralph, could you please fill out the relnote template with suggested wording? That would be a lot of help. Thanks!
Flags: needinfo?(giles)
Sorry, missed the template when I set the flag.

Release Note Request (optional, but appreciated)
[Why is this notable]: This feature is helpful to authors writing media players, and has been a deficiency of our webvtt support.
[Suggested wording]: Developer: `cuechange` events are now available on TextTrack objects.
[Links (documentation, blog post, etc)]: https://html.spec.whatwg.org/multipage/embedded-content.html#event-media-cuechange
Flags: needinfo?(giles)
Thanks Ralph! Added to Fx47 (Aurora) release notes.
You need to log in before you can comment on or make changes to this bug.