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

[Nicolas Dufresne]

Le samedi 29 février 2020 à 17:11 +0200, Laurent Pinchart a écrit :
> 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 :-)

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.

> 
> > +		}
> > +	}
> >  
> >  	g_clear_object(&self->allocator);
> >  	g_clear_pointer(&self->flow_combiner,
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 195 bytes
Desc: This is a digitally signed message part
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20200229/15b9bff6/attachment.sig>