[{"id":28984,"web_url":"https://patchwork.libcamera.org/comment/28984/","msgid":"<20240315130035.zqevlhnr4sj2mdcg@jasper>","date":"2024-03-15T13:00:35","subject":"Re: [libcamera-devel] [PATCH v3 6/7] apps: lc-compliance: Support\n\tmultiple streams in helpers","submitter":{"id":184,"url":"https://patchwork.libcamera.org/api/people/184/","name":"Stefan Klug","email":"stefan.klug@ideasonboard.com"},"content":"Hi Jacopo,\n\nthanks for the patch. I did not have a setup with support for\nmultistream. I compiled it and ran the tests which then got skipped (as\nextpected)\n\nOn Sat, Dec 30, 2023 at 05:29:11PM +0100, Jacopo Mondi 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> Signed-off-by: Jacopo Mondi \n> ---\n> src/apps/lc-compliance/helpers/capture.cpp | 85 ++++++++++++++-----\n> src/apps/lc-compliance/helpers/capture.h | 2 +-\n> src/apps/lc-compliance/tests/capture_test.cpp | 26 +++---\n> 3 files changed, 76 insertions(+), 37 deletions(-)\n> \n> diff --git a/src/apps/lc-compliance/helpers/capture.cpp b/src/apps/lc-compliance/helpers/capture.cpp\n> index 5aab973f0392..bb95af3d758c 100644\n> --- a/src/apps/lc-compliance/helpers/capture.cpp\n> +++ b/src/apps/lc-compliance/helpers/capture.cpp\n> @@ -7,6 +7,8 @@\n> \n> #include \"capture.h\"\n> \n> +#include \n> +\n> #include \n> \n> using namespace libcamera;\n> @@ -22,15 +24,27 @@ Capture::~Capture()\n> \tstop();\n> }\n> \n> -void Capture::configure(StreamRole role)\n> +void Capture::configure(Span roles)\n> {\n> -\tconfig_ = camera_->generateConfiguration({ role });\n> +\tconfig_ = camera_->generateConfiguration(roles);\n> \n> \tif (!config_) {\n> -\t\tstd::cout << \"Role not supported by camera\" << std::endl;\n> +\t\tstd::cout << \"Roles 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\nNit: Not sure about the rules here, but every comment I saw just ended with */ on the last line.\n\n> +\tauto largest =\n> +\t\tstd::max_element(config_->begin(), config_->end(),\n> +\t\t\t\t [](StreamConfiguration &l, StreamConfiguration &r)\n> +\t\t\t\t { return l.bufferCount < r.bufferCount; });\n> +\n> +\tfor (auto &stream : *config_)\n> +\t\tstream.bufferCount = largest->bufferCount;\n> +\n> \tif (config_->validate() != CameraConfiguration::Valid) {\n> \t\tconfig_.reset();\n> \t\tFAIL() << \"Configuration not valid\";\n> @@ -44,11 +58,17 @@ 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> +\tunsigned int i = 0;\n> +\tfor (auto const &config : *config_) {\n> +\t\tStream *stream = config.stream();\n> +\t\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> +\t\tASSERT_GE(count, 0) << \"Failed to allocate buffers for stream \" << i;\n> +\t\tEXPECT_EQ(count, config.bufferCount)\n> +\t\t\t<< \"Allocated less buffers than expected for stream \" << i;\n> +\n> +\t\t++i;\n> +\t}\n> \n> \tcamera_->requestCompleted.connect(this, &Capture::requestComplete);\n> \n> @@ -64,9 +84,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 (auto const &config : *config_) {\n> +\t\tStream *stream = config.stream();\n> +\t\tallocator_->free(stream);\n> +\t}\n> }\n> \n> /* CaptureBalanced */\n> @@ -80,14 +103,12 @@ void CaptureBalanced::capture(unsigned int numRequests)\n> {\n> \tstart();\n> \n> -\tStream *stream = config_->at(0).stream();\n> -\tconst std::vector> &buffers = allocator_->buffers(stream);\n> -\n> \t/* No point in testing less requests then the camera depth. */\n> -\tif (buffers.size() > numRequests) {\n> -\t\tstd::cout << \"Camera needs \" + std::to_string(buffers.size())\n> -\t\t\t+ \" requests, can't test only \"\n> -\t\t\t+ std::to_string(numRequests) << std::endl;\n> +\tconst unsigned int bufferCount = config_->at(0).bufferCount;\n> +\tif (bufferCount > numRequests) {\n> +\t\tstd::cout << \"Camera needs \" << bufferCount\n> +\t\t\t << \" requests, can't test only \" << numRequests\n> +\t\t\t << std::endl;\n\nI don't know the details here. Can different streams have different\nbufferCount? I guess that is unlikely. Maybe add a comment?\n\n> \t\tGTEST_SKIP();\n> \t}\n> \n> @@ -96,11 +117,21 @@ void CaptureBalanced::capture(unsigned int numRequests)\n> \tcaptureLimit_ = numRequests;\n> \n> \t/* Queue the recommended number of requests. */\n> -\tfor (const std::unique_ptr &buffer : buffers) {\n> +\tfor (unsigned int bufferIdx = 0; bufferIdx < bufferCount; ++bufferIdx) {\n> \t\tstd::unique_ptr request = camera_->createRequest();\n> \t\tASSERT_TRUE(request) << \"Can't create request\";\n> \n> -\t\tASSERT_EQ(request->addBuffer(stream, buffer.get()), 0) << \"Can't set buffer for request\";\n> +\t\t/* Add buffers for each stream. */\n> +\t\tunsigned int i = 0;\n> +\t\tfor (const auto &config : *config_) {\n> +\t\t\tStream *stream = config.stream();\n> +\t\t\tconst auto &buffers = allocator_->buffers(stream);\n> +\n> +\t\t\tASSERT_EQ(request->addBuffer(stream, buffers[bufferIdx].get()), 0)\n> +\t\t\t\t<< \"Can't add buffers for stream \" << i;\n> +\n> +\t\t\t++i;\n> +\t\t}\n> \n> \t\tASSERT_EQ(queueRequest(request.get()), 0) << \"Failed to queue request\";\n> \n> @@ -152,18 +183,26 @@ void CaptureUnbalanced::capture(unsigned int numRequests)\n> {\n> \tstart();\n> \n> -\tStream *stream = config_->at(0).stream();\n> -\tconst std::vector> &buffers = allocator_->buffers(stream);\n> -\n> \tcaptureCount_ = 0;\n> \tcaptureLimit_ = numRequests;\n> \n> \t/* Queue the recommended number of requests. */\n> -\tfor (const std::unique_ptr &buffer : buffers) {\n> +\tconst unsigned int bufferCount = config_->at(0).bufferCount;\n> +\tfor (unsigned int bufferIdx = 0; bufferIdx < bufferCount; ++bufferIdx) {\n> \t\tstd::unique_ptr request = camera_->createRequest();\n> \t\tASSERT_TRUE(request) << \"Can't create request\";\n> \n> -\t\tASSERT_EQ(request->addBuffer(stream, buffer.get()), 0) << \"Can't set buffer for request\";\n> +\t\t/* Add buffers for each stream. */\n> +\t\tunsigned int i = 0;\n> +\t\tfor (const auto &config : *config_) {\n> +\t\t\tStream *stream = config.stream();\n> +\t\t\tconst auto &buffers = allocator_->buffers(stream);\n> +\n> +\t\t\tASSERT_EQ(request->addBuffer(stream, buffers[bufferIdx].get()), 0)\n> +\t\t\t\t<< \"Can't add buffers for stream \" << i;\n> +\n> +\t\t\t++i;\n> +\t\t}\n\nSame question as above. If the bufferCount could differ between the\nstreams, this might be problematic.\n\nCheers,\nStefan\n\n> \n> \t\tASSERT_EQ(camera_->queueRequest(request.get()), 0) << \"Failed to queue request\";\n> \n> diff --git a/src/apps/lc-compliance/helpers/capture.h b/src/apps/lc-compliance/helpers/capture.h\n> index 0574ab1c7ac7..3e2b2889244d 100644\n> --- a/src/apps/lc-compliance/helpers/capture.h\n> +++ b/src/apps/lc-compliance/helpers/capture.h\n> @@ -16,7 +16,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 284d36307619..3d3cc97791d9 100644\n> --- a/src/apps/lc-compliance/tests/capture_test.cpp\n> +++ b/src/apps/lc-compliance/tests/capture_test.cpp\n> @@ -17,14 +17,14 @@\n> using namespace libcamera;\n> \n> const std::vector NUMREQUESTS = { 1, 2, 3, 5, 8, 13, 21, 34, 55, 89 };\n> -const std::vector ROLES = {\n> -\tStreamRole::Raw,\n> -\tStreamRole::StillCapture,\n> -\tStreamRole::VideoRecording,\n> -\tStreamRole::Viewfinder\n> +const std::vector> ROLES = {\n> +\t{ StreamRole::Raw },\n> +\t{ StreamRole::StillCapture },\n> +\t{ StreamRole::VideoRecording },\n> +\t{ StreamRole::Viewfinder },\n> };\n> \n> -class SingleStream : public testing::TestWithParam>\n> +class SingleStream : public testing::TestWithParam, int>>\n> {\n> public:\n> \tstatic std::string nameParameters(const testing::TestParamInfo &info);\n> @@ -67,7 +67,7 @@ std::string SingleStream::nameParameters(const testing::TestParamInfo \t\t{ StreamRole::Viewfinder, \"Viewfinder\" }\n> \t};\n> \n> -\tstd::string roleName = rolesMap[std::get<0>(info.param)];\n> +\tstd::string roleName = rolesMap[std::get<0>(info.param)[0]];\n> \tstd::string numRequestsName = std::to_string(std::get<1>(info.param));\n> \n> \treturn roleName + \"_\" + numRequestsName;\n> @@ -82,11 +82,11 @@ std::string SingleStream::nameParameters(const testing::TestParamInfo */\n> TEST_P(SingleStream, Capture)\n> {\n> -\tauto [role, numRequests] = GetParam();\n> +\tconst auto [role, numRequests] = GetParam();\n> \n> \tCaptureBalanced capture(camera_);\n> \n> -\tcapture.configure(role);\n> +\tcapture.configure(Span{ role });\n> \n> \tcapture.capture(numRequests);\n> }\n> @@ -100,12 +100,12 @@ TEST_P(SingleStream, Capture)\n> */\n> TEST_P(SingleStream, CaptureStartStop)\n> {\n> -\tauto [role, numRequests] = GetParam();\n> +\tconst auto [role, numRequests] = GetParam();\n> \tunsigned int numRepeats = 3;\n> \n> \tCaptureBalanced capture(camera_);\n> \n> -\tcapture.configure(role);\n> +\tcapture.configure(Span{ role });\n> \n> \tfor (unsigned int starts = 0; starts < numRepeats; starts++)\n> \t\tcapture.capture(numRequests);\n> @@ -120,11 +120,11 @@ TEST_P(SingleStream, CaptureStartStop)\n> */\n> TEST_P(SingleStream, UnbalancedStop)\n> {\n> -\tauto [role, numRequests] = GetParam();\n> +\tconst auto [role, numRequests] = GetParam();\n> \n> \tCaptureUnbalanced capture(camera_);\n> \n> -\tcapture.configure(role);\n> +\tcapture.configure(Span{ 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 A384ABD808\n\tfor ;\n\tFri, 15 Mar 2024 13:00:39 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4478E62C8D;\n\tFri, 15 Mar 2024 14:00:39 +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 C69C66294A\n\tfor ;\n\tFri, 15 Mar 2024 14:00:37 +0100 (CET)","from ideasonboard.com (unknown\n\t[IPv6:2a00:6020:448c:6c00:c477:4921:2179:342c])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id DAFC2E4;\n\tFri, 15 Mar 2024 14:00:13 +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=\"wJN8QpSY\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1710507614;\n\tbh=Z8geqX0RWNvXkRbgAD1GkGkGxOJtdRCvDvxTeg7r9IM=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=wJN8QpSYVaVujb3TasuKOP2vpzZoshWzDjF+1t5Oz8BiJETO+qjRsqhVTIcmkyIwq\n\tVkKBW1u39uxadtHZLoo/u4X97yHlaiHOanBbxwXqEcbJqZg9EpoH4IoSRhFfRaxi+o\n\t5rdsWY76Vi61pD3gXskG/QgJD28kpyJyb/1D99rE=","Date":"Fri, 15 Mar 2024 14:00:35 +0100","From":"Stefan Klug ","To":"Jacopo Mondi ","Subject":"Re: [libcamera-devel] [PATCH v3 6/7] apps: lc-compliance: Support\n\tmultiple streams in helpers","Message-ID":"<20240315130035.zqevlhnr4sj2mdcg@jasper>","References":"<20231230162912.827669-1-jacopo.mondi@ideasonboard.com>\n\t<20231230162912.827669-7-jacopo.mondi@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20231230162912.827669-7-jacopo.mondi@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","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":29004,"web_url":"https://patchwork.libcamera.org/comment/29004/","msgid":"","date":"2024-03-19T15:59:30","subject":"Re: [libcamera-devel] [PATCH v3 6/7] apps: lc-compliance: Support\n\tmultiple streams in helpers","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Stefan\n\nOn Fri, Mar 15, 2024 at 02:00:35PM +0100, Stefan Klug wrote:\n> Hi Jacopo,\n>\n> thanks for the patch. I did not have a setup with support for\n> multistream. I compiled it and ran the tests which then got skipped (as\n> extpected)\n>\n> On Sat, Dec 30, 2023 at 05:29:11PM +0100, Jacopo Mondi 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> > Signed-off-by: Jacopo Mondi \n> > ---\n> > src/apps/lc-compliance/helpers/capture.cpp | 85 ++++++++++++++-----\n> > src/apps/lc-compliance/helpers/capture.h | 2 +-\n> > src/apps/lc-compliance/tests/capture_test.cpp | 26 +++---\n> > 3 files changed, 76 insertions(+), 37 deletions(-)\n> >\n> > diff --git a/src/apps/lc-compliance/helpers/capture.cpp b/src/apps/lc-compliance/helpers/capture.cpp\n> > index 5aab973f0392..bb95af3d758c 100644\n> > --- a/src/apps/lc-compliance/helpers/capture.cpp\n> > +++ b/src/apps/lc-compliance/helpers/capture.cpp\n> > @@ -7,6 +7,8 @@\n> >\n> > #include \"capture.h\"\n> >\n> > +#include \n> > +\n> > #include \n> >\n> > using namespace libcamera;\n> > @@ -22,15 +24,27 @@ Capture::~Capture()\n> > \tstop();\n> > }\n> >\n> > -void Capture::configure(StreamRole role)\n> > +void Capture::configure(Span roles)\n> > {\n> > -\tconfig_ = camera_->generateConfiguration({ role });\n> > +\tconfig_ = camera_->generateConfiguration(roles);\n> >\n> > \tif (!config_) {\n> > -\t\tstd::cout << \"Role not supported by camera\" << std::endl;\n> > +\t\tstd::cout << \"Roles 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>\n> Nit: Not sure about the rules here, but every comment I saw just ended with */ on the last line.\n>\n\nThis is clearly wrong, I'll fix\n\n> > +\tauto largest =\n> > +\t\tstd::max_element(config_->begin(), config_->end(),\n> > +\t\t\t\t [](StreamConfiguration &l, StreamConfiguration &r)\n> > +\t\t\t\t { return l.bufferCount < r.bufferCount; });\n> > +\n> > +\tfor (auto &stream : *config_)\n> > +\t\tstream.bufferCount = largest->bufferCount;\n> > +\n> > \tif (config_->validate() != CameraConfiguration::Valid) {\n> > \t\tconfig_.reset();\n> > \t\tFAIL() << \"Configuration not valid\";\n> > @@ -44,11 +58,17 @@ 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> > +\tunsigned int i = 0;\n> > +\tfor (auto const &config : *config_) {\n> > +\t\tStream *stream = config.stream();\n> > +\t\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> > +\t\tASSERT_GE(count, 0) << \"Failed to allocate buffers for stream \" << i;\n> > +\t\tEXPECT_EQ(count, config.bufferCount)\n> > +\t\t\t<< \"Allocated less buffers than expected for stream \" << i;\n> > +\n> > +\t\t++i;\n> > +\t}\n> >\n> > \tcamera_->requestCompleted.connect(this, &Capture::requestComplete);\n> >\n> > @@ -64,9 +84,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 (auto const &config : *config_) {\n> > +\t\tStream *stream = config.stream();\n> > +\t\tallocator_->free(stream);\n> > +\t}\n> > }\n> >\n> > /* CaptureBalanced */\n> > @@ -80,14 +103,12 @@ void CaptureBalanced::capture(unsigned int numRequests)\n> > {\n> > \tstart();\n> >\n> > -\tStream *stream = config_->at(0).stream();\n> > -\tconst std::vector> &buffers = allocator_->buffers(stream);\n> > -\n> > \t/* No point in testing less requests then the camera depth. */\n> > -\tif (buffers.size() > numRequests) {\n> > -\t\tstd::cout << \"Camera needs \" + std::to_string(buffers.size())\n> > -\t\t\t+ \" requests, can't test only \"\n> > -\t\t\t+ std::to_string(numRequests) << std::endl;\n> > +\tconst unsigned int bufferCount = config_->at(0).bufferCount;\n> > +\tif (bufferCount > numRequests) {\n> > +\t\tstd::cout << \"Camera needs \" << bufferCount\n> > +\t\t\t << \" requests, can't test only \" << numRequests\n> > +\t\t\t << std::endl;\n>\n> I don't know the details here. Can different streams have different\n> bufferCount? I guess that is unlikely. Maybe add a comment?\n>\n\nTheoretically they could, as bufferCount is part of\nStreamConfiguration. In practice the bufferCount field is ill-defined\nand gets populated with the number of min buffers required by the\nvideo device node, which doesn't differ between capture devices (from\nwhich each stream is generated from) in any platform I know of.\n\nI can add a \\todo to keep track of this\n\n> > \t\tGTEST_SKIP();\n> > \t}\n> >\n> > @@ -96,11 +117,21 @@ void CaptureBalanced::capture(unsigned int numRequests)\n> > \tcaptureLimit_ = numRequests;\n> >\n> > \t/* Queue the recommended number of requests. */\n> > -\tfor (const std::unique_ptr &buffer : buffers) {\n> > +\tfor (unsigned int bufferIdx = 0; bufferIdx < bufferCount; ++bufferIdx) {\n> > \t\tstd::unique_ptr request = camera_->createRequest();\n> > \t\tASSERT_TRUE(request) << \"Can't create request\";\n> >\n> > -\t\tASSERT_EQ(request->addBuffer(stream, buffer.get()), 0) << \"Can't set buffer for request\";\n> > +\t\t/* Add buffers for each stream. */\n> > +\t\tunsigned int i = 0;\n> > +\t\tfor (const auto &config : *config_) {\n> > +\t\t\tStream *stream = config.stream();\n> > +\t\t\tconst auto &buffers = allocator_->buffers(stream);\n> > +\n> > +\t\t\tASSERT_EQ(request->addBuffer(stream, buffers[bufferIdx].get()), 0)\n> > +\t\t\t\t<< \"Can't add buffers for stream \" << i;\n> > +\n> > +\t\t\t++i;\n> > +\t\t}\n> >\n> > \t\tASSERT_EQ(queueRequest(request.get()), 0) << \"Failed to queue request\";\n> >\n> > @@ -152,18 +183,26 @@ void CaptureUnbalanced::capture(unsigned int numRequests)\n> > {\n> > \tstart();\n> >\n> > -\tStream *stream = config_->at(0).stream();\n> > -\tconst std::vector> &buffers = allocator_->buffers(stream);\n> > -\n> > \tcaptureCount_ = 0;\n> > \tcaptureLimit_ = numRequests;\n> >\n> > \t/* Queue the recommended number of requests. */\n> > -\tfor (const std::unique_ptr &buffer : buffers) {\n> > +\tconst unsigned int bufferCount = config_->at(0).bufferCount;\n> > +\tfor (unsigned int bufferIdx = 0; bufferIdx < bufferCount; ++bufferIdx) {\n> > \t\tstd::unique_ptr request = camera_->createRequest();\n> > \t\tASSERT_TRUE(request) << \"Can't create request\";\n> >\n> > -\t\tASSERT_EQ(request->addBuffer(stream, buffer.get()), 0) << \"Can't set buffer for request\";\n> > +\t\t/* Add buffers for each stream. */\n> > +\t\tunsigned int i = 0;\n> > +\t\tfor (const auto &config : *config_) {\n> > +\t\t\tStream *stream = config.stream();\n> > +\t\t\tconst auto &buffers = allocator_->buffers(stream);\n> > +\n> > +\t\t\tASSERT_EQ(request->addBuffer(stream, buffers[bufferIdx].get()), 0)\n> > +\t\t\t\t<< \"Can't add buffers for stream \" << i;\n> > +\n> > +\t\t\t++i;\n> > +\t\t}\n>\n> Same question as above. If the bufferCount could differ between the\n> streams, this might be problematic.\n>\n> Cheers,\n> Stefan\n>\n> >\n> > \t\tASSERT_EQ(camera_->queueRequest(request.get()), 0) << \"Failed to queue request\";\n> >\n> > diff --git a/src/apps/lc-compliance/helpers/capture.h b/src/apps/lc-compliance/helpers/capture.h\n> > index 0574ab1c7ac7..3e2b2889244d 100644\n> > --- a/src/apps/lc-compliance/helpers/capture.h\n> > +++ b/src/apps/lc-compliance/helpers/capture.h\n> > @@ -16,7 +16,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 284d36307619..3d3cc97791d9 100644\n> > --- a/src/apps/lc-compliance/tests/capture_test.cpp\n> > +++ b/src/apps/lc-compliance/tests/capture_test.cpp\n> > @@ -17,14 +17,14 @@\n> > using namespace libcamera;\n> >\n> > const std::vector NUMREQUESTS = { 1, 2, 3, 5, 8, 13, 21, 34, 55, 89 };\n> > -const std::vector ROLES = {\n> > -\tStreamRole::Raw,\n> > -\tStreamRole::StillCapture,\n> > -\tStreamRole::VideoRecording,\n> > -\tStreamRole::Viewfinder\n> > +const std::vector> ROLES = {\n> > +\t{ StreamRole::Raw },\n> > +\t{ StreamRole::StillCapture },\n> > +\t{ StreamRole::VideoRecording },\n> > +\t{ StreamRole::Viewfinder },\n> > };\n> >\n> > -class SingleStream : public testing::TestWithParam>\n> > +class SingleStream : public testing::TestWithParam, int>>\n> > {\n> > public:\n> > \tstatic std::string nameParameters(const testing::TestParamInfo &info);\n> > @@ -67,7 +67,7 @@ std::string SingleStream::nameParameters(const testing::TestParamInfo > \t\t{ StreamRole::Viewfinder, \"Viewfinder\" }\n> > \t};\n> >\n> > -\tstd::string roleName = rolesMap[std::get<0>(info.param)];\n> > +\tstd::string roleName = rolesMap[std::get<0>(info.param)[0]];\n> > \tstd::string numRequestsName = std::to_string(std::get<1>(info.param));\n> >\n> > \treturn roleName + \"_\" + numRequestsName;\n> > @@ -82,11 +82,11 @@ std::string SingleStream::nameParameters(const testing::TestParamInfo > */\n> > TEST_P(SingleStream, Capture)\n> > {\n> > -\tauto [role, numRequests] = GetParam();\n> > +\tconst auto [role, numRequests] = GetParam();\n> >\n> > \tCaptureBalanced capture(camera_);\n> >\n> > -\tcapture.configure(role);\n> > +\tcapture.configure(Span{ role });\n> >\n> > \tcapture.capture(numRequests);\n> > }\n> > @@ -100,12 +100,12 @@ TEST_P(SingleStream, Capture)\n> > */\n> > TEST_P(SingleStream, CaptureStartStop)\n> > {\n> > -\tauto [role, numRequests] = GetParam();\n> > +\tconst auto [role, numRequests] = GetParam();\n> > \tunsigned int numRepeats = 3;\n> >\n> > \tCaptureBalanced capture(camera_);\n> >\n> > -\tcapture.configure(role);\n> > +\tcapture.configure(Span{ role });\n> >\n> > \tfor (unsigned int starts = 0; starts < numRepeats; starts++)\n> > \t\tcapture.capture(numRequests);\n> > @@ -120,11 +120,11 @@ TEST_P(SingleStream, CaptureStartStop)\n> > */\n> > TEST_P(SingleStream, UnbalancedStop)\n> > {\n> > -\tauto [role, numRequests] = GetParam();\n> > +\tconst auto [role, numRequests] = GetParam();\n> >\n> > \tCaptureUnbalanced capture(camera_);\n> >\n> > -\tcapture.configure(role);\n> > +\tcapture.configure(Span{ 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 D07E7BD160\n\tfor ;\n\tTue, 19 Mar 2024 15:59:36 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id F30AA62CAA;\n\tTue, 19 Mar 2024 16:59:35 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 8000562CA8\n\tfor ;\n\tTue, 19 Mar 2024 16:59:34 +0100 (CET)","from ideasonboard.com (unknown\n\t[IPv6:2001:b07:5d2e:52c9:cc1e:e404:491f:e6ea])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 395E17E9;\n\tTue, 19 Mar 2024 16:59:07 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"Leo80ijW\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1710863947;\n\tbh=soLUM4vztW2qSTUjEL1vaI9cIfTjj1789tYqPtxS7Ok=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Leo80ijWebr7O5FhBEp1qQHrew96Xx6exr/YFANeUySPZ+MrGmqFwgHjVPKpGZak4\n\tRaAMtsqEyuoW5W6T22arjmbkFVTTAUZVLrR8NrtO7Ywo6fG9ptAS38VkrrE1XTH66b\n\tWp61hTuPeysnw3NiXeFQpZG18ez2RSW4RmoefNqE=","Date":"Tue, 19 Mar 2024 16:59:30 +0100","From":"Jacopo Mondi ","To":"Stefan Klug ","Cc":"Jacopo Mondi , \n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [libcamera-devel] [PATCH v3 6/7] apps: lc-compliance: Support\n\tmultiple streams in helpers","Message-ID":"","References":"<20231230162912.827669-1-jacopo.mondi@ideasonboard.com>\n\t<20231230162912.827669-7-jacopo.mondi@ideasonboard.com>\n\t<20240315130035.zqevlhnr4sj2mdcg@jasper>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20240315130035.zqevlhnr4sj2mdcg@jasper>","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":29005,"web_url":"https://patchwork.libcamera.org/comment/29005/","msgid":"","date":"2024-03-19T16:03:01","subject":"Re: [libcamera-devel] [PATCH v3 6/7] apps: lc-compliance: Support\n\tmultiple streams in helpers","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hello again\n\nOn Tue, Mar 19, 2024 at 04:59:30PM +0100, Jacopo Mondi wrote:\n> Hi Stefan\n>\n> On Fri, Mar 15, 2024 at 02:00:35PM +0100, Stefan Klug wrote:\n> > Hi Jacopo,\n> >\n> > thanks for the patch. I did not have a setup with support for\n> > multistream. I compiled it and ran the tests which then got skipped (as\n> > extpected)\n> >\n> > On Sat, Dec 30, 2023 at 05:29:11PM +0100, Jacopo Mondi 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> > > Signed-off-by: Jacopo Mondi \n> > > ---\n> > > src/apps/lc-compliance/helpers/capture.cpp | 85 ++++++++++++++-----\n> > > src/apps/lc-compliance/helpers/capture.h | 2 +-\n> > > src/apps/lc-compliance/tests/capture_test.cpp | 26 +++---\n> > > 3 files changed, 76 insertions(+), 37 deletions(-)\n> > >\n> > > diff --git a/src/apps/lc-compliance/helpers/capture.cpp b/src/apps/lc-compliance/helpers/capture.cpp\n> > > index 5aab973f0392..bb95af3d758c 100644\n> > > --- a/src/apps/lc-compliance/helpers/capture.cpp\n> > > +++ b/src/apps/lc-compliance/helpers/capture.cpp\n> > > @@ -7,6 +7,8 @@\n> > >\n> > > #include \"capture.h\"\n> > >\n> > > +#include \n> > > +\n> > > #include \n> > >\n> > > using namespace libcamera;\n> > > @@ -22,15 +24,27 @@ Capture::~Capture()\n> > > \tstop();\n> > > }\n> > >\n> > > -void Capture::configure(StreamRole role)\n> > > +void Capture::configure(Span roles)\n> > > {\n> > > -\tconfig_ = camera_->generateConfiguration({ role });\n> > > +\tconfig_ = camera_->generateConfiguration(roles);\n> > >\n> > > \tif (!config_) {\n> > > -\t\tstd::cout << \"Role not supported by camera\" << std::endl;\n> > > +\t\tstd::cout << \"Roles 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> >\n> > Nit: Not sure about the rules here, but every comment I saw just ended with */ on the last line.\n> >\n>\n> This is clearly wrong, I'll fix\n>\n> > > +\tauto largest =\n> > > +\t\tstd::max_element(config_->begin(), config_->end(),\n> > > +\t\t\t\t [](StreamConfiguration &l, StreamConfiguration &r)\n> > > +\t\t\t\t { return l.bufferCount < r.bufferCount; });\n> > > +\n> > > +\tfor (auto &stream : *config_)\n> > > +\t\tstream.bufferCount = largest->bufferCount;\n> > > +\n> > > \tif (config_->validate() != CameraConfiguration::Valid) {\n> > > \t\tconfig_.reset();\n> > > \t\tFAIL() << \"Configuration not valid\";\n> > > @@ -44,11 +58,17 @@ 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> > > +\tunsigned int i = 0;\n> > > +\tfor (auto const &config : *config_) {\n> > > +\t\tStream *stream = config.stream();\n> > > +\t\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> > > +\t\tASSERT_GE(count, 0) << \"Failed to allocate buffers for stream \" << i;\n> > > +\t\tEXPECT_EQ(count, config.bufferCount)\n> > > +\t\t\t<< \"Allocated less buffers than expected for stream \" << i;\n> > > +\n> > > +\t\t++i;\n> > > +\t}\n> > >\n> > > \tcamera_->requestCompleted.connect(this, &Capture::requestComplete);\n> > >\n> > > @@ -64,9 +84,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 (auto const &config : *config_) {\n> > > +\t\tStream *stream = config.stream();\n> > > +\t\tallocator_->free(stream);\n> > > +\t}\n> > > }\n> > >\n> > > /* CaptureBalanced */\n> > > @@ -80,14 +103,12 @@ void CaptureBalanced::capture(unsigned int numRequests)\n> > > {\n> > > \tstart();\n> > >\n> > > -\tStream *stream = config_->at(0).stream();\n> > > -\tconst std::vector> &buffers = allocator_->buffers(stream);\n> > > -\n> > > \t/* No point in testing less requests then the camera depth. */\n> > > -\tif (buffers.size() > numRequests) {\n> > > -\t\tstd::cout << \"Camera needs \" + std::to_string(buffers.size())\n> > > -\t\t\t+ \" requests, can't test only \"\n> > > -\t\t\t+ std::to_string(numRequests) << std::endl;\n> > > +\tconst unsigned int bufferCount = config_->at(0).bufferCount;\n> > > +\tif (bufferCount > numRequests) {\n> > > +\t\tstd::cout << \"Camera needs \" << bufferCount\n> > > +\t\t\t << \" requests, can't test only \" << numRequests\n> > > +\t\t\t << std::endl;\n> >\n> > I don't know the details here. Can different streams have different\n> > bufferCount? I guess that is unlikely. Maybe add a comment?\n> >\n>\n> Theoretically they could, as bufferCount is part of\n> StreamConfiguration. In practice the bufferCount field is ill-defined\n> and gets populated with the number of min buffers required by the\n> video device node, which doesn't differ between capture devices (from\n> which each stream is generated from) in any platform I know of.\n>\n> I can add a \\todo to keep track of this\n>\n\nAh ups, I already had one\n\n \\todo: Should all streams from a Camera have the same buffer count ?\n\n> > > \t\tGTEST_SKIP();\n> > > \t}\n> > >\n> > > @@ -96,11 +117,21 @@ void CaptureBalanced::capture(unsigned int numRequests)\n> > > \tcaptureLimit_ = numRequests;\n> > >\n> > > \t/* Queue the recommended number of requests. */\n> > > -\tfor (const std::unique_ptr &buffer : buffers) {\n> > > +\tfor (unsigned int bufferIdx = 0; bufferIdx < bufferCount; ++bufferIdx) {\n> > > \t\tstd::unique_ptr request = camera_->createRequest();\n> > > \t\tASSERT_TRUE(request) << \"Can't create request\";\n> > >\n> > > -\t\tASSERT_EQ(request->addBuffer(stream, buffer.get()), 0) << \"Can't set buffer for request\";\n> > > +\t\t/* Add buffers for each stream. */\n> > > +\t\tunsigned int i = 0;\n> > > +\t\tfor (const auto &config : *config_) {\n> > > +\t\t\tStream *stream = config.stream();\n> > > +\t\t\tconst auto &buffers = allocator_->buffers(stream);\n> > > +\n> > > +\t\t\tASSERT_EQ(request->addBuffer(stream, buffers[bufferIdx].get()), 0)\n> > > +\t\t\t\t<< \"Can't add buffers for stream \" << i;\n> > > +\n> > > +\t\t\t++i;\n> > > +\t\t}\n> > >\n> > > \t\tASSERT_EQ(queueRequest(request.get()), 0) << \"Failed to queue request\";\n> > >\n> > > @@ -152,18 +183,26 @@ void CaptureUnbalanced::capture(unsigned int numRequests)\n> > > {\n> > > \tstart();\n> > >\n> > > -\tStream *stream = config_->at(0).stream();\n> > > -\tconst std::vector> &buffers = allocator_->buffers(stream);\n> > > -\n> > > \tcaptureCount_ = 0;\n> > > \tcaptureLimit_ = numRequests;\n> > >\n> > > \t/* Queue the recommended number of requests. */\n> > > -\tfor (const std::unique_ptr &buffer : buffers) {\n> > > +\tconst unsigned int bufferCount = config_->at(0).bufferCount;\n> > > +\tfor (unsigned int bufferIdx = 0; bufferIdx < bufferCount; ++bufferIdx) {\n> > > \t\tstd::unique_ptr request = camera_->createRequest();\n> > > \t\tASSERT_TRUE(request) << \"Can't create request\";\n> > >\n> > > -\t\tASSERT_EQ(request->addBuffer(stream, buffer.get()), 0) << \"Can't set buffer for request\";\n> > > +\t\t/* Add buffers for each stream. */\n> > > +\t\tunsigned int i = 0;\n> > > +\t\tfor (const auto &config : *config_) {\n> > > +\t\t\tStream *stream = config.stream();\n> > > +\t\t\tconst auto &buffers = allocator_->buffers(stream);\n> > > +\n> > > +\t\t\tASSERT_EQ(request->addBuffer(stream, buffers[bufferIdx].get()), 0)\n> > > +\t\t\t\t<< \"Can't add buffers for stream \" << i;\n> > > +\n> > > +\t\t\t++i;\n> > > +\t\t}\n> >\n> > Same question as above. If the bufferCount could differ between the\n> > streams, this might be problematic.\n> >\n> > Cheers,\n> > Stefan\n> >\n> > >\n> > > \t\tASSERT_EQ(camera_->queueRequest(request.get()), 0) << \"Failed to queue request\";\n> > >\n> > > diff --git a/src/apps/lc-compliance/helpers/capture.h b/src/apps/lc-compliance/helpers/capture.h\n> > > index 0574ab1c7ac7..3e2b2889244d 100644\n> > > --- a/src/apps/lc-compliance/helpers/capture.h\n> > > +++ b/src/apps/lc-compliance/helpers/capture.h\n> > > @@ -16,7 +16,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 284d36307619..3d3cc97791d9 100644\n> > > --- a/src/apps/lc-compliance/tests/capture_test.cpp\n> > > +++ b/src/apps/lc-compliance/tests/capture_test.cpp\n> > > @@ -17,14 +17,14 @@\n> > > using namespace libcamera;\n> > >\n> > > const std::vector NUMREQUESTS = { 1, 2, 3, 5, 8, 13, 21, 34, 55, 89 };\n> > > -const std::vector ROLES = {\n> > > -\tStreamRole::Raw,\n> > > -\tStreamRole::StillCapture,\n> > > -\tStreamRole::VideoRecording,\n> > > -\tStreamRole::Viewfinder\n> > > +const std::vector> ROLES = {\n> > > +\t{ StreamRole::Raw },\n> > > +\t{ StreamRole::StillCapture },\n> > > +\t{ StreamRole::VideoRecording },\n> > > +\t{ StreamRole::Viewfinder },\n> > > };\n> > >\n> > > -class SingleStream : public testing::TestWithParam>\n> > > +class SingleStream : public testing::TestWithParam, int>>\n> > > {\n> > > public:\n> > > \tstatic std::string nameParameters(const testing::TestParamInfo &info);\n> > > @@ -67,7 +67,7 @@ std::string SingleStream::nameParameters(const testing::TestParamInfo > > \t\t{ StreamRole::Viewfinder, \"Viewfinder\" }\n> > > \t};\n> > >\n> > > -\tstd::string roleName = rolesMap[std::get<0>(info.param)];\n> > > +\tstd::string roleName = rolesMap[std::get<0>(info.param)[0]];\n> > > \tstd::string numRequestsName = std::to_string(std::get<1>(info.param));\n> > >\n> > > \treturn roleName + \"_\" + numRequestsName;\n> > > @@ -82,11 +82,11 @@ std::string SingleStream::nameParameters(const testing::TestParamInfo > > */\n> > > TEST_P(SingleStream, Capture)\n> > > {\n> > > -\tauto [role, numRequests] = GetParam();\n> > > +\tconst auto [role, numRequests] = GetParam();\n> > >\n> > > \tCaptureBalanced capture(camera_);\n> > >\n> > > -\tcapture.configure(role);\n> > > +\tcapture.configure(Span{ role });\n> > >\n> > > \tcapture.capture(numRequests);\n> > > }\n> > > @@ -100,12 +100,12 @@ TEST_P(SingleStream, Capture)\n> > > */\n> > > TEST_P(SingleStream, CaptureStartStop)\n> > > {\n> > > -\tauto [role, numRequests] = GetParam();\n> > > +\tconst auto [role, numRequests] = GetParam();\n> > > \tunsigned int numRepeats = 3;\n> > >\n> > > \tCaptureBalanced capture(camera_);\n> > >\n> > > -\tcapture.configure(role);\n> > > +\tcapture.configure(Span{ role });\n> > >\n> > > \tfor (unsigned int starts = 0; starts < numRepeats; starts++)\n> > > \t\tcapture.capture(numRequests);\n> > > @@ -120,11 +120,11 @@ TEST_P(SingleStream, CaptureStartStop)\n> > > */\n> > > TEST_P(SingleStream, UnbalancedStop)\n> > > {\n> > > -\tauto [role, numRequests] = GetParam();\n> > > +\tconst auto [role, numRequests] = GetParam();\n> > >\n> > > \tCaptureUnbalanced capture(camera_);\n> > >\n> > > -\tcapture.configure(role);\n> > > +\tcapture.configure(Span{ 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 9BCB5BD160\n\tfor ;\n\tTue, 19 Mar 2024 16:03:06 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id BE62D62D2B;\n\tTue, 19 Mar 2024 17:03:05 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 7678062CA8\n\tfor ;\n\tTue, 19 Mar 2024 17:03:04 +0100 (CET)","from ideasonboard.com (unknown\n\t[IPv6:2001:b07:5d2e:52c9:cc1e:e404:491f:e6ea])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 8EDBF480;\n\tTue, 19 Mar 2024 17:02:37 +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=\"Gim9imd1\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1710864157;\n\tbh=4BEiEczzUhEe4SjEh5y/4HidpRH0KNd2wjxEm0xvTGs=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Gim9imd1/TLAEOzUGoY0T/RYGm7YTnYhvDSlW2l/bTZM1EqTVuQfXZSAJZckJyBSX\n\twq2UVSMnO60fZysBh6eV7fXfZjWghkSZLEHgAaWnYPSSQmyqlmebofZJJVu8dd2CBp\n\t9ZnuPjwcLIGltSgCcp8yLY1blEEROxbqsE0rQ3/4=","Date":"Tue, 19 Mar 2024 17:03:01 +0100","From":"Jacopo Mondi ","To":"Jacopo Mondi ","Cc":"Stefan Klug , \n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [libcamera-devel] [PATCH v3 6/7] apps: lc-compliance: Support\n\tmultiple streams in helpers","Message-ID":"","References":"<20231230162912.827669-1-jacopo.mondi@ideasonboard.com>\n\t<20231230162912.827669-7-jacopo.mondi@ideasonboard.com>\n\t<20240315130035.zqevlhnr4sj2mdcg@jasper>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","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":29009,"web_url":"https://patchwork.libcamera.org/comment/29009/","msgid":"<20240320080902.gsieuvfwoyceugzo@jasper>","date":"2024-03-20T08:09:02","subject":"Re: [libcamera-devel] [PATCH v3 6/7] apps: lc-compliance: Support\n\tmultiple streams in helpers","submitter":{"id":184,"url":"https://patchwork.libcamera.org/api/people/184/","name":"Stefan Klug","email":"stefan.klug@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Tue, Mar 19, 2024 at 05:03:01PM +0100, Jacopo Mondi wrote:\n> Hello again\n> \n> On Tue, Mar 19, 2024 at 04:59:30PM +0100, Jacopo Mondi wrote:\n> > Hi Stefan\n> >\n> > On Fri, Mar 15, 2024 at 02:00:35PM +0100, Stefan Klug wrote:\n> > > Hi Jacopo,\n> > >\n> > > thanks for the patch. I did not have a setup with support for\n> > > multistream. I compiled it and ran the tests which then got skipped (as\n> > > extpected)\n> > >\n> > > On Sat, Dec 30, 2023 at 05:29:11PM +0100, Jacopo Mondi 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> > > > Signed-off-by: Jacopo Mondi \n> > > > ---\n> > > > src/apps/lc-compliance/helpers/capture.cpp | 85 ++++++++++++++-----\n> > > > src/apps/lc-compliance/helpers/capture.h | 2 +-\n> > > > src/apps/lc-compliance/tests/capture_test.cpp | 26 +++---\n> > > > 3 files changed, 76 insertions(+), 37 deletions(-)\n> > > >\n> > > > diff --git a/src/apps/lc-compliance/helpers/capture.cpp b/src/apps/lc-compliance/helpers/capture.cpp\n> > > > index 5aab973f0392..bb95af3d758c 100644\n> > > > --- a/src/apps/lc-compliance/helpers/capture.cpp\n> > > > +++ b/src/apps/lc-compliance/helpers/capture.cpp\n> > > > @@ -7,6 +7,8 @@\n> > > >\n> > > > #include \"capture.h\"\n> > > >\n> > > > +#include \n> > > > +\n> > > > #include \n> > > >\n> > > > using namespace libcamera;\n> > > > @@ -22,15 +24,27 @@ Capture::~Capture()\n> > > > \tstop();\n> > > > }\n> > > >\n> > > > -void Capture::configure(StreamRole role)\n> > > > +void Capture::configure(Span roles)\n> > > > {\n> > > > -\tconfig_ = camera_->generateConfiguration({ role });\n> > > > +\tconfig_ = camera_->generateConfiguration(roles);\n> > > >\n> > > > \tif (!config_) {\n> > > > -\t\tstd::cout << \"Role not supported by camera\" << std::endl;\n> > > > +\t\tstd::cout << \"Roles 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> > >\n> > > Nit: Not sure about the rules here, but every comment I saw just ended with */ on the last line.\n> > >\n> >\n> > This is clearly wrong, I'll fix\n> >\n> > > > +\tauto largest =\n> > > > +\t\tstd::max_element(config_->begin(), config_->end(),\n> > > > +\t\t\t\t [](StreamConfiguration &l, StreamConfiguration &r)\n> > > > +\t\t\t\t { return l.bufferCount < r.bufferCount; });\n> > > > +\n> > > > +\tfor (auto &stream : *config_)\n> > > > +\t\tstream.bufferCount = largest->bufferCount;\n> > > > +\n> > > > \tif (config_->validate() != CameraConfiguration::Valid) {\n> > > > \t\tconfig_.reset();\n> > > > \t\tFAIL() << \"Configuration not valid\";\n> > > > @@ -44,11 +58,17 @@ 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> > > > +\tunsigned int i = 0;\n> > > > +\tfor (auto const &config : *config_) {\n> > > > +\t\tStream *stream = config.stream();\n> > > > +\t\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> > > > +\t\tASSERT_GE(count, 0) << \"Failed to allocate buffers for stream \" << i;\n> > > > +\t\tEXPECT_EQ(count, config.bufferCount)\n> > > > +\t\t\t<< \"Allocated less buffers than expected for stream \" << i;\n> > > > +\n> > > > +\t\t++i;\n> > > > +\t}\n> > > >\n> > > > \tcamera_->requestCompleted.connect(this, &Capture::requestComplete);\n> > > >\n> > > > @@ -64,9 +84,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 (auto const &config : *config_) {\n> > > > +\t\tStream *stream = config.stream();\n> > > > +\t\tallocator_->free(stream);\n> > > > +\t}\n> > > > }\n> > > >\n> > > > /* CaptureBalanced */\n> > > > @@ -80,14 +103,12 @@ void CaptureBalanced::capture(unsigned int numRequests)\n> > > > {\n> > > > \tstart();\n> > > >\n> > > > -\tStream *stream = config_->at(0).stream();\n> > > > -\tconst std::vector> &buffers = allocator_->buffers(stream);\n> > > > -\n> > > > \t/* No point in testing less requests then the camera depth. */\n> > > > -\tif (buffers.size() > numRequests) {\n> > > > -\t\tstd::cout << \"Camera needs \" + std::to_string(buffers.size())\n> > > > -\t\t\t+ \" requests, can't test only \"\n> > > > -\t\t\t+ std::to_string(numRequests) << std::endl;\n> > > > +\tconst unsigned int bufferCount = config_->at(0).bufferCount;\n> > > > +\tif (bufferCount > numRequests) {\n> > > > +\t\tstd::cout << \"Camera needs \" << bufferCount\n> > > > +\t\t\t << \" requests, can't test only \" << numRequests\n> > > > +\t\t\t << std::endl;\n> > >\n> > > I don't know the details here. Can different streams have different\n> > > bufferCount? I guess that is unlikely. Maybe add a comment?\n> > >\n> >\n> > Theoretically they could, as bufferCount is part of\n> > StreamConfiguration. In practice the bufferCount field is ill-defined\n> > and gets populated with the number of min buffers required by the\n> > video device node, which doesn't differ between capture devices (from\n> > which each stream is generated from) in any platform I know of.\n> >\n> > I can add a \\todo to keep track of this\n> >\n> \n> Ah ups, I already had one\n> \n> \\todo: Should all streams from a Camera have the same buffer count ?\n\nOh I missed that. so \n\nReviewed-by: Stefan Klug \n\nCheers,\nStefan\n\n> \n> > > > \t\tGTEST_SKIP();\n> > > > \t}\n> > > >\n> > > > @@ -96,11 +117,21 @@ void CaptureBalanced::capture(unsigned int numRequests)\n> > > > \tcaptureLimit_ = numRequests;\n> > > >\n> > > > \t/* Queue the recommended number of requests. */\n> > > > -\tfor (const std::unique_ptr &buffer : buffers) {\n> > > > +\tfor (unsigned int bufferIdx = 0; bufferIdx < bufferCount; ++bufferIdx) {\n> > > > \t\tstd::unique_ptr request = camera_->createRequest();\n> > > > \t\tASSERT_TRUE(request) << \"Can't create request\";\n> > > >\n> > > > -\t\tASSERT_EQ(request->addBuffer(stream, buffer.get()), 0) << \"Can't set buffer for request\";\n> > > > +\t\t/* Add buffers for each stream. */\n> > > > +\t\tunsigned int i = 0;\n> > > > +\t\tfor (const auto &config : *config_) {\n> > > > +\t\t\tStream *stream = config.stream();\n> > > > +\t\t\tconst auto &buffers = allocator_->buffers(stream);\n> > > > +\n> > > > +\t\t\tASSERT_EQ(request->addBuffer(stream, buffers[bufferIdx].get()), 0)\n> > > > +\t\t\t\t<< \"Can't add buffers for stream \" << i;\n> > > > +\n> > > > +\t\t\t++i;\n> > > > +\t\t}\n> > > >\n> > > > \t\tASSERT_EQ(queueRequest(request.get()), 0) << \"Failed to queue request\";\n> > > >\n> > > > @@ -152,18 +183,26 @@ void CaptureUnbalanced::capture(unsigned int numRequests)\n> > > > {\n> > > > \tstart();\n> > > >\n> > > > -\tStream *stream = config_->at(0).stream();\n> > > > -\tconst std::vector> &buffers = allocator_->buffers(stream);\n> > > > -\n> > > > \tcaptureCount_ = 0;\n> > > > \tcaptureLimit_ = numRequests;\n> > > >\n> > > > \t/* Queue the recommended number of requests. */\n> > > > -\tfor (const std::unique_ptr &buffer : buffers) {\n> > > > +\tconst unsigned int bufferCount = config_->at(0).bufferCount;\n> > > > +\tfor (unsigned int bufferIdx = 0; bufferIdx < bufferCount; ++bufferIdx) {\n> > > > \t\tstd::unique_ptr request = camera_->createRequest();\n> > > > \t\tASSERT_TRUE(request) << \"Can't create request\";\n> > > >\n> > > > -\t\tASSERT_EQ(request->addBuffer(stream, buffer.get()), 0) << \"Can't set buffer for request\";\n> > > > +\t\t/* Add buffers for each stream. */\n> > > > +\t\tunsigned int i = 0;\n> > > > +\t\tfor (const auto &config : *config_) {\n> > > > +\t\t\tStream *stream = config.stream();\n> > > > +\t\t\tconst auto &buffers = allocator_->buffers(stream);\n> > > > +\n> > > > +\t\t\tASSERT_EQ(request->addBuffer(stream, buffers[bufferIdx].get()), 0)\n> > > > +\t\t\t\t<< \"Can't add buffers for stream \" << i;\n> > > > +\n> > > > +\t\t\t++i;\n> > > > +\t\t}\n> > >\n> > > Same question as above. If the bufferCount could differ between the\n> > > streams, this might be problematic.\n> > >\n> > > Cheers,\n> > > Stefan\n> > >\n> > > >\n> > > > \t\tASSERT_EQ(camera_->queueRequest(request.get()), 0) << \"Failed to queue request\";\n> > > >\n> > > > diff --git a/src/apps/lc-compliance/helpers/capture.h b/src/apps/lc-compliance/helpers/capture.h\n> > > > index 0574ab1c7ac7..3e2b2889244d 100644\n> > > > --- a/src/apps/lc-compliance/helpers/capture.h\n> > > > +++ b/src/apps/lc-compliance/helpers/capture.h\n> > > > @@ -16,7 +16,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 284d36307619..3d3cc97791d9 100644\n> > > > --- a/src/apps/lc-compliance/tests/capture_test.cpp\n> > > > +++ b/src/apps/lc-compliance/tests/capture_test.cpp\n> > > > @@ -17,14 +17,14 @@\n> > > > using namespace libcamera;\n> > > >\n> > > > const std::vector NUMREQUESTS = { 1, 2, 3, 5, 8, 13, 21, 34, 55, 89 };\n> > > > -const std::vector ROLES = {\n> > > > -\tStreamRole::Raw,\n> > > > -\tStreamRole::StillCapture,\n> > > > -\tStreamRole::VideoRecording,\n> > > > -\tStreamRole::Viewfinder\n> > > > +const std::vector> ROLES = {\n> > > > +\t{ StreamRole::Raw },\n> > > > +\t{ StreamRole::StillCapture },\n> > > > +\t{ StreamRole::VideoRecording },\n> > > > +\t{ StreamRole::Viewfinder },\n> > > > };\n> > > >\n> > > > -class SingleStream : public testing::TestWithParam>\n> > > > +class SingleStream : public testing::TestWithParam, int>>\n> > > > {\n> > > > public:\n> > > > \tstatic std::string nameParameters(const testing::TestParamInfo &info);\n> > > > @@ -67,7 +67,7 @@ std::string SingleStream::nameParameters(const testing::TestParamInfo > > > \t\t{ StreamRole::Viewfinder, \"Viewfinder\" }\n> > > > \t};\n> > > >\n> > > > -\tstd::string roleName = rolesMap[std::get<0>(info.param)];\n> > > > +\tstd::string roleName = rolesMap[std::get<0>(info.param)[0]];\n> > > > \tstd::string numRequestsName = std::to_string(std::get<1>(info.param));\n> > > >\n> > > > \treturn roleName + \"_\" + numRequestsName;\n> > > > @@ -82,11 +82,11 @@ std::string SingleStream::nameParameters(const testing::TestParamInfo > > > */\n> > > > TEST_P(SingleStream, Capture)\n> > > > {\n> > > > -\tauto [role, numRequests] = GetParam();\n> > > > +\tconst auto [role, numRequests] = GetParam();\n> > > >\n> > > > \tCaptureBalanced capture(camera_);\n> > > >\n> > > > -\tcapture.configure(role);\n> > > > +\tcapture.configure(Span{ role });\n> > > >\n> > > > \tcapture.capture(numRequests);\n> > > > }\n> > > > @@ -100,12 +100,12 @@ TEST_P(SingleStream, Capture)\n> > > > */\n> > > > TEST_P(SingleStream, CaptureStartStop)\n> > > > {\n> > > > -\tauto [role, numRequests] = GetParam();\n> > > > +\tconst auto [role, numRequests] = GetParam();\n> > > > \tunsigned int numRepeats = 3;\n> > > >\n> > > > \tCaptureBalanced capture(camera_);\n> > > >\n> > > > -\tcapture.configure(role);\n> > > > +\tcapture.configure(Span{ role });\n> > > >\n> > > > \tfor (unsigned int starts = 0; starts < numRepeats; starts++)\n> > > > \t\tcapture.capture(numRequests);\n> > > > @@ -120,11 +120,11 @@ TEST_P(SingleStream, CaptureStartStop)\n> > > > */\n> > > > TEST_P(SingleStream, UnbalancedStop)\n> > > > {\n> > > > -\tauto [role, numRequests] = GetParam();\n> > > > +\tconst auto [role, numRequests] = GetParam();\n> > > >\n> > > > \tCaptureUnbalanced capture(camera_);\n> > > >\n> > > > -\tcapture.configure(role);\n> > > > +\tcapture.configure(Span{ 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 A57B7C3272\n\tfor ;\n\tWed, 20 Mar 2024 08:09:08 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 86AB562D2D;\n\tWed, 20 Mar 2024 09:09:07 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 42C6961C5B\n\tfor ;\n\tWed, 20 Mar 2024 09:09:05 +0100 (CET)","from ideasonboard.com (unknown\n\t[IPv6:2a00:6020:448c:6c00:76a8:42ba:7e03:1ce4])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id CCB323F1;\n\tWed, 20 Mar 2024 09:08:37 +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=\"pbc043aF\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1710922117;\n\tbh=97CtVGpEz2mIHGjwqERAeCOMw2dO7cv15uCSCgmBZ/w=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=pbc043aFQdWGg3MwWbmeJwkx6kMmoxUU1+E0TAO1rZwHu+H/4PgE8W5a3fUW2ZrJ+\n\tlTZunWkOAMK1zC/7oKoac9lRGw5DmRC6yFpTPHHNWzgsamxeqQZtb5Ygx06iMNE2U1\n\tU0ocoF+G3CRHLIM4t2UcnAazwXo6w1lEISPSbk5o=","Date":"Wed, 20 Mar 2024 09:09:02 +0100","From":"Stefan Klug ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [libcamera-devel] [PATCH v3 6/7] apps: lc-compliance: Support\n\tmultiple streams in helpers","Message-ID":"<20240320080902.gsieuvfwoyceugzo@jasper>","References":"<20231230162912.827669-1-jacopo.mondi@ideasonboard.com>\n\t<20231230162912.827669-7-jacopo.mondi@ideasonboard.com>\n\t<20240315130035.zqevlhnr4sj2mdcg@jasper>\n\t\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","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\" "}}]