1129246 - Improve sample request and WaitForData promise interaction between MediaSourceReader and MediaDecoderStateMachine

Status: RESOLVED FIXED

(Core :: Audio/Video, defect, P1)

Description

[Bobby Holley (:bholley)]

I noticed a few sketchy spots, and some things that could be made more robust. I'm hoping that these will either fix the crashes in bug 1128171 or make them more diagnosable.

Comment 1

[Bobby Holley (:bholley)]

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

Comment 2

[Bobby Holley (:bholley)]

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

Comment 3

[Bobby Holley (:bholley)]

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

Comment 4

[Bobby Holley (:bholley)]

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

Comment 5

[Bobby Holley (:bholley)]

Attachment: Switch to MediaPromiseConsumerHolders for MDSM audio/video promises and remove RequestStatus. v1

Comment 6

[Bobby Holley (:bholley)]

Comment on attachment 8559584 [details] [diff] [review]
Switch to MediaPromiseConsumerHolders for MDSM audio/video promises and remove RequestStatus. v1

Oh wait - it's verboten to disconnect a MediaPromiseConsumerHolder on a thread other than the one it expects to be dispatched on, so the DORMANT thing doesn't work. Need to think about that a little bit.

Comment 7

[Bobby Holley (:bholley)]

Attachment: Switch to MediaPromiseConsumerHolders for MDSM audio/video promises and remove RequestStatus. v2

Actually, I think we can just avoid disconnecting in the dormant case
entirely. Whenever the promises are resolved, the receiving methods will just
ignore them when in DORMANT state.

Comment 8

[Bobby Holley (:bholley)]

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

Comment 9

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

Comment on attachment 8559967 [details] [diff] [review]
Switch to MediaPromiseConsumerHolders for MDSM audio/video promises and remove RequestStatus. v2

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

::: dom/media/MediaDecoderStateMachine.cpp
@@ +826,4 @@
>    SAMPLE_LOG("OnNotDecoded (aType=%u, aReason=%u)", aType, aReason);
>    bool isAudio = aType == MediaData::AUDIO_DATA;
>    MOZ_ASSERT_IF(!isAudio, aType == MediaData::VIDEO_DATA);
>  

Why not complete the m{Audio,Video}DataRequest here, instead of in the caller? MDSM::On{Audio,Video}NotDecoded() merely exist to bind a call to OnNotDecoded() with a MediaDataType first argument; all the logic is in here instead of in On{Audio,Video}NotDecoded().

