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.
Created attachment 282884 [details] patch
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.
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?
Created attachment 282886 [details] patch
Created attachment 282887 [details] patch
Created attachment 282889 [details] patch
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.
(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.
Committed r202897: <http://trac.webkit.org/changeset/202897>