Bug 72023 – [GStreamer] GstCaps and GstPad RefPtr implementation

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Philippe Normand]

To avoid unref explicit calls that make Martin unhappy :)

[Philippe Normand]

Created attachment 114490 [details]
proposed patch

[Gustavo Noronha (kov)]

Comment on attachment 114490 [details]
proposed patch

Attachment 114490 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/10399353

[Early Warning System Bot]

Comment on attachment 114490 [details]
proposed patch

Attachment 114490 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/10403311

[Philippe Normand]

Created attachment 114493 [details]
proposed patch

[Martin Robinson]

Comment on attachment 114493 [details]
proposed patch

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

Look like you are forgetting to use adoptGRef. Other than that this patch looks good. Thank you!

> Source/WebCore/platform/graphics/gstreamer/GRefPtrGStreamer.cpp:44
> +        gst_object_ref(GST_OBJECT(ptr));

I think you should use gst_object_ref_sink here.

> Source/WebCore/platform/graphics/gstreamer/GRefPtrGStreamer.cpp:57
> +        gst_caps_ref(ptr);

And here as well.

> Source/WebCore/platform/graphics/gstreamer/GStreamerGWorld.cpp:95
> +    GRefPtr<GstElement> tee = gst_bin_get_by_name(GST_BIN(videoSink.get()), "videoTee");

You need to call adoptGRef here if you don't want to leak the tee.

> Source/WebCore/platform/graphics/gstreamer/GStreamerGWorld.cpp:107
> +    GRefPtr<GstPad> srcPad = gst_element_get_request_pad(tee.get(), "src%d");
> +    GRefPtr<GstPad> sinkPad = gst_element_get_static_pad(queue, "sink");

Ditto.

> Source/WebCore/platform/graphics/gstreamer/GStreamerGWorld.cpp:163
> +    GRefPtr<GstElement> tee = gst_bin_get_by_name(GST_BIN(videoSink.get()), "videoTee");

The same for all of these as well.

[Philippe Normand]

(In reply to comment #5)
> (From update of attachment 114493 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=114493&action=review
> 
> Look like you are forgetting to use adoptGRef. Other than that this patch looks good. Thank you!
> 
> > Source/WebCore/platform/graphics/gstreamer/GRefPtrGStreamer.cpp:44
> > +        gst_object_ref(GST_OBJECT(ptr));
> 
> I think you should use gst_object_ref_sink here.
> 

Yes, giving it a try.

> > Source/WebCore/platform/graphics/gstreamer/GRefPtrGStreamer.cpp:57
> > +        gst_caps_ref(ptr);
> 
> And here as well.
> 

GstCaps is not a GstObject. It's a lightweight refcounted structure, gst_caps_ref is enough here I think.

> > Source/WebCore/platform/graphics/gstreamer/GStreamerGWorld.cpp:95
> > +    GRefPtr<GstElement> tee = gst_bin_get_by_name(GST_BIN(videoSink.get()), "videoTee");
> 
> You need to call adoptGRef here if you don't want to leak the tee.
> 

Right!

> > Source/WebCore/platform/graphics/gstreamer/GStreamerGWorld.cpp:107
> > +    GRefPtr<GstPad> srcPad = gst_element_get_request_pad(tee.get(), "src%d");
> > +    GRefPtr<GstPad> sinkPad = gst_element_get_static_pad(queue, "sink");
> 
> Ditto.
> 
> > Source/WebCore/platform/graphics/gstreamer/GStreamerGWorld.cpp:163
> > +    GRefPtr<GstElement> tee = gst_bin_get_by_name(GST_BIN(videoSink.get()), "videoTee");
> 
> The same for all of these as well.

Yes.

[Philippe Normand]

Created attachment 114697 [details]
proposed patch

[Martin Robinson]

Comment on attachment 114697 [details]
proposed patch

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

Looks good, but please fix the minor issues below before landing. Thanks!

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1753
> +    pad.clear();
> +    pad = adoptGRef(gst_element_get_static_pad(m_webkitVideoSink, "sink"));

You shouldn't need to clear the pad first, you can just reassign it.

> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:234
> +    GRefPtr<GstPad> targetPad = gst_element_get_static_pad(GST_ELEMENT(priv->appsrc), "src");

Missing adoptGRef here?

> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:733
> +            GRefPtr<GstCaps> caps = gst_caps_new_simple("application/x-icy", "metadata-interval", G_TYPE_INT, (gint) icyMetaInt, NULL);

Also need to use adoptRef here as well.

[Philippe Normand]

Committed r99977: <http://trac.webkit.org/changeset/99977>