@@ +1852,5 @@
>                    !needToDecodeAudio &&
>                    !needToDecodeVideo &&
>                    !IsPlaying();
>  
> +  SAMPLE_LOG("DispatchDecodeTasksIfNeeded needAudio=%d audioStatus=%s needVideo=%d videoStatus=%d needIdle=%d",

videoStatus=%s

@@ -2751,5 @@
>        DebugOnly<nsresult> rv = DecodeTaskQueue()->Dispatch(
>        NS_NewRunnableMethod(mReader, &MediaDecoderReader::ReleaseMediaResources));
>        MOZ_ASSERT(NS_SUCCEEDED(rv));
> -      mAudioRequestStatus = RequestStatus::Idle;
> -      mVideoRequestStatus = RequestStatus::Idle;

This is the only bit I'm not sure about... Can we somehow assert that the promises are resolved/rejected before we come out of dormant mode. That would be at the start of MediaDecoderStateMachine::DecodeMetadata().

Comment 10

[Bobby Holley (:bholley)]

(In reply to Chris Pearce (:cpearce) from comment #9)
> Why not complete the m{Audio,Video}DataRequest here, instead of in the
> caller? MDSM::On{Audio,Video}NotDecoded() merely exist to bind a call to
> OnNotDecoded() with a MediaDataType first argument; all the logic is in here
> instead of in On{Audio,Video}NotDecoded().

I can do that, sure - I'll also hoist the MOZ_ASSERT(OnDecodeThread()).

> videoStatus=%s

Good catch.
 
> @@ -2751,5 @@
> >        DebugOnly<nsresult> rv = DecodeTaskQueue()->Dispatch(
> >        NS_NewRunnableMethod(mReader, &MediaDecoderReader::ReleaseMediaResources));
> >        MOZ_ASSERT(NS_SUCCEEDED(rv));
> > -      mAudioRequestStatus = RequestStatus::Idle;
> > -      mVideoRequestStatus = RequestStatus::Idle;
> 
> This is the only bit I'm not sure about... Can we somehow assert that the
> promises are resolved/rejected before we come out of dormant mode. That
> would be at the start of MediaDecoderStateMachine::DecodeMetadata().

I'm concerned about doing that, because I don't have an entirely clear picture of how quickly the decoders flush and how quickly we can go into and then come out of dormant mode. My reasoning for doing just ignoring it is that the decoding pipeline knows that it has an outstanding request, and is going to resolve or reject it one way or another. If there's ever a way for it to "get stuck" with an outstanding sample request without either rejecting or making any progress on it, that's a bug and we should fix that anyway. So really, we just want to wait for the decoding pipeline to do its thing and finish resetting itself, and our MediaPromiseConsumerHolders will automatically get cleared. I'm not sure what bugs we're handing by doing anything about them here - my guess is that it's a holdover from back when sample requests could just get flushed from the task queue, which isn't the case anymore because Promise resolution/rejection is infallible.

What I _would_ like to do is to fix up the whole flushing story to work a bit more sanely. But that's a task for another bug.

I think I'd need to do an extra round-trip through try if I added any asserts here - so I'm going to push as-is, and can cover any additional asserts we decide we should add in a followup bug. Do you think the above analysis is sound, or is there additional work you'd like to do here?

Comment 11

[Bobby Holley (:bholley)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b4e3104507bf

Comment 12

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

(In reply to Bobby Holley (Busy with media, don't ask for DOM/JS/XPConnect things) from comment #10)
> > @@ -2751,5 @@
> > >        DebugOnly<nsresult> rv = DecodeTaskQueue()->Dispatch(
> > >        NS_NewRunnableMethod(mReader, &MediaDecoderReader::ReleaseMediaResources));
> > >        MOZ_ASSERT(NS_SUCCEEDED(rv));
> > > -      mAudioRequestStatus = RequestStatus::Idle;
> > > -      mVideoRequestStatus = RequestStatus::Idle;
> > 
> > This is the only bit I'm not sure about... Can we somehow assert that the
> > promises are resolved/rejected before we come out of dormant mode. That
> > would be at the start of MediaDecoderStateMachine::DecodeMetadata().
> 
> I'm concerned about doing that, because I don't have an entirely clear
> picture of how quickly the decoders flush and how quickly we can go into and
> then come out of dormant mode.

Currently the flush occurs in MediaDecoderReader::ResetDecode and blocks the decode task queue (we should really make ResetDecode return a promise; in another bug!).

The logic to enter/exit dormant mode happens on the main thread, so we could go into and come out of dormant mode instantaneously from the perspective of the state machine/decode threads. Indeed, we can come out of dormant mode before we've finished transitioning to dormant mode, thanks to us not being disciplined about which thread is allowed to change the state machine's state. :(


> My reasoning for doing just ignoring it is
> that the decoding pipeline knows that it has an outstanding request, and is
> going to resolve or reject it one way or another. If there's ever a way for
> it to "get stuck" with an outstanding sample request without either
> rejecting or making any progress on it, that's a bug and we should fix that
> anyway.

I agree. I was thinking that adding the assert that the tasks were canceled would make such failures more visible.

> I think I'd need to do an extra round-trip through try if I added any
> asserts here - so I'm going to push as-is, and can cover any additional
> asserts we decide we should add in a followup bug. Do you think the above
> analysis is sound, or is there additional work you'd like to do here?

A follow up that makes errors due to outstanding tasks when we come out of dormant mode more visible is fine.

Comment 13

[Bobby Holley (:bholley)]

(In reply to Chris Pearce (:cpearce) from comment #12)
> Indeed, we can come out of dormant mode
> before we've finished transitioning to dormant mode, thanks to us not being
> disciplined about which thread is allowed to change the state machine's
> state. :(

Yeah, this was my concern.

> I agree. I was thinking that adding the assert that the tasks were canceled
> would make such failures more visible.

But the issue is that we only know that the tasks were canceled asynchronously (by receiving the reject), and I'm not convinced that this is guaranteed to happen before we come out of dormant mode.

> A follow up that makes errors due to outstanding tasks when we come out of
> dormant mode more visible is fine.

Per the above, do you have concrete suggestions on how to do this in the current world? Or are you ok with waiting until we promise-ify dormant mode as you suggest?

Comment 14

[Bobby Holley (:bholley)]

Also NI ralph for uplift.

Comment 15

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

(In reply to Bobby Holley (Busy with media, don't ask for DOM/JS/XPConnect things) from comment #13)
> But the issue is that we only know that the tasks were canceled
> asynchronously (by receiving the reject), and I'm not convinced that this is
> guaranteed to happen before we come out of dormant mode.

Right, that's a valid concern.

> > A follow up that makes errors due to outstanding tasks when we come out of
> > dormant mode more visible is fine.
> 
> Per the above, do you have concrete suggestions on how to do this in the
> current world? Or are you ok with waiting until we promise-ify dormant mode
> as you suggest?

We can't safely do this until we can chain a "leave dormant" task on the resolution of an "enter dormant" promise right? So we may as well wait until we've promisified dormant mode.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b4e3104507bf

Comment 17

[Ralph Giles (:rillian)]

Comment on attachment 8559967 [details] [diff] [review]
Switch to MediaPromiseConsumerHolders for MDSM audio/video promises and remove RequestStatus. v2

Approval Request Comment
[Feature/regressing bug #]: MSE 
[User impact if declined]: Possible playback stalls or crashes with youtube video.
[Describe test coverage new/current, TreeHerder]: Landed on m-c.
[Risks and why]: This affects non-MSE playback, but has been fine on Nightly. Risk is low at this point.
[String/UUID change made/needed]: None.

This needs a simple rebase around bug 1111290.

Comment 18

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8559967 [details] [diff] [review]
Switch to MediaPromiseConsumerHolders for MDSM audio/video promises and remove RequestStatus. v2

MSE fix. Aurora+

Comment 19

[Ralph Giles (:rillian)]

https://hg.mozilla.org/releases/mozilla-aurora/pushloghtml?changeset=f24f0b0532f9