Does it matter for a single pipeline run? IMHO the leak is only bad if it would repeadedly request pads.

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.

Oups, wrong bug id. Sorry.

It breaks the make check-valgrind in core :/

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.

The only sane way is to keep track of which pads were request pads and release them appropriatly.

Wim, you mean keep track in gst-launch?

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",

*** Bug 636011 has been marked as a duplicate of this bug. ***

Created attachment 175857 [details] [review]
patch

Full patch.  Breaks a bunch of elements, but fixes the leaking problem with mpegtsmux, as in #636011.

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.

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

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

(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.

Will push this after the release, since there is still a chance for regressions in plugins I haven't fixed yet.

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.

That commit broke expected behaviour for request pads.

The proper fix is in comment #6.

We'll have to revert commit a5e44ffffaa6d7a8d7af8dcb77e37990996253a5 and all dependent commits.

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).

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.

(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.

Ok, so let's get something like this patch in again.... but maybe after 0.10.33? Or what's the plan now?

Not a blocker afaics...

David? Maybe only for 0.11 then?

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