1136399 - add test_WaitingOnMissingData_mp4.html and disable the webm version for now

Status: RESOLVED FIXED

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

Description

[Bobby Holley (:bholley)]

Given that webm is deprioritized right now, nobody is very interested in looking into failures in webm MSE tests.

Bug 1124493 indicates that test_WaitingOnMissingData.html has lots of intermittent problems. I'm going to land an mp4 version, and disable the webm one for now.

Comment 1

[Bobby Holley (:bholley)]

Attachment: Add test_WaitingOnMissingData_mp4.html and disable the webm version for now. v1

Comment 2

[Bobby Holley (:bholley)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=098a64f8a733

Comment 3

[Jean-Yves Avenard [:jya]]

Comment on attachment 8570203 [details] [diff] [review]
Add test_WaitingOnMissingData_mp4.html and disable the webm version for now. v1

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

::: dom/media/mediasource/test/mediasource.js
@@ +49,5 @@
> +  for (var i = start; i < end; ++i) {
> +    rv.push(i);
> +  }
> +  return rv;
> +}

I think all the tests would be cleared if the loop was i <= end

so we see which segments are loaded quickly.

But then that makes test_SeekTwice_mp4.html in need of an update

::: dom/media/mediasource/test/test_WaitingOnMissingData_mp4.html
@@ +17,5 @@
> +    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')
> +    .then(fetchAndLoad.bind(null, audiosb, 'bipbop/bipbop_audio', range(1, 5), '.m4s'))

I don't like those boundaries value.
At a glance, you can't tell that it's only loading 1-4 then 6-11 and so we are missing segment 5 (which is loaded later)

@@ +30,5 @@
> +          if (el.currentTime > 3.8) {
> +            el.removeEventListener("waiting", onwaiting);
> +            ok(true, "Received waiting event at time " + el.currentTime);
> +            resolve();
> +          }

shouldn't we simply resolve with an error if waiting was fired at currentTime < 3.8 (or whatever the end time is for video segment 4 or audio segment 5)?
When "waiting" is fired should be deterministic, and equal to min(audiosb.buffered.end(0), videosb.buffered.end(0))

that would also test that we properly entirely play the data we have and not stall before.

Right now, it seems that intermittently, we stall before reaching the end. I believe that this test will fail with a timeout error from time to time until that bug is solved (which would be a good thing).
Returning an error rather than causing the test to timeout will give greater clarity when the test fail.

The intermittent timeout showing up with webm is I believe also occurring with mp4.

