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

Status: VERIFIED FIXED

(Core :: Audio/Video, enhancement)

Description

[Rick Waldron [:rwaldron]]

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

Comment 1

[Matthew Gregan [:kinetik]]

Why do you want this?  Are you seeing problems with the performance of currentTime now?  If so, please supply a testcase.

Comment 2

[Rick Waldron [:rwaldron]]

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

Comment 3

[Rick Waldron [:rwaldron]]

Sorry, if that one times out, the test is here as well: http://code.bocoup.com/butter/issues/627139.html

Comment 4

[David Humphrey (:humph)]

This looks like lag setting/reading the value out of the decoder, and it looks like webkit is caching it to avoid that.

Comment 5

[Matthew Gregan [:kinetik]]

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."

Comment 6

[David Humphrey (:humph)]

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.

Comment 7

[Matthew Gregan [:kinetik]]

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.

Comment 8

[Matthew Gregan [:kinetik]]

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.

Comment 9

[Rick Waldron [:rwaldron]]

Apologies, and noted. Also, thanks for your time and help looking at this ticket, it's greatly appreciated.

Comment 10

[Matthew Gregan [:kinetik]]

Attachment: simple testcase, seeks to 20s

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.

Comment 11

[Rick Waldron [:rwaldron]]

That is consistent with the tests I've been running.

Comment 12

[Matthew Gregan [:kinetik]]

http://lists.whatwg.org/htdig.cgi/whatwg-whatwg.org/2011-January/029961.html

Comment 13

[Matthew Gregan [:kinetik]]

Thanks for reporting this, Rick!

Comment 14

[David Humphrey (:humph)]

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."

Comment 15

[Matthew Gregan [:kinetik]]

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.

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Let's do it.

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 18

[Matthew Gregan [:kinetik]]

Attachment: patch v0

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.

Comment 19

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+  // 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?

Comment 20

[Matthew Gregan [:kinetik]]

Attachment: patch v1

Split UpdatePlaybackPosition out so that an update-only-the-state-machine version is available.

Comment 21

[cajbir (:cajbir)]

http://hg.mozilla.org/mozilla-central/rev/d8fda3716eae

Comment 22

[u279076]

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?

Comment 23

[Matthew Gregan [:kinetik]]

(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.

Comment 24

[u279076]

Verified fixed with Firefox 4.0b12pre (20110213)