[webkit-reviews] review denied: [Bug 174817] [GStreamer] Refactor media player to use MediaTime consistently : [Attachment 316763] Patch

[bugzilla-daemon at webkit.org]

Xabier Rodríguez Calvar <calvaris at igalia.com> has denied Charlie Turner
<cturner at igalia.com>'s request for review:
Bug 174817: [GStreamer] Refactor media player to use MediaTime consistently
https://bugs.webkit.org/show_bug.cgi?id=174817

Attachment 316763: Patch

https://bugs.webkit.org/attachment.cgi?id=316763&action=review




--- Comment #7 from Xabier Rodríguez Calvar <calvaris at igalia.com> ---
Comment on attachment 316763
  --> https://bugs.webkit.org/attachment.cgi?id=316763
Patch

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

I really love this patch happening. Using MediaTime should save us from many
nasty rounding bugs in the long term (though it is not guaranteed that we won't
introduce some with this patch).

> Source/WebCore/ChangeLog:41
> +	   * platform/graphics/gstreamer/GStreamerUtilities.cpp:
> +	   (WebCore::toGstClockTime):
> +	   * platform/graphics/gstreamer/GStreamerUtilities.h:
> +	   * platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:
> +	   (WebCore::MediaPlayerPrivateGStreamer::MediaPlayerPrivateGStreamer):
> +	   (WebCore::MediaPlayerPrivateGStreamer::load):
> +	   (WebCore::MediaPlayerPrivateGStreamer::playbackPosition):
> +	   (WebCore::MediaPlayerPrivateGStreamer::durationMediaTime):
> +	   (WebCore::MediaPlayerPrivateGStreamer::currentMediaTime):
> +	   (WebCore::MediaPlayerPrivateGStreamer::seek):
> +	   (WebCore::MediaPlayerPrivateGStreamer::doSeek):
> +	   (WebCore::MediaPlayerPrivateGStreamer::updatePlaybackRate):
> +	   (WebCore::MediaPlayerPrivateGStreamer::asyncStateChangeDone):
> +	   (WebCore::MediaPlayerPrivateGStreamer::updateStates):
> +	   (WebCore::MediaPlayerPrivateGStreamer::didEnd):
> +	   * platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.h:
> +	   * platform/graphics/gstreamer/mse/GStreamerMediaSample.cpp:
> +	   (WebCore::GStreamerMediaSample::offsetTimestampsBy):
> +	   *
platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.cpp:
> +	   (WebCore::MediaPlayerPrivateGStreamerMSE::seek):
> +	  
(WebCore::MediaPlayerPrivateGStreamerMSE::notifySeekNeedsDataForTime):
> +	   (WebCore::MediaPlayerPrivateGStreamerMSE::doSeek):
> +	   (WebCore::MediaPlayerPrivateGStreamerMSE::isTimeBuffered):
> +	   (WebCore::MediaPlayerPrivateGStreamerMSE::currentMediaTime):
> +	   * platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.h:
> +	   * platform/graphics/gstreamer/mse/PlaybackPipeline.cpp:
> +	   (WebCore::PlaybackPipeline::enqueueSample):
> +	   * platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:
> +	   (webKitMediaSrcQueryWithParent):

You might be more verbose here.

>
Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:323
> +    GstClockTime positionGst = static_cast<GstClockTime>(position);

gstreamerPosition

>
Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:432
> +    bool failure = !gst_element_query_duration(m_pipeline.get(), timeFormat,
&timeLength);
> +    failure |= !GST_CLOCK_TIME_IS_VALID(timeLength);

Please keep this in one line.

>
Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:543
> +	       endTime = durationMediaTime().timeValue();

In these kind of cases, you might end up getting invalid times or maybe (I
don't know if it's possible or not but unless you're 100% sure we should follow
the safe path) times with a scale that is not GST_SECOND, meaning that
timeValue could return something that is not nanoseconds. We need to ensure
that and here, I think we should use toGstClockTime. Maybe it would be
interesting to rename the functions at utilities to have toGstUnsigned64Time
and inline toGstClockTime to call the former and static_cast.

>
Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:560
> +    int64_t currentPosition = playbackPosition().timeValue();

ditto.

>
Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:575
> -    GST_INFO("Need to mute audio?: %d", (int) mute);
> +    GST_INFO(mute ? "Need to mute audio" : "Do not need to mute audio");

No strong opinion about this specific case but I landed yesterday a patch to
help with this kind of debug messages. Very dumb function WTF::boolForPrinting
that takes a bool and converts it to "true" or "false". This could end up as:
GST_INFO("Need to mute audio: %s", boolForPrinting(mute));
But if you think it is better this way, be my guest to leave it like this.

>
Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.c
pp:200
> +    GST_DEBUG("m_seeking=%s, m_seekTime=%s", m_seeking ? "true" : "false",
toString(m_seekTime).utf8().data());

Keep boolForPrinting

>
Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.c
pp:669
> +    GST_DEBUG("Time %s buffered? %s", toString(time).utf8().data(), result ?
"Yes" : "No");

Use boolForPrinting.