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

[Laurent Pinchart]

Hi Nicolas,

On Sat, Feb 29, 2020 at 10:35:21AM -0500, Nicolas Dufresne wrote:
> Le samedi 29 février 2020 à 17:11 +0200, Laurent Pinchart a écrit :
> > 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 :-)
> 
> As this is a GObject, it could use a signal instead yes. I made that
> comment internally to Jakub, but decided to send anyway. As I still
> have many little typos and rebase errors, we'll have time to fix that
> by then.

I was referring to a libcamera Signal, but a glib signal works too :-)

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

-- 
Regards,

Laurent Pinchart