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 672006 - [playsink] leaks every instance
[playsink] leaks every instance
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-base
git master
Other Linux
: Normal major
: 0.10.37
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2012-03-13 16:52 UTC by Mark Nauwelaerts
Modified: 2012-07-25 09:01 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
playsink: avoid dangling circular refs (3.98 KB, patch)
2012-03-13 16:55 UTC, Mark Nauwelaerts
none Details | Review
pad: also update callback info when pad in desired blocked state (980 bytes, patch)
2012-03-13 16:58 UTC, Mark Nauwelaerts
none Details | Review
playsink: avoid circular ref between bin and internal pad (8.24 KB, patch)
2012-03-14 10:07 UTC, Mark Nauwelaerts
committed Details | Review

Description Mark Nauwelaerts 2012-03-13 16:52:52 UTC
Both playsink and the helper internal convert bin use following fragments frequently:

      gst_pad_set_blocked_async_full (opad, FALSE, sinkpad_blocked_cb,
          gst_object_ref (playsink), (GDestroyNotify) gst_object_unref);

The result is a (indirect) circular ref between opad (a pad somewhere internal in the bin) and the bin (e.g. playsink), so none of this gets cleaned up at overall pipeline _unref time.

It is also not reported by valgrind, unless one digs into the "still reachable".

Not entirely sure why additional refs were taken here in the first place in such persistent fashion to begin with ...
Comment 1 Mark Nauwelaerts 2012-03-13 16:55:51 UTC
Created attachment 209626 [details] [review]
playsink: avoid dangling circular refs

Patch breaks circular ref at shutdown (state change) time.  Only works though if core is also patched slightly (patch to follow).
Comment 2 Mark Nauwelaerts 2012-03-13 16:58:22 UTC
Created attachment 209627 [details] [review]
pad: also update callback info when pad in desired blocked state

To allow previous patch to really get rid of extra ref, core needs patching slightly to update callback info in more cases.

Since it seems that the fields in question appear to be public ones in the pad instance struct, the previous patch could also have manipulated those directly (or done something else altogether), but this feels like a cleaner approach.
Comment 3 Sebastian Dröge (slomo) 2012-03-14 09:30:00 UTC
I think the additional refs/unrefs for the pad blocks are not needed at all here.
Comment 4 Mark Nauwelaerts 2012-03-14 10:07:46 UTC
Created attachment 209686 [details] [review]
playsink: avoid circular ref between bin and internal pad

In that case, alternatively, present patch removes the additional _ref/_unref altogether.
Comment 5 Sebastian Dröge (slomo) 2012-03-14 10:12:47 UTC
Could you check in the other elements in the playback directory too? IIRC the same pattern is used in subtitleoverlay too, and decodebin2 too maybe.

Taking an additional ref should always be unneeded if the pad in question belongs to a child element of our bin
Comment 6 Mark Nauwelaerts 2012-03-14 10:36:25 UTC
Looks like it was already fixed in subtitleoverlay a while ago, and the extra ref in decodebin2 is taken on some internal pad, so it does not create a circular ref and will eventually go away.
Comment 7 Mark Nauwelaerts 2012-03-14 16:33:53 UTC
So, latest patch should do it then:

commit 9fc640b9e0ba807b7f8ce6bdbd7c754bdbfdb761
Author: Mark Nauwelaerts <mark.nauwelaerts@collabora.co.uk>
Date:   Wed Mar 14 11:04:25 2012 +0100

    playsink: remove circular ref between bin and internal pad
    
    ... by not assigning an additional ref to an async blocked callback,
    which should not be called anyway by the time the object is gone.
    
    Fixes #672006.
Comment 8 soho123.2012 2012-07-25 05:29:23 UTC
(In reply to comment #7)
> So, latest patch should do it then:
> 
> commit 9fc640b9e0ba807b7f8ce6bdbd7c754bdbfdb761
> Author: Mark Nauwelaerts <mark.nauwelaerts@collabora.co.uk>
> Date:   Wed Mar 14 11:04:25 2012 +0100
> 
>     playsink: remove circular ref between bin and internal pad
> 
>     ... by not assigning an additional ref to an async blocked callback,
>     which should not be called anyway by the time the object is gone.
> 
>     Fixes #672006.
hi,

Is there any patch necessary for gstreamer-0.10.36 tar file for this issue?
Comment 9 Tim-Philipp Müller 2012-07-25 08:37:46 UTC
No, the patch is in gst-plugins-base, and I gave you the link in the other bug report.
Comment 10 soho123.2012 2012-07-25 09:01:57 UTC
(In reply to comment #9)
> No, the patch is in gst-plugins-base, and I gave you the link in the other bug
> report.

hi Sir,

this may the cause for memory leak issue in base 0.10.36.
the verification testing is running.
Because the previous comments mentioned the patch for core (gstpad.c), 
I just like to confirm whether necessary patch core 0.10.36 or not.
And, 
Could you explain more detail about :
playsink: remove circular ref between bin and internal pad


thanks!