1129523 - Use proxying MediaPromise method calls for MediaDecoderStateMachine sample requests

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Bobby Holley (:bholley)]

The media code has poor threading invariants because, historically, running things on the right thread was hard - you needed to dispatch a custom runnable to invoke the method you want on the other thread, and then that method also needs to dispatch a custom runnable to send you back your answer.

MediaPromises solve the second problem nicely, but not the first (yet). I've been tossing around an idea to fix the first problem for a while, and last night I finally decided to set aside two hours and implement it. It seems to work well, and I think it will make our code less racey.

The basic idea is to use two promises. The first promise is created on the caller's thread, so that it can be Then()ed immediately. Under the hood, we dispatch a runnable to the target thread which invokes the right method, and chains the resulting promise to the first promise, so that the resolve/reject is forwarded through.

The upshot here is that callers like the MDSM can do all of their work in one place, rather than having to round-trip through a second runnable and then recheck all of the state to make sure things haven't changed.

Comment 1

[Bobby Holley (:bholley)]

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

Comment 2

[Bobby Holley (:bholley)]

Attachment: Part 1 - Implement MediaPromise proxies. v1

Comment 3

[Bobby Holley (:bholley)]

Attachment: Part 2 - Use ProxyMediaCall for video decode tasks. v1

Comment 4

[Bobby Holley (:bholley)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/272f537c1438
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/652754b5bb07

ni rillian for 37 uplift.

Comment 5

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

Comment on attachment 8559379 [details] [diff] [review]
Part 2 - Use ProxyMediaCall for video decode tasks. v1

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

::: dom/media/MediaDecoderStateMachine.cpp
@@ +1936,5 @@
> +  SAMPLE_LOG("Queueing audio task - queued=%i, decoder-queued=%o",
> +             AudioQueue().GetSize(), mReader->SizeOfAudioQueueInFrames());
> +
> +  mAudioRequestStatus = RequestStatus::Pending;
> +  ProxyMediaCall(DecodeTaskQueue(), mReader.get(), __func__, &MediaDecoderReader::RequestAudioData)

When MediaDecoderReader::RequestAudioData is called on the decoding thread, it is possible mState changes to something other than (DECODER_STATE_DECODING || DECODER_STATE_DECODING_FIRSTFRAME || DECODER_STATE_BUFFERING || DECODER_STATE_SEEKING). DecodeAudio() used to bail out in these cases but now MediaDecoderReader::RequestAudioData will go on anyway. This kinda change the logic. However, I wonder if the check in DecodeAudio() works. After checking mState and exiting the monitor, mState could change again before calling RequestAudioData().

Comment 6

[Bobby Holley (:bholley)]

(In reply to JW Wang [:jwwang] from comment #5)
> Comment on attachment 8559379 [details] [diff] [review]
> Part 2 - Use ProxyMediaCall for video decode tasks. v1
> 
> Review of attachment 8559379 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/media/MediaDecoderStateMachine.cpp
> @@ +1936,5 @@
> > +  SAMPLE_LOG("Queueing audio task - queued=%i, decoder-queued=%o",
> > +             AudioQueue().GetSize(), mReader->SizeOfAudioQueueInFrames());
> > +
> > +  mAudioRequestStatus = RequestStatus::Pending;
> > +  ProxyMediaCall(DecodeTaskQueue(), mReader.get(), __func__, &MediaDecoderReader::RequestAudioData)
> 
> When MediaDecoderReader::RequestAudioData is called on the decoding thread,
> it is possible mState changes to something other than
> (DECODER_STATE_DECODING || DECODER_STATE_DECODING_FIRSTFRAME ||
> DECODER_STATE_BUFFERING || DECODER_STATE_SEEKING). DecodeAudio() used to
> bail out in these cases but now MediaDecoderReader::RequestAudioData will go
> on anyway. This kinda change the logic. However, I wonder if the check in
> DecodeAudio() works. After checking mState and exiting the monitor, mState
> could change again before calling RequestAudioData().

Exactly - it was never really protecting us from much.

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/272f537c1438
https://hg.mozilla.org/mozilla-central/rev/652754b5bb07

Comment 8

[Ralph Giles (:rillian)]

Comment on attachment 8559378 [details] [diff] [review]
Part 1 - Implement MediaPromise proxies. v1

Requesting 37 uplift for both patches.

Approval Request Comment
[Feature/regressing bug #]: MSE
[User impact if declined]: Playback stalls with YouTube video.
[Describe test coverage new/current, TreeHerder]: Landed on m-c.
[Risks and why]: The first patch just adds new methods to media promise, which is minimal risk. The second refactors some of the MediaDecoderStateMachine. This affects non-MSE playback but seems low risk at this point.
[String/UUID change made/needed]: None.

Comment 9

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8559379 [details] [diff] [review]
Part 2 - Use ProxyMediaCall for video decode tasks. v1

[Triage Comment]

Comment 10

[Ralph Giles (:rillian)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/3339344d02a5
https://hg.mozilla.org/releases/mozilla-aurora/rev/c7855054fc68