1157803 - Use state mirroring for mPlayState

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Bobby Holley (:bholley)]

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.

Comment 1

[Bobby Holley (:bholley)]

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

Comment 2

[Bobby Holley (:bholley)]

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

Comment 3

[Bobby Holley (:bholley)]

Attachment: Part 1 - Add an extra assertion to MediaTaskQueue::BeginShutdown. v1

Comment 4

[Bobby Holley (:bholley)]

Attachment: Part 2 - Loosen dispatch requirements for state mirroring. v1

Comment 5

[Bobby Holley (:bholley)]

Attachment: Part 3 - Support connect-during-initialization for mirrors. v1

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.

Comment 6

[Bobby Holley (:bholley)]

Attachment: Part 4 - Don't auto-disconnect mirrors when the holder is destroyed off-owner-thread. v1

Another consequence of the MDSM's thread model.

Comment 7

[Bobby Holley (:bholley)]

Attachment: Part 5 - Const-correct some methods. v1

Comment 8

[Bobby Holley (:bholley)]

Attachment: Part 6 - Clean up Canonical assigment to avoid accidentally tripping the default operator= implementation. v1

Comment 9

[Bobby Holley (:bholley)]

Attachment: Part 7 - Mirror mPlayState and mNextState to the state machine task queue and eliminate cross-thread access. v1

Comment 10

[Bobby Holley (:bholley)]

Note that some of the ergonomics here are going to be improved in bug 1159558.

Comment 11

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

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

Comment 12

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

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.

Comment 13

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

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!

Comment 14

[Bobby Holley (:bholley)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/4d0265c15acc
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/4a2c2ef2e24a
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/219435f463d2
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/4a072723d1d9
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/cd9eec88aa05
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/3eee65370b2b
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/5b8e33d6ad71

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/4d0265c15acc
https://hg.mozilla.org/mozilla-central/rev/4a2c2ef2e24a
https://hg.mozilla.org/mozilla-central/rev/219435f463d2
https://hg.mozilla.org/mozilla-central/rev/4a072723d1d9
https://hg.mozilla.org/mozilla-central/rev/cd9eec88aa05
https://hg.mozilla.org/mozilla-central/rev/3eee65370b2b
https://hg.mozilla.org/mozilla-central/rev/5b8e33d6ad71