[libcamera-devel] [PATCH] gstreamer: Cleanup inner scope in gst_libcamera_src_task_run()
Nicolas Dufresne
nicolas.dufresne at collabora.com
Mon Mar 28 23:00:42 CEST 2022
Le lundi 28 mars 2022 à 00:41 +0530, Umang Jain via libcamera-devel a écrit :
> Hi,
>
> Just my 2 cents on a quick look.
>
> On 3/27/22 20:18, Laurent Pinchart via libcamera-devel wrote:
> > On Sun, Mar 27, 2022 at 05:36:29PM +0300, Laurent Pinchart wrote:
> > > 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 ?
> > Also, as far as I can tell, srcpads_ is only modified in
> > gst_libcamera_src_init(), gst_libcamera_src_request_new_pad() and
> > gst_libcamera_src_release_pad(). The former is called at init time only,
> > can the latter two (handling the .request_new_pad() and .release_pad())
> > we called while the task is running ? If so there's another possible
> > race, as srcpads_ is iterated earlier in this function, with no lock
> > taken.
>
>
> The task is run with a stream_lock taken I think, see:
>
> gst_task_set_lock(self->task, &self->stream_lock);
>
> in gst_libcamera_src_init()
>
> (I haven't looked anything deeper, which locks handles what etc.)
But gst_libcamera_src_request_new_pad() use the object lock to protect srcpads_
additions:
...
GLibLocker lock(GST_OBJECT(self));
self->state->srcpads_.push_back(reinterpret_cast<GstPad *>(g_object_ref(pad)));
...
So yes, the change does not break anything, but it changes obviously broken code
without fixing it. Though, I think the way forward is to special case that
function and use the gstreamer thread safe pad iterator. See
gst_element_iterate_src_pads(). As we can't hold the object lock while sending
events, or acquiring buffers from the pool (if blocking, if non-blocking that is
fine as long as the release of buffers don't need to take the object lock).
>
> >
> > > > > - 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]
More information about the libcamera-devel
mailing list