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

3032 diff mbox series

From: Jakub Adam <jakub.adam@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@collabora.com>
---
 src/gstreamer/gstlibcamerapool.cpp | 17 +++++++++++++++++
 src/gstreamer/gstlibcamerasrc.cpp  |  5 ++++-
 2 files changed, 21 insertions(+), 1 deletion(-)

Hi Nicolas and Jakub,

Thank you for the patch.

On Fri, Mar 06, 2020 at 03:26:36PM -0500, Nicolas Dufresne wrote:
> From: Jakub Adam <jakub.adam@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@collabora.com>

I like the signal !

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

> ---
>  src/gstreamer/gstlibcamerapool.cpp | 17 +++++++++++++++++
>  src/gstreamer/gstlibcamerasrc.cpp  |  5 ++++-
>  2 files changed, 21 insertions(+), 1 deletion(-)
> 
> diff --git a/src/gstreamer/gstlibcamerapool.cpp b/src/gstreamer/gstlibcamerapool.cpp
> index 3868905..8f53616 100644
> --- a/src/gstreamer/gstlibcamerapool.cpp
> +++ b/src/gstreamer/gstlibcamerapool.cpp
> @@ -14,6 +14,13 @@
>  
>  using namespace libcamera;
>  
> +enum {
> +	SIGNAL_BUFFER_NOTIFY,
> +	N_SIGNALS
> +};
> +
> +static guint signals[N_SIGNALS];
> +
>  struct _GstLibcameraPool {
>  	GstBufferPool parent;
>  
> @@ -55,7 +62,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)
> +		g_signal_emit(self, signals[SIGNAL_BUFFER_NOTIFY], 0);
>  }
>  
>  static void
> @@ -90,6 +102,11 @@ gst_libcamera_pool_class_init(GstLibcameraPoolClass *klass)
>  	pool_class->acquire_buffer = gst_libcamera_pool_acquire_buffer;
>  	pool_class->reset_buffer = gst_libcamera_pool_reset_buffer;
>  	pool_class->release_buffer = gst_libcamera_pool_release_buffer;
> +
> +	signals[SIGNAL_BUFFER_NOTIFY] = g_signal_new("buffer-notify",
> +						     G_OBJECT_CLASS_TYPE(klass), G_SIGNAL_RUN_LAST,
> +						     0, nullptr, nullptr, nullptr,
> +						     G_TYPE_NONE, 0);
>  }
>  
>  GstLibcameraPool *
> diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp
> index 22d9175..9755922 100644
> --- a/src/gstreamer/gstlibcamerasrc.cpp
> +++ b/src/gstreamer/gstlibcamerasrc.cpp
> @@ -439,6 +439,9 @@ 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());
> +		g_signal_connect_swapped(pool, "buffer-notify",
> +					 G_CALLBACK(gst_libcamera_resume_task), task);
> +
>  		gst_libcamera_pad_set_pool(srcpad, pool);
>  		gst_flow_combiner_add_pad(self->flow_combiner, srcpad);
>  	}
> @@ -474,7 +477,7 @@ 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);
> +		gst_libcamera_pad_set_pool(srcpad, nullptr);
>  
>  	g_clear_object(&self->allocator);
>  	g_clear_pointer(&self->flow_combiner,