Closed Bug 1230527 Opened 4 years ago Closed 4 years ago

loadeddata should be fired after seeked event

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox45 --- fixed

People

(Reporter: jya, Assigned: jya)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 1 obsolete file)

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)
Attachment #8695843 - Flags: review?(jwwang)
Assignee: nobody → jyavenard
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
Attachment #8695854 - Flags: review?(jwwang)
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.
Attachment #8695843 - Attachment is obsolete: true
Attachment #8695843 - Flags: review?(jwwang)
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.
Attachment #8696198 - Flags: review?(jwwang)
(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.
Flags: needinfo?(jyavenard)
(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.
Flags: needinfo?(jyavenard)
Attachment #8696198 - Flags: review?(jwwang) → review+
Attachment #8695854 - Flags: review?(jwwang) → review+
https://hg.mozilla.org/mozilla-central/rev/22f7f38e9a4f
https://hg.mozilla.org/mozilla-central/rev/67052f57edb1
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
You need to log in before you can comment on or make changes to this bug.