Closed Bug 1129523 Opened 5 years ago Closed 5 years ago

Use proxying MediaPromise method calls for MediaDecoderStateMachine sample requests

Categories

(Core :: Audio/Video, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla38
Tracking Status
firefox36 --- unaffected
firefox37 --- fixed
firefox38 --- fixed

People

(Reporter: bholley, Assigned: bholley)

References

Details

Attachments

(2 files)

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.
Attachment #8559378 - Flags: review?(matt.woodrow)
Attachment #8559378 - Flags: review?(cpearce)
Attachment #8559379 - Flags: review?(matt.woodrow)
Attachment #8559379 - Flags: review?(cpearce)
Attachment #8559378 - Flags: review?(cpearce) → review+
Attachment #8559379 - Flags: review?(cpearce) → review+
Attachment #8559379 - Flags: review?(matt.woodrow) → review+
Attachment #8559378 - Flags: review?(matt.woodrow) → review+
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().
(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.
https://hg.mozilla.org/mozilla-central/rev/272f537c1438
https://hg.mozilla.org/mozilla-central/rev/652754b5bb07
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Depends on: 1132023
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.
Flags: needinfo?(giles)
Attachment #8559378 - Flags: approval-mozilla-aurora?
Attachment #8559378 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8559379 [details] [diff] [review]
Part 2 - Use ProxyMediaCall for video decode tasks. v1

[Triage Comment]
Attachment #8559379 - Flags: approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.