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 402393 - [API][GstCollectPads] Allow elements to specify destroy notify for custom GstCollectData
[API][GstCollectPads] Allow elements to specify destroy notify for custom Gst...
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gstreamer (core)
git master
Other Linux
: Normal normal
: 0.10.12
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2007-01-30 09:32 UTC by James "Doc" Livingston
Modified: 2007-02-02 08:24 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
collectpads-destroy-notify.diff (5.47 KB, patch)
2007-01-30 19:42 UTC, Sebastian Dröge (slomo)
committed Details | Review
oggmux-leak.diff (2.23 KB, patch)
2007-01-30 19:43 UTC, Sebastian Dröge (slomo)
committed Details | Review
collectpads-abi.diff (3.20 KB, patch)
2007-02-01 18:51 UTC, Sebastian Dröge (slomo)
committed Details | Review

Description James "Doc" Livingston 2007-01-30 09:32:44 UTC
GstOggMux leaks some memory when it is finalised without ever having it's state changed but a sink pad was added. An example of this happening is when gnome-media's audio profile code creates a pipeline, just to see if it can.

gst_ogg_mux_request_new_pad() is called to create the new pad, but gst_ogg_mux_release_pad() is never called and gst_ogg_mux_clear_collectpads() isn't run because of the lack of downward state change.




Valgrind output:

==18548== 4,096 bytes in 1 blocks are definitely lost in loss record 21 of 28
==18548==    at 0x4021380: malloc (vg_replace_malloc.c:149)
==18548==    by 0x46CC56A: ogg_stream_init (in /usr/lib/libogg.so.0.5.3)
==18548==    by 0x4B77D42: gst_ogg_mux_request_new_pad (gstoggmux.c:417)
==18548==    by 0x409F4CA: gst_element_request_compatible_pad (gstutils.c:793)
==18548==    by 0x409F698: gst_element_get_compatible_pad (gstutils.c:967)
==18548==    by 0x40A026A: gst_element_link_pads (gstutils.c:1469)
==18548==    by 0x40A0CBC: gst_element_link_pads_filtered (gstutils.c:1660)
==18548==    by 0x40B5EEB: _gst_parse_launch (grammar.y:406)
==18548==    by 0x40A9847: gst_parse_launch (gstparse.c:159)
==18548==    by 0x80485CC: main (in /home/doc/programming/sources/gst-plugins-base/ext/ogg/foo)
==18548== 
==18548== 
==18548== 8,192 bytes in 1 blocks are definitely lost in loss record 24 of 28
==18548==    at 0x4021380: malloc (vg_replace_malloc.c:149)
==18548==    by 0x46CC57B: ogg_stream_init (in /usr/lib/libogg.so.0.5.3)
==18548==    by 0x4B77D42: gst_ogg_mux_request_new_pad (gstoggmux.c:417)
==18548==    by 0x409F4CA: gst_element_request_compatible_pad (gstutils.c:793)
==18548==    by 0x409F698: gst_element_get_compatible_pad (gstutils.c:967)
==18548==    by 0x40A026A: gst_element_link_pads (gstutils.c:1469)
==18548==    by 0x40A0CBC: gst_element_link_pads_filtered (gstutils.c:1660)
==18548==    by 0x40B5EEB: _gst_parse_launch (grammar.y:406)
==18548==    by 0x40A9847: gst_parse_launch (gstparse.c:159)
==18548==    by 0x80485CC: main (in /home/doc/programming/sources/gst-plugins-base/ext/ogg/foo)
==18548== 
==18548== 
==18548== 16,384 bytes in 1 blocks are definitely lost in loss record 25 of 28
==18548==    at 0x4021380: malloc (vg_replace_malloc.c:149)
==18548==    by 0x46CC555: ogg_stream_init (in /usr/lib/libogg.so.0.5.3)
==18548==    by 0x4B77D42: gst_ogg_mux_request_new_pad (gstoggmux.c:417)
==18548==    by 0x409F4CA: gst_element_request_compatible_pad (gstutils.c:793)
==18548==    by 0x409F698: gst_element_get_compatible_pad (gstutils.c:967)
==18548==    by 0x40A026A: gst_element_link_pads (gstutils.c:1469)
==18548==    by 0x40A0CBC: gst_element_link_pads_filtered (gstutils.c:1660)
==18548==    by 0x40B5EEB: _gst_parse_launch (grammar.y:406)
==18548==    by 0x40A9847: gst_parse_launch (gstparse.c:159)
==18548==    by 0x80485CC: main (in /home/doc/programming/sources/gst-plugins-base/ext/ogg/foo)


