1230527 - loadeddata should be fired after seeked event

Status: RESOLVED FIXED

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

Description

[Jean-Yves Avenard [:jya]]

This bug is mostly relevant to MSE, but can also apply to plain media playback.

Say that prior loading the video, we seek at position N.
then we load the video data (either by setting the src, or if MSE through the appropriate appendBuffer.

Per spec, we are to start immediately seeking upon reaching readyState = HAVE_METADATA "If the media element's default playback start position is greater than zero, then seek to that time, and let jumped be true."

In seeking, https://html.spec.whatwg.org/multipage/embedded-content.html#dom-media-seek

In step 13, we are to wait for a stable state during which "Wait until the user agent has established whether or not the media data for the new playback position is available, and, if it is, until it has decoded enough data to play back that position.".

once this is done we are to fire the timeupdate and seeked event.

From this reading, the first thing to do when the seek is complete is fire the "seeked" event.

Once the seek has completed, the readyState will move to HAVE_CURRENT_DATA and the loadeddata event will be fired.

This is the observed behaviour with all browsers to date (IE, Safari, Chrome)

With patches from bug 1130237 applied.
go to http://people.mozilla.org/~jyavenard/tests/mse_mp4/paper-offset.html

open a console and type:
document.querySelector('video').currentTime=3

It will fire the following events and in that order:
52084: * video.seeking
52794: * video.loadeddata
52797: * video.canplay
52800: * video.playing
52802: * video.canplaythrough
52814: * video.seeked

it should be in the order:
15200: * video.stalled
30789: * video.seeking
30796: * video.seeked
51027: * video.loadeddata
51029: * video.canplay
51030: * video.canplaythrough
51030: * video.playing

(the order of the last 2 do not matter)

Comment 1

[Jean-Yves Avenard [:jya]]

Attachment: P1. Ensure seeked event is fired prior loadeddata.

Comment 2

[Jean-Yves Avenard [:jya]]

Attachment: P2. Add mochitest.

It's also another way to check for 1130237: we make sure that seeking starts after playback when we have a gap at the beginning of the data

Comment 3

[Jean-Yves Avenard [:jya]]

Comment on attachment 8695843 [details] [diff] [review]
P1. Ensure seeked event is fired prior loadeddata.

ok.. it's not as simple as that patch made out to be.

As we can't call StartDecoding() if we have a pending firstframe event.

Comment 4

[Jean-Yves Avenard [:jya]]

Attachment: P1. Ensure seeked event is fired prior loadeddata.

We can't call StartDecoding() before FinishDecodeFirstFrame() as StartDecoding() expect a particular state when coming back from dormant.

I believe we could have changed StartDecoding() instead and detect that we're coming from SeekCompleted and that we're resuming from dormant, but I feel that the change of regression is too high as there are always some weird expected state in the MDSM.

Comment 5

[Jean-Yves Avenard [:jya]]

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

Comment 6

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

(In reply to Jean-Yves Avenard [:jya] from comment #0)
> From this reading, the first thing to do when the seek is complete is fire
> the "seeked" event.
> 
> Once the seek has completed, the readyState will move to HAVE_CURRENT_DATA
> and the loadeddata event will be fired.

I can't see why "loadeddata should be fired after seeked event" from the spec.

AFAIU, 'seeked' must be fired after step 12 which implies HAVE_CURRENT_DATA which happens before 'loadeddata'. I can't tell whether 'seeked' or 'loadeddata' should come first since they are partial orders.

Comment 7

[Jean-Yves Avenard [:jya]]

(In reply to JW Wang [:jwwang] from comment #6)

> I can't see why "loadeddata should be fired after seeked event" from the
> spec.
> 
> AFAIU, 'seeked' must be fired after step 12 which implies HAVE_CURRENT_DATA
> which happens before 'loadeddata'. I can't tell whether 'seeked' or
> 'loadeddata' should come first since they are partial orders.

Maybe you're right, though as I mentioned earlier I can see it differently. And seeing that's how the other browsers read it as well, we may as well do the same.

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/22f7f38e9a4f
https://hg.mozilla.org/integration/mozilla-inbound/rev/67052f57edb1

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/22f7f38e9a4f
https://hg.mozilla.org/mozilla-central/rev/67052f57edb1