[{"id":33005,"web_url":"https://patchwork.libcamera.org/comment/33005/","msgid":"<20250110013254.GM6159@pendragon.ideasonboard.com>","date":"2025-01-10T01:32:54","subject":"Re: [RFC PATCH v1 11/12] apps: lc-compliance: Support multiple\n\tstreams in helpers","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Barnabás,\n\nThank you for the patch.\n\nOn Fri, Dec 20, 2024 at 03:08:51PM +0000, 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.\n> \n> Multi-stream support will be added in next patches.\n> \n> Co-developed-by: Jacopo Mondi \n> Signed-off-by: Jacopo Mondi \n> Signed-off-by: Barnabás Pőcze \n> ---\n> src/apps/lc-compliance/helpers/capture.cpp | 83 ++++++++++++++-----\n> src/apps/lc-compliance/helpers/capture.h | 2 +-\n> src/apps/lc-compliance/tests/capture_test.cpp | 6 +-\n> 3 files changed, 66 insertions(+), 25 deletions(-)\n> \n> diff --git a/src/apps/lc-compliance/helpers/capture.cpp b/src/apps/lc-compliance/helpers/capture.cpp\n> index 7a05be9a3..38edb6f28 100644\n> --- a/src/apps/lc-compliance/helpers/capture.cpp\n> +++ b/src/apps/lc-compliance/helpers/capture.cpp\n> @@ -24,15 +24,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> \n> \tif (!config_) {\n> \t\tstd::cout << \"Role not supported by camera\" << std::endl;\n> \t\tGTEST_SKIP();\n> \t}\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\nWould this (untested) be clearer/simpler ?\n\n\tunsigned int maxBufferCount =\n\t\tstd::reduce(config_->begin(), config_->end(), 0,\n\t\t\t [](unsigned int a, const StreamConfiguration &b) {\n\t\t\t\t return std::max(a, b->bufferCount);\n\t\t\t });\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> \t\tFAIL() << \"Configuration not valid\";\n> @@ -46,12 +60,20 @@ void Capture::configure(StreamRole role)\n> \n> void Capture::start()\n> {\n> -\tStream *stream = config_->at(0).stream();\n> -\tint count = allocator_.allocate(stream);\n> +\tassert(config_);\n> +\tassert(!config_->empty());\n> +\tassert(!allocator_.allocated());\n> +\n> +\tfor (const auto &cfg : *config_) {\n> +\t\tStream *stream = cfg.stream();\n> +\t\tint count = allocator_.allocate(stream);\n> +\n> +\t\tASSERT_GE(count, 0) << \"Failed to allocate buffers\";\n> +\t\tEXPECT_EQ(count, cfg.bufferCount) << \"Allocated less buffers than expected\";\n> +\t\tASSERT_EQ(count, allocator_.buffers(stream).size()) << \"Unexpected number of buffers in allocator\";\n> +\t}\n> \n> -\tASSERT_GE(count, 0) << \"Failed to allocate buffers\";\n> -\tEXPECT_EQ(count, config_->at(0).bufferCount) << \"Allocated less buffers than expected\";\n> -\tASSERT_EQ(count, allocator_.buffers(stream).size()) << \"Unexpected number of buffers in allocator\";\n> +\tASSERT_TRUE(allocator_.allocated());\n> \n> \tcamera_->requestCompleted.connect(this, &Capture::requestComplete);\n> \n> @@ -71,9 +93,14 @@ 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\tint res = allocator_.free(cfg.stream());\n\ns/res/ret/ for consistency.\n\n> +\t\tASSERT_EQ(res, 0) << \"Failed to free buffers associated with stream\";\n> +\t}\n> +\n> +\tASSERT_FALSE(allocator_.allocated());\n> }\n> \n> void Capture::prepareRequests(unsigned int plannedRequests)\n> @@ -81,22 +108,36 @@ void Capture::prepareRequests(unsigned int plannedRequests)\n> \tassert(config_);\n> \tassert(requests_.empty());\n> \n> -\tStream *stream = config_->at(0).stream();\n> -\tconst std::vector> &buffers = allocator_.buffers(stream);\n> +\tstd::size_t maxBuffers = 0;\n> \n> -\t/* No point in testing less requests then the camera depth. */\n> -\tif (plannedRequests < buffers.size()) {\n> -\t\tstd::cout << \"Camera needs \" << buffers.size()\n> -\t\t\t << \" requests, can't test only \"\n> -\t\t\t << plannedRequests << std::endl;\n> -\t\tGTEST_SKIP();\n> +\tfor (const auto &cfg : *config_) {\n> +\t\tconst auto &buffers = allocator_.buffers(cfg.stream());\n> +\t\tASSERT_FALSE(buffers.empty()) << \"Zero buffers allocated for stream\";\n> +\n> +\t\t/* No point in testing less requests then the camera depth. */\n> +\t\tif (plannedRequests < buffers.size()) {\n> +\t\t\tstd::cout << \"Camera needs \" << buffers.size()\n> +\t\t\t\t << \" requests, can't test only \"\n> +\t\t\t\t << plannedRequests << std::endl;\n> +\t\t\tGTEST_SKIP();\n> +\t\t}\n\nI think this could be moved after the loop, testing\n\n\t\tif (plannedRequests < maxBuffers)\n\n> +\n> +\t\tmaxBuffers = std::max(maxBuffers, buffers.size());\n> \t}\n> \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> +\tfor (std::size_t i = 0; i < maxBuffers; i++) {\n> +\t\tstd::unique_ptr request = camera_->createRequest(i);\n> +\n> +\t\tfor (const auto &cfg : *config_) {\n> +\t\t\tStream *stream = cfg.stream();\n> +\t\t\tconst auto &buffers = allocator_.buffers(stream);\n> +\t\t\tassert(!buffers.empty());\n> \n> -\t\tASSERT_EQ(request->addBuffer(stream, buffer.get()), 0) << \"Can't set buffer for request\";\n> +\t\t\tif (i < buffers.size()) {\n\n\t\t\tif (i >= buffers.size())\n\t\t\t\tbreak;\n\n\t\t\tASSERT_EQ(request->addBuffer(stream, buffers[i].get()), 0)\n\t\t\t\t<< \"Can't add buffer to request\";\n\nReviewed-by: Laurent Pinchart \n\n> +\t\t\t\tASSERT_EQ(request->addBuffer(stream, buffers[i].get()), 0)\n> +\t\t\t\t\t<< \"Can't add buffer to request\";\n> +\t\t\t}\n> +\t\t}\n> \n> \t\trequests_.push_back(std::move(request));\n> \t}\n> diff --git a/src/apps/lc-compliance/helpers/capture.h b/src/apps/lc-compliance/helpers/capture.h\n> index 67c29021b..b3a390941 100644\n> --- a/src/apps/lc-compliance/helpers/capture.h\n> +++ b/src/apps/lc-compliance/helpers/capture.h\n> @@ -17,7 +17,7 @@\n> class Capture\n> {\n> public:\n> -\tvoid configure(libcamera::StreamRole role);\n> +\tvoid configure(libcamera::Span roles);\n> \n> protected:\n> \tCapture(std::shared_ptr camera);\n> diff --git a/src/apps/lc-compliance/tests/capture_test.cpp b/src/apps/lc-compliance/tests/capture_test.cpp\n> index 97465a612..c382fcf47 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> \tCaptureBalanced capture(camera_);\n> \n> -\tcapture.configure(role);\n> +\tcapture.configure(std::array{ role });\n> \n> \tcapture.capture(numRequests);\n> }\n> @@ -108,7 +108,7 @@ TEST_P(SingleStream, CaptureStartStop)\n> \n> \tCaptureBalanced capture(camera_);\n> \n> -\tcapture.configure(role);\n> +\tcapture.configure(std::array{ role });\n> \n> \tfor (unsigned int starts = 0; starts < numRepeats; starts++)\n> \t\tcapture.capture(numRequests);\n> @@ -127,7 +127,7 @@ TEST_P(SingleStream, UnbalancedStop)\n> \n> \tCaptureUnbalanced capture(camera_);\n> \n> -\tcapture.configure(role);\n> +\tcapture.configure(std::array{ role });\n> \n> \tcapture.capture(numRequests);\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 7BA8AC32F5\n\tfor ;\n\tFri, 10 Jan 2025 01:33:00 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 824B8684EA;\n\tFri, 10 Jan 2025 02:32:59 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E7969608A9\n\tfor ;\n\tFri, 10 Jan 2025 02:32:57 +0100 (CET)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id B2641CE6;\n\tFri, 10 Jan 2025 02:32:03 +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=\"idIM5YPv\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1736472723;\n\tbh=bNtYeOAbIaLmAe2Jlupl41xAqrpcj0YHmwjC/3jrXpY=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=idIM5YPvMiWbPL1m6NvlnsgnbmAMQOa0pAHFZ36YA2IFECjDPiuyJNgnmZZ6KTLKB\n\tI5JR5dEeGKbL4ShW/ANkLXSfnSrEK0HzMKy+G8eKnPcRN40WoA5FY7VOASpa0KVelQ\n\tp1IKzoC3tuEkUaDpPth0cM2KM8NOdbIA4eAFiDUY=","Date":"Fri, 10 Jan 2025 03:32:54 +0200","From":"Laurent Pinchart ","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Cc":"libcamera-devel@lists.libcamera.org,\n\tJacopo Mondi ","Subject":"Re: [RFC PATCH v1 11/12] apps: lc-compliance: Support multiple\n\tstreams in helpers","Message-ID":"<20250110013254.GM6159@pendragon.ideasonboard.com>","References":"<20241220150759.709756-1-pobrn@protonmail.com>\n\t<20241220150759.709756-12-pobrn@protonmail.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20241220150759.709756-12-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":33009,"web_url":"https://patchwork.libcamera.org/comment/33009/","msgid":"","date":"2025-01-10T09:27:24","subject":"Re: [RFC PATCH v1 11/12] apps: lc-compliance: Support multiple\n\tstreams in helpers","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 10., péntek 2:32 keltezéssel, Laurent Pinchart írta:\n\n> Hi Barnabás,\n> \n> Thank you for the patch.\n> \n> On Fri, Dec 20, 2024 at 03:08:51PM +0000, 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.\n> >\n> > Multi-stream support will be added in next patches.\n> >\n> > Co-developed-by: Jacopo Mondi \n> > Signed-off-by: Jacopo Mondi \n> > Signed-off-by: Barnabás Pőcze \n> > ---\n> > src/apps/lc-compliance/helpers/capture.cpp | 83 ++++++++++++++-----\n> > src/apps/lc-compliance/helpers/capture.h | 2 +-\n> > src/apps/lc-compliance/tests/capture_test.cpp | 6 +-\n> > 3 files changed, 66 insertions(+), 25 deletions(-)\n> >\n> > diff --git a/src/apps/lc-compliance/helpers/capture.cpp b/src/apps/lc-compliance/helpers/capture.cpp\n> > index 7a05be9a3..38edb6f28 100644\n> > --- a/src/apps/lc-compliance/helpers/capture.cpp\n> > +++ b/src/apps/lc-compliance/helpers/capture.cpp\n> > @@ -24,15 +24,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> >\n> > \tif (!config_) {\n> > \t\tstd::cout << \"Role not supported by camera\" << std::endl;\n> > \t\tGTEST_SKIP();\n> > \t}\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> Would this (untested) be clearer/simpler ?\n> \n> \tunsigned int maxBufferCount =\n> \t\tstd::reduce(config_->begin(), config_->end(), 0,\n> \t\t\t [](unsigned int a, const StreamConfiguration &b) {\n> \t\t\t\t return std::max(a, b->bufferCount);\n> \t\t\t });\n\nTo me both look about the same in terms of readability.\n\n\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> > \t\tFAIL() << \"Configuration not valid\";\n> > @@ -46,12 +60,20 @@ void Capture::configure(StreamRole role)\n> >\n> > void Capture::start()\n> > {\n> > -\tStream *stream = config_->at(0).stream();\n> > -\tint count = allocator_.allocate(stream);\n> > +\tassert(config_);\n> > +\tassert(!config_->empty());\n> > +\tassert(!allocator_.allocated());\n> > +\n> > +\tfor (const auto &cfg : *config_) {\n> > +\t\tStream *stream = cfg.stream();\n> > +\t\tint count = allocator_.allocate(stream);\n> > +\n> > +\t\tASSERT_GE(count, 0) << \"Failed to allocate buffers\";\n> > +\t\tEXPECT_EQ(count, cfg.bufferCount) << \"Allocated less buffers than expected\";\n> > +\t\tASSERT_EQ(count, allocator_.buffers(stream).size()) << \"Unexpected number of buffers in allocator\";\n> > +\t}\n> >\n> > -\tASSERT_GE(count, 0) << \"Failed to allocate buffers\";\n> > -\tEXPECT_EQ(count, config_->at(0).bufferCount) << \"Allocated less buffers than expected\";\n> > -\tASSERT_EQ(count, allocator_.buffers(stream).size()) << \"Unexpected number of buffers in allocator\";\n> > +\tASSERT_TRUE(allocator_.allocated());\n> >\n> > \tcamera_->requestCompleted.connect(this, &Capture::requestComplete);\n> >\n> > @@ -71,9 +93,14 @@ 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\tint res = allocator_.free(cfg.stream());\n> \n> s/res/ret/ for consistency.\n> \n> > +\t\tASSERT_EQ(res, 0) << \"Failed to free buffers associated with stream\";\n> > +\t}\n> > +\n> > +\tASSERT_FALSE(allocator_.allocated());\n> > }\n> >\n> > void Capture::prepareRequests(unsigned int plannedRequests)\n> > @@ -81,22 +108,36 @@ void Capture::prepareRequests(unsigned int plannedRequests)\n> > \tassert(config_);\n> > \tassert(requests_.empty());\n> >\n> > -\tStream *stream = config_->at(0).stream();\n> > -\tconst std::vector> &buffers = allocator_.buffers(stream);\n> > +\tstd::size_t maxBuffers = 0;\n> >\n> > -\t/* No point in testing less requests then the camera depth. */\n> > -\tif (plannedRequests < buffers.size()) {\n> > -\t\tstd::cout << \"Camera needs \" << buffers.size()\n> > -\t\t\t << \" requests, can't test only \"\n> > -\t\t\t << plannedRequests << std::endl;\n> > -\t\tGTEST_SKIP();\n> > +\tfor (const auto &cfg : *config_) {\n> > +\t\tconst auto &buffers = allocator_.buffers(cfg.stream());\n> > +\t\tASSERT_FALSE(buffers.empty()) << \"Zero buffers allocated for stream\";\n> > +\n> > +\t\t/* No point in testing less requests then the camera depth. */\n> > +\t\tif (plannedRequests < buffers.size()) {\n> > +\t\t\tstd::cout << \"Camera needs \" << buffers.size()\n> > +\t\t\t\t << \" requests, can't test only \"\n> > +\t\t\t\t << plannedRequests << std::endl;\n> > +\t\t\tGTEST_SKIP();\n> > +\t\t}\n> \n> I think this could be moved after the loop, testing\n> \n> \t\tif (plannedRequests < maxBuffers)\n> \n> > +\n> > +\t\tmaxBuffers = std::max(maxBuffers, buffers.size());\n> > \t}\n> >\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> > +\tfor (std::size_t i = 0; i < maxBuffers; i++) {\n> > +\t\tstd::unique_ptr request = camera_->createRequest(i);\n> > +\n> > +\t\tfor (const auto &cfg : *config_) {\n> > +\t\t\tStream *stream = cfg.stream();\n> > +\t\t\tconst auto &buffers = allocator_.buffers(stream);\n> > +\t\t\tassert(!buffers.empty());\n> >\n> > -\t\tASSERT_EQ(request->addBuffer(stream, buffer.get()), 0) << \"Can't set buffer for request\";\n> > +\t\t\tif (i < buffers.size()) {\n> \n> \t\t\tif (i >= buffers.size())\n> \t\t\t\tbreak;\n> \n> \t\t\tASSERT_EQ(request->addBuffer(stream, buffers[i].get()), 0)\n> \t\t\t\t<< \"Can't add buffer to request\";\n\nI think that changes the behaviour, but I believe `continue` would work.\n\n\nRegards,\nBarnabás Pőcze\n\n> \n> Reviewed-by: Laurent Pinchart \n> \n> > +\t\t\t\tASSERT_EQ(request->addBuffer(stream, buffers[i].get()), 0)\n> > +\t\t\t\t\t<< \"Can't add buffer to request\";\n> > +\t\t\t}\n> > +\t\t}\n> >\n> > \t\trequests_.push_back(std::move(request));\n> > \t}\n> > diff --git a/src/apps/lc-compliance/helpers/capture.h b/src/apps/lc-compliance/helpers/capture.h\n> > index 67c29021b..b3a390941 100644\n> > --- a/src/apps/lc-compliance/helpers/capture.h\n> > +++ b/src/apps/lc-compliance/helpers/capture.h\n> > @@ -17,7 +17,7 @@\n> > class Capture\n> > {\n> > public:\n> > -\tvoid configure(libcamera::StreamRole role);\n> > +\tvoid configure(libcamera::Span roles);\n> >\n> > protected:\n> > \tCapture(std::shared_ptr camera);\n> > diff --git a/src/apps/lc-compliance/tests/capture_test.cpp b/src/apps/lc-compliance/tests/capture_test.cpp\n> > index 97465a612..c382fcf47 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> > \tCaptureBalanced capture(camera_);\n> >\n> > -\tcapture.configure(role);\n> > +\tcapture.configure(std::array{ role });\n> >\n> > \tcapture.capture(numRequests);\n> > }\n> > @@ -108,7 +108,7 @@ TEST_P(SingleStream, CaptureStartStop)\n> >\n> > \tCaptureBalanced capture(camera_);\n> >\n> > -\tcapture.configure(role);\n> > +\tcapture.configure(std::array{ role });\n> >\n> > \tfor (unsigned int starts = 0; starts < numRepeats; starts++)\n> > \t\tcapture.capture(numRequests);\n> > @@ -127,7 +127,7 @@ TEST_P(SingleStream, UnbalancedStop)\n> >\n> > \tCaptureUnbalanced capture(camera_);\n> >\n> > -\tcapture.configure(role);\n> > +\tcapture.configure(std::array{ role });\n> >\n> > \tcapture.capture(numRequests);\n> > }\n> \n> --\n> Regards,\n> \n> Laurent Pinchart\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 23E81C32EF\n\tfor ;\n\tFri, 10 Jan 2025 09:27:32 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E8B92684EA;\n\tFri, 10 Jan 2025 10:27:30 +0100 (CET)","from mail-4316.protonmail.ch (mail-4316.protonmail.ch\n\t[185.70.43.16])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A858361882\n\tfor ;\n\tFri, 10 Jan 2025 10:27: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=\"qr0KK7d4\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=protonmail.com;\n\ts=protonmail3; t=1736501248; x=1736760448;\n\tbh=L+lGhp34TbGXSA8LhbcDLMWZebafSj8bHmdyPtNoiRs=;\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=qr0KK7d41Y3q01YnYEh9rco6DM+0c1Mzql8FyQx5uv7ZsDQ/zZoGO44K5HwvvummM\n\tPhmwerlD1tBy9FvgZQuz4+XxNWUvuYCht2Lw9Z8BYeD0GzA2Vjz+L+bt+phr2V2zrI\n\tMkZYOwzTyais/0BtYffWGomglAh5PA8eObYfoiKP3Kfyd8zKpLW9eaTtrcpRPQYF45\n\tkb4zhgBCPD4evLAS0O3SF335iHhqcm13RhXFE+67/4EV5W3m1iAoab02ao3dwNTgCw\n\tHtQPVAY81GvVd1NXvURCaCthmit7fdEB3EHbxP3sg3rqUD9OiBFKNrIBylXe0iuTS4\n\tYKyNc2kD59QlA==","Date":"Fri, 10 Jan 2025 09:27:24 +0000","To":"Laurent Pinchart ","From":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Cc":"libcamera-devel@lists.libcamera.org,\n\tJacopo Mondi ","Subject":"Re: [RFC PATCH v1 11/12] apps: lc-compliance: Support multiple\n\tstreams in helpers","Message-ID":"","In-Reply-To":"<20250110013254.GM6159@pendragon.ideasonboard.com>","References":"<20241220150759.709756-1-pobrn@protonmail.com>\n\t<20241220150759.709756-12-pobrn@protonmail.com>\n\t<20250110013254.GM6159@pendragon.ideasonboard.com>","Feedback-ID":"20568564:user:proton","X-Pm-Message-ID":"2e7e725af14b8defdd84ab9f69059bcc855d00cc","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":33012,"web_url":"https://patchwork.libcamera.org/comment/33012/","msgid":"<20250110094135.GA6611@pendragon.ideasonboard.com>","date":"2025-01-10T09:41:35","subject":"Re: [RFC PATCH v1 11/12] apps: lc-compliance: Support multiple\n\tstreams in helpers","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Fri, Jan 10, 2025 at 09:27:24AM +0000, Barnabás Pőcze wrote:\n> Hi\n> \n> \n> 2025. január 10., péntek 2:32 keltezéssel, Laurent Pinchart írta:\n> \n> > Hi Barnabás,\n> > \n> > Thank you for the patch.\n> > \n> > On Fri, Dec 20, 2024 at 03:08:51PM +0000, 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.\n> > >\n> > > Multi-stream support will be added in next patches.\n> > >\n> > > Co-developed-by: Jacopo Mondi \n> > > Signed-off-by: Jacopo Mondi \n> > > Signed-off-by: Barnabás Pőcze \n> > > ---\n> > > src/apps/lc-compliance/helpers/capture.cpp | 83 ++++++++++++++-----\n> > > src/apps/lc-compliance/helpers/capture.h | 2 +-\n> > > src/apps/lc-compliance/tests/capture_test.cpp | 6 +-\n> > > 3 files changed, 66 insertions(+), 25 deletions(-)\n> > >\n> > > diff --git a/src/apps/lc-compliance/helpers/capture.cpp b/src/apps/lc-compliance/helpers/capture.cpp\n> > > index 7a05be9a3..38edb6f28 100644\n> > > --- a/src/apps/lc-compliance/helpers/capture.cpp\n> > > +++ b/src/apps/lc-compliance/helpers/capture.cpp\n> > > @@ -24,15 +24,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> > >\n> > > \tif (!config_) {\n> > > \t\tstd::cout << \"Role not supported by camera\" << std::endl;\n> > > \t\tGTEST_SKIP();\n> > > \t}\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> > Would this (untested) be clearer/simpler ?\n> > \n> > \tunsigned int maxBufferCount =\n> > \t\tstd::reduce(config_->begin(), config_->end(), 0,\n> > \t\t\t [](unsigned int a, const StreamConfiguration &b) {\n> > \t\t\t\t return std::max(a, b->bufferCount);\n> > \t\t\t });\n> \n> To me both look about the same in terms of readability.\n> \n> \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> > > \t\tFAIL() << \"Configuration not valid\";\n> > > @@ -46,12 +60,20 @@ void Capture::configure(StreamRole role)\n> > >\n> > > void Capture::start()\n> > > {\n> > > -\tStream *stream = config_->at(0).stream();\n> > > -\tint count = allocator_.allocate(stream);\n> > > +\tassert(config_);\n> > > +\tassert(!config_->empty());\n> > > +\tassert(!allocator_.allocated());\n> > > +\n> > > +\tfor (const auto &cfg : *config_) {\n> > > +\t\tStream *stream = cfg.stream();\n> > > +\t\tint count = allocator_.allocate(stream);\n> > > +\n> > > +\t\tASSERT_GE(count, 0) << \"Failed to allocate buffers\";\n> > > +\t\tEXPECT_EQ(count, cfg.bufferCount) << \"Allocated less buffers than expected\";\n> > > +\t\tASSERT_EQ(count, allocator_.buffers(stream).size()) << \"Unexpected number of buffers in allocator\";\n> > > +\t}\n> > >\n> > > -\tASSERT_GE(count, 0) << \"Failed to allocate buffers\";\n> > > -\tEXPECT_EQ(count, config_->at(0).bufferCount) << \"Allocated less buffers than expected\";\n> > > -\tASSERT_EQ(count, allocator_.buffers(stream).size()) << \"Unexpected number of buffers in allocator\";\n> > > +\tASSERT_TRUE(allocator_.allocated());\n> > >\n> > > \tcamera_->requestCompleted.connect(this, &Capture::requestComplete);\n> > >\n> > > @@ -71,9 +93,14 @@ 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\tint res = allocator_.free(cfg.stream());\n> > \n> > s/res/ret/ for consistency.\n> > \n> > > +\t\tASSERT_EQ(res, 0) << \"Failed to free buffers associated with stream\";\n> > > +\t}\n> > > +\n> > > +\tASSERT_FALSE(allocator_.allocated());\n> > > }\n> > >\n> > > void Capture::prepareRequests(unsigned int plannedRequests)\n> > > @@ -81,22 +108,36 @@ void Capture::prepareRequests(unsigned int plannedRequests)\n> > > \tassert(config_);\n> > > \tassert(requests_.empty());\n> > >\n> > > -\tStream *stream = config_->at(0).stream();\n> > > -\tconst std::vector> &buffers = allocator_.buffers(stream);\n> > > +\tstd::size_t maxBuffers = 0;\n> > >\n> > > -\t/* No point in testing less requests then the camera depth. */\n> > > -\tif (plannedRequests < buffers.size()) {\n> > > -\t\tstd::cout << \"Camera needs \" << buffers.size()\n> > > -\t\t\t << \" requests, can't test only \"\n> > > -\t\t\t << plannedRequests << std::endl;\n> > > -\t\tGTEST_SKIP();\n> > > +\tfor (const auto &cfg : *config_) {\n> > > +\t\tconst auto &buffers = allocator_.buffers(cfg.stream());\n> > > +\t\tASSERT_FALSE(buffers.empty()) << \"Zero buffers allocated for stream\";\n> > > +\n> > > +\t\t/* No point in testing less requests then the camera depth. */\n> > > +\t\tif (plannedRequests < buffers.size()) {\n> > > +\t\t\tstd::cout << \"Camera needs \" << buffers.size()\n> > > +\t\t\t\t << \" requests, can't test only \"\n> > > +\t\t\t\t << plannedRequests << std::endl;\n> > > +\t\t\tGTEST_SKIP();\n> > > +\t\t}\n> > \n> > I think this could be moved after the loop, testing\n> > \n> > \t\tif (plannedRequests < maxBuffers)\n> > \n> > > +\n> > > +\t\tmaxBuffers = std::max(maxBuffers, buffers.size());\n> > > \t}\n> > >\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> > > +\tfor (std::size_t i = 0; i < maxBuffers; i++) {\n> > > +\t\tstd::unique_ptr request = camera_->createRequest(i);\n> > > +\n> > > +\t\tfor (const auto &cfg : *config_) {\n> > > +\t\t\tStream *stream = cfg.stream();\n> > > +\t\t\tconst auto &buffers = allocator_.buffers(stream);\n> > > +\t\t\tassert(!buffers.empty());\n> > >\n> > > -\t\tASSERT_EQ(request->addBuffer(stream, buffer.get()), 0) << \"Can't set buffer for request\";\n> > > +\t\t\tif (i < buffers.size()) {\n> > \n> > \t\t\tif (i >= buffers.size())\n> > \t\t\t\tbreak;\n> > \n> > \t\t\tASSERT_EQ(request->addBuffer(stream, buffers[i].get()), 0)\n> > \t\t\t\t<< \"Can't add buffer to request\";\n> \n> I think that changes the behaviour, but I believe `continue` would\n> work.\n\nOops, indeed, I meant continue.\n\n> > Reviewed-by: Laurent Pinchart \n> > \n> > > +\t\t\t\tASSERT_EQ(request->addBuffer(stream, buffers[i].get()), 0)\n> > > +\t\t\t\t\t<< \"Can't add buffer to request\";\n> > > +\t\t\t}\n> > > +\t\t}\n> > >\n> > > \t\trequests_.push_back(std::move(request));\n> > > \t}\n> > > diff --git a/src/apps/lc-compliance/helpers/capture.h b/src/apps/lc-compliance/helpers/capture.h\n> > > index 67c29021b..b3a390941 100644\n> > > --- a/src/apps/lc-compliance/helpers/capture.h\n> > > +++ b/src/apps/lc-compliance/helpers/capture.h\n> > > @@ -17,7 +17,7 @@\n> > > class Capture\n> > > {\n> > > public:\n> > > -\tvoid configure(libcamera::StreamRole role);\n> > > +\tvoid configure(libcamera::Span roles);\n> > >\n> > > protected:\n> > > \tCapture(std::shared_ptr camera);\n> > > diff --git a/src/apps/lc-compliance/tests/capture_test.cpp b/src/apps/lc-compliance/tests/capture_test.cpp\n> > > index 97465a612..c382fcf47 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> > > \tCaptureBalanced capture(camera_);\n> > >\n> > > -\tcapture.configure(role);\n> > > +\tcapture.configure(std::array{ role });\n> > >\n> > > \tcapture.capture(numRequests);\n> > > }\n> > > @@ -108,7 +108,7 @@ TEST_P(SingleStream, CaptureStartStop)\n> > >\n> > > \tCaptureBalanced capture(camera_);\n> > >\n> > > -\tcapture.configure(role);\n> > > +\tcapture.configure(std::array{ role });\n> > >\n> > > \tfor (unsigned int starts = 0; starts < numRepeats; starts++)\n> > > \t\tcapture.capture(numRequests);\n> > > @@ -127,7 +127,7 @@ TEST_P(SingleStream, UnbalancedStop)\n> > >\n> > > \tCaptureUnbalanced capture(camera_);\n> > >\n> > > -\tcapture.configure(role);\n> > > +\tcapture.configure(std::array{ role });\n> > >\n> > > \tcapture.capture(numRequests);\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 B9640C32F5\n\tfor ;\n\tFri, 10 Jan 2025 09:41:42 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 330C5684F3;\n\tFri, 10 Jan 2025 10:41:41 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A5F72684E1\n\tfor ;\n\tFri, 10 Jan 2025 10:41:39 +0100 (CET)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id EFAEC8FA;\n\tFri, 10 Jan 2025 10:40:44 +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=\"AhVJfF5K\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1736502045;\n\tbh=Ju+Pjt/V41wztHq6knwavAvhw2+TwL+THNXnmlnWj0Y=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=AhVJfF5Kb4Tn5fIzOr+DBULuQXEaFgbjU5Qf9V+H8UZmeYLIosw7Ow0UwOCqp0f7N\n\t5AAIhmZzGwitvv+n+NBC5f+Cle/ce5Xn29wsrhwxHOtMJAUDjBQUokXEbXJsRLl/+z\n\tS0TbSBrzvPzdOE3iDOgWL50GM0zE0cRe0idTeZzA=","Date":"Fri, 10 Jan 2025 11:41:35 +0200","From":"Laurent Pinchart ","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Cc":"libcamera-devel@lists.libcamera.org,\n\tJacopo Mondi ","Subject":"Re: [RFC PATCH v1 11/12] apps: lc-compliance: Support multiple\n\tstreams in helpers","Message-ID":"<20250110094135.GA6611@pendragon.ideasonboard.com>","References":"<20241220150759.709756-1-pobrn@protonmail.com>\n\t<20241220150759.709756-12-pobrn@protonmail.com>\n\t<20250110013254.GM6159@pendragon.ideasonboard.com>\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\" "}}]