GNOME Bugzilla – Bug 728490
v4l2src: Missing unref in decide_allocation
Last modified: 2014-06-03 18:58:31 UTC
Created attachment 274659 [details] [review] v4l2src: unref given pool before replacing it v4l2src decide_allocation function doesn't unref pool provided by downstream element before replacing it by its own pool. This bug is not present on master. Only on 1.2 branch I attached a patch which fix it
Review of attachment 274659 [details] [review]: ::: sys/v4l2/gstv4l2src.c @@ +529,1 @@ pool = GST_BUFFER_POOL_CAST (obj->pool); but does this mean you should ref obj->pool? otherwise aren't you giving the ownership to the query ?
Ok, gst_query_set_nth_allocation_pool() and gst_query_add_allocation_pool() will ref the given pool, so the pool is transferred none. gst_query_parse_nth_allocation_pool() give you a ref on the pool. This mean that if you didn't switch the pool to our own pool, you would be leaking a downstream pool. Here's what I propose to fix this correctly: after set_nth.. and add_allo, unref the pool. When replacing the pool, unref the pool from the allocation, and ref our own pool. This should be clean.
A patch for master would be nice too if needed.
(In reply to comment #2) > Ok, gst_query_set_nth_allocation_pool() and gst_query_add_allocation_pool() > will ref the given pool, so the pool is transferred none. > > gst_query_parse_nth_allocation_pool() give you a ref on the pool. This mean > that if you didn't switch the pool to our own pool, you would be leaking a > downstream pool. Here's what I propose to fix this correctly: > > after set_nth.. and add_allo, unref the pool. When replacing the pool, unref > the pool from the allocation, and ref our own pool. This should be clean. You are right, in the case we didn't switch the pool to our own pool, we will be leaking a ref. This case still happen on master so I will also do a patch for master branch.
Created attachment 274683 [details] [review] [1.2] v4l2src: fix bufferpool leak in decide_allocation() The updated patch for 1.2 branch
Created attachment 274686 [details] [review] [master] v4l2object: fix bufferpool leak in decide_allocation() Patch for master branch.
Created attachment 274688 [details] [review] [master] v4l2object: fix bufferpool leak in decide_allocation()
Created attachment 274692 [details] [review] [1.2] v4l2src: fix bufferpool leak in decide_allocation() Just update the commit msg.
ping ?
Review of attachment 274692 [details] [review]: Looks good, and matches what I already have in my branch. ::: sys/v4l2/gstv4l2src.c @@ +509,3 @@ GST_DEBUG_OBJECT (src, "read/write mode: no downstream pool, using our own"); + pool = gst_object_ref (GST_BUFFER_POOL_CAST (obj->pool)); no need for the cast here, gst_object_ref() takes a gpointer for this reason @@ +527,3 @@ + if (pool) + gst_object_unref (pool); + pool = gst_object_ref (GST_BUFFER_POOL_CAST (obj->pool)); Same.
Review of attachment 274688 [details] [review]: Same small changes.
Review of attachment 274688 [details] [review]: Same small changes, though I'll probably skip this one, as it's already in my branch.
Created attachment 275984 [details] [review] [1.2] v4l2src: fix bufferpool leak in decide_allocation() Remove casts
Created attachment 275987 [details] [review] [master] v4l2object: fix bufferpool leak in decide_allocation() Remove casts. But if your v4l2 branch will be merged in 1.4, We may skip this patch
This is fixed in master, need to be merged in 1.2 now, will do this week.
commit d3383f9d4ceb03b2ba9096b7641e8b4fba1771f9 Author: Nicolas Dufresne <nicolas.dufresne@collabora.com> Date: Wed Apr 16 17:04:42 2014 -0400 v4l2object: Don't leak downstream pool in propose_allocation parse_nth_allocation_pool() give a ref on the pool, we need to unref it when done
ping for 1.2 ?
Comment on attachment 275987 [details] [review] [master] v4l2object: fix bufferpool leak in decide_allocation() This is fixed now upstream
Comment on attachment 275984 [details] [review] [1.2] v4l2src: fix bufferpool leak in decide_allocation() commit 2dbd9d948dc35dfd6b63f3fc7b79a8b0d6c66c07 Author: Aurélien Zanelli <aurelien.zanelli@parrot.com> Date: Tue May 6 16:14:07 2014 +0200 v4l2src: fix bufferpool leak in decide_allocation() gst_query_parse_nth_allocation_pool() ref the pool it returns and gst_query_set_nth_allocation_pool()/gst_query_add_allocation_pool are transfert none. So when we replace it by our own pool in DMA/MMAP/USERPTR cases, we must unref the one given by gst_query_parse_nth_allocation_pool(). In all cases, pool should be unref at the end so to make it clear, we also ref our own pool when using it. https://bugzilla.gnome.org/show_bug.cgi?id=728490
Thanks for your patch and your time. This will be part of next stable 1.2 release.