Closed Bug 1144481 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: Audio/Video, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla40
Tracking Status
firefox40 --- fixed

People

(Reporter: bholley, Assigned: bholley)

References

Details

Attachments

(5 files)

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.
Blocks: 1144486
Depends on: 1144487
Blocks: MediaMonitor
Depends on: 1150687
Depends on: 1151656
Depends on: 1154802
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.
Attachment #8596330 - Flags: review?(jwwang)
We need this so that we can hook up the state mirroring in the subsequent patch.
Attachment #8596331 - Flags: review?(jwwang)
Attachment #8596329 - Flags: review?(jwwang) → review+
Attachment #8596330 - Flags: review?(jwwang) → review+
Attachment #8596331 - Flags: review?(jwwang) → review+
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.
Attachment #8596332 - Flags: review?(jwwang) → review+
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.
Attachment #8596333 - Flags: review?(jwwang) → review+
(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.
Blocks: 1157797
(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.
Blocks: 1157803
Blocks: 1158916
You need to log in before you can comment on or make changes to this bug.