1193124 - After fastSeek currentTime remains at seek target, not seek destination

Status: RESOLVED FIXED

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

Description

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

If you call HTMLMediaElement.fastSeek(x), and the MDSM ends up seeking to the keyframe at time t, HTMLMediaElement.currentTime will have value x rather than t after the seek.

You can observe this by either adding logging to test_fastSeek to dump currentTime in the "seeked" handler, or just calling fastSeek(t) on a paused video element; when you play after the seek you'll see the currentTime jump back slightly as the currentTime updates to the playback position of the keyframe.

Comment 1

[Ralph Giles (:rillian)]

Is this fixed?

Comment 2

[Jean-Yves Avenard [:jya]]

no...

The issue is in the spec, and is commented there: https://dxr.mozilla.org/mozilla-central/source/dom/html/HTMLMediaElement.cpp#1577

https://html.spec.whatwg.org/multipage/embedded-content.html#the-video-element
"If the approximate-for-speed flag is set, adjust the new playback position to a value that will allow for playback to resume promptly. If new playback position before this step is before current playback position, then the adjusted new playback position must also be before the current playback position. Similarly, if the new playback position before this step is after current playback position, then the adjusted new playback position must also be after the current playback position."

I don't see how this can be implemented in our current asynchronous API.

We could always tweak around it such as setting currentTime to the target value first, and then adjusted it prior firing the "seeked" event so if you read currentTime then you get the right value.

JW got some ideas on how this could be done?

Comment 3

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

It looks like you are talking about a different issue than what Chris said in comment 0.

The issue of comment 0 is currentTime() should return t instead of x in the 'seeked' handler.

