Closed
Bug 1144481
Opened 10 years ago
Closed 10 years ago
Prototype state-mirroring machinery to reduce dependency on the MediaDecoder monitor
Categories
(Core :: Audio/Video, defect)
Tracking
()
RESOLVED
FIXED
mozilla40
Tracking | Status | |
---|---|---|
firefox40 | --- | fixed |
People
(Reporter: bholley, Assigned: bholley)
References
Details
Attachments
(5 files)
Part 1 - Switch nsIThread implementation of AbstractThread::IsCurrentThreadIn() to use PRThreads. v1
(deleted),
patch
|
jwwang
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
jwwang
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
jwwang
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
jwwang
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
jwwang
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Updated•10 years ago
|
Blocks: MediaMonitor
Assignee | ||
Comment 1•10 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2d6d6db26c26 Looking Green :-)
Assignee | ||
Comment 3•10 years ago
|
||
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)
Assignee | ||
Comment 4•10 years ago
|
||
We need this so that we can hook up the state mirroring in the subsequent patch.
Attachment #8596331 -
Flags: review?(jwwang)
Updated•10 years ago
|
Attachment #8596329 -
Flags: review?(jwwang) → review+
Updated•10 years ago
|
Attachment #8596330 -
Flags: review?(jwwang) → review+
Updated•10 years ago
|
Attachment #8596331 -
Flags: review?(jwwang) → review+
Comment 7•10 years ago
|
||
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 8•10 years ago
|
||
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+
Assignee | ||
Comment 9•10 years ago
|
||
(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.
Assignee | ||
Comment 10•10 years ago
|
||
(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.
Assignee | ||
Comment 11•10 years ago
|
||
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•10 years ago
|
||
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
Status: NEW → RESOLVED
Closed: 10 years ago
status-firefox40:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
You need to log in
before you can comment on or make changes to this bug.
Description
•