Closed Bug 1106547 Opened 5 years ago Closed 5 years ago

MP4Reader sometimes finishes prematurely

Categories

(Core :: Audio/Video, defect)

x86_64
Windows 7
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla37
Tracking Status
firefox36 --- fixed
firefox37 --- fixed

People

(Reporter: pehrsons, Assigned: pehrsons)

References

Details

Attachments

(1 file, 2 obsolete files)

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
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: nobody → pehrsons
Status: NEW → ASSIGNED
Attachment #8530857 - Flags: review?(cpearce)
Blocks: 879717
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 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.
(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.
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+
Well there's one case where mEOS is false at least. Ideas?
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...
Try's green. Again an orange coming from bug 879717 patches.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/b5c15cfddc2d
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
I think this still didn't quite fix the issue, bug 1107274 tracks the remaining work.
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?
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.