Message ID | 20200227200407.490616-27-nicolas.dufresne@collabora.com |
---|---|
State | Accepted |
Headers | show |
Series |
|
Related | show |
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@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 | 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,
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@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 | 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,
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@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 | 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,
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); + } + } g_clear_object(&self->allocator); g_clear_pointer(&self->flow_combiner,