After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 750881 - glmixerbin: correctly free input chain on pad release
glmixerbin: correctly free input chain on pad release
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-bad
git master
Other Linux
: Normal normal
: 1.5.2
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2015-06-12 22:11 UTC by Vasilis Liaskovitis
Modified: 2015-06-15 08:04 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
gstglmixerbin: compare with ghost_pad on pad release (1.01 KB, patch)
2015-06-12 22:11 UTC, Vasilis Liaskovitis
none Details | Review
dynamic source mix (2.17 KB, text/x-c)
2015-06-13 08:46 UTC, Matthew Waters (ystreet00)
  Details

Description Vasilis Liaskovitis 2015-06-12 22:11:07 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?
Comment 1 Matthew Waters (ystreet00) 2015-06-13 08:46:42 UTC
Created attachment 305190 [details]
dynamic source mix

Test application
Comment 2 Matthew Waters (ystreet00) 2015-06-13 08:48:25 UTC
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
Comment 3 Vasilis Liaskovitis 2015-06-13 12:08:02 UTC
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
Comment 4 Matthew Waters (ystreet00) 2015-06-15 08:04:02 UTC
Yes, that seems sensible.