The fastSeek() method was added in bug 778077. I'm opening this followup bug because our implementation of fastSeek always seeks to the key frame before the specified target time, in violation of the spec. For the sake of example, suppose I'm at 00:25 in a video that has key frames at 00:20 and 00:30. I try to seek ahead 4 seconds to 00:29. But fastSeek(), as currently implemented will take me to 00:20. So even though I dragged the scrubber forward, I ended up going backwards in the movie. In addition to being really confusing to the user, this violates the WHATWG spec. See paragraph 9 of http://www.whatwg.org/specs/web-apps/current-work/multipage/the-video-element.html#dom-media-fastseek which says: > 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 suspect that "keyframe before the target" position is the correct choice in all cases except when it results in us going in the wrong direction. I'd love to get this fixed in Gecko ASAP. We've commited to using fastSeek in FirefoxOS 2.0, and we need to land Gaia patches to do that today. Unfortunately, we'll probably have to include JS code to workaround this bug by following fastSeek with a slow seek if the seek goes in the wrong direction. This bug only really manifests in videos that have sparse keyframes. One of our sample videos in Gaia is like that however. See $GAIA/test_media/Movies/elephants-dream.webm

Needinfo Chris since he did the original fastSeek() implementation

I agree this is bad UX. We could detect inside Gecko's seeking code when we were doing a fast seek forward and ended up behind the seek target, and then decode forwards to the seek target. This would basically convert a fastSeek into a slow seek, but that's about the best we could do here I think.

This testcase slowSeeks elephant's dream to 44s, then fastSeeks to 50. In Firefox Nightly Desktop builds, we end at t=31s (though you have to play the video to see the timeupdate to 31), which is the undesirable case djf is mentioning here.

djf: Can you please test this patch and check if it gives you the desired behaviour? Thanks. It seems to do so for me.

