The basic problem is that, once MediaDecoderStateMachine dispatches a decoding task, it won't try again until m{Audio,Video}RequestPending is cleared. So the reader absolutely cannot let a Request{Audio,Video}Data call go unanswered, since doing so will hang the state machine. Right now, MSE does just that. I have patches to fix it.

We don't seem to have something like this already, and this seemed better than introducing yet another new enum.

I'm going to add another one, and want this API to scale better than it does.

We take this opportunity to align the behavior of Finish() calls between audio and video EOS, invoking them unconditionally for both cases. Currently both cases always call Finish() immediately, with the exception of: (A) Video in seeking mode, where we may push mFirstVideoFrameAfterSeek before doing so, and (B) Video in the |default:| case. Push() and Finish() seem like orthogonal operations on MediaQueue, but we nonetheless preserve the old order just in case. There doesn't seem to be a good reason for (B).

Comment on attachment 8513839 [details] [diff] [review] Part 1 - Modify MediaData::Type so that it may serve as a general-purpose enum for distinguishing audio and video. v1 Review of attachment 8513839 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/media/MediaData.h @@ +24,5 @@ > class MediaData { > public: > > enum Type { > + eDataForAudio = 0, The (most common) convention in media code is to use all caps for enums, not eCamelCase. So please use the prevailing convention here. Please use: enum Type { AUDIO = 0, VIDEO = 1 }; If AUDIO/Video doesn't compile, try AUDIO_DATA, VIDEO_DATA.

Comment on attachment 8513840 [details] [diff] [review] Part 2 - Refactor RequestSampleCallback to use a single callback for all "not decoded" message. v1 Review of attachment 8513840 [details] [diff] [review]: ----------------------------------------------------------------- Looks fine, but I'm not sure about the change to MediaSourceReader.cpp... cajbir: Can you please check the changes to MediaSourceReader.* here please? ::: dom/media/MediaDecoderReader.cpp @@ +284,2 @@ > { > + if (aType == MediaData::eDataForAudio) { MOZ_ASSERT(aType == MediaData::Audio); Seems like a bad idea to have this silently accept non audio. ::: dom/media/MediaDecoderReader.h @@ +255,5 @@ > public: > NS_INLINE_DECL_THREADSAFE_REFCOUNTING(RequestSampleCallback) > > + enum NotDecodedReason { > + eEndOfStream, END_OF_STREAM, DECODE_ERROR ::: dom/media/mediasource/MediaSourceReader.cpp @@ +169,3 @@ > > + MOZ_ASSERT(aReason == RequestSampleCallback::eEndOfStream); > + if (IsEnded()) { The original MediaSourceReader::OnVideoEOS() only sends the EOS notification if SwitchVideoReader() fails. I'm not sure if this change is OK. Better get cajbir to check if this behaviour change is alright...

Comment on attachment 8513841 [details] [diff] [review] Part 3 - Unify MediaDecoderStateMachine::On{DecodeError,AudioEOS,VideoEOS} and eliminate duplicated logic. v1 Review of attachment 8513841 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/media/MediaDecoderStateMachine.cpp @@ +826,2 @@ > MOZ_ASSERT(aReason == RequestSampleCallback::eEndOfStream); > + if (!isAudio && mState == DECODER_STATE_SEEKING && Can you move this block into the switch statement, so that as much of the logic related to that case is localized there. Thanks.

Comment on attachment 8513842 [details] [diff] [review] Part 4 - Introduce a new NotDecodedReason eWaitingForData and use it for MSE. v1 Review of attachment 8513842 [details] [diff] [review]: ----------------------------------------------------------------- Nice. This is, in fact, very similar to what we need for EME to implement bug 1057168. ::: dom/media/MediaDecoderReader.h @@ +256,5 @@ > NS_INLINE_DECL_THREADSAFE_REFCOUNTING(RequestSampleCallback) > > enum NotDecodedReason { > eEndOfStream, > + eDecodeError, WAITNING_FOR_DATA ::: dom/media/mediasource/MediaSourceReader.cpp @@ +175,5 @@ > + > + // Otherwise, see if we can find a different reader that can pick up where we > + // left off. > + if (aType == MediaData::eDataForAudio && SwitchAudioReader(mLastAudioTime)) { > + RequestAudioData(); Note that, until we get the time to update our existing Readers to the newer async MediaDecoderReader implementation, RequestAudioData() is actually calling MediaDecoderReader::RequestAudioData(), which is actually decoding synchronously. Ditto for video. So your WaitingForData only works for MediaSourceReader, not for other Readers. This is fine for now, butat some stage we will probably want to fix MediaDecoderReader::RequestAudioData() to break out its while (AudioQueue().GetSize()==0) loop when the waiting-for-data notification comes in. Or move all our Readers over to the async model...

(In reply to Chris Pearce (:cpearce) from comment #9) > The original MediaSourceReader::OnVideoEOS() only sends the EOS notification > if SwitchVideoReader() fails. I'm not sure if this change is OK. Better get > cajbir to check if this behaviour change is alright... Yeah, that's a good point. I'll update the patch to fix that.

(In reply to Chris Pearce (:cpearce) from comment #10) > ::: dom/media/MediaDecoderStateMachine.cpp > @@ +826,2 @@ > > MOZ_ASSERT(aReason == RequestSampleCallback::eEndOfStream); > > + if (!isAudio && mState == DECODER_STATE_SEEKING && > > Can you move this block into the switch statement, so that as much of the > logic related to that case is localized there. Thanks. The reasons for this block being where it is are described in comment 6. I assume you mean that it's fine to potentially append frames to the video queue after calling Finish()? I'll go ahead and do that, but NIing you just to make sure that's what you meant.

I'm going to add another one, and want this API to scale better than it does.

(In reply to Bobby Holley (:bholley) from comment #13) > (In reply to Chris Pearce (:cpearce) from comment #10) > > ::: dom/media/MediaDecoderStateMachine.cpp > > @@ +826,2 @@ > > > MOZ_ASSERT(aReason == RequestSampleCallback::eEndOfStream); > > > + if (!isAudio && mState == DECODER_STATE_SEEKING && > > > > Can you move this block into the switch statement, so that as much of the > > logic related to that case is localized there. Thanks. > > The reasons for this block being where it is are described in comment 6. I > assume you mean that it's fine to potentially append frames to the video > queue after calling Finish()? I'll go ahead and do that, but NIing you just > to make sure that's what you meant. Ah I understand, the existing OnVideoEOS() implementation calls Finish() in every case inside the switch, whereas the OnAudioEOS() calls it before entering the switch. So we should not push data into the MediaQueue after calling Finish(), so please revert the change you made there back to match your previous patch, i.e. don't move Push() down into the seek case. Thanks for clarifying!

Is there a rebased or rolled up version of these patches I can apply to gecko-dev or inbound? I get merge conflicts and then compile errors if I fix the merges. I'ld like to be able to apply them before reviewing.

(In reply to cajbir (:cajbir) from comment #17) > Is there a rebased or rolled up version of these patches I can apply to > gecko-dev or inbound? I get merge conflicts and then compile errors if I fix > the merges. I'ld like to be able to apply them before reviewing. You can always find the current work here: https://github.com/bholley/gecko-dev/commits/mse_waitingevent I'll upload the patches again, fully rebased to trunk.

We don't seem to have something like this already, and this seemed better than introducing yet another new enum.

I'm going to add another one, and want this API to scale better than it does.

Simplified the previous patch with respect to decoder switching. We now only try again if the switch actually changes the decoder, rather than relying on the decoder's estimation of which frames it will be able to decode.

Comment on attachment 8514906 [details] [diff] [review] Part 2 - Refactor RequestSampleCallback to use a single callback for all "not decoded" message. v2 r=cpearce Review of attachment 8514906 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/media/mediasource/MediaSourceReader.cpp @@ +179,3 @@ > > + if (IsEnded()) { > + GetCallback()->OnNotDecoded(aType, aReason); Re-add the MSE_DEBUG call noting that this was EOS or add text if we are IsEnded in the initial MSE_DEBUG in the function.

Thanks for the reviews! remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/7d0240d750ee remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/b2e010c37d4c remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/7c7fc17410e2 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/3c5c16ff7733

https://hg.mozilla.org/mozilla-central/rev/7d0240d750ee https://hg.mozilla.org/mozilla-central/rev/b2e010c37d4c https://hg.mozilla.org/mozilla-central/rev/7c7fc17410e2 https://hg.mozilla.org/mozilla-central/rev/3c5c16ff7733