| Message ID | 20220327135258.1998-1-laurent.pinchart@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
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@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]
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@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. > > > - 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]
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@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.) > >>>> - 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]
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@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]
Le dimanche 27 mars 2022 à 17:36 +0300, Laurent Pinchart via libcamera-devel a écrit : > 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@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 ? Correct, it simply change the indentation on already non-thread safe block. That scope is a left over of when it was last thread safe before we changed it :-D > > > > - 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] >
Le dimanche 27 mars 2022 à 17:48 +0300, Laurent Pinchart via libcamera-devel a écrit : > 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@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. Correct, though gst_libcamera_src_request_new_pad / gst_libcamera_src_release_pad() is called by application through gst_element_request_pad() / gst_element_release_request_pad(), and that from application threads, which is random thread. Notice the lock being properly taken in request_new_pad(). > > > > > - 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] >
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); } - { - 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; + 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
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@ideasonboard.com> --- src/gstreamer/gstlibcamerasrc.cpp | 26 +++++++++++++------------- 1 file changed, 13 insertions(+), 13 deletions(-)