(In reply to Chris Pearce (:cpearce) from comment #4) > Created attachment 8437302 [details] [diff] [review] > Patch > > djf: Can you please test this patch and check if it gives you the desired > behaviour? Thanks. It seems to do so for me. Chris: I will try to test this, but don't hold your breath. I made the mistake of reconfiguring my B2G repo for the flame, and 20 hours later I now have to deal with the case-sensitive file system issue. I expect hours more of lost productivity before I have a gecko build that I can push to a phone.

Comment on attachment 8437302 [details] [diff] [review] Patch This patch solves the problem for the Gaia video app and sample video that does not have enough keyframes. Without the patch I could get stuck at 0:43 and clicking the button to skip ahead 10 seconds would put me right back at 0:43. With this patch, when I click the button to skip ahead 10 seconds, nothing happens for about 6 seconds but then I do indeed skip ahead 10 seconds. So it is still slow and annoying, but at least not completely broken. I'd recommend filing a followup bug to fix this right. Ideally, we would just skip to the next key frame in this situation and not have to do the slow seek at all. Also, fastSeek() still does not correctly implement the spec because it is supposed to seek to the closest key frame before or after the target time, but this implementation always goes to the key frame before the time instead of considering the one afterward. For now, this workaround should be good enough. Note that this bug is blocking 984576 which is a bug we commited to fix for FirefoxOS 2.0

Patch as above, with a mochitest.

Comment on attachment 8441076 [details] [diff] [review] Patch with test Review of attachment 8441076 [details] [diff] [review]: ----------------------------------------------------------------- r+ with changes. ::: content/media/MediaDecoderStateMachine.cpp @@ +1963,5 @@ > { > ReentrantMonitorAutoExit exitMon(mDecoder->GetReentrantMonitor()); > video = mReader->FindStartTime(nextSampleStartTime); > } > + Remove whitespace. @@ +1968,5 @@ > + if (seekTime > mediaTime && > + nextSampleStartTime < mediaTime && > + mSeekTarget.mType == SeekTarget::PrevSyncPoint) { > + // We we're doing a fastSeek, and we ended up *before* the previous > + // playback position. This is surprising UX, so switch to an accurate You should mention this is not per spec rather than surprising UX, if it is indeed not per spec. @@ +1969,5 @@ > + nextSampleStartTime < mediaTime && > + mSeekTarget.mType == SeekTarget::PrevSyncPoint) { > + // We we're doing a fastSeek, and we ended up *before* the previous > + // playback position. This is surprising UX, so switch to an accurate > + // seek and decode to the seek target. Mention that this behaviour is not per spec but implemented as a stop gap. Reference followup bug. ::: content/media/test/test_fastSeek-forwards.html @@ +1,4 @@ > +<!DOCTYPE HTML> > +<html> > +<!-- > +https://bugzilla.mozilla.org/show_bug.cgi?id=778077 Bug number is wrong. This references the original bug. Please reference the current bug as well. @@ +4,5 @@ > +https://bugzilla.mozilla.org/show_bug.cgi?id=778077 > +--> > +<head> > + <meta charset="utf-8"> > + <title>Test for Bug 778077</title> Bug number is wrong. This references the original bug. Please reference the current bug as well. @@ +11,5 @@ > + <script type="text/javascript" src="manifest.js"></script> > + <script type="application/javascript"> > + > + // Test that if we're after the closest keyframe and we seek forwards, that > + // we don't end up seeking backwards. We should actually/arguably seek to the Is arguably the right term for something that needs to be done per spec? @@ +12,5 @@ > + <script type="application/javascript"> > + > + // Test that if we're after the closest keyframe and we seek forwards, that > + // we don't end up seeking backwards. We should actually/arguably seek to the > + // keyframe after the target, but we don't implement this yet. Please file followup bug for the additional work and reference it here. @@ +20,5 @@ > + var v = event.target; > + ok(v.currentTime >= v.firstSeekTarget, v.name + " seek never go backwards. time=" + v.currentTime + " firstSeekTarget=" + v.firstSeekTarget + " secondSeekTarget=" + v.secondSeekTarget); > + manager.finished(v.token); > + }; > + Remove whitespace. @@ +24,5 @@ > + > + var onFirstSeekComplete = function(event) { > + var v = event.target; > + v.removeEventListener("seeked", onFirstSeekComplete); > + // Seek to 3/4 of the way between the start and the first keyframe Change '3/4' to 75%. @@ +26,5 @@ > + var v = event.target; > + v.removeEventListener("seeked", onFirstSeekComplete); > + // Seek to 3/4 of the way between the start and the first keyframe > + // using fastSeek. We then test that the currentTime doesn't drop back > + // to the previous keyframe, curretTime should go forwards. Change 'curretTime' to 'currentTime'. @@ +31,5 @@ > + v.addEventListener("seeked", onSecondSeekComplete); > + v.secondSeekTarget = v.keyframes[1] * 0.75; > + v.fastSeek(v.secondSeekTarget); > + } > + Remove whitespace. @@ +60,5 @@ > + > + </script> > +</head> > +<body> > +<a target="_blank" href="https://bugzilla.mozilla.org/show_bug.cgi?id=778077">Mozilla Bug 778077</a> Check bug number as mentioned previously.

(In reply to cajbir (:cajbir) from comment #8) > @@ +1968,5 @@ > > + if (seekTime > mediaTime && > > + nextSampleStartTime < mediaTime && > > + mSeekTarget.mType == SeekTarget::PrevSyncPoint) { > > + // We we're doing a fastSeek, and we ended up *before* the previous > > + // playback position. This is surprising UX, so switch to an accurate > > You should mention this is not per spec rather than surprising UX, if it is > indeed not per spec. We're making this change because seeking backwards when you try to seek forwards is surprising UX. The spec just says that fastSeek should be fast, it doesn't specify whether to snap to keyframe before or after the seek target. We concluded that we're better to seek to the keyframe after the seek target in this case, and I filed bug 1026330 to cover that.

Landed with review addressed: https://hg.mozilla.org/integration/mozilla-inbound/rev/67f48cc4e6c9

(In reply to Chris Pearce (:cpearce) from comment #9) > The spec just says that fastSeek should be fast, it doesn't specify whether > to snap to keyframe before or after the seek target. Is comment 0 incorrect then about it being a spec violation?

That's my reading of the spec.

(In reply to Chris Pearce (:cpearce) from comment #10) > Landed with review addressed: > https://hg.mozilla.org/integration/mozilla-inbound/rev/67f48cc4e6c9 Dear Chris： Does this fix-patch landed in v2.1？ because when playing 720p video in video app, when doing a fastseek, the video's play-head just skip, this is bad user experience.

Blake: Would you be able to answer Lin Hui's question in comment 14 please? Thanks!

Sorry Blake, yes I can answer this. Yes, this change made it into 2.1, but 2.1 include "async decoding", so the MediaDecoderStateMachine code changed a lot in 2.1, so it looks different. The code corresponding to https://hg.mozilla.org/integration/mozilla-inbound/rev/67f48cc4e6c9 in B2G2.1 is: https://github.com/mozilla/gecko-dev/blob/b2g34_v2_1/content/media/MediaDecoderStateMachine.cpp#L769

(In reply to Chris Pearce (:cpearce) from comment #16) > Yes, this change made it into 2.1, but 2.1 include "async decoding", so the > MediaDecoderStateMachine code changed a lot in 2.1, so it looks different. > > The code corresponding to > https://hg.mozilla.org/integration/mozilla-inbound/rev/67f48cc4e6c9 in > B2G2.1 is: > https://github.com/mozilla/gecko-dev/blob/b2g34_v2_1/content/media/ > MediaDecoderStateMachine.cpp#L769 Thanks for your reply. But when Playing 720p video, doing a fast seek, the play-head jump to rewind or forward, it's not accurate. I understanding that the fast seek may seek to the keyframe, so this may cause a small fluctuate， in most HD video, sometimes it will fluctuate 10 seconds, so cause the bad user experience. This issue happened both in flame_2.1, 7715ea_android & dolphin, could you help to find someone to check this issue? Thanks very much!

(In reply to lin.hui@spreadtrum.com from comment #17) > (In reply to Chris Pearce (:cpearce) from comment #16) > > Yes, this change made it into 2.1, but 2.1 include "async decoding", so the > > MediaDecoderStateMachine code changed a lot in 2.1, so it looks different. > > > > The code corresponding to > > https://hg.mozilla.org/integration/mozilla-inbound/rev/67f48cc4e6c9 in > > B2G2.1 is: > > https://github.com/mozilla/gecko-dev/blob/b2g34_v2_1/content/media/ > > MediaDecoderStateMachine.cpp#L769 > > Thanks for your reply. > > But when Playing 720p video, doing a fast seek, the play-head jump to rewind > or forward, it's not accurate. > > I understanding that the fast seek may seek to the keyframe, so this may > cause a small fluctuate， in most HD video, sometimes it will fluctuate 10 > seconds, so cause the bad user experience. > > This issue happened both in flame_2.1, 7715ea_android & dolphin, could you > help to find someone to check this issue? > > Thanks very much! Actually this is not an issue. It is a current design and also a tradeoff. AFAIK, many Android phones also seek to the keyframe for "better" user experience to quickly see the decoded frame instead of waiting the accurate sought frame. If you really want to do that (accurate seek), you can set currentTime[1] from VideoAPP instead of using fastSeek API. [1]https://html.spec.whatwg.org/multipage/embedded-content.html#dom-media-currenttime

(In reply to Blake Wu [:bwu][:blakewu] from comment #18) > Actually this is not an issue. It is a current design and also a tradeoff. > AFAIK, many Android phones also seek to the keyframe for "better" user > experience to quickly see the decoded frame instead of waiting the accurate > sought frame. > If you really want to do that (accurate seek), you can set currentTime[1] > from VideoAPP instead of using fastSeek API. > > [1]https://html.spec.whatwg.org/multipage/embedded-content.html#dom-media- > currenttime I agree with you, if we don not have fastseek, when you doing seeking, it will bring stuck issue, and I didn't care about this fastseek may have some skip, but our tester is hard to get along with, BTW, I won't fix that skip frame issue too. Thanks!