Closed Bug 1135785 Opened 5 years ago Closed 5 years ago

Run more things in MediaDecoderStateMachine on the state machine thread

Categories

(Core :: Audio/Video, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla39
Tracking Status
firefox39 --- fixed

People

(Reporter: bholley, Assigned: bholley)

References

Details

Attachments

(7 files)

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.
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.
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.
Attachment #8568295 - Flags: review?(cpearce)
This is necessary so that we can make On{Audio,Video}{,Not}Decoded run on the
state machine thread in the next patch.
Attachment #8568296 - Flags: review?(cpearce)
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.
Attachment #8568297 - Flags: review?(cpearce)
Attachment #8568294 - Flags: review?(cpearce) → review+
Attachment #8568295 - Flags: review?(cpearce) → review+
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.
Attachment #8568846 - Flags: review?(cpearce)
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.
Attachment #8568296 - Flags: review?(cpearce) → review+
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.
Attachment #8568297 - Flags: review?(cpearce) → review+
Attachment #8568298 - Flags: review?(cpearce) → review+
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?
Attachment #8568846 - Flags: review?(cpearce)
Attachment #8568846 - Flags: review+
Attachment #8568846 - Flags: feedback?(sotaro.ikeda.g)
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.
Attachment #8569342 - Flags: review?(cpearce)
Blocks: 1136827
(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.
Blocks: 1135424
Blocks: 1136873
Comment on attachment 8568846 [details] [diff] [review]
Part 4.5 - Stop invoking StopPlayback in SetDormant. v1

It is OK.
Attachment #8568846 - Flags: feedback?(sotaro.ikeda.g) → feedback+
Attachment #8569342 - Flags: review?(cpearce) → review+
Depends on: 1138072
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.
You need to log in before you can comment on or make changes to this bug.