1198094 - some MediaDataDecoder will call InputExhausted call even when not needing more data

Status: RESOLVED FIXED

(Core :: Audio/Video: Playback, defect, P2)

Description

[Jean-Yves Avenard [:jya]]

Some decoders will call InputExhausted as soon as they've completed processing MediaDataDecoder::Input() and only check if the incoming task queue is empty in order to not call InputExhausted.

This approach is flawed as potentially the decoder could be fed data continuously by the reader until a frame is output.
For example, if it takes a little while between the time a frame is input and the time the frame is output ; the reader could have fed a great number of compressed frame during that time.

Currently the problem isn't too severe, but with bug 1197075 it could be made worse.

Currently, it appears that only the WMF decoder is doing the right thing. That is calling InputExhausted when it actually does need more data.

We should ensure that the various MediaDataDecoders do not call InputExhausted unnecessarily.

That the MediaDataDecoder is saying "gimme more" as soon as it has received frame is bad.

Comment 1

[Jean-Yves Avenard [:jya]]

Attachment: P1. Limit rate at which InputExhausted could be called by mac decoder.

Comment 2

[Jean-Yves Avenard [:jya]]

Upon checking, it appears that only the mac video decoder had the issue.

All others will only call InputExhausted once they know that it couldn't decode a frame.

Turned out that the intel VP8 decoder took a similar approach and ensure that input - output is < decode_ahead

To check the EME / GMP decoders

Comment 3

[Ralph Giles (:rillian)]

Comment on attachment 8652162 [details] [diff] [review]
P1. Limit rate at which InputExhausted could be called by mac decoder.

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

::: dom/media/platforms/apple/AppleVDADecoder.h
@@ +125,5 @@
> +  // Number of times Input() was called number of times the decoder has called
> +  // its callback. This is used to calculate how many frames has been buffered
> +  // by the decoder.
> +  uint32_t mNumberInput;
> +  uint32_t mNumberOutput;

How about mQueuedFrames? One variable instead of two, and you only ever use the difference of these two anyway.

Comment 4

[Jean-Yves Avenard [:jya]]

Attachment: P1. Limit rate at which InputExhausted could be called by mac decoder.

Good point.

Comment 5

[Ralph Giles (:rillian)]

Comment on attachment 8652640 [details] [diff] [review]
P1. Limit rate at which InputExhausted could be called by mac decoder.

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

Thanks. r=me with the comments. addressed.

::: dom/media/platforms/apple/AppleVDADecoder.cpp
@@ +214,5 @@
>    // FIXME: Distinguish between errors and empty flushed frames.
>    if (status != noErr || !image) {
>      NS_WARNING("AppleVDADecoder decoder returned no data");
> +    image = nullptr;
> +  } else if (infoFlags & kVDADecodeInfo_FrameDropped) {

How do these changes related to the bug? setting image = nullptr seems a little dodgy here; We're relying on Apple to always handle null references in OutputFrame, and our code to do something sane with the outputs?

@@ +310,5 @@
>    );
>  
> +  if (mQueuedSamples > mMaxRefFrames) {
> +    // We had stopped requesting more input because we had received too much at
> +    // the time. We can ask for more once again.

Is this backward? Seems like this will ask for more input whenever SubmitFrame() isn't, and vice-versa.

@@ +313,5 @@
> +    // We had stopped requesting more input because we had received too much at
> +    // the time. We can ask for more once again.
> +    mCallback->InputExhausted();
> +  }
> +  mQueuedSamples--;

You should still check for underflow here. Posting a decoder error would be reasonable I think; once it underflows decoding would effectively stop anyway.

@@ +513,3 @@
>      LOG("AppleVDADecoder task queue empty; requesting more data");
>      mCallback->InputExhausted();
>    }

If we're unblocking the call to InputExhausted in OutputFrame() can't we just drop this clause entirely? That seems to be what the WMF decoder does. Not sure if it would work to move the mInputIncoming check there or not.

Comment 6

[Jean-Yves Avenard [:jya]]

