MediaFormatReader could potentially not exit waiting for data mode

RESOLVED FIXED in Firefox 49

Status

()

P2
normal
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: jya, Unassigned)

Tracking

(Blocks: 1 bug)

unspecified
mozilla49
Points:
---

Firefox Tracking Flags

(firefox49 fixed)

Details

Attachments

(1 attachment)

(Reporter)

Description

3 years ago
As I'm investigating bug 1258922, I thought I saw a potential problem with the way waiting for data is handled.

I'm not 100% positive it can actually happen, but the code could be made to explicitly handle this case.

If there's a pending task to notify the reader that new data has arrived, but also a demuxing request pending to indicate that it's waiting for data.

The last task to arrive will set the mode, so even though we now have new data ready to be parsed, as the last demux request was rejected with WAITING_FOR_DATA, the MFR will enter waiting mode and not exit until new data is pushed.

We shouldn't handle the NotifyDataArrived task if there's a pending demuxing task.
(Reporter)

Comment 1

3 years ago
Created attachment 8747512 [details]
MozReview Request: Bug 1269177: Wait for demuxing request to complete before treating new data notification. r?gerald

Review commit: https://reviewboard.mozilla.org/r/49913/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/49913/
Attachment #8747512 - Flags: review?(gsquelart)
Comment on attachment 8747512 [details]
MozReview Request: Bug 1269177: Wait for demuxing request to complete before treating new data notification. r?gerald

https://reviewboard.mozilla.org/r/49913/#review46637

r+ with nit:

::: dom/media/MediaFormatReader.cpp:787
(Diff revision 1)
> -  if (decoder.mDrainComplete || decoder.mDraining) {
> +  if (decoder.mDrainComplete || decoder.mDraining ||
> +      decoder.mDemuxRequest.Exists()) {
>      // We do not want to clear mWaitingForData or mDemuxEOS while
>      // a drain is in progress in order to properly complete the operation.

You should update the comment, as it's not only about draining anymore.
Attachment #8747512 - Flags: review?(gsquelart) → review+
(Reporter)

Comment 3

3 years ago
https://reviewboard.mozilla.org/r/49913/#review46659

::: dom/media/MediaFormatReader.cpp:787
(Diff revision 1)
> -  if (decoder.mDrainComplete || decoder.mDraining) {
> +  if (decoder.mDrainComplete || decoder.mDraining ||
> +      decoder.mDemuxRequest.Exists()) {
>      // We do not want to clear mWaitingForData or mDemuxEOS while
>      // a drain is in progress in order to properly complete the operation.

I did so in bug 1269178 as I had more exceptions to add as well.
(Reporter)

Comment 4

3 years ago
Comment on attachment 8747512 [details]
MozReview Request: Bug 1269177: Wait for demuxing request to complete before treating new data notification. r?gerald

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49913/diff/1-2/
Priority: -- → P2

Comment 6

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/e0086d2ba0d1
Status: NEW → RESOLVED
Last Resolved: 3 years 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.