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

[Umang Jain]

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.)

>
>>>> - 					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]