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 753306 - h264parse: fix MVC specific data memory leak
h264parse: fix MVC specific data memory leak
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-bad
git master
Other Linux
: Normal normal
: 1.5.90
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2015-08-06 04:08 UTC by Vineeth
Modified: 2015-08-16 13:38 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
fix memory leak (1.51 KB, patch)
2015-08-06 04:11 UTC, Vineeth
none Details | Review
fix memory leak (1.47 KB, patch)
2015-08-06 04:47 UTC, Vineeth
none Details | Review
h264parse: Clear SPS info after processing. (2.09 KB, patch)
2015-08-06 05:29 UTC, Jan Schmidt
committed Details | Review

Description Vineeth 2015-08-06 04:08:51 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.
Comment 1 Vineeth 2015-08-06 04:11:33 UTC
Created attachment 308830 [details] [review]
fix memory leak
Comment 2 Jan Schmidt 2015-08-06 04:33:45 UTC
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
Comment 3 Jan Schmidt 2015-08-06 04:33:48 UTC
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
Comment 4 Vineeth 2015-08-06 04:38:45 UTC
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..
Comment 5 Vineeth 2015-08-06 04:47:57 UTC
Created attachment 308831 [details] [review]
fix memory leak

Instead of clearing it in codecparser, clearing the same in h264parse.c
Comment 6 Jan Schmidt 2015-08-06 05:29:28 UTC
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 7 Jan Schmidt 2015-08-15 05:40:29 UTC
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
Comment 8 Sebastian Dröge (slomo) 2015-08-15 07:18:23 UTC
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?
Comment 9 Jan Schmidt 2015-08-15 11:34:59 UTC
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.