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

RESOLVED FIXED in Firefox 42

Status

()

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: jya, Assigned: jya)

Tracking

(Blocks 1 bug)

Trunk
mozilla42
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox42 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

4 years ago
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
(Assignee)

Comment 1

4 years ago
Behaviour is actually okay, thanks to bug 1171311.
Attachment #8639685 - Flags: review?(cpearce)
(Assignee)

Updated

4 years ago
Assignee: nobody → jyavenard
Depends on: 1171311
(Assignee)

Comment 3

4 years ago
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+
(Assignee)

Comment 5

4 years ago
(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>]).
(Assignee)

Comment 7

4 years ago
yes, that would work. This is what I suggested on IRC ; process the two sourcebuffers in parallel.
(Assignee)

Comment 8

4 years ago
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)
(Assignee)

Comment 9

4 years ago
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.
(Assignee)

Comment 10

4 years ago
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)
(Assignee)

Comment 11

4 years ago
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
Last Resolved: 4 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.