1090991 - MediaSourceReader needs to do something intelligent when it gets EOS from a subdecoder and doesn't have anything to switch to

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Bobby Holley (:bholley)]

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.

Comment 1

[Bobby Holley (:bholley)]

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=899c6add8b88

Comment 2

[Bobby Holley (:bholley)]

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=5c04ab797950

Comment 3

[Bobby Holley (:bholley)]

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=7239271828e4

Comment 4

[Bobby Holley (:bholley)]

Attachment: Part 1 - Modify MediaData::Type so that it may serve as a general-purpose enum for distinguishing audio and video. v1

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

Comment 5

[Bobby Holley (:bholley)]

Attachment: Part 2 - Refactor RequestSampleCallback to use a single callback for all "not decoded" message. v1

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

Comment 6

[Bobby Holley (:bholley)]

Attachment: Part 3 - Unify MediaDecoderStateMachine::On{DecodeError,AudioEOS,VideoEOS} and eliminate duplicated logic. v1

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 7

[Bobby Holley (:bholley)]

Attachment: Part 4 - Introduce a new NotDecodedReason eWaitingForData and use it for MSE. v1

Comment 8

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

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 9

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

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 10

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

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 11

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

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...

Comment 12

[Bobby Holley (:bholley)]

(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.

Comment 13

[Bobby Holley (:bholley)]

(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.

Comment 14

[Bobby Holley (:bholley)]

Attachment: Part 2 - Refactor RequestSampleCallback to use a single callback for all "not decoded" message. v2 r=cpearce

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

Comment 15

[Bobby Holley (:bholley)]

Attachment: Part 4 - Introduce a new NotDecodedReason eWaitingForData and use it for MSE. v2 r=cpearce

Comment 16

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

(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!

Comment 17

[cajbir (:cajbir)]

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.

Comment 18

[Bobby Holley (:bholley)]

(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.

Comment 19

[Bobby Holley (:bholley)]

Attachment: Part 1 - Modify MediaData::Type so that it may serve as a general-purpose enum for distinguishing audio and video. v2 r=cpearce

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

Comment 20

[Bobby Holley (:bholley)]

Attachment: Part 2 - Refactor RequestSampleCallback to use a single callback for all "not decoded" message. v2 r=cpearce

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

Comment 21

[Bobby Holley (:bholley)]

Attachment: Part 3 - Unify MediaDecoderStateMachine::On{DecodeError,AudioEOS,VideoEOS} and eliminate duplicated logic. v3 r=cpearce

Comment 22

[Bobby Holley (:bholley)]

Attachment: Part 4 - Introduce a new NotDecodedReason WAITING_FOR_DATA and use it for MSE. v2 r=cpearce

Comment 23

[Bobby Holley (:bholley)]

Attachment: Part 4 - Introduce a new NotDecodedReason WAITING_FOR_DATA and use it for MSE. v3 r=cpearce

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 24

[cajbir (:cajbir)]

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.

Comment 25

[Bobby Holley (:bholley)]

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

Comment 26

[Ryan VanderMeulen [:RyanVM]]

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