Bug 42178 - Update media element's seeking logic
Summary: Update media element's seeking logic
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC All
: P2 Normal
Assignee: Eric Carlson
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2010-07-13 10:58 PDT by Eric Carlson
Modified: 2010-07-24 14:43 PDT (History)
4 users (show)

See Also:


Attachments
WIP patch (6.61 KB, patch)
2010-07-13 14:02 PDT, Eric Carlson
no flags Details | Formatted Diff | Diff
proposed patch. (7.08 KB, patch)
2010-07-13 14:29 PDT, Eric Carlson
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Carlson 2010-07-13 10:58:23 PDT
The logic in HTMLMediaElement::seek is out of sync with the current spec.
Comment 1 Eric Carlson 2010-07-13 11:09:39 PDT
<rdar://problem/8185817>
Comment 2 Eric Carlson 2010-07-13 14:02:57 PDT
Created attachment 61413 [details]
WIP patch
Comment 3 Eric Carlson 2010-07-13 14:29:04 PDT
Created attachment 61416 [details]
proposed patch.
Comment 4 Darin Adler 2010-07-13 15:20:44 PDT
Comment on attachment 61416 [details]
proposed patch.

> +        if (abs(startTime - time) < closest)
> +            closest = abs(startTime - time);
> +        else if (abs(endTime - time) < closest)
> +            closest = abs(endTime - time);

The function used here should be fabsf, not abs. Calling abs converts the number to an int!

Also, I am concerned about the ignored ExceptionCode. Why is it OK to ignore it? Can there be an exception.

r=me but please fix that fabsf thing
Comment 5 Eric Carlson 2010-07-14 07:41:51 PDT
(In reply to comment #4)
> (From update of attachment 61416 [details])
> > +        if (abs(startTime - time) < closest)
> > +            closest = abs(startTime - time);
> > +        else if (abs(endTime - time) < closest)
> > +            closest = abs(endTime - time);
> 
> The function used here should be fabsf, not abs. Calling abs converts the number to an int!
> 
Ack, I assume it would use the "inline float" version in cmath. Thanks!

> Also, I am concerned about the ignored ExceptionCode. Why is it OK to ignore it? Can there be an exception.
> 
An exception is returned by start() and end() if the index is out of bounds, which can't happen here.

> r=me but please fix that fabsf thing
Thanks!
Comment 6 Eric Carlson 2010-07-14 07:42:17 PDT
http://trac.webkit.org/changeset/63319
Comment 7 WebKit Review Bot 2010-07-14 07:42:38 PDT
http://trac.webkit.org/changeset/63319 might have broken Qt Linux Release minimal
Comment 8 Jakub Wieczorek 2010-07-24 14:43:26 PDT
(In reply to comment #6)
> http://trac.webkit.org/changeset/63319

Are you sure this is correct? Aren't you returning the distance between time and the nearest position rather than the nearest position?

And this:
+ else if (fabs(endTime - time) < closest)
looks wrong. This shouldn't be an else if, endTime might be closer than startTime.

You could save some function calls and unnecessary input checks by operating on the private vector instead of calling the public functions.