1144481 - Prototype state-mirroring machinery to reduce dependency on the MediaDecoder monitor

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Bobby Holley (:bholley)]

There are a number of interaction patterns that make the media code racey. Some of them (in particular request/response patterns) are nicely handled by MediaPromises. Others (in particular shared state) are not. I aim to build a equally-useful primitive to manage the second class of use cases and make it easy to write safe code.

I've been pondering this for the last week or so, and have a rough plan for accomplishing this with a state-mirroring setup. I still need to flesh it out a bit more and give it a spin, at which point I'll post patches.

Comment 1

[Bobby Holley (:bholley)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2d6d6db26c26

Looking Green :-)

Comment 2

[Bobby Holley (:bholley)]

Attachment: Part 1 - Switch nsIThread implementation of AbstractThread::IsCurrentThreadIn() to use PRThreads. v1

Comment 3

[Bobby Holley (:bholley)]

Attachment: Part 2 - Invoke SeekCompleted synchronously. v1

The current behavior is a holdover from when this some of this tuff was happening
on the decode thread. It's problematic because nothing guarantees that we don't
call CheckIfSeekComplete twice, and queue up two SeekCompleted events.

Comment 4

[Bobby Holley (:bholley)]

Attachment: Part 3 - Force state machine setting to go through a setter. v1

We need this so that we can hook up the state mirroring in the subsequent patch.

Comment 5

[Bobby Holley (:bholley)]

Attachment: Part 4 - Implement state mirroring machinery. v1

Comment 6

[Bobby Holley (:bholley)]

Attachment: Part 5 - Use state mirroring for NextFrameStatus. v1

Comment 7

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8596332 [details] [diff] [review]
Part 4 - Implement state mirroring machinery. v1

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

LGTM.

