GNOME Bugzilla – Bug 720389
videodecoder: should release buffer pool sooner
Last modified: 2014-03-04 09:13:01 UTC
Created attachment 264141 [details] [review] inactivate and unref current negotiated buffer pool when doing a hard reset Currently in gstvideodecoder.c the last buffer pool negotiated is released in gst_video_decoder_finalize. It can cause some leak due to circular ref. For example for v4l2videodec, the CAPTURE v4l2bufferpool of the underlying v4l2object has a ref on the decoder: http://cgit.freedesktop.org/gstreamer/gst-plugins-good/commit/sys/v4l2/gstv4l2bufferpool.c?id=a1c34b540735a7a23b15cdb77ecba1b28adb93ee And the videodecoder has also a ref on this capture buffer pool. => circular ref. First I thought about removing this circular ref, but then what do you think about the above commit ? So another solution would be to release the buffer pool sooner in gstvideodecoder. Actually I think both should be done (remove circular ref in v4l2 and release buffer pool sooner in gstvideodecoder) So I suggest to release the buffer pool when doing a hard reset in gst_video_decoder_reset, which is called when going from and to READY state. Let me know what do you think about it.
I have tested it with more video and format, I have not seen any regression. (Also make check is ok) Also I noticed this patch is not applicable for gstvideoencoder because it does not keep a ref on the pool. For gstaudioencoder and gstaudiodecoder I can't see any pool variable. So is it ok to push ?
Comment on attachment 264141 [details] [review] inactivate and unref current negotiated buffer pool when doing a hard reset All good !
Comment on attachment 264141 [details] [review] inactivate and unref current negotiated buffer pool when doing a hard reset No, not all good :) a) Please also get rid of the allocator (if any) there b) This is called with full=false, flush_hard=true for FLUSH_STOP events. This means that after e.g. a flushing seek you will loose the buffer buffer pool.
(In reply to comment #3) > (From update of attachment 264141 [details] [review]) > No, not all good :) > > a) Please also get rid of the allocator (if any) there > > b) This is called with full=false, flush_hard=true for FLUSH_STOP events. This > means that after e.g. a flushing seek you will loose the buffer buffer pool. Sorry, I have been miss-lead by these full/flush_hard variable. Re-thinking it, nothing says that the format will change after a flush. If the format does not change, there is no point in re-negotiation ?
Yes, the format will usually not change after a flush. And usually re-negotiation is unnecessary after a flush, of course you could do it but it's not needed. Renegotiation can be useful if the format did not change though, like when downstream was relinked. We do that then already (the RECONFIGURE flag is set on the pad then).
(In reply to comment #5) > Yes, the format will usually not change after a flush. And usually > re-negotiation is unnecessary after a flush, of course you could do it but it's > not needed. > > Renegotiation can be useful if the format did not change though, like when > downstream was relinked. We do that then already (the RECONFIGURE flag is set > on the pad then). Oh, we need a step back a bit an review all of this, as not getting rid of the pool when that happens would be very dangerous. What happens if we still have output buffers from the pool driven by an "unlinked" element ?
Well, they should still be usable until the pool is deactivated.
Nonetheless the idea of this patch here is correct, just what I mentioned before is wrong and can easily be fixed.
Created attachment 264292 [details] [review] inactivate and unref current negotiated buffer pool and allocator when doing a full reset I move the unref to "full" block. Also unref allocator if there is one. I also marked reconfigure the src pad as there is no pool at this point. I think in the way we use gst_video_decoder_reset there is no need to do as the pad will be marked reconfigure somewhere else. But just in case.
Comment on attachment 264292 [details] [review] inactivate and unref current negotiated buffer pool and allocator when doing a full reset Perfect, push it (and remove the mark_reconfigure() bit, that's unneeded in any case)
Created attachment 264297 [details] [review] inactivate and unref current negotiated buffer pool and allocator when doing a full reset remove "mark reconfigure"
commit 2e38741b94f7a226ee32945b6f14ea439c987719 Author: Julien Isorce <julien.isorce@collabora.co.uk> Date: Mon Dec 16 15:53:41 2013 +0000 videodecoder: release buffer pool and allocator on full reset It allows to release the buffer pool sooner (i.e. when going to GST_STATE_READY). Previously it was released in finalize. Fixes bug https://bugzilla.gnome.org/show_bug.cgi?id=720389
*** Bug 725642 has been marked as a duplicate of this bug. ***