from the following program:

#include <gst/gst.h>
 
int main()
{
        GstElement *p;
 
        gst_init (NULL, NULL);
 
        p = gst_parse_launch ("fakesrc ! audioconvert ! vorbisenc ! oggmux ! fakesink", NULL);
        g_object_unref (p);
 
        gst_deinit ();
}
Comment 1 Sebastian Dröge (slomo) 2007-01-30 15:20:57 UTC
I can confirm this here with latest CVS and latest releases... will take a closer look later. Thanks for reporting :)
Comment 2 Sebastian Dröge (slomo) 2007-01-30 16:30:48 UTC
Ok, after the discussion on IRC it seems that a) gst_parse_launch should do something to keep care that request-pads are released in the end and b) some functions (gst_element_link for example) automatically request pads which make it additionally hard for applications to release them later (as the application does not know that there were requested pads)
Comment 3 Sebastian Dröge (slomo) 2007-01-30 19:41:32 UTC
Ok, it seems that this is in fact caused by the entries in GstOggPad that must be freed but can't because of API limitations in GstCollectPads.

To allow elements to use custom GstCollectData with entries that must be freed I propose the addition of a destroy notify for GstCollectPads which will be called when the GstCollectData will be freed.

The public API additions are:
typedef void (*GstCollectDataDestroyNotify) (GstCollectData* data);

GstCollectData *gst_collect_data_add_pad_full (GstCollectPads * pads, GstPad * pad, guint size, GstCollectDataDestroyNotify destroy_notify);

Everything is else private. Attached is a patch to implement this and a patch to use this in oggmux.


The other two bugs are still relevant but not the cause of this one :)

a) is filed as bug #402562  b) as bug #402497
Comment 4 Sebastian Dröge (slomo) 2007-01-30 19:42:38 UTC
Created attachment 81538 [details] [review]
collectpads-destroy-notify.diff

GstCollectPads API additions
Comment 5 Sebastian Dröge (slomo) 2007-01-30 19:43:10 UTC
Created attachment 81539 [details] [review]
oggmux-leak.diff

fix for the oggmux leak by using the new GstCollectPads API
Comment 6 Sebastian Dröge (slomo) 2007-02-01 17:53:01 UTC
Committed both patches...

2007-02-01  Sebastian Dröge  <slomo@circular-chaos.org>

        reviewed by: Wim Taymans <wim@fluendo.com>

        * docs/libs/gstreamer-libs-sections.txt:
        * libs/gst/base/gstcollectpads.c: (gst_collect_pads_finalize),
        (unref_data), (gst_collect_pads_add_pad),
        (gst_collect_pads_add_pad_full):
        * libs/gst/base/gstcollectpads.h:
        API: Add function to specify a destroy notification for custom
        GstCollectData when adding new pads in GstCollectPads (#402393).

2007-02-01  Sebastian Dröge  <slomo@circular-chaos.org>

        reviewed by: Wim Taymans <wim@fluendo.com>

        * ext/ogg/gstoggmux.c: (gst_ogg_mux_ogg_pad_destroy_notify),
        (gst_ogg_mux_request_new_pad), (gst_ogg_mux_release_pad):
        Use newly added GstCollectPads API to free the allocated resources in
        the GstOggPad structures (#402393).
Comment 7 Sebastian Dröge (slomo) 2007-02-01 18:51:16 UTC
This apparently breaks ABI because GstCollectData already contains as much new fields as padding was added in the beginning. New patch that works around this in the probably best but nonetheless ugly way is attached.
Comment 8 Sebastian Dröge (slomo) 2007-02-01 18:51:35 UTC
Created attachment 81688 [details] [review]
collectpads-abi.diff
Comment 9 Sebastian Dröge (slomo) 2007-02-01 19:01:24 UTC
2007-02-01  Sebastian Dröge  <slomo@circular-chaos.org>

	reviewed by: Tim-Philipp Müller <tim at centricular dot net>

	* libs/gst/base/gstcollectpads.c: (gst_collect_pads_finalize),
	(unref_data), (gst_collect_pads_add_pad_full):
	* libs/gst/base/gstcollectpads.h:
	Don't put the previously added destroy notify in the GstCollectData
	struct as all it's padding is already used and we don't want to break
	ABI. Instead put in the pad's GObject data for now. This should be
	cleaned up for 0.11 (#402393).