@@ +44,5 @@
> +                               fetchAndLoad(videosb, 'bipbop/bipbop_video', [6], '.m4s')]);
> +      loads.then(() => ms.endOfStream());
> +      return p;
> +    }).then(function() {
> +      isfuzzy(el.duration, 10, 0.1, "Video has correct duration.");

this shouldn't be a fuzzy with MP4. the duration will be a known value.
it was fuzzy with webm as we can't accurately determine the exact duration of the last frame

Comment 4

[Jean-Yves Avenard [:jya]]

Now, if the aim is to only test the waiting event is fired, you can r=me ... But if it is to test that waiting is fired only when it should be fired and not a millisecond earlier. we need to rework it.

Comment 5

[Bobby Holley (:bholley)]

(In reply to Jean-Yves Avenard [:jya] from comment #3)

> I don't like those boundaries value.
> At a glance, you can't tell that it's only loading 1-4 then 6-11 and so we
> are missing segment 5 (which is loaded later)

The semantics of range() are the same once used in python's range() function, the one from underscore.js, git, etc - [start, end).

> shouldn't we simply resolve with an error if waiting was fired at
> currentTime < 3.8

That's a good idea. Originally the webm test didn't wait for loadSegment to complete before invoking play(), which is why there was the possibility for earlier 'waiting' events. But the current infrastructure lets us assert that, and simplify the code.

> (or whatever the end time is for video segment 4 or audio
> segment 5)?
> When "waiting" is fired should be deterministic, and equal to
> min(audiosb.buffered.end(0), videosb.buffered.end(0))

Not necessarily. Video currentTime is based on the current video frame (not sure if that's spec correct, but that's what we do), so if the audio ends just before the next video frame, currentTime can be up to 1 frame's worth earlier than min(audioEnd, videoEnd). We could hard-code this value, but I think it actually does us a disservice to do that. I'll update the test to be more specific about the fuzz factor.
 
> that would also test that we properly entirely play the data we have and not
> stall before.

Agreed.

> Right now, it seems that intermittently, we stall before reaching the end. I
> believe that this test will fail with a timeout error from time to time
> until that bug is solved (which would be a good thing).

Which bug is this? Is it on file?

> Returning an error rather than causing the test to timeout will give greater
> clarity when the test fail.

By failing if we get another waiting event? We can't guarantee that unless we re-pause the video and wait for the buffer loads to complete, I think.

> The intermittent timeout showing up with webm is I believe also occurring
> with mp4.

If this test goes intermittently orange, we should definitely fix it. If you know of a bug that will make it do so, please let me know which one it is.


> @@ +44,5 @@
> > +                               fetchAndLoad(videosb, 'bipbop/bipbop_video', [6], '.m4s')]);
> > +      loads.then(() => ms.endOfStream());
> > +      return p;
> > +    }).then(function() {
> > +      isfuzzy(el.duration, 10, 0.1, "Video has correct duration.");
> 
> this shouldn't be a fuzzy with MP4. the duration will be a known value.
> it was fuzzy with webm as we can't accurately determine the exact duration
> of the last frame

By local experimentation, it looks like the duration appears to be equal to the duration of the longer source buffer (10.03102) rather than the shorter one (10.01). Moreover, try indicates that .currentTime is occasionally 10.076667. We should investigate this, but I don't think it's the most important thing to do right now. I'll file a followup bug.

Comment 6

[Bobby Holley (:bholley)]

Attachment: Add test_WaitingOnMissingData_mp4.html and disable the webm version for now. v2

Comment 7

[Bobby Holley (:bholley)]

Try push for the intermittent windows oranges in comment 2.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2cec9c39fb0f

Comment 8

[Jean-Yves Avenard [:jya]]

(In reply to Bobby Holley (:bholley) from comment #5)
> 
> The semantics of range() are the same once used in python's range()
> function, the one from underscore.js, git, etc - [start, end).

let's change all of those too while at it :)

> 
> > shouldn't we simply resolve with an error if waiting was fired at
> > currentTime < 3.8
> 
> That's a good idea. Originally the webm test didn't wait for loadSegment to
> complete before invoking play(), which is why there was the possibility for
> earlier 'waiting' events. But the current infrastructure lets us assert
> that, and simplify the code.
> 
> > (or whatever the end time is for video segment 4 or audio
> > segment 5)?
> > When "waiting" is fired should be deterministic, and equal to
> > min(audiosb.buffered.end(0), videosb.buffered.end(0))
> 
> Not necessarily. Video currentTime is based on the current video frame (not
> sure if that's spec correct, but that's what we do), so if the audio ends
> just before the next video frame, currentTime can be up to 1 frame's worth
> earlier than min(audioEnd, videoEnd). We could hard-code this value, but I
> think it actually does us a disservice to do that. I'll update the test to
> be more specific about the fuzz factor.

but it should be constant regardless right ? that should vary between runs.

> > Right now, it seems that intermittently, we stall before reaching the end. I
> > believe that this test will fail with a timeout error from time to time
> > until that bug is solved (which would be a good thing).
> 
> Which bug is this? Is it on file?

it is now: bug 1137576

> 
> > Returning an error rather than causing the test to timeout will give greater
> > clarity when the test fail.
> 
> By failing if we get another waiting event? We can't guarantee that unless
> we re-pause the video and wait for the buffer loads to complete, I think.

no, this is still about testing that "waiting" is fired when currentTime is near min(sbaudio.buffered.end(0), sbvideo.buffered.end(0))

> If this test goes intermittently orange, we should definitely fix it. If you
> know of a bug that will make it do so, please let me know which one it is.


> By local experimentation, it looks like the duration appears to be equal to
> the duration of the longer source buffer (10.03102) rather than the shorter
> one (10.01). Moreover, try indicates that .currentTime is occasionally
> 10.076667. We should investigate this, but I don't think it's the most
> important thing to do right now. I'll file a followup bug.

That's per spec.
http://w3c.github.io/media-source/#sourcebuffer-coded-frame-processing
step 5:
"If the media segment contains data beyond the current duration, then run the duration change algorithm with new duration set to the maximum of the current duration and the group end timestamp."

so mediasource.duration = max(sbaudio.buffered.end, sbvideo.buffered.end)

Comment 9

[Jean-Yves Avenard [:jya]]

Comment on attachment 8570289 [details] [diff] [review]
Add test_WaitingOnMissingData_mp4.html and disable the webm version for now. v2

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

That's good. At least this should now error at random rather than timing out. (assuming bug 1137576 is the cause, and I have the suspicion than it is)

Comment 10

[Bobby Holley (:bholley)]

(In reply to Bobby Holley (:bholley) from comment #7)
> Try push for the intermittent windows oranges in comment 2.
> 
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=2cec9c39fb0f

Looks like we intermittently fire 'waiting' at t=0. Let's see if we can get some logging out of it.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c20719916608

Comment 11

[Bobby Holley (:bholley)]

Finally got some logging out of it here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7fdbd0020c0e

What's happening is that the play() invocation comes when HTMLMediaElement's mReadyState is still nsIDOMHTMLMediaElement::HAVE_CURRENT_DATA, so we fire 'waiting' at 0, here:

https://dxr.mozilla.org/mozilla-central/source/dom/html/HTMLMediaElement.cpp#2229

I'm going to hack around it for and wait for 'playing' before waiting for 'waiting', but it seems like the MSE code should wait to fire 'updateend' until that information has been propagated to the HTMLMediaElement. I'll file a bug about that.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ac4d25e589f7

Comment 12

[Bobby Holley (:bholley)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9122a80208aa

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9122a80208aa