[{"id":23645,"web_url":"https://patchwork.libcamera.org/comment/23645/","msgid":"","date":"2022-06-28T13:05:52","subject":"Re: [libcamera-devel] [PATCH 09/13] gstreamer: Use dedicated lock\n\tfor request queues","submitter":{"id":31,"url":"https://patchwork.libcamera.org/api/people/31/","name":"Nicolas Dufresne","email":"nicolas.dufresne@collabora.com"},"content":"Hi Laurent,\n\nLe vendredi 24 juin 2022 à 02:22 +0300, Laurent Pinchart a écrit :\n> Add a new lock to the GstLibcameraSrcState class to protect the queued\n> and completed requests queues. This replaces the GstObject lock, and\n> minimizes the lock contention between the request completion handler and\n> the task run handler as the former must run as fast as possible.\n> \n> Signed-off-by: Laurent Pinchart \n> ---\n> src/gstreamer/gstlibcamerasrc.cpp | 39 ++++++++++++++++++++++---------\n> 1 file changed, 28 insertions(+), 11 deletions(-)\n> \n> diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp\n> index e30d45fa2223..b85ba39fb808 100644\n> --- a/src/gstreamer/gstlibcamerasrc.cpp\n> +++ b/src/gstreamer/gstlibcamerasrc.cpp\n> @@ -32,6 +32,8 @@\n> #include \n> #include \n> \n> +#include \n> +\n> #include \n> #include \n> #include \n> @@ -111,8 +113,13 @@ struct GstLibcameraSrcState {\n> \tstd::shared_ptr cam_;\n> \tstd::unique_ptr config_;\n> \tstd::vector srcpads_;\n> -\tstd::queue> queuedRequests_;\n> -\tstd::queue> completedRequests_;\n> +\n> +\tMutex lock_;\n\nI would love to see a comment explaining what this lock is for so that future\ncontributors knows when to take it, and when not.\n\nDid you know that GLib Mutex are a lot faster then pthread_mutex ? Might sounds\nsurprising, but they don't obey all of the POSIX requirement, they remains a\nperfect fit for streaming. I personally would have used these over a wrapper on\ntop of pthread, the change is fine though, I just thought of mentioning as you\nalready made pretty micro optimization removing few function call earlier in the\npatchset. IIRC this is in the 20x level of improvement, at least on Linux.\n\n> +\tstd::queue> queuedRequests_\n> +\t\tLIBCAMERA_TSA_GUARDED_BY(lock_);\n> +\tstd::queue> completedRequests_\n> +\t\tLIBCAMERA_TSA_GUARDED_BY(lock_);\n\nOf course, that would require extra effort to implement this \"guarded by\" thing.\nI'm just mentioning, not action needed. There is also GstAtomicQueue if (and\nonly if) contention isn't expected.\n\n> +\n> \tguint group_id_;\n> \n> \tvoid requestCompleted(Request *request);\n> @@ -155,12 +162,15 @@ GstStaticPadTemplate request_src_template = {\n> void\n> GstLibcameraSrcState::requestCompleted(Request *request)\n> {\n> -\tGLibLocker lock(GST_OBJECT(src_));\n> -\n> \tGST_DEBUG_OBJECT(src_, \"buffers are ready\");\n> \n> -\tstd::unique_ptr wrap = std::move(queuedRequests_.front());\n> -\tqueuedRequests_.pop();\n> +\tstd::unique_ptr wrap;\n> +\n> +\t{\n> +\t\tMutexLocker locker(lock_);\n> +\t\twrap = std::move(queuedRequests_.front());\n> +\t\tqueuedRequests_.pop();\n> +\t}\n> \n> \tg_return_if_fail(wrap->request_.get() == request);\n> \n> @@ -183,7 +193,10 @@ GstLibcameraSrcState::requestCompleted(Request *request)\n> \t\twrap->latency_ = sys_now - timestamp;\n> \t}\n> \n> -\tcompletedRequests_.push(std::move(wrap));\n> +\t{\n> +\t\tMutexLocker locker(lock_);\n> +\t\tcompletedRequests_.push(std::move(wrap));\n> +\t}\n> \n> \tgst_task_resume(src_->task);\n> }\n> @@ -289,16 +302,17 @@ gst_libcamera_src_task_run(gpointer user_data)\n> \t}\n> \n> \tif (wrap) {\n> -\t\tGLibLocker lock(GST_OBJECT(self));\n> \t\tGST_TRACE_OBJECT(self, \"Requesting buffers\");\n> \t\tstate->cam_->queueRequest(wrap->request_.get());\n> +\n> +\t\tMutexLocker locker(state->lock_);\n> \t\tstate->queuedRequests_.push(std::move(wrap));\n> \n> \t\t/* The RequestWrap will be deleted in the completion handler. */\n> \t}\n> \n> \t{\n> -\t\tGLibLocker lock(GST_OBJECT(self));\n> +\t\tMutexLocker locker(state->lock_);\n> \n> \t\tif (!state->completedRequests_.empty()) {\n> \t\t\twrap = std::move(state->completedRequests_.front());\n> @@ -358,7 +372,7 @@ gst_libcamera_src_task_run(gpointer user_data)\n> \t\tbool do_pause;\n> \n> \t\t{\n> -\t\t\tGLibLocker lock(GST_OBJECT(self));\n> +\t\t\tMutexLocker locker(state->lock_);\n> \t\t\tdo_pause = state->completedRequests_.empty();\n> \t\t}\n> \n> @@ -513,7 +527,10 @@ gst_libcamera_src_task_leave([[maybe_unused]] GstTask *task,\n> \n> \tstate->cam_->stop();\n> \n> -\tstate->completedRequests_ = {};\n> +\t{\n> +\t\tMutexLocker locker(state->lock_);\n> +\t\tstate->completedRequests_ = {};\n> +\t}\n> \n> \tfor (GstPad *srcpad : state->srcpads_)\n> \t\tgst_libcamera_pad_set_pool(srcpad, nullptr);\n\nDoes not solve concurrency between pads iteration and request/release pads, but\nthis wasn't the goal and was already broken.\n\nReviewed-by: Nicolas Dufresne ","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 BAB41BD808\n\tfor ;\n\tTue, 28 Jun 2022 13:06:05 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id BAC7165635;\n\tTue, 28 Jun 2022 15:06:04 +0200 (CEST)","from madras.collabora.co.uk (madras.collabora.co.uk\n\t[IPv6:2a00:1098:0:82:1000:25:2eeb:e5ab])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 500C26059D\n\tfor ;\n\tTue, 28 Jun 2022 15:06:02 +0200 (CEST)","from nicolas-tpx395.localdomain (192-222-136-102.qc.cable.ebox.net\n\t[192.222.136.102])\n\t(using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)\n\tkey-exchange X25519 server-signature RSA-PSS (4096 bits)\n\tserver-digest SHA256)\n\t(No client certificate requested) (Authenticated sender: nicolas)\n\tby madras.collabora.co.uk (Postfix) with ESMTPSA id 572196601856;\n\tTue, 28 Jun 2022 14:06:01 +0100 (BST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1656421564;\n\tbh=G90OYsQX6feSWczj0qYBluMbQCaM4s8N/MfO2rPxCck=;\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=ozsRbKuhkgayAlgbb/X3MBiJ+8kBjt81kmZ+hN3rJKECRdrbp9o7pocKcodBSZ6Gr\n\tt/7XNGAyvoOqPPYzg0j+kBzNyQN24xtZu651FypZyuzZtK2kKQdZ+ABmGRJCM+NXzv\n\tzdov1ntTeqFWLHdA8wwMgdxVAumma2slfK0uLB4HD60Dwth/wAKWMbvy97H1w+/xAV\n\tbctuBA64U3+3YOctA5pt6ZCeVgZAG4iDkR3h8QFsKDq9O/6OMq6cBEi53JjhmuG8Bw\n\t/9c4zJc7t6Y+jRcZ9+2MCNLok+Pk98y3QblPfGHUm7Mzm5atjSBqXeU+vfrnlDmHJq\n\tLowd5ztjN5O2Q==","v=1; a=rsa-sha256; c=relaxed/simple; d=collabora.com;\n\ts=mail; t=1656421561;\n\tbh=G90OYsQX6feSWczj0qYBluMbQCaM4s8N/MfO2rPxCck=;\n\th=Subject:From:To:Cc:Date:In-Reply-To:References:From;\n\tb=K/w5T0LAdsyEhrrbBizbnycDrfDemM0BUdOyWep9fSWrIAHZ3wry/HnWyuZ1xR+8X\n\tui8GOB5ZHZ287MHdZbXz7SO9IPSnxZo4zg62idmnguze81Q3YJ26ll602xp+PNv5Fz\n\teOvcqrcCoADi6snqBkRLDUkIA0DwXXU2KzmhTmiHzNLnD/tuFVNbZnrA4P90/PlL2V\n\tARuXDkwWiSe9mR2TXAxhOZEBln3T47MjQSXNdalMTx8GRw3jjAeyY9xDqvd3BS1oPq\n\tY5bieRXtqeBUDTFEXwC5kZ/cMtXhLi273K7lihm6Ks0BN72cu8ItRGQnyNGDIQRhyU\n\tZfzNLaiyvwy9g=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=collabora.com\n\theader.i=@collabora.com\n\theader.b=\"K/w5T0LA\"; dkim-atps=neutral","Message-ID":"","To":"Laurent Pinchart , \n\tlibcamera-devel@lists.libcamera.org","Date":"Tue, 28 Jun 2022 09:05:52 -0400","In-Reply-To":"<20220623232210.18742-10-laurent.pinchart@ideasonboard.com>","References":"<20220623232210.18742-1-laurent.pinchart@ideasonboard.com>\n\t<20220623232210.18742-10-laurent.pinchart@ideasonboard.com>","Content-Type":"text/plain; charset=\"UTF-8\"","Content-Transfer-Encoding":"quoted-printable","User-Agent":"Evolution 3.44.2 (3.44.2-1.fc36) ","MIME-Version":"1.0","Subject":"Re: [libcamera-devel] [PATCH 09/13] gstreamer: Use dedicated lock\n\tfor request queues","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":"Vedant Paranjape ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":23649,"web_url":"https://patchwork.libcamera.org/comment/23649/","msgid":"","date":"2022-06-28T13:45:44","subject":"Re: [libcamera-devel] [PATCH 09/13] gstreamer: Use dedicated lock\n\tfor request queues","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Nicolas,\n\nOn Tue, Jun 28, 2022 at 09:05:52AM -0400, Nicolas Dufresne wrote:\n> Le vendredi 24 juin 2022 à 02:22 +0300, Laurent Pinchart a écrit :\n> > Add a new lock to the GstLibcameraSrcState class to protect the queued\n> > and completed requests queues. This replaces the GstObject lock, and\n> > minimizes the lock contention between the request completion handler and\n> > the task run handler as the former must run as fast as possible.\n> > \n> > Signed-off-by: Laurent Pinchart \n> > ---\n> > src/gstreamer/gstlibcamerasrc.cpp | 39 ++++++++++++++++++++++---------\n> > 1 file changed, 28 insertions(+), 11 deletions(-)\n> > \n> > diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp\n> > index e30d45fa2223..b85ba39fb808 100644\n> > --- a/src/gstreamer/gstlibcamerasrc.cpp\n> > +++ b/src/gstreamer/gstlibcamerasrc.cpp\n> > @@ -32,6 +32,8 @@\n> > #include \n> > #include \n> > \n> > +#include \n> > +\n> > #include \n> > #include \n> > #include \n> > @@ -111,8 +113,13 @@ struct GstLibcameraSrcState {\n> > \tstd::shared_ptr cam_;\n> > \tstd::unique_ptr config_;\n> > \tstd::vector srcpads_;\n> > -\tstd::queue> queuedRequests_;\n> > -\tstd::queue> completedRequests_;\n> > +\n> > +\tMutex lock_;\n> \n> I would love to see a comment explaining what this lock is for so that future\n> contributors knows when to take it, and when not.\n> \n> Did you know that GLib Mutex are a lot faster then pthread_mutex ? Might sounds\n> surprising, but they don't obey all of the POSIX requirement, they remains a\n> perfect fit for streaming. I personally would have used these over a wrapper on\n> top of pthread, the change is fine though, I just thought of mentioning as you\n> already made pretty micro optimization removing few function call earlier in the\n> patchset. IIRC this is in the 20x level of improvement, at least on Linux.\n> \n> > +\tstd::queue> queuedRequests_\n> > +\t\tLIBCAMERA_TSA_GUARDED_BY(lock_);\n> > +\tstd::queue> completedRequests_\n> > +\t\tLIBCAMERA_TSA_GUARDED_BY(lock_);\n> \n> Of course, that would require extra effort to implement this \"guarded by\" thing.\n> I'm just mentioning, not action needed. There is also GstAtomicQueue if (and\n> only if) contention isn't expected.\n\nThe thread-safefy annotation is the reason why I picked the Mutex class,\nas it addresses your comment about documenting the data protected by the\nlock. It's also nice to get the compiler to validate the locking\npatterns. It's not strictly mandatory though.\n\nI wasn't aware of the differences in performance between the pthread\nmutex implementation and the GLib implementation, and I don't know how\nmuch difference it would make in practice here. Would you rather replace\nMutex + TSA with GLibMutex + a comment ? Or should this be done on top\nwhen/if needed ?\n\n> > +\n> > \tguint group_id_;\n> > \n> > \tvoid requestCompleted(Request *request);\n> > @@ -155,12 +162,15 @@ GstStaticPadTemplate request_src_template = {\n> > void\n> > GstLibcameraSrcState::requestCompleted(Request *request)\n> > {\n> > -\tGLibLocker lock(GST_OBJECT(src_));\n> > -\n> > \tGST_DEBUG_OBJECT(src_, \"buffers are ready\");\n> > \n> > -\tstd::unique_ptr wrap = std::move(queuedRequests_.front());\n> > -\tqueuedRequests_.pop();\n> > +\tstd::unique_ptr wrap;\n> > +\n> > +\t{\n> > +\t\tMutexLocker locker(lock_);\n> > +\t\twrap = std::move(queuedRequests_.front());\n> > +\t\tqueuedRequests_.pop();\n> > +\t}\n> > \n> > \tg_return_if_fail(wrap->request_.get() == request);\n> > \n> > @@ -183,7 +193,10 @@ GstLibcameraSrcState::requestCompleted(Request *request)\n> > \t\twrap->latency_ = sys_now - timestamp;\n> > \t}\n> > \n> > -\tcompletedRequests_.push(std::move(wrap));\n> > +\t{\n> > +\t\tMutexLocker locker(lock_);\n> > +\t\tcompletedRequests_.push(std::move(wrap));\n> > +\t}\n> > \n> > \tgst_task_resume(src_->task);\n> > }\n> > @@ -289,16 +302,17 @@ gst_libcamera_src_task_run(gpointer user_data)\n> > \t}\n> > \n> > \tif (wrap) {\n> > -\t\tGLibLocker lock(GST_OBJECT(self));\n> > \t\tGST_TRACE_OBJECT(self, \"Requesting buffers\");\n> > \t\tstate->cam_->queueRequest(wrap->request_.get());\n> > +\n> > +\t\tMutexLocker locker(state->lock_);\n> > \t\tstate->queuedRequests_.push(std::move(wrap));\n> > \n> > \t\t/* The RequestWrap will be deleted in the completion handler. */\n> > \t}\n> > \n> > \t{\n> > -\t\tGLibLocker lock(GST_OBJECT(self));\n> > +\t\tMutexLocker locker(state->lock_);\n> > \n> > \t\tif (!state->completedRequests_.empty()) {\n> > \t\t\twrap = std::move(state->completedRequests_.front());\n> > @@ -358,7 +372,7 @@ gst_libcamera_src_task_run(gpointer user_data)\n> > \t\tbool do_pause;\n> > \n> > \t\t{\n> > -\t\t\tGLibLocker lock(GST_OBJECT(self));\n> > +\t\t\tMutexLocker locker(state->lock_);\n> > \t\t\tdo_pause = state->completedRequests_.empty();\n> > \t\t}\n> > \n> > @@ -513,7 +527,10 @@ gst_libcamera_src_task_leave([[maybe_unused]] GstTask *task,\n> > \n> > \tstate->cam_->stop();\n> > \n> > -\tstate->completedRequests_ = {};\n> > +\t{\n> > +\t\tMutexLocker locker(state->lock_);\n> > +\t\tstate->completedRequests_ = {};\n> > +\t}\n> > \n> > \tfor (GstPad *srcpad : state->srcpads_)\n> > \t\tgst_libcamera_pad_set_pool(srcpad, nullptr);\n> \n> Does not solve concurrency between pads iteration and request/release pads, but\n> this wasn't the goal and was already broken.\n\nHopefully the next patches do :-)\n\n> Reviewed-by: Nicolas Dufresne ","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 84544BD808\n\tfor ;\n\tTue, 28 Jun 2022 13:46:06 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A580C65635;\n\tTue, 28 Jun 2022 15:46:05 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 596C86059D\n\tfor ;\n\tTue, 28 Jun 2022 15:46:04 +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 A59CA4A8;\n\tTue, 28 Jun 2022 15:46:03 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1656423965;\n\tbh=WIB4vhkjcaDJ30jBTrA1sTmFyE5YjxkFNuPMqZZ7vy0=;\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=Q+7s83XsrHnuPsTFEe0tJlMHDvOMV/gxflVKL9m0/Fcd1alrwvGOqvtg95ssTvB3P\n\t+ySsE73KHjgzJh/a618ckilDslfsFfObvwRramWkyyS/41lygZ/a1qaAHvdpr//3S9\n\t/bFURC6AjN6Kq1Q3TL66zY2rzT3jrK5S/iPwnHmIE3da8tR++Y0vmt/15J97nDR4gi\n\th9jKfqjslP3wciZaVRNSQ3umRbe2RgPBhCMxmy/AZ1+IJXVxF9uDWVlnS+88oN2Trs\n\tANRFSn3GPVBvh5mdTFaR2JTZq/hCb1MopovOm3UHkv8TrcBw1MGVW+SXMd5PJDpr1c\n\t6QecqMRO6hfDA==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1656423963;\n\tbh=WIB4vhkjcaDJ30jBTrA1sTmFyE5YjxkFNuPMqZZ7vy0=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=YKDYeFcnA06Ab/87T+h9cmvkwO/iJyYz5e7o7NqHkC7rBSFqULogtKXyzM8FHOdR4\n\tx/2ZxqS/dZw7aTweI9UvBlQYpxG6/3FY06AZ9jKTm9VrRJHBCOZZix4rc8Ek57H1Rb\n\tDD11JsDTs2JITW2ZXZvFAkr6sohB/lwyA4QSYX9M="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"YKDYeFcn\"; dkim-atps=neutral","Date":"Tue, 28 Jun 2022 16:45:44 +0300","To":"Nicolas Dufresne ","Message-ID":"","References":"<20220623232210.18742-1-laurent.pinchart@ideasonboard.com>\n\t<20220623232210.18742-10-laurent.pinchart@ideasonboard.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 09/13] gstreamer: Use dedicated lock\n\tfor request queues","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":23651,"web_url":"https://patchwork.libcamera.org/comment/23651/","msgid":"<842b0af1ed43c380bf683fa32f78d8c4122312fa.camel@collabora.com>","date":"2022-06-28T13:53:56","subject":"Re: [libcamera-devel] [PATCH 09/13] gstreamer: Use dedicated lock\n\tfor request queues","submitter":{"id":31,"url":"https://patchwork.libcamera.org/api/people/31/","name":"Nicolas Dufresne","email":"nicolas.dufresne@collabora.com"},"content":"Le mardi 28 juin 2022 à 16:45 +0300, Laurent Pinchart a écrit :\n> Hi Nicolas,\n> \n> On Tue, Jun 28, 2022 at 09:05:52AM -0400, Nicolas Dufresne wrote:\n> > Le vendredi 24 juin 2022 à 02:22 +0300, Laurent Pinchart a écrit :\n> > > Add a new lock to the GstLibcameraSrcState class to protect the queued\n> > > and completed requests queues. This replaces the GstObject lock, and\n> > > minimizes the lock contention between the request completion handler and\n> > > the task run handler as the former must run as fast as possible.\n> > > \n> > > Signed-off-by: Laurent Pinchart \n> > > ---\n> > > src/gstreamer/gstlibcamerasrc.cpp | 39 ++++++++++++++++++++++---------\n> > > 1 file changed, 28 insertions(+), 11 deletions(-)\n> > > \n> > > diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp\n> > > index e30d45fa2223..b85ba39fb808 100644\n> > > --- a/src/gstreamer/gstlibcamerasrc.cpp\n> > > +++ b/src/gstreamer/gstlibcamerasrc.cpp\n> > > @@ -32,6 +32,8 @@\n> > > #include \n> > > #include \n> > > \n> > > +#include \n> > > +\n> > > #include \n> > > #include \n> > > #include \n> > > @@ -111,8 +113,13 @@ struct GstLibcameraSrcState {\n> > > \tstd::shared_ptr cam_;\n> > > \tstd::unique_ptr config_;\n> > > \tstd::vector srcpads_;\n> > > -\tstd::queue> queuedRequests_;\n> > > -\tstd::queue> completedRequests_;\n> > > +\n> > > +\tMutex lock_;\n> > \n> > I would love to see a comment explaining what this lock is for so that future\n> > contributors knows when to take it, and when not.\n> > \n> > Did you know that GLib Mutex are a lot faster then pthread_mutex ? Might sounds\n> > surprising, but they don't obey all of the POSIX requirement, they remains a\n> > perfect fit for streaming. I personally would have used these over a wrapper on\n> > top of pthread, the change is fine though, I just thought of mentioning as you\n> > already made pretty micro optimization removing few function call earlier in the\n> > patchset. IIRC this is in the 20x level of improvement, at least on Linux.\n> > \n> > > +\tstd::queue> queuedRequests_\n> > > +\t\tLIBCAMERA_TSA_GUARDED_BY(lock_);\n> > > +\tstd::queue> completedRequests_\n> > > +\t\tLIBCAMERA_TSA_GUARDED_BY(lock_);\n> > \n> > Of course, that would require extra effort to implement this \"guarded by\" thing.\n> > I'm just mentioning, not action needed. There is also GstAtomicQueue if (and\n> > only if) contention isn't expected.\n> \n> The thread-safefy annotation is the reason why I picked the Mutex class,\n> as it addresses your comment about documenting the data protected by the\n> lock. It's also nice to get the compiler to validate the locking\n> patterns. It's not strictly mandatory though.\n> \n> I wasn't aware of the differences in performance between the pthread\n> mutex implementation and the GLib implementation, and I don't know how\n> much difference it would make in practice here. Would you rather replace\n> Mutex + TSA with GLibMutex + a comment ? Or should this be done on top\n> when/if needed ?\n\nOnly if needed. It does matter for lower latency audio, but for video which is\npretty low rate it will rarely make a big difference. This comment was just to\nkeep you inform that GMutex is not a pthread_mutex_t (and not POSIX compliant,\nwhich is the reason pthread under perform). It might matter when we start\nhandling frame burst and highly concurrent streaming (e.g. 100 or more streams).\n\n> \n> > > +\n> > > \tguint group_id_;\n> > > \n> > > \tvoid requestCompleted(Request *request);\n> > > @@ -155,12 +162,15 @@ GstStaticPadTemplate request_src_template = {\n> > > void\n> > > GstLibcameraSrcState::requestCompleted(Request *request)\n> > > {\n> > > -\tGLibLocker lock(GST_OBJECT(src_));\n> > > -\n> > > \tGST_DEBUG_OBJECT(src_, \"buffers are ready\");\n> > > \n> > > -\tstd::unique_ptr wrap = std::move(queuedRequests_.front());\n> > > -\tqueuedRequests_.pop();\n> > > +\tstd::unique_ptr wrap;\n> > > +\n> > > +\t{\n> > > +\t\tMutexLocker locker(lock_);\n> > > +\t\twrap = std::move(queuedRequests_.front());\n> > > +\t\tqueuedRequests_.pop();\n> > > +\t}\n> > > \n> > > \tg_return_if_fail(wrap->request_.get() == request);\n> > > \n> > > @@ -183,7 +193,10 @@ GstLibcameraSrcState::requestCompleted(Request *request)\n> > > \t\twrap->latency_ = sys_now - timestamp;\n> > > \t}\n> > > \n> > > -\tcompletedRequests_.push(std::move(wrap));\n> > > +\t{\n> > > +\t\tMutexLocker locker(lock_);\n> > > +\t\tcompletedRequests_.push(std::move(wrap));\n> > > +\t}\n> > > \n> > > \tgst_task_resume(src_->task);\n> > > }\n> > > @@ -289,16 +302,17 @@ gst_libcamera_src_task_run(gpointer user_data)\n> > > \t}\n> > > \n> > > \tif (wrap) {\n> > > -\t\tGLibLocker lock(GST_OBJECT(self));\n> > > \t\tGST_TRACE_OBJECT(self, \"Requesting buffers\");\n> > > \t\tstate->cam_->queueRequest(wrap->request_.get());\n> > > +\n> > > +\t\tMutexLocker locker(state->lock_);\n> > > \t\tstate->queuedRequests_.push(std::move(wrap));\n> > > \n> > > \t\t/* The RequestWrap will be deleted in the completion handler. */\n> > > \t}\n> > > \n> > > \t{\n> > > -\t\tGLibLocker lock(GST_OBJECT(self));\n> > > +\t\tMutexLocker locker(state->lock_);\n> > > \n> > > \t\tif (!state->completedRequests_.empty()) {\n> > > \t\t\twrap = std::move(state->completedRequests_.front());\n> > > @@ -358,7 +372,7 @@ gst_libcamera_src_task_run(gpointer user_data)\n> > > \t\tbool do_pause;\n> > > \n> > > \t\t{\n> > > -\t\t\tGLibLocker lock(GST_OBJECT(self));\n> > > +\t\t\tMutexLocker locker(state->lock_);\n> > > \t\t\tdo_pause = state->completedRequests_.empty();\n> > > \t\t}\n> > > \n> > > @@ -513,7 +527,10 @@ gst_libcamera_src_task_leave([[maybe_unused]] GstTask *task,\n> > > \n> > > \tstate->cam_->stop();\n> > > \n> > > -\tstate->completedRequests_ = {};\n> > > +\t{\n> > > +\t\tMutexLocker locker(state->lock_);\n> > > +\t\tstate->completedRequests_ = {};\n> > > +\t}\n> > > \n> > > \tfor (GstPad *srcpad : state->srcpads_)\n> > > \t\tgst_libcamera_pad_set_pool(srcpad, nullptr);\n> > \n> > Does not solve concurrency between pads iteration and request/release pads, but\n> > this wasn't the goal and was already broken.\n> \n> Hopefully the next patches do :-)\n> \n> > Reviewed-by: Nicolas Dufresne \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 B8037BD808\n\tfor ;\n\tTue, 28 Jun 2022 13:54:08 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id DE38565635;\n\tTue, 28 Jun 2022 15:54:07 +0200 (CEST)","from madras.collabora.co.uk (madras.collabora.co.uk\n\t[IPv6:2a00:1098:0:82:1000:25:2eeb:e5ab])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 161876059D\n\tfor ;\n\tTue, 28 Jun 2022 15:54:06 +0200 (CEST)","from nicolas-tpx395.localdomain (192-222-136-102.qc.cable.ebox.net\n\t[192.222.136.102])\n\t(using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)\n\tkey-exchange X25519 server-signature RSA-PSS (4096 bits))\n\t(No client certificate requested) (Authenticated sender: nicolas)\n\tby madras.collabora.co.uk (Postfix) with ESMTPSA id 38CC966015C8;\n\tTue, 28 Jun 2022 14:54:05 +0100 (BST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1656424447;\n\tbh=bIIcYGIotgTOd4N9eHasFHBHjwq+89cYQiSwYLZt0B8=;\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=0pMSBkCwdQYiJo0o9sBNjcOG/AgJk2VchvJNxdEcwocitsKYRwvUux7x/Woqbt0nK\n\t/xWYM5FvVBA1OeZqcMU7MlHeb5FqS0+deWvzdoiBknRMvFCQ2PG7fr33UzGOpM/eZo\n\tjJJm9zgbhsziVK1Ks7U4hhr2DDiHvoP16p5CAgESrDZfkNMT4/V8TFEXegRcMv944f\n\tPxOV1/gkvBGdgrIH5+wWNaaI0SeJ/zm6qSZxzB1lU/a28s4PFfLDJLf8MpDXxwEikp\n\tKRoRO9tn1HqFXdGua8raSKORYnlDMPtuiL4EILhajc3cWiK4zREnTthP7omLIFEkLN\n\tsPXwUx2Tveehg==","v=1; a=rsa-sha256; c=relaxed/simple; d=collabora.com;\n\ts=mail; t=1656424445;\n\tbh=bIIcYGIotgTOd4N9eHasFHBHjwq+89cYQiSwYLZt0B8=;\n\th=Subject:From:To:Cc:Date:In-Reply-To:References:From;\n\tb=i1Ow7fGA9SBHPzqAlAUnSOCQq2ruh2ATFrAbzBH7sHSZqrx5S3q8oEbmj8CoDYf9F\n\t0qkggSN0hTNUPJs66Sv2j0TGJbnTwXPlaGGyVe1tq+wLBx7UpTNrKEnuPJyIUPehi1\n\tWg67lf3QJ5m7Hj2AZYr4sw9481Cb72EkQPpi69ihPPPxOqzwx4mVSi1CjeU4uB1Oy0\n\tm5jXYXyUHxxxNf6n89BN/gif2cajtrk+7pSq81U6mV9h51/9XGnru/ycOdSqyIBIX4\n\tcDElzSEII81TWUJJOJRc9+bvug9t+TKT273h/7ykDu8aw1MQSUvdSv/Obow8iDSdlr\n\tCgwGabB3o9fHw=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=collabora.com\n\theader.i=@collabora.com\n\theader.b=\"i1Ow7fGA\"; dkim-atps=neutral","Message-ID":"<842b0af1ed43c380bf683fa32f78d8c4122312fa.camel@collabora.com>","To":"Laurent Pinchart ","Date":"Tue, 28 Jun 2022 09:53:56 -0400","In-Reply-To":"","References":"<20220623232210.18742-1-laurent.pinchart@ideasonboard.com>\n\t<20220623232210.18742-10-laurent.pinchart@ideasonboard.com>\n\t\n\t","Content-Type":"text/plain; charset=\"UTF-8\"","Content-Transfer-Encoding":"quoted-printable","User-Agent":"Evolution 3.44.2 (3.44.2-1.fc36) ","MIME-Version":"1.0","Subject":"Re: [libcamera-devel] [PATCH 09/13] gstreamer: Use dedicated lock\n\tfor request queues","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\" "}}]