GNOME Bugzilla – Bug 402393
[API][GstCollectPads] Allow elements to specify destroy notify for custom GstCollectData
Last modified: 2007-02-02 08:24:03 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 (); }
I can confirm this here with latest CVS and latest releases... will take a closer look later. Thanks for reporting :)
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)
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
Created attachment 81538 [details] [review] collectpads-destroy-notify.diff GstCollectPads API additions
Created attachment 81539 [details] [review] oggmux-leak.diff fix for the oggmux leak by using the new GstCollectPads API
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).
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.
Created attachment 81688 [details] [review] collectpads-abi.diff
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).