481106 - Scrubber may not be positioned at end of buffer bar when playback ends

Status: VERIFIED FIXED

(Toolkit :: Video/Audio Controls, defect)

Description

[Matthew Gregan [:kinetik]]

The code in for dealing with timeupdate events in videocontrols.xml#handleEvent does not move the scrubber if the media's currentTime has changed by less than 333ms since the last timeupdate event.  This can result in the scrubber's last position update being quite far from the end of the buffer bar when playing back short files--this is most obvious with Wave files.  See the file linked in the URL for an example.

I think the ended event should move the scrubber to the end of the buffer bar.

(I originally thought of making the code to discard frequent timeupdate events handle the special case of currentTime being within an epsilon of the duration and allowing the scrubber to move in that case.  But we might not know the duration, so we can't do that.)

Comment 1

[Matthew Gregan [:kinetik]]

Attachment: wip

So, this seems like the obvious fix... but after the scrubber reaches the end, something causes it to reset to the beginning.

Comment 2

[Justin Dolske [:Dolske]]

Taking this to get it on my radar for 3.5, but feel free to steal back if you really want to. :)

Comment 3

[Justin Dolske [:Dolske]]

Attachment: Short ogg clip

Easy to see the problem with this clip, it's short but wide, and the thumb usually stops a dozen pixels or so from the end.

Comment 4

[Justin Dolske [:Dolske]]

Attachment: Patch v.1

Comment 5

[Justin Dolske [:Dolske]]

(In reply to comment #1)

> but after the scrubber reaches the end,
> something causes it to reset to the beginning.

Two (!) bugs: I've found that on some videos, the time index of the last frame is -1. Seems like a core bug, it shouldn't ever be < 0. Additionally, your patch was feeding the raw currentTime value to showPosition(), when it actually expects milliseconds. So that would always make it jump to nearly the beginning.

Comment 7

[Dave Townsend [:mossop]]

Seems like a pretty visible problem given how easy it is to reproduce in bug 493523

Comment 8

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

(In reply to comment #5)
> Two (!) bugs: I've found that on some videos, the time index of the last frame
> is -1. Seems like a core bug, it shouldn't ever be < 0.

Probably bug 493431 for which I just landed a patch.

Comment 9

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

Wouldn't block release on this, would still like a patch.

Comment 10

[Justin Dolske [:Dolske]]

Attachment: Patch v.2

Updated patch, no change, just fix merge conflict with bug 492213 which will likely land before this.

Comment 11

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 378478 [details] [diff] [review]
Patch v.2

Since showPosition is always called with the same parameters (which aren't used elsewhere in the calling code), it seems to me like it would make the most sense to factor that code out a bit more and just have a showPosition() that takes a single argument indicating whether or not it should throttle based on the last lastTimeUpdate.

Comment 12

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 378478 [details] [diff] [review]
Patch v.2

a191=beltzner

Won't hold release for this, but should be super-safe. Make sure it bakes for a cycle on mozilla-central before you move it to 1.9.1

File a follow-up for tests?

Comment 13

[Justin Dolske [:Dolske]]

Pushed http://hg.mozilla.org/mozilla-central/rev/051f97ebed89

Comment 14

[Justin Dolske [:Dolske]]

Pushed to 191: http://hg.mozilla.org/releases/mozilla-1.9.1/rev/59d2e9c32a81

Comment 15

[Aakash Desai [:aakashd]]

verified FIXED on builds:

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090527 Minefield/3.6a1pre ID:20090527031500

and

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1pre) Gecko/20090527 Shiretoko/3.5pre ID:20090527031214