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 796452 - avdemux:fix memory leak
avdemux:fix memory leak
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-libav
git master
Other Linux
: Normal normal
: 1.14.2
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2018-05-30 09:26 UTC by rland
Modified: 2018-07-18 16:36 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
valgrind_result (312.65 KB, application/x-bzip)
2018-05-30 09:26 UTC, rland
  Details
proposed patch (1.17 KB, patch)
2018-05-30 09:29 UTC, rland
none Details | Review
avdemux-fix-memory-leak (4.92 KB, patch)
2018-05-31 02:27 UTC, rland
none Details | Review
avdemux fix memory leak. (5.11 KB, patch)
2018-05-31 02:51 UTC, rland
none Details | Review
avdemux:fix memory leak. (4.72 KB, patch)
2018-05-31 09:33 UTC, rland
committed Details | Review

Description rland 2018-05-30 09:26:43 UTC
Created attachment 372475 [details]
valgrind_result

Run:
valgrind -v --tool=memcheck  --track-fds=yes  --leak-check=full --show-reachable=yes --time-stamp=yes --undef-value-errors=no --malloc-fill=0xc --free-fill=0xd --freelist-vol=100000000 --trace-children=yes --num-callers=50  --suppressions=/home/shakin/work/src/ssc/gstreamer/cerbero/build/sources/linux_x86_64/gstreamer-1.0-1.14.0/common/gst.supp  --log-file=valgrind_gst gst-play-1.0 '/home/shakin/Desktop/test.ape'


definitely lost:
-------
==00:00:00:37.858 11101== 728 (88 direct, 640 indirect) bytes in 1 blocks are definitely lost in loss record 6,434 of 6,701
==00:00:00:37.858 11101==    at 0x4C2DB8F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==00:00:00:37.858 11101==    by 0x5E368FC: g_malloc (gmem.c:94)
==00:00:00:37.858 11101==    by 0x5E51721: g_slice_alloc (gslice.c:1025)
==00:00:00:37.858 11101==    by 0x5E19D0C: g_hash_table_new_full (ghash.c:717)
==00:00:00:37.858 11101==    by 0x5E19CE8: g_hash_table_new (ghash.c:680)
==00:00:00:37.858 11101==    by 0xC2011E5: gst_ffmpeg_metadata_to_tag_list (gstavdemux.c:1093)
==00:00:00:37.858 11101==    by 0xC200F59: gst_ffmpegdemux_get_stream (gstavdemux.c:1035)
==00:00:00:37.858 11101==    by 0xC201AEB: gst_ffmpegdemux_open (gstavdemux.c:1245)
==00:00:00:37.858 11101==    by 0xC2026C5: gst_ffmpegdemux_loop (gstavdemux.c:1406)
==00:00:00:37.858 11101==    by 0x58EBFD3: gst_task_func (gsttask.c:332)
==00:00:00:37.858 11101==    by 0x58ED1BB: default_func (gsttaskpool.c:69)
==00:00:00:37.858 11101==    by 0x5E60278: g_thread_pool_thread_proxy (gthreadpool.c:307)
==00:00:00:37.858 11101==    by 0x5E5FC7B: g_thread_proxy (gthread.c:784)
==00:00:00:37.858 11101==    by 0x645B6B9: start_thread (pthread_create.c:333)

...

==00:00:00:37.858 11101== 728 (88 direct, 640 indirect) bytes in 1 blocks are definitely lost in loss record 6,435 of 6,701
==00:00:00:37.858 11101==    at 0x4C2DB8F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==00:00:00:37.858 11101==    by 0x5E368FC: g_malloc (gmem.c:94)
==00:00:00:37.858 11101==    by 0x5E51721: g_slice_alloc (gslice.c:1025)
==00:00:00:37.858 11101==    by 0x5E19D0C: g_hash_table_new_full (ghash.c:717)
==00:00:00:37.858 11101==    by 0x5E19CE8: g_hash_table_new (ghash.c:680)
==00:00:00:37.858 11101==    by 0xC2011E5: gst_ffmpeg_metadata_to_tag_list (gstavdemux.c:1093)
==00:00:00:37.858 11101==    by 0xC202102: gst_ffmpegdemux_open (gstavdemux.c:1292)
==00:00:00:37.858 11101==    by 0xC2026C5: gst_ffmpegdemux_loop (gstavdemux.c:1406)
==00:00:00:37.858 11101==    by 0x58EBFD3: gst_task_func (gsttask.c:332)
==00:00:00:37.858 11101==    by 0x58ED1BB: default_func (gsttaskpool.c:69)
==00:00:00:37.858 11101==    by 0x5E60278: g_thread_pool_thread_proxy (gthreadpool.c:307)
==00:00:00:37.858 11101==    by 0x5E5FC7B: g_thread_proxy (gthread.c:784)
==00:00:00:37.858 11101==    by 0x645B6B9: start_thread (pthread_create.c:333)
Comment 1 rland 2018-05-30 09:29:23 UTC
Created attachment 372476 [details] [review]
proposed patch

proposed patch
Comment 2 Edward Hervey 2018-05-30 13:10:43 UTC
Review of attachment 372476 [details] [review]:

::: ext/libav/gstavdemux.c
@@ +1199,3 @@
 
+  if (tagmap)
+    g_hash_table_unref (tagmap);

You can't do this. That hashtable is meant to be re-used.

The proper fix would be to convert that tagmap to a regular static array (instead of a dynamicly built one).
Comment 3 rland 2018-05-31 02:27:16 UTC
Created attachment 372485 [details] [review]
avdemux-fix-memory-leak

>You can't do this. That hashtable is meant to be re-used.
indeed :)
Thanks for the review.
Try again,how about this ?
Comment 4 rland 2018-05-31 02:51:26 UTC
Created attachment 372486 [details] [review]
avdemux fix memory leak.

Deal with compiler warnings.
Comment 5 Edward Hervey 2018-05-31 07:40:59 UTC
Review of attachment 372486 [details] [review]:

No I meant something like the following. That table can just be statically present.

static struct {
 const gchar *ffmpeg_tag_name;
 const gchar *gst_tag_name;
} tagmapping[] = {
 { "album", GST_TAG_ALBUM },
 ...
};

...

for (i = 0; i < G_N_ELEMENTS(tagmapping); i++) {
  if (!g_strcmp0(tagmapping[i].ffmpeg_tag_name, something))
    /* Do something */
}
Comment 6 rland 2018-05-31 09:33:30 UTC
Created attachment 372489 [details] [review]
avdemux:fix memory leak.

Thanks for your patience, :),again.
Comment 7 Edward Hervey 2018-06-01 06:15:23 UTC
Thanks !