Summary: | [GStreamer][GL] switch to appsink | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Philippe Normand <pnormand> | ||||||||||
Component: | Platform | Assignee: | Nobody <webkit-unassigned> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | calvaris, cgarcia, pnormand, yoon, zan | ||||||||||
Priority: | P2 | ||||||||||||
Version: | WebKit Nightly Build | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Attachments: |
|
Description
Philippe Normand
2016-07-06 06:05:51 PDT
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> |