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 699786 - mpegtsmux: memory leak when using prepare_func
mpegtsmux: memory leak when using prepare_func
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-bad
1.x
Other Linux
: Normal normal
: 1.0.8
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2013-05-06 22:11 UTC by Greg Rutz
Modified: 2013-05-18 11:43 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Proposed fix (980 bytes, patch)
2013-05-06 22:11 UTC, Greg Rutz
needs-work Details | Review
Updated fix based on comments (1.07 KB, patch)
2013-05-07 14:28 UTC, Greg Rutz
committed Details | Review

Description Greg Rutz 2013-05-06 22:11:06 UTC
Created attachment 243438 [details] [review]
Proposed fix

In the mpegtsmux collect pads clip function (mpegtsmux_clip_inc_running_time), it is possible that a "prepare" function is necessary to modify the incoming buffer when it arrives on a pad (AC3 audio uses this prepare_func).  The prepare returns a newly alloc'd GstBuffer based on the incoming buffer data.  The call is made like this:

  if (pad_data->prepare_func) {
    MpegTsMux *mux = (MpegTsMux *) user_data;

    buf = pad_data->prepare_func (buf, pad_data, mux);
    if (buf)
      gst_buffer_replace (outbuf, buf);
  }

Since the clip function is supposed to return the buffer in an output parameter and not as a return value, the buffer returned from the prepare_func is passed to gst_buffer_replace.  gst_buffer_replace serves the purpose of allowing the new buffer to returned in a function parameter, but I think is has the unintended side-effect of reffing the buffer one more time.

My proposed patch will just add a call to gst_buffer_unref after gst_buffer_replace.  The other possible solution is to not use gst_buffer_replace at all and just do "*outbuf = buf".  I'm not sure if the atomicity of gst_buffer_replace is really necessary.
Comment 1 Sebastian Dröge (slomo) 2013-05-07 07:26:15 UTC
Review of attachment 243438 [details] [review]:

::: gst/mpegtsmux/mpegtsmux.c
@@ +1023,2 @@
       gst_buffer_replace (outbuf, buf);
+      gst_buffer_unref (buf);

Shouldn't this be something like this?
*outbuf = pad_data->prepare_func (...);
gst_buffer_unref (buf);

The prepare_func creates a new buffer that is returned but the original buf is never unreffed.
Comment 2 Greg Rutz 2013-05-07 14:28:24 UTC
Created attachment 243492 [details] [review]
Updated fix based on comments
Comment 3 Sebastian Dröge (slomo) 2013-05-07 14:38:25 UTC
commit 4d4fd09a3a00ce97dcaacb30377e350391fd64b0
Author: Greg Rutz <greg@gsr-tek.com>
Date:   Tue May 7 08:26:03 2013 -0600

    mpegtsmux: Fix memory leak when using prepare_func
    
    prepare_func will allocate a new buffer to replace the original
    one. Instead of using gst_buffer_replace (which causes an extra
    refcount increment on the new buffer), we just unref the original
    buffer.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=699786