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 707282 - Memory leak in h264 codecparser
Memory leak in h264 codecparser
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-bad
git master
Other Linux
: Normal major
: 1.3.90
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks: 720305
 
 
Reported: 2013-09-02 11:58 UTC by sreerenj
Modified: 2014-06-28 08:45 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
codecparsers: h264: fix memory leak in GstH264PPS. (6.25 KB, patch)
2014-06-27 09:09 UTC, Gwenole Beauchesne
committed Details | Review

Description sreerenj 2013-09-02 11:58:40 UTC
The gst_h264_parse_pps() method is allocating memory dynamically for slice_group_id field of pps which is not freeing. 

In order to support dynamic allocation we have to change the codecparser implementation.
This is exactly what we did in https://bugzilla.gnome.org/show_bug.cgi?id=685215 to support mvc by providing dynamic allocation support in nal_parsing APIs by implementing copy() and free() methods for sps.

Suggestions?
Comment 1 Sebastian Dröge (slomo) 2013-09-02 13:40:02 UTC
Yes, exactly what you proposed :)
Comment 2 sreerenj 2013-09-05 07:58:35 UTC
(In reply to comment #1)
> Yes, exactly what you proposed :)

Okay. This patch would be copy-paste from https://bugzilla.gnome.org/show_bug.cgi?id=685215 .
So I prefer Gwenole to do that since he is the initial author.

What do you think? :)
Comment 3 Sebastian Dröge (slomo) 2013-10-01 21:40:57 UTC
Gwenole?
Comment 4 Alex Băluț 2014-01-09 22:48:25 UTC
h264 is the only multithreaded encoder (as far as I know in Pitivi land), would be nice to have it usable. Gwenole, interested in fixing this?
Comment 5 Alex Băluț 2014-01-10 11:03:38 UTC
Sorry, I had the impression that the problem is with the encoder, but it's with the decoder.
Comment 6 Thiago Sousa Santos 2014-06-20 01:53:33 UTC
Ping?
Comment 7 Gwenole Beauchesne 2014-06-20 04:51:35 UTC
OK, I will create an internal gst_h264_pps_copy() function, and expose an external gst_h264_pps_clear().
Comment 8 Gwenole Beauchesne 2014-06-27 09:09:59 UTC
Created attachment 279365 [details] [review]
codecparsers: h264: fix memory leak in GstH264PPS.

The patch formerly promised to address this issue. :)
Comment 9 Sebastian Dröge (slomo) 2014-06-27 09:19:20 UTC
Review of attachment 279365 [details] [review]:

Please push, but before fix these small things :)

::: gst-libs/gst/codecparsers/gsth264parser.c
@@ +227,3 @@
+ */
+static gboolean
+gst_h264_pps_copy (GstH264PPS * dst_pps, const GstH264PPS * src_pps)

The src_pps should be const probably

@@ +239,3 @@
+    dst_pps->slice_group_id = g_memdup (src_pps->slice_group_id,
+        src_pps->pic_size_in_map_units_minus1 + 1);
+    if (!dst_pps->slice_group_id)

g_memdup() will abort() if it fails to allocate memory
Comment 10 Tim-Philipp Müller 2014-06-28 08:44:48 UTC
This is fixed now, right?

commit 9bd186a960132b2141e2baffebc458501133d582
Author: Gwenole Beauchesne <gwenole.beauchesne@intel.com>
Date:   Fri Jun 27 10:53:20 2014 +0200

    codecparsers: h264: fix memory leak in GstH264PPS.
    
    The gst_h264_parse_pps() function dynamically allocates the slice
    group ids map array, so that needs to be cleared before parsing a
    new PPS NAL unit again, or when it is no longer needed.
    
    Likewise, a clean copy to the internal NAL parser state needs to be
    performed so that to avoid a double-free corruption.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=707282
    
    Signed-off-by: Gwenole Beauchesne <gwenole.beauchesne@intel.com>