[libcamera-devel] [PATCH v1 1/3] gstreamer: Fix deadlock when last allocator ref is held by buffer

[Laurent Pinchart]

Hi Nicolas,

Thank you for the patch.

On Wed, Aug 25, 2021 at 05:18:50PM -0400, Nicolas Dufresne wrote:
> From: Nicolas Dufresne <nicolas.dufresne at collabora.com>
> 
> This deadlock occurs when a buffer is holding the last reference on
> the allocator. In gst_libcamera_allocator_release() we must drop the
> object lock before dropping the last ref of that object since the
> destructor will lock it again causing deadlock.
> 
> This was notice while switching camera or resolution in Cheese software.
> 
> Signed-off-by: Nicolas Dufresne <nicolas.dufresne at collabora.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

> ---
>  src/gstreamer/gstlibcameraallocator.cpp | 17 ++++++++++-------
>  1 file changed, 10 insertions(+), 7 deletions(-)
> 
> diff --git a/src/gstreamer/gstlibcameraallocator.cpp b/src/gstreamer/gstlibcameraallocator.cpp
> index 7bd8ba2d..a8fa4f86 100644
> --- a/src/gstreamer/gstlibcameraallocator.cpp
> +++ b/src/gstreamer/gstlibcameraallocator.cpp
> @@ -108,15 +108,18 @@ gst_libcamera_allocator_release(GstMiniObject *mini_object)
>  {
>  	GstMemory *mem = GST_MEMORY_CAST(mini_object);
>  	GstLibcameraAllocator *self = GST_LIBCAMERA_ALLOCATOR(mem->allocator);
> -	GLibLocker lock(GST_OBJECT(self));
> -	auto *frame = reinterpret_cast<FrameWrap *>(gst_mini_object_get_qdata(mini_object, FrameWrap::getQuark()));
>  
> -	gst_memory_ref(mem);
> +	{
> +		GLibLocker lock(GST_OBJECT(self));
> +		auto *frame = reinterpret_cast<FrameWrap *>(gst_mini_object_get_qdata(mini_object, FrameWrap::getQuark()));
> +
> +		gst_memory_ref(mem);
>  
> -	if (frame->releasePlane()) {
> -		auto *pool = reinterpret_cast<GQueue *>(g_hash_table_lookup(self->pools, frame->stream_));
> -		g_return_val_if_fail(pool, TRUE);
> -		g_queue_push_tail(pool, frame);
> +		if (frame->releasePlane()) {
> +			auto *pool = reinterpret_cast<GQueue *>(g_hash_table_lookup(self->pools, frame->stream_));
> +			g_return_val_if_fail(pool, TRUE);
> +			g_queue_push_tail(pool, frame);
> +		}
>  	}
>  
>  	/* Keep last in case we are holding on the last allocator ref. */

-- 
Regards,

Laurent Pinchart