GNOME Bugzilla – Bug 596078
Playbin2 takes ref of audio-/video-sink parameter
Last modified: 2009-09-29 14:43:38 UTC
#include <gst/gst.h> gint main (gint argc, gchar **argv) { GstElement *playbin = NULL, *fakesink = NULL; GMainLoop *loop; gst_init (NULL, NULL); playbin = gst_element_factory_make ("playbin2", NULL); fakesink = gst_element_factory_make ("fakesink", NULL); g_object_set (G_OBJECT (playbin), "audio-sink", fakesink, NULL); gst_object_unref (playbin); gst_object_unref (fakesink); return 0; } => GStreamer-CRITICAL **: gst_object_unref: assertion `((GObject *) object)->ref_count > 0' failed
playbin does as well
Created attachment 143793 [details] [review] 0001-playbin2-Fix-reference-handling-of-audio-sink-video-.patch I'll push that change after 0.10.25 release.
Comment on attachment 143793 [details] [review] 0001-playbin2-Fix-reference-handling-of-audio-sink-video-.patch Wrong patch...
Created attachment 143795 [details] [review] 0001-playbin2-Fix-reference-handling-of-audio-sink-video-.patch Correct patch :)
Make sure that this is very explicitly mentioned in release notes, because it breaks existing code that leaves out the unref to avoid the problem...
Camerabin add refcounts. Its probably good to ref. It would be nice to cover that in our unit tests too. The problem with the unref is that it causes a leak for apps that used custom elements so already. So I agree with Arnout that we need to point that out very clearly.
Am I missing something? gst_play_sink_set_sink/gst_play_sink_set_vis_plugin already add a ref, so gst_value_get_object is fine in the caller. I think the problem is the one I started discussing with Wim some time ago, that gstplaysink calls gst_object_sink to sink the floating reference on the passed element. That seems to have been removed for the audio/video sinks when gst_play_sink_set_video_sink and gst_play_sink_set_audio_sink were unified into the gst_play_sink_set_sink() function, but it still seems to be true for the gst_play_sink_set_vis_plugin() function - causing some inconsistency. I think playbin2 should not sink the ref on the passed objects, and make it uniformly the caller's responsibility.
(In reply to comment #7) > Am I missing something? gst_play_sink_set_sink/gst_play_sink_set_vis_plugin > already add a ref, so gst_value_get_object is fine in the caller. > > I think the problem is the one I started discussing with Wim some time ago, > that gstplaysink calls gst_object_sink to sink the floating reference on the > passed element. That seems to have been removed for the audio/video sinks when > gst_play_sink_set_video_sink and gst_play_sink_set_audio_sink were unified into > the gst_play_sink_set_sink() function, but it still seems to be true for the > gst_play_sink_set_vis_plugin() function - causing some inconsistency. > > I think playbin2 should not sink the ref on the passed objects, and make it > uniformly the caller's responsibility. The problem is, that _get_object is called (meaning, you don't own the reference). This is then reffed and sinked later (refcount += 1 and the refcount -= 1). So in the end playbin/playsink don't own any reference to the passed object (only the GValue owns one reference, GLib owns another reference internally and the caller owns the remaining reference). So right, either playbin/playsink shouldn't sink the object and only ref it. Or it should be as with my current patch ;) I don't know if playbin/playsink should sink the object, it probably makes sense because they "own" the object. Jan, do you think we should fix that for 0.10.25 already? (In reply to comment #6) > Camerabin add refcounts. Its probably good to ref. It would be nice to cover > that in our unit tests too. > > The problem with the unref is that it causes a leak for apps that used custom > elements so already. So I agree with Arnout that we need to point that out very > clearly. Yes, should definitely be in the release notes (although we don't give any API guarantees for playbin2 yet).
(In reply to comment #8) > The problem is, that _get_object is called (meaning, you don't own the > reference). This is then reffed and sinked later (refcount += 1 and the > refcount -= 1). So in the end playbin/playsink don't own any reference to the > passed object (only the GValue owns one reference, GLib owns another reference > internally and the caller owns the remaining reference). This is where the disparity arises - when playbin2 refs and sinks the passed element (as playbin1 does) the caller *doesn't* own their reference any more - they gave it away when they set the element onto playbin. If an application wants to keep a reference, they need to add a ref before passing the element into the audio/video-sink > So right, either playbin/playsink shouldn't sink the object and only ref it. Or > it should be as with my current patch ;) > > I don't know if playbin/playsink should sink the object, it probably makes > sense because they "own" the object. > > Jan, do you think we should fix that for 0.10.25 already? > > (In reply to comment #6) > > Camerabin add refcounts. Its probably good to ref. It would be nice to cover > > that in our unit tests too. > > > > The problem with the unref is that it causes a leak for apps that used custom > > elements so already. So I agree with Arnout that we need to point that out very > > clearly. > > Yes, should definitely be in the release notes (although we don't give any API > guarantees for playbin2 yet). It should be clarified in the playbin2 docs. I am marking this as a blocker bug, because a) we need to resolve the reference counting policy and b) I think the playbin2 behaviour has changed since 0.10.24 - it still sinks the ref on the visualisation element, but no longer on the audio/video/subpic/text sinks.
(In reply to comment #9) > (In reply to comment #8) > > > The problem is, that _get_object is called (meaning, you don't own the > > reference). This is then reffed and sinked later (refcount += 1 and the > > refcount -= 1). So in the end playbin/playsink don't own any reference to the > > passed object (only the GValue owns one reference, GLib owns another reference > > internally and the caller owns the remaining reference). > > This is where the disparity arises - when playbin2 refs and sinks the passed > element (as playbin1 does) the caller *doesn't* own their reference any more - > they gave it away when they set the element onto playbin. > > If an application wants to keep a reference, they need to add a ref before > passing the element into the audio/video-sink That's not good for bindings. GObject properties should never steal the reference from the caller, that would be a problem for bindings. so playbin2 should either a) get_object + ref (aka dup_object) or b) get_object + ref (or dup) + ref + sink > > So right, either playbin/playsink shouldn't sink the object and only ref it. Or > > it should be as with my current patch ;) > > > > I don't know if playbin/playsink should sink the object, it probably makes > > sense because they "own" the object. > > > > Jan, do you think we should fix that for 0.10.25 already? > > > > (In reply to comment #6) > > > Camerabin add refcounts. Its probably good to ref. It would be nice to cover > > > that in our unit tests too. > > > > > > The problem with the unref is that it causes a leak for apps that used custom > > > elements so already. So I agree with Arnout that we need to point that out very > > > clearly. > > > > Yes, should definitely be in the release notes (although we don't give any API > > guarantees for playbin2 yet). > > It should be clarified in the playbin2 docs. I am marking this as a blocker > bug, because a) we need to resolve the reference counting policy and b) I think > the playbin2 behaviour has changed since 0.10.24 - it still sinks the ref on > the visualisation element, but no longer on the audio/video/subpic/text sinks. Hm, it still sinks audio/video/subpic/text sinks (playbin2) and the vis element (playsink).
Created attachment 144246 [details] [review] 0002-playbin2-playsink-Use-gst_object_ref_sink-instead-of.patch
There's still a problem with both of these patches applied, which is that playbin2 takes ownership of the floating reference if the caller hasn't already. That means that the naive test case above is still incorrect - the caller lost their reference to the element (the floating reference), but it happens to be OK because playbin2 held onto an extra reference by dup'ing the object. If the caller had sunk the floating reference before passing the element to playbin2, then it's: g_value_dup_object () (refcount += 1) gst_object_ref_sink () (floating ref was gone, so refcount += 1) Now playbin2 owns 2 references to the element but will only drop one. As far as I can see, there is no way to avoid having playbin2 sink the floating reference either, because if it adds the element to a bin, then the GstBin will take ownership regardless. I think the moral of the story is that playbin2 is already doing the only thing it can, however bad that might be for bindings. If applications need to keep a reference to the passed element, they should ref and sink it first themselves.
So the summary of the story is, that this is no bug here and bindings (in this case Vala or the programmer) should make sure that things are sinked already. Makes my first patch invalid, the second one is still valid because it doesn't change anything ;)