[{"id":26528,"web_url":"https://patchwork.libcamera.org/comment/26528/","msgid":"<20230302103152.nfchw5khjg34lblw@uno.localdomain>","date":"2023-03-02T10:31:52","subject":"Re: [libcamera-devel] [PATCH v1 6/8] libcamera: apps: lcc: Add\n\tmulti-stream capture test framework","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Naush\n thanks for adding this, it's a lot of work!\n\nOn Fri, Feb 03, 2023 at 09:44:22AM +0000, Naushir Patuck via libcamera-devel wrote:\n> Add a framework for testing multi-stream captures with lcc. To start\n> with, a single test class, MultiStream, has been implemented. This class\n> is based off the SimpleCaptureBalanced behavior, and tests that the\n> pipeline handler completes the exact number of requests queued to it.\n>\n> Add a test to capture_test.cpp using the MultiStream class to test the\n> pipeline handler running with two simultaneous streams. The test\n> framework permutate across a list of named stream pairs.\n>\n> Signed-off-by: Naushir Patuck \n> ---\n> src/apps/lc-compliance/capture_test.cpp | 77 ++++++++++\n> src/apps/lc-compliance/meson.build | 1 +\n> src/apps/lc-compliance/multi_capture.cpp | 187 +++++++++++++++++++++++\n> src/apps/lc-compliance/multi_capture.h | 61 ++++++++\n> 4 files changed, 326 insertions(+)\n> create mode 100644 src/apps/lc-compliance/multi_capture.cpp\n> create mode 100644 src/apps/lc-compliance/multi_capture.h\n>\n> diff --git a/src/apps/lc-compliance/capture_test.cpp b/src/apps/lc-compliance/capture_test.cpp\n> index 37138dfb3d2e..8d534161e985 100644\n> --- a/src/apps/lc-compliance/capture_test.cpp\n> +++ b/src/apps/lc-compliance/capture_test.cpp\n> @@ -11,6 +11,7 @@\n> #include \n>\n> #include \"environment.h\"\n> +#include \"multi_capture.h\"\n> #include \"simple_capture.h\"\n>\n> using namespace libcamera;\n> @@ -32,6 +33,13 @@ const std::vector ROLES = {\n> \tStreamRole::Viewfinder\n> };\n>\n> +const std::vector> MULTIROLES = {\n> +\t{ StreamRole::Raw, StreamRole::StillCapture },\n> +\t{ StreamRole::Raw, StreamRole::VideoRecording },\n> +\t{ StreamRole::StillCapture, StreamRole::VideoRecording },\n> +\t{ StreamRole::VideoRecording, StreamRole::VideoRecording },\n> +};\n> +\n> } /* namespace */\n>\n> class SingleStream : public testing::TestWithParam>\n> @@ -137,3 +145,72 @@ INSTANTIATE_TEST_SUITE_P(CaptureTests,\n> \t\t\t testing::Combine(testing::ValuesIn(ROLES),\n> \t\t\t\t\t testing::ValuesIn(NUMREQUESTS)),\n> \t\t\t SingleStream::nameParameters);\n> +\n> +class MultiStream : public testing::TestWithParam, int>>\n> +{\n> +public:\n> +\tstatic std::string nameParameters(const testing::TestParamInfo &info);\n> +\n> +protected:\n> +\tvoid SetUp() override;\n> +\tvoid TearDown() override;\n> +\n> +\tstd::shared_ptr camera_;\n> +};\n> +\n> +/*\n> + * We use gtest's SetUp() and TearDown() instead of constructor and destructor\n> + * in order to be able to assert on them.\n> + */\n> +void MultiStream::SetUp()\n> +{\n> +\tEnvironment *env = Environment::get();\n> +\n> +\tcamera_ = env->cm()->get(env->cameraId());\n> +\n> +\tASSERT_EQ(camera_->acquire(), 0);\n> +}\n> +\n> +void MultiStream::TearDown()\n> +{\n> +\tif (!camera_)\n> +\t\treturn;\n> +\n> +\tcamera_->release();\n> +\tcamera_.reset();\n> +}\n> +\n> +std::string MultiStream::nameParameters(const testing::TestParamInfo &info)\n> +{\n> +\tstd::string roleName = rolesMap[std::get<0>(info.param).first] + \"_\" +\n> +\t\t\t rolesMap[std::get<0>(info.param).second];\n> +\tstd::string numRequestsName = std::to_string(std::get<1>(info.param));\n> +\n> +\treturn roleName + \"_\" + numRequestsName;\n> +}\n> +\n> +/*\n> + * Test multi-stream capture cycles\n> + *\n> + * Makes sure the camera completes the exact number of requests queued. Example\n> + * failure is a camera that completes less requests than the number of requests\n> + * queued.\n> + */\n> +TEST_P(MultiStream, Capture)\n> +{\n> +\tconstexpr unsigned int NumStreams = 2;\n> +\n> +\tauto [roles, numRequests] = GetParam();\n> +\n> +\tMultiCapture capture(camera_);\n> +\n> +\tcapture.configure({ roles.first, roles.second });\n> +\n> +\tcapture.capture(numRequests, NumStreams);\n> +}\n> +\n> +INSTANTIATE_TEST_SUITE_P(MultiCaptureTests,\n> +\t\t\t MultiStream,\n> +\t\t\t testing::Combine(testing::ValuesIn(MULTIROLES),\n> +\t\t\t\t\t testing::ValuesIn(NUMREQUESTS)),\n> +\t\t\t MultiStream::nameParameters);\n> diff --git a/src/apps/lc-compliance/meson.build b/src/apps/lc-compliance/meson.build\n> index 51d9075ac30b..0357cda2f301 100644\n> --- a/src/apps/lc-compliance/meson.build\n> +++ b/src/apps/lc-compliance/meson.build\n> @@ -13,6 +13,7 @@ lc_compliance_enabled = true\n> lc_compliance_sources = files([\n> 'environment.cpp',\n> 'main.cpp',\n> + 'multi_capture.cpp',\n> 'simple_capture.cpp',\n> 'capture_test.cpp',\n> ])\n> diff --git a/src/apps/lc-compliance/multi_capture.cpp b/src/apps/lc-compliance/multi_capture.cpp\n> new file mode 100644\n> index 000000000000..23becf964fd7\n> --- /dev/null\n> +++ b/src/apps/lc-compliance/multi_capture.cpp\n> @@ -0,0 +1,187 @@\n> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> +/*\n> + * Copyright (C) 2023, Raspberry Pi Ltd\n> + *\n> + * multi_capture.cpp - Multi-stream capture helper\n> + */\n> +#include \"multi_capture.h\"\n> +\n> +#include \n> +\n> +#include \n> +\n> +using namespace libcamera;\n> +\n> +MultiCaptureBase::MultiCaptureBase(std::shared_ptr camera)\n> +\t: loop_(nullptr), camera_(camera),\n> +\t allocator_(std::make_unique(camera))\n> +{\n> +}\n> +\n> +MultiCaptureBase::~MultiCaptureBase()\n> +{\n> +\tstop();\n> +}\n> +\n> +void MultiCaptureBase::configure(const libcamera::StreamRoles &roles)\n> +{\n> +\tconfig_ = camera_->generateConfiguration(roles);\n> +\n> +\tif (!config_) {\n> +\t\tstd::cout << \"Roles not supported by camera\" << std::endl;\n> +\t\tGTEST_SKIP();\n> +\t}\n> +\n> +\t/* Set the buffer counts to the largest value across all streams */\n> +\tauto largest =\n> +\t\tstd::max_element(config_->begin(), config_->end(),\n> +\t\t\t\t [](libcamera::StreamConfiguration &l, libcamera::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> +\tupdateConfig();\n> +\n> +\tif (config_->validate() != CameraConfiguration::Valid) {\n> +\t\tconfig_.reset();\n> +\t\tFAIL() << \"Configuration not valid\";\n> +\t}\n> +\n> +\tif (camera_->configure(config_.get())) {\n> +\t\tconfig_.reset();\n> +\t\tFAIL() << \"Failed to configure camera\";\n> +\t}\n> +}\n> +\n> +void MultiCaptureBase::start()\n> +{\n> +\tunsigned int i = 0;\n> +\n> +\tfor (auto const &config : *config_) {\n> +\t\tStream *stream = config.stream();\n> +\t\tint count = allocator_->allocate(stream);\n> +\n> +\t\tASSERT_GE(count, 0) << \"Failed to allocate buffers for stream \"\n> +\t\t\t\t << i;\n> +\t\tEXPECT_EQ(count, config.bufferCount)\n> +\t\t\t<< \"Alocated less buffers than expected for stream \"\n> +\t\t\t<< i;\n> +\t\ti++;\n> +\t}\n> +\n> +\tcamera_->requestCompleted.connect(this, &MultiCaptureBase::requestComplete);\n> +\n> +\tASSERT_EQ(camera_->start(), 0) << \"Failed to start camera\";\n> +}\n> +\n> +void MultiCaptureBase::stop()\n> +{\n> +\tif (!config_ || !allocator_->allocated())\n> +\t\treturn;\n> +\n> +\tcamera_->stop();\n> +\n> +\tcamera_->requestCompleted.disconnect(this);\n> +\n> +\trequests_.clear();\n> +\n> +\tfor (auto const &config : *config_) {\n> +\t\tStream *stream = config.stream();\n> +\t\tallocator_->free(stream);\n> +\t}\n> +}\n\nWhile I understand the need to differentiate the capture operations,\nI wonder if you have considered augmenting the SimpleCapture to\noperate on a vector of roles instead than on a single one. The\nimplementation of these base methods seems very similar\n\n> +\n> +std::vector\n> +MultiCaptureBase::prepareBuffers(unsigned int numRequests, unsigned int numStreams)\n> +{\n> +\tstd::vector buffers;\n> +\n> +\tfor (unsigned int i = 0; i < numStreams; i++) {\n> +\t\tStream *stream = config_->at(i).stream();\n> +\n> +\t\tbuffers.emplace_back(&allocator_->buffers(stream));\n> +\n> +\t\t/* No point in testing less requests then the camera depth. */\n> +\t\tif (buffers.back()->size() > numRequests) {\n> +\t\t\tstd::cout << \"Stream \" << i\n> +\t\t\t\t << \" needs \" << std::to_string(buffers.back()->size())\n> +\t\t\t\t << \" requests, can't test only \"\n> +\t\t\t\t << std::to_string(numRequests) << std::endl;\n> +\t\t\treturn {};\n> +\t\t}\n> +\t}\n> +\n> +\treturn buffers;\n> +}\n> +\n> +/* MultiCapture */\n> +\n> +MultiCapture::MultiCapture(std::shared_ptr camera)\n> +\t: MultiCaptureBase(camera)\n> +{\n> +}\n> +\n> +void MultiCapture::capture(unsigned int numRequests, unsigned int numStreams)\n> +{\n> +\tstart();\n> +\n> +\tqueueCount_ = 0;\n> +\tcaptureCount_ = 0;\n> +\tcaptureLimit_ = numRequests;\n> +\n> +\tstd::vector\n> +\t\tbuffers = prepareBuffers(numRequests, numStreams);\n> +\n> +\tif (!buffers.size())\n> +\t\tGTEST_SKIP();\n> +\n> +\t/* Queue the recommended number of requests. */\n> +\tconst unsigned int inFlightRequests = config_->at(0).bufferCount;\n> +\tfor (unsigned int i = 0; i < inFlightRequests; i++) {\n> +\t\tstd::unique_ptr request = camera_->createRequest();\n> +\t\tASSERT_TRUE(request) << \"Can't create request\";\n> +\n> +\t\tfor (unsigned int j = 0; j < numStreams; j++) {\n> +\t\t\tconst FrameBufferList *bufferList = buffers[j];\n> +\t\t\tStream *stream = config_->at(j).stream();\n> +\n> +\t\t\tASSERT_EQ(request->addBuffer(stream, (*bufferList)[i].get()), 0)\n> +\t\t\t\t<< \"Can't set buffer for request\";\n> +\t\t}\n> +\n> +\t\tASSERT_EQ(queueRequest(request.get()), 0)\n> +\t\t\t<< \"Failed to queue request\";\n> +\t\trequests_.push_back(std::move(request));\n> +\t}\n> +\n> +\t/* Run capture session. */\n> +\tloop_ = new EventLoop();\n> +\tloop_->exec();\n> +\tstop();\n> +\tdelete loop_;\n> +\n> +\tASSERT_EQ(captureCount_, captureLimit_);\n> +}\n> +\n> +int MultiCapture::queueRequest(Request *request)\n> +{\n> +\tqueueCount_++;\n> +\tif (queueCount_ > captureLimit_)\n> +\t\treturn 0;\n> +\n> +\treturn camera_->queueRequest(request);\n> +}\n> +\n> +void MultiCapture::requestComplete(Request *request)\n> +{\n> +\tcaptureCount_++;\n> +\tif (captureCount_ >= captureLimit_) {\n> +\t\tloop_->exit(0);\n> +\t\treturn;\n> +\t}\n> +\n> +\trequest->reuse(Request::ReuseBuffers);\n> +\tif (queueRequest(request))\n> +\t\tloop_->exit(-EINVAL);\n> +}\n> diff --git a/src/apps/lc-compliance/multi_capture.h b/src/apps/lc-compliance/multi_capture.h\n> new file mode 100644\n> index 000000000000..9037099988e5\n> --- /dev/null\n> +++ b/src/apps/lc-compliance/multi_capture.h\n> @@ -0,0 +1,61 @@\n> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> +/*\n> + * Copyright (C) 2023, Raspberry Pi Ltd\n> + *\n> + * multi_capture.h - Multi-stream capture helper\n> + */\n> +\n> +#pragma once\n> +\n> +#include \n> +#include \n> +\n> +#include \n> +\n> +#include \"../common/event_loop.h\"\n> +\n> +using FrameBufferList = std::vector>;\n> +\n> +class MultiCaptureBase\n> +{\n> +public:\n> +\tvoid configure(const libcamera::StreamRoles &roles);\n> +\n> +protected:\n> +\tMultiCaptureBase(std::shared_ptr camera);\n> +\tvirtual ~MultiCaptureBase();\n> +\n> +\tvoid start();\n> +\tvoid stop();\n> +\n> +\tstd::vector\n> +\tprepareBuffers(unsigned int numRequests, unsigned int numStreams);\n> +\n> +\tvirtual void requestComplete(libcamera::Request *request) = 0;\n> +\tvirtual void updateConfig() = 0;\n> +\n> +\tEventLoop *loop_;\n> +\n> +\tstd::shared_ptr camera_;\n> +\tstd::unique_ptr allocator_;\n> +\tstd::unique_ptr config_;\n> +\tstd::vector> requests_;\n> +};\n> +\n> +class MultiCapture : public MultiCaptureBase\n> +{\n> +public:\n> +\tMultiCapture(std::shared_ptr camera);\n> +\n> +\tvoid capture(unsigned int numRequests, unsigned int numStreams);\n> +\n> +protected:\n> +\tint queueRequest(libcamera::Request *request);\n> +\tvoid requestComplete(libcamera::Request *request) override;\n> +\n> +\tvoid updateConfig() override {}\n\nI wonder if it wouldn't be best to implement this inline in MultiCaptureHints\ninstead of making it a part of the class hierarcy\n\n> +\n> +\tunsigned int queueCount_;\n> +\tunsigned int captureCount_;\n> +\tunsigned int captureLimit_;\n> +};\n> --\n> 2.25.1\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 203E5BE080\n\tfor ;\n\tThu, 2 Mar 2023 10:31:59 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id EE9C2626C6;\n\tThu, 2 Mar 2023 11:31:57 +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 594A762668\n\tfor ;\n\tThu, 2 Mar 2023 11:31:57 +0100 (CET)","from ideasonboard.com (host-87-18-61-24.retail.telecomitalia.it\n\t[87.18.61.24])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 6744B56A;\n\tThu, 2 Mar 2023 11:31:56 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1677753118;\n\tbh=X3IdexUZFe8lAA9qW7w76+Y5VAt+GItFnC+78BI3Oho=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=e4rPvt27w4FGrwfBaIgDg05uanbmZcTt3ac+taOoKAg5O7+rfNVq3rMMItCx0Ozw7\n\tE8Oncj5WdxMvVZKvNJb0wQIbgzVmaxqM/OvBCOA5+OMIBkXAjmElQVqC5EZqUQ+QFh\n\t0nwB/Kb/oRQOk1wNXBFz+Kvl0kpMwQ7iALP8r8zvNPfg+LfsJB4YjgVHSmre2myz1l\n\t5MyvA3DiZZq4nVgeAZA8wMQv4MwcF+lCJaN+w0eWwkVT4GdMwgyCAzOwCUeGah4TC4\n\tJ+0SRDKnNkrRY0DoqjH/mwZvslsdPsLCM9RQw1txeG2I0BY6zaLcCEHUkxmsBOt8E3\n\tnhiyFhnZ7gkgA==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1677753117;\n\tbh=X3IdexUZFe8lAA9qW7w76+Y5VAt+GItFnC+78BI3Oho=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=BMb73WM0LPkORTRkfXpsU/UxXdyXntytxfdR2lRReU1meF80sD+1htvf8oUfld3wM\n\tLQQPU1+PUr6LfaQABAMRqsOH8g+t0wX4xbQqO2qIMWH+VX/gSlUlV9s4fzHh04a7gC\n\tbKHWq3Y1hSjNmS+Vh9/12EXV0ADP8OEP+xRgh2TM="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"BMb73WM0\"; dkim-atps=neutral","Date":"Thu, 2 Mar 2023 11:31:52 +0100","To":"Naushir Patuck ","Message-ID":"<20230302103152.nfchw5khjg34lblw@uno.localdomain>","References":"<20230203094424.25243-1-naush@raspberrypi.com>\n\t<20230203094424.25243-7-naush@raspberrypi.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20230203094424.25243-7-naush@raspberrypi.com>","Subject":"Re: [libcamera-devel] [PATCH v1 6/8] libcamera: apps: lcc: Add\n\tmulti-stream capture test framework","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","From":"Jacopo Mondi via libcamera-devel ","Reply-To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]