However, the spec issue (https://dxr.mozilla.org/mozilla-central/source/dom/html/HTMLMediaElement.cpp#1577) implies currentTime() should return t in the 'seeking' handler.

I have no idea how the spec issue can be resolved. However, the issue in comment 0 seems a bug in MDSM to me.

Comment 4

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

The issue in comment 0 is gone if we call play() on the media element in test_fastSeek.html.
10 INFO TEST-PASS | dom/media/test/test_fastSeek.html | gizmo.mp4 seekTo=1.5 currentTime (1.5) should be close to seek target initially 
11 INFO TEST-PASS | dom/media/test/test_fastSeek.html | gizmo.mp4 seekTo=1.5 currentTime (0.981333) should be end up roughly after keyframe (1) after fastSeek 
12 INFO TEST-PASS | dom/media/test/test_fastSeek.html | gizmo.mp4 seekTo=1.5 currentTime (0.981333) should be end up less than target after fastSeek 

Without calling play():
0 INFO TEST-PASS | dom/media/test/test_fastSeek.html | gizmo.mp4 seekTo=1.5 currentTime (1.5) should be close to seek target initially
11 INFO TEST-PASS | dom/media/test/test_fastSeek.html | gizmo.mp4 seekTo=1.5 currentTime (1.5) should be end up roughly after keyframe (1) after fastSeek
12 INFO TEST-PASS | dom/media/test/test_fastSeek.html | gizmo.mp4 seekTo=1.5 currentTime (1.5) should be end up less than target after fastSeek

Hi Chris,
Is this the exact problem you described in comment 0 that needs to be fixed?

Comment 5

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

The issue is as JW has stated: the currentTime should reflect where the seek terminated by the time the "seeked" handler runs. In fact, ideally it should reflect where the seek will terminate by the time the "seeking" handler runs, to match the behaviour of a non-fast seek.

The reason the issue "disappears" if the video is playing is because since we're playing the current playback position will update as playback advances after the seek, but before the "seeked" handler has had a chance to run.

So no, I don't think the issue is fixed.

The current spec is here:
https://html.spec.whatwg.org/multipage/embedded-content.html#dom-media-fastseek

Everything after step 5 onwards runs in parallel to the script that called fastSeek().

I think the comment at https://dxr.mozilla.org/mozilla-central/source/dom/html/HTMLMediaElement.cpp#1577 is incorrectly assuming that JS should observer the currentTime being set to the "new playback position" (the keyframe time) inside the fastSeek() function call. But as the spec is currently written, we can update the current playback position in parallel as the seek progresses; in step 11 of the fastSeek() method definition.

I think it makes sense to change MediaDecoderStateMachine::InitiateSeek() to somehow figure out where the fastSeek will end up, and pass that to its UpdatePlaybackPositionInternal() call so that the before  the MediaDecoder::SeekingStarted observer runs we'll have updated the playback position to where the seek will end up before the "seeking" event runs. Note I'm saying "seeking" here, not "seeked".

Probably that means we'll need to break up MediaDecoderStateMachine::InitiateSeek() into (asynchronously?) asking the reader where the seek will end up, and then in a continuation passing that to UpdatePlaybackPositionInternal() and notifying mOnSeekingStart before doing the seek.

We should also file a spec issue to swap steps 10 and 11 so that it's clear in the spec that the current playback position should be updated before the "seeking" event runs.

I also note that Safari is the only other browser to implement fastSeek.

Comment 6

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

Attachment: 1193124_ensure_update_position.patch

Ensure logical position is updated after seek. But this patch breaks test_fastSeek.html for bug 1239182 is not yet fixed.

Comment 7

[Tzuhao Kuo [:kaku]]

NextFrameSeekTask needs to base on this patch because that the seek target time is unknown before we invoke a NextFrameSeekTask and so we should update the target time after the NextFrameSeekTask is done.

Comment 8

[Tzuhao Kuo [:kaku]]

I rebased this patch onto the current MC and here is the try result: https://treeherder.mozilla.org/#/jobs?repo=try&revision=01fd72bea548&filter-tier=1&group_state=expanded.

The test_fastSeek.html no longer fails, but something wrong in the Windows..., will check it.

Comment 9

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

Thanks for taking time in this bug. Can I assign the bug to you?

Comment 10

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

Per discussion offline.

Comment 11

[Tzuhao Kuo [:kaku]]

Quick update about the try failure.

test_bug465498.html loads a media source, play it to the end and then set its currentTime to 0. While receiving the 'seeked' event, check the media's currentTime equals to 0 or not. 

The test case fails on small-shot.mp3 and small-shot-mp3.mp4 in the Windows platform. Takes small-shot.mp3 as example, the audio's 1st sample starts from 26122 with duration 26122, and the 2nd sample start form 52244 with duration 26122, and so on. Set the audio's currentTime to 0 leads to two operations:
(1st) MDSM invokes MediaFormatReader's Seek operation with target 26122. This one works well.
(2nd) MDSM invokes MediaFormatReader's RequestAudioData() operation which returns the 2nd example starting form 52244. However, the expected result is the 1st sample starting from 26122 which will be adjusted to 0. 

Will open a separate bug for this issue.

Comment 12

[Tzuhao Kuo [:kaku]]

Bug 1272267 is resolved and in the m-c now. Rebase this patch onto it and push it to MozReview.

Comment 13

[Tzuhao Kuo [:kaku]]

Attachment: MozReview Request: Bug 1193124 - ensure logical position is updated after seek. r=jwwang.

Review commit: https://reviewboard.mozilla.org/r/54520/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/54520/

Comment 14

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

Comment on attachment 8755285 [details]
MozReview Request: Bug 1193124 - ensure logical position is updated after seek. r=jwwang.

Looks fine to me, but JW is pretty much the owner of MediaDecoder* now, so he should review this.

Comment 15

[Tzuhao Kuo [:kaku]]

Comment on attachment 8755285 [details]
MozReview Request: Bug 1193124 - ensure logical position is updated after seek. r=jwwang.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54520/diff/1-2/

Comment 16

[Tzuhao Kuo [:kaku]]

Thanks for the review!

Comment 17

[Tzuhao Kuo [:kaku]]

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

try looks good.

Comment 18

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fff23fd67c6e

Comment 19

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

Comment on attachment 8755285 [details]
MozReview Request: Bug 1193124 - ensure logical position is updated after seek. r=jwwang.

https://reviewboard.mozilla.org/r/54520/#review51670

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/fff23fd67c6e