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

Message ID 20210825211852.1207168-2-nicolas@ndufresne.ca
State Accepted
Headers show
Series
  • Fix Gnome Cheese and multiple camera
Related show

Commit Message

Nicolas Dufresne Aug. 25, 2021, 9:18 p.m. UTC 
From: Nicolas Dufresne <nicolas.dufresne@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@collabora.com>
---
 src/gstreamer/gstlibcameraallocator.cpp | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

Comments

Laurent Pinchart Aug. 25, 2021, 10:42 p.m. UTC | #1 
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@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@collabora.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart@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. */

Patch
diff mbox series 

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. */