Pending decoded frames are dropped by MediaFormatReader when changing resolution.

RESOLVED FIXED in Firefox 49

Status

()

Core
Audio/Video: Playback
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: jya, Unassigned)

Tracking

(Blocks: 1 bug)

unspecified
mozilla49
Points:
---

Firefox Tracking Flags

(firefox49 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

(Reporter)

Description

a year ago
When the resolution is changing, the decoder is properly drained. However, depending on timing, it is possible that all pending decoded frames are dropped.

When during demuxing we detect a change of source id, we ask to drain the decoder and schedule a new update.

When MediaFormatReader::Update gets to run again, the decoder is then drain and Update exits.

Now assuming there are no new request for data, we immediately and handle the demuxed frames. There it sees that draining has already occurred and immediately reset the state ; which will drop all pending frames.

We shouldn't attempt to demux new frames or handle demuxed frames until the draincomplete flag has been processed ; which is when all pending decoded frames have already been returned.
(Reporter)

Comment 1

a year ago
Note that the issue only occurs with vp9, because the vp9 decoder has a latency of 0. So when draining the decoder draincomplete is immediately set.

with h264, draining would be set as we would have pending frames in the h264 decoder; in which case NeedInput returns false.
(Reporter)

Comment 2

a year ago
Created attachment 8759439 [details]
Bug 1277508: P1. Don't attempt to demux new samples while we're currently draining.

We should only attempt to demux new samples once all pending decoded frames have been returned. Otherwise, the next demuxing attempt once a resolution change has been detected will reset the state and drop all decoded frames.

Review commit: https://reviewboard.mozilla.org/r/57418/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/57418/
Attachment #8759439 - Flags: review?(dglastonbury)
Attachment #8759440 - Flags: review?(dglastonbury)
(Reporter)

Comment 3

a year ago
Created attachment 8759440 [details]
Bug 1277508: P2. Add HasPendingDrain convenience method.

Review commit: https://reviewboard.mozilla.org/r/57420/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/57420/
Attachment #8759439 - Flags: review?(dglastonbury) → review+
Comment on attachment 8759439 [details]
Bug 1277508: P1. Don't attempt to demux new samples while we're currently draining.

https://reviewboard.mozilla.org/r/57418/#review54252
Comment on attachment 8759440 [details]
Bug 1277508: P2. Add HasPendingDrain convenience method.

https://reviewboard.mozilla.org/r/57420/#review54254
Attachment #8759440 - Flags: review?(dglastonbury) → review+

Comment 6

a year ago
Pushed by jyavenard@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9cef6a01859a
P1. Don't attempt to demux new samples while we're currently draining. r=kamidphish
https://hg.mozilla.org/integration/mozilla-inbound/rev/8b89d98ce322
P2. Add HasPendingDrain convenience method. r=kamidphish

Comment 7

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/9cef6a01859a
https://hg.mozilla.org/mozilla-central/rev/8b89d98ce322
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox49: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
You need to log in before you can comment on or make changes to this bug.