Bug 55217 - [GStreamer] Frame accurate seeking isn't always accurate
Summary: [GStreamer] Frame accurate seeking isn't always accurate
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: 528+ (Nightly build)
Hardware: All OS X 10.6
: P2 Normal
Assignee: Nobody
URL: http://www.massive-interactive.nl/htm...
Keywords: HTML5
Depends on:
Blocks:
 
Reported: 2011-02-25 07:13 PST by Philippe Normand
Modified: 2011-03-16 01:53 PDT (History)
9 users (show)

See Also:


Attachments
proposed patch (6.28 KB, patch)
2011-02-25 07:23 PST, Philippe Normand
no flags Details | Formatted Diff | Diff
proposed patch (6.28 KB, patch)
2011-02-25 07:27 PST, Philippe Normand
no flags Details | Formatted Diff | Diff
proposed patch (4.30 KB, patch)
2011-03-10 07:59 PST, Philippe Normand
mrobinson: review-
Details | Formatted Diff | Diff
proposed patch (4.17 KB, patch)
2011-03-15 02:20 PDT, Philippe Normand
mrobinson: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Philippe Normand 2011-02-25 07:13:42 PST
+++ This bug was initially created as a clone of Bug #52697 +++

The current GStreamer MediaPlayer backend doesn't use frame accurate seeking. In addition to using the GST_SEEK_FLAG_ACCURATE flag it should support a frame step event when possible (positive playback rate, frame requested is ahead of current playback position).
Comment 1 Philippe Normand 2011-02-25 07:23:04 PST
Created attachment 83807 [details]
proposed patch
Comment 2 WebKit Review Bot 2011-02-25 07:26:16 PST
Attachment 83807 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:552:  Missing spaces around |  [whitespace/operators] [3]
Total errors found: 1 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Philippe Normand 2011-02-25 07:27:20 PST
Created attachment 83808 [details]
proposed patch
Comment 4 Philippe Normand 2011-03-01 00:42:21 PST
Hi Sebastian,

Can you have a look at this patch when you get time? I did my tests with the website linked to that bug.

Thanks in advance for any feedback :)
Comment 5 Philippe Normand 2011-03-09 09:08:27 PST
Comment on attachment 83808 [details]
proposed patch

Pulling out of review, I have a new patch, simpler, without using the step event. Will upload it soon :)
Comment 6 Philippe Normand 2011-03-10 07:59:20 PST
Created attachment 85331 [details]
proposed patch
Comment 7 Martin Robinson 2011-03-14 15:29:56 PDT
Comment on attachment 85331 [details]
proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=85331&action=review

Looks good to me, though I think the time calculation logic could be cleaned up a bit to make it clearer.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:530
> +    glong microSeconds;
> +    float microSecondsf = modf(time, &seconds);
> +    microSecondsf *= 1000000;
> +    microSeconds = static_cast<glong>(roundf(microSecondsf / 10000) * 10000);
> +    timeValue.tv_sec = static_cast<glong>(seconds);
> +    timeValue.tv_usec = microSeconds;
> +
> +    GstClockTime clockTime = GST_TIMEVAL_TO_TIME(timeValue);
> +    LOG_VERBOSE(Media, "Seek: %" GST_TIME_FORMAT, GST_TIME_ARGS(clockTime));

I think you should probably just dispense with the microseconds variable altogether and rename microsecondsf to microseconds.

float seconds;
float microseconds = modf(time, &seconds) * 1000000;
timeValue.tv_sec = static_cast<glong>(seconds);
timeValue.tv_usec = static_cast<glong>(roundf(microSecondsf / 10000) * 10000);

I'm still a bit confused by this logic, because you multiply microsecondsf by 1000000 and then immediately divide by 10000.
Comment 8 Philippe Normand 2011-03-14 16:13:02 PDT
(In reply to comment #7)
> 
> I think you should probably just dispense with the microseconds variable altogether and rename microsecondsf to microseconds.
> 

Agreed.

> float seconds;
> float microseconds = modf(time, &seconds) * 1000000;
> timeValue.tv_sec = static_cast<glong>(seconds);
> timeValue.tv_usec = static_cast<glong>(roundf(microSecondsf / 10000) * 10000);
> 
> I'm still a bit confused by this logic, because you multiply microsecondsf by 1000000 and then immediately divide by 10000.

An example of time is 2.155999 seconds. modf() will return 0.155999 seconds. So to get the microseconds value we multiply by 1000000.

The division and multiplication are used to round. If microseconds is something like 155999:

roundf(15.5999) * 10000 = 160000 microseconds

I'll have another look at this tomorrow morning, thanks for the review anyway :)
Comment 9 Philippe Normand 2011-03-15 02:20:21 PDT
Created attachment 85789 [details]
proposed patch
Comment 10 Martin Robinson 2011-03-15 08:01:56 PDT
Comment on attachment 85789 [details]
proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=85789&action=review

Looks good to me!

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:524
> +    GTimeVal timeValue;
> +    float seconds;
> +    float microSeconds = modf(time, &seconds) * 1000000;
> +    timeValue.tv_sec = static_cast<glong>(seconds);
> +    timeValue.tv_usec = static_cast<glong>(roundf(microSeconds / 10000) * 10000);

Super nit here, but do you mind moving GTimeVal timeValue right down to where you first use it?
Comment 11 Philippe Normand 2011-03-16 01:53:02 PDT
Committed r81228: <http://trac.webkit.org/changeset/81228>