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 722462 - tsdemux: Fix leak of PCROffsetGroup
tsdemux: Fix leak of PCROffsetGroup
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-bad
git master
Other All
: Normal normal
: 1.3.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2014-01-17 23:24 UTC by Andrey Utkin
Modified: 2014-01-18 09:34 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
patch (1.06 KB, patch)
2014-01-17 23:24 UTC, Andrey Utkin
needs-work Details | Review
patch v2 (1.12 KB, patch)
2014-01-18 01:57 UTC, Andrey Utkin
committed Details | Review

Description Andrey Utkin 2014-01-17 23:24:36 UTC
Created attachment 266588 [details] [review]
patch

How to reproduce:

 $ ffmpeg -loglevel fatal -t 10 -f lavfi -i testsrc -f mpegts - | G_SLICE=always-malloc valgrind --suppressions=/usr/local/src/gstreamer/common/gst.supp --show-reachable=no gst-launch-1.0 filesrc location=/dev/stdin ! tsdemux ! fakesink
==9174== Memcheck, a memory error detector
==9174== Copyright (C) 2002-2012, and GNU GPL'd, by Julian Seward et al.
==9174== Using Valgrind-3.8.1 and LibVEX; rerun with -h for copyright info
==9174== Command: gst-launch-1.0 filesrc location=/dev/stdin ! tsdemux ! fakesink
==9174== 
GStreamer has detected that it is running inside valgrind.
It might now take different code paths to ease debugging.
Of course, this may also lead to different bugs.
Setting pipeline to PAUSED ...
Pipeline is PREROLLING ...
Pipeline is PREROLLED ...
Setting pipeline to PLAYING ...
New clock: GstSystemClock
Got EOS from element "pipeline0".
Execution ended after 0:00:00.276477238
Setting pipeline to PAUSED ...
Setting pipeline to READY ...
Setting pipeline to NULL ...
Freeing pipeline ...
==9174== 
==9174== HEAP SUMMARY:
==9174==     in use at exit: 394,590 bytes in 1,943 blocks
==9174==   total heap usage: 73,890 allocs, 71,947 frees, 7,246,127 bytes allocated
==9174== 
==9174== 200 (24 direct, 176 indirect) bytes in 1 blocks are definitely lost in loss record 1,753 of 1,819
==9174==    at 0x4C2C5DB: malloc (vg_replace_malloc.c:270)
==9174==    by 0x591221A: g_malloc (gmem.c:159)
==9174==    by 0x592AC50: g_slice_alloc (gslice.c:1003)
==9174==    by 0x5905FB2: g_list_prepend (glist.c:279)
==9174==    by 0x78B76D2: _insert_group_after (mpegtspacketizer.c:1702)
==9174==    by 0x78B7FB4: _set_current_group (mpegtspacketizer.c:1787)
==9174==    by 0x78B8DFF: record_pcr (mpegtspacketizer.c:1897)
==9174==    by 0x78B0D6E: mpegts_packetizer_parse_adaptation_field_control (mpegtspacketizer.c:376)
==9174==    by 0x78B1238: mpegts_packetizer_parse_packet (mpegtspacketizer.c:448)
==9174==    by 0x78B2167: mpegts_packetizer_next_packet (mpegtspacketizer.c:816)
==9174==    by 0x78C0864: mpegts_base_chain (mpegtsbase.c:1109)
==9174==    by 0x4EB12A4: gst_pad_chain_data_unchecked (gstpad.c:3773)
==9174==    by 0x4EB1F37: gst_pad_push_data (gstpad.c:4006)
==9174==    by 0x4EB2488: gst_pad_push (gstpad.c:4109)
==9174==    by 0x767904F: gst_base_src_loop (gstbasesrc.c:2785)
==9174==    by 0x4EE6047: gst_task_func (gsttask.c:316)
==9174==    by 0x4EE7142: default_func (gsttaskpool.c:70)
==9174==    by 0x59379BD: g_thread_pool_thread_proxy (gthreadpool.c:309)
==9174==    by 0x59373D2: g_thread_proxy (gthread.c:798)
==9174==    by 0x601BDA5: start_thread (in /lib64/libpthread-2.15.so)
==9174== 
==9174== LEAK SUMMARY:
==9174==    definitely lost: 24 bytes in 1 blocks
==9174==    indirectly lost: 176 bytes in 2 blocks
==9174==      possibly lost: 0 bytes in 0 blocks
==9174==    still reachable: 32,188 bytes in 561 blocks
==9174==         suppressed: 362,202 bytes in 1,379 blocks
==9174== Reachable blocks (those to which a pointer was found) are not shown.
==9174== To see them, rerun with: --leak-check=full --show-reachable=yes
==9174== 
==9174== For counts of detected and suppressed errors, rerun with: -v
==9174== ERROR SUMMARY: 1 errors from 1 contexts (suppressed: 152 from 152)
Comment 1 Thiago Sousa Santos 2014-01-18 01:21:39 UTC
Review of attachment 266588 [details] [review]:

I can confirm that this patch fixes the leak, just have one minor nitpick about it.

::: gst/mpegtsdemux/mpegtspacketizer.c
@@ -102,1 +103,4 @@
   for (i = 0; i < packetizer->lastobsid; i++) {
+    for (tmp = packetizer->observations[i]->groups; tmp; tmp = tmp->next) {
+      g_free (((PCROffsetGroup *) tmp->data)->values);
+      g_slice_free (PCROffsetGroup, tmp->data);

How about creating a pcr_offset_group_free function and call it from here with maybe using g_list_free_full instead?

Should make it easier to understand and reuse in case needed.
Comment 2 Andrey Utkin 2014-01-18 01:57:44 UTC
Created attachment 266598 [details] [review]
patch v2

rearranged code by request
Comment 3 Thiago Sousa Santos 2014-01-18 03:33:59 UTC
Thanks for the update, did a minimum change and pushed it to master

commit 0caa4cdfd876b13748a91f119543261062af43ea
Author: Andrey Utkin <andrey.krieger.utkin@gmail.com>
Date:   Sat Jan 18 01:19:36 2014 +0200

    tsdemux: Fix leak of PCROffsetGroup
    
    https://bugzilla.gnome.org/show_bug.cgi?id=722462
Comment 4 Andrey Utkin 2014-01-18 06:45:21 UTC
Thanks.