[{"id":32968,"web_url":"https://patchwork.libcamera.org/comment/32968/","msgid":"","date":"2025-01-08T11:15:45","subject":"Re: [RFC PATCH v1 10/12] apps: lc-compliance: Move request creation\n\tinto common function","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Barnabas\n\nOn Fri, Dec 20, 2024 at 03:08:46PM +0000, Barnabás Pőcze wrote:\n> `CaptureBalanced` and `CaptureUnbalanced` share the same logic\n> for creating requests and assigning buffers, only the queueing\n> of requests is slightly different. Move this common part into\n> a separate function in the base class.\n>\n> Signed-off-by: Barnabás Pőcze \n> ---\n> src/apps/lc-compliance/helpers/capture.cpp | 66 +++++++++++-----------\n> src/apps/lc-compliance/helpers/capture.h | 2 +\n> 2 files changed, 35 insertions(+), 33 deletions(-)\n>\n> diff --git a/src/apps/lc-compliance/helpers/capture.cpp b/src/apps/lc-compliance/helpers/capture.cpp\n> index 5cc7635f7..7a05be9a3 100644\n> --- a/src/apps/lc-compliance/helpers/capture.cpp\n> +++ b/src/apps/lc-compliance/helpers/capture.cpp\n> @@ -7,6 +7,8 @@\n>\n> #include \"capture.h\"\n>\n> +#include \n> +\n> #include \n>\n> using namespace libcamera;\n> @@ -74,43 +76,51 @@ void Capture::stop()\n> \tallocator_.free(stream);\n> }\n>\n> -/* CaptureBalanced */\n> -\n> -CaptureBalanced::CaptureBalanced(std::shared_ptr camera)\n> -\t: Capture(std::move(camera))\n> +void Capture::prepareRequests(unsigned int plannedRequests)\n> {\n> -}\n> -\n> -void CaptureBalanced::capture(unsigned int numRequests)\n> -{\n> -\tstart();\n> +\tassert(config_);\n> +\tassert(requests_.empty());\n>\n> \tStream *stream = config_->at(0).stream();\n> \tconst std::vector> &buffers = allocator_.buffers(stream);\n>\n> \t/* No point in testing less requests then the camera depth. */\n> -\tif (buffers.size() > numRequests) {\n> -\t\tstd::cout << \"Camera needs \" + std::to_string(buffers.size())\n> -\t\t\t+ \" requests, can't test only \"\n> -\t\t\t+ std::to_string(numRequests) << std::endl;\n> +\tif (plannedRequests < buffers.size()) {\n> +\t\tstd::cout << \"Camera needs \" << buffers.size()\n> +\t\t\t << \" requests, can't test only \"\n> +\t\t\t << plannedRequests << std::endl;\n\nI don't see this check being present in the original implementation of\nCaptureUnbalanced::capture(). What am I missing here ?\n\nThanks\n j\n\n> \t\tGTEST_SKIP();\n> \t}\n>\n> -\tqueueCount_ = 0;\n> -\tcaptureCount_ = 0;\n> -\tcaptureLimit_ = numRequests;\n> -\n> -\t/* Queue the recommended number of requests. */\n> \tfor (const std::unique_ptr &buffer : buffers) {\n> \t\tstd::unique_ptr request = camera_->createRequest();\n> \t\tASSERT_TRUE(request) << \"Can't create request\";\n>\n> \t\tASSERT_EQ(request->addBuffer(stream, buffer.get()), 0) << \"Can't set buffer for request\";\n>\n> -\t\tASSERT_EQ(queueRequest(request.get()), 0) << \"Failed to queue request\";\n> -\n> \t\trequests_.push_back(std::move(request));\n> \t}\n> +}\n> +\n> +/* CaptureBalanced */\n> +\n> +CaptureBalanced::CaptureBalanced(std::shared_ptr camera)\n> +\t: Capture(std::move(camera))\n> +{\n> +}\n> +\n> +void CaptureBalanced::capture(unsigned int numRequests)\n> +{\n> +\tstart();\n> +\n> +\tqueueCount_ = 0;\n> +\tcaptureCount_ = 0;\n> +\tcaptureLimit_ = numRequests;\n> +\n> +\tprepareRequests(numRequests);\n> +\n> +\tfor (const auto &request : requests_)\n> +\t\tqueueRequest(request.get());\n>\n> \t/* Run capture session. */\n> \tint status = result_.wait();\n> @@ -156,23 +166,13 @@ void CaptureUnbalanced::capture(unsigned int numRequests)\n> {\n> \tstart();\n>\n> -\tStream *stream = config_->at(0).stream();\n> -\tconst std::vector> &buffers = allocator_.buffers(stream);\n> -\n> \tcaptureCount_ = 0;\n> \tcaptureLimit_ = numRequests;\n>\n> -\t/* Queue the recommended number of requests. */\n> -\tfor (const std::unique_ptr &buffer : buffers) {\n> -\t\tstd::unique_ptr request = camera_->createRequest();\n> -\t\tASSERT_TRUE(request) << \"Can't create request\";\n> +\tprepareRequests(numRequests);\n>\n> -\t\tASSERT_EQ(request->addBuffer(stream, buffer.get()), 0) << \"Can't set buffer for request\";\n> -\n> -\t\tASSERT_EQ(camera_->queueRequest(request.get()), 0) << \"Failed to queue request\";\n> -\n> -\t\trequests_.push_back(std::move(request));\n> -\t}\n> +\tfor (const auto &request : requests_)\n> +\t\tcamera_->queueRequest(request.get());\n>\n> \t/* Run capture session. */\n> \tint status = result_.wait();\n> diff --git a/src/apps/lc-compliance/helpers/capture.h b/src/apps/lc-compliance/helpers/capture.h\n> index 8582ade8e..67c29021b 100644\n> --- a/src/apps/lc-compliance/helpers/capture.h\n> +++ b/src/apps/lc-compliance/helpers/capture.h\n> @@ -26,6 +26,8 @@ protected:\n> \tvoid start();\n> \tvoid stop();\n>\n> +\tvoid prepareRequests(unsigned int plannedRequests);\n> +\n> \tvirtual void requestComplete(libcamera::Request *request) = 0;\n>\n> \tstd::shared_ptr camera_;\n> --\n> 2.47.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 26132C32EF\n\tfor ;\n\tWed, 8 Jan 2025 11:15:56 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4759D684E4;\n\tWed, 8 Jan 2025 12:15:55 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 06F1B608AB\n\tfor ;\n\tWed, 8 Jan 2025 12:15:53 +0100 (CET)","from ideasonboard.com (mob-5-90-140-184.net.vodafone.it\n\t[5.90.140.184])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 2F4C43E;\n\tWed, 8 Jan 2025 12:14:59 +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=\"lovHeQ76\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1736334901;\n\tbh=qRqVRXXxbTVl8Ljmo33KKBQGm0SH9+ae8t7SjQAon8I=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=lovHeQ76gYWgRlLoghYhQ2uOskAi8c8dlK8ryh5rj3LAXdKOzuG0gZO7yYCzd3dJc\n\t+CvJYz/OlfFwXhd2srQOOg5uvzHk0vojS4bTJ88GTf+bGrol/lmg0+NGJUKsPmFAJS\n\tKlv5vY+VLregiMnbAmJBHmQTfR6/NjMS2JEzmJ30=","Date":"Wed, 8 Jan 2025 12:15:45 +0100","From":"Jacopo Mondi ","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [RFC PATCH v1 10/12] apps: lc-compliance: Move request creation\n\tinto common function","Message-ID":"","References":"<20241220150759.709756-1-pobrn@protonmail.com>\n\t<20241220150759.709756-11-pobrn@protonmail.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20241220150759.709756-11-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":33042,"web_url":"https://patchwork.libcamera.org/comment/33042/","msgid":"","date":"2025-01-13T11:24:46","subject":"Re: [RFC PATCH v1 10/12] apps: lc-compliance: Move request creation\n\tinto common function","submitter":{"id":133,"url":"https://patchwork.libcamera.org/api/people/133/","name":"Pőcze Barnabás","email":"pobrn@protonmail.com"},"content":"2025. január 8., szerda 12:15 keltezéssel, Jacopo Mondi írta:\n\n> Hi Barnabas\n> \n> On Fri, Dec 20, 2024 at 03:08:46PM +0000, Barnabás Pőcze wrote:\n> > `CaptureBalanced` and `CaptureUnbalanced` share the same logic\n> > for creating requests and assigning buffers, only the queueing\n> > of requests is slightly different. Move this common part into\n> > a separate function in the base class.\n> >\n> > Signed-off-by: Barnabás Pőcze \n> > ---\n> > src/apps/lc-compliance/helpers/capture.cpp | 66 +++++++++++-----------\n> > src/apps/lc-compliance/helpers/capture.h | 2 +\n> > 2 files changed, 35 insertions(+), 33 deletions(-)\n> >\n> > diff --git a/src/apps/lc-compliance/helpers/capture.cpp b/src/apps/lc-compliance/helpers/capture.cpp\n> > index 5cc7635f7..7a05be9a3 100644\n> > --- a/src/apps/lc-compliance/helpers/capture.cpp\n> > +++ b/src/apps/lc-compliance/helpers/capture.cpp\n> > @@ -7,6 +7,8 @@\n> >\n> > #include \"capture.h\"\n> >\n> > +#include \n> > +\n> > #include \n> >\n> > using namespace libcamera;\n> > @@ -74,43 +76,51 @@ void Capture::stop()\n> > \tallocator_.free(stream);\n> > }\n> >\n> > -/* CaptureBalanced */\n> > -\n> > -CaptureBalanced::CaptureBalanced(std::shared_ptr camera)\n> > -\t: Capture(std::move(camera))\n> > +void Capture::prepareRequests(unsigned int plannedRequests)\n> > {\n> > -}\n> > -\n> > -void CaptureBalanced::capture(unsigned int numRequests)\n> > -{\n> > -\tstart();\n> > +\tassert(config_);\n> > +\tassert(requests_.empty());\n> >\n> > \tStream *stream = config_->at(0).stream();\n> > \tconst std::vector> &buffers = allocator_.buffers(stream);\n> >\n> > \t/* No point in testing less requests then the camera depth. */\n> > -\tif (buffers.size() > numRequests) {\n> > -\t\tstd::cout << \"Camera needs \" + std::to_string(buffers.size())\n> > -\t\t\t+ \" requests, can't test only \"\n> > -\t\t\t+ std::to_string(numRequests) << std::endl;\n> > +\tif (plannedRequests < buffers.size()) {\n> > +\t\tstd::cout << \"Camera needs \" << buffers.size()\n> > +\t\t\t << \" requests, can't test only \"\n> > +\t\t\t << plannedRequests << std::endl;\n> \n> I don't see this check being present in the original implementation of\n> CaptureUnbalanced::capture(). What am I missing here ?\n\nYou're not missing anything, it was an oversight on my part. I did fix it for the\nnext version, but I am now thinking of removing this check altogether. I would\nthink that no matter how many requests are queued, the camera should behave, so\ntesting even just one request, for example, could make sense since that is not\nlikely to happen during normal operations. As well as it would simplify the code\neven more. What do you think?\n\n\nRegards,\nBarnabás Pőcze\n\n\n> \n> Thanks\n> j\n> \n> > \t\tGTEST_SKIP();\n> > \t}\n> >\n> > -\tqueueCount_ = 0;\n> > -\tcaptureCount_ = 0;\n> > -\tcaptureLimit_ = numRequests;\n> > -\n> > -\t/* Queue the recommended number of requests. */\n> > \tfor (const std::unique_ptr &buffer : buffers) {\n> > \t\tstd::unique_ptr request = camera_->createRequest();\n> > \t\tASSERT_TRUE(request) << \"Can't create request\";\n> >\n> > \t\tASSERT_EQ(request->addBuffer(stream, buffer.get()), 0) << \"Can't set buffer for request\";\n> >\n> > -\t\tASSERT_EQ(queueRequest(request.get()), 0) << \"Failed to queue request\";\n> > -\n> > \t\trequests_.push_back(std::move(request));\n> > \t}\n> > +}\n> > +\n> > +/* CaptureBalanced */\n> > +\n> > +CaptureBalanced::CaptureBalanced(std::shared_ptr camera)\n> > +\t: Capture(std::move(camera))\n> > +{\n> > +}\n> > +\n> > +void CaptureBalanced::capture(unsigned int numRequests)\n> > +{\n> > +\tstart();\n> > +\n> > +\tqueueCount_ = 0;\n> > +\tcaptureCount_ = 0;\n> > +\tcaptureLimit_ = numRequests;\n> > +\n> > +\tprepareRequests(numRequests);\n> > +\n> > +\tfor (const auto &request : requests_)\n> > +\t\tqueueRequest(request.get());\n> >\n> > \t/* Run capture session. */\n> > \tint status = result_.wait();\n> > @@ -156,23 +166,13 @@ void CaptureUnbalanced::capture(unsigned int numRequests)\n> > {\n> > \tstart();\n> >\n> > -\tStream *stream = config_->at(0).stream();\n> > -\tconst std::vector> &buffers = allocator_.buffers(stream);\n> > -\n> > \tcaptureCount_ = 0;\n> > \tcaptureLimit_ = numRequests;\n> >\n> > -\t/* Queue the recommended number of requests. */\n> > -\tfor (const std::unique_ptr &buffer : buffers) {\n> > -\t\tstd::unique_ptr request = camera_->createRequest();\n> > -\t\tASSERT_TRUE(request) << \"Can't create request\";\n> > +\tprepareRequests(numRequests);\n> >\n> > -\t\tASSERT_EQ(request->addBuffer(stream, buffer.get()), 0) << \"Can't set buffer for request\";\n> > -\n> > -\t\tASSERT_EQ(camera_->queueRequest(request.get()), 0) << \"Failed to queue request\";\n> > -\n> > -\t\trequests_.push_back(std::move(request));\n> > -\t}\n> > +\tfor (const auto &request : requests_)\n> > +\t\tcamera_->queueRequest(request.get());\n> >\n> > \t/* Run capture session. */\n> > \tint status = result_.wait();\n> > diff --git a/src/apps/lc-compliance/helpers/capture.h b/src/apps/lc-compliance/helpers/capture.h\n> > index 8582ade8e..67c29021b 100644\n> > --- a/src/apps/lc-compliance/helpers/capture.h\n> > +++ b/src/apps/lc-compliance/helpers/capture.h\n> > @@ -26,6 +26,8 @@ protected:\n> > \tvoid start();\n> > \tvoid stop();\n> >\n> > +\tvoid prepareRequests(unsigned int plannedRequests);\n> > +\n> > \tvirtual void requestComplete(libcamera::Request *request) = 0;\n> >\n> > \tstd::shared_ptr camera_;\n> > --\n> > 2.47.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 79615C326C\n\tfor ;\n\tMon, 13 Jan 2025 11:24:52 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7F31268509;\n\tMon, 13 Jan 2025 12:24:51 +0100 (CET)","from mail-10628.protonmail.ch (mail-10628.protonmail.ch\n\t[79.135.106.28])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 14F43684D9\n\tfor ;\n\tMon, 13 Jan 2025 12:24:50 +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=\"pq6jTDy5\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=protonmail.com;\n\ts=protonmail3; t=1736767488; x=1737026688;\n\tbh=Fz4PM0LYO44t+paaAeiq/CqzbTdCabGn1vw/1Z4vv3Y=;\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=pq6jTDy5b+jZIE/JQ2xZiamKd75FH7MuTP1igA4M3z2iqKmBLCxl0nfl4UFLXBrPW\n\t6ENgTI6XpFFBGKEDcvrJlaHfVEAQA4vjm1J8lq6Xio4p1v3zPV3D7btLD52Ga05Io4\n\txzHMkLxTTEPevKuWxoc1EQelUYZSLePbMjs4Cp+OCzjsfsHfiLsUTmMKLevpFw+YNt\n\t8b5G15xjw3XIpVbS+ay04+SJVZVomsqL/iMT/8/llfQS6GI0VY9xHKPLtmRNUCIdTl\n\tdbLmvfNZC3Irhe6155PQtBQYziC/Cz7iDsiX1R/FvOLi38sbN39jlKqRYJBsatPo5F\n\tPTN/pQRKI17+Q==","Date":"Mon, 13 Jan 2025 11:24:46 +0000","To":"Jacopo Mondi ","From":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [RFC PATCH v1 10/12] apps: lc-compliance: Move request creation\n\tinto common function","Message-ID":"","In-Reply-To":"","References":"<20241220150759.709756-1-pobrn@protonmail.com>\n\t<20241220150759.709756-11-pobrn@protonmail.com>\n\t","Feedback-ID":"20568564:user:proton","X-Pm-Message-ID":"ec51f97dabae98c3bda1c6698f98283486af26a1","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\" "}}]