1135785 - Run more things in MediaDecoderStateMachine on the state machine thread

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Bobby Holley (:bholley)]

Right now we have very poor threading invariants in the MDSM. In general, there's very little rhyme or reason as to which assertion we make in a given method:

(A) this runs on the state machine thread
(B) this runs on the decode thread
(C) this method runs on either the state machine thread or the decode thread
(D) (no assertion at all)

As far as I can tell, this is primarily because decode requests need to be made on the decode thread, and samples come back on the decode thread. So we end up with a lot of machinery running on the decode thread, and that bleeds into the various helper methods and so on.

Now that we have MediaPromises and ProxyMediaCall, we can be much more precise about dispatching work to the decode task queue without actually running any of the MDSM machinery on that task queue. This will allow us to make most of the MDSM machinery exclusive to the state machine thread, which in turn will allow us to reduce our dependency on the catch-all decode monitor.

In addition to improve our threading invariants, this is somewhat urgent because we can only disconnect media promises on the dispatch thread, which means that bug 1135170's increased reliance on disconnection requires more stuff to be on the state machine thread.

This was originally part of bug 1135170, but I'm splitting it off because I think it merits its own landing.

Comment 1

[Bobby Holley (:bholley)]

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

This has opt-only orange because MediaTaskQueue::IsCurrentThreadIn lies in release builds. I added a patch to fix that, which means this should be ready for review.

Comment 2

[Bobby Holley (:bholley)]

Attachment: Part 1 - Introduce a 1-argument overload of ProxyMediaCall. v1

Comment 3

[Bobby Holley (:bholley)]

Attachment: Part 2 - Make MediaTaskQueue::IsCurrentThreadIn actually do the right thing on release builds. v1

The current situation is really dangerous because it compiles on release builds,
but just lies. This bit me when I tried to use it for non-assertion purposes.

My reading of the reasoning for the current setup in bug 968016 is that we didn't
trust nsIEventTarget::IsCurrentThreadOn or thought it might be slow. But the
implementation of MediaTaskQueue::IsCurrentThreadIn doesn't actually use that, and
indeed currently does all of the work for this feature in release builds anyway.

Comment 4

[Bobby Holley (:bholley)]

Attachment: Part 3 - Make DecodeError safe to run on any thread. v1

This is necessary so that we can make On{Audio,Video}{,Not}Decoded run on the
state machine thread in the next patch.

Comment 5

[Bobby Holley (:bholley)]

Attachment: Part 4 - Return samples on state machine thread. v1

This is necessary because we're going to want to start disconnecting sample
and seek requests directly from the state machine thread, and the machinery
asserts that disconnection happens on the same thread as resolution.

More generally, this is the right thing to do architecturally, and will help
wean us off the monitor.

Comment 6

[Bobby Holley (:bholley)]

Attachment: Part 5 - Hoist some work onto the state machine thread and tighten down our assertions. v1

Comment 7

[Bobby Holley (:bholley)]

Full try push, just to confirm: https://treeherder.mozilla.org/#/jobs?repo=try&revision=85736ecdcc9f

Comment 8

[Bobby Holley (:bholley)]

Attachment: Part 4.5 - Stop invoking StopPlayback in SetDormant. v1

This already gets incoded in the DECODER_STATE_DORMANT case of RunStateMachine,
which will run momentarily on the state machine thread. Doing this allows us to
avoid calling StopPlayback on the main thread.

Comment 9

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8568296 [details] [diff] [review]
Part 3 - Make DecodeError safe to run on any thread. v1

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

::: dom/media/MediaDecoderStateMachine.cpp
@@ +2151,5 @@
>      // Already shutdown.
>      return;
>    }
>  
> +  // DecodeError should probably be redesigned so that it doesn't need to run

Yes, DecodeError should probably be redesigned so that it doesn't need to run on the decode task queue.

In fact, I don't see why DecodeError needs to be on the decode task queue. You should be able to proxy it over to the state machine thread... Unless there's a reason not to, please do that.

Comment 10

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8568297 [details] [diff] [review]
Part 4 - Return samples on state machine thread. v1

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

As I said on vidyo, my only concern with this is that when we block the state machine thread waiting for audio streams to shutdown we could be blocking the delivery of audio samples to other MDSM's sharing the same state machine thread.

We should move towards making the state machine thread a task queue that runs on a larger thread pool, so that all MDSMs don't have to share the same state machine thread. We can just use the decode thread pool for this, there's no reason why we really need more than 1 shared thread pool for media.

Comment 11

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8568846 [details] [diff] [review]
Part 4.5 - Stop invoking StopPlayback in SetDormant. v1

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

Sotaro: I assume this change is OK with you?

Comment 12

[Bobby Holley (:bholley)]

Attachment: Part 4.6 - Stop manually resetting mCurrentSeekTarget in MDSM::SeekCompleted and rely on the AutoSetOnScopeExit instead. v1

For some reason the current code is resetting it twice - once explicitly and
once with the AutoSetOnScopeExit. To make matters worse, we have a monitor drop
between the two. So when DecodeSeek runs on the decode task queue but SeekCompleted
runs on the state machine thread, we can start another DecodeSeek during the monitor
drop, and then clobber it with the AutoSeetOnScopeExit, causing us to hang.

This is a non-issue with the patches in bug 1135170, but necessary to make the
patches in this bug independently green.

Comment 13

[Bobby Holley (:bholley)]

(In reply to Chris Pearce (:cpearce) from comment #9)
> In fact, I don't see why DecodeError needs to be on the decode task queue.
> You should be able to proxy it over to the state machine thread... Unless
> there's a reason not to, please do that.

The synchronous dispatch and comment about shutdown crashes scare me, so I'll do that in a separate followup. Filed bug 1136827.

Comment 14

[Bobby Holley (:bholley)]

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

Comment 15

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8568846 [details] [diff] [review]
Part 4.5 - Stop invoking StopPlayback in SetDormant. v1

It is OK.

Comment 16

[Bobby Holley (:bholley)]

All green with the patch in the dependent bug:

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

Note that there is some test_eme_playback.html orange there, which I confirmed fixed by rebasing on top of cpearce's fix for it that already hit inbound: https://treeherder.mozilla.org/#/jobs?repo=try&revision=cf2a1869f52d

Just waiting on review for the dependent bug and we should be good to go here.

Comment 17

[Bobby Holley (:bholley)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/dc0b58714ffc
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/e637962ba64b
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/2a077477b16c
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/ede75f0e87f0
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/a9bf129cdbdb
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/6a49d13612c8
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/0b402720106c

Comment 18

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

https://hg.mozilla.org/mozilla-central/rev/dc0b58714ffc
https://hg.mozilla.org/mozilla-central/rev/e637962ba64b
https://hg.mozilla.org/mozilla-central/rev/2a077477b16c
https://hg.mozilla.org/mozilla-central/rev/ede75f0e87f0
https://hg.mozilla.org/mozilla-central/rev/a9bf129cdbdb
https://hg.mozilla.org/mozilla-central/rev/6a49d13612c8
https://hg.mozilla.org/mozilla-central/rev/0b402720106c