Closed Bug 1129246 Opened 5 years ago Closed 5 years ago

Improve sample request and WaitForData promise interaction between MediaSourceReader and MediaDecoderStateMachine

Categories

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

x86
macOS
defect

Tracking

()

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

People

(Reporter: bholley, Assigned: bholley)

References

Details

Attachments

(1 file, 1 obsolete file)

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.
Depends on: 1129247
Depends on: 1129523
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.
Attachment #8559584 - Flags: review?(cpearce)
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.
Attachment #8559584 - Attachment is obsolete: true
Attachment #8559967 - Flags: review?(cpearce)
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().
Attachment #8559967 - Flags: review?(cpearce) → review+
(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?
Flags: needinfo?(cpearce)
(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.
Flags: needinfo?(cpearce)
(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?
Flags: needinfo?(cpearce)
Also NI ralph for uplift.
Flags: needinfo?(giles)
(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.
Flags: needinfo?(cpearce)
Priority: -- → P1
https://hg.mozilla.org/mozilla-central/rev/b4e3104507bf
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Flags: needinfo?(giles)
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.
Attachment #8559967 - Flags: approval-mozilla-aurora?
Comment on attachment 8559967 [details] [diff] [review]
Switch to MediaPromiseConsumerHolders for MDSM audio/video promises and remove RequestStatus. v2

MSE fix. Aurora+
Attachment #8559967 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.