Closed
Bug 1161903
Opened 9 years ago
Closed 9 years ago
DrainComplete() can be called after mDrainComplete is reset
Categories
(Core :: Audio/Video, defect)
Core
Audio/Video
Tracking
()
RESOLVED
FIXED
mozilla40
Tracking | Status | |
---|---|---|
firefox40 | --- | fixed |
People
(Reporter: karlt, Assigned: karlt)
Details
Attachments
(2 files, 2 obsolete files)
1.23 KB,
patch
|
mattwoodrow
:
review+
|
Details | Diff | Splinter Review |
7.83 KB,
patch
|
cpearce
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•9 years ago
|
||
DrainComplete() can also be called during Flush().
Attachment #8601917 -
Flags: review?(matt.woodrow)
Assignee | ||
Comment 2•9 years ago
|
||
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)
Assignee | ||
Comment 3•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8899304213e1 https://treeherder.mozilla.org/#/jobs?repo=try&revision=3ef4f9d34e4a
Assignee | ||
Updated•9 years ago
|
Attachment #8601918 -
Flags: review?(cpearce)
Assignee | ||
Comment 4•9 years ago
|
||
This time dropping the pending input samples.
Attachment #8601947 -
Flags: review?(cpearce)
Assignee | ||
Updated•9 years ago
|
Attachment #8601918 -
Attachment is obsolete: true
Comment 5•9 years ago
|
||
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+
Updated•9 years ago
|
Attachment #8601917 -
Flags: review?(matt.woodrow) → review+
Assignee | ||
Comment 6•9 years ago
|
||
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)
Assignee | ||
Updated•9 years ago
|
Attachment #8601947 -
Attachment is obsolete: true
Updated•9 years ago
|
Attachment #8602497 -
Flags: review?(cpearce) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/9a8248d7999e https://hg.mozilla.org/integration/mozilla-inbound/rev/7b11fc3efb14
https://hg.mozilla.org/mozilla-central/rev/9a8248d7999e https://hg.mozilla.org/mozilla-central/rev/7b11fc3efb14
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Comment 9•9 years ago
|
||
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.
Description
•