Closed Bug 1229256 Opened 4 years ago Closed 4 years ago

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

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox45 --- fixed

People

(Reporter: jya, Assigned: jya)

References

()

Details

Attachments

(3 files, 1 obsolete file)

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.
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.
Depends on: 1229299
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.
Attachment #8694517 - Flags: review?(jwwang)
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.
Attachment #8694518 - Flags: review?(jwwang)
Ensure that the canplay event is fired even when the media is paused and preload=metadata
Attachment #8694519 - Flags: review?(jwwang)
Attachment #8694517 - Flags: review?(jwwang) → review+
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.
Attachment #8694518 - Flags: review?(jwwang) → review+
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.
Attachment #8694519 - Flags: review?(jwwang) → review+
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.
(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.
(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
(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().
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.
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.
(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 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.
Attachment #8694519 - Attachment is obsolete: true
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
Attachment #8694966 - Flags: review+
Duplicate of this bug: 1234821
Depends on: 1267036
Assignee: nobody → jyavenard
You need to log in before you can comment on or make changes to this bug.