[{"id":33314,"web_url":"https://patchwork.libcamera.org/comment/33314/","msgid":"","date":"2025-02-06T17:31:53","subject":"Re: [RFC PATCH v3 16/21] apps: lc-compliance: Merge\n\t`CaptureBalanced` and `CaptureUnbalanced`","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:22AM +0000, Barnabás Pőcze wrote:\n> The above two classes have very similar implementations, in fact, the\n> only essential difference is how many requests are queued. `CaptureBalanced`\n> queues a predetermined number of requests, while `CaptureUnbalanced`\n> queues requests without limit.\n>\n> This can be addressed by introducing a \"capture\" and a \"queue\" limit\n> into the `Capture` class, which determine at most how many requests\n> can be queued, and how many request completions are expected before\n> stopping.\n>\n> Signed-off-by: Barnabás Pőcze \n\nReviewed-by: Jacopo Mondi \n\nThanks\n j\n\n> ---\n> src/apps/lc-compliance/helpers/capture.cpp | 142 ++++++------------\n> src/apps/lc-compliance/helpers/capture.h | 50 ++----\n> src/apps/lc-compliance/tests/capture_test.cpp | 12 +-\n> 3 files changed, 71 insertions(+), 133 deletions(-)\n>\n> diff --git a/src/apps/lc-compliance/helpers/capture.cpp b/src/apps/lc-compliance/helpers/capture.cpp\n> index 43db15d2d..940646f6c 100644\n> --- a/src/apps/lc-compliance/helpers/capture.cpp\n> +++ b/src/apps/lc-compliance/helpers/capture.cpp\n> @@ -7,12 +7,14 @@\n>\n> #include \"capture.h\"\n>\n> +#include \n> +\n> #include \n>\n> using namespace libcamera;\n>\n> Capture::Capture(std::shared_ptr camera)\n> -\t: loop_(nullptr), camera_(std::move(camera)),\n> +\t: camera_(std::move(camera)),\n> \t allocator_(camera_)\n\nThis now fits on the previous line\n\n> {\n> }\n> @@ -40,153 +42,109 @@ void Capture::configure(StreamRole role)\n> \t}\n> }\n>\n> -void Capture::start()\n> +void Capture::run(unsigned int captureLimit, std::optional queueLimit)\n> {\n> -\tStream *stream = config_->at(0).stream();\n> -\tint count = allocator_.allocate(stream);\n> +\tassert(!queueLimit || captureLimit <= *queueLimit);\n>\n> -\tASSERT_GE(count, 0) << \"Failed to allocate buffers\";\n> -\tEXPECT_EQ(count, config_->at(0).bufferCount) << \"Allocated less buffers than expected\";\n> +\tcaptureLimit_ = captureLimit;\n> +\tqueueLimit_ = queueLimit;\n>\n> -\tcamera_->requestCompleted.connect(this, &Capture::requestComplete);\n> +\tcaptureCount_ = queueCount_ = 0;\n>\n> -\tASSERT_EQ(camera_->start(), 0) << \"Failed to start camera\";\n> -}\n> +\tEventLoop loop;\n> +\tloop_ = &loop;\n>\n> -void Capture::stop()\n> -{\n> -\tif (!config_ || !allocator_.allocated())\n> -\t\treturn;\n> -\n> -\tcamera_->stop();\n> +\tstart();\n> +\tprepareRequests();\n>\n> -\tcamera_->requestCompleted.disconnect(this);\n> +\tfor (const auto &request : requests_)\n> +\t\tqueueRequest(request.get());\n>\n> -\tStream *stream = config_->at(0).stream();\n> -\trequests_.clear();\n> -\tallocator_.free(stream);\n> -}\n> +\tEXPECT_EQ(loop_->exec(), 0);\n>\n> -/* CaptureBalanced */\n> +\tstop();\n>\n> -CaptureBalanced::CaptureBalanced(std::shared_ptr camera)\n> -\t: Capture(std::move(camera))\n> -{\n> +\tEXPECT_LE(captureLimit_, captureCount_);\n> +\tEXPECT_LE(captureCount_, queueCount_);\n> +\tEXPECT_TRUE(!queueLimit_ || queueCount_ <= *queueLimit_);\n> }\n>\n> -void CaptureBalanced::capture(unsigned int numRequests)\n> +void Capture::prepareRequests()\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> +\tif (queueLimit_ && *queueLimit_ < buffers.size()) {\n> \t\tGTEST_SKIP() << \"Camera needs \" << buffers.size()\n> -\t\t\t << \" requests, can't test only \" << numRequests;\n> +\t\t\t << \" requests, can't test only \" << *queueLimit_;\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> -\t/* Run capture session. */\n> -\tloop_ = new EventLoop();\n> -\tloop_->exec();\n> -\tstop();\n> -\tdelete loop_;\n> -\n> -\tASSERT_EQ(captureCount_, captureLimit_);\n> }\n>\n> -int CaptureBalanced::queueRequest(Request *request)\n> +int Capture::queueRequest(libcamera::Request *request)\n> {\n> -\tqueueCount_++;\n> -\tif (queueCount_ > captureLimit_)\n> +\tif (queueLimit_ && queueCount_ >= *queueLimit_)\n> \t\treturn 0;\n>\n> -\treturn camera_->queueRequest(request);\n> +\tint ret = camera_->queueRequest(request);\n> +\tif (ret < 0)\n> +\t\treturn ret;\n> +\n> +\tqueueCount_ += 1;\n> +\treturn 0;\n> }\n>\n> -void CaptureBalanced::requestComplete(Request *request)\n> +void Capture::requestComplete(Request *request)\n> {\n> -\tEXPECT_EQ(request->status(), Request::Status::RequestComplete)\n> -\t\t<< \"Request didn't complete successfully\";\n> -\n> \tcaptureCount_++;\n> \tif (captureCount_ >= captureLimit_) {\n> \t\tloop_->exit(0);\n> \t\treturn;\n> \t}\n>\n> +\tEXPECT_EQ(request->status(), Request::Status::RequestComplete)\n> +\t\t<< \"Request didn't complete successfully\";\n> +\n> \trequest->reuse(Request::ReuseBuffers);\n> \tif (queueRequest(request))\n> \t\tloop_->exit(-EINVAL);\n> }\n>\n> -/* CaptureUnbalanced */\n> -\n> -CaptureUnbalanced::CaptureUnbalanced(std::shared_ptr camera)\n> -\t: Capture(std::move(camera))\n> -{\n> -}\n> -\n> -void CaptureUnbalanced::capture(unsigned int numRequests)\n> +void Capture::start()\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> -\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> +\tint count = allocator_.allocate(stream);\n>\n> -\t\trequests_.push_back(std::move(request));\n> -\t}\n> +\tASSERT_GE(count, 0) << \"Failed to allocate buffers\";\n> +\tEXPECT_EQ(count, config_->at(0).bufferCount) << \"Allocated less buffers than expected\";\n>\n> -\t/* Run capture session. */\n> -\tloop_ = new EventLoop();\n> -\tint status = loop_->exec();\n> -\tstop();\n> -\tdelete loop_;\n> +\tcamera_->requestCompleted.connect(this, &Capture::requestComplete);\n>\n> -\tASSERT_EQ(status, 0);\n> +\tASSERT_EQ(camera_->start(), 0) << \"Failed to start camera\";\n> }\n>\n> -void CaptureUnbalanced::requestComplete(Request *request)\n> +void Capture::stop()\n> {\n> -\tcaptureCount_++;\n> -\tif (captureCount_ >= captureLimit_) {\n> -\t\tloop_->exit(0);\n> +\tif (!config_ || !allocator_.allocated())\n> \t\treturn;\n> -\t}\n>\n> -\tEXPECT_EQ(request->status(), Request::Status::RequestComplete)\n> -\t\t<< \"Request didn't complete successfully\";\n> +\tcamera_->stop();\n>\n> -\trequest->reuse(Request::ReuseBuffers);\n> -\tif (camera_->queueRequest(request))\n> -\t\tloop_->exit(-EINVAL);\n> +\tcamera_->requestCompleted.disconnect(this);\n> +\n> +\tStream *stream = config_->at(0).stream();\n> +\trequests_.clear();\n> +\tallocator_.free(stream);\n> }\n> diff --git a/src/apps/lc-compliance/helpers/capture.h b/src/apps/lc-compliance/helpers/capture.h\n> index a4cc3a99e..ede395e2a 100644\n> --- a/src/apps/lc-compliance/helpers/capture.h\n> +++ b/src/apps/lc-compliance/helpers/capture.h\n> @@ -8,6 +8,7 @@\n> #pragma once\n>\n> #include \n> +#include \n>\n> #include \n>\n> @@ -16,51 +17,30 @@\n> class Capture\n> {\n> public:\n> +\tCapture(std::shared_ptr camera);\n> +\t~Capture();\n> +\n> \tvoid configure(libcamera::StreamRole role);\n> +\tvoid run(unsigned int captureLimit, std::optional queueLimit = {});\n>\n> -protected:\n> -\tCapture(std::shared_ptr camera);\n> -\tvirtual ~Capture();\n> +private:\n> +\tLIBCAMERA_DISABLE_COPY_AND_MOVE(Capture)\n>\n> \tvoid start();\n> \tvoid stop();\n>\n> -\tvirtual void requestComplete(libcamera::Request *request) = 0;\n> -\n> -\tEventLoop *loop_;\n> +\tvoid prepareRequests();\n> +\tint queueRequest(libcamera::Request *request);\n> +\tvoid requestComplete(libcamera::Request *request);\n>\n> \tstd::shared_ptr camera_;\n> \tlibcamera::FrameBufferAllocator allocator_;\n> \tstd::unique_ptr config_;\n> \tstd::vector> requests_;\n> -};\n> -\n> -class CaptureBalanced : public Capture\n> -{\n> -public:\n> -\tCaptureBalanced(std::shared_ptr camera);\n> -\n> -\tvoid capture(unsigned int numRequests);\n> -\n> -private:\n> -\tint queueRequest(libcamera::Request *request);\n> -\tvoid requestComplete(libcamera::Request *request) override;\n> -\n> -\tunsigned int queueCount_;\n> -\tunsigned int captureCount_;\n> -\tunsigned int captureLimit_;\n> -};\n> -\n> -class CaptureUnbalanced : public Capture\n> -{\n> -public:\n> -\tCaptureUnbalanced(std::shared_ptr camera);\n> -\n> -\tvoid capture(unsigned int numRequests);\n> -\n> -private:\n> -\tvoid requestComplete(libcamera::Request *request) override;\n>\n> -\tunsigned int captureCount_;\n> -\tunsigned int captureLimit_;\n> +\tEventLoop *loop_ = nullptr;\n> +\tunsigned int captureLimit_ = 0;\n> +\tstd::optional queueLimit_;\n> +\tunsigned int captureCount_ = 0;\n> +\tunsigned int queueCount_ = 0;\n> };\n> diff --git a/src/apps/lc-compliance/tests/capture_test.cpp b/src/apps/lc-compliance/tests/capture_test.cpp\n> index 97465a612..93bed48f0 100644\n> --- a/src/apps/lc-compliance/tests/capture_test.cpp\n> +++ b/src/apps/lc-compliance/tests/capture_test.cpp\n> @@ -87,11 +87,11 @@ TEST_P(SingleStream, Capture)\n> {\n> \tauto [role, numRequests] = GetParam();\n>\n> -\tCaptureBalanced capture(camera_);\n> +\tCapture capture(camera_);\n>\n> \tcapture.configure(role);\n>\n> -\tcapture.capture(numRequests);\n> +\tcapture.run(numRequests, numRequests);\n> }\n>\n> /*\n> @@ -106,12 +106,12 @@ TEST_P(SingleStream, CaptureStartStop)\n> \tauto [role, numRequests] = GetParam();\n> \tunsigned int numRepeats = 3;\n>\n> -\tCaptureBalanced capture(camera_);\n> +\tCapture capture(camera_);\n>\n> \tcapture.configure(role);\n>\n> \tfor (unsigned int starts = 0; starts < numRepeats; starts++)\n> -\t\tcapture.capture(numRequests);\n> +\t\tcapture.run(numRequests, numRequests);\n> }\n>\n> /*\n> @@ -125,11 +125,11 @@ TEST_P(SingleStream, UnbalancedStop)\n> {\n> \tauto [role, numRequests] = GetParam();\n>\n> -\tCaptureUnbalanced capture(camera_);\n> +\tCapture capture(camera_);\n>\n> \tcapture.configure(role);\n>\n> -\tcapture.capture(numRequests);\n> +\tcapture.run(numRequests);\n> }\n>\n> INSTANTIATE_TEST_SUITE_P(CaptureTests,\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 D7D90C32EF\n\tfor ;\n\tThu, 6 Feb 2025 17:31:59 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2F2946053F;\n\tThu, 6 Feb 2025 18:31:59 +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 93F966053F\n\tfor ;\n\tThu, 6 Feb 2025 18:31:57 +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 A6EE61198;\n\tThu, 6 Feb 2025 18:30: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=\"U3Dzy+zh\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1738863043;\n\tbh=a7tsowgnhVBAbYjfoIXh4nRbhK4lIYnocmvBusmiLZM=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=U3Dzy+zhD4yNxhyqt7AWBh/9QVgVj31MOPGVbF7nu3o9b84KcZ7GCVZcG2LFOAIr4\n\t2jig5fqYnf1Tyn95VJfhz9B7TFrfXz66Yn9m0tJgi32Z5ThxqV3pCcTVuq0/EcLfuD\n\t+X4shW2kq7lM+xLcLUskRpj3tml0KDGETecTYYMA=","Date":"Thu, 6 Feb 2025 18:31:53 +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 16/21] apps: lc-compliance: Merge\n\t`CaptureBalanced` and `CaptureUnbalanced`","Message-ID":"","References":"<20250130115001.1129305-1-pobrn@protonmail.com>\n\t<20250130115001.1129305-17-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-17-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\" "}}]