Closed Bug 1269177 Opened 5 years ago Closed 5 years ago

MediaFormatReader could potentially not exit waiting for data mode

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox49 --- fixed

People

(Reporter: jya, Unassigned)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

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.
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+
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.
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/
https://hg.mozilla.org/mozilla-central/rev/e0086d2ba0d1
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
You need to log in before you can comment on or make changes to this bug.