[libcamera-devel] [PATCH] gstreamer: Cleanup inner scope in gst_libcamera_src_task_run()
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Sun Mar 27 16:36:27 CEST 2022
On Sun, Mar 27, 2022 at 10:25:27AM -0400, Nicolas Dufresne wrote:
> Le 27 mars 2022 09 h 52, Laurent Pinchart a écrit :
>
> > The code block that checks for flow errors in
> > gst_libcamera_src_task_run() is located in an inner scope whose purpose
> > it to handling locking. The flow error handling however occurs before
> > the lock is taken, so there's no need to place it in the inner scope.
> > Move it one level up.
>
> > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > ---
> > src/gstreamer/gstlibcamerasrc.cpp | 26 +++++++++++++-------------
> > 1 file changed, 13 insertions(+), 13 deletions(-)
>
> > diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/
> > gstlibcamerasrc.cpp
> > index 46fd02d207be..8016a98d6a4d 100644
> > --- a/src/gstreamer/gstlibcamerasrc.cpp
> > +++ b/src/gstreamer/gstlibcamerasrc.cpp
> > @@ -311,21 +311,21 @@ gst_libcamera_src_task_run(gpointer user_data)
> > srcpad, ret);
> > }
> >
> > - {
^---- [1]
> > - if (ret != GST_FLOW_OK) {
> > - if (ret == GST_FLOW_EOS) {
> > - g_autoptr(GstEvent) eos = gst_event_new_eos();
> > - guint32 seqnum = gst_util_seqnum_next();
> > - gst_event_set_seqnum(eos, seqnum);
> > - for (GstPad *srcpad : state->srcpads_)
>
> Iterating the srcpads_ is not thread safe. Won't this change remove the locking
> for that ?
The patch doesn't change locking at all as far as I can tell, there's no
lock taken at the beginning of the scope ([1]). There's a lock taken
below ([2]), was it meant to cover this iteration too ?
> > - gst_pad_push_event(srcpad, gst_event_ref(eos));
> > - } else if (ret != GST_FLOW_FLUSHING) {
> > - GST_ELEMENT_FLOW_ERROR(self, ret);
> > - }
> > - gst_task_stop(self->task);
> > - return;
> > + if (ret != GST_FLOW_OK) {
> > + if (ret == GST_FLOW_EOS) {
> > + g_autoptr(GstEvent) eos = gst_event_new_eos();
> > + guint32 seqnum = gst_util_seqnum_next();
> > + gst_event_set_seqnum(eos, seqnum);
> > + for (GstPad *srcpad : state->srcpads_)
> > + gst_pad_push_event(srcpad, gst_event_ref(eos));
> > + } else if (ret != GST_FLOW_FLUSHING) {
> > + GST_ELEMENT_FLOW_ERROR(self, ret);
> > }
> > + gst_task_stop(self->task);
> > + return;
> > + }
> >
> > + {
> > /*
> > * Here we need to decide if we want to pause. This needs to
> > * happen in lock step with the callback thread which may want
*/
GLibLocker lock(GST_OBJECT(self));
^---- [2]
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list