Closed Bug 1161903 Opened 5 years ago Closed 5 years ago

DrainComplete() can be called after mDrainComplete is reset

Categories

(Core :: Audio/Video, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla40
Tracking Status
firefox40 --- fixed

People

(Reporter: karlt, Assigned: karlt)

Details

Attachments

(2 files, 2 obsolete files)

Perhaps this is theoretical only but I don't see anything to ensure that a
pending DrainComplete is received before MP4Reader::Flush() runs.

Also, it is theoretically possible that there is already a decode task and a
drain task in WMFMediaDataDecoder's queue before Flush() is called.  Since
changes in bug 1155432, Flush() can return before the DrainComplete runs.
That could result in EOS on the a flushing reader, or possibly a different reader as SharedDecoderManager depends on Flush() to finish callbacks on one reader before switching to another.
DrainComplete() can also be called during Flush().
Attachment #8601917 - Flags: review?(matt.woodrow)
Using a Decode task already in the queue had the potential for out of order
flushing.

This is similar in behavior to the implementation prior to f550eb7809b6, but
keeps mDecoder->Flush() on the platform decoder's task queue, in the hope of
avoiding any ordering problems from calling in the middle of decoding input,
or races with MFTDecoder::mDiscontinuity.

The contract is not clear on whether DrainComplete() should be run during
Flush(), but I've kept it in this version.
Attachment #8601918 - Flags: review?(cpearce)
Attachment #8601918 - Flags: review?(cpearce)
This time dropping the pending input samples.
Attachment #8601947 - Flags: review?(cpearce)
Attachment #8601918 - Attachment is obsolete: true
Comment on attachment 8601947 [details] [diff] [review]
ensure pending DrainComplete is not run after Flush()

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

::: dom/media/fmp4/wmf/WMFMediaDataDecoder.cpp
@@ +144,3 @@
>    mIsFlushing = true;
> +  mTaskQueue->Dispatch(NS_NewRunnableMethod(this, &WMFMediaDataDecoder::ProcessFlush));
> +  mTaskQueue->AwaitIdle();

I think we're better to have the while(!mIsFlushing){mon.Wait();} loop here, and reset mIsFlushing in ProcessFlush(), so that there's no possibility for something else to stick stuff in the task queue and interfere with our flush.

I'd also like to move away from using MediaTaskQueue::AwaitIdle().
Attachment #8601947 - Flags: review?(cpearce) → review+
Attachment #8601917 - Flags: review?(matt.woodrow) → review+
This replaces the atomic with the monitor and removes AwaitIdle().

Also, the new Dispatch calls use nsCOMPtr/already_addRefed in line with
bug 1116905
Attachment #8602497 - Flags: review?(cpearce)
Attachment #8601947 - Attachment is obsolete: true
Attachment #8602497 - Flags: review?(cpearce) → review+
Comment on attachment 8601917 [details] [diff] [review]
reset mDrainComplete after Flush() as DrainComplete() may be called before Flush()

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

while reworking the decoder, I also notice an issue with mDrainComplete, it is never set back to false outside Flush(), this means that if we encounter an EOS (which may only be temporary when used with MSE) we will drain once, set mDrainComplete and then report back END_OF_STREAM.
The next time we hit EOS, as mDrainComplete is already set, END_OF_STREAM will be fired immediately and we'll never output the remaining frames until more data is added to the resource.

I believe this is what is causing the intermittent failures on test_WaitingForData_mp4 (not sure of the exact name right now)
You need to log in before you can comment on or make changes to this bug.