[{"id":33137,"web_url":"https://patchwork.libcamera.org/comment/33137/","msgid":"","date":"2025-01-22T12:20:06","subject":"Re: [RFC PATCH v2 13/16] 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 Tue, Jan 14, 2025 at 06:22:52PM +0000, Barnabás Pőcze wrote:\n> The above two classes implement very similar, in fact, the only essential\n\ns/implement/implementations are/ ?\n\n> difference is how many requests are queued. `CaptureBalanced` queues\n> a predetermined number of requests, while `CaptureUnbalanced` queues\n> 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> ---\n> src/apps/lc-compliance/helpers/capture.cpp | 141 +++++++-----------\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(+), 132 deletions(-)\n>\n> diff --git a/src/apps/lc-compliance/helpers/capture.cpp b/src/apps/lc-compliance/helpers/capture.cpp\n> index 43db15d2d..77e87c9e4 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> {\n> }\n> @@ -40,153 +42,110 @@ 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\nCan loop_ be made a class member instead of being created here ?\nEach test run will create an instance of the Capture class if I'm not\nmistaken\n\n>\n> -void Capture::stop()\n> -{\n> -\tif (!config_ || !allocator_.allocated())\n> -\t\treturn;\n> +\tstart();\n> +\tprepareRequests(queueLimit_);\n>\n> -\tcamera_->stop();\n> +\tfor (const auto &request : requests_)\n> +\t\tqueueRequest(request.get());\n>\n> -\tcamera_->requestCompleted.disconnect(this);\n> +\tEXPECT_EQ(loop_->exec(), 0);\n>\n> -\tStream *stream = config_->at(0).stream();\n> -\trequests_.clear();\n> -\tallocator_.free(stream);\n> -}\n> +\tstop();\n>\n> -/* CaptureBalanced */\n> +\tloop_ = nullptr;\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(std::optional queueLimit)\n\nqueueLimit_ is a class member, initialized by the caller of this\nfunction. Do you need to pass it in here ?\n\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> +\tif (int ret = camera_->queueRequest(request); ret < 0)\n> +\t\treturn ret;\n\nThis is valid, but a bit unusual for the code base.\n\n int ret = camera_->queueRequest(request);\n if (ret)\n return ret;\n\nis more commmon.\n\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..173421fd2 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(std::optional queueLimit = {});\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\nCan we remove \"Unbalanced\" from the test comment ? I always found it a\nbit weird and now that we have removed the class with the\ncorresponding name I think the comment can be\n\n/*\n * Test stop with queued requests\n *\n * Makes sure the camera supports a stop with requests queued. Example failure\n * is a camera that does not handle cancelation of buffers coming back from the\n * video device while stopping.\n */\n\nThe rest looks good, thanks\nReviewed-by: Jacopo Mondi \n\n> }\n>\n> INSTANTIATE_TEST_SUITE_P(CaptureTests,\n> --\n> 2.48.0\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 CD2C9C3200\n\tfor ;\n\tWed, 22 Jan 2025 12:20:13 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9DC4068558;\n\tWed, 22 Jan 2025 13:20:12 +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 B220668549\n\tfor ;\n\tWed, 22 Jan 2025 13:20:10 +0100 (CET)","from ideasonboard.com (93-61-96-190.ip145.fastwebnet.it\n\t[93.61.96.190])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id B09D57E0;\n\tWed, 22 Jan 2025 13:19: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=\"CsdRhx8g\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1737548347;\n\tbh=92gTAJ+/hSjcHSa4MwPupkJv7NkEjl2pnGKVbJv24rg=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=CsdRhx8g0uSjc244qYNP3IisiBzOZo7euG5SuoS9snsezEIdPVRIirA9GxzPqS4C6\n\tew1TOXiBD+AuXTaTGSQXAlDPceymj+RMPWqPMv4a4dehxyamKu9wqpq5g+7uuUUtQf\n\tl8SLOC57G8buktClTJa9Wl/5DsM3lHphH69xTaQA=","Date":"Wed, 22 Jan 2025 13:20:06 +0100","From":"Jacopo Mondi ","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [RFC PATCH v2 13/16] apps: lc-compliance: Merge\n\t`CaptureBalanced` and `CaptureUnbalanced`","Message-ID":"","References":"<20250114182143.1773762-1-pobrn@protonmail.com>\n\t<20250114182143.1773762-14-pobrn@protonmail.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20250114182143.1773762-14-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":33141,"web_url":"https://patchwork.libcamera.org/comment/33141/","msgid":"","date":"2025-01-23T01:56:19","subject":"Re: [RFC PATCH v2 13/16] apps: lc-compliance: Merge\n\t`CaptureBalanced` and `CaptureUnbalanced`","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"On Tue, Jan 14, 2025 at 06:22:52PM +0000, Barnabás Pőcze wrote:\n> The above two classes implement very similar, in fact, the only essential\n> difference is how many requests are queued. `CaptureBalanced` queues\n> a predetermined number of requests, while `CaptureUnbalanced` queues\n> 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\nWith Jacopo's comments addressed,\n\nReviewed-by: Paul Elder \n\n> ---\n> src/apps/lc-compliance/helpers/capture.cpp | 141 +++++++-----------\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(+), 132 deletions(-)\n> \n> diff --git a/src/apps/lc-compliance/helpers/capture.cpp b/src/apps/lc-compliance/helpers/capture.cpp\n> index 43db15d2d..77e87c9e4 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> {\n> }\n> @@ -40,153 +42,110 @@ 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> +\tstart();\n> +\tprepareRequests(queueLimit_);\n> \n> -\tcamera_->stop();\n> +\tfor (const auto &request : requests_)\n> +\t\tqueueRequest(request.get());\n> \n> -\tcamera_->requestCompleted.disconnect(this);\n> +\tEXPECT_EQ(loop_->exec(), 0);\n> \n> -\tStream *stream = config_->at(0).stream();\n> -\trequests_.clear();\n> -\tallocator_.free(stream);\n> -}\n> +\tstop();\n> \n> -/* CaptureBalanced */\n> +\tloop_ = nullptr;\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(std::optional queueLimit)\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> +\tif (int ret = camera_->queueRequest(request); 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..173421fd2 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(std::optional queueLimit = {});\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.0\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 1F4C1BE08B\n\tfor ;\n\tThu, 23 Jan 2025 01:56:28 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5852768558;\n\tThu, 23 Jan 2025 02:56:27 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 0AFD461878\n\tfor ;\n\tThu, 23 Jan 2025 02:56:26 +0100 (CET)","from pyrite.rasen.tech (unknown [IPv6:2603:6081:63f0:60f0::17f2])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 5593ED1F;\n\tThu, 23 Jan 2025 02:55: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=\"nScdqPL9\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1737597322;\n\tbh=r9KpxuM8cUoBV87xPwWCIBqhskGS98OGpk/rx5JiSw4=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=nScdqPL9wixITPBVrFpeQuPo6sc4gcjunLCiM2cm4fuTQ69Hk2tZd2MkK36H5/v/b\n\tiDYwtxW+P9XGaQvX9clTTT/gooIS3FKutqYLdrCtVsgKa9CSpC8zd2Zrsd7VjwLe/j\n\tOXtJp5lnymZ2KKXi5p3Bcv25aRehcknnznD+XJMo=","Date":"Wed, 22 Jan 2025 20:56:19 -0500","From":"Paul Elder ","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [RFC PATCH v2 13/16] apps: lc-compliance: Merge\n\t`CaptureBalanced` and `CaptureUnbalanced`","Message-ID":"","References":"<20250114182143.1773762-1-pobrn@protonmail.com>\n\t<20250114182143.1773762-14-pobrn@protonmail.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20250114182143.1773762-14-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":33150,"web_url":"https://patchwork.libcamera.org/comment/33150/","msgid":"","date":"2025-01-24T10:06:23","subject":"Re: [RFC PATCH v2 13/16] apps: lc-compliance: Merge\n\t`CaptureBalanced` and `CaptureUnbalanced`","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. január 22., szerda 13:20 keltezéssel, Jacopo Mondi írta:\n\n> Hi Barnabás\n> \n> On Tue, Jan 14, 2025 at 06:22:52PM +0000, Barnabás Pőcze wrote:\n> > The above two classes implement very similar, in fact, the only essential\n> \n> s/implement/implementations are/ ?\n> \n> > difference is how many requests are queued. `CaptureBalanced` queues\n> > a predetermined number of requests, while `CaptureUnbalanced` queues\n> > 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> > ---\n> > src/apps/lc-compliance/helpers/capture.cpp | 141 +++++++-----------\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(+), 132 deletions(-)\n> >\n> > diff --git a/src/apps/lc-compliance/helpers/capture.cpp b/src/apps/lc-compliance/helpers/capture.cpp\n> > index 43db15d2d..77e87c9e4 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> > {\n> > }\n> > @@ -40,153 +42,110 @@ 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> Can loop_ be made a class member instead of being created here ?\n> Each test run will create an instance of the Capture class if I'm not\n> mistaken\n\nI agree it would be better, I would actually prefer using a single event loop\nduring all tests if possible. But in order to do that, there would need to be a\nway to cancel the callbacks submitted with `EventLoop::callLater()`.\n\nOne potential way to implement it would be to add a second argument to `callLater()`,\nnamed something like \"cookie\", and then add a new function like `cancelLater(cookie)`.\nAny ideas, comments?\n\n\n> \n> >\n> > -void Capture::stop()\n> > -{\n> > -\tif (!config_ || !allocator_.allocated())\n> > -\t\treturn;\n> > +\tstart();\n> > +\tprepareRequests(queueLimit_);\n> >\n> > -\tcamera_->stop();\n> > +\tfor (const auto &request : requests_)\n> > +\t\tqueueRequest(request.get());\n> >\n> > -\tcamera_->requestCompleted.disconnect(this);\n> > +\tEXPECT_EQ(loop_->exec(), 0);\n> >\n> > -\tStream *stream = config_->at(0).stream();\n> > -\trequests_.clear();\n> > -\tallocator_.free(stream);\n> > -}\n> > +\tstop();\n> >\n> > -/* CaptureBalanced */\n> > +\tloop_ = nullptr;\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(std::optional queueLimit)\n> \n> queueLimit_ is a class member, initialized by the caller of this\n> function. Do you need to pass it in here ?\n\nProbably not.\n\n\n> \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> > +\tif (int ret = camera_->queueRequest(request); ret < 0)\n> > +\t\treturn ret;\n> \n> This is valid, but a bit unusual for the code base.\n> \n> int ret = camera_->queueRequest(request);\n> if (ret)\n> return ret;\n> \n> is more commmon.\n\nACK\n\n\n> \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..173421fd2 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(std::optional queueLimit = {});\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> Can we remove \"Unbalanced\" from the test comment ? I always found it a\n> bit weird and now that we have removed the class with the\n> corresponding name I think the comment can be\n> \n> /*\n> * Test stop with queued requests\n> *\n> * Makes sure the camera supports a stop with requests queued. Example failure\n> * is a camera that does not handle cancelation of buffers coming back from the\n> * video device while stopping.\n> */\n> \n> The rest looks good, thanks\n> Reviewed-by: Jacopo Mondi \n\nAnd what about the name (`CaptureUnbalanced`)? Should that change as well?\n\n\nRegards,\nBarnabás Pőcze\n\n\n> \n> > }\n> >\n> > INSTANTIATE_TEST_SUITE_P(CaptureTests,\n> > --\n> > 2.48.0\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 360D5C322E\n\tfor ;\n\tFri, 24 Jan 2025 10:06:32 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 45B456855C;\n\tFri, 24 Jan 2025 11:06:31 +0100 (CET)","from mail-10629.protonmail.ch (mail-10629.protonmail.ch\n\t[79.135.106.29])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 4991A68506\n\tfor ;\n\tFri, 24 Jan 2025 11:06:29 +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=\"O1OWY8Sa\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=protonmail.com;\n\ts=protonmail3; t=1737713187; x=1737972387;\n\tbh=LOtM49r5lZ9rhfubiZxfRp6v5yaAU4Nqslnwz9fdTr8=;\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=O1OWY8SaelfHauOgZ7iRbRlhpkbEaEMQoW32b/DfgRtrmMmDrOz+qCynB9/Z0N1A9\n\tFxLCN7jKYLo1y674TjSfOpE2XfgxEuDMlG99szFIDPPkxhl2QPS/ORXMsFp1EHSI3y\n\tRuv5zkPmTpvMZgJ7UFE4XkYTS4jbmTxXbGtdSa2QLyPZfexoLjTcnZz6FrAz5MrUDe\n\tqAlsAG993uwhB8kq9NU1xBSC2VvClpL9nm8PY4sCzmUvHuZhWAnN2r35tDOc0xJVoS\n\t9SEz210ugKKn1+luLr8dt5k0leGYIU0i4m5IYRoYdAtAc85HS5GHuRx20mqNuWk8Td\n\tZ+qperJb0T2hA==","Date":"Fri, 24 Jan 2025 10:06:23 +0000","To":"Jacopo Mondi ","From":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [RFC PATCH v2 13/16] apps: lc-compliance: Merge\n\t`CaptureBalanced` and `CaptureUnbalanced`","Message-ID":"","In-Reply-To":"","References":"<20250114182143.1773762-1-pobrn@protonmail.com>\n\t<20250114182143.1773762-14-pobrn@protonmail.com>\n\t","Feedback-ID":"20568564:user:proton","X-Pm-Message-ID":"dfdac012cfebe1f6b1c8e5fd3615c9125f85ac3d","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\" "}},{"id":33152,"web_url":"https://patchwork.libcamera.org/comment/33152/","msgid":"","date":"2025-01-24T11:21:52","subject":"Re: [RFC PATCH v2 13/16] 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 Fri, Jan 24, 2025 at 10:06:23AM +0000, Barnabás Pőcze wrote:\n> Hi\n>\n>\n> 2025. január 22., szerda 13:20 keltezéssel, Jacopo Mondi írta:\n>\n> > Hi Barnabás\n> >\n> > On Tue, Jan 14, 2025 at 06:22:52PM +0000, Barnabás Pőcze wrote:\n> > > The above two classes implement very similar, in fact, the only essential\n> >\n> > s/implement/implementations are/ ?\n> >\n> > > difference is how many requests are queued. `CaptureBalanced` queues\n> > > a predetermined number of requests, while `CaptureUnbalanced` queues\n> > > 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> > > ---\n> > > src/apps/lc-compliance/helpers/capture.cpp | 141 +++++++-----------\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(+), 132 deletions(-)\n> > >\n> > > diff --git a/src/apps/lc-compliance/helpers/capture.cpp b/src/apps/lc-compliance/helpers/capture.cpp\n> > > index 43db15d2d..77e87c9e4 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> > > {\n> > > }\n> > > @@ -40,153 +42,110 @@ 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> > Can loop_ be made a class member instead of being created here ?\n> > Each test run will create an instance of the Capture class if I'm not\n> > mistaken\n>\n> I agree it would be better, I would actually prefer using a single event loop\n> during all tests if possible. But in order to do that, there would need to be a\n> way to cancel the callbacks submitted with `EventLoop::callLater()`.\n>\n> One potential way to implement it would be to add a second argument to `callLater()`,\n> named something like \"cookie\", and then add a new function like `cancelLater(cookie)`.\n> Any ideas, comments?\n\nIf cancelling the pending callbacks should happen at EvenLoop::exit()\ntime, can't we simply clear the calls_ queue there ?\n\n>\n>\n> >\n> > >\n> > > -void Capture::stop()\n> > > -{\n> > > -\tif (!config_ || !allocator_.allocated())\n> > > -\t\treturn;\n> > > +\tstart();\n> > > +\tprepareRequests(queueLimit_);\n> > >\n> > > -\tcamera_->stop();\n> > > +\tfor (const auto &request : requests_)\n> > > +\t\tqueueRequest(request.get());\n> > >\n> > > -\tcamera_->requestCompleted.disconnect(this);\n> > > +\tEXPECT_EQ(loop_->exec(), 0);\n> > >\n> > > -\tStream *stream = config_->at(0).stream();\n> > > -\trequests_.clear();\n> > > -\tallocator_.free(stream);\n> > > -}\n> > > +\tstop();\n> > >\n> > > -/* CaptureBalanced */\n> > > +\tloop_ = nullptr;\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(std::optional queueLimit)\n> >\n> > queueLimit_ is a class member, initialized by the caller of this\n> > function. Do you need to pass it in here ?\n>\n> Probably not.\n>\n>\n> >\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> > > +\tif (int ret = camera_->queueRequest(request); ret < 0)\n> > > +\t\treturn ret;\n> >\n> > This is valid, but a bit unusual for the code base.\n> >\n> > int ret = camera_->queueRequest(request);\n> > if (ret)\n> > return ret;\n> >\n> > is more commmon.\n>\n> ACK\n>\n>\n> >\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..173421fd2 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(std::optional queueLimit = {});\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> > Can we remove \"Unbalanced\" from the test comment ? I always found it a\n> > bit weird and now that we have removed the class with the\n> > corresponding name I think the comment can be\n> >\n> > /*\n> > * Test stop with queued requests\n> > *\n> > * Makes sure the camera supports a stop with requests queued. Example failure\n> > * is a camera that does not handle cancelation of buffers coming back from the\n> > * video device while stopping.\n> > */\n> >\n> > The rest looks good, thanks\n> > Reviewed-by: Jacopo Mondi \n>\n> And what about the name (`CaptureUnbalanced`)? Should that change as well?\n>\n>\n> Regards,\n> Barnabás Pőcze\n>\n>\n> >\n> > > }\n> > >\n> > > INSTANTIATE_TEST_SUITE_P(CaptureTests,\n> > > --\n> > > 2.48.0\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 7A057C322E\n\tfor ;\n\tFri, 24 Jan 2025 11:21:59 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 747886855E;\n\tFri, 24 Jan 2025 12:21:58 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B8A0068557\n\tfor ;\n\tFri, 24 Jan 2025 12:21:56 +0100 (CET)","from ideasonboard.com (93-61-96-190.ip145.fastwebnet.it\n\t[93.61.96.190])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 64FEF465;\n\tFri, 24 Jan 2025 12:20:52 +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=\"qPHoAjFv\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1737717652;\n\tbh=36aN+5WquVOA8I8FvIh+akMkSpHqRx+RWQERPzLrO9g=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=qPHoAjFvKuRXZHaI8hz5vbUTJjeVAGc2WWxFluzR453S1sdX1jU83bjDJA8BCH2oJ\n\tJjOipmEixnaeKRJMGKH855GG5SYchvtVjE5BfODJcaPcL4fiStAwajRs7zIrChgV9O\n\tPv1SrXUI90ObhwHDflNS8SLw9plsfUYP+xTGBWAo=","Date":"Fri, 24 Jan 2025 12:21:52 +0100","From":"Jacopo Mondi ","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Cc":"Jacopo Mondi , \n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [RFC PATCH v2 13/16] apps: lc-compliance: Merge\n\t`CaptureBalanced` and `CaptureUnbalanced`","Message-ID":"","References":"<20250114182143.1773762-1-pobrn@protonmail.com>\n\t<20250114182143.1773762-14-pobrn@protonmail.com>\n\t\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"","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":33154,"web_url":"https://patchwork.libcamera.org/comment/33154/","msgid":"","date":"2025-01-24T11:31:22","subject":"Re: [RFC PATCH v2 13/16] apps: lc-compliance: Merge\n\t`CaptureBalanced` and `CaptureUnbalanced`","submitter":{"id":133,"url":"https://patchwork.libcamera.org/api/people/133/","name":"Pőcze Barnabás","email":"pobrn@protonmail.com"},"content":"2025. január 24., péntek 12:21 keltezéssel, Jacopo Mondi írta:\n\n> Hi Barnabás\n> \n> On Fri, Jan 24, 2025 at 10:06:23AM +0000, Barnabás Pőcze wrote:\n> > Hi\n> >\n> >\n> > 2025. január 22., szerda 13:20 keltezéssel, Jacopo Mondi írta:\n> >\n> > > Hi Barnabás\n> > >\n> > > On Tue, Jan 14, 2025 at 06:22:52PM +0000, Barnabás Pőcze wrote:\n> > > > The above two classes implement very similar, in fact, the only essential\n> > >\n> > > s/implement/implementations are/ ?\n> > >\n> > > > difference is how many requests are queued. `CaptureBalanced` queues\n> > > > a predetermined number of requests, while `CaptureUnbalanced` queues\n> > > > 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> > > > ---\n> > > > src/apps/lc-compliance/helpers/capture.cpp | 141 +++++++-----------\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(+), 132 deletions(-)\n> > > >\n> > > > diff --git a/src/apps/lc-compliance/helpers/capture.cpp b/src/apps/lc-compliance/helpers/capture.cpp\n> > > > index 43db15d2d..77e87c9e4 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> > > > {\n> > > > }\n> > > > @@ -40,153 +42,110 @@ 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> > > Can loop_ be made a class member instead of being created here ?\n> > > Each test run will create an instance of the Capture class if I'm not\n> > > mistaken\n> >\n> > I agree it would be better, I would actually prefer using a single event loop\n> > during all tests if possible. But in order to do that, there would need to be a\n> > way to cancel the callbacks submitted with `EventLoop::callLater()`.\n> >\n> > One potential way to implement it would be to add a second argument to `callLater()`,\n> > named something like \"cookie\", and then add a new function like `cancelLater(cookie)`.\n> > Any ideas, comments?\n> \n> If cancelling the pending callbacks should happen at EvenLoop::exit()\n> time, can't we simply clear the calls_ queue there ?\n\nI suppose it depends on how generic or how lc-compliance specific solution is desired.\n\n\nRegards,\nBarnabás Pőcze\n\n\n> \n> >\n> >\n> > >\n> > > >\n> > > > -void Capture::stop()\n> > > > -{\n> > > > -\tif (!config_ || !allocator_.allocated())\n> > > > -\t\treturn;\n> > > > +\tstart();\n> > > > +\tprepareRequests(queueLimit_);\n> > > >\n> > > > -\tcamera_->stop();\n> > > > +\tfor (const auto &request : requests_)\n> > > > +\t\tqueueRequest(request.get());\n> > > >\n> > > > -\tcamera_->requestCompleted.disconnect(this);\n> > > > +\tEXPECT_EQ(loop_->exec(), 0);\n> > > >\n> > > > -\tStream *stream = config_->at(0).stream();\n> > > > -\trequests_.clear();\n> > > > -\tallocator_.free(stream);\n> > > > -}\n> > > > +\tstop();\n> > > >\n> > > > -/* CaptureBalanced */\n> > > > +\tloop_ = nullptr;\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(std::optional queueLimit)\n> > >\n> > > queueLimit_ is a class member, initialized by the caller of this\n> > > function. Do you need to pass it in here ?\n> >\n> > Probably not.\n> >\n> >\n> > >\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> > > > +\tif (int ret = camera_->queueRequest(request); ret < 0)\n> > > > +\t\treturn ret;\n> > >\n> > > This is valid, but a bit unusual for the code base.\n> > >\n> > > int ret = camera_->queueRequest(request);\n> > > if (ret)\n> > > return ret;\n> > >\n> > > is more commmon.\n> >\n> > ACK\n> >\n> >\n> > >\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..173421fd2 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(std::optional queueLimit = {});\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> > > Can we remove \"Unbalanced\" from the test comment ? I always found it a\n> > > bit weird and now that we have removed the class with the\n> > > corresponding name I think the comment can be\n> > >\n> > > /*\n> > > * Test stop with queued requests\n> > > *\n> > > * Makes sure the camera supports a stop with requests queued. Example failure\n> > > * is a camera that does not handle cancelation of buffers coming back from the\n> > > * video device while stopping.\n> > > */\n> > >\n> > > The rest looks good, thanks\n> > > Reviewed-by: Jacopo Mondi \n> >\n> > And what about the name (`CaptureUnbalanced`)? Should that change as well?\n> >\n> >\n> > Regards,\n> > Barnabás Pőcze\n> >\n> >\n> > >\n> > > > }\n> > > >\n> > > > INSTANTIATE_TEST_SUITE_P(CaptureTests,\n> > > > --\n> > > > 2.48.0\n> > > >\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 5B18FC3220\n\tfor ;\n\tFri, 24 Jan 2025 11:31:30 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 757086855E;\n\tFri, 24 Jan 2025 12:31:29 +0100 (CET)","from mail-4322.protonmail.ch (mail-4322.protonmail.ch\n\t[185.70.43.22])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 7D5A168556\n\tfor ;\n\tFri, 24 Jan 2025 12:31:27 +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=\"UghbItxO\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=protonmail.com;\n\ts=protonmail3; t=1737718286; x=1737977486;\n\tbh=60xuWCmkZfYTklW/xtZl/9O5YtkOp8xsTIbJd+bzXyM=;\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=UghbItxO0UQIqUp6uhDEXBjBhgz4p8PNxX9X689J+wf5zp6k2fL9dNxOJILTxFibT\n\t8YJYaAEi/U+nHRYO5qi5q1PRS9B19kcUEPLxa5cI0a7rNu0W3aZSauDT7lyCm8yFXq\n\tRuLRbOJZTBnOHf65+GHdL3fYQTGpD92YFjm2TXq/cyUxlEzsM7vwaxN/7WHlnMRwn5\n\tO342I2LEVtioys3pHzmzn7vtuisEVZhVTaGLzamf8rOh+RgPFMvHnF1BxVfBme47TG\n\tA2Mc9zboeip9fjdgkyBDEnfPLpb2BIAkA2dMoNYGAVoUCr1JITIe4FChRS3la8mZ+a\n\tCsPD/Lz6L6UiA==","Date":"Fri, 24 Jan 2025 11:31:22 +0000","To":"Jacopo Mondi ","From":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [RFC PATCH v2 13/16] apps: lc-compliance: Merge\n\t`CaptureBalanced` and `CaptureUnbalanced`","Message-ID":"","In-Reply-To":"","References":"<20250114182143.1773762-1-pobrn@protonmail.com>\n\t<20250114182143.1773762-14-pobrn@protonmail.com>\n\t\n\t\n\t","Feedback-ID":"20568564:user:proton","X-Pm-Message-ID":"2df7663dc4603196580eee56034d18bae20ea009","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\" "}},{"id":33155,"web_url":"https://patchwork.libcamera.org/comment/33155/","msgid":"<52ja3lcyhpzxmjj5of3xylewvoskzlw4eao6iolpq7sgw3xl72@br3gszzptj5t>","date":"2025-01-24T11:59:08","subject":"Re: [RFC PATCH v2 13/16] 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 Fri, Jan 24, 2025 at 11:31:22AM +0000, Barnabás Pőcze wrote:\n> 2025. január 24., péntek 12:21 keltezéssel, Jacopo Mondi írta:\n>\n> > Hi Barnabás\n> >\n> > On Fri, Jan 24, 2025 at 10:06:23AM +0000, Barnabás Pőcze wrote:\n> > > Hi\n> > >\n> > >\n> > > 2025. január 22., szerda 13:20 keltezéssel, Jacopo Mondi írta:\n> > >\n> > > > Hi Barnabás\n> > > >\n> > > > On Tue, Jan 14, 2025 at 06:22:52PM +0000, Barnabás Pőcze wrote:\n> > > > > The above two classes implement very similar, in fact, the only essential\n> > > >\n> > > > s/implement/implementations are/ ?\n> > > >\n> > > > > difference is how many requests are queued. `CaptureBalanced` queues\n> > > > > a predetermined number of requests, while `CaptureUnbalanced` queues\n> > > > > 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> > > > > ---\n> > > > > src/apps/lc-compliance/helpers/capture.cpp | 141 +++++++-----------\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(+), 132 deletions(-)\n> > > > >\n> > > > > diff --git a/src/apps/lc-compliance/helpers/capture.cpp b/src/apps/lc-compliance/helpers/capture.cpp\n> > > > > index 43db15d2d..77e87c9e4 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> > > > > {\n> > > > > }\n> > > > > @@ -40,153 +42,110 @@ 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> > > > Can loop_ be made a class member instead of being created here ?\n> > > > Each test run will create an instance of the Capture class if I'm not\n> > > > mistaken\n> > >\n> > > I agree it would be better, I would actually prefer using a single event loop\n> > > during all tests if possible. But in order to do that, there would need to be a\n> > > way to cancel the callbacks submitted with `EventLoop::callLater()`.\n> > >\n> > > One potential way to implement it would be to add a second argument to `callLater()`,\n> > > named something like \"cookie\", and then add a new function like `cancelLater(cookie)`.\n> > > Any ideas, comments?\n> >\n> > If cancelling the pending callbacks should happen at EvenLoop::exit()\n> > time, can't we simply clear the calls_ queue there ?\n>\n> I suppose it depends on how generic or how lc-compliance specific solution is desired.\n>\n\nWhat makes you think the solution would be lc-compliance specific ?\nEvery event loop will evenually be terminated by a call to exit(), am\nI wrong ? And we should also clean up the queue in the destructor.\nWhat am I missing ?\n\nFor sure, I missed your question at the end of the previous email. See\nbelow.\n\n>\n> Regards,\n> Barnabás Pőcze\n>\n>\n> >\n> > >\n> > >\n> > > >\n> > > > >\n> > > > > -void Capture::stop()\n> > > > > -{\n> > > > > -\tif (!config_ || !allocator_.allocated())\n> > > > > -\t\treturn;\n> > > > > +\tstart();\n> > > > > +\tprepareRequests(queueLimit_);\n> > > > >\n> > > > > -\tcamera_->stop();\n> > > > > +\tfor (const auto &request : requests_)\n> > > > > +\t\tqueueRequest(request.get());\n> > > > >\n> > > > > -\tcamera_->requestCompleted.disconnect(this);\n> > > > > +\tEXPECT_EQ(loop_->exec(), 0);\n> > > > >\n> > > > > -\tStream *stream = config_->at(0).stream();\n> > > > > -\trequests_.clear();\n> > > > > -\tallocator_.free(stream);\n> > > > > -}\n> > > > > +\tstop();\n> > > > >\n> > > > > -/* CaptureBalanced */\n> > > > > +\tloop_ = nullptr;\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(std::optional queueLimit)\n> > > >\n> > > > queueLimit_ is a class member, initialized by the caller of this\n> > > > function. Do you need to pass it in here ?\n> > >\n> > > Probably not.\n> > >\n> > >\n> > > >\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> > > > > +\tif (int ret = camera_->queueRequest(request); ret < 0)\n> > > > > +\t\treturn ret;\n> > > >\n> > > > This is valid, but a bit unusual for the code base.\n> > > >\n> > > > int ret = camera_->queueRequest(request);\n> > > > if (ret)\n> > > > return ret;\n> > > >\n> > > > is more commmon.\n> > >\n> > > ACK\n> > >\n> > >\n> > > >\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..173421fd2 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(std::optional queueLimit = {});\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> > > > Can we remove \"Unbalanced\" from the test comment ? I always found it a\n> > > > bit weird and now that we have removed the class with the\n> > > > corresponding name I think the comment can be\n> > > >\n> > > > /*\n> > > > * Test stop with queued requests\n> > > > *\n> > > > * Makes sure the camera supports a stop with requests queued. Example failure\n> > > > * is a camera that does not handle cancelation of buffers coming back from the\n> > > > * video device while stopping.\n> > > > */\n> > > >\n> > > > The rest looks good, thanks\n> > > > Reviewed-by: Jacopo Mondi \n> > >\n> > > And what about the name (`CaptureUnbalanced`)? Should that change as well?\n> > >\n\nUp to you. These tests have always been 'unbounded' rather than\n'unbalanced' to me.\n\n> > >\n> > > Regards,\n> > > Barnabás Pőcze\n> > >\n> > >\n> > > >\n> > > > > }\n> > > > >\n> > > > > INSTANTIATE_TEST_SUITE_P(CaptureTests,\n> > > > > --\n> > > > > 2.48.0\n> > > > >\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 243BBC322E\n\tfor ;\n\tFri, 24 Jan 2025 11:59:15 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2197C6855D;\n\tFri, 24 Jan 2025 12:59:14 +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 B099168556\n\tfor ;\n\tFri, 24 Jan 2025 12:59:12 +0100 (CET)","from ideasonboard.com (93-61-96-190.ip145.fastwebnet.it\n\t[93.61.96.190])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 4FBA8465;\n\tFri, 24 Jan 2025 12:58:08 +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=\"pzKBlGcc\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1737719888;\n\tbh=Zc7N9SuxRxSFtgSBUQsFU6DsNN0TMQpjLmcQRWT0LzQ=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=pzKBlGccR56RMKkF5KzZMXa0MQO+Ea+/0vwQ6TGZLp2WXZ+4pYy0ojmSMImuCWsQC\n\t75ZCRrsMZVqOUNzRUZwtGNtMwHUebv+hYLIALg6ZpQGrScZfKPYhicCsUyqcQIQqFf\n\t1eHyAw6HiTeXoA25zNtlYBuUMpiXNqflYhx4wgtg=","Date":"Fri, 24 Jan 2025 12:59:08 +0100","From":"Jacopo Mondi ","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Cc":"Jacopo Mondi , \n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [RFC PATCH v2 13/16] apps: lc-compliance: Merge\n\t`CaptureBalanced` and `CaptureUnbalanced`","Message-ID":"<52ja3lcyhpzxmjj5of3xylewvoskzlw4eao6iolpq7sgw3xl72@br3gszzptj5t>","References":"<20250114182143.1773762-1-pobrn@protonmail.com>\n\t<20250114182143.1773762-14-pobrn@protonmail.com>\n\t\n\t\n\t\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"","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":33156,"web_url":"https://patchwork.libcamera.org/comment/33156/","msgid":"","date":"2025-01-24T12:25:57","subject":"Re: [RFC PATCH v2 13/16] apps: lc-compliance: Merge\n\t`CaptureBalanced` and `CaptureUnbalanced`","submitter":{"id":133,"url":"https://patchwork.libcamera.org/api/people/133/","name":"Pőcze Barnabás","email":"pobrn@protonmail.com"},"content":"2025. január 24., péntek 12:59 keltezéssel, Jacopo Mondi írta:\n\n> Hi Barnabás\n> \n> On Fri, Jan 24, 2025 at 11:31:22AM +0000, Barnabás Pőcze wrote:\n> > 2025. január 24., péntek 12:21 keltezéssel, Jacopo Mondi írta:\n> >\n> > > Hi Barnabás\n> > >\n> > > On Fri, Jan 24, 2025 at 10:06:23AM +0000, Barnabás Pőcze wrote:\n> > > > Hi\n> > > >\n> > > >\n> > > > 2025. január 22., szerda 13:20 keltezéssel, Jacopo Mondi írta:\n> > > >\n> > > > > Hi Barnabás\n> > > > >\n> > > > > On Tue, Jan 14, 2025 at 06:22:52PM +0000, Barnabás Pőcze wrote:\n> > > > > > The above two classes implement very similar, in fact, the only essential\n> > > > >\n> > > > > s/implement/implementations are/ ?\n> > > > >\n> > > > > > difference is how many requests are queued. `CaptureBalanced` queues\n> > > > > > a predetermined number of requests, while `CaptureUnbalanced` queues\n> > > > > > 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> > > > > > ---\n> > > > > > src/apps/lc-compliance/helpers/capture.cpp | 141 +++++++-----------\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(+), 132 deletions(-)\n> > > > > >\n> > > > > > diff --git a/src/apps/lc-compliance/helpers/capture.cpp b/src/apps/lc-compliance/helpers/capture.cpp\n> > > > > > index 43db15d2d..77e87c9e4 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> > > > > > {\n> > > > > > }\n> > > > > > @@ -40,153 +42,110 @@ 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> > > > > Can loop_ be made a class member instead of being created here ?\n> > > > > Each test run will create an instance of the Capture class if I'm not\n> > > > > mistaken\n> > > >\n> > > > I agree it would be better, I would actually prefer using a single event loop\n> > > > during all tests if possible. But in order to do that, there would need to be a\n> > > > way to cancel the callbacks submitted with `EventLoop::callLater()`.\n> > > >\n> > > > One potential way to implement it would be to add a second argument to `callLater()`,\n> > > > named something like \"cookie\", and then add a new function like `cancelLater(cookie)`.\n> > > > Any ideas, comments?\n> > >\n> > > If cancelling the pending callbacks should happen at EvenLoop::exit()\n> > > time, can't we simply clear the calls_ queue there ?\n> >\n> > I suppose it depends on how generic or how lc-compliance specific solution is desired.\n> >\n> \n> What makes you think the solution would be lc-compliance specific ?\n> Every event loop will evenually be terminated by a call to exit(), am\n> I wrong ? And we should also clean up the queue in the destructor.\n> What am I missing ?\n\nThe queue is already cleared when an `EventLoop` is destroyed.\n\nI can imagine scenarios where temporarily stopping and then restarting the event\nloop - in such a way that it is mostly transparent to the code running in it -\nmight be desirable (e.g. migrating between threads).\n\nThere is nothing that would prevent the restarting of the event loop today,\nand thus clearing the call queue may not always be desirable. But I suppose\nwe could wait until there is an actual use case before addressing this.\n\nAnother thing, I think it would be a good thing if every component could use\na single event loop implementation. In my opinion it is not ideal that libcamera\nproper has an event loop that is completely different from that of cam, lc-compliance.\n\n\n> \n> For sure, I missed your question at the end of the previous email. See\n> below.\n> \n> >\n> > Regards,\n> > Barnabás Pőcze\n> >\n> >\n> > >\n> > > >\n> > > >\n> > > > >\n> > > > > >\n> > > > > > -void Capture::stop()\n> > > > > > -{\n> > > > > > -\tif (!config_ || !allocator_.allocated())\n> > > > > > -\t\treturn;\n> > > > > > +\tstart();\n> > > > > > +\tprepareRequests(queueLimit_);\n> > > > > >\n> > > > > > -\tcamera_->stop();\n> > > > > > +\tfor (const auto &request : requests_)\n> > > > > > +\t\tqueueRequest(request.get());\n> > > > > >\n> > > > > > -\tcamera_->requestCompleted.disconnect(this);\n> > > > > > +\tEXPECT_EQ(loop_->exec(), 0);\n> > > > > >\n> > > > > > -\tStream *stream = config_->at(0).stream();\n> > > > > > -\trequests_.clear();\n> > > > > > -\tallocator_.free(stream);\n> > > > > > -}\n> > > > > > +\tstop();\n> > > > > >\n> > > > > > -/* CaptureBalanced */\n> > > > > > +\tloop_ = nullptr;\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(std::optional queueLimit)\n> > > > >\n> > > > > queueLimit_ is a class member, initialized by the caller of this\n> > > > > function. Do you need to pass it in here ?\n> > > >\n> > > > Probably not.\n> > > >\n> > > >\n> > > > >\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> > > > > > +\tif (int ret = camera_->queueRequest(request); ret < 0)\n> > > > > > +\t\treturn ret;\n> > > > >\n> > > > > This is valid, but a bit unusual for the code base.\n> > > > >\n> > > > > int ret = camera_->queueRequest(request);\n> > > > > if (ret)\n> > > > > return ret;\n> > > > >\n> > > > > is more commmon.\n> > > >\n> > > > ACK\n> > > >\n> > > >\n> > > > >\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..173421fd2 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(std::optional queueLimit = {});\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> > > > > Can we remove \"Unbalanced\" from the test comment ? I always found it a\n> > > > > bit weird and now that we have removed the class with the\n> > > > > corresponding name I think the comment can be\n> > > > >\n> > > > > /*\n> > > > > * Test stop with queued requests\n> > > > > *\n> > > > > * Makes sure the camera supports a stop with requests queued. Example failure\n> > > > > * is a camera that does not handle cancelation of buffers coming back from the\n> > > > > * video device while stopping.\n> > > > > */\n> > > > >\n> > > > > The rest looks good, thanks\n> > > > > Reviewed-by: Jacopo Mondi \n> > > >\n> > > > And what about the name (`CaptureUnbalanced`)? Should that change as well?\n> > > >\n> \n> Up to you. These tests have always been 'unbounded' rather than\n> 'unbalanced' to me.\n\nACK\n\n\nRegards,\nBarnabás Pőcze\n\n\n> \n> > > >\n> > > > Regards,\n> > > > Barnabás Pőcze\n> > > >\n> > > >\n> > > > >\n> > > > > > }\n> > > > > >\n> > > > > > INSTANTIATE_TEST_SUITE_P(CaptureTests,\n> > > > > > --\n> > > > > > 2.48.0\n> > > > > >\n> > > > > >\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 666FEC322E\n\tfor ;\n\tFri, 24 Jan 2025 12:26:05 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0E5B56855D;\n\tFri, 24 Jan 2025 13:26:04 +0100 (CET)","from mail-10631.protonmail.ch (mail-10631.protonmail.ch\n\t[79.135.106.31])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C27B368556\n\tfor ;\n\tFri, 24 Jan 2025 13:26:01 +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=\"LOl2i+lO\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=protonmail.com;\n\ts=protonmail3; t=1737721560; x=1737980760;\n\tbh=1uPWLjz4TyCBzAmp8yY7o307Kpq8Yw9lUomjq2u1ev0=;\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=LOl2i+lOU4ixceJHTKpcJrXfdqB3r8l6oNZ88gA9aGY3KbTbJvQw8EXqUT1wzVuqn\n\tQQYJq6+KAz6k05+9F5+B9CPilb8ZTpjPcahA6UCCMJmnTKvs3kFpe4H2L9EwNIQLkL\n\tqu0T2zHLG89n93bTVADi5L8bkIGEWaAEqwhSnSOLmvWOz1Ud7g3yjGDitv3mJvGfmi\n\tGnHVd69rb75xAeZrqK8h5Bu+VZRj3Rz+0Bc6lpfBLSYUiaPZV0WKcXJXc7Ph2Guo4L\n\tsq78cQNoNYDUlDUgnFlL89dHaQ/f5ck153YB0gcs2u42yT8aIgIaiMhhXe4G0+8SCS\n\t1iO/5Sm2aspqA==","Date":"Fri, 24 Jan 2025 12:25:57 +0000","To":"Jacopo Mondi ,\n\tLaurent Pinchart ","From":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [RFC PATCH v2 13/16] apps: lc-compliance: Merge\n\t`CaptureBalanced` and `CaptureUnbalanced`","Message-ID":"","In-Reply-To":"<52ja3lcyhpzxmjj5of3xylewvoskzlw4eao6iolpq7sgw3xl72@br3gszzptj5t>","References":"<20250114182143.1773762-1-pobrn@protonmail.com>\n\t<20250114182143.1773762-14-pobrn@protonmail.com>\n\t\n\t\n\t\n\t\n\t<52ja3lcyhpzxmjj5of3xylewvoskzlw4eao6iolpq7sgw3xl72@br3gszzptj5t>","Feedback-ID":"20568564:user:proton","X-Pm-Message-ID":"ebfa58f1756eee72023f6d349f9ffc90064446a8","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\" "}},{"id":33157,"web_url":"https://patchwork.libcamera.org/comment/33157/","msgid":"","date":"2025-01-24T13:16:42","subject":"Re: [RFC PATCH v2 13/16] 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 Fri, Jan 24, 2025 at 12:25:57PM +0000, Barnabás Pőcze wrote:\n> 2025. január 24., péntek 12:59 keltezéssel, Jacopo Mondi írta:\n>\n> > Hi Barnabás\n> >\n> > On Fri, Jan 24, 2025 at 11:31:22AM +0000, Barnabás Pőcze wrote:\n> > > 2025. január 24., péntek 12:21 keltezéssel, Jacopo Mondi írta:\n> > >\n> > > > Hi Barnabás\n> > > >\n> > > > On Fri, Jan 24, 2025 at 10:06:23AM +0000, Barnabás Pőcze wrote:\n> > > > > Hi\n> > > > >\n> > > > >\n> > > > > 2025. január 22., szerda 13:20 keltezéssel, Jacopo Mondi írta:\n> > > > >\n> > > > > > Hi Barnabás\n> > > > > >\n> > > > > > On Tue, Jan 14, 2025 at 06:22:52PM +0000, Barnabás Pőcze wrote:\n> > > > > > > The above two classes implement very similar, in fact, the only essential\n> > > > > >\n> > > > > > s/implement/implementations are/ ?\n> > > > > >\n> > > > > > > difference is how many requests are queued. `CaptureBalanced` queues\n> > > > > > > a predetermined number of requests, while `CaptureUnbalanced` queues\n> > > > > > > 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> > > > > > > ---\n> > > > > > > src/apps/lc-compliance/helpers/capture.cpp | 141 +++++++-----------\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(+), 132 deletions(-)\n> > > > > > >\n> > > > > > > diff --git a/src/apps/lc-compliance/helpers/capture.cpp b/src/apps/lc-compliance/helpers/capture.cpp\n> > > > > > > index 43db15d2d..77e87c9e4 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> > > > > > > {\n> > > > > > > }\n> > > > > > > @@ -40,153 +42,110 @@ 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> > > > > > Can loop_ be made a class member instead of being created here ?\n> > > > > > Each test run will create an instance of the Capture class if I'm not\n> > > > > > mistaken\n> > > > >\n> > > > > I agree it would be better, I would actually prefer using a single event loop\n> > > > > during all tests if possible. But in order to do that, there would need to be a\n> > > > > way to cancel the callbacks submitted with `EventLoop::callLater()`.\n> > > > >\n> > > > > One potential way to implement it would be to add a second argument to `callLater()`,\n> > > > > named something like \"cookie\", and then add a new function like `cancelLater(cookie)`.\n> > > > > Any ideas, comments?\n> > > >\n> > > > If cancelling the pending callbacks should happen at EvenLoop::exit()\n> > > > time, can't we simply clear the calls_ queue there ?\n> > >\n> > > I suppose it depends on how generic or how lc-compliance specific solution is desired.\n> > >\n> >\n> > What makes you think the solution would be lc-compliance specific ?\n> > Every event loop will evenually be terminated by a call to exit(), am\n> > I wrong ? And we should also clean up the queue in the destructor.\n> > What am I missing ?\n>\n> The queue is already cleared when an `EventLoop` is destroyed.\n>\n> I can imagine scenarios where temporarily stopping and then restarting the event\n> loop - in such a way that it is mostly transparent to the code running in it -\n> might be desirable (e.g. migrating between threads).\n>\n> There is nothing that would prevent the restarting of the event loop today,\n> and thus clearing the call queue may not always be desirable. But I suppose\n> we could wait until there is an actual use case before addressing this.\n\nTrue that. I presume as long as we document that stopping an event\nloop clears all the pending callback, I think it's fine for now.\n\n>\n> Another thing, I think it would be a good thing if every component could use\n> a single event loop implementation. In my opinion it is not ideal that libcamera\n> proper has an event loop that is completely different from that of cam, lc-compliance.\n>\n\nmmm, maybe I miss what you mean by \"component\", but cam and\nlc-compliance are effectively applications using the libcamera API and\nwe don't want applications to run in the same even loop as libcamera ?\n\n>\n> >\n> > For sure, I missed your question at the end of the previous email. See\n> > below.\n> >\n> > >\n> > > Regards,\n> > > Barnabás Pőcze\n> > >\n> > >\n> > > >\n> > > > >\n> > > > >\n> > > > > >\n> > > > > > >\n> > > > > > > -void Capture::stop()\n> > > > > > > -{\n> > > > > > > -\tif (!config_ || !allocator_.allocated())\n> > > > > > > -\t\treturn;\n> > > > > > > +\tstart();\n> > > > > > > +\tprepareRequests(queueLimit_);\n> > > > > > >\n> > > > > > > -\tcamera_->stop();\n> > > > > > > +\tfor (const auto &request : requests_)\n> > > > > > > +\t\tqueueRequest(request.get());\n> > > > > > >\n> > > > > > > -\tcamera_->requestCompleted.disconnect(this);\n> > > > > > > +\tEXPECT_EQ(loop_->exec(), 0);\n> > > > > > >\n> > > > > > > -\tStream *stream = config_->at(0).stream();\n> > > > > > > -\trequests_.clear();\n> > > > > > > -\tallocator_.free(stream);\n> > > > > > > -}\n> > > > > > > +\tstop();\n> > > > > > >\n> > > > > > > -/* CaptureBalanced */\n> > > > > > > +\tloop_ = nullptr;\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(std::optional queueLimit)\n> > > > > >\n> > > > > > queueLimit_ is a class member, initialized by the caller of this\n> > > > > > function. Do you need to pass it in here ?\n> > > > >\n> > > > > Probably not.\n> > > > >\n> > > > >\n> > > > > >\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> > > > > > > +\tif (int ret = camera_->queueRequest(request); ret < 0)\n> > > > > > > +\t\treturn ret;\n> > > > > >\n> > > > > > This is valid, but a bit unusual for the code base.\n> > > > > >\n> > > > > > int ret = camera_->queueRequest(request);\n> > > > > > if (ret)\n> > > > > > return ret;\n> > > > > >\n> > > > > > is more commmon.\n> > > > >\n> > > > > ACK\n> > > > >\n> > > > >\n> > > > > >\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..173421fd2 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(std::optional queueLimit = {});\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> > > > > > Can we remove \"Unbalanced\" from the test comment ? I always found it a\n> > > > > > bit weird and now that we have removed the class with the\n> > > > > > corresponding name I think the comment can be\n> > > > > >\n> > > > > > /*\n> > > > > > * Test stop with queued requests\n> > > > > > *\n> > > > > > * Makes sure the camera supports a stop with requests queued. Example failure\n> > > > > > * is a camera that does not handle cancelation of buffers coming back from the\n> > > > > > * video device while stopping.\n> > > > > > */\n> > > > > >\n> > > > > > The rest looks good, thanks\n> > > > > > Reviewed-by: Jacopo Mondi \n> > > > >\n> > > > > And what about the name (`CaptureUnbalanced`)? Should that change as well?\n> > > > >\n> >\n> > Up to you. These tests have always been 'unbounded' rather than\n> > 'unbalanced' to me.\n>\n> ACK\n>\n>\n> Regards,\n> Barnabás Pőcze\n>\n>\n> >\n> > > > >\n> > > > > Regards,\n> > > > > Barnabás Pőcze\n> > > > >\n> > > > >\n> > > > > >\n> > > > > > > }\n> > > > > > >\n> > > > > > > INSTANTIATE_TEST_SUITE_P(CaptureTests,\n> > > > > > > --\n> > > > > > > 2.48.0\n> > > > > > >\n> > > > > > >\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 A7F5CC3220\n\tfor ;\n\tFri, 24 Jan 2025 13:16:47 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C760E6855D;\n\tFri, 24 Jan 2025 14:16:46 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2F99E68556\n\tfor ;\n\tFri, 24 Jan 2025 14:16:45 +0100 (CET)","from ideasonboard.com (93-61-96-190.ip145.fastwebnet.it\n\t[93.61.96.190])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id A8CF4465;\n\tFri, 24 Jan 2025 14:15:40 +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=\"H8WC9/G+\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1737724540;\n\tbh=juXRjajvRdYjRjGDQKiQWNF96e0oJ9C2eyEIAkz/WZI=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=H8WC9/G+L1s7OlVB3YEtng2gM5JUZ2hVCMXOBMK2MoV27xnMwwYF7CKixcON5+tiO\n\tlMLaK8hFDxjwwQQPEwf+ywYNcQ/tg0vaR9be+N+/aOjYcSsGVdEhFH/O0Yk2xnqsUn\n\tMcn2ahUohhRqWvC7LeSE/Ys8Vzm18Jh3VRUpBki4=","Date":"Fri, 24 Jan 2025 14:16:42 +0100","From":"Jacopo Mondi ","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Cc":"Jacopo Mondi , \n\tLaurent Pinchart ,\n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [RFC PATCH v2 13/16] apps: lc-compliance: Merge\n\t`CaptureBalanced` and `CaptureUnbalanced`","Message-ID":"","References":"<20250114182143.1773762-1-pobrn@protonmail.com>\n\t<20250114182143.1773762-14-pobrn@protonmail.com>\n\t\n\t\n\t\n\t\n\t<52ja3lcyhpzxmjj5of3xylewvoskzlw4eao6iolpq7sgw3xl72@br3gszzptj5t>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"","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":33158,"web_url":"https://patchwork.libcamera.org/comment/33158/","msgid":"","date":"2025-01-24T13:34:39","subject":"Re: [RFC PATCH v2 13/16] apps: lc-compliance: Merge\n\t`CaptureBalanced` and `CaptureUnbalanced`","submitter":{"id":133,"url":"https://patchwork.libcamera.org/api/people/133/","name":"Pőcze Barnabás","email":"pobrn@protonmail.com"},"content":"2025. január 24., péntek 14:16 keltezéssel, Jacopo Mondi írta:\n\n> Hi Barnabás\n> \n> On Fri, Jan 24, 2025 at 12:25:57PM +0000, Barnabás Pőcze wrote:\n> > 2025. január 24., péntek 12:59 keltezéssel, Jacopo Mondi írta:\n> >\n> > > Hi Barnabás\n> > >\n> > > On Fri, Jan 24, 2025 at 11:31:22AM +0000, Barnabás Pőcze wrote:\n> > > > 2025. január 24., péntek 12:21 keltezéssel, Jacopo Mondi írta:\n> > > >\n> > > > > Hi Barnabás\n> > > > >\n> > > > > On Fri, Jan 24, 2025 at 10:06:23AM +0000, Barnabás Pőcze wrote:\n> > > > > > Hi\n> > > > > >\n> > > > > >\n> > > > > > 2025. január 22., szerda 13:20 keltezéssel, Jacopo Mondi írta:\n> > > > > >\n> > > > > > > Hi Barnabás\n> > > > > > >\n> > > > > > > On Tue, Jan 14, 2025 at 06:22:52PM +0000, Barnabás Pőcze wrote:\n> > > > > > > > The above two classes implement very similar, in fact, the only essential\n> > > > > > >\n> > > > > > > s/implement/implementations are/ ?\n> > > > > > >\n> > > > > > > > difference is how many requests are queued. `CaptureBalanced` queues\n> > > > > > > > a predetermined number of requests, while `CaptureUnbalanced` queues\n> > > > > > > > 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> > > > > > > > ---\n> > > > > > > > src/apps/lc-compliance/helpers/capture.cpp | 141 +++++++-----------\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(+), 132 deletions(-)\n> > > > > > > >\n> > > > > > > > diff --git a/src/apps/lc-compliance/helpers/capture.cpp b/src/apps/lc-compliance/helpers/capture.cpp\n> > > > > > > > index 43db15d2d..77e87c9e4 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> > > > > > > > {\n> > > > > > > > }\n> > > > > > > > @@ -40,153 +42,110 @@ 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> > > > > > > Can loop_ be made a class member instead of being created here ?\n> > > > > > > Each test run will create an instance of the Capture class if I'm not\n> > > > > > > mistaken\n> > > > > >\n> > > > > > I agree it would be better, I would actually prefer using a single event loop\n> > > > > > during all tests if possible. But in order to do that, there would need to be a\n> > > > > > way to cancel the callbacks submitted with `EventLoop::callLater()`.\n> > > > > >\n> > > > > > One potential way to implement it would be to add a second argument to `callLater()`,\n> > > > > > named something like \"cookie\", and then add a new function like `cancelLater(cookie)`.\n> > > > > > Any ideas, comments?\n> > > > >\n> > > > > If cancelling the pending callbacks should happen at EvenLoop::exit()\n> > > > > time, can't we simply clear the calls_ queue there ?\n> > > >\n> > > > I suppose it depends on how generic or how lc-compliance specific solution is desired.\n> > > >\n> > >\n> > > What makes you think the solution would be lc-compliance specific ?\n> > > Every event loop will evenually be terminated by a call to exit(), am\n> > > I wrong ? And we should also clean up the queue in the destructor.\n> > > What am I missing ?\n> >\n> > The queue is already cleared when an `EventLoop` is destroyed.\n> >\n> > I can imagine scenarios where temporarily stopping and then restarting the event\n> > loop - in such a way that it is mostly transparent to the code running in it -\n> > might be desirable (e.g. migrating between threads).\n> >\n> > There is nothing that would prevent the restarting of the event loop today,\n> > and thus clearing the call queue may not always be desirable. But I suppose\n> > we could wait until there is an actual use case before addressing this.\n> \n> True that. I presume as long as we document that stopping an event\n> loop clears all the pending callback, I think it's fine for now.\n> \n> >\n> > Another thing, I think it would be a good thing if every component could use\n> > a single event loop implementation. In my opinion it is not ideal that libcamera\n> > proper has an event loop that is completely different from that of cam, lc-compliance.\n> >\n> \n> mmm, maybe I miss what you mean by \"component\", but cam and\n> lc-compliance are effectively applications using the libcamera API and\n> we don't want applications to run in the same even loop as libcamera ?\n\nOops, maybe I wasn't too clear. My point is that libcamera proper has an event\nloop implementation (`EventDispatcher[Poll]`), and the applications also have a\nseparate implementation (`EventLoop`). I think having two completely separate\nimplementations is likely not necessary.\n\n\n> \n> >\n> > >\n> > > For sure, I missed your question at the end of the previous email. See\n> > > below.\n> > >\n> > > >\n> > > > Regards,\n> > > > Barnabás Pőcze\n> > > >\n> > > >\n> > > > >\n> > > > > >\n> > > > > >\n> > > > > > >\n> > > > > > > >\n> > > > > > > > -void Capture::stop()\n> > > > > > > > -{\n> > > > > > > > -\tif (!config_ || !allocator_.allocated())\n> > > > > > > > -\t\treturn;\n> > > > > > > > +\tstart();\n> > > > > > > > +\tprepareRequests(queueLimit_);\n> > > > > > > >\n> > > > > > > > -\tcamera_->stop();\n> > > > > > > > +\tfor (const auto &request : requests_)\n> > > > > > > > +\t\tqueueRequest(request.get());\n> > > > > > > >\n> > > > > > > > -\tcamera_->requestCompleted.disconnect(this);\n> > > > > > > > +\tEXPECT_EQ(loop_->exec(), 0);\n> > > > > > > >\n> > > > > > > > -\tStream *stream = config_->at(0).stream();\n> > > > > > > > -\trequests_.clear();\n> > > > > > > > -\tallocator_.free(stream);\n> > > > > > > > -}\n> > > > > > > > +\tstop();\n> > > > > > > >\n> > > > > > > > -/* CaptureBalanced */\n> > > > > > > > +\tloop_ = nullptr;\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(std::optional queueLimit)\n> > > > > > >\n> > > > > > > queueLimit_ is a class member, initialized by the caller of this\n> > > > > > > function. Do you need to pass it in here ?\n> > > > > >\n> > > > > > Probably not.\n> > > > > >\n> > > > > >\n> > > > > > >\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> > > > > > > > +\tif (int ret = camera_->queueRequest(request); ret < 0)\n> > > > > > > > +\t\treturn ret;\n> > > > > > >\n> > > > > > > This is valid, but a bit unusual for the code base.\n> > > > > > >\n> > > > > > > int ret = camera_->queueRequest(request);\n> > > > > > > if (ret)\n> > > > > > > return ret;\n> > > > > > >\n> > > > > > > is more commmon.\n> > > > > >\n> > > > > > ACK\n> > > > > >\n> > > > > >\n> > > > > > >\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..173421fd2 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(std::optional queueLimit = {});\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> > > > > > > Can we remove \"Unbalanced\" from the test comment ? I always found it a\n> > > > > > > bit weird and now that we have removed the class with the\n> > > > > > > corresponding name I think the comment can be\n> > > > > > >\n> > > > > > > /*\n> > > > > > > * Test stop with queued requests\n> > > > > > > *\n> > > > > > > * Makes sure the camera supports a stop with requests queued. Example failure\n> > > > > > > * is a camera that does not handle cancelation of buffers coming back from the\n> > > > > > > * video device while stopping.\n> > > > > > > */\n> > > > > > >\n> > > > > > > The rest looks good, thanks\n> > > > > > > Reviewed-by: Jacopo Mondi \n> > > > > >\n> > > > > > And what about the name (`CaptureUnbalanced`)? Should that change as well?\n> > > > > >\n> > >\n> > > Up to you. These tests have always been 'unbounded' rather than\n> > > 'unbalanced' to me.\n> >\n> > ACK\n> >\n> >\n> > Regards,\n> > Barnabás Pőcze\n> >\n> >\n> > >\n> > > > > >\n> > > > > > Regards,\n> > > > > > Barnabás Pőcze\n> > > > > >\n> > > > > >\n> > > > > > >\n> > > > > > > > }\n> > > > > > > >\n> > > > > > > > INSTANTIATE_TEST_SUITE_P(CaptureTests,\n> > > > > > > > --\n> > > > > > > > 2.48.0\n> > > > > > > >\n> > > > > > > >\n> > > > > > >\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 A612DC322E\n\tfor ;\n\tFri, 24 Jan 2025 13:34:48 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 861596855D;\n\tFri, 24 Jan 2025 14:34:47 +0100 (CET)","from mail-10629.protonmail.ch (mail-10629.protonmail.ch\n\t[79.135.106.29])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2360668556\n\tfor ;\n\tFri, 24 Jan 2025 14:34:46 +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=\"XWwhyMlD\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=protonmail.com;\n\ts=protonmail3; t=1737725684; x=1737984884;\n\tbh=tXs9nlXEiyCNVWSBLZbBG0BBWB6zlDUWwtTzutnbUcg=;\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=XWwhyMlDgHDhDE7T+Wx7CrCB7nFpyuAtNA3dbA+j96WPp3TI6e79m7kOUV/KE8Mlc\n\tNfpqwicmpBjzhP6bk6VzeMi6Svas6n1dohZpFFoFCHJQpMyXwuhB9+fAedTfjUmeLH\n\tsJqBBGg0s7xA1kiEtekyx7ghiPAplLVHyVPbwin9wLQwwR+4OqCVoJdsoUVbif3LhG\n\taiN/ZwBygWSI8tYZ4JUuRSY0agT3zSb7FOrboRrb1CeKv4kmlEgz6fWJDW1j3N3HCp\n\tR55uCNEPpXpGSMMSiynNEgPtHdDcJ77Oc4YUuz3vHuJRctxqcddF+mQd32znbuo91s\n\tQLlmLrBhawauA==","Date":"Fri, 24 Jan 2025 13:34:39 +0000","To":"Jacopo Mondi ","From":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Cc":"Laurent Pinchart ,\n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [RFC PATCH v2 13/16] apps: lc-compliance: Merge\n\t`CaptureBalanced` and `CaptureUnbalanced`","Message-ID":"","In-Reply-To":"","References":"<20250114182143.1773762-1-pobrn@protonmail.com>\n\t<20250114182143.1773762-14-pobrn@protonmail.com>\n\t\n\t\n\t\n\t\n\t<52ja3lcyhpzxmjj5of3xylewvoskzlw4eao6iolpq7sgw3xl72@br3gszzptj5t>\n\t\n\t","Feedback-ID":"20568564:user:proton","X-Pm-Message-ID":"33dcb34fb1c9a1bd2d4bda9ccd3d6ab8946de329","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\" "}}]