[{"id":16689,"web_url":"https://patchwork.libcamera.org/comment/16689/","msgid":"","date":"2021-04-29T11:48:33","subject":"Re: [libcamera-devel] [RFC PATCH 2/2] lc-compliance: Refactor using\n\tGoogletest","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hello Nícolas,\n\nThanks for your work.\n\nOn 2021-04-22 18:06:09 -0300, Nícolas F. R. A. Prado wrote:\n> Refactor lc-compliance using Googletest as the test framework.\n\nI really like the gtest rework from a first reading of this patch. \nUnfortunately it no longer applies to master so I have not been able to \ntake it for a test drive.\n\nI like that we get most if not all features we discussed such as skip \nand test suite/case selection form the CLI more or less out-for-the box \nfor free.\n\nSome random comments below.\n\n> \n> Signed-off-by: Nícolas F. R. A. Prado \n> ---\n> src/lc-compliance/main.cpp | 60 +++++------\n> src/lc-compliance/meson.build | 3 +\n> src/lc-compliance/simple_capture.cpp | 73 +++++---------\n> src/lc-compliance/simple_capture.h | 8 +-\n> src/lc-compliance/single_stream.cpp | 142 ++++++++++++++-------------\n> 5 files changed, 132 insertions(+), 154 deletions(-)\n> \n> diff --git a/src/lc-compliance/main.cpp b/src/lc-compliance/main.cpp\n> index 54cee54aa978..04cd0732ee3d 100644\n> --- a/src/lc-compliance/main.cpp\n> +++ b/src/lc-compliance/main.cpp\n> @@ -14,8 +14,12 @@\n> #include \"../cam/options.h\"\n> #include \"tests.h\"\n> \n> +#include \"gtest/gtest.h\"\n\nAs gtest is pulled in as an dependably and not added to the libcamera \ntee should this not be ?\n\n> +\n> using namespace libcamera;\n> \n> +std::shared_ptr cam;\n> +\n\nWould it be possible to factor the code so that the Camera instance is \nnot a global variable? I'm thinking that this will not scale when tests \nrequiring more then one camera are needed.\n\n> enum {\n> \tOptCamera = 'c',\n> \tOptHelp = 'h',\n> @@ -28,14 +32,13 @@ public:\n> \t~Harness();\n> \n> \tint exec();\n> +\tint init();\n> \n> private:\n> -\tint init();\n> \tvoid listCameras();\n> \n> \tOptionsParser::Options options_;\n> \tstd::unique_ptr cm_;\n> -\tstd::shared_ptr camera_;\n> };\n> \n> Harness::Harness(const OptionsParser::Options &options)\n> @@ -46,33 +49,14 @@ Harness::Harness(const OptionsParser::Options &options)\n> \n> Harness::~Harness()\n> {\n> -\tif (camera_) {\n> -\t\tcamera_->release();\n> -\t\tcamera_.reset();\n> +\tif (cam) {\n> +\t\tcam->release();\n> +\t\tcam.reset();\n> \t}\n> \n> \tcm_->stop();\n> }\n> \n> -int Harness::exec()\n> -{\n> -\tint ret = init();\n> -\tif (ret)\n> -\t\treturn ret;\n> -\n> -\tstd::vector results;\n> -\n> -\tresults.push_back(testSingleStream(camera_));\n> -\n> -\tfor (const Results &result : results) {\n> -\t\tret = result.summary();\n> -\t\tif (ret)\n> -\t\t\treturn ret;\n> -\t}\n> -\n> -\treturn 0;\n> -}\n> -\n> int Harness::init()\n> {\n> \tint ret = cm_->start();\n> @@ -89,14 +73,14 @@ int Harness::init()\n> \t}\n> \n> \tconst std::string &cameraId = options_[OptCamera];\n> -\tcamera_ = cm_->get(cameraId);\n> -\tif (!camera_) {\n> +\tcam = cm_->get(cameraId);\n> +\tif (!cam) {\n> \t\tstd::cout << \"Camera \" << cameraId << \" not found, available cameras:\" << std::endl;\n> \t\tlistCameras();\n> \t\treturn -ENODEV;\n> \t}\n> \n> -\tif (camera_->acquire()) {\n> +\tif (cam->acquire()) {\n> \t\tstd::cout << \"Failed to acquire camera\" << std::endl;\n> \t\treturn -EINVAL;\n> \t}\n> @@ -108,8 +92,8 @@ int Harness::init()\n> \n> void Harness::listCameras()\n> {\n> -\tfor (const std::shared_ptr &cam : cm_->cameras())\n> -\t\tstd::cout << \"- \" << cam.get()->id() << std::endl;\n> +\tfor (const std::shared_ptr &c : cm_->cameras())\n> +\t\tstd::cout << \"- \" << c.get()->id() << std::endl;\n> }\n> \n> static int parseOptions(int argc, char **argv, OptionsParser::Options *options)\n> @@ -133,6 +117,17 @@ static int parseOptions(int argc, char **argv, OptionsParser::Options *options)\n> \treturn 0;\n> }\n> \n> +/* Make asserts act like exceptions, otherwise they only fail (or skip) the\n> + * current function */\n\nnit:\n\n /*\n * Multieline comments start\n * with a blank line.\n */\n\n> +class ThrowListener : public testing::EmptyTestEventListener {\n> + void OnTestPartResult(const testing::TestPartResult& result) override {\n> + if (result.type() == testing::TestPartResult::kFatalFailure\n> +\t\t || result.type() == testing::TestPartResult::kSkip) {\n> + throw testing::AssertionException(result);\n> + }\n> + }\n> +};\n\nIndentation looks odd here.\n\n> +\n> int main(int argc, char **argv)\n> {\n> \tOptionsParser::Options options;\n> @@ -143,6 +138,11 @@ int main(int argc, char **argv)\n> \t\treturn EXIT_FAILURE;\n> \n> \tHarness harness(options);\n> +\tret = harness.init();\n> +\tif (ret)\n> +\t\treturn ret;\n> \n> -\treturn harness.exec() ? EXIT_FAILURE : EXIT_SUCCESS;\n> +\t::testing::InitGoogleTest(&argc, argv);\n> +\ttesting::UnitTest::GetInstance()->listeners().Append(new ThrowListener);\n> +\treturn RUN_ALL_TESTS();\n> }\n> diff --git a/src/lc-compliance/meson.build b/src/lc-compliance/meson.build\n> index a2bfcceb1259..4c41ce6da43a 100644\n> --- a/src/lc-compliance/meson.build\n> +++ b/src/lc-compliance/meson.build\n> @@ -18,10 +18,13 @@ lc_compliance_sources = files([\n> 'single_stream.cpp',\n> ])\n> \n> +gtest_dep = dependency('gtest', main: true)\n> +\n> lc_compliance = executable('lc-compliance', lc_compliance_sources,\n> dependencies : [\n> libatomic,\n> libcamera_dep,\n> libevent,\n> + gtest_dep\n\nAppaned a trailing ','. This is done to prepare for the next time this \nfile needs to be edited as the diff will then be easier to read as it \nwill only touch new lines.\n\n> ],\n> install : true)\n> diff --git a/src/lc-compliance/simple_capture.cpp b/src/lc-compliance/simple_capture.cpp\n> index e6715ac07f12..24abbff138a3 100644\n> --- a/src/lc-compliance/simple_capture.cpp\n> +++ b/src/lc-compliance/simple_capture.cpp\n> @@ -6,6 +6,7 @@\n> */\n> \n> #include \"simple_capture.h\"\n> +#include \"gtest/gtest.h\"\n> \n> using namespace libcamera;\n> \n> @@ -20,35 +21,29 @@ SimpleCapture::~SimpleCapture()\n> \tstop();\n> }\n> \n> -Results::Result SimpleCapture::configure(StreamRole role)\n> +void SimpleCapture::configure(StreamRole role)\n> {\n> \tconfig_ = camera_->generateConfiguration({ role });\n> \n> \tif (config_->validate() != CameraConfiguration::Valid) {\n> \t\tconfig_.reset();\n> -\t\treturn { Results::Fail, \"Configuration not valid\" };\n> +\t\tFAIL() << \"Configuration not valid\";\n> \t}\n> \n> \tif (camera_->configure(config_.get())) {\n> \t\tconfig_.reset();\n> -\t\treturn { Results::Fail, \"Failed to configure camera\" };\n> +\t\tFAIL() << \"Failed to configure camera\";\n> \t}\n> -\n> -\treturn { Results::Pass, \"Configure camera\" };\n> }\n> \n> -Results::Result SimpleCapture::start()\n> +void SimpleCapture::start()\n> {\n> \tStream *stream = config_->at(0).stream();\n> -\tif (allocator_->allocate(stream) < 0)\n> -\t\treturn { Results::Fail, \"Failed to allocate buffers\" };\n> +\tASSERT_GE(allocator_->allocate(stream), 0) << \"Failed to allocate buffers\";\n\nI like these asserts instead of the return type I used in the initial \nversion ;-)\n\n> \n> -\tif (camera_->start())\n> -\t\treturn { Results::Fail, \"Failed to start camera\" };\n> +\tASSERT_TRUE(!camera_->start()) << \"Failed to start camera\";\n> \n> \tcamera_->requestCompleted.connect(this, &SimpleCapture::requestComplete);\n> -\n> -\treturn { Results::Pass, \"Started camera\" };\n> }\n> \n> void SimpleCapture::stop()\n> @@ -73,22 +68,19 @@ SimpleCaptureBalanced::SimpleCaptureBalanced(std::shared_ptr camera)\n> {\n> }\n> \n> -Results::Result SimpleCaptureBalanced::capture(unsigned int numRequests)\n> +void SimpleCaptureBalanced::capture(unsigned int numRequests)\n> {\n> -\tResults::Result ret = start();\n> -\tif (ret.first != Results::Pass)\n> -\t\treturn ret;\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\t/* Cache buffers.size() before we destroy it in stop() */\n> -\t\tint buffers_size = buffers.size();\n> -\n> -\t\treturn { Results::Skip, \"Camera needs \" + std::to_string(buffers_size)\n> -\t\t\t+ \" requests, can't test only \" + std::to_string(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> +\t\tGTEST_SKIP();\n> \t}\n> \n> \tqueueCount_ = 0;\n> @@ -99,17 +91,11 @@ Results::Result SimpleCaptureBalanced::capture(unsigned int numRequests)\n> \tstd::vector> requests;\n> \tfor (const std::unique_ptr &buffer : buffers) {\n> \t\tstd::unique_ptr request = camera_->createRequest();\n> -\t\tif (!request) {\n> -\t\t\treturn { Results::Fail, \"Can't create request\" };\n> -\t\t}\n> +\t\tASSERT_TRUE(request) << \"Can't create request\";\n> \n> -\t\tif (request->addBuffer(stream, buffer.get())) {\n> -\t\t\treturn { Results::Fail, \"Can't set buffer for request\" };\n> -\t\t}\n> +\t\tASSERT_FALSE(request->addBuffer(stream, buffer.get())) << \"Can't set buffer for request\";\n> \n> -\t\tif (queueRequest(request.get()) < 0) {\n> -\t\t\treturn { Results::Fail, \"Failed to queue request\" };\n> -\t\t}\n> +\t\tASSERT_GE(queueRequest(request.get()), 0) << \"Failed to queue request\";\n> \n> \t\trequests.push_back(std::move(request));\n> \t}\n> @@ -120,12 +106,7 @@ Results::Result SimpleCaptureBalanced::capture(unsigned int numRequests)\n> \tstop();\n> \tdelete loop_;\n> \n> -\tif (captureCount_ != captureLimit_)\n> -\t\treturn { Results::Fail, \"Got \" + std::to_string(captureCount_) +\n> -\t\t\t\" request, wanted \" + std::to_string(captureLimit_) };\n> -\n> -\treturn { Results::Pass, \"Balanced capture of \" +\n> -\t\tstd::to_string(numRequests) + \" requests\" };\n> +\tASSERT_EQ(captureCount_, captureLimit_);\n> }\n> \n> int SimpleCaptureBalanced::queueRequest(Request *request)\n> @@ -157,11 +138,9 @@ SimpleCaptureUnbalanced::SimpleCaptureUnbalanced(std::shared_ptr camera)\n> {\n> }\n> \n> -Results::Result SimpleCaptureUnbalanced::capture(unsigned int numRequests)\n> +void SimpleCaptureUnbalanced::capture(unsigned int numRequests)\n> {\n> -\tResults::Result ret = start();\n> -\tif (ret.first != Results::Pass)\n> -\t\treturn ret;\n> +\tstart();\n> \n> \tStream *stream = config_->at(0).stream();\n> \tconst std::vector> &buffers = allocator_->buffers(stream);\n> @@ -173,17 +152,11 @@ Results::Result SimpleCaptureUnbalanced::capture(unsigned int numRequests)\n> \tstd::vector> requests;\n> \tfor (const std::unique_ptr &buffer : buffers) {\n> \t\tstd::unique_ptr request = camera_->createRequest();\n> -\t\tif (!request) {\n> -\t\t\treturn { Results::Fail, \"Can't create request\" };\n> -\t\t}\n> +\t\tASSERT_TRUE(request) << \"Can't create request\";\n> \n> -\t\tif (request->addBuffer(stream, buffer.get())) {\n> -\t\t\treturn { Results::Fail, \"Can't set buffer for request\" };\n> -\t\t}\n> +\t\tASSERT_FALSE(request->addBuffer(stream, buffer.get())) << \"Can't set buffer for request\";\n> \n> -\t\tif (camera_->queueRequest(request.get()) < 0) {\n> -\t\t\treturn { Results::Fail, \"Failed to queue request\" };\n> -\t\t}\n> +\t\tASSERT_GE(camera_->queueRequest(request.get()), 0) << \"Failed to queue request\";\n> \n> \t\trequests.push_back(std::move(request));\n> \t}\n> @@ -194,7 +167,7 @@ Results::Result SimpleCaptureUnbalanced::capture(unsigned int numRequests)\n> \tstop();\n> \tdelete loop_;\n> \n> -\treturn { status ? Results::Fail : Results::Pass, \"Unbalanced capture of \" + std::to_string(numRequests) + \" requests\" };\n> +\tASSERT_FALSE(status);\n> }\n> \n> void SimpleCaptureUnbalanced::requestComplete(Request *request)\n> diff --git a/src/lc-compliance/simple_capture.h b/src/lc-compliance/simple_capture.h\n> index d9de53fb63a3..0f8465083456 100644\n> --- a/src/lc-compliance/simple_capture.h\n> +++ b/src/lc-compliance/simple_capture.h\n> @@ -17,13 +17,13 @@\n> class SimpleCapture\n> {\n> public:\n> -\tResults::Result configure(libcamera::StreamRole role);\n> +\tvoid configure(libcamera::StreamRole role);\n> \n> protected:\n> \tSimpleCapture(std::shared_ptr camera);\n> \tvirtual ~SimpleCapture();\n> \n> -\tResults::Result start();\n> +\tvoid start();\n> \tvoid stop();\n> \n> \tvirtual void requestComplete(libcamera::Request *request) = 0;\n> @@ -40,7 +40,7 @@ class SimpleCaptureBalanced : public SimpleCapture\n> public:\n> \tSimpleCaptureBalanced(std::shared_ptr camera);\n> \n> -\tResults::Result capture(unsigned int numRequests);\n> +\tvoid capture(unsigned int numRequests);\n> \n> private:\n> \tint queueRequest(libcamera::Request *request);\n> @@ -56,7 +56,7 @@ class SimpleCaptureUnbalanced : public SimpleCapture\n> public:\n> \tSimpleCaptureUnbalanced(std::shared_ptr camera);\n> \n> -\tResults::Result capture(unsigned int numRequests);\n> +\tvoid capture(unsigned int numRequests);\n> \n> private:\n> \tvoid requestComplete(libcamera::Request *request) override;\n> diff --git a/src/lc-compliance/single_stream.cpp b/src/lc-compliance/single_stream.cpp\n> index 8318b42f42d6..18830cd9bce5 100644\n> --- a/src/lc-compliance/single_stream.cpp\n> +++ b/src/lc-compliance/single_stream.cpp\n> @@ -6,92 +6,94 @@\n> */\n> \n> #include \n> +#include \n> \n> #include \"simple_capture.h\"\n> #include \"tests.h\"\n> +#include \"gtest/gtest.h\"\n> \n> using namespace libcamera;\n> \n> -Results::Result testRequestBalance(std::shared_ptr camera,\n> -\t\t\t\t StreamRole role, unsigned int startCycles,\n> -\t\t\t\t unsigned int numRequests)\n> +extern std::shared_ptr cam;\n> +\n> +std::vector NUMREQUESTS = {1, 2, 3, 5, 8, 13, 21, 34, 55, 89};\n> +\n> +class FixedRequestsTest :\n> + public testing::TestWithParam> {\n> +};\n> +\n> +/*\n> + * Test single capture cycles\n> + *\n> + * Makes sure the camera completes the exact number of requests queued. Example\n> + * failure is a camera that needs N+M requests queued to complete N requests to\n> + * the application.\n> + */\n> +TEST_P(FixedRequestsTest, BalancedSingleCycle)\n> {\n> -\tSimpleCaptureBalanced capture(camera);\n> +\tauto [role, numRequests] = GetParam();\n> \n> -\tResults::Result ret = capture.configure(role);\n> -\tif (ret.first != Results::Pass)\n> -\t\treturn ret;\n> +\tSimpleCaptureBalanced capture(cam);\n> \n> -\tfor (unsigned int starts = 0; starts < startCycles; starts++) {\n> -\t\tret = capture.capture(numRequests);\n> -\t\tif (ret.first != Results::Pass)\n> -\t\t\treturn ret;\n> -\t}\n> +\tcapture.configure(role);\n> \n> -\treturn { Results::Pass, \"Balanced capture of \" +\n> -\t\tstd::to_string(numRequests) + \" requests with \" +\n> -\t\tstd::to_string(startCycles) + \" start cycles\" };\n> +\tcapture.capture(numRequests);\n> }\n> \n> -Results::Result testRequestUnbalance(std::shared_ptr camera,\n> -\t\t\t\t StreamRole role, unsigned int numRequests)\n> +/*\n> + * Test multiple start/stop cycles\n> + *\n> + * Makes sure the camera supports multiple start/stop cycles. Example failure is\n> + * a camera that does not clean up correctly in its error path but is only\n> + * tested by single-capture applications.\n> + */\n> +TEST_P(FixedRequestsTest, BalancedMultiCycle)\n> {\n> -\tSimpleCaptureUnbalanced capture(camera);\n> +\tauto [role, numRequests] = GetParam();\n> +\tunsigned int numRepeats = 3;\n> \n> -\tResults::Result ret = capture.configure(role);\n> -\tif (ret.first != Results::Pass)\n> -\t\treturn ret;\n> +\tSimpleCaptureBalanced capture(cam);\n> \n> -\treturn capture.capture(numRequests);\n> +\tcapture.configure(role);\n> +\n> +\tfor (unsigned int starts = 0; starts < numRepeats; starts++)\n> +\t\tcapture.capture(numRequests);\n> }\n> \n> -Results testSingleStream(std::shared_ptr camera)\n> +/*\n> + * Test unbalanced stop\n> + *\n> + * Makes sure the camera supports a stop with requests queued. Example failure\n> + * is a camera that does not handle cancelation of buffers coming back from the\n> + * video device while stopping.\n> + */\n> +TEST_P(FixedRequestsTest, Unbalanced)\n> {\n> -\tstatic const std::vector> roles = {\n> -\t\t{ \"raw\", Raw },\n> -\t\t{ \"still\", StillCapture },\n> -\t\t{ \"video\", VideoRecording },\n> -\t\t{ \"viewfinder\", Viewfinder },\n> -\t};\n> -\tstatic const std::vector numRequests = { 1, 2, 3, 5, 8, 13, 21, 34, 55, 89 };\n> -\n> -\tResults results(numRequests.size() * roles.size() * 3);\n> -\n> -\tfor (const auto &role : roles) {\n> -\t\tstd::cout << \"= Test role \" << role.first << std::endl;\n> -\t\t/*\n> -\t\t * Test single capture cycles\n> -\t\t *\n> -\t\t * Makes sure the camera completes the exact number of requests queued.\n> -\t\t * Example failure is a camera that needs N+M requests queued to\n> -\t\t * complete N requests to the application.\n> -\t\t */\n> -\t\tstd::cout << \"* Test single capture cycles\" << std::endl;\n> -\t\tfor (unsigned int num : numRequests)\n> -\t\t\tresults.add(testRequestBalance(camera, role.second, 1, num));\n> -\n> -\t\t/*\n> -\t\t * Test multiple start/stop cycles\n> -\t\t *\n> -\t\t * Makes sure the camera supports multiple start/stop cycles.\n> -\t\t * Example failure is a camera that does not clean up correctly in its\n> -\t\t * error path but is only tested by single-capture applications.\n> -\t\t */\n> -\t\tstd::cout << \"* Test multiple start/stop cycles\" << std::endl;\n> -\t\tfor (unsigned int num : numRequests)\n> -\t\t\tresults.add(testRequestBalance(camera, role.second, 3, num));\n> -\n> -\t\t/*\n> -\t\t * Test unbalanced stop\n> -\t\t *\n> -\t\t * Makes sure the camera supports a stop with requests queued.\n> -\t\t * Example failure is a camera that does not handle cancelation\n> -\t\t * of buffers coming back from the video device while stopping.\n> -\t\t */\n> -\t\tstd::cout << \"* Test unbalanced stop\" << std::endl;\n> -\t\tfor (unsigned int num : numRequests)\n> -\t\t\tresults.add(testRequestUnbalance(camera, role.second, num));\n> -\t}\n> -\n> -\treturn results;\n> +\tauto [role, numRequests] = GetParam();\n> +\n> +\tSimpleCaptureUnbalanced capture(cam);\n> +\n> +\tcapture.configure(role);\n> +\n> +\tcapture.capture(numRequests);\n> }\n> +\n> +INSTANTIATE_TEST_SUITE_P(Raw,\n> + FixedRequestsTest,\n> + testing::Combine(testing::Values(Raw),\n> +\t\t\t\t\ttesting::ValuesIn(NUMREQUESTS)));\n> +\n> +INSTANTIATE_TEST_SUITE_P(StillCapture,\n> + FixedRequestsTest,\n> + testing::Combine(testing::Values(StillCapture),\n> +\t\t\t\t\ttesting::ValuesIn(NUMREQUESTS)));\n> +\n> +INSTANTIATE_TEST_SUITE_P(VideoRecording,\n> + FixedRequestsTest,\n> + testing::Combine(testing::Values(VideoRecording),\n> +\t\t\t\t\ttesting::ValuesIn(NUMREQUESTS)));\n> +\n> +INSTANTIATE_TEST_SUITE_P(Viewfinder,\n> + FixedRequestsTest,\n> + testing::Combine(testing::Values(Viewfinder),\n> +\t\t\t\t\ttesting::ValuesIn(NUMREQUESTS)));\n> -- \n> 2.31.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 2B457BDE45\n\tfor ;\n\tThu, 29 Apr 2021 11:48:37 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6CB8D68909;\n\tThu, 29 Apr 2021 13:48:36 +0200 (CEST)","from mail-lf1-x135.google.com (mail-lf1-x135.google.com\n\t[IPv6:2a00:1450:4864:20::135])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 409C360511\n\tfor ;\n\tThu, 29 Apr 2021 13:48:35 +0200 (CEST)","by mail-lf1-x135.google.com with SMTP id 2so11602581lft.4\n\tfor ;\n\tThu, 29 Apr 2021 04:48:35 -0700 (PDT)","from localhost (h-209-203.A463.priv.bahnhof.se. [155.4.209.203])\n\tby smtp.gmail.com with ESMTPSA id\n\tu4sm507251lji.95.2021.04.29.04.48.33\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tThu, 29 Apr 2021 04:48:33 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=ragnatech-se.20150623.gappssmtp.com\n\theader.i=@ragnatech-se.20150623.gappssmtp.com\n\theader.b=\"yl25wiRo\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ragnatech-se.20150623.gappssmtp.com; s=20150623;\n\th=date:from:to:cc:subject:message-id:references:mime-version\n\t:content-disposition:content-transfer-encoding:in-reply-to;\n\tbh=xYqiU0DY9NrX6tX/Gs0E5iOnz0YRD4DY8kjcEf4Rdcg=;\n\tb=yl25wiRoAxNdITgvclDz3MdJpfQeVIuexSzM3hu2OBaoBphuleCB5hK6JkA7ClEzBH\n\tFeSyBbm1dXfww/l1EAyN2TCyszuge6rJS6+583jh3dgytXdRCpQJUDGsahbCT6De353G\n\t314xFnfLw5LqQcLnnTBQ5CJjrLQSMX05HUzN4In/qkQSRxIByj76FbiTGkC1Ez8DWUEm\n\tvsxS5VCBzq8HwntSGXBx8QzvGePiYqV+iyQg9W14aOMGAiMCfixoMPPqczNKP0RpH7qL\n\tUGm/QNIgeEIxiahMS+DBYLwM2DCJMM4KVCYTyVYVjwolj1rsZmp+ZlfbcXt9hcnIYun0\n\tDXQA==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:date:from:to:cc:subject:message-id:references\n\t:mime-version:content-disposition:content-transfer-encoding\n\t:in-reply-to;\n\tbh=xYqiU0DY9NrX6tX/Gs0E5iOnz0YRD4DY8kjcEf4Rdcg=;\n\tb=VTbLRiKeBJwa7hVeW59XZRWdLTP2mz3QW2aF5srhCN6Ic+TG+S6EXuLmZVhGXQVBX9\n\tnKj/waTSJwTDsz8UrlsWpmKXykA+qqxXWFO0Wf99Cc/IiaFTNIpY8lyRaCq3L2N7V5On\n\tfwE6eGHnr24V12WbB5cT3C+et1J/e5gpDHgWIVM89gTm9GSYbygkvdmenFiRBmmM4Qoc\n\tMq7FaN70uf0A9c/fI3hymzYP0pO5PKB3gJ1wfkrzBnot5oxt4UlGz6O3XPYSZvIK1YFj\n\tMNymDTYVOUkmYnhw1HxLoiMvmw1E2lFQx27WI8zJWBcvIAwoq8iulF0gBP99eMDGftFL\n\t9WGw==","X-Gm-Message-State":"AOAM5338w31KIPTKrxv0WWpCQPOV5TI0D58Z32XdHgZHponIkZrHQQ8+\n\tqfmmTIhqRW7QRLcF7w/gpSSdYw==","X-Google-Smtp-Source":"ABdhPJxRifsr8RyyrNHY4rnxsSBQ5H2ehlwPFRxSJaG9pySBOgramSv1JPCvT2tOR0JZdyLgSqKerg==","X-Received":"by 2002:ac2:5f19:: with SMTP id\n\t25mr25333078lfq.328.1619696914453; \n\tThu, 29 Apr 2021 04:48:34 -0700 (PDT)","Date":"Thu, 29 Apr 2021 13:48:33 +0200","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"=?iso-8859-1?q?N=EDcolas_F=2E_R=2E_A=2E?= Prado ","Message-ID":"","References":"<20210422210609.425987-1-nfraprado@collabora.com>\n\t<20210422210609.425987-3-nfraprado@collabora.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210422210609.425987-3-nfraprado@collabora.com>","Subject":"Re: [libcamera-devel] [RFC PATCH 2/2] lc-compliance: Refactor using\n\tGoogletest","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, kernel@collabora.com,\n\t=?iso-8859-1?q?Andr=E9?= Almeida ","Content-Type":"text/plain; charset=\"iso-8859-1\"","Content-Transfer-Encoding":"quoted-printable","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16702,"web_url":"https://patchwork.libcamera.org/comment/16702/","msgid":"","date":"2021-04-30T13:19:03","subject":"Re: [libcamera-devel] [RFC PATCH 2/2] lc-compliance: Refactor using\n\tGoogletest","submitter":{"id":84,"url":"https://patchwork.libcamera.org/api/people/84/","name":"Nícolas F. R. A. Prado","email":"nfraprado@collabora.com"},"content":"Hi Niklas,\n\nthanks for the feedback.\n\nEm 2021-04-29 08:48, Niklas Söderlund escreveu:\n\n> Hello Nícolas,\n>\n> Thanks for your work.\n>\n> On 2021-04-22 18:06:09 -0300, Nícolas F. R. A. Prado wrote:\n> > Refactor lc-compliance using Googletest as the test framework.\n>\n> I really like the gtest rework from a first reading of this patch.\n> Unfortunately it no longer applies to master so I have not been able to\n> take it for a test drive.\n>\n> I like that we get most if not all features we discussed such as skip\n> and test suite/case selection form the CLI more or less out-for-the box\n> for free.\n>\n> Some random comments below.\n>\n> > \n> > Signed-off-by: Nícolas F. R. A. Prado \n> > ---\n> > src/lc-compliance/main.cpp | 60 +++++------\n> > src/lc-compliance/meson.build | 3 +\n> > src/lc-compliance/simple_capture.cpp | 73 +++++---------\n> > src/lc-compliance/simple_capture.h | 8 +-\n> > src/lc-compliance/single_stream.cpp | 142 ++++++++++++++-------------\n> > 5 files changed, 132 insertions(+), 154 deletions(-)\n> > \n> > diff --git a/src/lc-compliance/main.cpp b/src/lc-compliance/main.cpp\n> > index 54cee54aa978..04cd0732ee3d 100644\n> > --- a/src/lc-compliance/main.cpp\n> > +++ b/src/lc-compliance/main.cpp\n> > @@ -14,8 +14,12 @@\n> > #include \"../cam/options.h\"\n> > #include \"tests.h\"\n> > \n> > +#include \"gtest/gtest.h\"\n>\n> As gtest is pulled in as an dependably and not added to the libcamera\n> tee should this not be ?\n\nIt should indeed. Fixed for v2.\n\n>\n> > +\n> > using namespace libcamera;\n> > \n> > +std::shared_ptr cam;\n> > +\n>\n> Would it be possible to factor the code so that the Camera instance is\n> not a global variable? I'm thinking that this will not scale when tests\n> requiring more then one camera are needed.\n\nSo... This is something that annoyed me as well. The short answer is I think it\ncould, but the test registration code wouldn't be as clean.\n\nThe not so short answer: The only way I think this could be done is by adding a\nparameter to every test case with the camera instance. But I'm currently\nregistering the test suites statically:\n\nINSTANTIATE_TEST_SUITE_P(Raw,\n FixedRequestsTest,\n testing::Combine(testing::Values(Raw),\n\t\t\t\t\ttesting::ValuesIn(NUMREQUESTS)));\n\nSo in order to pass the camera instance, they would need to be registered\ndynamically, as the example in [1] shows, after the camera to use is parsed from\nthe command line.\n\n[1] https://google.github.io/googletest/advanced.html#registering-tests-programmatically\n\nBut indeed, this seems to be needed to support working with multiple cameras.\nThe other solutions I was thinking about: making the global variable a vector of\ncameras and/or running gtest multiple times and each time with the camera\nvariable set differently, wouldn't give a single report at the end with the\nresults from all cameras which would make it harder to parse the results.\n\nI'm not sure which tests would need multiple cameras (if you think of examples\nlet me know), but at least being able to run each of the tests for each of the\ncameras on a system in a single lc-compliance run would be important I think.\n\nSo I'll take a stab at it and see how things go.\n\nRegarding the other issues, I'll have them fixed for v2.\n\nThanks,\nNícolas","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 0604DBDE49\n\tfor ;\n\tFri, 30 Apr 2021 13:20:25 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7F86F68901;\n\tFri, 30 Apr 2021 15:20:24 +0200 (CEST)","from bhuna.collabora.co.uk (bhuna.collabora.co.uk [46.235.227.227])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 160DB688A5\n\tfor ;\n\tFri, 30 Apr 2021 15:20:24 +0200 (CEST)","from localhost (unknown\n\t[IPv6:2804:14c:1a9:2978:15e8:6e30:d93a:4c57])\n\t(using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128\n\tbits))\n\t(No client certificate requested) (Authenticated sender: nfraprado)\n\tby bhuna.collabora.co.uk (Postfix) with ESMTPSA id A35541F43A1C;\n\tFri, 30 Apr 2021 14:20:21 +0100 (BST)"],"Mime-Version":"1.0","To":"=?utf-8?q?Niklas_S=C3=B6derlund?= ","From":"=?utf-8?b?TsOtY29sYXMgRi4gUi4gQS4gUHJhZG8=?= ","Date":"Fri, 30 Apr 2021 10:19:03 -0300","Message-Id":"","In-Reply-To":"","Subject":"Re: [libcamera-devel] [RFC PATCH 2/2] lc-compliance: Refactor using\n\tGoogletest","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, kernel@collabora.com, =?utf-8?q?A?=\n\t=?utf-8?q?ndr=C3=A9_Almeida?= ","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16716,"web_url":"https://patchwork.libcamera.org/comment/16716/","msgid":"","date":"2021-05-01T07:07:28","subject":"Re: [libcamera-devel] [RFC PATCH 2/2] lc-compliance: Refactor using\n\tGoogletest","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hello Nícolas,\n\nThanks for your efforts!\n\nOn 2021-04-30 10:19:03 -0300, Nícolas F. R. A. Prado wrote:\n> Hi Niklas,\n> \n> thanks for the feedback.\n> \n> Em 2021-04-29 08:48, Niklas Söderlund escreveu:\n> \n> > Hello Nícolas,\n> >\n> > Thanks for your work.\n> >\n> > On 2021-04-22 18:06:09 -0300, Nícolas F. R. A. Prado wrote:\n> > > Refactor lc-compliance using Googletest as the test framework.\n> >\n> > I really like the gtest rework from a first reading of this patch.\n> > Unfortunately it no longer applies to master so I have not been able to\n> > take it for a test drive.\n> >\n> > I like that we get most if not all features we discussed such as skip\n> > and test suite/case selection form the CLI more or less out-for-the box\n> > for free.\n> >\n> > Some random comments below.\n> >\n> > > \n> > > Signed-off-by: Nícolas F. R. A. Prado \n> > > ---\n> > > src/lc-compliance/main.cpp | 60 +++++------\n> > > src/lc-compliance/meson.build | 3 +\n> > > src/lc-compliance/simple_capture.cpp | 73 +++++---------\n> > > src/lc-compliance/simple_capture.h | 8 +-\n> > > src/lc-compliance/single_stream.cpp | 142 ++++++++++++++-------------\n> > > 5 files changed, 132 insertions(+), 154 deletions(-)\n> > > \n> > > diff --git a/src/lc-compliance/main.cpp b/src/lc-compliance/main.cpp\n> > > index 54cee54aa978..04cd0732ee3d 100644\n> > > --- a/src/lc-compliance/main.cpp\n> > > +++ b/src/lc-compliance/main.cpp\n> > > @@ -14,8 +14,12 @@\n> > > #include \"../cam/options.h\"\n> > > #include \"tests.h\"\n> > > \n> > > +#include \"gtest/gtest.h\"\n> >\n> > As gtest is pulled in as an dependably and not added to the libcamera\n> > tee should this not be ?\n> \n> It should indeed. Fixed for v2.\n> \n> >\n> > > +\n> > > using namespace libcamera;\n> > > \n> > > +std::shared_ptr cam;\n> > > +\n> >\n> > Would it be possible to factor the code so that the Camera instance is\n> > not a global variable? I'm thinking that this will not scale when tests\n> > requiring more then one camera are needed.\n> \n> So... This is something that annoyed me as well. The short answer is I think it\n> could, but the test registration code wouldn't be as clean.\n\nIMHO almost any mess that removes a global variable is worth it :-)\n\n> \n> The not so short answer: The only way I think this could be done is by adding a\n> parameter to every test case with the camera instance. But I'm currently\n> registering the test suites statically:\n> \n> INSTANTIATE_TEST_SUITE_P(Raw,\n> FixedRequestsTest,\n> testing::Combine(testing::Values(Raw),\n> \t\t\t\t\ttesting::ValuesIn(NUMREQUESTS)));\n> \n> So in order to pass the camera instance, they would need to be registered\n> dynamically, as the example in [1] shows, after the camera to use is parsed from\n> the command line.\n> \n> [1] https://google.github.io/googletest/advanced.html#registering-tests-programmatically\n> \n> But indeed, this seems to be needed to support working with multiple cameras.\n> The other solutions I was thinking about: making the global variable a vector of\n> cameras and/or running gtest multiple times and each time with the camera\n> variable set differently, wouldn't give a single report at the end with the\n> results from all cameras which would make it harder to parse the results.\n> \n> I'm not sure which tests would need multiple cameras (if you think of examples\n> let me know), but at least being able to run each of the tests for each of the\n> cameras on a system in a single lc-compliance run would be important I think.\n\nIt's a tricky question and its implementation may be even tricker. The \nfirst 'problem' I have in mind for this type of test are pipelines where \nsome part of the capture pipeline are shared between two discrete \nlibcamera cameras.\n\nIf we look closer at the RkISP1 pipeline handler it has a activeCamera_ \nvariable that designates witch camera is currently using the shared ISP. \nIt would be cool if lc-compliance had a test case that somehow could \nfigure out (either thru the API or informed using cli arguments) which \ncameras share resources and test that they error our correctly when \ntried to be used at the same time.\n\n> \n> So I'll take a stab at it and see how things go.\n> \n> Regarding the other issues, I'll have them fixed for v2.\n\nNice! Remember the on-top definition. As long as your work only uses a \n'previous bad idea' and not introduces a new one, it's fine to fix \nthings on-top :-)\n\n> \n> Thanks,\n> Nícolas","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 8212BBDE6A\n\tfor ;\n\tSat, 1 May 2021 07:07:35 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E099868901;\n\tSat, 1 May 2021 09:07:34 +0200 (CEST)","from mail-lj1-x22f.google.com (mail-lj1-x22f.google.com\n\t[IPv6:2a00:1450:4864:20::22f])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 4319C688AE\n\tfor ;\n\tSat, 1 May 2021 09:07:33 +0200 (CEST)","by mail-lj1-x22f.google.com with SMTP id a36so796277ljq.8\n\tfor ;\n\tSat, 01 May 2021 00:07:33 -0700 (PDT)","from localhost (h-209-203.A463.priv.bahnhof.se. [155.4.209.203])\n\tby smtp.gmail.com with ESMTPSA id\n\tu27sm452902lfm.239.2021.05.01.00.07.28\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tSat, 01 May 2021 00:07:29 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=ragnatech-se.20150623.gappssmtp.com\n\theader.i=@ragnatech-se.20150623.gappssmtp.com\n\theader.b=\"bSeEqpLs\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ragnatech-se.20150623.gappssmtp.com; s=20150623;\n\th=date:from:to:cc:subject:message-id:references:mime-version\n\t:content-disposition:content-transfer-encoding:in-reply-to;\n\tbh=59/FU5J8rQhpu4C0J9sLCMMJtGcV5SnqDIL8PJ7Ocbc=;\n\tb=bSeEqpLsyc60INdGC1Xr3+hgwPayT3E+2w84EpzGFlU3G0x4kTNPkgYVgtpTgmpqJU\n\t753yK8gFE3c/OWRcgAHIw9GjgFRtGqkRw3hCpauU5nWJft3XstMiq4f/K/3YahI9daky\n\tZbCmah1r1YrIlCPY+ZNeQ2n/FujguUkVMkfXbbpc0QfxBZpj6Im3YYrpVpgaEVDZEDNK\n\trd6oGvzyXiAorI4jhZBbldVDgUgzgDYWipi5fbBnyNicnLyrLj8v5tnz1ZNqlLj65jz1\n\tY8IuEVFCOGZU5mK3RGz3A4PdfBgxkts5VyqlgevTKDq2kZw3lX0NbyrxPSUwmBl44A6+\n\txKgg==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:date:from:to:cc:subject:message-id:references\n\t:mime-version:content-disposition:content-transfer-encoding\n\t:in-reply-to;\n\tbh=59/FU5J8rQhpu4C0J9sLCMMJtGcV5SnqDIL8PJ7Ocbc=;\n\tb=A/iGBQ6ykV4YI1eLGQoOXHnJJzVrj2kNHcwyoQWkHOtGt4sBsg6Iki5JEM2gACl4LC\n\t/2qfpMgVDeSDTDo2jYWiW8bTDSTFk4qtDGAl28JQwJj610lYw0ltB6L+jtZ8/2Cfbr9U\n\tPChIO7SJs6OHzle7Mj0hAudd7hM/4SZ52mszpDloGd+TnsBNDr0se/7ZMfK+4mgp6US5\n\tW0lRXVHQwGCX4cAN7eIaGjm4X96sfzL+JvDPcOA3wkhbKj8X1XwRO+yAXzUfYxpbRF+T\n\tn3FijpEF0cX5F1OQKXAjq9LWyL2RlcYbWe2O1lKXHZD3837dMT/cogBlg/7m6ZRPm31V\n\tkliQ==","X-Gm-Message-State":"AOAM530C8h34I+tP9lsxQqaZigRqB/l4HZmU6YaqVc52L8n3WesDDmxf\n\tYKtZV7KIYr2OPmig8Hmy7KQOAiuPqVGdOg==","X-Google-Smtp-Source":"ABdhPJwy8rTf1e7QU9Ggb85bJqzvK8v2dNQvyraj2sLwzo1SRJu9i71HneFSFxf8LAwyFoHBFWEFcw==","X-Received":"by 2002:a2e:8144:: with SMTP id t4mr6230302ljg.57.1619852852650; \n\tSat, 01 May 2021 00:07:32 -0700 (PDT)","Date":"Sat, 1 May 2021 09:07:28 +0200","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"=?iso-8859-1?q?N=EDcolas_F=2E_R=2E_A=2E?= Prado ","Message-ID":"","References":"\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [RFC PATCH 2/2] lc-compliance: Refactor using\n\tGoogletest","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, kernel@collabora.com,\n\t=?iso-8859-1?q?Andr=E9?= Almeida ","Content-Type":"text/plain; charset=\"iso-8859-1\"","Content-Transfer-Encoding":"quoted-printable","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]