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 754212 - cutter: Fix buffer leak
cutter: Fix buffer leak
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-good
git master
Other Linux
: Normal normal
: 1.6.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2015-08-28 01:27 UTC by Vineeth
Modified: 2015-10-07 18:05 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
fix buffer leak (1.96 KB, patch)
2015-08-28 01:28 UTC, Vineeth
none Details | Review
fix buffer leak (2.32 KB, patch)
2015-08-28 07:46 UTC, Vineeth
none Details | Review
fix buffer leak (2.10 KB, patch)
2015-08-28 08:14 UTC, Vineeth
none Details | Review
fix buffer leak (2.13 KB, patch)
2015-10-05 04:11 UTC, Vineeth
committed Details | Review

Description Vineeth 2015-08-28 01:27:46 UTC
Buffer is added to the internal cache, and pushed only when accumulated buffer duration crosses 200 ms. So when the chain ends, the buffer accumulated is not freed. Adding a finalize method to free the cache.

leak with the below pipeline
audiotestsrc num-buffers=200 ! cutter threshold=1 ! fakesink

==27003== 18,424 (12 direct, 18,412 indirect) bytes in 1 blocks are definitely lost in loss record 3,577 of 3,579
==27003==    at 0x402C17C: malloc (in /usr/lib/valgrind/vgpreload_memcheck-x86-linux.so)
==27003==    by 0x43D1BE2: g_malloc (in /lib/i386-linux-gnu/libglib-2.0.so.0.4002.0)
==27003==    by 0x43E8281: g_slice_alloc (in /lib/i386-linux-gnu/libglib-2.0.so.0.4002.0)
==27003==    by 0x43C83E0: g_list_append (in /lib/i386-linux-gnu/libglib-2.0.so.0.4002.0)
==27003==    by 0x404817E: gst_cutter_chain (gstcutter.c:370)
==27003==    by 0x41F0BC7: gst_validate_pad_monitor_chain_func (gst-validate-pad-monitor.c:2039)
==27003==    by 0x427EC7D: gst_pad_push_data (gstpad.c:4085)
==27003==    by 0x4874A5A: gst_base_src_loop (gstbasesrc.c:2845)
==27003==    by 0x42B3138: gst_task_func (gsttask.c:331)
==27003==    by 0x42B41EE: default_func (gsttaskpool.c:68)
==27003==    by 0x43F3404: ??? (in /lib/i386-linux-gnu/libglib-2.0.so.0.4002.0)
==27003==    by 0x43F29A9: ??? (in /lib/i386-linux-gnu/libglib-2.0.so.0.4002.0)
==27003==    by 0x4498F6F: start_thread (pthread_create.c:312)
==27003==    by 0x4599BED: clone (clone.S:129)
Comment 1 Vineeth 2015-08-28 01:28:52 UTC
Created attachment 310146 [details] [review]
fix buffer leak
Comment 2 Sebastian Dröge (slomo) 2015-08-28 07:12:51 UTC
Comment on attachment 310146 [details] [review]
fix buffer leak

Anything stream specific should be freed at latest in PAUSED->READY, or when the caps change, or in the case of buffers when FLUSH_STOP happens.
Comment 3 Vineeth 2015-08-28 07:46:55 UTC
Created attachment 310152 [details] [review]
fix buffer leak

Freeing the buffer in PAUSED to READY, as flush stop is not getting called for this.

Please review :)
Comment 4 Sebastian Dröge (slomo) 2015-08-28 08:02:53 UTC
Review of attachment 310152 [details] [review]:

::: gst/cutter/gstcutter.c
@@ +253,3 @@
+    {
+      GstBuffer *prebuf;
+      while (filter->pre_buffer) {

Why not just g_list_free_full(filter->pre_buffer, (GFunc) gst_buffer_unref, NULL)? :)


And I don't know what this element is doing or if it works at all, so please check if it makes sense to also free those buffers on caps changes or flushing ;)
Comment 5 Vineeth 2015-08-28 08:14:30 UTC
Created attachment 310156 [details] [review]
fix buffer leak

I think, freeing here is enough. Not needed in caps change and flushing
Comment 6 Sebastian Dröge (slomo) 2015-10-02 14:37:06 UTC
Review of attachment 310156 [details] [review]:

::: gst/cutter/gstcutter.c
@@ +251,3 @@
+  switch (transition) {
+    case GST_STATE_CHANGE_PAUSED_TO_READY:
+      g_list_free_full (filter->pre_buffer, (GDestroyNotify) gst_buffer_unref);

You should set the list to NULL after freeing.
Comment 7 Vineeth 2015-10-05 04:11:52 UTC
Created attachment 312660 [details] [review]
fix buffer leak

Setting list to NULL after freeing
Comment 8 Sebastian Dröge (slomo) 2015-10-05 11:04:08 UTC
commit 15b08e0bd509df13cafdff13bc067aaa7c6bb2aa
Author: Vineeth TM <vineeth.tm@samsung.com>
Date:   Mon Oct 5 13:10:56 2015 +0900

    cutter: Fix buffer leak
    
    Buffer is added to the internal cache, and pushed only when accumulated
    buffer duration crosses 200 ms. So when the chain ends, the buffer accumulated
    is not freed. Freeing the cache when the state changes from PAUSED to READY.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=754212