(In reply to Ralph Giles (:rillian) from comment #5)
> Comment on attachment 8652640 [details] [diff] [review]
> P1. Limit rate at which InputExhausted could be called by mac decoder.
> 
> Review of attachment 8652640 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks. r=me with the comments. addressed.
> 
> ::: dom/media/platforms/apple/AppleVDADecoder.cpp
> @@ +214,5 @@
> >    // FIXME: Distinguish between errors and empty flushed frames.
> >    if (status != noErr || !image) {
> >      NS_WARNING("AppleVDADecoder decoder returned no data");
> > +    image = nullptr;
> > +  } else if (infoFlags & kVDADecodeInfo_FrameDropped) {
> 
> How do these changes related to the bug? setting image = nullptr seems a
> little dodgy here; We're relying on Apple to always handle null references
> in OutputFrame, and our code to do something sane with the outputs?

this was changed so we could access mQueuedSamples safely without the use of a monitor and always call InputExhausted from our task queue.

Before we would have aborted early, now we set the frame to nullptr and OutputFrame will instead skip them, but only after updating mQueuedSamples and calling InputExhausted.

> 
> @@ +310,5 @@
> >    );
> >  
> > +  if (mQueuedSamples > mMaxRefFrames) {
> > +    // We had stopped requesting more input because we had received too much at
> > +    // the time. We can ask for more once again.
> 
> Is this backward? Seems like this will ask for more input whenever
> SubmitFrame() isn't, and vice-versa.

When we do this Input wouldn't have called InputExhausted because mQueuedSamples > mMaxRefFrames, this would always have been mQueuedSamples == mMaxRefFrames+1.

So when the callback occurs, we still have mQueuedSamples == mMaxRefFrames+1.
The idea is that we limit the number of calls to InputExhausted before the decoder starts to output frames. Once it does, we fire InputExhausted whenever a frame has been decoded

That way, the number of times InputExhausted was called will never be insane should the decoder takes a while to return the first frame.

For an average YouTube video, my mac pro would fire 171 InputExhausted before the first frame was returned.
This happens because demuxing is very fast and the MediaFormatReader was able to feed a frame to the decoder very quickly which would immediately call InputExhausted.

> 
> @@ +313,5 @@
> > +    // We had stopped requesting more input because we had received too much at
> > +    // the time. We can ask for more once again.
> > +    mCallback->InputExhausted();
> > +  }
> > +  mQueuedSamples--;
> 
> You should still check for underflow here. Posting a decoder error would be
> reasonable I think; once it underflows decoding would effectively stop
> anyway.

I could, but it cant ever happen unless VDA or VT is buggy ; it cant return more frames than we input.

> 
> @@ +513,3 @@
> >      LOG("AppleVDADecoder task queue empty; requesting more data");
> >      mCallback->InputExhausted();
> >    }
> 
> If we're unblocking the call to InputExhausted in OutputFrame() can't we
> just drop this clause entirely? That seems to be what the WMF decoder does.
> Not sure if it would work to move the mInputIncoming check there or not.

unfortunately no.
As the callback won't be called after a few frames typically. So we would not call InputExhausted after the first frame and playback would stall.

Comment 7

[Jean-Yves Avenard [:jya]]

(In reply to Ralph Giles (:rillian) from comment #5)
> Is this backward? Seems like this will ask for more input whenever
> SubmitFrame() isn't, and vice-versa.

to be more clear, no it isn't backward.

this is exactly what we want to achieve.

Allow Input to call InputExhausted until mMaxRefFrames are queued ; and after that we call InputExhausted whenever the decoder return a frame.

This leaves us with exactly mMaxRefFrames frames queued in the decoder at any given time

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/30a8392db2c2

Comment 9

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/2b4d90f62a5c for mQueuedSamples assertion failures like https://treeherder.mozilla.org/logviewer.html#?job_id=13354643&repo=mozilla-inbound

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/36c3c314bbbd

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/36c3c314bbbd

Comment 12

[Ralph Giles (:rillian)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/eef6993d896e

Comment 13

[Ralph Giles (:rillian)]

Backed out for a youtube playback regression. See Bug 1199573.
https://hg.mozilla.org/releases/mozilla-aurora/rev/5bb661db5c6c

Comment 14

[Ralph Giles (:rillian)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/0f4b50dab0f7