GNOME Bugzilla – Bug 753306
h264parse: fix MVC specific data memory leak
Last modified: 2015-08-16 13:38:29 UTC
When i run a specific file using valgrind, the following leak happens. ==20932== 69,420 bytes in 267 blocks are definitely lost in loss record 3,745 of 3,746 ==20932== at 0x402E109: calloc (in /usr/lib/valgrind/vgpreload_memcheck-x86-linux.so) ==20932== by 0x4251C4A: g_malloc0 (in /lib/i386-linux-gnu/libglib-2.0.so.0.4002.0) ==20932== by 0x4251F2A: g_malloc0_n (in /lib/i386-linux-gnu/libglib-2.0.so.0.4002.0) ==20932== by 0x7CE6F5C: gst_h264_parse_subset_sps (gsth264parser.c:1699) ==20932== by 0x7CE7ACB: gst_h264_parser_parse_subset_sps (gsth264parser.c:1844) ==20932== by 0x7CAE554: gst_h264_parse_process_nal (gsth264parse.c:720) ==20932== by 0x7CB3694: gst_h264_parse_handle_frame (gsth264parse.c:1199) ==20932== by 0x4855F96: gst_base_parse_handle_buffer (gstbaseparse.c:1985) ==20932== by 0x48567B2: gst_base_parse_scan_frame.isra.13 (gstbaseparse.c:3247) ==20932== by 0x485BE1E: gst_base_parse_loop (gstbaseparse.c:3326) ==20932== by 0x4113A58: gst_task_func (gsttask.c:331) ==20932== by 0x4114BFE: default_func (gsttaskpool.c:68) ==20932== by 0x4273404: ??? (in /lib/i386-linux-gnu/libglib-2.0.so.0.4002.0) ==20932== by 0x42729A9: ??? (in /lib/i386-linux-gnu/libglib-2.0.so.0.4002.0) ==20932== by 0x4317F6F: start_thread (pthread_create.c:312) ==20932== by 0x4419BED: clone (clone.S:129) This is because, while parsing mvc data, the following are being allocated mvc->view, mvc->level_value. The same are being allocated again for destination when copying using, gst_h264_sps_mvc_copy, and the allocated values for src is not getting freed. Hence freeing the same after copy.
Created attachment 308830 [details] [review] fix memory leak
Review of attachment 308830 [details] [review]: This is actually a bug in h264parse, not calling gst_h264_sps_clear() on the returned SPS. The codec parser library can't do it - one of the points of gst_h264_parser_parse_subset_sps() is to return the extra MVC info (as well as storing it). Clearing the info before returning defeats the point
gst_h264_parser_parse_subset_sps() gets the extra MVC info into nalparser variable That is where the copying happens gst_h264_sps_copy (&nalparser->sps[sps->id], sps); so whats happening here is, first MVC data is parsed into sps. And then it is being copied into nalparser->sps[] now the sps being passed to gst_h264_parser_parse_subset_sps() is a local variable. GstH264SPS sps = { 0, } which will be not valid as soon as the function exits.. so the memory allocated for sps will be leaked.. If not clear in codecparser, it should be freed in h264parse so it will be something like pres = gst_h264_parser_parse_subset_sps (nalparser, nalu, &sps, TRUE); gst_h264_sps_clear (&sps); This is the right fix? I added it in codecparsers, to make sure whoever uses it will not need to do it again..
Created attachment 308831 [details] [review] fix memory leak Instead of clearing it in codecparser, clearing the same in h264parse.c
Created attachment 308835 [details] [review] h264parse: Clear SPS info after processing. The SPS struct might be filled out by a call to gst_h264_parser_parse_subset_sps, which fills out dynamically allocated data and requires a call to gst_h264_sps_clear() to free it. Also make sure to clear out any allocated SPS data when returning an error.
Comment on attachment 308835 [details] [review] h264parse: Clear SPS info after processing. commit fdac09d843f6fed06f222485e8f43959430d994a Author: Jan Schmidt <jan@centricular.com> Date: Thu Aug 6 14:33:54 2015 +1000 h264parse: Clear SPS info after processing. The SPS struct might be filled out by a call to gst_h264_parser_parse_subset_sps, which fills out dynamically allocated data and requires a call to gst_h264_sps_clear() to free it. Also make sure to clear out any allocated SPS data when returning an error. https://bugzilla.gnome.org/show_bug.cgi?id=753306
The same change should be applied to the h265 parser too. It's almost the same code. Should it also happen in the PPS (and VPS for h265) code?
Looks like all the h.265 parser allocations are part of the larger structure - no dynamic alloc to free beyond the codec parser itself afaics.