Closed
Bug 1106547
Opened 9 years ago
Closed 9 years ago
MP4Reader sometimes finishes prematurely
Categories
(Core :: Audio/Video, defect)
Tracking
()
RESOLVED
FIXED
mozilla37
People
(Reporter: pehrsons, Assigned: pehrsons)
References
Details
Attachments
(1 file, 2 obsolete files)
4.44 KB,
patch
|
pehrsons
:
review+
Sylvestre
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
dom/test/test_eme_playback.html started failing only on Windows with my patches for bug 879717 because we would never get the first video frame for short-cenc.mp4. This happened because on Windows we'd only get output frames after calling Drain() on the decoder. Meanwhile, we had already reported end of stream when the input stream ended; the same time as Drain was called. Hence the series of events was: * Input() for all samples * report end of stream, media element goes to ended state * Drain() * Drain pushes out all available output samples * DrainComplete() try with bug 879717: https://tbpl.mozilla.org/?tree=Try&rev=358178b9ce40
Assignee | ||
Comment 1•9 years ago
|
||
Seems reasonable to wait with reporting EOS until the drain has completed. Then all video frames that the platform decoder might be holding on to have been passed on to us. Fixes it for me on Windows 7. try to come soon.
Assignee | ||
Comment 2•9 years ago
|
||
try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=7819e8e6bc14
Comment 3•9 years ago
|
||
Comment on attachment 8530857 [details] [diff] [review] Return EOS after drain is complete, rather than before Review of attachment 8530857 [details] [diff] [review]: ----------------------------------------------------------------- Can you also remove DecoderData.mDrainComplete? It's write only. ::: dom/media/fmp4/MP4Reader.cpp @@ +729,5 @@ > + data.mDrainComplete = true; > + eos = data.mEOS; > + mon.NotifyAll(); > + } > + if (eos) { Can DrainComplete() really fire if mEOS is not true? I feel like we should be asserting that mEOS is true, if we can.
Attachment #8530857 -
Flags: review?(cpearce) → review+
Comment 4•9 years ago
|
||
Comment on attachment 8530857 [details] [diff] [review] Return EOS after drain is complete, rather than before Review of attachment 8530857 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/media/fmp4/MP4Reader.cpp @@ +727,5 @@ > + { > + MonitorAutoLock mon(data.mMonitor); > + data.mDrainComplete = true; > + eos = data.mEOS; > + mon.NotifyAll(); Also, you should be able to remove the NotifyAll() call here, since we no longer wait on the monitor.
See Also: → 1097116
Assignee | ||
Comment 5•9 years ago
|
||
(In reply to Chris Pearce (:cpearce) from comment #3) > Comment on attachment 8530857 [details] [diff] [review] > Return EOS after drain is complete, rather than before > > Review of attachment 8530857 [details] [diff] [review]: > ----------------------------------------------------------------- > > Can you also remove DecoderData.mDrainComplete? It's write only. > > ::: dom/media/fmp4/MP4Reader.cpp > @@ +729,5 @@ > > + data.mDrainComplete = true; > > + eos = data.mEOS; > > + mon.NotifyAll(); > > + } > > + if (eos) { > > Can DrainComplete() really fire if mEOS is not true? I feel like we should > be asserting that mEOS is true, if we can. Currently no, but I wasn't sure if there could be other use cases for Drain() that could be used in the future. I'll turn things around and assert mEOS instead.
Assignee | ||
Comment 6•9 years ago
|
||
Removes mDrainComplete from DecoderData and asserts mEOS in DrainComplete(), per comment 3 and 4. Here's a try for safety and to see that removing the mon.NotifyAll() was OK: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=c4286536228f
Attachment #8530857 -
Attachment is obsolete: true
Attachment #8531039 -
Flags: review+
Assignee | ||
Comment 7•9 years ago
|
||
Well there's one case where mEOS is false at least. Ideas?
Comment 8•9 years ago
|
||
OK, I maybe this could happen if we seek and hit EOS, and then seek again somewhere else (which resets the EOS flag) before the DrainComplete callback has a chance to run. We should only call ReturnEOS if mEOS is true I guess...
Assignee | ||
Comment 9•9 years ago
|
||
Checking mEOS again. Carries forward r=cpearce. https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=ce424feb96ce
Attachment #8531039 -
Attachment is obsolete: true
Attachment #8531097 -
Flags: review+
Assignee | ||
Comment 10•9 years ago
|
||
Try's green. Again an orange coming from bug 879717 patches.
Keywords: checkin-needed
Comment 11•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/b5c15cfddc2d
Keywords: checkin-needed
Comment 12•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/b5c15cfddc2d
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
Comment 13•9 years ago
|
||
I think this still didn't quite fix the issue, bug 1107274 tracks the remaining work.
Comment 14•9 years ago
|
||
Comment on attachment 8531097 [details] [diff] [review] Return EOS after drain is complete, rather than before (carries r=cpearce) Approval Request Comment [Feature/regressing bug #]: MSE [User impact if declined]: Less consistent playback support, video sites more likely to use flash. [Describe test coverage new/current, TBPL]: Landed on m-c. [Risks and why]: Low; bug fix to playback handling. [String/UUID change made/needed]: None
Attachment #8531097 -
Flags: approval-mozilla-aurora?
Updated•9 years ago
|
status-firefox36:
--- → affected
status-firefox37:
--- → fixed
Updated•9 years ago
|
Attachment #8531097 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in
before you can comment on or make changes to this bug.
Description
•