[libcamera-devel] [PATCH v2 26/27] gst: libcamerasrc: Prevent src task deadlock on exhausted buffer pool

[Laurent Pinchart]

Hi Nicolas and Jakub,

Thank you for the patch.

On Thu, Feb 27, 2020 at 03:04:06PM -0500, Nicolas Dufresne wrote:
> From: Jakub Adam <jakub.adam at collabora.com>
> 
> Allow GstLibcameraPool to notify the source when a new buffer has become
> available in a previously exhausted buffer pool. This can be used to
> resume a src task that got paused because it couldn't acquire a buffer.
> 
> Without this change the src task will never resume from pause once the
> pool gets exhausted.
> 
> To trigger the deadlock (it doesn't happen every time), run:
> 
>   gst-launch-1.0 libcamerasrc ! queue ! glimagesink
> 
> Signed-off-by: Jakub Adam <jakub.adam at collabora.com>
> ---
>  src/gstreamer/gstlibcamerapool.cpp | 16 ++++++++++++++++
>  src/gstreamer/gstlibcamerapool.h   |  6 ++++++
>  src/gstreamer/gstlibcamerasrc.cpp  | 14 ++++++++++++--
>  3 files changed, 34 insertions(+), 2 deletions(-)
> 
> diff --git a/src/gstreamer/gstlibcamerapool.cpp b/src/gstreamer/gstlibcamerapool.cpp
> index f85c3aa..5aa0eeb 100644
> --- a/src/gstreamer/gstlibcamerapool.cpp
> +++ b/src/gstreamer/gstlibcamerapool.cpp
> @@ -18,6 +18,8 @@ struct _GstLibcameraPool {
>  
>  	GstAtomicQueue *queue;
>  	GstLibcameraAllocator *allocator;
> +	GstLibcameraBufferNotify buffer_notify;
> +	gpointer buffer_notify_data;
>  	Stream *stream;
>  };
>  
> @@ -54,7 +56,12 @@ static void
>  gst_libcamera_pool_release_buffer(GstBufferPool *pool, GstBuffer *buffer)
>  {
>  	GstLibcameraPool *self = GST_LIBCAMERA_POOL(pool);
> +	bool do_notify = gst_atomic_queue_length(self->queue) == 0;
> +
>  	gst_atomic_queue_push(self->queue, buffer);
> +
> +	if (do_notify && self->buffer_notify)
> +		self->buffer_notify(self->buffer_notify_data);
>  }
>  
>  static void
> @@ -108,6 +115,15 @@ gst_libcamera_pool_new(GstLibcameraAllocator *allocator, Stream *stream)
>  	return pool;
>  }
>  
> +void
> +gst_libcamera_pool_set_buffer_notify(GstLibcameraPool *self,
> +				     GstLibcameraBufferNotify notify,
> +				     gpointer data)
> +{
> +	self->buffer_notify = notify;
> +	self->buffer_notify_data = data;
> +}
> +
>  Stream *
>  gst_libcamera_pool_get_stream(GstLibcameraPool *self)
>  {
> diff --git a/src/gstreamer/gstlibcamerapool.h b/src/gstreamer/gstlibcamerapool.h
> index 6fd2913..545be01 100644
> --- a/src/gstreamer/gstlibcamerapool.h
> +++ b/src/gstreamer/gstlibcamerapool.h
> @@ -20,9 +20,15 @@
>  #define GST_TYPE_LIBCAMERA_POOL gst_libcamera_pool_get_type()
>  G_DECLARE_FINAL_TYPE(GstLibcameraPool, gst_libcamera_pool, GST_LIBCAMERA, POOL, GstBufferPool)
>  
> +typedef void (*GstLibcameraBufferNotify)(gpointer data);
> +
>  GstLibcameraPool *gst_libcamera_pool_new(GstLibcameraAllocator *allocator,
>  					 libcamera::Stream *stream);
>  
> +void gst_libcamera_pool_set_buffer_notify(GstLibcameraPool *self,
> +					  GstLibcameraBufferNotify notify,
> +					  gpointer data);
> +
>  libcamera::Stream *gst_libcamera_pool_get_stream(GstLibcameraPool *self);
>  
>  libcamera::Stream *gst_libcamera_buffer_get_stream(GstBuffer *buffer);
> diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp
> index fe60584..ecbfe37 100644
> --- a/src/gstreamer/gstlibcamerasrc.cpp
> +++ b/src/gstreamer/gstlibcamerasrc.cpp
> @@ -434,6 +434,10 @@ gst_libcamera_src_task_enter(GstTask *task, GThread *thread, gpointer user_data)
>  		const StreamConfiguration &stream_cfg = state->config->at(i);
>  		GstLibcameraPool *pool = gst_libcamera_pool_new(self->allocator,
>  								stream_cfg.stream());
> +		gst_libcamera_pool_set_buffer_notify(pool,
> +						     (GstLibcameraBufferNotify)gst_libcamera_resume_task,
> +						     task);
> +
>  		gst_libcamera_pad_set_pool(srcpad, pool);
>  		gst_flow_combiner_add_pad(self->flow_combiner, srcpad);
>  	}
> @@ -468,8 +472,14 @@ gst_libcamera_src_task_leave(GstTask *task, GThread *thread, gpointer user_data)
>  
>  	state->cam->stop();
>  
> -	for (GstPad *srcpad : state->srcpads)
> -		gst_libcamera_pad_set_pool(srcpad, NULL);
> +	for (GstPad *srcpad : state->srcpads) {
> +		auto *pool = gst_libcamera_pad_get_pool(srcpad);
> +
> +		if (pool) {
> +			gst_libcamera_pool_set_buffer_notify(pool, NULL, NULL);
> +			gst_libcamera_pad_set_pool(srcpad, NULL);

s/NULL/nullptr/

Up to you, but you could also use a Signal. This would however require
C++ objects, so some refactoring would be needed, and it may not be
worth it, but adding ad-hoc callback registration is a step on the
slippery slope to towards spaghetti code :-)

> +		}
> +	}
>  
>  	g_clear_object(&self->allocator);
>  	g_clear_pointer(&self->flow_combiner,

-- 
Regards,

Laurent Pinchart