[webkit-reviews] review denied: [Bug 99362] [GStreamer] GstCaps are leaked when building with gstreamer-1.0 : [Attachment 168782] patch proposal: use GRefPtr<GstCaps> for webkitGstGetPadCaps; also use GRefPtr in webkitVideoSinkRender

[bugzilla-daemon at webkit.org]

Martin Robinson <mrobinson at webkit.org> has denied arno. <arno at renevier.net>'s
request for review:
Bug 99362: [GStreamer] GstCaps are leaked when building with gstreamer-1.0
https://bugs.webkit.org/show_bug.cgi?id=99362

Attachment 168782: patch proposal: use GRefPtr<GstCaps> for
webkitGstGetPadCaps; also use GRefPtr in webkitVideoSinkRender
https://bugs.webkit.org/attachment.cgi?id=168782&action=review

------- Additional Comments from Martin Robinson <mrobinson at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=168782&action=review



Looks nice, though I have a couple nits.

> Source/WebCore/platform/graphics/gstreamer/GStreamerVersioning.cpp:57
> +    if (!caps)
> +	   return 0;
> +#ifdef GST_API_VERSION_1
> +	   return adoptGRef(caps);
> +#else
> +	   return GRefPtr<GstCaps>(caps);
> +#endif
>  }

You don't actually need to check for null here or do an explicit call to
GRefPtr. Is the problem that GST_PAD_CAPS doesn't return a new reference, but
gst_pad_query_caps/gst_pad_get_current_caps does?

This method can just be:

#ifdef GST_API_VERSION_1
GStCaps* caps = gst_pad_get_current_caps(pad);
if (!caps)
    caps = gst_pad_query_caps(pad, 0);
return adoptGRef(caps); // gst_pad_query_caps and gst_pad_get_current_caps
return a new reference.
#else
return GST_PAD_CAPS(pad);
#endif.

> Source/WebCore/platform/graphics/gstreamer/GStreamerVersioning.h:25
> +
>  #include <gst/gst.h>

Nit: There should be no empty line here.

> Source/WebCore/platform/graphics/gstreamer/VideoSinkGStreamer.cpp:-187
> -#ifdef GST_API_VERSION_1
> -	   gst_caps_unref(caps);
> -#endif

Nice cleanup!