Bug 107445 – [gstreamer] MediaPlayerPrivateGStreamer should take ownership of the playbin

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Chris Dumez]

In gstreamer 1.0, gst_element_factory_make() now returns a floating reference [1]. MediaPlayerPrivateGStreamer calls gst_element_factory_make() to create the playbin object but does not take ownership of the object. As a consequence, the object keeps floating until it is unref'd in the MediaPlayerPrivateGStreamer destructor.

We should call gst_object_ref_sink() on the playbin object returned by gst_element_factory_make() to effectively take ownership of it.

[1] http://gstreamer.freedesktop.org/data/doc/gstreamer/head/gstreamer/html/GstElementFactory.html#gst-element-factory-make

[Chris Dumez]

Created attachment 183772 [details]
Patch

[Philippe Normand]

How about using an adopted GRefPtr<GstElement>?

[Chris Dumez]

(In reply to comment #2)
> How about using an adopted GRefPtr<GstElement>?

Hmm. We cannot adopt since it is floating.

Did I understand correctly that you want to make m_playBin a GRefPtr<GstElement> ? If so, then we need to adopt with gstreamer 0.10 and *not* adopt with gstreamer 1.0. I'm fine with that. However, I will likely need to replace uses to m_playBin everywhere by m_playBin.get(), right?

[Philippe Normand]

Comment on attachment 183772 [details]
Patch

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

Right, Christophe... Well, Martin are you OK with this patch?

> Source/WebCore/ChangeLog:17
> +We should call gst_object_ref_sink() on the playbin object returned by gst_element_factory_make() to effectively take ownership of it.

Left over?

[Chris Dumez]

Comment on attachment 183772 [details]
Patch

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

>> Source/WebCore/ChangeLog:17
>> +We should call gst_object_ref_sink() on the playbin object returned by gst_element_factory_make() to effectively take ownership of it.
> 
> Left over?

Oops.

[Chris Dumez]

(In reply to comment #2)
> How about using an adopted GRefPtr<GstElement>?

Well, I do not mind using a smart pointer if you prefer. However, keep in mind this involves more changes.

Also note that we are already doing the following a few lines below:

// Take ownership.
gst_object_ref_sink(m_videoSinkBin);

[Chris Dumez]

Created attachment 183778 [details]
Patch

Fix changelog (not using smart pointer yet).

[Chris Dumez]

Created attachment 183949 [details]
Patch using a smart pointer (alternative)

[Philippe Normand]

Comment on attachment 183949 [details]
Patch using a smart pointer (alternative)

LGTM!

[WebKit Review Bot]

Comment on attachment 183949 [details]
Patch using a smart pointer (alternative)

Clearing flags on attachment: 183949

Committed r140414: <http://trac.webkit.org/changeset/140414>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Chris Dumez]

Comment on attachment 183949 [details]
Patch using a smart pointer (alternative)

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

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1797
> +    // In gstreamer 1.0, gst_element_factory_make returns a floating

Based on the results on the Qt bots, it seems like the gstreamer 0.10 doc may be wrong and gst_element_factory_make() is returning a floating reference there as well :(
I'll upload a fix soon.