GNOME Bugzilla – Bug 432755
[videorate] leaks buffer if flow != OK
Last modified: 2008-08-11 19:30:14 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.
Created attachment 86895 [details] [review] don't leak the buffer on flush error
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), (videorate_suite): Unit test for the above by Yours Truly.
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. http://webcvs.freedesktop.org/gstreamer/gst-plugins-base/gst/videorate/gstvideorate.c?view=markup 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.
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).
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.