[{"id":33629,"web_url":"https://patchwork.libcamera.org/comment/33629/","msgid":"","date":"2025-03-18T08:43:04","subject":"Re: [PATCH v4 1/2] apps: lc-compliance: Support multiple streams in\n\thelpers","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, Mar 14, 2025 at 06:42:18PM +0100, Barnabás Pőcze wrote:\n> Prepare to add a test suite for capture operations with multiple\n> streams.\n>\n> Modify the Capture helper class to support multiple roles and streams\n> in the configure() and capture() operations. The buffer count\n\ns/capture()/start()/ ?\n\n> of each stream is asserted to be the same.\n>\n> Multi-stream support will be added in next patches.\n>\n> Signed-off-by: Barnabás Pőcze \n> ---\n> src/apps/lc-compliance/helpers/capture.cpp | 64 ++++++++++++++-----\n> src/apps/lc-compliance/helpers/capture.h | 2 +-\n> src/apps/lc-compliance/tests/capture_test.cpp | 6 +-\n> 3 files changed, 51 insertions(+), 21 deletions(-)\n>\n> diff --git a/src/apps/lc-compliance/helpers/capture.cpp b/src/apps/lc-compliance/helpers/capture.cpp\n> index f2c6d58ce..2a3fa3b68 100644\n> --- a/src/apps/lc-compliance/helpers/capture.cpp\n> +++ b/src/apps/lc-compliance/helpers/capture.cpp\n> @@ -23,12 +23,29 @@ Capture::~Capture()\n> \tstop();\n> }\n>\n> -void Capture::configure(StreamRole role)\n> +void Capture::configure(libcamera::Span roles)\n> {\n> -\tconfig_ = camera_->generateConfiguration({ role });\n> +\tassert(!roles.empty());\n>\n> +\tconfig_ = camera_->generateConfiguration(roles);\n> \tif (!config_)\n> -\t\tGTEST_SKIP() << \"Role not supported by camera\";\n> +\t\tGTEST_SKIP() << \"Roles not supported by camera\";\n> +\n> +\tASSERT_EQ(config_->size(), roles.size()) << \"Unexpected number of streams in configuration\";\n> +\n> +\t/*\n> +\t * Set the buffers count to the largest value across all streams.\n> +\t * \\todo: Should all streams from a Camera have the same buffer count ?\n> +\t */\n> +\tauto largest =\n> +\t\tstd::max_element(config_->begin(), config_->end(),\n> +\t\t\t\t [](const StreamConfiguration &l, const StreamConfiguration &r)\n> +\t\t\t\t { return l.bufferCount < r.bufferCount; });\n> +\n> +\tassert(largest != config_->end());\n\nThe only way this could happen if config is empty, something you check\nfor a few lines above\n\n> +\n> +\tfor (auto &cfg : *config_)\n> +\t\tcfg.bufferCount = largest->bufferCount;\n>\n> \tif (config_->validate() != CameraConfiguration::Valid) {\n> \t\tconfig_.reset();\n> @@ -103,29 +120,37 @@ void Capture::start()\n> \tassert(!allocator_.allocated());\n> \tassert(requests_.empty());\n>\n> -\tStream *stream = config_->at(0).stream();\n> -\tint count = allocator_.allocate(stream);\n> -\n> -\tASSERT_GE(count, 0) << \"Failed to allocate buffers\";\n> -\tEXPECT_EQ(count, config_->at(0).bufferCount) << \"Allocated less buffers than expected\";\n> -\n> -\tconst std::vector> &buffers = allocator_.buffers(stream);\n> +\tconst auto bufferCount = config_->at(0).bufferCount;\n\nThis hunk changes the logic to use bufferCount in place of buffers.size()\n\nHowever, the code made sure that\n EXPECT_EQ(count, config_->at(0).bufferCount)\n\nso indeed the two are equivalent\n\n>\n> \t/* No point in testing less requests then the camera depth. */\n> -\tif (queueLimit_ && *queueLimit_ < buffers.size()) {\n> -\t\tGTEST_SKIP() << \"Camera needs \" << buffers.size()\n> +\tif (queueLimit_ && *queueLimit_ < bufferCount) {\n> +\t\tGTEST_SKIP() << \"Camera needs \" << bufferCount\n> \t\t\t << \" requests, can't test only \" << *queueLimit_;\n> \t}\n>\n> -\tfor (const std::unique_ptr &buffer : buffers) {\n> +\tfor (std::size_t i = 0; i < bufferCount; i++) {\n> \t\tstd::unique_ptr request = camera_->createRequest();\n> \t\tASSERT_TRUE(request) << \"Can't create request\";\n> +\t\trequests_.push_back(std::move(request));\n> +\t}\n>\n> -\t\tASSERT_EQ(request->addBuffer(stream, buffer.get()), 0) << \"Can't set buffer for request\";\n> +\tfor (const auto &cfg : *config_) {\n> +\t\tStream *stream = cfg.stream();\n>\n> -\t\trequests_.push_back(std::move(request));\n> +\t\tint count = allocator_.allocate(stream);\n> +\t\tASSERT_GE(count, 0) << \"Failed to allocate buffers\";\n> +\n> +\t\tconst auto &buffers = allocator_.buffers(stream);\n> +\t\tASSERT_EQ(buffers.size(), bufferCount) << \"Mismatching buffer count\";\n> +\n> +\t\tfor (std::size_t i = 0; i < bufferCount; i++) {\n> +\t\t\tASSERT_EQ(requests_[i]->addBuffer(stream, buffers[i].get()), 0)\n> +\t\t\t\t<< \"Failed to add buffer to request\";\n> +\t\t}\n> \t}\n> +\tASSERT_TRUE(allocator_.allocated());\n\nThis has been validated by the above assretions for count > 0\n\n> +\n> \tcamera_->requestCompleted.connect(this, &Capture::requestComplete);\n>\n> \tASSERT_EQ(camera_->start(), 0) << \"Failed to start camera\";\n> @@ -140,7 +165,12 @@ void Capture::stop()\n>\n> \tcamera_->requestCompleted.disconnect(this);\n>\n> -\tStream *stream = config_->at(0).stream();\n> \trequests_.clear();\n> -\tallocator_.free(stream);\n> +\n> +\tfor (const auto &cfg : *config_) {\n> +\t\tEXPECT_EQ(allocator_.free(cfg.stream()), 0)\n> +\t\t\t<< \"Failed to free buffers associated with stream\";\n> +\t}\n> +\n> +\tEXPECT_FALSE(allocator_.allocated());\n\n> }\n> diff --git a/src/apps/lc-compliance/helpers/capture.h b/src/apps/lc-compliance/helpers/capture.h\n> index 0e7b848fb..ea01c11c1 100644\n> --- a/src/apps/lc-compliance/helpers/capture.h\n> +++ b/src/apps/lc-compliance/helpers/capture.h\n> @@ -20,7 +20,7 @@ public:\n> \tCapture(std::shared_ptr camera);\n> \t~Capture();\n>\n> -\tvoid configure(libcamera::StreamRole role);\n> +\tvoid configure(libcamera::Span roles);\n> \tvoid run(unsigned int captureLimit, std::optional queueLimit = {});\n>\n> private:\n> diff --git a/src/apps/lc-compliance/tests/capture_test.cpp b/src/apps/lc-compliance/tests/capture_test.cpp\n> index 93bed48f0..06f15bdb4 100644\n> --- a/src/apps/lc-compliance/tests/capture_test.cpp\n> +++ b/src/apps/lc-compliance/tests/capture_test.cpp\n> @@ -89,7 +89,7 @@ TEST_P(SingleStream, Capture)\n>\n> \tCapture capture(camera_);\n>\n> -\tcapture.configure(role);\n> +\tcapture.configure({ { role } });\n>\n> \tcapture.run(numRequests, numRequests);\n> }\n> @@ -108,7 +108,7 @@ TEST_P(SingleStream, CaptureStartStop)\n>\n> \tCapture capture(camera_);\n>\n> -\tcapture.configure(role);\n> +\tcapture.configure({ { role } });\n>\n> \tfor (unsigned int starts = 0; starts < numRepeats; starts++)\n> \t\tcapture.run(numRequests, numRequests);\n> @@ -127,7 +127,7 @@ TEST_P(SingleStream, UnbalancedStop)\n>\n> \tCapture capture(camera_);\n>\n> -\tcapture.configure(role);\n> +\tcapture.configure({ { role } });\n>\n> \tcapture.run(numRequests);\n\nPatch looks good!\n\nReviewed-by: Jacopo Mondi \n\nThanks\n j\n\n> }\n> --\n> 2.48.1\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 98085C32F5\n\tfor ;\n\tTue, 18 Mar 2025 08:43:10 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5FF1E68947;\n\tTue, 18 Mar 2025 09:43:09 +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 4C4E7687FA\n\tfor ;\n\tTue, 18 Mar 2025 09:43:08 +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 B2943A2F;\n\tTue, 18 Mar 2025 09:41:25 +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=\"dEq+3CBA\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1742287285;\n\tbh=AzXwiHIApqDM4bQKfS2mpuxTb9iHkFafuqLUOjQ4rR8=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=dEq+3CBA0jgubvgjINt7Ldc4/ukQ66+n1uzDIZYvKasyshBd6kKKufEeB7HDWBuip\n\tMNiIMPOY9VYta2jaPccyf9c848S5XGkC0V2DPGRV76f+yfME6CuTS7LO2Y6L8U77TC\n\t51db6GkZVICkLxeg+aRcBY7+c0vICu+YxEAD2jOM=","Date":"Tue, 18 Mar 2025 09:43:04 +0100","From":"Jacopo Mondi ","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v4 1/2] apps: lc-compliance: Support multiple streams in\n\thelpers","Message-ID":"","References":"<20250314174220.1015597-1-barnabas.pocze@ideasonboard.com>\n\t<20250314174220.1015597-2-barnabas.pocze@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20250314174220.1015597-2-barnabas.pocze@ideasonboard.com>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]