[libcamera-devel] [PATCH] gstreamer: Cleanup inner scope in gst_libcamera_src_task_run()

[Laurent Pinchart]

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