::: dom/media/StateMirroring.h
@@ +49,5 @@
> +template<typename T> class AbstractMirror;
> +
> +/*
> + * AbstractCanonical is a superclass from which all Canonical values must
> + * inherit. It serves a the interface of operations which may be performed (via

It serves as the

@@ +58,5 @@
> +class AbstractCanonical
> +{
> +public:
> +  NS_INLINE_DECL_THREADSAFE_REFCOUNTING(AbstractCanonical)
> +  AbstractCanonical(AbstractThread* aThread) : mOwnerThread(aThread) {}

Can we call AbstractThread::GetCurrent() internally without requiring |aThread| if AbstractCanonical is always initialized in the same owning thread?

@@ +70,5 @@
> +};
> +
> +/*
> + * AbstractMirror is a superclass from which all Mirror values must
> + * inherit. It serves a the interface of operations which may be performed (via

serves as the

@@ +120,5 @@
> +    MIRROR_LOG("%s [%p] adding mirror %p", mName, this, aMirror);
> +    MOZ_ASSERT(OwnerThread()->IsCurrentThreadIn());
> +    MOZ_ASSERT(!mMirrors.Contains(aMirror));
> +    mMirrors.AppendElement(aMirror);
> +    aMirror->OwnerThread()->Dispatch(MakeNotifier(aMirror));

Can we do without exposing the idea of OwnerThread and let the callee figure out the right way to dispatch a job?

@@ +227,5 @@
> +      return;
> +    }
> +
> +    for (size_t i = 0; i < mMirrors.Length(); ++i) {
> +      OwnerThread()->TailDispatcher().AddStateChangeTask(mMirrors[i]->OwnerThread(), MakeNotifier(mMirrors[i]));

It looks like we still need to expose the idea of OwnerThread somehow.

Comment 8

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8596333 [details] [diff] [review]
Part 5 - Use state mirroring for NextFrameStatus. v1

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

::: dom/html/HTMLMediaElement.cpp
@@ +2853,3 @@
>  public:
> +  explicit StreamListener(HTMLMediaElement* aElement, const char* aName) :
> +    WatchTarget(aName),

We can just say WatchTarget("HTMLMediaElement::StreamListener") without requiring the caller to pass aName.

::: dom/media/MediaDecoder.cpp
@@ +1056,5 @@
>    PlaybackPositionChanged();
>    ChangeState(PLAY_STATE_ENDED);
>    InvalidateWithFlags(VideoFrameContainer::INVALIDATE_FORCE);
>  
> +  mReadyStateWatchTarget->Notify(); // XXXbholley - Still necessary?

I guess not.　Now the state machine is responsible for update next frame status when playback reaches the end.

::: dom/media/MediaDecoderStateMachine.cpp
@@ -804,5 @@
>    // TODO: Send aSample to MSG and recalculate readystate before pushing,
>    // otherwise AdvanceFrame may pop the sample before we have a chance
>    // to reach playing.
>    AudioQueue().Push(aSample);
> -  if (mState > DECODER_STATE_DECODING_FIRSTFRAME) {

Why removing the check for |mState > DECODER_STATE_DECODING_FIRSTFRAME|?

@@ +3034,5 @@
>    // If the number of audio/video frames queued has changed, either by
>    // this function popping and playing a video frame, or by the audio
>    // thread popping and playing an audio frame, we may need to update our
>    // ready state. Post an update to do so.
> +  UpdateNextFrameStatus();

I think this one is no longer needed since Pop{Audio,Video} will do it.

@@ +3201,5 @@
> +  MediaDecoderOwner::NextFrameStatus status;
> +  const char* statusString;
> +  if (IsBuffering()) {
> +    status = MediaDecoderOwner::NEXT_FRAME_UNAVAILABLE_BUFFERING;
> +    statusString = "NEXT_FRAME_UNAVAILABLE_BUFFERING";

It is better to have a lookup table to convert the enum value to a string.

Comment 9

[Bobby Holley (:bholley)]

(In reply to JW Wang [:jwwang] from comment #7)
> > +public:
> > +  NS_INLINE_DECL_THREADSAFE_REFCOUNTING(AbstractCanonical)
> > +  AbstractCanonical(AbstractThread* aThread) : mOwnerThread(aThread) {}
> 
> Can we call AbstractThread::GetCurrent() internally without requiring
> |aThread| if AbstractCanonical is always initialized in the same owning
> thread?

Unfortunately not - the counterexample is the MDSM constructor, which runs on the main thread, but needs to initialize mNextFrameStatus after initializing mTaskQueue.

Comment 10

[Bobby Holley (:bholley)]

(In reply to JW Wang [:jwwang] from comment #8)
> We can just say WatchTarget("HTMLMediaElement::StreamListener") without
> requiring the caller to pass aName.

The convention is for the name to be the name of the member rather than the type name. It's not a big deal, but I think that's a good enough reason to leave it as-is.

> 
> ::: dom/media/MediaDecoder.cpp
> @@ +1056,5 @@
> >    PlaybackPositionChanged();
> >    ChangeState(PLAY_STATE_ENDED);
> >    InvalidateWithFlags(VideoFrameContainer::INVALIDATE_FORCE);
> >  
> > +  mReadyStateWatchTarget->Notify(); // XXXbholley - Still necessary?
> 
> I guess not.　Now the state machine is responsible for update next frame
> status when playback reaches the end.

I'll fix this in bug 1157797.

> ::: dom/media/MediaDecoderStateMachine.cpp
> @@ -804,5 @@
> >    // TODO: Send aSample to MSG and recalculate readystate before pushing,
> >    // otherwise AdvanceFrame may pop the sample before we have a chance
> >    // to reach playing.
> >    AudioQueue().Push(aSample);
> > -  if (mState > DECODER_STATE_DECODING_FIRSTFRAME) {
> 
> Why removing the check for |mState > DECODER_STATE_DECODING_FIRSTFRAME|?

Because there are actually situations where we need to update readyState based on mNextFrameStatus when we're in state DECODER_STATE_DECODING_FIRSTFRAME (this was causing android orange on test_HaveMetadataUnbufferedSeek.html). In the current world, we end up triggering UpdateReadyState for other reasons, and then we call GetNextFrameStatus cross-thread, and everything works. But in the new world, we need to be very explicit about recomputing when we need to, because otherwise state updates never get sent.

> >    // If the number of audio/video frames queued has changed, either by
> >    // this function popping and playing a video frame, or by the audio
> >    // thread popping and playing an audio frame, we may need to update our
> >    // ready state. Post an update to do so.
> > +  UpdateNextFrameStatus();
> 
> I think this one is no longer needed since Pop{Audio,Video} will do it.

Yeah, make sense. I'm going to bounce it to bug 1157797 as well so that it can go through try.

> @@ +3201,5 @@
> > +  MediaDecoderOwner::NextFrameStatus status;
> > +  const char* statusString;
> > +  if (IsBuffering()) {
> > +    status = MediaDecoderOwner::NEXT_FRAME_UNAVAILABLE_BUFFERING;
> > +    statusString = "NEXT_FRAME_UNAVAILABLE_BUFFERING";
> 
> It is better to have a lookup table to convert the enum value to a string.

Is it, though? The downside of the state table is that people forget to update it, and then we end up with out-of-bound reads (this happens almost every time we add a new MDSM state). We need the lookup table more in the MDSM state case, because the callers pass in DECODERS_STATE_FOO directly to ChangeState, which means that the LUT helps us avoid a big unnecessary switch() statement. But in the UpdateNextFrameStatus case, we already need all the conditionals, so it seems less error-prone to just do all this work there.

Comment 11

[Bobby Holley (:bholley)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/b7bbc6003ef8
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/cd7f67ac308d
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/7703262fb954
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/c9617113b453
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/9bb004e50cc8

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b7bbc6003ef8
https://hg.mozilla.org/mozilla-central/rev/cd7f67ac308d
https://hg.mozilla.org/mozilla-central/rev/7703262fb954
https://hg.mozilla.org/mozilla-central/rev/c9617113b453
https://hg.mozilla.org/mozilla-central/rev/9bb004e50cc8