[webkit-reviews] review granted: [Bug 209119] [GStreamer][MSE] Youtube 'live stream'/H264 URLs fail to play, VP8/9 URLs play OK : [Attachment 397093] Patch

[bugzilla-daemon at webkit.org]

Xabier Rodríguez Calvar <calvaris at igalia.com> has granted  review:
Bug 209119: [GStreamer][MSE] Youtube 'live stream'/H264 URLs fail to play,
VP8/9 URLs play OK
https://bugs.webkit.org/show_bug.cgi?id=209119

Attachment 397093: Patch

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




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

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

>
Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.c
pp:253
> +static bool checkStateChangeShouldDelaySeek(GstStateChangeReturn
getStateResult, GstState currentState, GstState newState)
> +{
> +    if (getStateResult != GST_STATE_CHANGE_ASYNC)
> +	   return false;
> +    if (GST_STATE_TRANSITION(currentState, newState) ==
GST_STATE_CHANGE_PLAYING_TO_PAUSED)
> +	   return false;
> +    if (currentState == GST_STATE_READY && newState >= GST_STATE_PAUSED)
> +	   return false;
> +    return true;
> +}

This function looks much better than the code before. If I can do a nit, I
would do just: shouldDelaySeek. And similar for the variables you use below.

>
Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.c
pp:364
> +	   GST_DEBUG("doSeek(): Initial seek succeeded, returning true");

You don't need to specify the function in the seek, it's included in the log
already.

>
Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.c
pp:373
> +	       GST_DEBUG("doSeek(): gst_element_seek() failed, returning
false");

Ditto

>
Source/WebCore/platform/graphics/gstreamer/mse/MediaPlayerPrivateGStreamerMSE.c
pp:377
> +	   GST_DEBUG("doSeek(): gst_element_seek() succeeded, returning true");

Ditto

>
Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:7
74
> +	       initialSeekSegmentFixerProbe, gst_segment_copy(segment.get()),
reinterpret_cast<GDestroyNotify>(gst_segment_free));

I don't think it is needed to copy the segment here, right? We are not using it
anymore so I think we can either .release() the GUniquePtr or just not use it.