1229256 - canplay and canplaythrough event are never fired when media element is paused.

Status: RESOLVED FIXED

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

Description

[Jean-Yves Avenard [:jya]]

A simple example:
http://localhost/~jyavenard/public_html/tests/h264.html

will load an element, but not set to autoplay.

the events canplay and canplaythrough are never fired.

If the video is cached to the end however, canplay and canplaythrough are immediately fired even if paused.

Comment 1

[Jean-Yves Avenard [:jya]]

http://people.mozilla.org/~jyavenard/tests/h264.html

Comment 2

[Jean-Yves Avenard [:jya]]

The issue in our implementation is that MediaDecoderStateMachine::HaveNextFrameData() which will determine if we can move to readyState = HAVE_FUTURE_DATA simply look at the size of the decoded video frames queue and if it's > 1

However, when the media element is paused, we will never attempt to decode ahead and will stop as soon as the video frame is decoded.

The HTMLMediaElement::UpdateReadyStateInternal() however, has a workaround that checks if the videos has been entirely buffered and if so set readyState to HAVE_ENOUGH_DATA

Several way we can fix this, the easiest appear to simply decode at least one frame in advance, even in paused mode.

Comment 3

[Jean-Yves Avenard [:jya]]

Attachment: P1. Only check readyState update upon buffered range completion.

When MediaDecoder::NotifyDataArrived is called, the buffered range hasn't been updated as of yet, causing unnecessary calls to UpdateReadyState().
Delay the readyState update until the buffered range is modified.

Comment 4

[Jean-Yves Avenard [:jya]]

Attachment: P2. Use buffered range to determine next frame availability.

To avoid potential regression with some of our tests expecting our old particular behaviour, we only use the buffered range to determine the next frame status if the old method determined that the next frame was unavailable due to the MediaDecodeStateMachine not having decoded the next frame yet.

Comment 5

[Jean-Yves Avenard [:jya]]

Attachment: P3. Add mochitest testing new behaviour.

Ensure that the canplay event is fired even when the media is paused and preload=metadata

Comment 6

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8694518 [details] [diff] [review]
P2. Use buffered range to determine next frame availability.

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

::: dom/media/MediaDecoder.cpp
@@ +1820,5 @@
> +  // Use the buffered range to consider if we have the next frame available.
> +  media::TimeUnit currentPosition =
> +    media::TimeUnit::FromMicroseconds(CurrentPosition());
> +  media::TimeInterval interval(currentPosition,
> +                               currentPosition + media::TimeUnit::FromMicroseconds(DEFAULT_NEXT_FRAME_AVAILABLE_BUFFERED));

You can define the constant in this function because it is not accessed elsewhere.

Comment 7

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8694519 [details] [diff] [review]
P3. Add mochitest testing new behaviour.

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

