[{"id":23677,"web_url":"https://patchwork.libcamera.org/comment/23677/","msgid":"<841109a2-de4c-d190-cfd9-648226fca868@ideasonboard.com>","date":"2022-06-30T09:08:14","subject":"Re: [libcamera-devel] [PATCH v2 09/12] gstreamer: Fix pads locking","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi Laurent,\n\nThank you for the patch.\n\nOn 6/30/22 05:32, Laurent Pinchart via libcamera-devel wrote:\n> The srcpads_ vector is protected by two different locks, the GstObject\n> lock of the libcamerasrc element, and the stream_lock that covers the\n> run function of the thread. This isn't correct. Use the stream_lock\n> consistently to protect the pads.\n>\n> Signed-off-by: Laurent Pinchart \n> Reviewed-by: Nicolas Dufresne \n\n\nReviewed-by: Umang Jain \n\n> ---\n> Changes since v1:\n>\n> - Add a comment about lock ordering\n> ---\n> src/gstreamer/gstlibcamerasrc.cpp | 73 +++++++++++++++++--------------\n> 1 file changed, 40 insertions(+), 33 deletions(-)\n>\n> diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp\n> index 6f9a03c515d2..c92ca7d29fe6 100644\n> --- a/src/gstreamer/gstlibcamerasrc.cpp\n> +++ b/src/gstreamer/gstlibcamerasrc.cpp\n> @@ -112,12 +112,18 @@ struct GstLibcameraSrcState {\n> \tstd::shared_ptr cm_;\n> \tstd::shared_ptr cam_;\n> \tstd::unique_ptr config_;\n> -\tstd::vector srcpads_;\n> +\n> +\tstd::vector srcpads_; /* Protected by stream_lock */\n> \n> \t/*\n> \t * Contention on this lock_ must be minimized, as it has to be taken in\n> \t * the realtime-sensitive requestCompleted() handler to protect\n> \t * queuedRequests_ and completedRequests_.\n> +\t *\n> +\t * stream_lock must be taken before lock_ in contexts where both locks\n> +\t * need to be taken. In particular, this means that the lock_ must not\n> +\t * be held while calling into other graph elements (e.g. when calling\n> +\t * gst_pad_query()).\n> \t */\n> \tMutex lock_;\n> \tstd::queue> queuedRequests_\n> @@ -354,36 +360,34 @@ gst_libcamera_src_task_run(gpointer user_data)\n> \t\t\t\t\t\t\tsrcpad, ret);\n> \t}\n> \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} else if (ret != GST_FLOW_FLUSHING) {\n> +\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 * Here we need to decide if we want to pause. This needs to\n> +\t * happen in lock step with the callback thread which may want\n> +\t * to resume the task and might push pending buffers.\n> +\t */\n> +\tbool do_pause;\n> +\n> \t{\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> -\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> -\t\t}\n> -\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 * to resume the task and might push pending buffers.\n> -\t\t */\n> -\t\tbool do_pause;\n> -\n> -\t\t{\n> -\t\t\tMutexLocker locker(state->lock_);\n> -\t\t\tdo_pause = state->completedRequests_.empty();\n> -\t\t}\n> -\n> -\t\tif (do_pause)\n> -\t\t\tgst_task_pause(self->task);\n> +\t\tMutexLocker locker(state->lock_);\n> +\t\tdo_pause = state->completedRequests_.empty();\n> \t}\n> +\n> +\tif (do_pause)\n> +\t\tgst_task_pause(self->task);\n> }\n> \n> static void\n> @@ -537,8 +541,11 @@ gst_libcamera_src_task_leave([[maybe_unused]] GstTask *task,\n> \t\tstate->completedRequests_ = {};\n> \t}\n> \n> -\tfor (GstPad *srcpad : state->srcpads_)\n> -\t\tgst_libcamera_pad_set_pool(srcpad, nullptr);\n> +\t{\n> +\t\tGLibRecLocker locker(&self->stream_lock);\n> +\t\tfor (GstPad *srcpad : state->srcpads_)\n> +\t\t\tgst_libcamera_pad_set_pool(srcpad, nullptr);\n> +\t}\n> \n> \tg_clear_object(&self->allocator);\n> \tg_clear_pointer(&self->flow_combiner,\n> @@ -697,7 +704,7 @@ gst_libcamera_src_request_new_pad(GstElement *element, GstPadTemplate *templ,\n> \tg_object_ref_sink(pad);\n> \n> \tif (gst_element_add_pad(element, pad)) {\n> -\t\tGLibLocker lock(GST_OBJECT(self));\n> +\t\tGLibRecLocker lock(&self->stream_lock);\n> \t\tself->state->srcpads_.push_back(reinterpret_cast(g_object_ref(pad)));\n> \t} else {\n> \t\tGST_ELEMENT_ERROR(element, STREAM, FAILED,\n> @@ -717,7 +724,7 @@ gst_libcamera_src_release_pad(GstElement *element, GstPad *pad)\n> \tGST_DEBUG_OBJECT(self, \"Pad %\" GST_PTR_FORMAT \" being released\", pad);\n> \n> \t{\n> -\t\tGLibLocker lock(GST_OBJECT(self));\n> +\t\tGLibRecLocker lock(&self->stream_lock);\n> \t\tstd::vector &pads = self->state->srcpads_;\n> \t\tauto begin_iterator = pads.begin();\n> \t\tauto end_iterator = pads.end();","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 7F461BD808\n\tfor ;\n\tThu, 30 Jun 2022 09:08:27 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id CE8CA6564E;\n\tThu, 30 Jun 2022 11:08:26 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 93EB66054C\n\tfor ;\n\tThu, 30 Jun 2022 11:08:25 +0200 (CEST)","from [IPV6:2401:4900:1f3f:ca21:e286:106b:5da4:9482] (unknown\n\t[IPv6:2401:4900:1f3f:ca21:e286:106b:5da4:9482])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id B3F6345F;\n\tThu, 30 Jun 2022 11:08:22 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1656580106;\n\tbh=csL2gDRYpcXWC244U9qJRbJ3JvXrQ/J9UJawZEXs1u8=;\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=b3tnhgTt/speMvy4YoezQZ7AFYMOXYksg5KBJ/p7Kk5Fq9QC19sVV9QDyhk2ATQaT\n\tdwVf4jQjIAHhGh0y/ISs6d9G84FxGlJudidjKybre1IK337BTA0HN8RwSPHuHS4t63\n\tOapzWLJn4cM4a97Mzff8dpLPiyjuHt9l75QEScLg4Ex2wVdTu+T9hb0ze0xz+UBTOl\n\t3b3Dh2YJxJ9O/Nb+jVS956xACggYVF+1yRMt9CtgIWUvpy333BRQk9Vzm7XArj9DcN\n\t8FHVo6xn6CcHvB2Ef4ekqK1DCQjZXybNi7b3WCCZSs4Vl1tzNfoDQ3xuoz3cNRAJvk\n\t5awxcnN0+dbPw==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1656580105;\n\tbh=csL2gDRYpcXWC244U9qJRbJ3JvXrQ/J9UJawZEXs1u8=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=gAselZ3Ml4c3L9Csgp30YQa2NnP5SjRuGRLK9q82UmDo4bmDj2Nt988skZM14bfUv\n\tTxEX97eLyVeP9NfmAxpYsEcRtrnM8HfqYpno0v4styaXWkSACkY+lIBh/XSp+VxIpq\n\tGQ/dQpYrThYfkEAGXcFxgUsKcnd3tPb5qqKL1WuM="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"gAselZ3M\"; dkim-atps=neutral","Message-ID":"<841109a2-de4c-d190-cfd9-648226fca868@ideasonboard.com>","Date":"Thu, 30 Jun 2022 14:38:14 +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\tlibcamera-devel@lists.libcamera.org","References":"<20220630000251.31295-1-laurent.pinchart@ideasonboard.com>\n\t<20220630000251.31295-10-laurent.pinchart@ideasonboard.com>","In-Reply-To":"<20220630000251.31295-10-laurent.pinchart@ideasonboard.com>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"7bit","Subject":"Re: [libcamera-devel] [PATCH v2 09/12] gstreamer: Fix pads locking","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":"Nicolas Dufresne ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]