currentTime reports "old" value after seeking until "seeking" is fired

VERIFIED FIXED

Status

()

enhancement
VERIFIED FIXED
9 years ago
9 years ago

People

(Reporter: rwaldron, Assigned: kinetik)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(blocking2.0 -)

Details

(Whiteboard: [popcorn.js])

Attachments

(2 attachments, 1 obsolete attachment)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US) AppleWebKit/534.16 (KHTML, like Gecko) Chrome/10.0.634.0 Safari/534.16
Build Identifier: Gecko/20101203 Firefox/3.6.13 ( .NET CLR 3.5.30729)

Make it possible for HTMLMediaElement to cache the movie time and report 'currentTime' as [cached time + elapsed wall time]. The media engine returns the maximum duration it is safe to calculate time before resampling the actual movie time with the new maximumDurationToCacheMovieTime method. Because this may be different for different media engines the default return value is 0, making it an opt-in feature.


https://bugs.webkit.org/show_bug.cgi?id=49009

Reproducible: Always
Status: UNCONFIRMED → NEW
Ever confirmed: true
Version: unspecified → Trunk
Whiteboard: [popcorn.js]
Why do you want this?  Are you seeing problems with the performance of currentTime now?  If so, please supply a testcase.
There is a latency between the moment you set the currentTime  and when you can next reliably get the currentTime 

http://dl.dropbox.com/u/3531958/627139.html
Sorry, if that one times out, the test is here as well: http://code.bocoup.com/butter/issues/627139.html
This looks like lag setting/reading the value out of the decoder, and it looks like webkit is caching it to avoid that.
Okay, that has nothing to do with comment 0 and the WebKit bug, as far as I can tell.  You're seeking, and then expecting currentTime to immediately report the value you set it to?

Referring to:
http://www.whatwg.org/specs/web-apps/current-work/multipage/video.html#seeking

Note that currentTime becomes the new value just before seeking is fired, and that this happens after step 4, which says "The remainder of these steps must be run asynchronously."
So other browsers seem to be caching the time in the media element and then invalidating it when the decoder is ready to report the new time.  This means that you can elem.currentTime = X and then read back X without waiting on the events.

I do see what you're saying looking at the spec.  I just know that in the wild it's working differently.
I think immediately reporting the new currentTime after setting it would be
more intuitive, but it will require a spec change to get consistent behaviour
across browsers.  I'll bring it up on WhatWG today.
Summary: Make it possible for HTMLMediaElement to cache the movie time and report 'currentTime' as [cached time + elapsed wall time] → currentTime reports "old" value after seeking until "seeking" is fired
For future reference, it's easier to deal with bugs if you are clear about the behaviour you're seeing and the behaviour you expect, rather than requesting a change be made without explaining why.
Apologies, and noted. Also, thanks for your time and help looking at this ticket, it's greatly appreciated.
Opera 11, Chrome 10, Safari 5, and IE9 beta reports:
before setting currentTime=0
after setting currentTime=20
event=seeking currentTime=20
event=seeked currentTime=20

Firefox 4 reports:
before setting currentTime=0
after setting currentTime=0
event=seeking currentTime=20
event=seeked currentTime=20

Firefox 3.6 reports:
before setting currentTime=0
after setting currentTime=0
event=seeking currentTime=0
event=seeked currentTime=19.999000549316406

So we're certainly the odd duck out.
That is consistent with the tests I've been running.
Thanks for reporting this, Rick!
Further info from the list, also referencing the bug Rick posted above:

"In WebKit this happens because currentTime isn't maintained in HTMLMediaElement (modulo the caching added in https://bugs.webkit.org/show_bug.cgi?id=49009), it is whatever the media engine (QuickTime, GStreamer, etc) reports. When currentTime is set the media engine is asked to seek immediately so the asynchronous section may run in parallel to the script, and therefore the seek may actually have completed by the time you check currentTime."
We'll change the behaviour to match other implementations: http://lists.whatwg.org/htdig.cgi/whatwg-whatwg.org/2011-January/030031.html

This probably needs to block, since it'll be better for users if we change this as soon as possible so we only have two different Firefox behaviours (pre-4.0 and post-4.0) rather than allowing 4.0 and post-4.0 to differ.
blocking2.0: --- → ?
Let's do it.
Assignee: nobody → kinetik
blocking2.0: ? → final+
Actually, let's not block on this, because if we release FF4 like this, it's not really bad. But I'd like to see this fixed for FF4 and I'll approve a patch.
blocking2.0: final+ → -
Posted patch patch v0 (obsolete) — Splinter Review
Update the decoder's concept of current time immediately when requested to seek.

It turns out that we don't implement step 2 of the seek algorithm correctly, where an in-flight seek should be cancelled when another is requested.  This interacts badly with reporting the new currentTime immediately, so I've altered our behaviour (and tests) to match the spec.

I'm not cancelling the underlying reader's seek at this stage, just hiding the result from the user (that is, not firing "seeked").  If this turns out to be a problem, we can look at making the blocking seek call cancellable.

Tests pass locally and on tryserver.
Attachment #508527 - Flags: review?(roc)
+  // Update the playback position. This can result in a timeupdate event and
+  // an invalidate of the frame being dispatched asynchronously if there is
+  // no such event currently queued and the position update is an external
+  // update.

You need to explain what the difference is between an "internal" and "external" update.

+  virtual void UpdatePlaybackPosition(PRInt64 aTime, PRBool aInternalUpdate = PR_FALSE) = 0;

Boolean parameters suck. How about just splitting out a new function "UpdateInternalPlaybackPosition" and calling it from the right place?
Posted patch patch v1Splinter Review
Split UpdatePlaybackPosition out so that an update-only-the-state-machine version is available.
Attachment #508527 - Attachment is obsolete: true
Attachment #508647 - Flags: review?(roc)
Attachment #508527 - Flags: review?(roc)
Whiteboard: [popcorn.js] → [popcorn.js][needs landing]
http://hg.mozilla.org/mozilla-central/rev/d8fda3716eae
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Whiteboard: [popcorn.js][needs landing] → [popcorn.js]
Can someone please explain to me what I should see in both of the testcases so I can verify this bug:
http://dl.dropbox.com/u/3531958/627139.html
http://code.bocoup.com/butter/issues/627139.html

Thanks

Also, is this sufficiently covered by test automation or is a Litmus test desired?
(In reply to comment #22)
> http://dl.dropbox.com/u/3531958/627139.html

Should display:
video.currentTime === 0, 0, PASS
video.currentTime === 25, 25, PASS

in the page.

> http://code.bocoup.com/butter/issues/627139.html

Ignore the "Firefox (4b) 1/2" text, that's just static text in a <pre>.

Should display:
["video.currentTime === 0", 0, "PASS"]
["video.currentTime === 25", 25, "PASS"]

in the console.  Note that it doesn't appear in the included Firebug-lite console, but it does appear in the Web Console if you have it open.

> Also, is this sufficiently covered by test automation or is a Litmus test
> desired?

It's pretty well covered by the mochitests in content/media/test.
Verified fixed with Firefox 4.0b12pre (20110213)
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.