Closed
Bug 1198094
Opened 9 years ago
Closed 9 years ago
some MediaDataDecoder will call InputExhausted call even when not needing more data
Categories
(Core :: Audio/Video: Playback, defect, P2)
Core
Audio/Video: Playback
Tracking
()
RESOLVED
FIXED
mozilla43
People
(Reporter: jya, Assigned: jya)
References
Details
Attachments
(2 files)
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
rillian
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•9 years ago
|
||
Attachment #8652162 -
Flags: review?(giles)
Assignee | ||
Comment 2•9 years ago
|
||
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•9 years ago
|
||
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.
Attachment #8652162 -
Flags: review?(giles)
Comment 5•9 years ago
|
||
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.
Attachment #8652640 -
Flags: review?(giles) → review+
Assignee | ||
Comment 6•9 years ago
|
||
(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.
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → jyavenard
Assignee | ||
Comment 7•9 years ago
|
||
(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 9•9 years ago
|
||
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
Updated•9 years ago
|
Priority: -- → P2
Comment 10•9 years ago
|
||
Comment 11•9 years ago
|
||
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
Comment 12•9 years ago
|
||
status-firefox42:
--- → fixed
Comment 13•9 years ago
|
||
Backed out for a youtube playback regression. See Bug 1199573.
https://hg.mozilla.org/releases/mozilla-aurora/rev/5bb661db5c6c
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Updated•9 years ago
|
Status: REOPENED → RESOLVED
Closed: 9 years ago → 9 years ago
Resolution: --- → FIXED
Comment 14•9 years ago
|
||
You need to log in
before you can comment on or make changes to this bug.
Description
•