GNOME Bugzilla – Bug 475723
cleanup static caps correctly
Last modified: 2007-09-19 12:31:56 UTC
GstElementfactory causes one time leaks as such: 59,991 (3,024 direct, 56,967 indirect) bytes in 252 blocks are definitely lost in loss record 3,987 of 3,994 at 0x4021835: malloc (vg_replace_malloc.c:207) by 0x41592C5: g_malloc (gmem.c:131) by 0x4168E97: g_slice_alloc (gslice.c:777) by 0x4133E4A: g_ptr_array_sized_new (garray.c:372) by 0x4133E91: g_ptr_array_new (garray.c:366) by 0x4066C56: gst_static_caps_get (gstcaps.c:460) by 0x472176C: analyze_new_pad (gstdecodebin2.c:1299) by 0x47227B3: type_found (gstdecodebin2.c:1148) by 0x40B831C: gst_marshal_VOID__UINT_BOXED (gstmarshal.c:507) by 0x40E762A: g_closure_invoke (gclosure.c:490) by 0x40F8102: signal_emit_unlocked_R (gsignal.c:2440) by 0x40F9626: g_signal_emit_valist (gsignal.c:2199) by 0x40F97E8: g_signal_emit (gsignal.c:2243) by 0x47727FA: gst_type_find_element_activate (gsttypefindelement.c:744) by 0x408AB2B: gst_pad_set_active (gstpad.c:648) by 0x407143A: activate_pads (gstelement.c:2486) by 0x407E3C6: gst_iterator_fold (gstiterator.c:503) by 0x4070EB1: iterator_activate_fold_with_resync (gstelement.c:2518) by 0x4070F77: gst_element_pads_activate (gstelement.c:2561) by 0x40712E5: gst_element_change_state_func (gstelement.c:2626) by 0x4770BE7: gst_type_find_element_change_state (gsttypefindelement.c:771) by 0x406DB08: gst_element_change_state (gstelement.c:2402) by 0x406DF68: gst_element_set_state_func (gstelement.c:2352) by 0x406CB52: gst_element_set_state (gstelement.c:2258) by 0x4061024: gst_bin_change_state_func (gstbin.c:1920) by 0x4723FC5: gst_decode_bin_change_state (gstdecodebin2.c:2197) by 0x406DB08: gst_element_change_state (gstelement.c:2402) by 0x406DF68: gst_element_set_state_func (gstelement.c:2352) by 0x406CB52: gst_element_set_state (gstelement.c:2258) by 0x4061024: gst_bin_change_state_func (gstbin.c:1920) by 0x408D9C6: gst_pipeline_change_state (gstpipeline.c:503) by 0x406DB08: gst_element_change_state (gstelement.c:2402) by 0x406D7B4: gst_element_continue_state (gstelement.c:2112) by 0x406DBE5: gst_element_change_state (gstelement.c:2439) by 0x406DF68: gst_element_set_state_func (gstelement.c:2352) by 0x406CB52: gst_element_set_state (gstelement.c:2258) by 0x804B1A2: main (gst-launch.c:757)
Created attachment 95322 [details] [review] release static caps gstelementfactory.c | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-)
Created attachment 95323 [details] [review] release static caps
I know I'm just nitpicking, but here it goes: - I'm not sure if the g_atomic_int_get() + g_atomic_int_set() really buy you anything here. It's still racy, isn't it? (Also, shouldn't the > 0 check be a == 1 check? Otherwise you're freeing the caps structures knowing that something else is still using them, no?). If we really want to cater for thread-safety and the possibility that other pieces of code might still hold a ref to the caps embedded in the GstStaticCaps within the GstStaticPadTemplate, then we need something more clever than what we're currently doing anyway, don't we? Given that we're freeing the template memory which in effect frees the memory the GstCaps point to (if I'm not mistaken) ... - I somehow feel that code that meddles with internal caps structure fields belongs into gstcaps.c. Maybe something like __gst_static_caps_template_clear()? Same for the static pad template really, but then that's more a matter of taste/personal preference in the end :)
Err, I meant __gst_static_caps_clear().
if you look at gstcaps.c::gst_static_caps_get() * the alternative/addition to g_atomic_int_{s,g}et() would be to G_LOCK (static_caps_lock); * yes, the > 0 check should be a == 1 * the releasing of the memory is correct, as its dynamiclly allocated. we are not releasing the caps memory, but the one of the structure. * as the dirty details of how gst_static_caps_get() works is in gstcaps.c the function that releases them can be put there too. But then also gst_static_caps_get() should be in gststaticcaps.c. So what about gstelementfactory.c::gst_element_factory_cleanup() calling gstpadtemplate.c::__gst_static_pad_template_clear() and this calling gstcaps.c::__gst_static_caps_clear()
> The releasing of the memory is correct, as its dynamically allocated. > we are not releasing the caps memory, but the one of the structure. I don't think the caps are dynamically allocated. Try this: Index: gstelementfactory.c =================================================================== RCS file: /cvs/gstreamer/gstreamer/gst/gstelementfactory.c,v retrieving revision 1.133 diff -u -p -r1.133 gstelementfactory.c --- gstelementfactory.c 25 Jul 2007 18:37:12 -0000 1.133 +++ gstelementfactory.c 11 Sep 2007 14:53:53 -0000 @@ -217,7 +217,11 @@ gst_element_factory_cleanup (GstElementF for (item = factory->staticpadtemplates; item; item = item->next) { GstStaticPadTemplate *templ = item->data; + GstCaps *caps = (GstCaps *) &(templ->static_caps); + g_print ("tmpl = %p-%p\ncaps = %p-%p\n\n", + (guint8 *) templ, ((guint8 *) templ) + sizeof (GstStaticPadTemplate), + (guint8 *) caps, ((guint8 *) caps) + sizeof (GstCaps)); g_free (templ->name_template); g_free ((gchar *) templ->static_caps.string); memset (&(templ->static_caps), 0, sizeof (GstStaticCaps));
Sorry, I probably misread what you said. What I meant to say is that if we want to cater for the case where other bits of code still hold a ref to the caps (ie. caps->refcount > 1), then we have no choice but to leak the GstStaticPadTemplate struct here, unless we add special code to GstCaps to make this case work too.
2007-09-19 Stefan Kost <ensonic@users.sf.net> * gst/gstelementfactory.c: Release static caps. Fixes #475723. (Patch committed with FIXME added and monotonic ops removed).