Closed Bug 1157803 Opened 6 years ago Closed 6 years ago

Use state mirroring for mPlayState

Categories

(Core :: Audio/Video, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla40
Tracking Status
firefox40 --- fixed

People

(Reporter: bholley, Assigned: bholley)

References

Details

Attachments

(7 files)

State mirroring is about to land (\o/), and I think the next good application would be to mirror MediaDecoder::mPlayState into the MDSM, so that we don't need to call GetPlayState cross-thread. Combined with the right state-watching calls, we should be able to eliminated MDSM::Play, as well as MediaDecoder::ApplyStateToStateMachine.
Blocks: 1158448
The MDSM is constructed and destroyed on the main thread, but runs most
everything else on the task queue. So we need to bend the rules a bit here
to conveniently connect its mirrors during construction.
Attachment #8599046 - Flags: review?(jwwang)
Another consequence of the MDSM's thread model.
Attachment #8599047 - Flags: review?(jwwang)
Attachment #8599048 - Flags: review?(jwwang)
Blocks: 1159558
Note that some of the ergonomics here are going to be improved in bug 1159558.
Comment on attachment 8599044 [details] [diff] [review]
Part 1 - Add an extra assertion to MediaTaskQueue::BeginShutdown. v1

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

::: dom/media/MediaTaskQueue.cpp
@@ +158,5 @@
>  
>  nsRefPtr<ShutdownPromise>
>  MediaTaskQueue::BeginShutdown()
>  {
> +  // Make the there are no tasks for this queue waiting in the caller's tail

Make sure
Attachment #8599044 - Flags: review?(jwwang) → review+
Attachment #8599045 - Flags: review?(jwwang) → review+
Attachment #8599046 - Flags: review?(jwwang) → review+
Comment on attachment 8599047 [details] [diff] [review]
Part 4 - Don't auto-disconnect mirrors when the holder is destroyed off-owner-thread. v1

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

::: dom/media/StateMirroring.h
@@ +351,5 @@
> +        // If holder destruction happens on a thread other than the mirror's
> +        // owner thread, manual disconnection is mandatory. We should make this
> +        // more automatic by hooking it up to task queue shutdown.
> +        MOZ_DIAGNOSTIC_ASSERT(!mMirror->IsConnected());
> +      }

This is fine. But I think it is easier for the client code to remember "always do manual disconnection before destroying Holder" than to remember "do manual disconnection if you are gonna destroy Holder on some other thread". Again, we should avoid exposing thread details to the client code when possible.
Attachment #8599047 - Flags: review?(jwwang) → review+
Attachment #8599048 - Flags: review?(jwwang) → review+
Attachment #8599049 - Flags: review?(jwwang) → review+
Comment on attachment 8599050 [details] [diff] [review]
Part 7 - Mirror mPlayState and mNextState to the state machine task queue and eliminate cross-thread access. v1

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

Good!
Attachment #8599050 - Flags: review?(jwwang) → review+
You need to log in before you can comment on or make changes to this bug.