GNOME Bugzilla – Bug 750881
glmixerbin: correctly free input chain on pad release
Last modified: 2015-06-15 08:04:02 UTC
Created attachment 305183 [details] [review] gstglmixerbin: compare with ghost_pad on pad release I am testing with an application that uses a glvideomixer + glimagesink pipeline to play a video file playlist(a bit large for a testcase, but i can try to cut down if its helpful). When releasing a pad for the current file in order to move to the next file, memory is leaked. When releasing a glmixerbin pad in gst_gl_mixer_bin_release_pad(), chain->mixer_pad is compared to the pad being released. I think chain->ghost_pad should be compared to the incoming pad instead, since gst_gl_mixer_bin_request_pad returns the ghost pad to requesting functions. The current comparison results in _free_input_chain never getting called. The problem doesn't show up with gst-play-1.0 or other playbin pipeline because (as far as i understand) input-selector is in front of the mixer, and no mixer pads are released when going through a file playlist. I have a patch that uses ghost_pad, attaching below (also the bin's lock need to not be held, in order to remove elements from the bin). _free_input_chain is now called, however i still don't see gst_gl_upload_finalize and gst_gl_color_convert_finalize being called, i would expect them to be called now. So i still see leaking memory on pad release even with this patch. Does the first patch make any sense? Is there any extra suggestion for correctly freeing the input chain?
Created attachment 305190 [details] dynamic source mix Test application
A couple of things were wrong :) commit 4796cef882569c772e497584a3472dfce6e66a70 Author: Matthew Waters <matthew@centricular.com> Date: Sat Jun 13 18:43:04 2015 +1000 glmixerbin: implement proper dynamic pad removal https://bugzilla.gnome.org/show_bug.cgi?id=750881
thanks for the fix! One more question: i have to make this also work on top of older repo (gst-plugins-bad 4521524de), where gstglvideomixer was still based on gstglmixer (and not gstglmixerbin). Even in that old design, gstglmixer's _clean_upload() function is not called on pad release, leading to similar leaks (_clean_upload is only called in gst_gl_mixer_stop). In this design there is no gst_gl_mixer_release_pad function, since this is handled by parent class videoaggregator. Are there any suggestions on how to properly handle this in the old design? Does it make sense to add a gst_gl_mixer_release_pad? e.g. : diff --git a/ext/gl/gstglmixer.c b/ext/gl/gstglmixer.c index 1532697..f5acd98 100644 --- a/ext/gl/gstglmixer.c +++ b/ext/gl/gstglmixer.c @@ -59,6 +59,8 @@ static GstBuffer *_default_pad_upload_buffer (GstGLMixer * mix, static void gst_gl_mixer_set_context (GstElement * element, GstContext * context); +static gboolean _clean_upload (GstAggregator * agg, GstAggregatorPad * aggpad, + gpointer udata); enum { @@ -574,10 +576,28 @@ static gboolean gst_gl_mixer_set_allocation (GstGLMixer * mix, static void gst_gl_mixer_finalize (GObject * object); static void +gst_gl_mixer_release_pad (GstElement * element, GstPad * pad) +{ + GstAggregatorPad *aggpad; + GstAggregator *agg = NULL; + + agg = GST_AGGREGATOR (element); + aggpad = GST_AGGREGATOR_PAD (pad); + + + _clean_upload (agg, aggpad, NULL); + GST_ELEMENT_CLASS (gst_gl_mixer_parent_class)->release_pad (GST_ELEMENT + (agg), pad); + + return; +} + +static void gst_gl_mixer_class_init (GstGLMixerClass * klass) { GObjectClass *gobject_class; GstElementClass *element_class; + GstElementClass *gstelement_class = (GstElementClass *) klass; GstVideoAggregatorClass *videoaggregator_class = (GstVideoAggregatorClass *) klass; @@ -602,6 +622,9 @@ gst_gl_mixer_class_init (GstGLMixerClass * klass) element_class->set_context = GST_DEBUG_FUNCPTR (gst_gl_mixer_set_context); + gstelement_class->release_pad = + GST_DEBUG_FUNCPTR (gst_gl_mixer_release_pad); + Maybe this is relevant for a 1.4.x backport also, though I am not working on 1.4.x at the moment. thanks for any hints
Yes, that seems sensible.