Bug 159466 – [GStreamer][GL] switch to appsink

• Component: Platform
• Status: RESOLVED FIXED

[Philippe Normand]

Using fakesink for rendering is a bit weird because usually that element is used in tests. Appsink should be used instead. The following patch will be a refactoring.

[Philippe Normand]

Created attachment 282884 [details]
patch

[Carlos Garcia Campos]

Comment on attachment 282884 [details]
patch

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

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:592
> +static GstFlowReturn newSampleCallback(GstElement* sink, gpointer userData)
> +{
> +    MediaPlayerPrivateGStreamerBase* player = reinterpret_cast<MediaPlayerPrivateGStreamerBase*>(userData);

Since you are casting the callback in the g_signal_connect with G_CALLBACK, you can directly use MediaPlayerPrivateGStreamerBase* as parameter and avoid this cast.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:595
> +    GstSample* sample = gst_app_sink_pull_sample(GST_APP_SINK(sink));
> +
> +    player->triggerRepaint(sample);

You don't need the local variable, unless triggerRepaint doesn't adopt the sample, in which case, you need the local variable but you are leaking the sample here, since gst_app_sink_pull_sample is transfer full

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:604
> +    MediaPlayerPrivateGStreamerBase* player = reinterpret_cast<MediaPlayerPrivateGStreamerBase*>(userData);
> +    GstSample* sample = gst_app_sink_pull_preroll(GST_APP_SINK(sink));
> +
> +    player->triggerRepaint(sample);

Same comments here, the sample is leaked

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.h:133
> +    void triggerRepaint(GstSample*);
> +

This shouldn't be public, make the callbacks private static members if you need to access private API instead of exposing it.

[Philippe Normand]

Comment on attachment 282884 [details]
patch

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

>> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.h:133
>> +
> 
> This shouldn't be public, make the callbacks private static members if you need to access private API instead of exposing it.

Why?

[Philippe Normand]

Created attachment 282886 [details]
patch

[Philippe Normand]

Created attachment 282887 [details]
patch

[Philippe Normand]

Created attachment 282889 [details]
patch

[Xabier Rodríguez Calvar]

Comment on attachment 282889 [details]
patch

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

I leave Carlos the r+ as he reviwed first.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:176
> +            g_signal_handlers_disconnect_matched(appsink.get(), G_SIGNAL_MATCH_DATA, 0, 0, nullptr, nullptr, this);

Nit that I recommend to change before landing: g_signal_handlers_disconnect_by_data is less complex to use and more readable.

[Carlos Garcia Campos]

(In reply to comment #3)
> Comment on attachment 282884 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=282884&action=review
> 
> >> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.h:133
> >> +
> > 
> > This shouldn't be public, make the callbacks private static members if you need to access private API instead of exposing it.
> 
> Why?

Because it's only used internally, you made it public to call it from static functions, not to make it really public API.

[Philippe Normand]

Committed r202897: <http://trac.webkit.org/changeset/202897>