[{"id":33545,"web_url":"https://patchwork.libcamera.org/comment/33545/","msgid":"","date":"2025-03-03T14:31:30","subject":"Re: [PATCH v1] libcamera: camera_manager: Do not emit signals while\n\tholding lock","submitter":{"id":117,"url":"https://patchwork.libcamera.org/api/people/117/","name":"Cheng-Hao Yang","email":"chenghaoyang@chromium.org"},"content":"Hi Barnabás,\n\nThank you for the patch. It makes a lot of sense to me, only that I'm\nnot sure if `std::move`s are necessary. It probably saves new\ninstances of shared_ptr though.\n\nReviewed-by: Harvey Yang \n\nBR,\nHarvey\n\nOn Mon, Mar 3, 2025 at 10:20 PM Barnabás Pőcze\n wrote:\n>\n> Both `CameraManager::Private::{add,remove}Camera()` emit the\n> `camera{Added,Removed}` signals, respectively, while holding the\n> lock protecting the list of cameras.\n>\n> This is problematic because if a callback tries to call `cameras()`,\n> then the same (non-recursive) lock would be locked again.\n>\n> Furthermore, there is no real need to hold the lock while user code\n> is running, so release the lock as soon as possible.\n>\n> Signed-off-by: Barnabás Pőcze \n> ---\n> src/libcamera/camera_manager.cpp | 16 +++++++++-------\n> 1 file changed, 9 insertions(+), 7 deletions(-)\n>\n> diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp\n> index 87e6717ec..d728ac44a 100644\n> --- a/src/libcamera/camera_manager.cpp\n> +++ b/src/libcamera/camera_manager.cpp\n> @@ -202,6 +202,7 @@ void CameraManager::Private::addCamera(std::shared_ptr camera)\n> {\n> ASSERT(Thread::current() == this);\n>\n> +{\n> MutexLocker locker(mutex_);\n>\n> for (const std::shared_ptr &c : cameras_) {\n> @@ -213,13 +214,12 @@ void CameraManager::Private::addCamera(std::shared_ptr camera)\n> }\n> }\n>\n> - cameras_.push_back(std::move(camera));\n> -\n> - unsigned int index = cameras_.size() - 1;\n> + cameras_.push_back(camera);\n> +}\n>\n> /* Report the addition to the public signal */\n> CameraManager *const o = LIBCAMERA_O_PTR();\n> - o->cameraAdded.emit(cameras_[index]);\n> + o->cameraAdded.emit(std::move(camera));\n> }\n>\n> /**\n> @@ -236,6 +236,7 @@ void CameraManager::Private::removeCamera(std::shared_ptr camera)\n> {\n> ASSERT(Thread::current() == this);\n>\n> +{\n> MutexLocker locker(mutex_);\n>\n> auto iter = std::find_if(cameras_.begin(), cameras_.end(),\n> @@ -245,14 +246,15 @@ void CameraManager::Private::removeCamera(std::shared_ptr camera)\n> if (iter == cameras_.end())\n> return;\n>\n> + cameras_.erase(iter);\n> +}\n> +\n> LOG(Camera, Debug)\n> << \"Unregistering camera '\" << camera->id() << \"'\";\n>\n> - cameras_.erase(iter);\n> -\n> /* Report the removal to the public signal */\n> CameraManager *const o = LIBCAMERA_O_PTR();\n> - o->cameraRemoved.emit(camera);\n> + o->cameraRemoved.emit(std::move(camera));\n> }\n>\n> /**\n> --\n> 2.48.1\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 A7A19C3257\n\tfor ;\n\tMon, 3 Mar 2025 14:31:44 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B6B8A687EB;\n\tMon, 3 Mar 2025 15:31:43 +0100 (CET)","from mail-lj1-x22b.google.com (mail-lj1-x22b.google.com\n\t[IPv6:2a00:1450:4864:20::22b])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 99FEE68772\n\tfor ;\n\tMon, 3 Mar 2025 15:31:42 +0100 (CET)","by mail-lj1-x22b.google.com with SMTP id\n\t38308e7fff4ca-30613802a04so46194441fa.2\n\tfor ;\n\tMon, 03 Mar 2025 06:31:42 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"QiK3wFEM\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=chromium.org; s=google; t=1741012302; x=1741617102;\n\tdarn=lists.libcamera.org; \n\th=content-transfer-encoding:cc:to:subject:message-id:date:from\n\t:in-reply-to:references:mime-version:from:to:cc:subject:date\n\t:message-id:reply-to;\n\tbh=8NPaOCf0XX1IhWFKL+GS0iK5mUprpbO3Yz2S9hq18iw=;\n\tb=QiK3wFEM5mgBbB4PrcJIi/wjfktA/RtEPP5xxCLHL/FdbiIgV0Bv4BdE7B3IjwOuMI\n\tavSGoDTlml/tHV9DH5yPtE8rJkjrrTfPgGXnGJzmU5M6fenwAJmAuU9vppTSI1Smt5nV\n\txN7wVD4+9rIuj/UiQqGdurHQ5NECUub+ymbIE=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1741012302; x=1741617102;\n\th=content-transfer-encoding:cc:to:subject:message-id:date:from\n\t:in-reply-to:references:mime-version:x-gm-message-state:from:to:cc\n\t:subject:date:message-id:reply-to;\n\tbh=8NPaOCf0XX1IhWFKL+GS0iK5mUprpbO3Yz2S9hq18iw=;\n\tb=Yv0Ku4z2n9RfqbREymFr/BSdi58SRnrQLboPTnlUJSIs7BPVyTBkW+iyv/KFW8I+Dp\n\t5wfyt3x3jGfMggvmFTfLrulfFJov6mSebaz6ULM15Jnb1UvwFHP/Nr4Zuqc++tTfhP1R\n\tNp2/8bw6YY78IV/v4xW+/BMHcjfsinr7ZbtV3KS7NJ8+dlVAspA0DjGuOutdZuDlXLDq\n\t8uYU6ba2rm06puy7mH8ViUByzO9KJB1LNGZutVzcImkjEwEsyk9nbpl+Fv5Coud6Gteu\n\tvT6DmzoEA+booEP+mRuMTFozMYvjx9QbAHLsMeSQpd/14YLvsISzojLO1v21YJRVsWJr\n\tZJSQ==","X-Gm-Message-State":"AOJu0Yz6kiYu1LuM1e+40VHHMp63JULmd3KqMqWVXTJ0GjQG9CwNlXhC\n\t7OZwdNOw9mLEFg8dCDx04vXCrMUoyG+uJRVLKigMcpUPs/JkDVelpDazdBGUG2xYqCris6WOCkx\n\ttzFjnavXjr2xz4QHy6YjXu97kEuffgZCGsOODenjcSGLAoQkImg==","X-Gm-Gg":"ASbGnctNWmSIm2Xqr9JJzuLnF34QxUHkQxVQBKTNeBBYPccwgLoHcLeI168HidHYY/t\n\tOSCyqU/e4RJuX85OFeTlcrgfDUeX6992mu3b55XrnYJDs0nC2PKOaz1Jjsaq8nN+Hmp66LZ73o1\n\tWvOQ5XtKL1ZS2AYO+gpVXOFP1NUNU=","X-Google-Smtp-Source":"AGHT+IEsQ2WU9qJXTe86gE4q8I5SPE0xn/4U7ah37eCyhJrlx8u/b7VSMw0S3VV2KD6AWv1tfuJNOkH3orjw96ehea4=","X-Received":"by 2002:a2e:b8c3:0:b0:30b:c91d:35cd with SMTP id\n\t38308e7fff4ca-30bc91d3c83mr1794791fa.11.1741012301539;\n\tMon, 03 Mar 2025 06:31:41 -0800 (PST)","MIME-Version":"1.0","References":"<20250303142010.714280-1-barnabas.pocze@ideasonboard.com>","In-Reply-To":"<20250303142010.714280-1-barnabas.pocze@ideasonboard.com>","From":"Cheng-Hao Yang ","Date":"Mon, 3 Mar 2025 22:31:30 +0800","X-Gm-Features":"AQ5f1Jqi_JJv5Pmb4Q56kGW_SZfIEEBhfY_ror6bN0QEl2hDXXT0r_Y_rVxC4do","Message-ID":"","Subject":"Re: [PATCH v1] libcamera: camera_manager: Do not emit signals while\n\tholding lock","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"UTF-8\"","Content-Transfer-Encoding":"quoted-printable","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":33567,"web_url":"https://patchwork.libcamera.org/comment/33567/","msgid":"<174108344751.2914008.16937782545444148964@ping.linuxembedded.co.uk>","date":"2025-03-04T10:17:27","subject":"Re: [PATCH v1] libcamera: camera_manager: Do not emit signals while\n\tholding lock","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Barnabás Pőcze (2025-03-03 14:20:10)\n> Both `CameraManager::Private::{add,remove}Camera()` emit the\n> `camera{Added,Removed}` signals, respectively, while holding the\n> lock protecting the list of cameras.\n> \n> This is problematic because if a callback tries to call `cameras()`,\n> then the same (non-recursive) lock would be locked again.\n> \n> Furthermore, there is no real need to hold the lock while user code\n> is running, so release the lock as soon as possible.\n> \n\nThis all sounds quite reasonable to me, so only a stylistic comment\nbelow...\n\n> Signed-off-by: Barnabás Pőcze \n> ---\n> src/libcamera/camera_manager.cpp | 16 +++++++++-------\n> 1 file changed, 9 insertions(+), 7 deletions(-)\n> \n> diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp\n> index 87e6717ec..d728ac44a 100644\n> --- a/src/libcamera/camera_manager.cpp\n> +++ b/src/libcamera/camera_manager.cpp\n> @@ -202,6 +202,7 @@ void CameraManager::Private::addCamera(std::shared_ptr camera)\n> {\n> ASSERT(Thread::current() == this);\n> \n> +{\n\nHaving the inner scope at the leftmost layer is my only niggle here. I\nthink I would suggest that the MutexLocker scope should be indented\n\n> MutexLocker locker(mutex_);\n> \n> for (const std::shared_ptr &c : cameras_) {\n> @@ -213,13 +214,12 @@ void CameraManager::Private::addCamera(std::shared_ptr camera)\n> }\n> }\n> \n> - cameras_.push_back(std::move(camera));\n> -\n> - unsigned int index = cameras_.size() - 1;\n> + cameras_.push_back(camera);\n> +}\n> \n> /* Report the addition to the public signal */\n> CameraManager *const o = LIBCAMERA_O_PTR();\n> - o->cameraAdded.emit(cameras_[index]);\n> + o->cameraAdded.emit(std::move(camera));\n\nIt feels a bit weird that we move the camera to the signal... It's a\nshared pointer, so I guess it doesn't matter ... it's not actually\ntransferring ownership to the transient signal emitter ?\n\nI guess this is because it's not used locally after this so it avoids a\ncopy of the shared_ptr? \n\n> }\n> \n> /**\n> @@ -236,6 +236,7 @@ void CameraManager::Private::removeCamera(std::shared_ptr camera)\n> {\n> ASSERT(Thread::current() == this);\n> \n> +{\n> MutexLocker locker(mutex_);\n> \n> auto iter = std::find_if(cameras_.begin(), cameras_.end(),\n> @@ -245,14 +246,15 @@ void CameraManager::Private::removeCamera(std::shared_ptr camera)\n> if (iter == cameras_.end())\n> return;\n> \n> + cameras_.erase(iter);\n> +}\n> +\n\nSame thoughts on the indent and std::move usage below too.\n\n> LOG(Camera, Debug)\n> << \"Unregistering camera '\" << camera->id() << \"'\";\n> \n> - cameras_.erase(iter);\n> -\n> /* Report the removal to the public signal */\n> CameraManager *const o = LIBCAMERA_O_PTR();\n> - o->cameraRemoved.emit(camera);\n> + o->cameraRemoved.emit(std::move(camera));\n> }\n> \n> /**\n> -- \n> 2.48.1\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 1DD07C3257\n\tfor ;\n\tTue, 4 Mar 2025 10:17:33 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 38C2B687FA;\n\tTue, 4 Mar 2025 11:17:32 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 638A468754\n\tfor ;\n\tTue, 4 Mar 2025 11:17:30 +0100 (CET)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust6594.18-1.cable.virginm.net [86.31.185.195])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 47AD71083;\n\tTue, 4 Mar 2025 11:15:58 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"LU8IPghw\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1741083358;\n\tbh=fojRlkXNibeI/tADWJjJ9hPq8BqFraBKaq3FCHZgD7Y=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=LU8IPghw8BT6OTiNviXv0Xs3Ps7OJvvhM5VyRgSdHl3tUJwA72tDDFlivUyizblIS\n\tcaAT0ZXDQxo0e/PkXRwx/EdRT7SuL6njKcjoIGc1T/7hK8UyBitfq7jlEw0X9SN3MH\n\tnzBmrbP+lOU+SOSkJ3d58W7go8tFvXr4TWpq2kE4=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20250303142010.714280-1-barnabas.pocze@ideasonboard.com>","References":"<20250303142010.714280-1-barnabas.pocze@ideasonboard.com>","Subject":"Re: [PATCH v1] libcamera: camera_manager: Do not emit signals while\n\tholding lock","From":"Kieran Bingham ","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Tue, 04 Mar 2025 10:17:27 +0000","Message-ID":"<174108344751.2914008.16937782545444148964@ping.linuxembedded.co.uk>","User-Agent":"alot/0.10","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":33573,"web_url":"https://patchwork.libcamera.org/comment/33573/","msgid":"<6ec0e2a9-1dd3-4cf4-a26c-6e8b555991d2@ideasonboard.com>","date":"2025-03-04T13:34:11","subject":"Re: [PATCH v1] libcamera: camera_manager: Do not emit signals while\n\tholding lock","submitter":{"id":216,"url":"https://patchwork.libcamera.org/api/people/216/","name":"Barnabás Pőcze","email":"barnabas.pocze@ideasonboard.com"},"content":"Hi\n\n\n2025. 03. 03. 15:31 keltezéssel, Cheng-Hao Yang írta:\n> Hi Barnabás,\n> \n> Thank you for the patch. It makes a lot of sense to me, only that I'm\n> not sure if `std::move`s are necessary. It probably saves new\n> instances of shared_ptr though.\n\nThey are not necessary, but I thought I'll make that change\nif I am already here.\n\n\nRegards,\nBarnabás Pőcze\n\n> \n> Reviewed-by: Harvey Yang \n> \n> BR,\n> Harvey\n> \n> On Mon, Mar 3, 2025 at 10:20 PM Barnabás Pőcze\n> wrote:\n>>\n>> Both `CameraManager::Private::{add,remove}Camera()` emit the\n>> `camera{Added,Removed}` signals, respectively, while holding the\n>> lock protecting the list of cameras.\n>>\n>> This is problematic because if a callback tries to call `cameras()`,\n>> then the same (non-recursive) lock would be locked again.\n>>\n>> Furthermore, there is no real need to hold the lock while user code\n>> is running, so release the lock as soon as possible.\n>>\n>> Signed-off-by: Barnabás Pőcze \n>> ---\n>> src/libcamera/camera_manager.cpp | 16 +++++++++-------\n>> 1 file changed, 9 insertions(+), 7 deletions(-)\n>>\n>> diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp\n>> index 87e6717ec..d728ac44a 100644\n>> --- a/src/libcamera/camera_manager.cpp\n>> +++ b/src/libcamera/camera_manager.cpp\n>> @@ -202,6 +202,7 @@ void CameraManager::Private::addCamera(std::shared_ptr camera)\n>> {\n>> ASSERT(Thread::current() == this);\n>>\n>> +{\n>> MutexLocker locker(mutex_);\n>>\n>> for (const std::shared_ptr &c : cameras_) {\n>> @@ -213,13 +214,12 @@ void CameraManager::Private::addCamera(std::shared_ptr camera)\n>> }\n>> }\n>>\n>> - cameras_.push_back(std::move(camera));\n>> -\n>> - unsigned int index = cameras_.size() - 1;\n>> + cameras_.push_back(camera);\n>> +}\n>>\n>> /* Report the addition to the public signal */\n>> CameraManager *const o = LIBCAMERA_O_PTR();\n>> - o->cameraAdded.emit(cameras_[index]);\n>> + o->cameraAdded.emit(std::move(camera));\n>> }\n>>\n>> /**\n>> @@ -236,6 +236,7 @@ void CameraManager::Private::removeCamera(std::shared_ptr camera)\n>> {\n>> ASSERT(Thread::current() == this);\n>>\n>> +{\n>> MutexLocker locker(mutex_);\n>>\n>> auto iter = std::find_if(cameras_.begin(), cameras_.end(),\n>> @@ -245,14 +246,15 @@ void CameraManager::Private::removeCamera(std::shared_ptr camera)\n>> if (iter == cameras_.end())\n>> return;\n>>\n>> + cameras_.erase(iter);\n>> +}\n>> +\n>> LOG(Camera, Debug)\n>> << \"Unregistering camera '\" << camera->id() << \"'\";\n>>\n>> - cameras_.erase(iter);\n>> -\n>> /* Report the removal to the public signal */\n>> CameraManager *const o = LIBCAMERA_O_PTR();\n>> - o->cameraRemoved.emit(camera);\n>> + o->cameraRemoved.emit(std::move(camera));\n>> }\n>>\n>> /**\n>> --\n>> 2.48.1\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 9F26AC3257\n\tfor ;\n\tTue, 4 Mar 2025 13:34:19 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4A5B668777;\n\tTue, 4 Mar 2025 14:34:18 +0100 (CET)","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 BEB7C68755\n\tfor ;\n\tTue, 4 Mar 2025 14:34:15 +0100 (CET)","from [192.168.33.3] (185.221.143.4.nat.pool.zt.hu [185.221.143.4])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 65F018FA;\n\tTue, 4 Mar 2025 14:32:43 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"TrupEuh7\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1741095163;\n\tbh=aQg/k+HfpHcmYj6jTyDKeIGQBuzrUaWT/2r/zPUk+eQ=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=TrupEuh7nFmdpVP0EBYUhbyTeU+w0DLQ/JMZkNTxum+81rtSGkYbe3nQPGqbuilET\n\tv+abO1BzqFdPOnM4gjzln3LCEyWHshH/nftfK6OFNHKceTO+u+1uL0x8ls73693XGE\n\tML4PcHeS2ypvMc1NfPNZdCdTYWaJxfxzz/0ItIEE=","Message-ID":"<6ec0e2a9-1dd3-4cf4-a26c-6e8b555991d2@ideasonboard.com>","Date":"Tue, 4 Mar 2025 14:34:11 +0100","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH v1] libcamera: camera_manager: Do not emit signals while\n\tholding lock","To":"Cheng-Hao Yang ","Cc":"libcamera-devel@lists.libcamera.org","References":"<20250303142010.714280-1-barnabas.pocze@ideasonboard.com>\n\t","From":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Content-Language":"en-US, hu-HU","In-Reply-To":"","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":33574,"web_url":"https://patchwork.libcamera.org/comment/33574/","msgid":"<5b2c364a-911e-49ec-9bd4-8663779aee6e@ideasonboard.com>","date":"2025-03-04T13:34:51","subject":"Re: [PATCH v1] libcamera: camera_manager: Do not emit signals while\n\tholding lock","submitter":{"id":216,"url":"https://patchwork.libcamera.org/api/people/216/","name":"Barnabás Pőcze","email":"barnabas.pocze@ideasonboard.com"},"content":"Hi\n\n\n2025. 03. 04. 11:17 keltezéssel, Kieran Bingham írta:\n> Quoting Barnabás Pőcze (2025-03-03 14:20:10)\n>> Both `CameraManager::Private::{add,remove}Camera()` emit the\n>> `camera{Added,Removed}` signals, respectively, while holding the\n>> lock protecting the list of cameras.\n>>\n>> This is problematic because if a callback tries to call `cameras()`,\n>> then the same (non-recursive) lock would be locked again.\n>>\n>> Furthermore, there is no real need to hold the lock while user code\n>> is running, so release the lock as soon as possible.\n>>\n> \n> This all sounds quite reasonable to me, so only a stylistic comment\n> below...\n> \n>> Signed-off-by: Barnabás Pőcze \n>> ---\n>> src/libcamera/camera_manager.cpp | 16 +++++++++-------\n>> 1 file changed, 9 insertions(+), 7 deletions(-)\n>>\n>> diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp\n>> index 87e6717ec..d728ac44a 100644\n>> --- a/src/libcamera/camera_manager.cpp\n>> +++ b/src/libcamera/camera_manager.cpp\n>> @@ -202,6 +202,7 @@ void CameraManager::Private::addCamera(std::shared_ptr camera)\n>> {\n>> ASSERT(Thread::current() == this);\n>> \n>> +{\n> \n> Having the inner scope at the leftmost layer is my only niggle here. I\n> think I would suggest that the MutexLocker scope should be indented\n\nOkay, I'll indent it; my thinking was to avoid the excessive diff that\nit causes; but I agree it does look odd.\n\n\n> \n>> MutexLocker locker(mutex_);\n>> \n>> for (const std::shared_ptr &c : cameras_) {\n>> @@ -213,13 +214,12 @@ void CameraManager::Private::addCamera(std::shared_ptr camera)\n>> }\n>> }\n>> \n>> - cameras_.push_back(std::move(camera));\n>> -\n>> - unsigned int index = cameras_.size() - 1;\n>> + cameras_.push_back(camera);\n>> +}\n>> \n>> /* Report the addition to the public signal */\n>> CameraManager *const o = LIBCAMERA_O_PTR();\n>> - o->cameraAdded.emit(cameras_[index]);\n>> + o->cameraAdded.emit(std::move(camera));\n> \n> It feels a bit weird that we move the camera to the signal... It's a\n> shared pointer, so I guess it doesn't matter ... it's not actually\n> transferring ownership to the transient signal emitter ?\n> \n> I guess this is because it's not used locally after this so it avoids a\n> copy of the shared_ptr?\n\nYes, the `CameraManager` already has a reference to it in `cameras_`, so\nthe local reference in `camera` can be transferred.\n\n\nRegards,\nBarnabás Pőcze\n\n\n> \n>> }\n>> \n>> /**\n>> @@ -236,6 +236,7 @@ void CameraManager::Private::removeCamera(std::shared_ptr camera)\n>> {\n>> ASSERT(Thread::current() == this);\n>> \n>> +{\n>> MutexLocker locker(mutex_);\n>> \n>> auto iter = std::find_if(cameras_.begin(), cameras_.end(),\n>> @@ -245,14 +246,15 @@ void CameraManager::Private::removeCamera(std::shared_ptr camera)\n>> if (iter == cameras_.end())\n>> return;\n>> \n>> + cameras_.erase(iter);\n>> +}\n>> +\n> \n> Same thoughts on the indent and std::move usage below too.\n> \n>> LOG(Camera, Debug)\n>> << \"Unregistering camera '\" << camera->id() << \"'\";\n>> \n>> - cameras_.erase(iter);\n>> -\n>> /* Report the removal to the public signal */\n>> CameraManager *const o = LIBCAMERA_O_PTR();\n>> - o->cameraRemoved.emit(camera);\n>> + o->cameraRemoved.emit(std::move(camera));\n>> }\n>> \n>> /**\n>> -- \n>> 2.48.1\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 ADC02C3257\n\tfor ;\n\tTue, 4 Mar 2025 13:34:57 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6867868779;\n\tTue, 4 Mar 2025 14:34:57 +0100 (CET)","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 F074868755\n\tfor ;\n\tTue, 4 Mar 2025 14:34:54 +0100 (CET)","from [192.168.33.3] (185.221.143.4.nat.pool.zt.hu [185.221.143.4])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id D968A8FA;\n\tTue, 4 Mar 2025 14:33:22 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"IC4bA3V6\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1741095203;\n\tbh=QIcC3JlQz3W+MfFog9T8PsAgi+Grls9nstvIDJNFPHc=;\n\th=Date:Subject:To:References:From:In-Reply-To:From;\n\tb=IC4bA3V6VIDSi8De88kM0ZEyawh5GbOLHiohczXzDmik6D+oFTIPtz+usA26yw/LD\n\tDTXuRQtJ67I1+Mau0YIkJN9L0DDvuNY1+h22tWhFhacaSvk+bMwrffH+N3WmFg+Dnj\n\tYs9mmgxniNakDNYROe/+7M8TT7dPdCPnHd7YUNE8=","Message-ID":"<5b2c364a-911e-49ec-9bd4-8663779aee6e@ideasonboard.com>","Date":"Tue, 4 Mar 2025 14:34:51 +0100","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH v1] libcamera: camera_manager: Do not emit signals while\n\tholding lock","To":"Kieran Bingham ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20250303142010.714280-1-barnabas.pocze@ideasonboard.com>\n\t<174108344751.2914008.16937782545444148964@ping.linuxembedded.co.uk>","From":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Content-Language":"en-US, hu-HU","In-Reply-To":"<174108344751.2914008.16937782545444148964@ping.linuxembedded.co.uk>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":33582,"web_url":"https://patchwork.libcamera.org/comment/33582/","msgid":"<20250305045203.GA32668@pendragon.ideasonboard.com>","date":"2025-03-05T04:52:03","subject":"Re: [PATCH v1] libcamera: camera_manager: Do not emit signals while\n\tholding lock","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Tue, Mar 04, 2025 at 02:34:51PM +0100, Barnabás Pőcze wrote:\n> 2025. 03. 04. 11:17 keltezéssel, Kieran Bingham írta:\n> > Quoting Barnabás Pőcze (2025-03-03 14:20:10)\n> >> Both `CameraManager::Private::{add,remove}Camera()` emit the\n> >> `camera{Added,Removed}` signals, respectively, while holding the\n> >> lock protecting the list of cameras.\n> >>\n> >> This is problematic because if a callback tries to call `cameras()`,\n> >> then the same (non-recursive) lock would be locked again.\n> >>\n> >> Furthermore, there is no real need to hold the lock while user code\n> >> is running, so release the lock as soon as possible.\n> > \n> > This all sounds quite reasonable to me, so only a stylistic comment\n> > below...\n> > \n> >> Signed-off-by: Barnabás Pőcze \n> >> ---\n> >> src/libcamera/camera_manager.cpp | 16 +++++++++-------\n> >> 1 file changed, 9 insertions(+), 7 deletions(-)\n> >>\n> >> diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp\n> >> index 87e6717ec..d728ac44a 100644\n> >> --- a/src/libcamera/camera_manager.cpp\n> >> +++ b/src/libcamera/camera_manager.cpp\n> >> @@ -202,6 +202,7 @@ void CameraManager::Private::addCamera(std::shared_ptr camera)\n> >> {\n> >> ASSERT(Thread::current() == this);\n> >> \n> >> +{\n> > \n> > Having the inner scope at the leftmost layer is my only niggle here. I\n> > think I would suggest that the MutexLocker scope should be indented\n> \n> Okay, I'll indent it; my thinking was to avoid the excessive diff that\n> it causes; but I agree it does look odd.\n\nWith that fixed,\n\nReviewed-by: Laurent Pinchart \n\n> >> MutexLocker locker(mutex_);\n> >> \n> >> for (const std::shared_ptr &c : cameras_) {\n> >> @@ -213,13 +214,12 @@ void CameraManager::Private::addCamera(std::shared_ptr camera)\n> >> }\n> >> }\n> >> \n> >> - cameras_.push_back(std::move(camera));\n> >> -\n> >> - unsigned int index = cameras_.size() - 1;\n> >> + cameras_.push_back(camera);\n> >> +}\n> >> \n> >> /* Report the addition to the public signal */\n> >> CameraManager *const o = LIBCAMERA_O_PTR();\n> >> - o->cameraAdded.emit(cameras_[index]);\n> >> + o->cameraAdded.emit(std::move(camera));\n> > \n> > It feels a bit weird that we move the camera to the signal... It's a\n> > shared pointer, so I guess it doesn't matter ... it's not actually\n> > transferring ownership to the transient signal emitter ?\n> > \n> > I guess this is because it's not used locally after this so it avoids a\n> > copy of the shared_ptr?\n> \n> Yes, the `CameraManager` already has a reference to it in `cameras_`, so\n> the local reference in `camera` can be transferred.\n> \n> >> }\n> >> \n> >> /**\n> >> @@ -236,6 +236,7 @@ void CameraManager::Private::removeCamera(std::shared_ptr camera)\n> >> {\n> >> ASSERT(Thread::current() == this);\n> >> \n> >> +{\n> >> MutexLocker locker(mutex_);\n> >> \n> >> auto iter = std::find_if(cameras_.begin(), cameras_.end(),\n> >> @@ -245,14 +246,15 @@ void CameraManager::Private::removeCamera(std::shared_ptr camera)\n> >> if (iter == cameras_.end())\n> >> return;\n> >> \n> >> + cameras_.erase(iter);\n> >> +}\n> >> +\n> > \n> > Same thoughts on the indent and std::move usage below too.\n> > \n> >> LOG(Camera, Debug)\n> >> << \"Unregistering camera '\" << camera->id() << \"'\";\n> >> \n> >> - cameras_.erase(iter);\n> >> -\n> >> /* Report the removal to the public signal */\n> >> CameraManager *const o = LIBCAMERA_O_PTR();\n> >> - o->cameraRemoved.emit(camera);\n> >> + o->cameraRemoved.emit(std::move(camera));\n> >> }\n> >> \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 795B6C3257\n\tfor ;\n\tWed, 5 Mar 2025 04:52:29 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 897CC687F0;\n\tWed, 5 Mar 2025 05:52:28 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 4FCF261809\n\tfor ;\n\tWed, 5 Mar 2025 05:52:27 +0100 (CET)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 5E6CB1047;\n\tWed, 5 Mar 2025 05:50:54 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"V/Un1DNn\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1741150254;\n\tbh=Jawws4yu/XtYkskoW6NAzIpUAUgbo6sjiC77SbOfeLM=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=V/Un1DNnm38ongjAGgdMbfh2LsupkQitKUlfLCvXPvniuD9UfcaHZb/Bls8sCTyDp\n\t3Cz2O32R2WQe82TC8cxth1EiCSm6yh9cC01F6wLcRRHTXWgIXFl0UYNQXxWyr185li\n\td6nfk61EtpsqKaif7aEi9c1/a5xvn50AQ5vwg8lQ=","Date":"Wed, 5 Mar 2025 06:52:03 +0200","From":"Laurent Pinchart ","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Cc":"Kieran Bingham ,\n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v1] libcamera: camera_manager: Do not emit signals while\n\tholding lock","Message-ID":"<20250305045203.GA32668@pendragon.ideasonboard.com>","References":"<20250303142010.714280-1-barnabas.pocze@ideasonboard.com>\n\t<174108344751.2914008.16937782545444148964@ping.linuxembedded.co.uk>\n\t<5b2c364a-911e-49ec-9bd4-8663779aee6e@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<5b2c364a-911e-49ec-9bd4-8663779aee6e@ideasonboard.com>","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]