[{"id":33315,"web_url":"https://patchwork.libcamera.org/comment/33315/","msgid":"<2xylvk3rluv6yzv3i4ftedesp3fysxktpek3mdwcocp5lerflc@vkqy3j4n5xie>","date":"2025-02-06T17:33:17","subject":"Re: [RFC PATCH v3 19/21] apps: lc-compliance: Run request completion\n\thandler in \"main\" thread","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Barnabás\n\nOn Thu, Jan 30, 2025 at 11:51:41AM +0000, Barnabás Pőcze wrote:\n> Currently, `Capture::requestCompleted()` runs in the `CameraManager`'s\n> thread. This makes it a bit more complicated to use googletest and\n> report errors in those callbacks since lc-compliance sets up\n> googletest to throw exceptions on fatal errors / test skip, but\n> those exceptions are only caught on the \"main\" thread, the one\n> running the test suite.\n>\n> To minimize the burden of dealing with synchronization in tests,\n> execute `Capture::requestCompleted()` in the event loop's thread\n> by utilizing `EventLoop::callLater()`.\n>\n> Signed-off-by: Barnabás Pőcze \n\nYou mentioned an issue with libevent interaction in v2 ? What is it\nand how has it been fixed ?\n\nThe patch itself looks good to me\nReviewed-by: Jacopo Mondi \n\nThanks\n j\n\n> ---\n> src/apps/lc-compliance/helpers/capture.cpp | 24 +++++++++++++---------\n> src/apps/lc-compliance/helpers/capture.h | 2 +-\n> 2 files changed, 15 insertions(+), 11 deletions(-)\n>\n> diff --git a/src/apps/lc-compliance/helpers/capture.cpp b/src/apps/lc-compliance/helpers/capture.cpp\n> index 4a8627662..5032470d9 100644\n> --- a/src/apps/lc-compliance/helpers/capture.cpp\n> +++ b/src/apps/lc-compliance/helpers/capture.cpp\n> @@ -127,17 +127,13 @@ void Capture::prepareRequests()\n> \t}\n> }\n>\n> -int Capture::queueRequest(libcamera::Request *request)\n> +void Capture::queueRequest(libcamera::Request *request)\n> {\n> \tif (queueLimit_ && queueCount_ >= *queueLimit_)\n> -\t\treturn 0;\n> -\n> -\tint ret = camera_->queueRequest(request);\n> -\tif (ret < 0)\n> -\t\treturn ret;\n> +\t\treturn;\n>\n> +\tASSERT_EQ(camera_->queueRequest(request), 0);\n> \tqueueCount_ += 1;\n> -\treturn 0;\n> }\n>\n> void Capture::requestComplete(Request *request)\n> @@ -152,8 +148,7 @@ void Capture::requestComplete(Request *request)\n> \t\t<< \"Request didn't complete successfully\";\n>\n> \trequest->reuse(Request::ReuseBuffers);\n> -\tif (queueRequest(request))\n> -\t\tloop_->exit(-EINVAL);\n> +\tqueueRequest(request);\n> }\n>\n> void Capture::start()\n> @@ -172,7 +167,14 @@ void Capture::start()\n>\n> \tASSERT_TRUE(allocator_.allocated());\n>\n> -\tcamera_->requestCompleted.connect(this, &Capture::requestComplete);\n> +\tcamera_->requestCompleted.connect(this, [this](libcamera::Request *request) {\n> +\t\t/* Runs in the CameraManager thread. */\n> +\n> +\t\tloop_->callLater([this, request] {\n> +\t\t\t/* Run handler in the context of the event loop. */\n> +\t\t\trequestComplete(request);\n> +\t\t}, reinterpret_cast(this));\n> +\t});\n>\n> \tASSERT_EQ(camera_->start(), 0) << \"Failed to start camera\";\n> }\n> @@ -188,6 +190,8 @@ void Capture::stop()\n>\n> \trequests_.clear();\n>\n> +\tloop_->cancelLater(reinterpret_cast(this));\n> +\n> \tfor (const auto &cfg : *config_) {\n> \t\tint ret = allocator_.free(cfg.stream());\n> \t\tEXPECT_EQ(ret, 0) << \"Failed to free buffers associated with stream\";\n> diff --git a/src/apps/lc-compliance/helpers/capture.h b/src/apps/lc-compliance/helpers/capture.h\n> index 48a8dadcb..dacce1fe2 100644\n> --- a/src/apps/lc-compliance/helpers/capture.h\n> +++ b/src/apps/lc-compliance/helpers/capture.h\n> @@ -30,7 +30,7 @@ private:\n> \tvoid stop();\n>\n> \tvoid prepareRequests();\n> -\tint queueRequest(libcamera::Request *request);\n> +\tvoid queueRequest(libcamera::Request *request);\n> \tvoid requestComplete(libcamera::Request *request);\n>\n> \tstd::shared_ptr camera_;\n> --\n> 2.48.1\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 3B480C32EF\n\tfor ;\n\tThu, 6 Feb 2025 17:33:23 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E4B1F685FF;\n\tThu, 6 Feb 2025 18:33:22 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 9393C6053F\n\tfor ;\n\tThu, 6 Feb 2025 18:33:21 +0100 (CET)","from ideasonboard.com (mob-5-90-139-204.net.vodafone.it\n\t[5.90.139.204])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id C80301198;\n\tThu, 6 Feb 2025 18:32:07 +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=\"m1TnGLYO\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1738863128;\n\tbh=0fPsU6YLuIGRoO/HKY4M9hnKCjU83G6wd/pxB0n90CY=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=m1TnGLYOltTLK13PecX1DJ0YHlG4SQ3NqoPPjPQvkwSUbocC63K4d/KNFR6jPwZ0C\n\tf8dfo8MmtNmioEWu23xpDgjBd1ccFxCpD335NhuRlerAagIgmQEh8jj5Z9jbKWW5Ym\n\tpD4AfjwJ5W6UTcBjKxrz6qHi/JHqWfCDfyZEsl6U=","Date":"Thu, 6 Feb 2025 18:33:17 +0100","From":"Jacopo Mondi ","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [RFC PATCH v3 19/21] apps: lc-compliance: Run request completion\n\thandler in \"main\" thread","Message-ID":"<2xylvk3rluv6yzv3i4ftedesp3fysxktpek3mdwcocp5lerflc@vkqy3j4n5xie>","References":"<20250130115001.1129305-1-pobrn@protonmail.com>\n\t<20250130115001.1129305-20-pobrn@protonmail.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20250130115001.1129305-20-pobrn@protonmail.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\" "}},{"id":33324,"web_url":"https://patchwork.libcamera.org/comment/33324/","msgid":"","date":"2025-02-10T08:05:06","subject":"Re: [RFC PATCH v3 19/21] apps: lc-compliance: Run request completion\n\thandler in \"main\" thread","submitter":{"id":133,"url":"https://patchwork.libcamera.org/api/people/133/","name":"Pőcze Barnabás","email":"pobrn@protonmail.com"},"content":"Hi\n\n\n2025. február 6., csütörtök 18:33 keltezéssel, Jacopo Mondi írta:\n\n> Hi Barnabás\n>\n> On Thu, Jan 30, 2025 at 11:51:41AM +0000, Barnabás Pőcze wrote:\n> > Currently, `Capture::requestCompleted()` runs in the `CameraManager`'s\n> > thread. This makes it a bit more complicated to use googletest and\n> > report errors in those callbacks since lc-compliance sets up\n> > googletest to throw exceptions on fatal errors / test skip, but\n> > those exceptions are only caught on the \"main\" thread, the one\n> > running the test suite.\n> >\n> > To minimize the burden of dealing with synchronization in tests,\n> > execute `Capture::requestCompleted()` in the event loop's thread\n> > by utilizing `EventLoop::callLater()`.\n> >\n> > Signed-off-by: Barnabás Pőcze \n>\n> You mentioned an issue with libevent interaction in v2 ? What is it\n> and how has it been fixed ?\n\nAn exception cannot leave an event callback otherwise libevent will enter an\ninconsistent state. This was addressed by catching exceptions from deferred calls,\nbreaking the event loop, and then rethrowing the exception from `EventLoop::exec()`.\n\n\n\n>\n> The patch itself looks good to me\n> Reviewed-by: Jacopo Mondi \n>\n> Thanks\n> j\n>\n> > ---\n> > src/apps/lc-compliance/helpers/capture.cpp | 24 +++++++++++++---------\n> > src/apps/lc-compliance/helpers/capture.h | 2 +-\n> > 2 files changed, 15 insertions(+), 11 deletions(-)\n> >\n> > diff --git a/src/apps/lc-compliance/helpers/capture.cpp b/src/apps/lc-compliance/helpers/capture.cpp\n> > index 4a8627662..5032470d9 100644\n> > --- a/src/apps/lc-compliance/helpers/capture.cpp\n> > +++ b/src/apps/lc-compliance/helpers/capture.cpp\n> > @@ -127,17 +127,13 @@ void Capture::prepareRequests()\n> > \t}\n> > }\n> >\n> > -int Capture::queueRequest(libcamera::Request *request)\n> > +void Capture::queueRequest(libcamera::Request *request)\n> > {\n> > \tif (queueLimit_ && queueCount_ >= *queueLimit_)\n> > -\t\treturn 0;\n> > -\n> > -\tint ret = camera_->queueRequest(request);\n> > -\tif (ret < 0)\n> > -\t\treturn ret;\n> > +\t\treturn;\n> >\n> > +\tASSERT_EQ(camera_->queueRequest(request), 0);\n> > \tqueueCount_ += 1;\n> > -\treturn 0;\n> > }\n> >\n> > void Capture::requestComplete(Request *request)\n> > @@ -152,8 +148,7 @@ void Capture::requestComplete(Request *request)\n> > \t\t<< \"Request didn't complete successfully\";\n> >\n> > \trequest->reuse(Request::ReuseBuffers);\n> > -\tif (queueRequest(request))\n> > -\t\tloop_->exit(-EINVAL);\n> > +\tqueueRequest(request);\n> > }\n> >\n> > void Capture::start()\n> > @@ -172,7 +167,14 @@ void Capture::start()\n> >\n> > \tASSERT_TRUE(allocator_.allocated());\n> >\n> > -\tcamera_->requestCompleted.connect(this, &Capture::requestComplete);\n> > +\tcamera_->requestCompleted.connect(this, [this](libcamera::Request *request) {\n> > +\t\t/* Runs in the CameraManager thread. */\n> > +\n> > +\t\tloop_->callLater([this, request] {\n> > +\t\t\t/* Run handler in the context of the event loop. */\n> > +\t\t\trequestComplete(request);\n> > +\t\t}, reinterpret_cast(this));\n> > +\t});\n> >\n> > \tASSERT_EQ(camera_->start(), 0) << \"Failed to start camera\";\n> > }\n> > @@ -188,6 +190,8 @@ void Capture::stop()\n> >\n> > \trequests_.clear();\n> >\n> > +\tloop_->cancelLater(reinterpret_cast(this));\n> > +\n> > \tfor (const auto &cfg : *config_) {\n> > \t\tint ret = allocator_.free(cfg.stream());\n> > \t\tEXPECT_EQ(ret, 0) << \"Failed to free buffers associated with stream\";\n> > diff --git a/src/apps/lc-compliance/helpers/capture.h b/src/apps/lc-compliance/helpers/capture.h\n> > index 48a8dadcb..dacce1fe2 100644\n> > --- a/src/apps/lc-compliance/helpers/capture.h\n> > +++ b/src/apps/lc-compliance/helpers/capture.h\n> > @@ -30,7 +30,7 @@ private:\n> > \tvoid stop();\n> >\n> > \tvoid prepareRequests();\n> > -\tint queueRequest(libcamera::Request *request);\n> > +\tvoid queueRequest(libcamera::Request *request);\n> > \tvoid requestComplete(libcamera::Request *request);\n> >\n> > \tstd::shared_ptr camera_;\n> > --\n> > 2.48.1\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 2DA98C32AF\n\tfor ;\n\tMon, 10 Feb 2025 08:05:15 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id CB78F68616;\n\tMon, 10 Feb 2025 09:05:14 +0100 (CET)","from mail-4316.protonmail.ch (mail-4316.protonmail.ch\n\t[185.70.43.16])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 9E589685FF\n\tfor ;\n\tMon, 10 Feb 2025 09:05:12 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=protonmail.com header.i=@protonmail.com\n\theader.b=\"lDOh7nKC\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=protonmail.com;\n\ts=protonmail3; t=1739174712; x=1739433912;\n\tbh=Mv1vHu4PepiTPR6sP1FaOQnZY2ncHt70Uxco8MfjbQg=;\n\th=Date:To:From:Cc:Subject:Message-ID:In-Reply-To:References:\n\tFeedback-ID:From:To:Cc:Date:Subject:Reply-To:Feedback-ID:\n\tMessage-ID:BIMI-Selector:List-Unsubscribe:List-Unsubscribe-Post;\n\tb=lDOh7nKCGMKFAbMNDt5Zbk3S6vVb3fUJUl1rppi5A020bO9MAC1lh2GSOaiKYkcIm\n\tLrP/h9h8GSfyir6BWcppxRGTb6S66ND+gmi0PYRLThM3HB7P41IaCahAyE+LmKwPKT\n\tXGVrfc0OoYft+/Ewuntk9nR4/xTOGTe3n0ERJj4hxPqrvLoo6Kcyh1hji/71dtsOx0\n\tPjXeK2acXSGvw6QHlW6Qty6eGkbHT820awoAT69+3icM0jI99CDCj23YNFliRaXNAi\n\t4u7n4NGtrn/Hl3uXNXrhh7WxBbAGnx/CVq4sbjh88D5HiQVZpxGwx+uzLZevBh/hip\n\te5wRLG9v36qhA==","Date":"Mon, 10 Feb 2025 08:05:06 +0000","To":"Jacopo Mondi ","From":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [RFC PATCH v3 19/21] apps: lc-compliance: Run request completion\n\thandler in \"main\" thread","Message-ID":"","In-Reply-To":"<2xylvk3rluv6yzv3i4ftedesp3fysxktpek3mdwcocp5lerflc@vkqy3j4n5xie>","References":"<20250130115001.1129305-1-pobrn@protonmail.com>\n\t<20250130115001.1129305-20-pobrn@protonmail.com>\n\t<2xylvk3rluv6yzv3i4ftedesp3fysxktpek3mdwcocp5lerflc@vkqy3j4n5xie>","Feedback-ID":"20568564:user:proton","X-Pm-Message-ID":"e770d31c1d0f52b228949e87378dfed5d1ddb554","MIME-Version":"1.0","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\" "}}]