Bug 72023 - [GStreamer] GstCaps and GstPad RefPtr implementation
Summary: [GStreamer] GstCaps and GstPad RefPtr implementation
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 69835
  Show dependency treegraph
 
Reported: 2011-11-10 06:29 PST by Philippe Normand
Modified: 2011-11-11 08:36 PST (History)
3 users (show)

See Also:


Attachments
proposed patch (16.17 KB, patch)
2011-11-10 06:58 PST, Philippe Normand
gns: commit-queue-
Details | Formatted Diff | Diff
proposed patch (16.10 KB, patch)
2011-11-10 07:08 PST, Philippe Normand
mrobinson: review-
Details | Formatted Diff | Diff
proposed patch (16.58 KB, patch)
2011-11-11 07:29 PST, Philippe Normand
mrobinson: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Philippe Normand 2011-11-10 06:29:51 PST
To avoid unref explicit calls that make Martin unhappy :)
Comment 1 Philippe Normand 2011-11-10 06:58:03 PST
Created attachment 114490 [details]
proposed patch
Comment 2 Gustavo Noronha (kov) 2011-11-10 07:06:30 PST
Comment on attachment 114490 [details]
proposed patch

Attachment 114490 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/10399353
Comment 3 Early Warning System Bot 2011-11-10 07:06:34 PST
Comment on attachment 114490 [details]
proposed patch

Attachment 114490 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/10403311
Comment 4 Philippe Normand 2011-11-10 07:08:06 PST
Created attachment 114493 [details]
proposed patch
Comment 5 Martin Robinson 2011-11-10 09:13:12 PST
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.
Comment 6 Philippe Normand 2011-11-11 07:20:02 PST
(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.
Comment 7 Philippe Normand 2011-11-11 07:29:04 PST
Created attachment 114697 [details]
proposed patch
Comment 8 Martin Robinson 2011-11-11 08:27:46 PST
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.
Comment 9 Philippe Normand 2011-11-11 08:36:29 PST
Committed r99977: <http://trac.webkit.org/changeset/99977>