880094 - [webvtt] Run needed tasks asynchronously in HTMLTrackElement

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Rick Eyre (:reyre)]

In bug 833385 comment 48 bz commented that we should be implementing certain parts of Loading HTMLTrackElement asynchronously. 

Most notably:

> The spec says this too; see the "stable state" bits.
> 
> Note that we have some infrastructure for the "stable state" stuff over in     > nsHTMLMediaElement.  We should consider hoisting it up to somehere more shared  > and using it here.  A followup is OK, as long as you stop doing things         > completely sync before landing.

There are a few places that we would need to do this. Such as AsyncAwaitStableState:

https://mxr.mozilla.org/mozilla-central/source/content/html/content/src/HTMLMediaElement.cpp#685

As to what tasks we would need to do this for we should follow the spec for that. Here is one of those such occurrences http://www.whatwg.org/specs/web-apps/current-work/multipage/the-video-element.html#concept-media-load-algorithm

Comment 1

[Rick Eyre (:reyre)]

Chris, if we want to move the AsyncAwaitStableState stuff out of the media element to a place that both the HTMLMediaElement and the HTMLTrackElement can utilize it would nsGenericHTMLElement be the best place?

Also, I'm wondering if moving this code would be a bit overkill as we really only need it when calling LoadResource() on an HTMLTrackElement and in the bits of this algorithm here:
http://www.whatwg.org/specs/web-apps/current-work/multipage/the-video-element.html#start-the-track-processing-model . Also, that algorithm should be being run by a TextTrack object most likely and it has no access to nsGenericHTMLElement so we would probably have to add infrastructure there for it.

Can you please advise on what might be the best way to go about doing this?

Comment 2

[Chris Pearce [:cpearce (Not reading bugmail)]]

Thanks for asking! The public interface for sync sections is already exposed as nsIAppShell::RunInStableState(nsIRunnable).

But the important thing here is to preserve the fact that we want to be able to cancel pending sync sections when a new media load is started, i.e. we need to be running nsMediaEvents, and nsMediaEvent must check its IsCancelled() function before we run the sync section.

This means if we start a new load, we won't run sync sections from the previous load.

So I think that you should expose a public method HTMLMediaElement::RunInStableState(nsIRunnable*). When you call this method, you pass it an NS_NewRunnableMethod. We'll use this internally in media element too.  e.g., HTMLMediaElement::QueueLoadFromSourceTask() would call:

RunInStableState(NS_NewRunnableMethod(this, &HTMLMediaElement::LoadFromSourceChildren));

CallInStableState() then basically will do what the static AsyncAwaitStableState() function does, except we must change nsSyncSection to take an nsIRunnable parameter instead of a SyncSectionFn parameter. That way we preserve the cancellability, and we share code with the track element. And by passing in an NS_NewRunnableMethod() when we call this from the track element, we ensure that the track element stays alive while this runs, as the runnable holds a reference count to the object being called.

Make sense?

Comment 3

[Rick Eyre (:reyre)]

Makes perfect sense Chris. Thanks!

This is a lot simpler then what I was thinking we would need.

Comment 4

[Rick Eyre (:reyre)]

If we're changing nsSyncFunc to take an nsIRunnable how will we extract the HTMLMediaElement from it as we need to pass this to the nsMediaEvent?

We could cast it to an nsRunnableMethodImp as that is what NS_NewRunnableMethod returns and have access to it that way, but that's kind of breaking the abstraction.

Also, if we want to call this from HTMLTrackElement we need to pass in HTMLTrackElement as NS_NewRunnableMethod's first argument so we wouldn't be able to extract the HTMLMediaElement like this.

What about if we passed in an NS_NewRunnableMethod and the HTMLMediaElement separately to RunInStableState? So RunInStableState(nsIRunnable*, HTMLMediaElement*)?

Comment 5

[Chris Pearce [:cpearce (Not reading bugmail)]]

