[webkit-reviews] review denied: [Bug 55217] [GStreamer] Frame accurate seeking isn't always accurate : [Attachment 85331] proposed patch

[bugzilla-daemon at webkit.org]

Martin Robinson <mrobinson at webkit.org> has denied Philippe Normand
<pnormand at igalia.com>'s request for review:
Bug 55217: [GStreamer] Frame accurate seeking isn't always accurate
https://bugs.webkit.org/show_bug.cgi?id=55217

Attachment 85331: proposed patch
https://bugs.webkit.org/attachment.cgi?id=85331&action=review

------- Additional Comments from Martin Robinson <mrobinson at webkit.org>
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.