[{"id":22457,"web_url":"https://patchwork.libcamera.org/comment/22457/","msgid":"","date":"2022-03-27T14:36:27","subject":"Re: [libcamera-devel] [PATCH] gstreamer: Cleanup inner scope in\n\tgst_libcamera_src_task_run()","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Sun, Mar 27, 2022 at 10:25:27AM -0400, Nicolas Dufresne wrote:\n> Le 27 mars 2022 09 h 52, Laurent Pinchart a écrit :\n> \n> > The code block that checks for flow errors in\n> > gst_libcamera_src_task_run() is located in an inner scope whose purpose\n> > it to handling locking. The flow error handling however occurs before\n> > the lock is taken, so there's no need to place it in the inner scope.\n> > Move it one level up.\n> \n> > Signed-off-by: Laurent Pinchart \n> > ---\n> > src/gstreamer/gstlibcamerasrc.cpp | 26 +++++++++++++-------------\n> > 1 file changed, 13 insertions(+), 13 deletions(-)\n> \n> > diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/\n> > gstlibcamerasrc.cpp\n> > index 46fd02d207be..8016a98d6a4d 100644\n> > --- a/src/gstreamer/gstlibcamerasrc.cpp\n> > +++ b/src/gstreamer/gstlibcamerasrc.cpp\n> > @@ -311,21 +311,21 @@ gst_libcamera_src_task_run(gpointer user_data)\n> > \t\t\t\t\t\t\tsrcpad, ret);\n> > \t}\n> >\n> > - \t{\n\n\t^---- [1]\n\n> > - \t\tif (ret != GST_FLOW_OK) {\n> > - \t\t\tif (ret == GST_FLOW_EOS) {\n> > - \t\t\t\tg_autoptr(GstEvent) eos = gst_event_new_eos();\n> > - \t\t\t\tguint32 seqnum = gst_util_seqnum_next();\n> > - \t\t\t\tgst_event_set_seqnum(eos, seqnum);\n> > - \t\t\t\tfor (GstPad *srcpad : state->srcpads_)\n> \n> Iterating the srcpads_ is not thread safe. Won't this change remove the locking\n> for that ?\n\nThe patch doesn't change locking at all as far as I can tell, there's no\nlock taken at the beginning of the scope ([1]). There's a lock taken\nbelow ([2]), was it meant to cover this iteration too ?\n\n> > - \t\t\t\t\tgst_pad_push_event(srcpad, gst_event_ref(eos));\n> > - \t\t\t} else if (ret != GST_FLOW_FLUSHING) {\n> > - \t\t\t\tGST_ELEMENT_FLOW_ERROR(self, ret);\n> > - \t\t\t}\n> > - \t\t\tgst_task_stop(self->task);\n> > - \t\t\treturn;\n> > + \tif (ret != GST_FLOW_OK) {\n> > + \t\tif (ret == GST_FLOW_EOS) {\n> > + \t\t\tg_autoptr(GstEvent) eos = gst_event_new_eos();\n> > + \t\t\tguint32 seqnum = gst_util_seqnum_next();\n> > + \t\t\tgst_event_set_seqnum(eos, seqnum);\n> > + \t\t\tfor (GstPad *srcpad : state->srcpads_)\n> > + \t\t\t\tgst_pad_push_event(srcpad, gst_event_ref(eos));\n> > + \t\t\t} else if (ret != GST_FLOW_FLUSHING) {\n> > + \t\t\t\tGST_ELEMENT_FLOW_ERROR(self, ret);\n> > \t\t}\n> > + \t\tgst_task_stop(self->task);\n> > + \t\treturn;\n> > + \t}\n> > \n> > + \t{\n> > \t\t/*\n> > \t\t * Here we need to decide if we want to pause. This needs to\n> > \t\t * happen in lock step with the callback thread which may want\n\t\t */\n\t\tGLibLocker lock(GST_OBJECT(self));\n\n\t\t^---- [2]","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id F19D3C0F1B\n\tfor ;\n\tSun, 27 Mar 2022 14:36:32 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6E2DF600AB;\n\tSun, 27 Mar 2022 16:36:32 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 0C8BD600AB\n\tfor ;\n\tSun, 27 Mar 2022 16:36:30 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 6B1F32EC;\n\tSun, 27 Mar 2022 16:36:29 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1648391792;\n\tbh=x17Byk1gnaIyfh0Phbpw7BczWN4NtiujKWOGa8nbTco=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=OGaO/K7KB28MMtCRxhK9cwYtmnQw5oYm2PiRELeSIr4RcLdStzGbWKCU0NyTw59wq\n\tjFa1j2Akxb8NqLUI6Vlkp18avGvnA3FX/5+Cxn+tLoyer5v4R2Q695jbgAShV40oet\n\tWpS8uw2+8HE1YHN+PBJ8pkSGFk6j90iTSnLIT7BGsOfOTZCJIc6hcDUrO7iwP+Z1GA\n\tzXJUYOBAbQtmTliscioprS8R3fu8inucLEp4ArrK4jtEM2T/WM0mg8yFEl4cUckWYr\n\t9NgXgMwj0OSXZ+Rg/DK4RlJS0kzOpzX4vWLMkZUq0a67W9/Gre4C7Tx+UvKMKGZ9HR\n\tp6XYRHV8X/KOA==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1648391789;\n\tbh=x17Byk1gnaIyfh0Phbpw7BczWN4NtiujKWOGa8nbTco=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=AlHq/dgmmXJibCM+o11yRUTaHyddTTqHaD9o92RHBtH/f57cYJBrl6rObLaEOPwsm\n\t/o95nR8Hh+i/3qToGKhpmazDWfx59cWJiJ/9D6R6escL3bvXHZTvG0up9DAonQSj9B\n\tefTcv8sRIwnX9tmpQEqEEcmmc+ENXE1TG888a2Bs="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"AlHq/dgm\"; dkim-atps=neutral","Date":"Sun, 27 Mar 2022 17:36:27 +0300","To":"Nicolas Dufresne ","Message-ID":"","References":"<20220327135258.1998-1-laurent.pinchart@ideasonboard.com>\n\t<869c8e42-7987-44ad-a499-b4421003a896@email.android.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<869c8e42-7987-44ad-a499-b4421003a896@email.android.com>","Subject":"Re: [libcamera-devel] [PATCH] gstreamer: Cleanup inner scope in\n\tgst_libcamera_src_task_run()","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org,\n\tVedant Paranjape ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":22458,"web_url":"https://patchwork.libcamera.org/comment/22458/","msgid":"","date":"2022-03-27T14:48:58","subject":"Re: [libcamera-devel] [PATCH] gstreamer: Cleanup inner scope in\n\tgst_libcamera_src_task_run()","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Sun, Mar 27, 2022 at 05:36:29PM +0300, Laurent Pinchart wrote:\n> On Sun, Mar 27, 2022 at 10:25:27AM -0400, Nicolas Dufresne wrote:\n> > Le 27 mars 2022 09 h 52, Laurent Pinchart a écrit :\n> > \n> > > The code block that checks for flow errors in\n> > > gst_libcamera_src_task_run() is located in an inner scope whose purpose\n> > > it to handling locking. The flow error handling however occurs before\n> > > the lock is taken, so there's no need to place it in the inner scope.\n> > > Move it one level up.\n> > \n> > > Signed-off-by: Laurent Pinchart \n> > > ---\n> > > src/gstreamer/gstlibcamerasrc.cpp | 26 +++++++++++++-------------\n> > > 1 file changed, 13 insertions(+), 13 deletions(-)\n> > \n> > > diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/\n> > > gstlibcamerasrc.cpp\n> > > index 46fd02d207be..8016a98d6a4d 100644\n> > > --- a/src/gstreamer/gstlibcamerasrc.cpp\n> > > +++ b/src/gstreamer/gstlibcamerasrc.cpp\n> > > @@ -311,21 +311,21 @@ gst_libcamera_src_task_run(gpointer user_data)\n> > > \t\t\t\t\t\t\tsrcpad, ret);\n> > > \t}\n> > >\n> > > - \t{\n> \n> \t^---- [1]\n> \n> > > - \t\tif (ret != GST_FLOW_OK) {\n> > > - \t\t\tif (ret == GST_FLOW_EOS) {\n> > > - \t\t\t\tg_autoptr(GstEvent) eos = gst_event_new_eos();\n> > > - \t\t\t\tguint32 seqnum = gst_util_seqnum_next();\n> > > - \t\t\t\tgst_event_set_seqnum(eos, seqnum);\n> > > - \t\t\t\tfor (GstPad *srcpad : state->srcpads_)\n> > \n> > Iterating the srcpads_ is not thread safe. Won't this change remove the locking\n> > for that ?\n> \n> The patch doesn't change locking at all as far as I can tell, there's no\n> lock taken at the beginning of the scope ([1]). There's a lock taken\n> below ([2]), was it meant to cover this iteration too ?\n\nAlso, as far as I can tell, srcpads_ is only modified in\ngst_libcamera_src_init(), gst_libcamera_src_request_new_pad() and\ngst_libcamera_src_release_pad(). The former is called at init time only,\ncan the latter two (handling the .request_new_pad() and .release_pad())\nwe called while the task is running ? If so there's another possible\nrace, as srcpads_ is iterated earlier in this function, with no lock\ntaken.\n\n> > > - \t\t\t\t\tgst_pad_push_event(srcpad, gst_event_ref(eos));\n> > > - \t\t\t} else if (ret != GST_FLOW_FLUSHING) {\n> > > - \t\t\t\tGST_ELEMENT_FLOW_ERROR(self, ret);\n> > > - \t\t\t}\n> > > - \t\t\tgst_task_stop(self->task);\n> > > - \t\t\treturn;\n> > > + \tif (ret != GST_FLOW_OK) {\n> > > + \t\tif (ret == GST_FLOW_EOS) {\n> > > + \t\t\tg_autoptr(GstEvent) eos = gst_event_new_eos();\n> > > + \t\t\tguint32 seqnum = gst_util_seqnum_next();\n> > > + \t\t\tgst_event_set_seqnum(eos, seqnum);\n> > > + \t\t\tfor (GstPad *srcpad : state->srcpads_)\n> > > + \t\t\t\tgst_pad_push_event(srcpad, gst_event_ref(eos));\n> > > + \t\t\t} else if (ret != GST_FLOW_FLUSHING) {\n> > > + \t\t\t\tGST_ELEMENT_FLOW_ERROR(self, ret);\n> > > \t\t}\n> > > + \t\tgst_task_stop(self->task);\n> > > + \t\treturn;\n> > > + \t}\n> > > \n> > > + \t{\n> > > \t\t/*\n> > > \t\t * Here we need to decide if we want to pause. This needs to\n> > > \t\t * happen in lock step with the callback thread which may want\n> \t\t */\n> \t\tGLibLocker lock(GST_OBJECT(self));\n> \n> \t\t^---- [2]","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 57491C3256\n\tfor ;\n\tSun, 27 Mar 2022 14:49:02 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B37C9600B0;\n\tSun, 27 Mar 2022 16:49:01 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B93F5600AB\n\tfor ;\n\tSun, 27 Mar 2022 16:49:00 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 013A52EC;\n\tSun, 27 Mar 2022 16:48:59 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1648392541;\n\tbh=fUllLw62QdU1yQz2uByLGcptNkkBjcMs9+tg1I0e5k4=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=pkBGnbKMbwt6WmKl9l/n8szIUR1T6+ILYa8qiqWSTs9frQhw8B9CIswWwEEqlHHHV\n\tKZLtZlkxNYmEJ8RE2UWpWZWLuQB5gnJ6ldcyhwG1VflvXAft87PUxrky+IcNkYq0nU\n\txMx5ENCpV8CUHWwFX51KjAo83Mrt+OkQ1YiHD+qfeT/QLNl8DVc0eB2Lmdm2XjsU8D\n\tz+/V3Kikm9Zw4bNvIKSGdD7yeZJrYkiek/h0yoKebmoYBtscyiFkDvVXyek4dYXKqx\n\t5OqOFop8n4c4D7nWkSR9yHxWAnUyEP9mR7yEEQK7ac59Bv+Kv5NJRaI1DnEych9iiI\n\tMlAPlHPvHal6Q==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1648392540;\n\tbh=fUllLw62QdU1yQz2uByLGcptNkkBjcMs9+tg1I0e5k4=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=qa/JHDO1isRI0ms/dfhEg4wMS1L5cZE9xnkWhvb99PK2Kajykq4TqaVOSBin2II9N\n\tVVOlKkAnSPTqTeSFVvKScTRx2jfjGh+RusZElSkWq7fyWEpLPpdU+SG9xx73tCv8EA\n\tPCPrnMJsINYxj3p3oDvQbmO/xHOPBj+3BxKD7s8c="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"qa/JHDO1\"; dkim-atps=neutral","Date":"Sun, 27 Mar 2022 17:48:58 +0300","To":"Nicolas Dufresne ","Message-ID":"","References":"<20220327135258.1998-1-laurent.pinchart@ideasonboard.com>\n\t<869c8e42-7987-44ad-a499-b4421003a896@email.android.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH] gstreamer: Cleanup inner scope in\n\tgst_libcamera_src_task_run()","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org,\n\tVedant Paranjape ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":22459,"web_url":"https://patchwork.libcamera.org/comment/22459/","msgid":"<4ba28fe0-e2ca-5158-a8fa-f01aa7955a83@ideasonboard.com>","date":"2022-03-27T19:11:16","subject":"Re: [libcamera-devel] [PATCH] gstreamer: Cleanup inner scope in\n\tgst_libcamera_src_task_run()","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi,\n\nJust my 2 cents on a quick look.\n\nOn 3/27/22 20:18, Laurent Pinchart via libcamera-devel wrote:\n> On Sun, Mar 27, 2022 at 05:36:29PM +0300, Laurent Pinchart wrote:\n>> On Sun, Mar 27, 2022 at 10:25:27AM -0400, Nicolas Dufresne wrote:\n>>> Le 27 mars 2022 09 h 52, Laurent Pinchart a écrit :\n>>>\n>>>> The code block that checks for flow errors in\n>>>> gst_libcamera_src_task_run() is located in an inner scope whose purpose\n>>>> it to handling locking. The flow error handling however occurs before\n>>>> the lock is taken, so there's no need to place it in the inner scope.\n>>>> Move it one level up.\n>>>> Signed-off-by: Laurent Pinchart \n>>>> ---\n>>>> src/gstreamer/gstlibcamerasrc.cpp | 26 +++++++++++++-------------\n>>>> 1 file changed, 13 insertions(+), 13 deletions(-)\n>>>> diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/\n>>>> gstlibcamerasrc.cpp\n>>>> index 46fd02d207be..8016a98d6a4d 100644\n>>>> --- a/src/gstreamer/gstlibcamerasrc.cpp\n>>>> +++ b/src/gstreamer/gstlibcamerasrc.cpp\n>>>> @@ -311,21 +311,21 @@ gst_libcamera_src_task_run(gpointer user_data)\n>>>> \t\t\t\t\t\t\tsrcpad, ret);\n>>>> \t}\n>>>>\n>>>> - \t{\n>> \t^---- [1]\n>>\n>>>> - \t\tif (ret != GST_FLOW_OK) {\n>>>> - \t\t\tif (ret == GST_FLOW_EOS) {\n>>>> - \t\t\t\tg_autoptr(GstEvent) eos = gst_event_new_eos();\n>>>> - \t\t\t\tguint32 seqnum = gst_util_seqnum_next();\n>>>> - \t\t\t\tgst_event_set_seqnum(eos, seqnum);\n>>>> - \t\t\t\tfor (GstPad *srcpad : state->srcpads_)\n>>> Iterating the srcpads_ is not thread safe. Won't this change remove the locking\n>>> for that ?\n>> The patch doesn't change locking at all as far as I can tell, there's no\n>> lock taken at the beginning of the scope ([1]). There's a lock taken\n>> below ([2]), was it meant to cover this iteration too ?\n> Also, as far as I can tell, srcpads_ is only modified in\n> gst_libcamera_src_init(), gst_libcamera_src_request_new_pad() and\n> gst_libcamera_src_release_pad(). The former is called at init time only,\n> can the latter two (handling the .request_new_pad() and .release_pad())\n> we called while the task is running ? If so there's another possible\n> race, as srcpads_ is iterated earlier in this function, with no lock\n> taken.\n\n\nThe task is run with a stream_lock taken I think, see:\n\n     gst_task_set_lock(self->task, &self->stream_lock);\n\nin gst_libcamera_src_init()\n\n(I haven't looked anything deeper, which locks handles what etc.)\n\n>\n>>>> - \t\t\t\t\tgst_pad_push_event(srcpad, gst_event_ref(eos));\n>>>> - \t\t\t} else if (ret != GST_FLOW_FLUSHING) {\n>>>> - \t\t\t\tGST_ELEMENT_FLOW_ERROR(self, ret);\n>>>> - \t\t\t}\n>>>> - \t\t\tgst_task_stop(self->task);\n>>>> - \t\t\treturn;\n>>>> + \tif (ret != GST_FLOW_OK) {\n>>>> + \t\tif (ret == GST_FLOW_EOS) {\n>>>> + \t\t\tg_autoptr(GstEvent) eos = gst_event_new_eos();\n>>>> + \t\t\tguint32 seqnum = gst_util_seqnum_next();\n>>>> + \t\t\tgst_event_set_seqnum(eos, seqnum);\n>>>> + \t\t\tfor (GstPad *srcpad : state->srcpads_)\n>>>> + \t\t\t\tgst_pad_push_event(srcpad, gst_event_ref(eos));\n>>>> + \t\t\t} else if (ret != GST_FLOW_FLUSHING) {\n>>>> + \t\t\t\tGST_ELEMENT_FLOW_ERROR(self, ret);\n>>>> \t\t}\n>>>> + \t\tgst_task_stop(self->task);\n>>>> + \t\treturn;\n>>>> + \t}\n>>>> \n>>>> + \t{\n>>>> \t\t/*\n>>>> \t\t * Here we need to decide if we want to pause. This needs to\n>>>> \t\t * happen in lock step with the callback thread which may want\n>> \t\t */\n>> \t\tGLibLocker lock(GST_OBJECT(self));\n>>\n>> \t\t^---- [2]","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id BA082C0F1B\n\tfor ;\n\tSun, 27 Mar 2022 19:11:23 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1D66E600B0;\n\tSun, 27 Mar 2022 21:11:23 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B36DC600AB\n\tfor ;\n\tSun, 27 Mar 2022 21:11:21 +0200 (CEST)","from [IPV6:2401:4900:1f3e:6e7e:1ff:a5c2:a86e:ae1b] (unknown\n\t[IPv6:2401:4900:1f3e:6e7e:1ff:a5c2:a86e:ae1b])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 2D8B62F7;\n\tSun, 27 Mar 2022 21:11:19 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1648408283;\n\tbh=cpBdOWQ0UfC0rJ1kBIeltq/bVnAxIYVy8PLdRdbb7l4=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=27bEW04LysDWKL/TCnJPBpa0OUnXVtDpRQilb2X7K2euZkEbAjD9fgxjRnbasIv83\n\t/UTgIOgPFOCkDoj3XahZzycMkt20q0EP8xQN6xeI0bpYDqX5Ij5zWT9UxOFeX+LAqp\n\tQZqkXw3H2M8h2KKJyZO1GMwYwK+ZjXUA7EhXDuelNTqsuga7WEaM9zT1IEGbMGgNbC\n\tfj1bDOsrUvdwsOVodrGCLM9UiOVzAy/rO6iABlRpesn7hI2Wv45bz59eJAXwaDMZs2\n\tyVdq75wlD3GVEyJE5VChj0nX+KXeqLPAqQcR5AHAXoYyF2rgx1qee/M7pYp1HoYXxt\n\tGsZAswkTYBxtg==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1648408281;\n\tbh=cpBdOWQ0UfC0rJ1kBIeltq/bVnAxIYVy8PLdRdbb7l4=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=wKqfZj9o8BvUMN3Mti26b9zQjnIMQQQe9/XCs62p7yfaUgrwLy5k6p5lTMLYQopoM\n\tNxBbjJBTLtoE/+LH7Ae7ic4wHsJgtx3TetKo5ReSyMnmhIMzxbAqiOIQ6kVFLD9PaJ\n\tEqOGIpANWPFWFMP+VEIS/3jwNQ4YK0LqOSfHHk4Y="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"wKqfZj9o\"; dkim-atps=neutral","Message-ID":"<4ba28fe0-e2ca-5158-a8fa-f01aa7955a83@ideasonboard.com>","Date":"Mon, 28 Mar 2022 00:41:16 +0530","MIME-Version":"1.0","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101\n\tThunderbird/91.4.1","Content-Language":"en-US","To":"Laurent Pinchart ,\n\tNicolas Dufresne ","References":"<20220327135258.1998-1-laurent.pinchart@ideasonboard.com>\n\t<869c8e42-7987-44ad-a499-b4421003a896@email.android.com>\n\t\n\t","In-Reply-To":"","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH] gstreamer: Cleanup inner scope in\n\tgst_libcamera_src_task_run()","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Umang Jain via libcamera-devel ","Reply-To":"Umang Jain ","Cc":"libcamera-devel@lists.libcamera.org,\n\tVedant Paranjape ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":22494,"web_url":"https://patchwork.libcamera.org/comment/22494/","msgid":"","date":"2022-03-28T21:00:42","subject":"Re: [libcamera-devel] [PATCH] gstreamer: Cleanup inner scope in\n\tgst_libcamera_src_task_run()","submitter":{"id":31,"url":"https://patchwork.libcamera.org/api/people/31/","name":"Nicolas Dufresne","email":"nicolas.dufresne@collabora.com"},"content":"Le lundi 28 mars 2022 à 00:41 +0530, Umang Jain via libcamera-devel a écrit :\n> Hi,\n> \n> Just my 2 cents on a quick look.\n> \n> On 3/27/22 20:18, Laurent Pinchart via libcamera-devel wrote:\n> > On Sun, Mar 27, 2022 at 05:36:29PM +0300, Laurent Pinchart wrote:\n> > > On Sun, Mar 27, 2022 at 10:25:27AM -0400, Nicolas Dufresne wrote:\n> > > > Le 27 mars 2022 09 h 52, Laurent Pinchart a écrit :\n> > > > \n> > > > > The code block that checks for flow errors in\n> > > > > gst_libcamera_src_task_run() is located in an inner scope whose purpose\n> > > > > it to handling locking. The flow error handling however occurs before\n> > > > > the lock is taken, so there's no need to place it in the inner scope.\n> > > > > Move it one level up.\n> > > > > Signed-off-by: Laurent Pinchart \n> > > > > ---\n> > > > > src/gstreamer/gstlibcamerasrc.cpp | 26 +++++++++++++-------------\n> > > > > 1 file changed, 13 insertions(+), 13 deletions(-)\n> > > > > diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/\n> > > > > gstlibcamerasrc.cpp\n> > > > > index 46fd02d207be..8016a98d6a4d 100644\n> > > > > --- a/src/gstreamer/gstlibcamerasrc.cpp\n> > > > > +++ b/src/gstreamer/gstlibcamerasrc.cpp\n> > > > > @@ -311,21 +311,21 @@ gst_libcamera_src_task_run(gpointer user_data)\n> > > > > \t\t\t\t\t\t\tsrcpad, ret);\n> > > > > \t}\n> > > > > \n> > > > > - \t{\n> > > \t^---- [1]\n> > > \n> > > > > - \t\tif (ret != GST_FLOW_OK) {\n> > > > > - \t\t\tif (ret == GST_FLOW_EOS) {\n> > > > > - \t\t\t\tg_autoptr(GstEvent) eos = gst_event_new_eos();\n> > > > > - \t\t\t\tguint32 seqnum = gst_util_seqnum_next();\n> > > > > - \t\t\t\tgst_event_set_seqnum(eos, seqnum);\n> > > > > - \t\t\t\tfor (GstPad *srcpad : state->srcpads_)\n> > > > Iterating the srcpads_ is not thread safe. Won't this change remove the locking\n> > > > for that ?\n> > > The patch doesn't change locking at all as far as I can tell, there's no\n> > > lock taken at the beginning of the scope ([1]). There's a lock taken\n> > > below ([2]), was it meant to cover this iteration too ?\n> > Also, as far as I can tell, srcpads_ is only modified in\n> > gst_libcamera_src_init(), gst_libcamera_src_request_new_pad() and\n> > gst_libcamera_src_release_pad(). The former is called at init time only,\n> > can the latter two (handling the .request_new_pad() and .release_pad())\n> > we called while the task is running ? If so there's another possible\n> > race, as srcpads_ is iterated earlier in this function, with no lock\n> > taken.\n> \n> \n> The task is run with a stream_lock taken I think, see:\n> \n>     gst_task_set_lock(self->task, &self->stream_lock);\n> \n> in gst_libcamera_src_init()\n> \n> (I haven't looked anything deeper, which locks handles what etc.)\n\nBut gst_libcamera_src_request_new_pad() use the object lock to protect srcpads_\nadditions:\n\n...\n\n\t\tGLibLocker lock(GST_OBJECT(self));\n\t\tself->state->srcpads_.push_back(reinterpret_cast(g_object_ref(pad)));\n...\n\nSo yes, the change does not break anything, but it changes obviously broken code\nwithout fixing it. Though, I think the way forward is to special case that\nfunction and use the gstreamer thread safe pad iterator. See\ngst_element_iterate_src_pads(). As we can't hold the object lock while sending\nevents, or acquiring buffers from the pool (if blocking, if non-blocking that is\nfine as long as the release of buffers don't need to take the object lock).\n\n> \n> > \n> > > > > - \t\t\t\t\tgst_pad_push_event(srcpad, gst_event_ref(eos));\n> > > > > - \t\t\t} else if (ret != GST_FLOW_FLUSHING) {\n> > > > > - \t\t\t\tGST_ELEMENT_FLOW_ERROR(self, ret);\n> > > > > - \t\t\t}\n> > > > > - \t\t\tgst_task_stop(self->task);\n> > > > > - \t\t\treturn;\n> > > > > + \tif (ret != GST_FLOW_OK) {\n> > > > > + \t\tif (ret == GST_FLOW_EOS) {\n> > > > > + \t\t\tg_autoptr(GstEvent) eos = gst_event_new_eos();\n> > > > > + \t\t\tguint32 seqnum = gst_util_seqnum_next();\n> > > > > + \t\t\tgst_event_set_seqnum(eos, seqnum);\n> > > > > + \t\t\tfor (GstPad *srcpad : state->srcpads_)\n> > > > > + \t\t\t\tgst_pad_push_event(srcpad, gst_event_ref(eos));\n> > > > > + \t\t\t} else if (ret != GST_FLOW_FLUSHING) {\n> > > > > + \t\t\t\tGST_ELEMENT_FLOW_ERROR(self, ret);\n> > > > > \t\t}\n> > > > > + \t\tgst_task_stop(self->task);\n> > > > > + \t\treturn;\n> > > > > + \t}\n> > > > > \n> > > > > + \t{\n> > > > > \t\t/*\n> > > > > \t\t * Here we need to decide if we want to pause. This needs to\n> > > > > \t\t * happen in lock step with the callback thread which may want\n> > > \t\t */\n> > > \t\tGLibLocker lock(GST_OBJECT(self));\n> > > \n> > > \t\t^---- [2]","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id B230AC3256\n\tfor ;\n\tMon, 28 Mar 2022 21:01:00 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id CCB7F65633;\n\tMon, 28 Mar 2022 23:00:59 +0200 (CEST)","from bhuna.collabora.co.uk (bhuna.collabora.co.uk\n\t[IPv6:2a00:1098:0:82:1000:25:2eeb:e3e3])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id EEA1D604C0\n\tfor ;\n\tMon, 28 Mar 2022 23:00:57 +0200 (CEST)","from [127.0.0.1] (localhost [127.0.0.1])\n\t(Authenticated sender: nicolas) with ESMTPSA id EEA3D1F41B53"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1648501259;\n\tbh=0u9OIbYqqdi+13vVBRKh1QTaMVoNiW9IVEpHewwTSX8=;\n\th=To:Date:In-Reply-To:References:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=vT3f3wBoMRy1B5Zl8yTVUzyit87vOn9p+mihSk56ftzyQQB+GwwcLzn0suWJjB/q6\n\tlqQeKVYyDU6ICCZcGpsZoKkCMZFcpf/dQd+lZ2a2Po7azUp3dfrpZ97dKekEkzdXC6\n\tJ5k0CZ2YuznLwHOqK9/6MbnFvC0zHeqULekar8wFAtIQjpScAMLPzJ0RHWDpVIMJ3+\n\tmCuqHwxZIBQi9I29+Wa6RFmCFWEBwcupmuDE+zJl9rUOtkv5q6OslEYcAbpMg7YhyB\n\tUkIxsZXZzXT5mL7qgW098qK/YT0CV2bEW1OZ8CbM8Yd/O6EPARHUrWft1yalYMoahd\n\tsX/I+Po3brDFw==","v=1; a=rsa-sha256; c=relaxed/simple; d=collabora.com;\n\ts=mail; t=1648501257;\n\tbh=0u9OIbYqqdi+13vVBRKh1QTaMVoNiW9IVEpHewwTSX8=;\n\th=Subject:From:To:Cc:Date:In-Reply-To:References:From;\n\tb=l+7882b5kbHjv0n4dGtXM01SDJBZm7XuvLPWtHTMneDH0oerKbhn3EGQ0lt1vrLbh\n\tIwWWtTQeoT1Uc98luE3+q8rU09otwSHLYUwEs2Y+THqLA7oF3bm6Qrb3Nb5r6vcTgs\n\teYURYbGrVPzL4f8cb8FFxwr80jH62oNXj9i92XisMdE1ffMoXA2f1cxJay3sOkeKpF\n\tVMWaAl0lSNYIBxyq3zhh/MMUrS47aRWxPNue2ERm5sxBtqDVqpUxFeD6FIKs24uMbz\n\tkslb+E1Ef3jmCVYvw5zAqvXOKLHtVrVCPrBtKt1myZLKeZd4pyNuOSnDlgffWL35k6\n\tJbEpQOgfdVWtw=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=collabora.com\n\theader.i=@collabora.com\n\theader.b=\"l+7882b5\"; dkim-atps=neutral","Message-ID":"","To":"Umang Jain , Laurent Pinchart\n\t","Date":"Mon, 28 Mar 2022 17:00:42 -0400","In-Reply-To":"<4ba28fe0-e2ca-5158-a8fa-f01aa7955a83@ideasonboard.com>","References":"<20220327135258.1998-1-laurent.pinchart@ideasonboard.com>\n\t<869c8e42-7987-44ad-a499-b4421003a896@email.android.com>\n\t\n\t\n\t<4ba28fe0-e2ca-5158-a8fa-f01aa7955a83@ideasonboard.com>","Content-Type":"text/plain; charset=\"UTF-8\"","Content-Transfer-Encoding":"quoted-printable","User-Agent":"Evolution 3.44.0 (3.44.0-1.fc36) ","MIME-Version":"1.0","Subject":"Re: [libcamera-devel] [PATCH] gstreamer: Cleanup inner scope in\n\tgst_libcamera_src_task_run()","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Nicolas Dufresne via libcamera-devel\n\t","Reply-To":"Nicolas Dufresne ","Cc":"libcamera-devel@lists.libcamera.org,\n\tVedant Paranjape ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":22495,"web_url":"https://patchwork.libcamera.org/comment/22495/","msgid":"<953fab17ebe20cca0349497cf04a4ecfa2e832d8.camel@collabora.com>","date":"2022-03-28T21:02:09","subject":"Re: [libcamera-devel] [PATCH] gstreamer: Cleanup inner scope in\n\tgst_libcamera_src_task_run()","submitter":{"id":31,"url":"https://patchwork.libcamera.org/api/people/31/","name":"Nicolas Dufresne","email":"nicolas.dufresne@collabora.com"},"content":"Le dimanche 27 mars 2022 à 17:36 +0300, Laurent Pinchart via libcamera-devel a\nécrit :\n> On Sun, Mar 27, 2022 at 10:25:27AM -0400, Nicolas Dufresne wrote:\n> > Le 27 mars 2022 09 h 52, Laurent Pinchart a écrit :\n> > \n> > > The code block that checks for flow errors in\n> > > gst_libcamera_src_task_run() is located in an inner scope whose purpose\n> > > it to handling locking. The flow error handling however occurs before\n> > > the lock is taken, so there's no need to place it in the inner scope.\n> > > Move it one level up.\n> > \n> > > Signed-off-by: Laurent Pinchart \n> > > ---\n> > > src/gstreamer/gstlibcamerasrc.cpp | 26 +++++++++++++-------------\n> > > 1 file changed, 13 insertions(+), 13 deletions(-)\n> > \n> > > diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/\n> > > gstlibcamerasrc.cpp\n> > > index 46fd02d207be..8016a98d6a4d 100644\n> > > --- a/src/gstreamer/gstlibcamerasrc.cpp\n> > > +++ b/src/gstreamer/gstlibcamerasrc.cpp\n> > > @@ -311,21 +311,21 @@ gst_libcamera_src_task_run(gpointer user_data)\n> > > \t\t\t\t\t\t\tsrcpad, ret);\n> > > \t}\n> > > \n> > > - \t{\n> \n> \t^---- [1]\n> \n> > > - \t\tif (ret != GST_FLOW_OK) {\n> > > - \t\t\tif (ret == GST_FLOW_EOS) {\n> > > - \t\t\t\tg_autoptr(GstEvent) eos = gst_event_new_eos();\n> > > - \t\t\t\tguint32 seqnum = gst_util_seqnum_next();\n> > > - \t\t\t\tgst_event_set_seqnum(eos, seqnum);\n> > > - \t\t\t\tfor (GstPad *srcpad : state->srcpads_)\n> > \n> > Iterating the srcpads_ is not thread safe. Won't this change remove the locking\n> > for that ?\n> \n> The patch doesn't change locking at all as far as I can tell, there's no\n> lock taken at the beginning of the scope ([1]). There's a lock taken\n> below ([2]), was it meant to cover this iteration too ?\n\nCorrect, it simply change the indentation on already non-thread safe block. That\nscope is a left over of when it was last thread safe before we changed it :-D\n\n> \n> > > - \t\t\t\t\tgst_pad_push_event(srcpad, gst_event_ref(eos));\n> > > - \t\t\t} else if (ret != GST_FLOW_FLUSHING) {\n> > > - \t\t\t\tGST_ELEMENT_FLOW_ERROR(self, ret);\n> > > - \t\t\t}\n> > > - \t\t\tgst_task_stop(self->task);\n> > > - \t\t\treturn;\n> > > + \tif (ret != GST_FLOW_OK) {\n> > > + \t\tif (ret == GST_FLOW_EOS) {\n> > > + \t\t\tg_autoptr(GstEvent) eos = gst_event_new_eos();\n> > > + \t\t\tguint32 seqnum = gst_util_seqnum_next();\n> > > + \t\t\tgst_event_set_seqnum(eos, seqnum);\n> > > + \t\t\tfor (GstPad *srcpad : state->srcpads_)\n> > > + \t\t\t\tgst_pad_push_event(srcpad, gst_event_ref(eos));\n> > > + \t\t\t} else if (ret != GST_FLOW_FLUSHING) {\n> > > + \t\t\t\tGST_ELEMENT_FLOW_ERROR(self, ret);\n> > > \t\t}\n> > > + \t\tgst_task_stop(self->task);\n> > > + \t\treturn;\n> > > + \t}\n> > > \n> > > + \t{\n> > > \t\t/*\n> > > \t\t * Here we need to decide if we want to pause. This needs to\n> > > \t\t * happen in lock step with the callback thread which may want\n> \t\t */\n> \t\tGLibLocker lock(GST_OBJECT(self));\n> \n> \t\t^---- [2]\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 09EB3C3256\n\tfor ;\n\tMon, 28 Mar 2022 21:02:22 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B8CB665633;\n\tMon, 28 Mar 2022 23:02:21 +0200 (CEST)","from bhuna.collabora.co.uk (bhuna.collabora.co.uk [46.235.227.227])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id CCCAA604C0\n\tfor ;\n\tMon, 28 Mar 2022 23:02:19 +0200 (CEST)","from [127.0.0.1] (localhost [127.0.0.1])\n\t(Authenticated sender: nicolas) with ESMTPSA id 271D01F429D1"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1648501341;\n\tbh=xV0a3+uoPL3PGDy2Wep30MNh6NF8afidj+P6MXC59K8=;\n\th=To:Date:In-Reply-To:References:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=B5rp0HtHxm4xCd1v422TL16g3Mb4fZUzvNeh9XfDbEibPMUB5OaoAOHApfiHdJy8w\n\tmVd9T9GU2zOSZG8nN/OxqzGKfc8wEyzoNma4Lsv47vODLxg/qH/G6hVrhVG45VpeAF\n\t2WfXqCA/1mk4FVs2Xky5sh5QK0DldzGjqVg1a/G6ssMei/a46PtJe6c9z/l7AEm4J5\n\t+GaH9GfMzmIiJH6OJpQAWFKLj6KW+HiUc7XdZwBqhD0lyBX43hC/wD0/1r0KCUDQQr\n\t1d6pJXFjcPZ3EBelBJh+3mUZN162HEHaHxo+aqVeo+mKo11UjYoDZV8VPxbqXzgPqF\n\t6if5hA8c5NACg==","v=1; a=rsa-sha256; c=relaxed/simple; d=collabora.com;\n\ts=mail; t=1648501339;\n\tbh=xV0a3+uoPL3PGDy2Wep30MNh6NF8afidj+P6MXC59K8=;\n\th=Subject:From:To:Cc:Date:In-Reply-To:References:From;\n\tb=fbPqLB+F1yAzkEkF1c4NveXjxbCA3Q4+A18ihUY0Scy/8DuFdN40cxGj+e2FTx9fx\n\tHCxbBL2NrGU5kKU9Xxdv0Z8WVyjHShtTW1L6Lvcj0/x762aCXXCDv1P8B8SC9zNBbB\n\t36lMLvnwUxJJD5cHankauDhxJ9ioylO/8TjkcGLJ42Pevusdp8mlc0oUxIyfewwm//\n\te66E0lGMjBbGtJ/P90GuRX2blY4HzDXLCVNFkARNLpuvzuwd/srW8DRsb/3p/mWXx1\n\tuejTaZbwfV3s2j/JrA5jzBPgN+dCr+dOnt78tqP12N7puKbAnKIm+++yE9XSt5o3FU\n\tK5NsMBcl8Ttng=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=collabora.com\n\theader.i=@collabora.com\n\theader.b=\"fbPqLB+F\"; dkim-atps=neutral","Message-ID":"<953fab17ebe20cca0349497cf04a4ecfa2e832d8.camel@collabora.com>","To":"Laurent Pinchart ","Date":"Mon, 28 Mar 2022 17:02:09 -0400","In-Reply-To":"","References":"<20220327135258.1998-1-laurent.pinchart@ideasonboard.com>\n\t<869c8e42-7987-44ad-a499-b4421003a896@email.android.com>\n\t","Content-Type":"text/plain; charset=\"UTF-8\"","Content-Transfer-Encoding":"quoted-printable","User-Agent":"Evolution 3.44.0 (3.44.0-1.fc36) ","MIME-Version":"1.0","Subject":"Re: [libcamera-devel] [PATCH] gstreamer: Cleanup inner scope in\n\tgst_libcamera_src_task_run()","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Nicolas Dufresne via libcamera-devel\n\t","Reply-To":"Nicolas Dufresne ","Cc":"libcamera-devel@lists.libcamera.org,\n\tVedant Paranjape ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":22496,"web_url":"https://patchwork.libcamera.org/comment/22496/","msgid":"<42882775d216ec94662c75d5e424af7173c3fedf.camel@collabora.com>","date":"2022-03-28T21:04:09","subject":"Re: [libcamera-devel] [PATCH] gstreamer: Cleanup inner scope in\n\tgst_libcamera_src_task_run()","submitter":{"id":31,"url":"https://patchwork.libcamera.org/api/people/31/","name":"Nicolas Dufresne","email":"nicolas.dufresne@collabora.com"},"content":"Le dimanche 27 mars 2022 à 17:48 +0300, Laurent Pinchart via libcamera-devel a\nécrit :\n> On Sun, Mar 27, 2022 at 05:36:29PM +0300, Laurent Pinchart wrote:\n> > On Sun, Mar 27, 2022 at 10:25:27AM -0400, Nicolas Dufresne wrote:\n> > > Le 27 mars 2022 09 h 52, Laurent Pinchart a écrit :\n> > > \n> > > > The code block that checks for flow errors in\n> > > > gst_libcamera_src_task_run() is located in an inner scope whose purpose\n> > > > it to handling locking. The flow error handling however occurs before\n> > > > the lock is taken, so there's no need to place it in the inner scope.\n> > > > Move it one level up.\n> > > \n> > > > Signed-off-by: Laurent Pinchart \n> > > > ---\n> > > > src/gstreamer/gstlibcamerasrc.cpp | 26 +++++++++++++-------------\n> > > > 1 file changed, 13 insertions(+), 13 deletions(-)\n> > > \n> > > > diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/\n> > > > gstlibcamerasrc.cpp\n> > > > index 46fd02d207be..8016a98d6a4d 100644\n> > > > --- a/src/gstreamer/gstlibcamerasrc.cpp\n> > > > +++ b/src/gstreamer/gstlibcamerasrc.cpp\n> > > > @@ -311,21 +311,21 @@ gst_libcamera_src_task_run(gpointer user_data)\n> > > > \t\t\t\t\t\t\tsrcpad, ret);\n> > > > \t}\n> > > > \n> > > > - \t{\n> > \n> > \t^---- [1]\n> > \n> > > > - \t\tif (ret != GST_FLOW_OK) {\n> > > > - \t\t\tif (ret == GST_FLOW_EOS) {\n> > > > - \t\t\t\tg_autoptr(GstEvent) eos = gst_event_new_eos();\n> > > > - \t\t\t\tguint32 seqnum = gst_util_seqnum_next();\n> > > > - \t\t\t\tgst_event_set_seqnum(eos, seqnum);\n> > > > - \t\t\t\tfor (GstPad *srcpad : state->srcpads_)\n> > > \n> > > Iterating the srcpads_ is not thread safe. Won't this change remove the locking\n> > > for that ?\n> > \n> > The patch doesn't change locking at all as far as I can tell, there's no\n> > lock taken at the beginning of the scope ([1]). There's a lock taken\n> > below ([2]), was it meant to cover this iteration too ?\n> \n> Also, as far as I can tell, srcpads_ is only modified in\n> gst_libcamera_src_init(), gst_libcamera_src_request_new_pad() and\n> gst_libcamera_src_release_pad(). The former is called at init time only,\n> can the latter two (handling the .request_new_pad() and .release_pad())\n> we called while the task is running ? If so there's another possible\n> race, as srcpads_ is iterated earlier in this function, with no lock\n> taken.\n\nCorrect, though gst_libcamera_src_request_new_pad /\ngst_libcamera_src_release_pad() is called by application through\ngst_element_request_pad() / gst_element_release_request_pad(), and that from\napplication threads, which is random thread. Notice the lock being properly\ntaken in request_new_pad().\n\n> \n> > > > - \t\t\t\t\tgst_pad_push_event(srcpad, gst_event_ref(eos));\n> > > > - \t\t\t} else if (ret != GST_FLOW_FLUSHING) {\n> > > > - \t\t\t\tGST_ELEMENT_FLOW_ERROR(self, ret);\n> > > > - \t\t\t}\n> > > > - \t\t\tgst_task_stop(self->task);\n> > > > - \t\t\treturn;\n> > > > + \tif (ret != GST_FLOW_OK) {\n> > > > + \t\tif (ret == GST_FLOW_EOS) {\n> > > > + \t\t\tg_autoptr(GstEvent) eos = gst_event_new_eos();\n> > > > + \t\t\tguint32 seqnum = gst_util_seqnum_next();\n> > > > + \t\t\tgst_event_set_seqnum(eos, seqnum);\n> > > > + \t\t\tfor (GstPad *srcpad : state->srcpads_)\n> > > > + \t\t\t\tgst_pad_push_event(srcpad, gst_event_ref(eos));\n> > > > + \t\t\t} else if (ret != GST_FLOW_FLUSHING) {\n> > > > + \t\t\t\tGST_ELEMENT_FLOW_ERROR(self, ret);\n> > > > \t\t}\n> > > > + \t\tgst_task_stop(self->task);\n> > > > + \t\treturn;\n> > > > + \t}\n> > > > \n> > > > + \t{\n> > > > \t\t/*\n> > > > \t\t * Here we need to decide if we want to pause. This needs to\n> > > > \t\t * happen in lock step with the callback thread which may want\n> > \t\t */\n> > \t\tGLibLocker lock(GST_OBJECT(self));\n> > \n> > \t\t^---- [2]\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id E83A1C0F1B\n\tfor ;\n\tMon, 28 Mar 2022 21:04:20 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5958065635;\n\tMon, 28 Mar 2022 23:04:20 +0200 (CEST)","from bhuna.collabora.co.uk (bhuna.collabora.co.uk\n\t[IPv6:2a00:1098:0:82:1000:25:2eeb:e3e3])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 9477F604C0\n\tfor ;\n\tMon, 28 Mar 2022 23:04:19 +0200 (CEST)","from [127.0.0.1] (localhost [127.0.0.1])\n\t(Authenticated sender: nicolas) with ESMTPSA id BE9B61F429D1"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1648501460;\n\tbh=vj6YsUd2ARo/n/xjhxdhKOrHPJi7AZAtfa+oI2v+5Co=;\n\th=To:Date:In-Reply-To:References:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=vHtv0RGoLyHjhg5thFynfCriou5uO1UV+SHJeICHf86Rgxv8KaVhKA1es9kHiAGoK\n\t9mcJH5IoJVXGdlQd+jNLFTu5p1XiodjNJdGVdR9BpD16jhetOtqk5qOInmNBDLGJny\n\tJoxAqSMUursVJDKj1oZC8T/AbRFN8foWVpdyOC1r1pbbhxlnNSpJy1mM8Eg31XkvGb\n\tYlmhapRGcHWEQC17x7GzjZsufQb6Ac9XqJxX2mzpS9XUrJ2xKqXVHTXlqjX4Xr0YKC\n\tBxRmkohOGaxK7tM7J/lblwxC6Zfg8iIXmIfBTddyUy5w29fWlP0ihjdjOMYq3VYvHz\n\tWrbETwlsEMdSw==","v=1; a=rsa-sha256; c=relaxed/simple; d=collabora.com;\n\ts=mail; t=1648501459;\n\tbh=vj6YsUd2ARo/n/xjhxdhKOrHPJi7AZAtfa+oI2v+5Co=;\n\th=Subject:From:To:Cc:Date:In-Reply-To:References:From;\n\tb=BZyQ0HcYVhcP9pLQ24EamXYEz4yRksvcKFpu4PxmpdWmzvLr78UkW5DinyHp/O6Fd\n\t7wrbq8osrTSv2bWC6lUF6clSHoJd13jHrkGuqV2PWmNlCMnVanTFsIAMfvROuArM9z\n\tWzOfYy6/HdwsBbWr5vYHl/H2NWot0YKaK6OkRYcxy+dpecG+T87PKBAIE8aqtItdhN\n\tHP3+FOTwyKXJI4D8fy4XUk0Smguhy9ki1ZoIrNSdEbSON4QWrRBCYD3IY9sxti+wz8\n\tzfP1Te0bgGA75vFdxoBhiPQdV+KFg6K4O7S5FdCy7fAUkZiBiJPEq0CHOfJFf8bshi\n\t0q0Shw+2vpqLg=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=collabora.com\n\theader.i=@collabora.com\n\theader.b=\"BZyQ0HcY\"; dkim-atps=neutral","Message-ID":"<42882775d216ec94662c75d5e424af7173c3fedf.camel@collabora.com>","To":"Laurent Pinchart ","Date":"Mon, 28 Mar 2022 17:04:09 -0400","In-Reply-To":"","References":"<20220327135258.1998-1-laurent.pinchart@ideasonboard.com>\n\t<869c8e42-7987-44ad-a499-b4421003a896@email.android.com>\n\t\n\t","Content-Type":"text/plain; charset=\"UTF-8\"","Content-Transfer-Encoding":"quoted-printable","User-Agent":"Evolution 3.44.0 (3.44.0-1.fc36) ","MIME-Version":"1.0","Subject":"Re: [libcamera-devel] [PATCH] gstreamer: Cleanup inner scope in\n\tgst_libcamera_src_task_run()","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Nicolas Dufresne via libcamera-devel\n\t","Reply-To":"Nicolas Dufresne ","Cc":"libcamera-devel@lists.libcamera.org,\n\tVedant Paranjape ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]