Closed Bug 1188210 Opened 5 years ago Closed 5 years ago

Add mochitest to ensure that we can transition from WAITING_FOR_DATA to END_OF_STREAM

Categories

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

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla42
Tracking Status
firefox42 --- fixed

People

(Reporter: jya, Assigned: jya)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 2 obsolete files)

This is one behaviour I'm ensure about with the new MSE.

If we have a race as we're playing data that is currently being appended.
It's possible that we reach the end of playback while the mediasource is still open (as opposed to "ended"). When that happens will will enter WAITING_FOR_DATA state

Should mediasource.endOfStream() now be called, we must transition from WAITING_FOR_DATA to END_OF_STREAM so that the "ended" event can be fired against the media element.

This is one theory I'm guessing could explain the intermittent timeout with test_eme_playback.html
Behaviour is actually okay, thanks to bug 1171311.
Attachment #8639685 - Flags: review?(cpearce)
Assignee: nobody → jyavenard
Depends on: 1171311
actually the patch that added the proper behavior was bug 1171311, patch #5
Comment on attachment 8639685 [details] [diff] [review]
Add mochitest to ensure transition from WAITING to ENDED.

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

::: dom/media/mediasource/test/test_WaitingToEndedTransition_mp4.html
@@ +16,5 @@
> +  once(ms, 'sourceopen').then(function() {
> +    ok(true, "Receive a sourceopen event");
> +    var audiosb = ms.addSourceBuffer("audio/mp4");
> +    var videosb = ms.addSourceBuffer("video/mp4");
> +    fetchAndLoad(audiosb, 'bipbop/bipbop_audio', ['init'], '.mp4')

Instead of doing all these operations serially, can you use:

Promises.all([
fetchAndLoad(audiosb, 'bipbop/bipbop_audio', ['init'], '.mp4'),
fetchAndLoad(videosb, 'bipbop/bipbop_video', ['init'], '.mp4'),
fetchAndLoad(audiosb, 'bipbop/bipbop_audio', range(1, 5), '.m4s'),
fetchAndLoad(videosb, 'bipbop/bipbop_video', range(1, 6), '.m4s')
])

(And I don't understand why you have "bind(null, ..." in your promises).

Promises.all() returns a promise that resolves when all promises in the array passed to it resolve.

That would make the test marginally faster.

If you're relying on the serial behaviour, then that won't work obviously.
Attachment #8639685 - Flags: review?(cpearce) → review+
(In reply to Chris Pearce (:cpearce) from comment #4)

> ::: dom/media/mediasource/test/test_WaitingToEndedTransition_mp4.html
> @@ +16,5 @@
> > +  once(ms, 'sourceopen').then(function() {
> > +    ok(true, "Receive a sourceopen event");
> > +    var audiosb = ms.addSourceBuffer("audio/mp4");
> > +    var videosb = ms.addSourceBuffer("video/mp4");
> > +    fetchAndLoad(audiosb, 'bipbop/bipbop_audio', ['init'], '.mp4')
> 
> Instead of doing all these operations serially, can you use:
> 
> Promises.all([
> fetchAndLoad(audiosb, 'bipbop/bipbop_audio', ['init'], '.mp4'),
> fetchAndLoad(videosb, 'bipbop/bipbop_video', ['init'], '.mp4'),
> fetchAndLoad(audiosb, 'bipbop/bipbop_audio', range(1, 5), '.m4s'),
> fetchAndLoad(videosb, 'bipbop/bipbop_video', range(1, 6), '.m4s')
> ])
> 
> (And I don't understand why you have "bind(null, ..." in your promises).
> 
> Promises.all() returns a promise that resolves when all promises in the
> array passed to it resolve.
> 
> That would make the test marginally faster.
> 
> If you're relying on the serial behaviour, then that won't work obviously.

This would give me no guarantee that the appendBuffer are happening in a given order.
It is perfectly possible that say bipbop_audio1.m4s download quicker than the init segment ; it would then be appended first and would cause an error.

Additionally, you can only run a single appendBuffer at a time and you must wait for the previous one to complete, otherwise the sourcebuffer will throw an exception.
So you must do append to the same sourcebuffer serially.

As far as the bind(null, ) bits, I have no idea to be honest. But it doesn't work without it, and that's what bholley did so I just did the same :)
(In reply to Jean-Yves Avenard [:jya] from comment #5)
> (In reply to Chris Pearce (:cpearce) from comment #4)
> > Instead of doing all these operations serially, can you use:
> > 
> > Promises.all([
> > fetchAndLoad(audiosb, 'bipbop/bipbop_audio', ['init'], '.mp4'),
> > fetchAndLoad(videosb, 'bipbop/bipbop_video', ['init'], '.mp4'),
> > fetchAndLoad(audiosb, 'bipbop/bipbop_audio', range(1, 5), '.m4s'),
> > fetchAndLoad(videosb, 'bipbop/bipbop_video', range(1, 6), '.m4s')
> > ])
> 
> This would give me no guarantee that the appendBuffer are happening in a
> given order.
> It is perfectly possible that say bipbop_audio1.m4s download quicker than
> the init segment ; it would then be appended first and would cause an error.

What you could do instead is append all inits first, then after they're all fully appended, send the rest, e.g. something like: Promises.all([<audio init>, <video init>]).then(Promises.all[<audio m4s's>, <video m4s's>]).
yes, that would work. This is what I suggested on IRC ; process the two sourcebuffers in parallel.
The name IsInitSegmentPresent and IsMediaSegmentPresent was misleading. As they are to return true only if data *starts* with such segment.
Attachment #8640302 - Flags: review?(ajones)
We considered that mActiveTrack was a global variable, however it is reset each time a new init segment is received.
Should two init segments be received in the same appendBuffer it would have been set to false, causing metadata to never be parsed.
Comment on attachment 8640302 [details] [diff] [review]
P2. Ensure data starts with and not just contains.

wrong bug #
Attachment #8640302 - Attachment is obsolete: true
Attachment #8640302 - Flags: review?(ajones)
Comment on attachment 8640303 [details] [diff] [review]
P3. Disambiguate naming of mActiveTrack boolean.

wrong bug#
Attachment #8640303 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/6eecb93e09cd
Status: NEW → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
You need to log in before you can comment on or make changes to this bug.