[{"id":20038,"web_url":"https://patchwork.libcamera.org/comment/20038/","msgid":"","date":"2021-10-04T23:22:23","subject":"Re: [libcamera-devel] [RFC PATCH] lc-compliance: Add test to set\n\ttest pattern mode","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Hiro,\n\nThank you for the patch.\n\nOn Mon, Oct 04, 2021 at 07:23:08PM +0900, Hirokazu Honda wrote:\n> This adds the test to verify the requested test pattern mode is\n> correctly applied in request metadata.\n> \n> Signed-off-by: Hirokazu Honda \n> ---\n> src/lc-compliance/capture_test.cpp | 15 ++++\n> src/lc-compliance/simple_capture.cpp | 101 +++++++++++++++++++++++++++\n> src/lc-compliance/simple_capture.h | 16 +++++\n> 3 files changed, 132 insertions(+)\n> \n> diff --git a/src/lc-compliance/capture_test.cpp b/src/lc-compliance/capture_test.cpp\n> index 52578207..fc0b5322 100644\n> --- a/src/lc-compliance/capture_test.cpp\n> +++ b/src/lc-compliance/capture_test.cpp\n> @@ -121,6 +121,21 @@ TEST_P(SingleStream, UnbalancedStop)\n> \tcapture.capture(numRequests);\n> }\n> \n> +TEST_P(SingleStream, TestPatternMode)\n> +{\n> +\tauto [role, numRequests] = GetParam();\n\nI don't think this test should be parametrized by role and number of\nrequests, those shouldn't matter for the feature being tested. You can\nuse a fixed role and a fixed number of requests instead. You can't use\nSingleStream, as the test would still be parametrized even if you ignore\nthe value of the parameters. I'd thus create a new test suite related to\ncontrols in a separate file, to host the TestPatternMode test.\n\n> +\n> +\tSimpleCaptureTestPatternMode capture(camera_);\n> +\tcapture.configure(role);\n> +\n> +\tauto testPatternModes = capture.availableTestPatternModes();\n> +\tif (testPatternModes.empty())\n> +\t\treturn;\n\nThe test should be reported as skipped when this happens, will it be the\ncase ?\n\n> +\n> +\tfor (const int32_t testPatternMode : testPatternModes)\n> +\t\tcapture.capture(testPatternMode, numRequests);\n> +}\n> +\n> INSTANTIATE_TEST_SUITE_P(CaptureTests,\n> \t\t\t SingleStream,\n> \t\t\t testing::Combine(testing::ValuesIn(ROLES),\n> diff --git a/src/lc-compliance/simple_capture.cpp b/src/lc-compliance/simple_capture.cpp\n> index ab5cb35c..f279b6a7 100644\n> --- a/src/lc-compliance/simple_capture.cpp\n> +++ b/src/lc-compliance/simple_capture.cpp\n> @@ -189,3 +189,104 @@ void SimpleCaptureUnbalanced::requestComplete(Request *request)\n> \tif (camera_->queueRequest(request))\n> \t\tloop_->exit(-EINVAL);\n> }\n> +\n> +/* SimpleCaptureTestPatternMode */\n\nI would also move this to the new source file, it's specific to the test\npattern test, not a generic helper.\n\nCould you expand this command a little bit to explain what the test does\n? A \\todo to indicate that the contents of the buffer should be checked\nwould also be useful.\n\n> +\n> +SimpleCaptureTestPatternMode::SimpleCaptureTestPatternMode(std::shared_ptr camera)\n> +\t: SimpleCapture(camera)\n> +{\n> +}\n> +\n> +std::vector SimpleCaptureTestPatternMode::availableTestPatternModes() const\n> +{\n> +\tconst auto &controls = camera_->controls();\n> +\tconst auto it = controls.find(&controls::draft::TestPatternMode);\n> +\tif (it == controls.end())\n> +\t\treturn {};\n> +\n> +\tstd::vector testPatternModes;\n> +\tconst std::vector &values = it->second.values();\n> +\tfor (const auto &value : values)\n> +\t\ttestPatternModes.push_back(value.get());\n> +\n> +\treturn testPatternModes;\n> +}\n> +\n> +void SimpleCaptureTestPatternMode::capture(int32_t testPatternMode,\n> +\t\t\t\t\t unsigned int numRequests)\n> +{\n> +\tstart();\n> +\n> +\tStream *stream = config_->at(0).stream();\n> +\tconst std::vector> &buffers = allocator_->buffers(stream);\n> +\t/* No point in testing less requests then the camera depth. */\n> +\tif (buffers.size() > numRequests) {\n> +\t\tstd::cout << \"Camera needs \" << buffers.size()\n> +\t\t\t << \" requests, can't test only \"\n> +\t\t\t << numRequests << std::endl;\n> +\t\tGTEST_SKIP();\n> +\t}\n> +\n> +\tcaptureCount_ = 0;\n> +\tcaptureLimit_ = numRequests;\n> +\ttestPatternMode_ = testPatternMode;\n> +\n> +\t/* Queue the recommended number of reqeuests. */\n> +\tstd::vector> requests;\n> +\tfor (const std::unique_ptr &buffer : buffers) {\n> +\t\tstd::unique_ptr request = camera_->createRequest();\n> +\t\tASSERT_TRUE(request) << \"Can't create request\";\n> +\n> +\t\trequest->controls().set(controls::draft::TestPatternMode,\n> +\t\t\t\t\ttestPatternMode);\n> +\n> +\t\tASSERT_EQ(request->addBuffer(stream, buffer.get()), 0) << \"Can't set buffer for request\";\n> +\n> +\t\tASSERT_EQ(camera_->queueRequest(request.get()), 0) << \"Failed to queue request\";\n> +\n> +\t\trequests.push_back(std::move(request));\n> +\t}\n> +\n> +\t/* Run capture session. */\n> +\tloop_ = new EventLoop();\n> +\tint status = loop_->exec();\n> +\tstop();\n> +\tdelete loop_;\n> +\n> +\tASSERT_EQ(status, 0);\n> +}\n> +\n> +void SimpleCaptureTestPatternMode::requestComplete(Request *request)\n> +{\n> +\tcaptureCount_++;\n> +\tif (captureCount_ >= captureLimit_) {\n> +\t\tloop_->exit(0);\n> +\t\treturn;\n> +\t}\n> +\n> +\tif (!request->metadata().contains(controls::draft::TestPatternMode)) {\n> +\t\tif (testPatternMode_ != controls::draft::TestPatternModeOff) {\n> +\t\t\tstd::cerr << \"no test pattern mode mismatch: expected=\"\n> +\t\t\t\t << testPatternMode_ << std::endl;\n> +\t\t\tloop_->exit(-EINVAL);\n> +\t\t\treturn;\n> +\t\t}\n> +\t} else {\n> +\t\tconst int32_t testPatternMode =\n> +\t\t\trequest->metadata().get(controls::draft::TestPatternMode);\n> +\t\tif (testPatternMode_ != testPatternMode) {\n> +\t\t\tstd::cerr << \"test pattern mode mismatch: expected=\"\n> +\t\t\t\t << testPatternMode_ << \", actual=\"\n> +\t\t\t\t << testPatternMode << std::endl;\n> +\n> +\t\t\tloop_->exit(-EINVAL);\n> +\t\t\treturn;\n> +\t\t}\n> +\t}\n> +\n> +\trequest->reuse(Request::ReuseBuffers);\n> +\trequest->controls().set(controls::draft::TestPatternMode,\n> +\t\t\t\ttestPatternMode_);\n> +\tif (camera_->queueRequest(request))\n> +\t\tloop_->exit(-EINVAL);\n> +}\n> diff --git a/src/lc-compliance/simple_capture.h b/src/lc-compliance/simple_capture.h\n> index 100ffd66..7989a308 100644\n> --- a/src/lc-compliance/simple_capture.h\n> +++ b/src/lc-compliance/simple_capture.h\n> @@ -64,4 +64,20 @@ private:\n> \tunsigned int captureLimit_;\n> };\n> \n> +class SimpleCaptureTestPatternMode : public SimpleCapture\n> +{\n> +public:\n> +\tSimpleCaptureTestPatternMode(std::shared_ptr camera);\n> +\n> +\tstd::vector availableTestPatternModes() const;\n> +\tvoid capture(int32_t testPatternMode, unsigned int numRequests);\n> +\n> +private:\n> +\tvoid requestComplete(libcamera::Request *request) override;\n> +\n> +\tunsigned int captureCount_;\n> +\tunsigned int captureLimit_;\n> +\tint32_t testPatternMode_;\n> +};\n> +\n> #endif /* __LC_COMPLIANCE_SIMPLE_CAPTURE_H__ */","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 BCE9DBDC71\n\tfor ;\n\tMon, 4 Oct 2021 23:22:33 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 230DE691B6;\n\tTue, 5 Oct 2021 01:22:33 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 619A869189\n\tfor ;\n\tTue, 5 Oct 2021 01:22:30 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id D34AE25B;\n\tTue, 5 Oct 2021 01:22:29 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"b3wTNbP1\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1633389750;\n\tbh=RWbKYC+sO/0CkvbKRmQa2T3Idp3KY7IXiK/XlHi6MF4=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=b3wTNbP121Ejk4wPhTM0PL1XiW4imzt9DRCgr+x2M6xxzNMLZ6gNQ/e+mMBIe4GoE\n\tc0HZvZwI2yRS2szK8El0mEP8TmALJ4tQZroB+3lD7J6/b7by7Xz7b2U0XCdsCNHMV6\n\tipPTbBJQnm4UYU3GF/2efmVIu4+SiyCUenjzA5n0=","Date":"Tue, 5 Oct 2021 02:22:23 +0300","From":"Laurent Pinchart ","To":"Hirokazu Honda ","Message-ID":"","References":"<20211004102308.3726725-1-hiroh@chromium.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20211004102308.3726725-1-hiroh@chromium.org>","Subject":"Re: [libcamera-devel] [RFC PATCH] lc-compliance: Add test to set\n\ttest pattern mode","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\" "}}]