Bug 55217 – [GStreamer] Frame accurate seeking isn't always accurate

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Philippe Normand]

+++ 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).

[Philippe Normand]

Created attachment 83807 [details]
proposed patch

[WebKit Review Bot]

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.

[Philippe Normand]

Created attachment 83808 [details]
proposed patch

[Philippe Normand]

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 :)

[Philippe Normand]

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 :)

[Philippe Normand]

Created attachment 85331 [details]
proposed patch

[Martin Robinson]

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.

[Philippe Normand]

(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 :)

[Philippe Normand]

Created attachment 85789 [details]
proposed patch

[Martin Robinson]

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?

[Philippe Normand]

Committed r81228: <http://trac.webkit.org/changeset/81228>