Closed Bug 882677 Opened 7 years ago Closed 6 years ago

[webvtt] Implement the 'sourcing out of band text tracks' algorithm

Categories

(Core :: Audio/Video, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla31

People

(Reporter: reyre, Assigned: reyre)

References

()

Details

Attachments

(5 files, 9 obsolete files)

23.39 KB, patch
cpearce
: review+
Details | Diff | Splinter Review
1.29 KB, patch
Details | Diff | Splinter Review
1.28 KB, patch
Details | Diff | Splinter Review
6.07 KB, patch
Details | Diff | Splinter Review
5.27 KB, patch
Details | Diff | Splinter Review
These are for text tracks that come from the 'track' element.
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → WONTFIX
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---
Assignee: nobody → rick.eyre
Parts of this have been done in various bugs.

Text track selection: Bug 977302
Remove and add track events: Bug 893309
TextTrack Mode being set correctly: Bug 983207

The main thing left to do is implement the 'track processing model'. Most of this is taken care of by the WebVTTListener class. We just need to take care of firing 'load' and 'error' events at the TrackElement at the appropriate places.
We also need to make sure that we remove all TextTrackCues from a TextTrack if its source attribute changes.
Attachment #8391470 - Attachment is obsolete: true
Attachment #8392887 - Flags: review?(cpearce)
Attachment #8392891 - Flags: review?(cpearce)
Comment on attachment 8392887 [details] [diff] [review]
Part 1 v2: Dispatch loaded and error events to the HTMLTrackElement r=cpearce

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

::: content/html/content/src/HTMLTrackElement.cpp
@@ +329,5 @@
> +{
> +  if (mTrack) {
> +    switch (aReadyState) {
> +      case TextTrackReadyState::Loaded:
> +        DispatchTrustedEvent(NS_LITERAL_STRING("loaded"));

I think you should queue a task to dispatch these events asynchronously. The spec says they can be dispatched synchronously in a synchronous section (i.e. when there are no other events running), which I think is equivalent to dispatching a task to dispatch the event.

I think this is a good idea because there could be other events earlier in the event queue that need to arrive before this one.
Attachment #8392887 - Flags: review?(cpearce) → review-
Comment on attachment 8392888 [details] [diff] [review]
Part 2 v1: Handle ParsingErrors from vtt.js correctly r=cpearce.

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

Almost there. I think you need to be stricter about error handling.

::: content/media/WebVTTListener.cpp
@@ +183,5 @@
> +{
> +  // We only care about files that have a bad WebVTT file signature right now
> +  // as that means the file failed to load.
> +  if (errorCode == ErrorCodes::BadSignature) {
> +    mElement->SetReadyState(TextTrackReadyState::Error);

It seems like WebVTTListener::OnParsingError() should only be called when there's a fatal parsing error, and if there's a fatal parsing error we should probably set the ready state to error.

That is, I think you should always set the readystate to error, irrespective of the input errorcode.

::: content/media/WebVTTListener.h
@@ +47,5 @@
>  
>  private:
> +  // List of ErrorCodes returned from the WebVTTParser that we care about.
> +  enum ErrorCodes {
> +    BadSignature = 0

What about BadTimeStamp? You use that in vtt.jsm. Do you also need a generic/unknown error code?

::: content/media/webvtt/vtt.jsm
@@ +1167,5 @@
> +    // If the error is a ParsingError then catch it and report it to the
> +    // consumer if possible, otherwise, throw it.
> +    reportOrThrowError: function(e) {
> +      if (e instanceof ParsingError) {
> +        this.onparsingerror && this.onparsingerror(e);

This code does not throw the error if it can't be processed, but the comment implies that it should be. i.e. if (e instanceof ParsingError && !this.onparsingerror), the error will not be thrown, right? Shouldn't it be?
Attachment #8392888 - Flags: review?(cpearce) → review-
Attachment #8392889 - Flags: review?(cpearce) → review+
Attachment #8392890 - Flags: review?(cpearce) → review+
Comment on attachment 8392891 [details] [diff] [review]
Part 5 v1: Add track element tests r=cpearce.

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

::: content/media/test/test_trackelementevent.html
@@ +37,5 @@
> +      ok(true, "A loaded or error event should have happened.");
> +      events++ && events == 3 && SimpleTest.finish();
> +    }
> +
> +    trackOne.addEventListener("loaded", countEvent);

Add "error" and "loaded" event listeners for all tracks; you should not receive both right?
Attachment #8392891 - Flags: review?(cpearce) → review+
(In reply to Chris Pearce (:cpearce) from comment #10)

> It seems like WebVTTListener::OnParsingError() should only be called when
> there's a fatal parsing error, and if there's a fatal parsing error we
> should probably set the ready state to error.
> 
> That is, I think you should always set the readystate to error, irrespective
> of the input errorcode.

We can't do that, at least not the way it's structured right now. ParsingError is a way to differentiate between the exceptions that the Parser raises to manage its internal state and actual exceptions that happened due to bad code. So parsing errors are errors that don't actually stop the parser, but just cause it to switch to a new state.

The exception for that is BadSignature, which stops the Parser because it hasn't found a valid WebVTT file signature. I'm not sure if we can call a BadSignature error fatal because the parser has parsed it correctly, it's just that it's not a VTT file.

The only ParsingError that Gecko would care about currently is the BadSignature one because the "sourcing out-of-band text tracks" algorithm calls for setting the track's state to 'failed to load' if the file in question has been found not to be a valid VTT file.

> ::: content/media/webvtt/vtt.jsm
> @@ +1167,5 @@
> > +    // If the error is a ParsingError then catch it and report it to the
> > +    // consumer if possible, otherwise, throw it.
> > +    reportOrThrowError: function(e) {
> > +      if (e instanceof ParsingError) {
> > +        this.onparsingerror && this.onparsingerror(e);
> 
> This code does not throw the error if it can't be processed, but the comment
> implies that it should be. i.e. if (e instanceof ParsingError &&
> !this.onparsingerror), the error will not be thrown, right? Shouldn't it be?

Yeah, the wording is a bit confusing here. I can update that. What I intended to say is what the code does. If it's parsing error then report it if we can, if it's not a parsing error then throw it like normal.
(In reply to Chris Pearce (:cpearce) from comment #11)
> ::: content/media/test/test_trackelementevent.html
> @@ +37,5 @@
> > +      ok(true, "A loaded or error event should have happened.");
> > +      events++ && events == 3 && SimpleTest.finish();
> > +    }
> > +
> > +    trackOne.addEventListener("loaded", countEvent);
> 
> Add "error" and "loaded" event listeners for all tracks; you should not
> receive both right?

Ah, good idea. I think it would be good as well to create another function called something like 'shouldNotBeCalled' for the events that shouldn't be called. That way if the incorrect events are called, but the total called events are still 3, we don't get a false positive.
Updated the dispatching of the events so that they're dispatched asynchronously. Rebased to current m-c as well.
Attachment #8392887 - Attachment is obsolete: true
Attachment #8399551 - Flags: review?(cpearce)
- I've updated the comment in vtt.js that was confusing. To the wording in comment 12.
- Please see comment 12 for an explanation of why we can't fail on every error code.
- I've also had to update the update-webvtt.js script to reflect some changes in the architecture of vtt.js.
Attachment #8392888 - Attachment is obsolete: true
Attachment #8399554 - Flags: review?(cpearce)
Rebasing against m-c.

Carrying forward r=cpearce.
Attachment #8392889 - Attachment is obsolete: true
Rebasing against m-c. Carrying forward r=cpearce.
Attachment #8392890 - Attachment is obsolete: true
Attachment #8399557 - Attachment is patch: true
Attachment #8399557 - Attachment mime type: message/rfc822 → text/plain
test_trackelementevent.html now adds event listeners for events that shouldn't be fired.

Carrying forward r=cpearce.
Attachment #8392891 - Attachment is obsolete: true
Comment on attachment 8399551 [details] [diff] [review]
Part 1 v3: Dispatch loaded and error events to the HTMLTrackElement r=cpearce

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

::: content/html/content/src/HTMLTrackElement.cpp
@@ +362,5 @@
> +
> +void
> +HTMLTrackElement::DispatchTrackRunnable(const nsAString& aEventName)
> +{
> +  nsCOMPtr<nsIRunnable> trackRunner = new TrackRunnable(this, aEventName);

Can you instead use:

nsCOMPtr<nsIRunnable> trackRunner =
NS_NewRunnableMethodWithArg<nsString>(this,
                                      &HTMLTrackElement::DispatchTrustedEvent,
                                      aEventName);

Then you can delete TrackRunnable.
Attachment #8399551 - Flags: review?(cpearce) → review+
Attachment #8399554 - Flags: review?(cpearce) → review+
Thanks for review Chris.

(In reply to Chris Pearce (:cpearce) from comment #19)

> Can you instead use:
> 
> nsCOMPtr<nsIRunnable> trackRunner =
> NS_NewRunnableMethodWithArg<nsString>(this,
>                                      
> &HTMLTrackElement::DispatchTrustedEvent,
>                                       aEventName);
> 
> Then you can delete TrackRunnable.

I was looking for something simpler then making a Runnable subclass. This is great.
Updated to use NS_NewRunnableMethodWithArg instead of the TrackRunner class.

Carrying forward r=cpearce.
Attachment #8399551 - Attachment is obsolete: true
We were failing on TextTrack::SetReadyState trying to compare uint <= 0, which is always true. I've updated it so that we don't check the low range, because it will always be 0, and only check the high range.

Carrying forward r=cpearce.
Attachment #8399605 - Attachment is obsolete: true
Were failing because we hadn't flipped the regions pref on, so loading the track would fail and hence the test would fail.
Attachment #8399558 - Attachment is obsolete: true
I have tested this and it doesn't seem to work in Aurora 32.0a2 (2014-07-05), just looking from the code shown in this issue, I see that you are dispatching a 'loaded' event. But you should set the readyState to the 'loaded' state and then dispatch a 'load' event (not a loaded) one.
(In reply to Alexander Farkas from comment #27)
> I have tested this and it doesn't seem to work in Aurora 32.0a2
> (2014-07-05), just looking from the code shown in this issue, I see that you
> are dispatching a 'loaded' event. But you should set the readyState to the
> 'loaded' state and then dispatch a 'load' event (not a loaded) one.

Thanks for the correction Alex, I'll open a new bug for this.
Follow up bug filed as 1035505.
You need to log in before you can comment on or make changes to this bug.