GNOME Bugzilla – Bug 637553
[autoconvert] Never removes elements
Last modified: 2013-07-17 13:01:57 UTC
autoconvert currently only adds elements and pads to itself but never removes them again later. This makes it impossible to re-use autoconvert and also causes memory leaks. Just try to run the autovideoconvert unit test to see a few leaked elements and pads. (It could also use g_object_set_qdata_full() instead of the weakref hack it currently uses for the internal src/sinkpad)
The subelements should be freed when the autoconvert is disposed (since its just a bin). Otherwise I don't know what other time would be right to remove them. I think the bug seen frim the autovideoconvert unit test is a ref leak somewhere. Also, I just had a look at the autovideoconvert source code, it looks very much overcomplicated. If we care about changes in the list of factories, it should be in the base autoconvert (but frankly and I dont think we care).
The elements should be removed when going back to READY (same for the factory changes). The factory changes make sense if new elements are installed via the automatic plugin installation... but for that to work autoconvert also has to post missing-element messages on the bus :)
The problem with automatic plugin installation is that autoconvert has no idea what kind of plugin it wants... For example, in farsight we use it to switch between ffenc_h263 and ffenc_h263p and here its to change colorspace convertors.. Also, autoconvert intentially does not allow changing the list of factories once it as been created (for simplicity's sake). So the only problem I see is the leak that the unit test of autovideoconvert finds.
Yes, the leak is the biggest problem I guess. For missing elements autoconvert could provide a signal and autovideoconvert could ask for video converters in the required format then. Apart from that, does autoconvert support changing caps? Like, in the case of autovideoconvert, a switch from bayer to normal rgb, which then would require autoconvert to use different elements. If it doesn't this should definitely be added to autoconvert... or autovideoconvert shouldn't use it.
Yes, autoconvert definitely supports changing caps, look at the setcaps() function. It tries the current element, if that fails, it tries to change.
Created attachment 176765 [details] [review] may fix some leaks in autoconvert unref sink and src pad after gst_pad_by_direction calls unref element if gst_auto_convert_activate_element failed
commit 825052ba3d0799090ba3bb51a444141896491687 Author: Sebastian Dröge <sebastian.droege@collabora.co.uk> Date: Tue Dec 21 18:01:28 2010 +0100 autoconvert: Fix some more leaks and reorganize unref code commit f0ebcba6c4ce8a6a04a3b041abbd34610c0dc8e7 Author: benjamin gaignard <benjamin.gaignard@stericsson> Date: Mon Dec 20 15:33:28 2010 +0100 autoconvert: Avoid some leaks in autoconvert unref sink and src pad after gst_pad_by_direction calls unref element if gst_auto_convert_activate_element failed. See bug #637553. Now only a single leak is left, might be somewhere else though
Not sure where this comes from ==30467== 797 (40 direct, 757 indirect) bytes in 1 blocks are definitely lost in loss record 2,076 of 2,176 ==30467== at 0x4A074E8: malloc (vg_replace_malloc.c:236) ==30467== by 0x3236A4C814: g_malloc (gmem.c:164) ==30467== by 0x3236A61979: g_slice_alloc (gslice.c:842) ==30467== by 0x4C954B1: gst_structure_id_empty_new_with_size (gststructure.c:123) ==30467== by 0x4C961BA: gst_structure_id_new (gststructure.c:737) ==30467== by 0x4C638E4: gst_event_new_latency (gstevent.c:1069) ==30467== by 0x4C4A644: gst_bin_do_latency_func (gstbin.c:2365) ==30467== by 0x4CB95CB: gst_marshal_BOOLEAN__VOID (gstmarshal.c:481) ==30467== by 0x3238C1033D: g_closure_invoke (gclosure.c:766) ==30467== by 0x3238C28FF1: signal_emit_unlocked_R (gsignal.c:3290) ==30467== by 0x3238C2A97B: g_signal_emit_valist (gsignal.c:2993) ==30467== by 0x3238C2B362: g_signal_emit (gsignal.c:3040) ==30467== by 0x4C4BCBB: gst_bin_recalculate_latency (gstbin.c:2324) ==30467== by 0x4C4C0FE: gst_bin_change_state_func (gstbin.c:2418) ==30467== by 0x4C7E000: gst_pipeline_change_state (gstpipeline.c:482) ==30467== by 0x4C5EDAB: gst_element_change_state (gstelement.c:2615) ==30467== by 0x4C46C18: gst_bin_continue_func (gstbin.c:2729) ==30467== by 0x3236A6EC0E: g_thread_pool_thread_proxy (gthreadpool.c:319) ==30467== by 0x3236A6CD43: g_thread_create_proxy (gthread.c:1897) ==30467== by 0x3860A068B9: start_thread (pthread_create.c:300)
> From: benjamin gaignard <benjamin.gaignard@stericsson> Could you fix your name + e-mail address please for future patches?