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 432755 - [videorate] leaks buffer if flow != OK
[videorate] leaks buffer if flow != OK
Product: GStreamer
Classification: Platform
Component: gst-plugins-base
Other Linux
: Normal normal
: 0.10.13
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Reported: 2007-04-23 21:11 UTC by Dan Williams
Modified: 2008-08-11 19:30 UTC
See Also:
GNOME target: ---
GNOME version: ---

don't leak the buffer on flush error (604 bytes, patch)
2007-04-24 02:31 UTC, Dan Williams
committed Details | Review

Description Dan Williams 2007-04-23 21:11:17 UTC
Reproducible using this pipeline:

v4l2src ! videorate ! video/x-raw-yuv,framerate=5/1 ! ffmpegcolorspace ! videoflip ! fakesink

and this one:

v4l2src ! videorate ! video/x-raw-yuv,framerate=5/1 ! ffmpegcolorspace ! theoraenc ! fakesink

1) gst_video_rate_chain() gets a new buffer
2) gst_video_rate_chain() executes:
        if ((res = gst_video_rate_flush_prev (videorate)) != GST_FLOW_OK)
3) during the call to gst_video_rate_flush_prev(), the user hits Ctl+C and the pipeline gets set to PAUSED
4) The PAUSED pipeline state causes this statement in gst_video_rate_flush_prev() to return something other than GST_FLOW_OK:
        res = gst_pad_push (videorate->srcpad, outbuf);
5) gst_video_rate_flush_prev() returns the ! GST_FLOW_OK result
6) gst_video_rate_chain() then executes the following, jumping to the 'done' label:
        if ((res = gst_video_rate_flush_prev (videorate)) != GST_FLOW_OK)
           goto done;
7) gst_video_rate_chain() returns, leaking 'buffer'

Not sure what the best fix is, whether to just unref the buffer on error, or to do something with swap_prev() or what.
Comment 1 Dan Williams 2007-04-24 02:31:49 UTC
Created attachment 86895 [details] [review]
don't leak the buffer on flush error
Comment 2 Tim-Philipp Müller 2007-04-24 15:00:29 UTC
Applied, thanks!

  2007-04-24  Tim-Philipp Müller  <tim at centricular dot net>

        Patch by: Dan Williams <dcbw redhat com>

        * gst/videorate/gstvideorate.c: (gst_video_rate_chain):
          Don't leak incoming buffer if gst_pad_push() returns a
          non-OK flow. Fixes #432755.
        * tests/check/elements/videorate.c: (GST_START_TEST),
          Unit test for the above by Yours Truly.

Comment 3 Stefan Sauer (gstreamer, gtkdoc dev) 2008-07-10 14:03:34 UTC
Today one IRC:
15:19 < yves> Hi : I have a question about a past fix for a leak in videorate : bug 432755 fix rev 1.50 of gstvideorate.c : the fix is about 
              unrefing a gst-buffer when pad_push return != GST_FLOW_OK
15:20 < yves> However, according to the doc, the pad_push function should take the ownership of the buffer in all case.
15:20 < yves> So, I think this fix may not be done at the right place. Maybe there is something in the gstreamer mechanic that is causing 
              this buffer leak

From briefly lloking at it, it indeed looks suspicious.

gst_video_rate_flush_prev() is called several time, but only in one place (chain()) the return value is checked. Maybe gst_video_rate_flush_prev() should unref the buffer if its not pushing it (eos_before_buffers), so that it like pad_push() takes ownership.
Comment 4 Mark Nauwelaerts 2008-07-12 20:32:59 UTC
I have had a look at videorate and I can not find a leak (nor do the tests at the moment).  Specifically, gst_video_rate_flush_prev cannot create a leak, as it only adds 1 ref to a cached buffer and then gives away this ref to a _pad_push (whatever that one's outcome may be).  It is indeed called in a few places without checking the return value, as it would have little value in those other cases (event handling), nor is there anything being leaked by not checking the return value in those event cases.  _flush_prev need not take ownership (of the cached buffer), since it is always replaced using _swap_prev, which does not leak the previous one (and the state_change in any case cleans the cache).

Regarding the IRC fragment:
* a pad_push always takes ownership (regardless of outcome), but note that it is not the _chain's buffer (as an input parameter) that has been pushed, but a previously cached buffer (or rather, an extra created ref to that cached buffer), as all done in _flush_prev (the extra ref it creates is taken over by the peer as a result of the _pad_push it performs, regardless of that result)
* similarly, _videorate_chain must take ownership of its input buffer, whether it succeeds or fails.  Roughly, if it succeeds, the incoming buffer's ref is retained as the buffer is put into the cache (by using _swap_prev), or the incoming buffer is simply dropped (gst_buffer_unref) in case of some failure.
As far as I can tell, all _chain's code paths do either one of these, so the input buffer can then not be leaked.
(and the original fix added a missing _unref to one of the failure paths)

Please indicate the suspected leak in more detail (or re-close this bug).
Comment 5 Yves Lefebvre 2008-08-11 19:03:00 UTC
It is me who sent the IRC comment : You are indeed correct with the description and the leak I was chasing turn out to be elsewhere in my own code... So you may close this issue. Sorry about the confusion.