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 402562 - [gst_parse_launch] Requesting pads but not releasing them
[gst_parse_launch] Requesting pads but not releasing them
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gstreamer (core)
git master
Other Linux
: Normal major
: 0.11.2
Assigned To: David Schleef
GStreamer Maintainers
: 636011 (view as bug list)
Depends on:
Blocks:
 
 
Reported: 2007-01-30 19:41 UTC by Sebastian Dröge (slomo)
Modified: 2012-01-26 15:44 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
release requenstpads when unlinking elements upon removal (1.23 KB, patch)
2008-08-28 12:12 UTC, Stefan Sauer (gstreamer, gtkdoc dev)
none Details | Review
patch (1.32 KB, patch)
2010-12-05 03:15 UTC, David Schleef
none Details | Review
call release_pad at dispose time (1.86 KB, patch)
2010-12-05 03:41 UTC, David Schleef
committed Details | Review

Description Sebastian Dröge (slomo) 2007-01-30 19:41:12 UTC
Hi,
as discussed on IRC (also see bug #402393) gst_parse_launch is requesting pads but does not care for releasing after usage. This could possibly lead to leaks somewhere.

Bye
Comment 1 Stefan Sauer (gstreamer, gtkdoc dev) 2007-08-05 18:24:33 UTC
Does it matter for a single pipeline run? IMHO the leak is only bad if it would repeadedly request pads.
Comment 2 Julien Isorce 2008-07-21 19:27:27 UTC
Hi, 

Note that gst_element_link_filtered (videoscale, textoverlay, caps) failed. 
And gst_element_link_pads_filtered(videoscale, "src", textoverlay, "video_sink", caps) works.

J.
Comment 3 Julien Isorce 2008-07-21 19:28:59 UTC
Oups, wrong bug id. Sorry.
Comment 4 Stefan Sauer (gstreamer, gtkdoc dev) 2008-08-28 08:45:11 UTC
It breaks the make check-valgrind in core :/
Comment 5 Stefan Sauer (gstreamer, gtkdoc dev) 2008-08-28 12:12:51 UTC
Created attachment 117520 [details] [review]
release requenstpads when unlinking elements upon removal

Bah, grammar.y uses gst_element_link_pads_filtered() and therefore the use of requestpads is entierly internal. 

Now I wonder if in theory gstbin.c::unlink_pads() should do cleanup like in the patch. Te patch has one flaw though. I am afraid it is not safe to release the request-pad when removing an element from the bin, unless its the request-pad that belongs to the element being removed. Just imagine having:
tee ! xyz
when app remove xyz, its unlinked, but we can't release the requestpad, as the app might want to link something to it. Not sure if we could use refcounts of the request pad.
Comment 6 Wim Taymans 2008-10-13 14:11:11 UTC
The only sane way is to keep track of which pads were request pads and release them appropriatly.
Comment 7 Stefan Sauer (gstreamer, gtkdoc dev) 2008-10-13 16:29:24 UTC
Wim, you mean keep track in gst-launch?
Comment 8 David Schleef 2010-12-05 03:12:17 UTC
I think the problem here is that gst_element_dispose() calls the internal function instead of calling ->release_pad().  Unfortunately, this makes lots of element fail, probably because they're not expecting release_pad() to be called so late in the cleanup process.


@@ -2916,7 +2919,10 @@ gst_element_dispose (GObject * object)
   /* first we break all our links with the outside */
   while (element->pads && element->pads->data) {
     /* don't call _remove_pad with NULL */
-    gst_element_remove_pad (element, GST_PAD_CAST (element->pads->data));
+    if (oclass->release_pad)
+      (oclass->release_pad) (element, GST_PAD_CAST (element->pads->data));
+    else
+      gst_element_remove_pad (element, GST_PAD_CAST (element->pads->data));
   }
   if (G_UNLIKELY (element->pads != NULL)) {
     g_critical ("could not remove pads from element %s",
Comment 9 David Schleef 2010-12-05 03:12:36 UTC
*** Bug 636011 has been marked as a duplicate of this bug. ***
Comment 10 David Schleef 2010-12-05 03:15:12 UTC
Created attachment 175857 [details] [review]
patch

Full patch.  Breaks a bunch of elements, but fixes the leaking problem with mpegtsmux, as in #636011.
Comment 11 David Schleef 2010-12-05 03:41:18 UTC
Created attachment 175858 [details] [review]
call release_pad at dispose time

New patch, this time only calling release_pad on request pads.  Fixes the segfaults, and seems to work well.
Comment 12 Sebastian Dröge (slomo) 2010-12-11 16:31:26 UTC
I talked to Wim about the same change some months ago and he said that pads should never be released automatically*. The application requested them and only the application should ever release them again later.

* of course automatically requested pads by gst_parse_launch() should be released automatically when the pipeline gets destroyed
Comment 13 Sebastian Dröge (slomo) 2010-12-11 16:32:50 UTC
Also your patch leads to crashes when pads without template are added to the element... but other than that the patch is fine if we really want this behaviour
Comment 14 David Schleef 2010-12-11 18:28:59 UTC
(In reply to comment #12)
> I talked to Wim about the same change some months ago and he said that pads
> should never be released automatically*.

Presumably this rule does not apply once the parent element is destroyed.

I've been running this patch for a while now -- there have been some crashers in plugin code that's not expecting this behavior, so I'll make sure those get fixed first.
Comment 15 David Schleef 2011-01-05 21:42:07 UTC
Will push this after the release, since there is still a chance for regressions in plugins I haven't fixed yet.
Comment 16 David Schleef 2011-02-18 03:21:47 UTC
commit a5e44ffffaa6d7a8d7af8dcb77e37990996253a5
Author: David Schleef <ds@schleef.org>
Date:   Sat Dec 4 18:53:55 2010 -0800

    element: Call ->release_pad() to clean up pad
    
    Fixes #636011 and #402562.
Comment 17 Edward Hervey 2011-02-21 10:46:38 UTC
That commit broke expected behaviour for request pads.
Comment 18 Edward Hervey 2011-02-21 10:47:34 UTC
The proper fix is in comment #6.

We'll have to revert commit a5e44ffffaa6d7a8d7af8dcb77e37990996253a5 and all dependent commits.
Comment 19 Mark Nauwelaerts 2011-02-21 11:06:10 UTC
Also, fwiw, even if application does not release them, most (if not all) muxers for instance arrange to have their request pads cleaned up at _finalize time, so it should not leak anyway (and if it does, it is a bit of an element bug).
Comment 20 Edward Hervey 2011-02-21 11:34:14 UTC
Commit: b39ccb5ac3e5be1851f1dc10b004610de1520a01
URL:    http://cgit.freedesktop.org/gstreamer/gstreamer/commit/?id=b39ccb5ac3e5be1851f1dc10b004610de1520a01

Author: Edward Hervey <edward.hervey@collabora.co.uk>
Date:   Mon Feb 21 11:55:50 2011 +0100

Revert "element: Call ->release_pad() to clean up pad"

This commit changes the request pad behaviour for plugins and applications.

Reopens Bug #402562

The proper fix for that bug is to keep track of created request pads.

This reverts commit a5e44ffffaa6d7a8d7af8dcb77e37990996253a5.
Comment 21 David Schleef 2011-03-25 21:18:09 UTC
(In reply to comment #17)
> That commit broke expected behaviour for request pads.

Actually, no it doesn't.

When the parent element is finalized, the behavior of request pads is random.  Some elements clean them up, others don't.  This patch simply took the existing behavior of the more popular elements (qtmux, matroskamux, videomixer) and made it the default behavior.

The resulting bugs were because broken elements were broken.  They were not expecting pads to be released in ElementClass's finalize().  They are still broken, as any app that unref's the pad after the pipeline will trigger the same bugs.

My original core patch should have cleaned up request pads in dispose, not finalize, thus avoiding triggering the element bugs anyway.
Comment 22 Sebastian Dröge (slomo) 2011-04-08 12:46:36 UTC
Ok, so let's get something like this patch in again.... but maybe after 0.10.33? Or what's the plan now?
Comment 23 Tim-Philipp Müller 2011-04-09 19:14:50 UTC
Not a blocker afaics...
Comment 24 Sebastian Dröge (slomo) 2011-05-30 15:56:41 UTC
David? Maybe only for 0.11 then?
Comment 25 Tim-Philipp Müller 2012-01-26 15:44:37 UTC
commit 9048b4dc2af3d80ff696280565143975da80d017
Author: David Schleef <ds@schleef.org>
Date:   Sat Dec 4 18:53:55 2010 -0800

    element: call ->release_pad() to clean up pad
    
    https://bugzilla.gnome.org/show_bug.cgi?id=636011
    https://bugzilla.gnome.org/show_bug.cgi?id=402562