(In reply to Rick Eyre (:reyre) from comment #4)
> If we're changing nsSyncFunc to take an nsIRunnable how will we extract the
> HTMLMediaElement from it as we need to pass this to the nsMediaEvent?

Pass the "this" pointer in HTMLMediaElement::
RunInStableState(nsIRunnable*) when you construct nsSyncSection.

Presumably the Track element has a reference to it's media element, through which it calls RunInStableState(), so the media element has a reference to itself.

The nsIRunnable* that you pass to RunInStableState() carries with it a reference to the text track (NS_NewRunnableMethod takes care of this for you).


i.e., in track element call this:

mediaElement->RunInStableState(NS_NewRunnableMethod(this/*track element this*/, HTMLTextTrack::SomeFuncToRunInStableState));


and in media element:

HTMLMediaElement::RunInStableState(nsIRunnable* runnable)
{
  nsCOMPtr<nsIRunnable> syncSection(new nsSyncSection(this /* media elememt this*/, runnable);
  nsCOMPtr<nsIAppShell> appShell = do_GetService(kAppShellCID);
  appShell->RunInStableState(event);
}

You need to change nsSyncSection's constructor to take the reference to the runnable, and nsSyncSection::Run should call the Run() method on the runnbale passed into its constructor.

Does that make sense? We can Skype about it tonight-EST/tomorrow-NZT if you like.

Comment 6

[Rick Eyre (:reyre)]

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

> Does that make sense? We can Skype about it tonight-EST/tomorrow-NZT if you
> like.

I think I got it now. I'll definitely take you up on that call if I get anymore trouble. 

Thanks Chris.

Comment 7

[Rick Eyre (:reyre)]

Attachment: v1: Provide a stable state for asynchronous HTMLTrackElement functions

Comment 8

[Rick Eyre (:reyre)]

Attachment: v1: Provide a stable state for asynchronous HTMLTrackElement functions

Attached wrong file last time.

Comment 9

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 768688 [details] [diff] [review]
v1: Provide a stable state for asynchronous HTMLTrackElement functions

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

r=cpearce with the changes I list below. Looks good. :)

::: content/html/content/public/HTMLMediaElement.h
@@ +320,4 @@
>     */
>    void SetRequestHeaders(nsIHttpChannel* aChannel);
>  
> +  /** 

Nit: Trailing whitespace here.

::: content/html/content/src/HTMLMediaElement.cpp
@@ +660,4 @@
>  class nsSyncSection : public nsMediaEvent
>  {
>  private:
> +  nsIRunnable* mRunnable;

This needs to be an nsCOMPtr<nsIRunnable>, else the runnable could be deleted before it gets a chance to run, and then fireworks will happen.

@@ -679,5 @@
>  };
>  
>  static NS_DEFINE_CID(kAppShellCID, NS_APPSHELL_CID);
>  
> -// Asynchronously awaits a stable state, whereupon aClosure runs on the main

Please add the second sentence of this comment to the comment on RunInStableState in HTMLMediaElement.h.

Comment 10

[Rick Eyre (:reyre)]

Attachment: v2: Provide a stable state for asynchronous HTMLTrackElement functions r=cpearce

(In reply to Chris Pearce (:cpearce) from comment #9)
> Nit: Trailing whitespace here.

Fixed.

> This needs to be an nsCOMPtr<nsIRunnable>, else the runnable could be
> deleted before it gets a chance to run, and then fireworks will happen.

Fixed.

> Please add the second sentence of this comment to the comment on
> RunInStableState in HTMLMediaElement.h.

Done.  Sorry about that.

Thanks for the review and your help Chris! :)

Carrying forward r=cpearce

Comment 11

[Rick Eyre (:reyre)]

Attachment: v2: Provide a stable state for asynchronous HTMLTrackElement functions r=cpearce

In our weekly call Ralph said that Chris' review of this patch was enough. I've updated the commit message to reflect Ralph no longer reviewing.

Carrying forward r=cpearce

Try push: https://tbpl.mozilla.org/?tree=Try&rev=ced2cfacfaf8

Comment 12

[Rick Eyre (:reyre)]

Attachment: v3: v2: Provide a stable state for asynchronous HTMLTrackElement functions r=cpearce

Was failing test_texttrackcue.html as now the TrackElement's data is loaded before "loadedmetadata" event is dispatched. I've fixed it so the test now waits for the event to be dispatched and the track element to be ready before testing the cues.

I've also set the media element to have preload=auto in the test so that tests on Android will pass.

Carrying forward r=cpearce.

Try push: https://tbpl.mozilla.org/?tree=Try&rev=b93ce5eb96ca

Comment 13

[Rick Eyre (:reyre)]

Try looks green so I'm marking for checkin now.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fbeb1c23c115

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/fbeb1c23c115