[{"id":33309,"web_url":"https://patchwork.libcamera.org/comment/33309/","msgid":"","date":"2025-02-06T17:11:44","subject":"Re: [RFC PATCH v3 17/21] apps: lc-compliance: Support multiple\n\tstreams in helpers","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Barnabás\n\nOn Thu, Jan 30, 2025 at 11:51:28AM +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> Reviewed-by: Paul Elder \n> ---\n> src/apps/lc-compliance/helpers/capture.cpp | 77 +++++++++++++++----\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(+), 19 deletions(-)\n>\n> diff --git a/src/apps/lc-compliance/helpers/capture.cpp b/src/apps/lc-compliance/helpers/capture.cpp\n> index 940646f6c..4a8627662 100644\n> --- a/src/apps/lc-compliance/helpers/capture.cpp\n> +++ b/src/apps/lc-compliance/helpers/capture.cpp\n> @@ -24,13 +24,31 @@ 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\tGTEST_SKIP() << \"Role 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\nThe way we currently handle bufferCount is sub-optimal, so for the\ntime being I would leave the \\todo in place\n\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\nCan this happen ?\n\n> +\n> +\tfor (auto &cfg : *config_)\n> +\t\tcfg.bufferCount = largest->bufferCount;\n> +\n\nI presume having all streams with the same buffer count makes it way\neasier to handle request queuing etc. However there might be system\nwhere this might not be possible ? I guess we'll revisit if they\nappear\n\n> \tif (config_->validate() != CameraConfiguration::Valid) {\n> \t\tconfig_.reset();\n> \t\tFAIL() << \"Configuration not valid\";\n> @@ -74,20 +92,36 @@ void Capture::prepareRequests()\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> +\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\tmaxBuffers = std::max(maxBuffers, buffers.size());\n\nGive the above, I guess all streams have the same buffer count ?\nIf that's the case, can we record it in a comment ? Otherwise when\nwe'll re-look at this in a few months we'll wonder why we have to\ncompute maxBuffers if all streams have the same buffer count.\n\n(actually the choice of how many buffers to allocate is left to\nPipelineHandler::exportBuffers(). All (?) our implementations use\nbufferCount at the moment. If we want this to be enforced we should\ncheck that all streams have allocated the same buffers, as we forced\nbufferCount to have the same value for all streams ?) This can also be\nrecorded in a todo comment if you agree ?\n\n> +\t}\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_ < maxBuffers) {\n> +\t\tGTEST_SKIP() << \"Camera needs \" << maxBuffers\n> \t\t\t << \" requests, can't test only \" << *queueLimit_;\n> \t}\n\nNo need for {}, right ?\n\n>\n> -\tfor (const std::unique_ptr &buffer : buffers) {\n> -\t\tstd::unique_ptr request = camera_->createRequest();\n> +\tfor (std::size_t i = 0; i < maxBuffers; i++) {\n> +\t\tstd::unique_ptr request = camera_->createRequest(i);\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> +\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\t\tif (i >= buffers.size())\n> +\t\t\t\tcontinue;\n\nAs per the above, we could assert i < buffer.size() ?\n\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> +\t\t}\n>\n> \t\trequests_.push_back(std::move(request));\n> \t}\n> @@ -124,11 +158,19 @@ void Capture::requestComplete(Request *request)\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\nThis last check includes the above GE(0)\n\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_TRUE(allocator_.allocated());\n>\n> \tcamera_->requestCompleted.connect(this, &Capture::requestComplete);\n>\n> @@ -144,7 +186,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\tint ret = allocator_.free(cfg.stream());\n> +\t\tEXPECT_EQ(ret, 0) << \"Failed to free buffers associated with stream\";\n\nIf ret doesn't have to be returned\n\n\tfor (const auto &cfg : *config_)\n\t\tEXPECT_EQ(allocator_.free(cfg.stream(), 0)\n << \"Failed to free buffers associated with stream\";\n\n> +\t}\n> +\n> +\tEXPECT_FALSE(allocator_.allocated());\n> }\n> diff --git a/src/apps/lc-compliance/helpers/capture.h b/src/apps/lc-compliance/helpers/capture.h\n> index ede395e2a..48a8dadcb 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..147e17019 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(std::array{ role });\n\nIs there any advantage in passing in a Span compared to\npassing a const reference to the container (it's an std::array<> in\nthis patch, an std::vector<> since the next one).\n\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(std::array{ 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(std::array{ role });\n>\n> \tcapture.run(numRequests);\n> }\n> --\n> 2.48.1\n>\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id F1012C32EA\n\tfor ;\n\tThu, 6 Feb 2025 17:11:49 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B4111685FA;\n\tThu, 6 Feb 2025 18:11:49 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 50C1E685EB\n\tfor ;\n\tThu, 6 Feb 2025 18:11:48 +0100 (CET)","from ideasonboard.com (mob-5-90-139-204.net.vodafone.it\n\t[5.90.139.204])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 8BA821198;\n\tThu, 6 Feb 2025 18:10:34 +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=\"bEjZIW9y\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1738861834;\n\tbh=/KgxH/Fh2InUt0rNbsqEvI1R3zDn6zuBfH+M90SGCd8=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=bEjZIW9y81VCh1BVL0+RT6MiobGR4O9X7gD8ORNxh0pFrBQG4ohYE8zmCHQOWA/bs\n\tKYupnugY/r6RM3blJZ9xH/UzcPEzS47U1g9w7pxVWV2U14f16/wBq5tecHjL/fCqZR\n\tBxYe4Pu4eZBq7qO/9vuutJyW9svIC2QhaS7d9ZzE=","Date":"Thu, 6 Feb 2025 18:11:44 +0100","From":"Jacopo Mondi ","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Cc":"libcamera-devel@lists.libcamera.org, \n\tJacopo Mondi ,\n\tPaul Elder ","Subject":"Re: [RFC PATCH v3 17/21] apps: lc-compliance: Support multiple\n\tstreams in helpers","Message-ID":"","References":"<20250130115001.1129305-1-pobrn@protonmail.com>\n\t<20250130115001.1129305-18-pobrn@protonmail.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20250130115001.1129305-18-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":33319,"web_url":"https://patchwork.libcamera.org/comment/33319/","msgid":"","date":"2025-02-06T18:23:57","subject":"Re: [RFC PATCH v3 17/21] 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. február 6., csütörtök 18:11 keltezéssel, Jacopo Mondi írta:\n\n> Hi Barnabás\n> \n> On Thu, Jan 30, 2025 at 11:51:28AM +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> > Reviewed-by: Paul Elder \n> > ---\n> > src/apps/lc-compliance/helpers/capture.cpp | 77 +++++++++++++++----\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(+), 19 deletions(-)\n> >\n> > diff --git a/src/apps/lc-compliance/helpers/capture.cpp b/src/apps/lc-compliance/helpers/capture.cpp\n> > index 940646f6c..4a8627662 100644\n> > --- a/src/apps/lc-compliance/helpers/capture.cpp\n> > +++ b/src/apps/lc-compliance/helpers/capture.cpp\n> > @@ -24,13 +24,31 @@ 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\tGTEST_SKIP() << \"Role 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> \n> The way we currently handle bufferCount is sub-optimal, so for the\n> time being I would leave the \\todo in place\n\nSorry, I don't quite understand what you mean by \"leave the \\todo in place\".\n\n\n> \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> \n> Can this happen ?\n\nI don't think so.\n\n\n> \n> > +\n> > +\tfor (auto &cfg : *config_)\n> > +\t\tcfg.bufferCount = largest->bufferCount;\n> > +\n> \n> I presume having all streams with the same buffer count makes it way\n> easier to handle request queuing etc. However there might be system\n> where this might not be possible ? I guess we'll revisit if they\n> appear\n> \n> > \tif (config_->validate() != CameraConfiguration::Valid) {\n> > \t\tconfig_.reset();\n> > \t\tFAIL() << \"Configuration not valid\";\n> > @@ -74,20 +92,36 @@ void Capture::prepareRequests()\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> > +\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\tmaxBuffers = std::max(maxBuffers, buffers.size());\n> \n> Give the above, I guess all streams have the same buffer count ?\n> If that's the case, can we record it in a comment ? Otherwise when\n> we'll re-look at this in a few months we'll wonder why we have to\n> compute maxBuffers if all streams have the same buffer count.\n> \n> (actually the choice of how many buffers to allocate is left to\n> PipelineHandler::exportBuffers(). All (?) our implementations use\n> bufferCount at the moment. If we want this to be enforced we should\n> check that all streams have allocated the same buffers, as we forced\n> bufferCount to have the same value for all streams ?) This can also be\n> recorded in a todo comment if you agree ?\n\nI have opened https://bugs.libcamera.org/show_bug.cgi?id=251 in relation with this topic.\nThe semantics of that field are not entirely clear to me. So I tried to handle\nmismatching buffer counts gracefully. Nonetheless, I would not consider the\nimplementation here good by any means. Maybe it would indeed be better\nto require the same buffer count for now.\n\n\n> \n> > +\t}\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_ < maxBuffers) {\n> > +\t\tGTEST_SKIP() << \"Camera needs \" << maxBuffers\n> > \t\t\t << \" requests, can't test only \" << *queueLimit_;\n> > \t}\n> \n> No need for {}, right ?\n\nI usually use `{}` for when there are multiple lines and checkstyle.py did not\ncomplain, so should I change it?\n\n\n> \n> >\n> > -\tfor (const std::unique_ptr &buffer : buffers) {\n> > -\t\tstd::unique_ptr request = camera_->createRequest();\n> > +\tfor (std::size_t i = 0; i < maxBuffers; i++) {\n> > +\t\tstd::unique_ptr request = camera_->createRequest(i);\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> > +\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\t\tif (i >= buffers.size())\n> > +\t\t\t\tcontinue;\n> \n> As per the above, we could assert i < buffer.size() ?\n> \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> > +\t\t}\n> >\n> > \t\trequests_.push_back(std::move(request));\n> > \t}\n> > @@ -124,11 +158,19 @@ void Capture::requestComplete(Request *request)\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> \n> This last check includes the above GE(0)\n\n`EXPECT_*()` checks fail the test but they do not abort the execution.\n\n\n> \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_TRUE(allocator_.allocated());\n> >\n> > \tcamera_->requestCompleted.connect(this, &Capture::requestComplete);\n> >\n> > @@ -144,7 +186,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\tint ret = allocator_.free(cfg.stream());\n> > +\t\tEXPECT_EQ(ret, 0) << \"Failed to free buffers associated with stream\";\n> \n> If ret doesn't have to be returned\n\nACK\n\n\n> \n> \tfor (const auto &cfg : *config_)\n> \t\tEXPECT_EQ(allocator_.free(cfg.stream(), 0)\n> << \"Failed to free buffers associated with stream\";\n> \n> > +\t}\n> > +\n> > +\tEXPECT_FALSE(allocator_.allocated());\n> > }\n> > diff --git a/src/apps/lc-compliance/helpers/capture.h b/src/apps/lc-compliance/helpers/capture.h\n> > index ede395e2a..48a8dadcb 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..147e17019 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(std::array{ role });\n> \n> Is there any advantage in passing in a Span compared to\n> passing a const reference to the container (it's an std::array<> in\n> this patch, an std::vector<> since the next one).\n\nWell, I did not see a good enough reason not to use one. :)\n\n\n> \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(std::array{ 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(std::array{ role });\n> >\n> > \tcapture.run(numRequests);\n> > }\n> > --\n> > 2.48.1\n> >\n> >","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 5C6C2C32EA\n\tfor ;\n\tThu, 6 Feb 2025 18:24:05 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8C35B685FF;\n\tThu, 6 Feb 2025 19:24:04 +0100 (CET)","from mail-40131.protonmail.ch (mail-40131.protonmail.ch\n\t[185.70.40.131])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 0DBDC6053F\n\tfor ;\n\tThu, 6 Feb 2025 19:24:03 +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=\"Mc/rALNa\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=protonmail.com;\n\ts=protonmail3; t=1738866242; x=1739125442;\n\tbh=y6aCBDPaRrDFdTilRxUF1GnAR096TSDo+PKxqzunTJM=;\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=Mc/rALNabYrFg52v0H4d8EhEPyE1tRuzvD2t9ljIiq0NJfK6E1JYf20K67CtWsJuU\n\tonO+4ahXiww+9YFo3+czOKk6nfM4GSgy+DCXlCLi8ZxGJ50RqPcKTM+8J/SuZaR/0N\n\t/Fw1Mad77IZMAV3eoiT7u9sKPrDxyci40gFTsTPcsOxdaBXBXLiiaLwra5eyJ+PHHp\n\tqmzWCfedA2X1qSOs6C1zb/O9u93hRTT5u4rp8Hs9p5goApkLtlwUS49+04L+SK6IjQ\n\tAt5s7plJQ2M5QkAsLpNR4KdHIHzbi4q9GNt1W51PjDwvHfiesESjg8Hbo7ANJX98ro\n\t7f9HxaI22IkQg==","Date":"Thu, 06 Feb 2025 18:23:57 +0000","To":"Jacopo Mondi ","From":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Cc":"libcamera-devel@lists.libcamera.org,\n\tPaul Elder ","Subject":"Re: [RFC PATCH v3 17/21] apps: lc-compliance: Support multiple\n\tstreams in helpers","Message-ID":"","In-Reply-To":"","References":"<20250130115001.1129305-1-pobrn@protonmail.com>\n\t<20250130115001.1129305-18-pobrn@protonmail.com>\n\t","Feedback-ID":"20568564:user:proton","X-Pm-Message-ID":"523ac833c1286c57bd2bdf0e4df6e025f5240558","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":33320,"web_url":"https://patchwork.libcamera.org/comment/33320/","msgid":"","date":"2025-02-06T19:24:07","subject":"Re: [RFC PATCH v3 17/21] apps: lc-compliance: Support multiple\n\tstreams in helpers","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Barnabás\n\nOn Thu, Feb 06, 2025 at 06:23:57PM +0000, Barnabás Pőcze wrote:\n> Hi\n>\n>\n> 2025. február 6., csütörtök 18:11 keltezéssel, Jacopo Mondi írta:\n>\n> > Hi Barnabás\n> >\n> > On Thu, Jan 30, 2025 at 11:51:28AM +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> > > Reviewed-by: Paul Elder \n> > > ---\n> > > src/apps/lc-compliance/helpers/capture.cpp | 77 +++++++++++++++----\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(+), 19 deletions(-)\n> > >\n> > > diff --git a/src/apps/lc-compliance/helpers/capture.cpp b/src/apps/lc-compliance/helpers/capture.cpp\n> > > index 940646f6c..4a8627662 100644\n> > > --- a/src/apps/lc-compliance/helpers/capture.cpp\n> > > +++ b/src/apps/lc-compliance/helpers/capture.cpp\n> > > @@ -24,13 +24,31 @@ 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\tGTEST_SKIP() << \"Role 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> >\n> > The way we currently handle bufferCount is sub-optimal, so for the\n> > time being I would leave the \\todo in place\n>\n> Sorry, I don't quite understand what you mean by \"leave the \\todo in place\".\n>\n\nSorry, I was just reinforcing that bufferCount is poorly handled, and\nI agree with the todo being there\n\n>\n> >\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> >\n> > Can this happen ?\n>\n> I don't think so.\n>\n>\n> >\n> > > +\n> > > +\tfor (auto &cfg : *config_)\n> > > +\t\tcfg.bufferCount = largest->bufferCount;\n> > > +\n> >\n> > I presume having all streams with the same buffer count makes it way\n> > easier to handle request queuing etc. However there might be system\n> > where this might not be possible ? I guess we'll revisit if they\n> > appear\n> >\n> > > \tif (config_->validate() != CameraConfiguration::Valid) {\n> > > \t\tconfig_.reset();\n> > > \t\tFAIL() << \"Configuration not valid\";\n> > > @@ -74,20 +92,36 @@ void Capture::prepareRequests()\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> > > +\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\tmaxBuffers = std::max(maxBuffers, buffers.size());\n> >\n> > Give the above, I guess all streams have the same buffer count ?\n> > If that's the case, can we record it in a comment ? Otherwise when\n> > we'll re-look at this in a few months we'll wonder why we have to\n> > compute maxBuffers if all streams have the same buffer count.\n> >\n> > (actually the choice of how many buffers to allocate is left to\n> > PipelineHandler::exportBuffers(). All (?) our implementations use\n> > bufferCount at the moment. If we want this to be enforced we should\n> > check that all streams have allocated the same buffers, as we forced\n> > bufferCount to have the same value for all streams ?) This can also be\n> > recorded in a todo comment if you agree ?\n>\n> I have opened https://bugs.libcamera.org/show_bug.cgi?id=251 in relation with this topic.\n\nThanks\n\n> The semantics of that field are not entirely clear to me. So I tried to handle\n> mismatching buffer counts gracefully. Nonetheless, I would not consider the\n> implementation here good by any means. Maybe it would indeed be better\n> to require the same buffer count for now.\n\nOk, then let's compute the 'largest' bufferCount as above and set all\ncfg.bufferCount to that as you're doing already.\n\nThen just make sure in prepareRequests() that all streams have the\nsame number of buffers and that should be it ?\n\n>\n>\n> >\n> > > +\t}\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_ < maxBuffers) {\n> > > +\t\tGTEST_SKIP() << \"Camera needs \" << maxBuffers\n> > > \t\t\t << \" requests, can't test only \" << *queueLimit_;\n> > > \t}\n> >\n> > No need for {}, right ?\n>\n> I usually use `{}` for when there are multiple lines and checkstyle.py did not\n> complain, so should I change it?\n>\n\nit's a single statement, so theoretically, yes. Practically, whatever :)\n\n>\n> >\n> > >\n> > > -\tfor (const std::unique_ptr &buffer : buffers) {\n> > > -\t\tstd::unique_ptr request = camera_->createRequest();\n> > > +\tfor (std::size_t i = 0; i < maxBuffers; i++) {\n> > > +\t\tstd::unique_ptr request = camera_->createRequest(i);\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> > > +\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\t\tif (i >= buffers.size())\n> > > +\t\t\t\tcontinue;\n> >\n> > As per the above, we could assert i < buffer.size() ?\n\nIf we validate that all streams have the same number of buffers you\ncan remove this\n\n> >\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> > > +\t\t}\n> > >\n> > > \t\trequests_.push_back(std::move(request));\n> > > \t}\n> > > @@ -124,11 +158,19 @@ void Capture::requestComplete(Request *request)\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> >\n> > This last check includes the above GE(0)\n>\n> `EXPECT_*()` checks fail the test but they do not abort the execution.\n\nASSERT_EQ() then ?\n\n>\n>\n> >\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_TRUE(allocator_.allocated());\n> > >\n> > > \tcamera_->requestCompleted.connect(this, &Capture::requestComplete);\n> > >\n> > > @@ -144,7 +186,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\tint ret = allocator_.free(cfg.stream());\n> > > +\t\tEXPECT_EQ(ret, 0) << \"Failed to free buffers associated with stream\";\n> >\n> > If ret doesn't have to be returned\n>\n> ACK\n>\n>\n> >\n> > \tfor (const auto &cfg : *config_)\n> > \t\tEXPECT_EQ(allocator_.free(cfg.stream(), 0)\n> > << \"Failed to free buffers associated with stream\";\n> >\n> > > +\t}\n> > > +\n> > > +\tEXPECT_FALSE(allocator_.allocated());\n> > > }\n> > > diff --git a/src/apps/lc-compliance/helpers/capture.h b/src/apps/lc-compliance/helpers/capture.h\n> > > index ede395e2a..48a8dadcb 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..147e17019 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(std::array{ role });\n> >\n> > Is there any advantage in passing in a Span compared to\n> > passing a const reference to the container (it's an std::array<> in\n> > this patch, an std::vector<> since the next one).\n>\n> Well, I did not see a good enough reason not to use one. :)\n>\n\nTrue that :)\n\nThanks\n j\n\n>\n> >\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(std::array{ 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(std::array{ role });\n> > >\n> > > \tcapture.run(numRequests);\n> > > }\n> > > --\n> > > 2.48.1\n> > >\n> > >","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 10E46C32EF\n\tfor ;\n\tThu, 6 Feb 2025 19:24:14 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id ECB6468600;\n\tThu, 6 Feb 2025 20:24: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 A98296053F\n\tfor ;\n\tThu, 6 Feb 2025 20:24:11 +0100 (CET)","from ideasonboard.com (mob-5-90-139-204.net.vodafone.it\n\t[5.90.139.204])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 827CE1193;\n\tThu, 6 Feb 2025 20:22:57 +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=\"e7p2GV2m\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1738869777;\n\tbh=GmevnKizKur2BXOTgEEywandWPUlMIjB6dOILCrsIBY=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=e7p2GV2mp8CnnaSPXYZhQXR9Ffqjku2ShT+OEtGkbVrOiFJihXgGEa9L2NZ/n1mis\n\tGSCFWstsAeIYdilKj+tiA4abNycjGOEswGvSISuA9zJHx36VAaIKC2rC0AgSFJfwm2\n\tYqHFFiKEjFVb0QX0kZNwNR5v86k0AWtGZM1l0TFE=","Date":"Thu, 6 Feb 2025 20:24:07 +0100","From":"Jacopo Mondi ","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Cc":"Jacopo Mondi , \n\tlibcamera-devel@lists.libcamera.org,\n\tPaul Elder ","Subject":"Re: [RFC PATCH v3 17/21] apps: lc-compliance: Support multiple\n\tstreams in helpers","Message-ID":"","References":"<20250130115001.1129305-1-pobrn@protonmail.com>\n\t<20250130115001.1129305-18-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":33322,"web_url":"https://patchwork.libcamera.org/comment/33322/","msgid":"<-grPKUn44_dXPpKmB2rCrc03S2GguICQalT8jSIH4iG6_lIrQ1_tZcO5wBilfnMd3YE199zwDyNw5HHa1b7cTlqRcO-KQ8Xig_IvWvxZ-c0=@protonmail.com>","date":"2025-02-10T08:03:40","subject":"Re: [RFC PATCH v3 17/21] 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. február 6., csütörtök 20:24 keltezéssel, Jacopo Mondi írta:\n\n> Hi Barnabás\n>\n> On Thu, Feb 06, 2025 at 06:23:57PM +0000, Barnabás Pőcze wrote:\n> > Hi\n> >\n> >\n> > 2025. február 6., csütörtök 18:11 keltezéssel, Jacopo Mondi írta:\n> >\n> > > Hi Barnabás\n> > >\n> > > On Thu, Jan 30, 2025 at 11:51:28AM +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> > > > Reviewed-by: Paul Elder \n> > > > ---\n> > > > src/apps/lc-compliance/helpers/capture.cpp | 77 +++++++++++++++----\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(+), 19 deletions(-)\n> > > >\n> > > > diff --git a/src/apps/lc-compliance/helpers/capture.cpp b/src/apps/lc-compliance/helpers/capture.cpp\n> > > > index 940646f6c..4a8627662 100644\n> > > > --- a/src/apps/lc-compliance/helpers/capture.cpp\n> > > > +++ b/src/apps/lc-compliance/helpers/capture.cpp\n> > > > @@ -24,13 +24,31 @@ 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\tGTEST_SKIP() << \"Role 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> > >\n> > > The way we currently handle bufferCount is sub-optimal, so for the\n> > > time being I would leave the \\todo in place\n> >\n> > Sorry, I don't quite understand what you mean by \"leave the \\todo in place\".\n> >\n>\n> Sorry, I was just reinforcing that bufferCount is poorly handled, and\n> I agree with the todo being there\n>\n> >\n> > >\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> > >\n> > > Can this happen ?\n> >\n> > I don't think so.\n> >\n> >\n> > >\n> > > > +\n> > > > +\tfor (auto &cfg : *config_)\n> > > > +\t\tcfg.bufferCount = largest->bufferCount;\n> > > > +\n> > >\n> > > I presume having all streams with the same buffer count makes it way\n> > > easier to handle request queuing etc. However there might be system\n> > > where this might not be possible ? I guess we'll revisit if they\n> > > appear\n> > >\n> > > > \tif (config_->validate() != CameraConfiguration::Valid) {\n> > > > \t\tconfig_.reset();\n> > > > \t\tFAIL() << \"Configuration not valid\";\n> > > > @@ -74,20 +92,36 @@ void Capture::prepareRequests()\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> > > > +\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\tmaxBuffers = std::max(maxBuffers, buffers.size());\n> > >\n> > > Give the above, I guess all streams have the same buffer count ?\n> > > If that's the case, can we record it in a comment ? Otherwise when\n> > > we'll re-look at this in a few months we'll wonder why we have to\n> > > compute maxBuffers if all streams have the same buffer count.\n> > >\n> > > (actually the choice of how many buffers to allocate is left to\n> > > PipelineHandler::exportBuffers(). All (?) our implementations use\n> > > bufferCount at the moment. If we want this to be enforced we should\n> > > check that all streams have allocated the same buffers, as we forced\n> > > bufferCount to have the same value for all streams ?) This can also be\n> > > recorded in a todo comment if you agree ?\n> >\n> > I have opened https://bugs.libcamera.org/show_bug.cgi?id=251 in relation with this topic.\n>\n> Thanks\n>\n> > The semantics of that field are not entirely clear to me. So I tried to handle\n> > mismatching buffer counts gracefully. Nonetheless, I would not consider the\n> > implementation here good by any means. Maybe it would indeed be better\n> > to require the same buffer count for now.\n>\n> Ok, then let's compute the 'largest' bufferCount as above and set all\n> cfg.bufferCount to that as you're doing already.\n>\n> Then just make sure in prepareRequests() that all streams have the\n> same number of buffers and that should be it ?\n\nI think so. I'll rewrite this part to require matching buffer counts.\n\n\n>\n> >\n> >\n> > >\n> > > > +\t}\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_ < maxBuffers) {\n> > > > +\t\tGTEST_SKIP() << \"Camera needs \" << maxBuffers\n> > > > \t\t\t << \" requests, can't test only \" << *queueLimit_;\n> > > > \t}\n> > >\n> > > No need for {}, right ?\n> >\n> > I usually use `{}` for when there are multiple lines and checkstyle.py did not\n> > complain, so should I change it?\n> >\n>\n> it's a single statement, so theoretically, yes. Practically, whatever :)\n>\n> >\n> > >\n> > > >\n> > > > -\tfor (const std::unique_ptr &buffer : buffers) {\n> > > > -\t\tstd::unique_ptr request = camera_->createRequest();\n> > > > +\tfor (std::size_t i = 0; i < maxBuffers; i++) {\n> > > > +\t\tstd::unique_ptr request = camera_->createRequest(i);\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> > > > +\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\t\tif (i >= buffers.size())\n> > > > +\t\t\t\tcontinue;\n> > >\n> > > As per the above, we could assert i < buffer.size() ?\n>\n> If we validate that all streams have the same number of buffers you\n> can remove this\n>\n> > >\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> > > > +\t\t}\n> > > >\n> > > > \t\trequests_.push_back(std::move(request));\n> > > > \t}\n> > > > @@ -124,11 +158,19 @@ void Capture::requestComplete(Request *request)\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> > >\n> > > This last check includes the above GE(0)\n> >\n> > `EXPECT_*()` checks fail the test but they do not abort the execution.\n>\n> ASSERT_EQ() then ?\n\nOops, sorry, this was a bit of an incomplete reply. `ASSERT_*()` will cause an\nexception to be thrown, so the execution of the test is aborted. So removing\neither one will change the behaviour. I also think it's nice to get a different\nmessage for the error case.\n\n\n>\n> >\n> >\n> > >\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_TRUE(allocator_.allocated());\n> > > >\n> > > > \tcamera_->requestCompleted.connect(this, &Capture::requestComplete);\n> > > >\n> > > > @@ -144,7 +186,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\tint ret = allocator_.free(cfg.stream());\n> > > > +\t\tEXPECT_EQ(ret, 0) << \"Failed to free buffers associated with stream\";\n> > >\n> > > If ret doesn't have to be returned\n> >\n> > ACK\n> >\n> >\n> > >\n> > > \tfor (const auto &cfg : *config_)\n> > > \t\tEXPECT_EQ(allocator_.free(cfg.stream(), 0)\n> > > << \"Failed to free buffers associated with stream\";\n> > >\n> > > > +\t}\n> > > > +\n> > > > +\tEXPECT_FALSE(allocator_.allocated());\n> > > > }\n> > > > diff --git a/src/apps/lc-compliance/helpers/capture.h b/src/apps/lc-compliance/helpers/capture.h\n> > > > index ede395e2a..48a8dadcb 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..147e17019 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(std::array{ role });\n> > >\n> > > Is there any advantage in passing in a Span compared to\n> > > passing a const reference to the container (it's an std::array<> in\n> > > this patch, an std::vector<> since the next one).\n> >\n> > Well, I did not see a good enough reason not to use one. :)\n> >\n>\n> True that :)\n>\n> Thanks\n> j\n>\n> >\n> > >\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(std::array{ 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(std::array{ role });\n> > > >\n> > > > \tcapture.run(numRequests);\n> > > > }\n> > > > --\n> > > > 2.48.1\n> > > >\n> > > >","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id F3F6DC32AF\n\tfor ;\n\tMon, 10 Feb 2025 08:03:51 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C207B6860E;\n\tMon, 10 Feb 2025 09:03:50 +0100 (CET)","from mail-40134.protonmail.ch (mail-40134.protonmail.ch\n\t[185.70.40.134])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 504266033B\n\tfor ;\n\tMon, 10 Feb 2025 09:03:48 +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=\"wdPcWkL9\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=protonmail.com;\n\ts=protonmail3; t=1739174627; x=1739433827;\n\tbh=kFkSHCB1LUaxQ1KZIy4HBmwKambSLmra1kyJKvJLRnQ=;\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=wdPcWkL9Px/T+1E2XKHSz+NGXVHs86RRsnm8dsKTbW56kTQxhrmAPfvP4xfkpXNx2\n\t7oeFqGuRigb13ttSBPBM3VNXVPRktik2ioHs5l7qbmjXBtoccRg11cn2icXYCjHAug\n\tUM1HgKS3sCJ9ABiL3vZFqy3xN59zBR4983G6kV9TUaynv++Dl2/A9/RkTI/UuSdBBC\n\tNbkgwvVp/dllBn2ME8tkou4YLtPDCbIt3klAxM81bT7/MVXqC+ybMPdv4asWuAQyos\n\tD6/qbdXVPLGpCFZAQaSsYF3vRNn4n/ibYbqPhYBx889UUWQApC00+CDWeElUO5438K\n\tzVQ9P5sYj4HMg==","Date":"Mon, 10 Feb 2025 08:03:40 +0000","To":"Jacopo Mondi ","From":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Cc":"libcamera-devel@lists.libcamera.org,\n\tPaul Elder ","Subject":"Re: [RFC PATCH v3 17/21] apps: lc-compliance: Support multiple\n\tstreams in helpers","Message-ID":"<-grPKUn44_dXPpKmB2rCrc03S2GguICQalT8jSIH4iG6_lIrQ1_tZcO5wBilfnMd3YE199zwDyNw5HHa1b7cTlqRcO-KQ8Xig_IvWvxZ-c0=@protonmail.com>","In-Reply-To":"","References":"<20250130115001.1129305-1-pobrn@protonmail.com>\n\t<20250130115001.1129305-18-pobrn@protonmail.com>\n\t\n\t\n\t","Feedback-ID":"20568564:user:proton","X-Pm-Message-ID":"7091e43e899c7b72682e227e5be58e9a032cc944","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\" "}}]