::: dom/media/test/test_play_events_3.html
@@ +18,5 @@
> +// to fire "canplay".
> +function gotCanPlayEvent(event) {
> +  var v = event.target;
> +  ok(true,
> +     "Check expected event got " + event.type + " at " + v._state + " for " + v.src);

v.src would be a long URI which is too verbose. I would prefer to say "v.name".

@@ +19,5 @@
> +function gotCanPlayEvent(event) {
> +  var v = event.target;
> +  ok(true,
> +     "Check expected event got " + event.type + " at " + v._state + " for " + v.src);
> +  ok(v._gotmetadata == 1, "got loadedmedatadata event");

_gotmetadata should be a boolean.

@@ +30,5 @@
> +  var v = document.createElement('video');
> +  v.preload = "metadata";
> +  v.token = token;
> +  manager.started(token);
> +  v._state = 0;

_state is never used.

@@ +31,5 @@
> +  v.preload = "metadata";
> +  v.token = token;
> +  manager.started(token);
> +  v._state = 0;
> +  v._gotmetadata = 0;

This should be a boolean.

Comment 8

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8694519 [details] [diff] [review]
P3. Add mochitest testing new behaviour.

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

::: dom/media/test/test_play_events_3.html
@@ +27,5 @@
> +}
> +
> +function initTest(test, token) {
> +  var v = document.createElement('video');
> +  v.preload = "metadata";

You should use preload="auto". If you don't, you're risking adding another random orange.
 
With preload="metadata", we stop the download of media data when the first frame is produced. There's no guarantee that you'll get at least 250ms of data buffered before that happens. This could especially be the case if the network runs slow, like in mochitests on some of our test machines, then we could very well end up producing the first frame before we've buffered much of the resource over a slow connection.

Comment 9

[Jean-Yves Avenard [:jya]]

(In reply to JW Wang [:jwwang] from comment #6)
> Comment on attachment 8694518 [details] [diff] [review]
> P2. Use buffered range to determine next frame availability.
> 
> Review of attachment 8694518 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/media/MediaDecoder.cpp
> @@ +1820,5 @@
> > +  // Use the buffered range to consider if we have the next frame available.
> > +  media::TimeUnit currentPosition =
> > +    media::TimeUnit::FromMicroseconds(CurrentPosition());
> > +  media::TimeInterval interval(currentPosition,
> > +                               currentPosition + media::TimeUnit::FromMicroseconds(DEFAULT_NEXT_FRAME_AVAILABLE_BUFFERED));
> 
> You can define the constant in this function because it is not accessed
> elsewhere.

not yet.

I use it later for the NextFrameBufferedStatus() mediasource override.

Comment 10

[Jean-Yves Avenard [:jya]]

(In reply to Chris Pearce (:cpearce) from comment #8)
> Comment on attachment 8694519 [details] [diff] [review]
> P3. Add mochitest testing new behaviour.
> 
> Review of attachment 8694519 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/media/test/test_play_events_3.html
> @@ +27,5 @@
> > +}
> > +
> > +function initTest(test, token) {
> > +  var v = document.createElement('video');
> > +  v.preload = "metadata";
> 
> You should use preload="auto". If you don't, you're risking adding another
> random orange.

with preload=auto, then the MDSM will download and decode in advance making mNextFrameStatus always be at NEXT_FRAME_AVAILABLE.

As such, there would be nothing to test that isn't already working like we want.

I'd prefer to lower the buffered threshold if that was going to be the case.

Having said that, this mochitest isn't related to the speed of the media resource, nor would it be affected by the first frame being produced before we got the data buffered.

We will never move readyState past HAVE_CURRENT_DATA if we haven't decoded the first frame (this was fixed by bug 1217304)

So the concerns you mentioned aren't applicable here I don't think

Comment 11

[Jean-Yves Avenard [:jya]]

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

Comment 12

[JW Wang [:jwwang] [:jw_wang]]

(In reply to Jean-Yves Avenard [:jya] from comment #10)
> Having said that, this mochitest isn't related to the speed of the media
> resource, nor would it be affected by the first frame being produced before
> we got the data buffered.

I think Chris is right. If download speed is low, the network connection might be suspended before we get 250ms worth data which will affect the result of MediaDecoder::NextFrameBufferedStatus().

Comment 13

[Jean-Yves Avenard [:jya]]

how could it get suspended?

I may as well remove the mochitest then and only have a MSE equivalent where we are in full control of the content.

Comment 14

[JW Wang [:jwwang] [:jw_wang]]

https://hg.mozilla.org/mozilla-central/file/974fe614d5299159dc16d98d97d76af653158d29/dom/html/HTMLMediaElement.cpp#l3461

We would suspend MediaResource after 1st frame is loaded when preload=metadata to minimize memory usage.

Comment 15

[Jean-Yves Avenard [:jya]]

(In reply to JW Wang [:jwwang] from comment #14)
> We would suspend MediaResource after 1st frame is loaded when
> preload=metadata to minimize memory usage.

but that code is always reached regardless (as preload=metadata).
Yet, the buffered range is always > 250ms with all those small media.

we do suspend the decoder, but it still download at least 32kB of content (which is all we need to get 250ms of buffered range)

Comment 16

[Jean-Yves Avenard [:jya]]

Comment on attachment 8694519 [details] [diff] [review]
P3. Add mochitest testing new behaviour.

can't be bothered arguing on this anymore..

can't provide a test that is deterministic enough on such new (and proper) behaviour.

Comment 17

[Jean-Yves Avenard [:jya]]

Attachment: [MSE] P3. Prevent crash should buffered range be read while shutting down.

Moving this patch from bug 1194624 to here as attempting to read the buffered range while shutting down will cause a crash.
Carrying r+ from Gerald

Comment 18

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1148f6a8b576
https://hg.mozilla.org/integration/mozilla-inbound/rev/be3eab5c43f0
https://hg.mozilla.org/integration/mozilla-inbound/rev/b493cde48ac0

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1148f6a8b576
https://hg.mozilla.org/mozilla-central/rev/be3eab5c43f0
https://hg.mozilla.org/mozilla-central/rev/b493cde48ac0