[{"id":15678,"web_url":"https://patchwork.libcamera.org/comment/15678/","msgid":"","date":"2021-03-14T20:47:17","subject":"Re: [libcamera-devel] [PATCH v3 1/2] lc-compliance: Add a libcamera\n\tcompliance tool","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Niklas,\n\nThank you for the patch.\n\nOn Wed, Mar 10, 2021 at 04:44:13PM +0100, Niklas Söderlund wrote:\n> Add a compliance tool to ease testing of cameras. In contrast to the\n> unit-tests under test/ that aims to test the internal components of\n> libcamera the compliance tool aims to test application use-cases and to\n> some extend the public API.\n\ns/extend/extent/\n\n> This change adds the boilerplate code of a simple framework for the\n> creation of tests. The tests aim both to demonstrate the tool and to\n> catch real problems. The tests added are:\n> \n> - Test that if one queues exactly N requests to a camera exactly N\n> requests are eventually completed.\n\nThis is an ongoing discussion, but not a reason to delay merging this\nseries. We can always change the tests if we decide to reconsider this\nbehaviour.\n\n> - Test that a configured camera can be started and stopped multiple\n> times in an attempt to exercise cleanup code paths otherwise not\n> often tested with 'cam' for example.\n> \n> Signed-off-by: Niklas Söderlund \n\nYou'll find lots of comments below. Not all of them need to be addressed\nnow. Small and uncontroversial issues could be fixed in v4, while other\nissues can be addressed later. Appropriate \\todo comments would be\nuseful to ensure we don't forget about them.\n\n> ---\n> * Changes since v1\n> - Improve language in commit message and comments.\n> - Test all roles as they may exercise different code paths in the\n> pipeline.\n> - Move SimpleCapture to its own .cpp/.h files.\n> \n> * Changes since v2\n> - Fold in a use-after-free bug fix from Kieran, thanks!\n> ---\n> src/lc-compliance/main.cpp | 139 +++++++++++++++++++++++++\n> src/lc-compliance/meson.build | 25 +++++\n> src/lc-compliance/results.cpp | 75 ++++++++++++++\n> src/lc-compliance/results.h | 45 ++++++++\n> src/lc-compliance/simple_capture.cpp | 149 +++++++++++++++++++++++++++\n> src/lc-compliance/simple_capture.h | 54 ++++++++++\n> src/lc-compliance/single_stream.cpp | 75 ++++++++++++++\n> src/lc-compliance/tests.h | 16 +++\n\nI still wonder if we should reorganize test/ and store lc-compliance\nthere, but this can be handled later if desired.\n\n> src/meson.build | 2 +\n> 9 files changed, 580 insertions(+)\n> create mode 100644 src/lc-compliance/main.cpp\n> create mode 100644 src/lc-compliance/meson.build\n> create mode 100644 src/lc-compliance/results.cpp\n> create mode 100644 src/lc-compliance/results.h\n> create mode 100644 src/lc-compliance/simple_capture.cpp\n> create mode 100644 src/lc-compliance/simple_capture.h\n> create mode 100644 src/lc-compliance/single_stream.cpp\n> create mode 100644 src/lc-compliance/tests.h\n> \n> diff --git a/src/lc-compliance/main.cpp b/src/lc-compliance/main.cpp\n> new file mode 100644\n> index 0000000000000000..e1cbce7eac3df2bc\n> --- /dev/null\n> +++ b/src/lc-compliance/main.cpp\n> @@ -0,0 +1,139 @@\n> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> +/*\n> + * Copyright (C) 2020, Google Inc.\n> + *\n> + * main.cpp - lc-compliance - The libcamera compliance tool\n> + */\n> +\n> +#include \n> +#include \n> +#include \n> +\n> +#include \n> +\n> +#include \"../cam/options.h\"\n\nAlso not a blocker, but should we move options.h and options.cpp to\nsrc/???/ ? This is the third tool that uses the option parser.\n\n> +#include \"tests.h\"\n> +\n> +using namespace libcamera;\n> +\n> +class Harness\n> +{\n> +public:\n> +\tHarness();\n> +\t~Harness();\n> +\n> +\tint exec(int argc, char **argv);\n\nMy Qt bias would push me to pass argc and argv to the constructor, and\nturn exec() into a function that takes no argument. It won't have much\ninfluence on anything.\n\nOr maybe parsing arguments in main(), and passing the\nOptionsParser::Options to the constructor of the Harness class ? It may\nmake error handling for argument parsing easier.\n\n> +\n> +private:\n> +\tenum {\n> +\t\tOptCamera = 'c',\n> +\t\tOptHelp = 'h',\n> +\t};\n> +\n> +\tint parseOptions(int argc, char **argv);\n> +\tint init(int argc, char **argv);\n> +\n> +\tOptionsParser::Options options_;\n> +\tstd::unique_ptr cm_;\n> +\tstd::shared_ptr camera_;\n> +};\n> +\n> +Harness::Harness()\n> +{\n> +\tcm_ = std::make_unique();\n> +}\n> +\n> +Harness::~Harness()\n> +{\n> +\tif (camera_) {\n> +\t\tcamera_->release();\n> +\t\tcamera_.reset();\n> +\t}\n> +\n> +\tcm_->stop();\n> +}\n> +\n> +int Harness::exec(int argc, char **argv)\n> +{\n> +\tint ret = init(argc, argv);\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(int argc, char **argv)\n> +{\n> +\tint ret = parseOptions(argc, argv);\n> +\tif (ret < 0)\n> +\t\treturn ret;\n\nMaybe\n\n\tif (ret == -EINTR)\n\t\treturn 0;\n\tif (ret < 0)\n\t\treturn ret;\n\n? Specifying the help option should result in immediate termination, but\nnot with EXIT_FAILURE.\n\n> +\n> +\tret = cm_->start();\n> +\tif (ret) {\n> +\t\tstd::cout << \"Failed to start camera manager: \"\n> +\t\t\t << strerror(-ret) << std::endl;\n> +\t\treturn ret;\n> +\t}\n> +\n> +\tif (!options_.isSet(OptCamera)) {\n> +\t\tstd::cout << \"No camera specified, available cameras:\" << std::endl;\n> +\t\tfor (const std::shared_ptr &cam : cm_->cameras())\n> +\t\t\tstd::cout << \"- \" << cam.get()->id() << std::endl;\n\nYou could move this to a separate listCameras() function, as it's also\nuseful below.\n\n> +\t\treturn -ENODEV;\n> +\t}\n> +\n> +\tconst std::string &cameraId = options_[OptCamera];\n> +\tcamera_ = cm_->get(cameraId);\n> +\tif (!camera_) {\n> +\t\tstd::cout << \"Camera \" << cameraId << \" not found, available cameras:\" << std::endl;\n> +\t\tfor (const std::shared_ptr &cam : cm_->cameras())\n> +\t\t\tstd::cout << \"- \" << cam.get()->id() << std::endl;\n> +\t\treturn -ENODEV;\n> +\t}\n> +\n> +\tif (camera_->acquire()) {\n> +\t\tstd::cout << \"Failed to acquire camera\" << std::endl;\n> +\t\treturn -EINVAL;\n> +\t}\n> +\n> +\tstd::cout << \"Using camera \" << cameraId << std::endl;\n> +\n> +\treturn 0;\n> +}\n> +\n> +int Harness::parseOptions(int argc, char **argv)\n> +{\n> +\tOptionsParser parser;\n> +\tparser.addOption(OptCamera, OptionString,\n> +\t\t\t \"Specify which camera to operate on, by id\", \"camera\",\n> +\t\t\t ArgumentRequired, \"camera\");\n> +\tparser.addOption(OptHelp, OptionNone, \"Display this help message\",\n> +\t\t\t \"help\");\n> +\n> +\toptions_ = parser.parse(argc, argv);\n> +\tif (!options_.valid())\n> +\t\treturn -EINVAL;\n> +\n> +\tif (options_.empty() || options_.isSet(OptHelp)) {\n> +\t\tparser.usage();\n> +\t\treturn options_.empty() ? -EINVAL : -EINTR;\n> +\t}\n\nShould we drop the empty check here ? I assume it's here because the\ncamera option is mandatory. That's enforced by init(), and it's actually\nimpossible to get init() to list the cameras, as if the camera option\nisn't specified, the check here will return an error.\n\n> +\n> +\treturn 0;\n> +}\n> +\n> +int main(int argc, char **argv)\n> +{\n> +\tHarness harness;\n> +\treturn harness.exec(argc, argv) ? EXIT_FAILURE : 0;\n\ns/0/EXIT_SUCCESS/ ?\n\n> +}\n> diff --git a/src/lc-compliance/meson.build b/src/lc-compliance/meson.build\n> new file mode 100644\n> index 0000000000000000..68164537c1055f28\n> --- /dev/null\n> +++ b/src/lc-compliance/meson.build\n> @@ -0,0 +1,25 @@\n> +# SPDX-License-Identifier: CC0-1.0\n> +\n> +libevent = dependency('libevent_pthreads', required : false)\n\nIf desired, you could move this from src/cam/meson.build to\nsrc/meson.build to avoid finding the dependency twice. Up to you.\n\n> +\n> +if not libevent.found()\n> + warning('libevent_pthreads not found, \\'lc-compliance\\' application will not be compiled')\n> + subdir_done()\n> +endif\n> +\n> +lc_compliance_sources = files([\n> + '../cam/event_loop.cpp',\n\nAnother candidate for src/???/ :-)\n\n> + '../cam/options.cpp',\n> + 'main.cpp',\n> + 'results.cpp',\n> + 'simple_capture.cpp',\n> + 'single_stream.cpp',\n> +])\n> +\n> +lc_compliance = executable('lc-compliance', lc_compliance_sources,\n> + dependencies : [\n> + libatomic,\n> + libcamera_dep,\n> + libevent,\n> + ],\n> + install : true)\n> diff --git a/src/lc-compliance/results.cpp b/src/lc-compliance/results.cpp\n> new file mode 100644\n> index 0000000000000000..8c42eb2d6822aa60\n> --- /dev/null\n> +++ b/src/lc-compliance/results.cpp\n> @@ -0,0 +1,75 @@\n> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> +/*\n> + * Copyright (C) 2020, Google Inc.\n> + *\n> + * results.cpp - Test result aggregator\n> + */\n> +\n> +#include \"results.h\"\n> +\n> +#include \n> +\n> +void Results::add(const Result &result)\n> +{\n> +\tif (result.first == Pass)\n> +\t\tpassed_++;\n> +\telse if (result.first == Fail)\n> +\t\tfailed_++;\n> +\telse if (result.first == Skip)\n> +\t\tskipped_++;\n> +\n> +\tprintResult(result);\n> +}\n> +\n> +void Results::add(Status status, const std::string &message)\n> +{\n> +\tadd({ status, message });\n> +}\n> +\n> +void Results::fail(const std::string &message)\n> +{\n> +\tadd(Fail, message);\n> +}\n> +\n> +void Results::pass(const std::string &message)\n> +{\n> +\tadd(Pass, message);\n> +}\n> +\n> +void Results::skip(const std::string &message)\n> +{\n> +\tadd(Skip, message);\n> +}\n> +\n> +int Results::summary() const\n> +{\n> +\tif (failed_ + passed_ + skipped_ != planned_) {\n> +\t\tstd::cout << \"Planned and executed number of tests differ \"\n> +\t\t\t << failed_ + passed_ + skipped_ << \" executed \"\n> +\t\t\t << planned_ << \" planned\" << std::endl;\n> +\n> +\t\treturn -EINVAL;\n> +\t}\n> +\n> +\tstd::cout << planned_ << \" tests executed, \"\n> +\t\t << passed_ << \" tests passed, \"\n> +\t\t << skipped_ << \" tests skipped and \"\n> +\t\t << failed_ << \" tests failed \" << std::endl;\n> +\n> +\treturn 0;\n> +}\n> +\n> +void Results::printResult(const Result &result)\n> +{\n> +\tstd::string prefix;\n> +\n> +\t/* \\todo Make parsable as TAP. */\n> +\tif (result.first == Pass)\n> +\t\tprefix = \"PASS\";\n> +\telse if (result.first == Fail)\n> +\t\tprefix = \"FAIL\";\n> +\telse if (result.first == Skip)\n> +\t\tprefix = \"SKIP\";\n> +\n> +\tstd::cout << \"- \" << prefix << \" - \" << result.second << std::endl;\n\nMaybe s/\" - \"/\": \"/ ? (That's the second one, after the prefix)\n\n> +}\n> diff --git a/src/lc-compliance/results.h b/src/lc-compliance/results.h\n> new file mode 100644\n> index 0000000000000000..a02fd5ab46edd62c\n> --- /dev/null\n> +++ b/src/lc-compliance/results.h\n> @@ -0,0 +1,45 @@\n> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> +/*\n> + * Copyright (C) 2020, Google Inc.\n> + *\n> + * results.h - Test result aggregator\n> + */\n> +#ifndef __LC_COMPLIANCE_RESULTS_H__\n> +#define __LC_COMPLIANCE_RESULTS_H__\n> +\n> +#include \n\n#include \n\nfor std::pair.\n\n> +\n> +class Results\n> +{\n> +public:\n> +\tenum Status {\n> +\t\tFail,\n> +\t\tPass,\n> +\t\tSkip,\n> +\t};\n> +\n> +\tusing Result = std::pair;\n> +\n> +\tResults(unsigned int planned)\n> +\t\t: planned_(planned), passed_(0), failed_(0), skipped_(0)\n> +\t{\n> +\t}\n> +\n> +\tvoid add(const Result &result);\n> +\tvoid add(Status status, const std::string &message);\n> +\tvoid fail(const std::string &message);\n> +\tvoid pass(const std::string &message);\n> +\tvoid skip(const std::string &message);\n\nI foresee code sharing with test/ in the future (resisting the urge to\ngive it a go myself already :-)). How about adding a \\todo somewhere ?\nWe don't have to reinvent the wheel, we could use an existing test\nframework if there's one that fits our needs.\n\n> +\n> +\tint summary() const;\n> +\n> +private:\n> +\tvoid printResult(const Result &result);\n> +\n> +\tunsigned int planned_;\n> +\tunsigned int passed_;\n> +\tunsigned int failed_;\n> +\tunsigned int skipped_;\n> +};\n> +\n> +#endif /* __LC_COMPLIANCE_RESULTS_H__ */\n> diff --git a/src/lc-compliance/simple_capture.cpp b/src/lc-compliance/simple_capture.cpp\n> new file mode 100644\n> index 0000000000000000..fac8db379118efbd\n> --- /dev/null\n> +++ b/src/lc-compliance/simple_capture.cpp\n> @@ -0,0 +1,149 @@\n> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> +/*\n> + * Copyright (C) 2020, Google Inc.\n> + *\n> + * simple_capture.cpp - Simple capture helper\n> + */\n> +\n> +#include \"simple_capture.h\"\n> +\n> +using namespace libcamera;\n> +\n> +SimpleCapture::SimpleCapture(std::shared_ptr camera)\n> +\t: camera_(camera), allocator_(std::make_unique(camera))\n> +{\n> +}\n> +\n> +SimpleCapture::~SimpleCapture()\n> +{\n> +}\n> +\n> +Results::Result 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}\n> +\n> +\tif (camera_->configure(config_.get())) {\n> +\t\tconfig_.reset();\n> +\t\treturn { Results::Fail, \"Failed to configure camera\" };\n> +\t}\n> +\n> +\treturn { Results::Pass, \"Configure camera\" };\n> +}\n\nIs returning a Result here the best option. Shouldn't test results be\nconstructed by tests, and helper classes return statuses that let tests\ndecide what to do ? In particular, the success message generated by this\nfunction will never be used, as success applies to a full test, not to\nan invidual step.\n\nOn the other hand, the helper classes have the most detailed knowledge\nabout the errors they can encounter. I'm not sure what would be best. I\nwonder if this could be a valid use case for using exceptions. It would\nallow writing tests with assert-like functions. For instance, the code\nbelow that tests\n\n\tif (captureCount_ != captureLimit_)\n\t\treturn { Results::Fail, \"Got \" + std::to_string(captureCount_) + \" request, wanted \" + std::to_string(captureLimit_) };\n\ncould be replaced with a\n\n\tfailIfNotEqual(captureCount_, captureLimit, \"request(s)\");\n\nwhich would generate the message automatically.\n\n> +\n> +Results::Result 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> +\n> +\tif (camera_->start())\n> +\t\treturn { Results::Fail, \"Failed to start camera\" };\n> +\n> +\tcamera_->requestCompleted.connect(this, &SimpleCapture::requestComplete);\n> +\n> +\treturn { Results::Pass, \"Started camera\" };\n> +}\n> +\n> +Results::Result SimpleCapture::stop()\n> +{\n> +\tStream *stream = config_->at(0).stream();\n> +\n> +\tcamera_->stop();\n> +\n> +\tcamera_->requestCompleted.disconnect(this, &SimpleCapture::requestComplete);\n> +\n> +\tallocator_->free(stream);\n> +\n> +\treturn { Results::Pass, \"Stopped camera\" };\n\nGenerically speaking, should this kind of function be made idempotent,\nand be called from destructors ? It would make error handling easier.\n\n> +}\n> +\n> +/* SimpleCaptureBalanced */\n> +\n> +SimpleCaptureBalanced::SimpleCaptureBalanced(std::shared_ptr camera)\n> +\t: SimpleCapture(camera)\n> +{\n> +}\n> +\n> +Results::Result SimpleCaptureBalanced::capture(unsigned int numRequests)\n> +{\n> +\tResults::Result ret = start();\n> +\tif (ret.first != Results::Pass)\n> +\t\treturn ret;\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> +\t\tstop();\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\nThis makes me wonder what the granularity of a test should be. What is\n\"one test\", should it be a call to this function with a given number of\nbuffers, or one call to testSingleStream() ? I'm leaning towards the\nlater, defining a test as something that can be selected in a test plan.\nI can imagine selecting testSingleStream() through a command line\nargument (or through a configuration file), going for a smaller\ngranularity doesn't seem too useful. We need to log the status of\nindividual steps in a test, but at the top (and TAP) level, I'd limit it\nto the higher level.\n\n> +\t}\n> +\n> +\tqueueCount_ = 0;\n> +\tcaptureCount_ = 0;\n> +\tcaptureLimit_ = numRequests;\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\tif (!request) {\n> +\t\t\tstop();\n> +\t\t\treturn { Results::Fail, \"Can't create request\" };\n> +\t\t}\n> +\n> +\t\tif (request->addBuffer(stream, buffer.get())) {\n> +\t\t\tstop();\n> +\t\t\treturn { Results::Fail, \"Can't set buffer for request\" };\n> +\t\t}\n> +\n> +\t\tif (queueRequest(request.get()) < 0) {\n> +\t\t\tstop();\n> +\t\t\treturn { Results::Fail, \"Failed to queue request\" };\n> +\t\t}\n> +\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\nShould we construct the event loop in the constructor of the base class,\nand delete it in its destructor ?\n\n> +\n> +\tif (captureCount_ != captureLimit_)\n> +\t\treturn { Results::Fail, \"Got \" + std::to_string(captureCount_) + \" request, wanted \" + std::to_string(captureLimit_) };\n\nThis is a very long line.\n\n> +\n> +\treturn { Results::Pass, \"Balanced capture of \" + std::to_string(numRequests) + \" requests\" };\n> +}\n> +\n> +int SimpleCaptureBalanced::queueRequest(Request *request)\n> +{\n> +\tqueueCount_++;\n> +\tif (queueCount_ > captureLimit_)\n> +\t\treturn 0;\n> +\n> +\treturn camera_->queueRequest(request);\n> +}\n> +\n> +void SimpleCaptureBalanced::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/lc-compliance/simple_capture.h b/src/lc-compliance/simple_capture.h\n> new file mode 100644\n> index 0000000000000000..3a6afc538c623050\n> --- /dev/null\n> +++ b/src/lc-compliance/simple_capture.h\n> @@ -0,0 +1,54 @@\n> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> +/*\n> + * Copyright (C) 2020, Google Inc.\n> + *\n> + * simple_capture.h - Simple capture helper\n> + */\n> +#ifndef __LC_COMPLIANCE_SIMPLE_CAPTURE_H__\n> +#define __LC_COMPLIANCE_SIMPLE_CAPTURE_H__\n> +\n> +#include \n> +\n> +#include \n> +\n> +#include \"../cam/event_loop.h\"\n> +#include \"results.h\"\n> +\n> +class SimpleCapture\n> +{\n> +public:\n> +\tResults::Result configure(libcamera::StreamRole role);\n> +\n> +protected:\n> +\tSimpleCapture(std::shared_ptr camera);\n\nYou can pass a libcamera::Camera * as a borrowed reference here, as the\ncaller guarantees that the reference stays valid for the lifetime of\nthis class.\n\n> +\tvirtual ~SimpleCapture();\n> +\n> +\tResults::Result start();\n> +\tResults::Result stop();\n> +\n> +\tvirtual void requestComplete(libcamera::Request *request) = 0;\n> +\n> +\tEventLoop *loop_;\n> +\n> +\tstd::shared_ptr camera_;\n> +\tstd::unique_ptr allocator_;\n> +\tstd::unique_ptr config_;\n> +};\n> +\n> +class SimpleCaptureBalanced : public SimpleCapture\n> +{\n> +public:\n> +\tSimpleCaptureBalanced(std::shared_ptr camera);\n> +\n> +\tResults::Result capture(unsigned int numRequests);\n> +\n> +private:\n> +\tint queueRequest(libcamera::Request *request);\n> +\tvoid requestComplete(libcamera::Request *request) override;\n> +\n> +\tunsigned int queueCount_;\n> +\tunsigned int captureCount_;\n> +\tunsigned int captureLimit_;\n> +};\n> +\n> +#endif /* __LC_COMPLIANCE_SIMPLE_CAPTURE_H__ */\n> diff --git a/src/lc-compliance/single_stream.cpp b/src/lc-compliance/single_stream.cpp\n> new file mode 100644\n> index 0000000000000000..0ed6f5dcfb5a516d\n> --- /dev/null\n> +++ b/src/lc-compliance/single_stream.cpp\n> @@ -0,0 +1,75 @@\n> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> +/*\n> + * Copyright (C) 2020, Google Inc.\n> + *\n> + * single_stream.cpp - Test a single camera stream\n> + */\n> +\n> +#include \n> +\n> +#include \"simple_capture.h\"\n> +#include \"tests.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> +{\n> +\tSimpleCaptureBalanced capture(camera);\n> +\n> +\tResults::Result ret = capture.configure(role);\n> +\tif (ret.first != Results::Pass)\n> +\t\treturn ret;\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> +\n> +\treturn { Results::Pass, \"Balanced capture of \" + std::to_string(numRequests) + \" requests with \" + std::to_string(startCycles) + \" start cycles\" };\n\nThat's a very long line.\n\n> +}\n> +\n> +Results testSingleStream(std::shared_ptr camera)\n\nSame here, you can pass a borrowed reference as a pointer.\n\n> +{\n> +\tconst 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> +\tconst std::vector numRequests = { 1, 2, 3, 5, 8, 13, 21, 34, 55, 89 };\n\nShould both be static const ?\n\nI'm tempted to generate the fibonacci suite at runtime ;-) Totally\noverkill of course.\n\n> +\n> +\tResults results(numRequests.size() * roles.size() * 2);\n> +\n> +\tif (!camera)\n> +\t\treturn results;\n> +\n\nIs this needed, or can we assume the caller will give us a valid camera\n?\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> +\t}\n> +\n> +\treturn results;\n> +}\n> diff --git a/src/lc-compliance/tests.h b/src/lc-compliance/tests.h\n> new file mode 100644\n> index 0000000000000000..396605214e4b8980\n> --- /dev/null\n> +++ b/src/lc-compliance/tests.h\n> @@ -0,0 +1,16 @@\n> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> +/*\n> + * Copyright (C) 2020, Google Inc.\n> + *\n> + * tests.h - Test modules\n> + */\n> +#ifndef __LC_COMPLIANCE_TESTS_H__\n> +#define __LC_COMPLIANCE_TESTS_H__\n> +\n> +#include \n> +\n> +#include \"results.h\"\n> +\n> +Results testSingleStream(std::shared_ptr camera);\n> +\n> +#endif /* __LC_COMPLIANCE_TESTS_H__ */\n> diff --git a/src/meson.build b/src/meson.build\n> index c908b0675773301d..a8a6a572e4cf9482 100644\n> --- a/src/meson.build\n> +++ b/src/meson.build\n> @@ -20,6 +20,8 @@ subdir('android')\n> subdir('libcamera')\n> subdir('ipa')\n> \n> +subdir('lc-compliance')\n> +\n> subdir('cam')\n> subdir('qcam')\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 5EB8EBD80E\n\tfor ;\n\tSun, 14 Mar 2021 20:47:58 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5B4D668AA3;\n\tSun, 14 Mar 2021 21:47:54 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 96720602E3\n\tfor ;\n\tSun, 14 Mar 2021 21:47:53 +0100 (CET)","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 E7BC33E6;\n\tSun, 14 Mar 2021 21:47:52 +0100 (CET)"],"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=\"pV3yQ448\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1615754873;\n\tbh=MrHpEwcAMAtBouOrcNkRgQRU+MK/HmVFkK7ZD+bXPec=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=pV3yQ448+tkPmTyxL9kGBJDmYesNqCVIIML4P0Eo1f81HOe8M5TX5wF2X1I4knBt2\n\t9KsGYOg2vPRG7MjaI0oM7XF/8DgeKx9hNPnWLxvBNgG6rwrIT6m6iaPiQTSyirEnaq\n\t0juwDRYKYWwQimTUZwC8mFwcNOs2GVeZkjUKxVnQ=","Date":"Sun, 14 Mar 2021 22:47:17 +0200","From":"Laurent Pinchart ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Message-ID":"","References":"<20210310154414.3560115-1-niklas.soderlund@ragnatech.se>\n\t<20210310154414.3560115-2-niklas.soderlund@ragnatech.se>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210310154414.3560115-2-niklas.soderlund@ragnatech.se>","Subject":"Re: [libcamera-devel] [PATCH v3 1/2] lc-compliance: Add a libcamera\n\tcompliance tool","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","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16022,"web_url":"https://patchwork.libcamera.org/comment/16022/","msgid":"","date":"2021-03-29T16:44:51","subject":"Re: [libcamera-devel] [PATCH v3 1/2] lc-compliance: Add a libcamera\n\tcompliance tool","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Laurent,\n\nThanks for your feedback.\n\nOn 2021-03-14 22:47:17 +0200, Laurent Pinchart wrote:\n\n\n\n> > diff --git a/src/lc-compliance/simple_capture.cpp \n> > b/src/lc-compliance/simple_capture.cpp\n> > new file mode 100644\n> > index 0000000000000000..fac8db379118efbd\n> > --- /dev/null\n> > +++ b/src/lc-compliance/simple_capture.cpp\n> > @@ -0,0 +1,149 @@\n> > +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> > +/*\n> > + * Copyright (C) 2020, Google Inc.\n> > + *\n> > + * simple_capture.cpp - Simple capture helper\n> > + */\n> > +\n> > +#include \"simple_capture.h\"\n> > +\n> > +using namespace libcamera;\n> > +\n> > +SimpleCapture::SimpleCapture(std::shared_ptr camera)\n> > +\t: camera_(camera), allocator_(std::make_unique(camera))\n> > +{\n> > +}\n> > +\n> > +SimpleCapture::~SimpleCapture()\n> > +{\n> > +}\n> > +\n> > +Results::Result 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}\n> > +\n> > +\tif (camera_->configure(config_.get())) {\n> > +\t\tconfig_.reset();\n> > +\t\treturn { Results::Fail, \"Failed to configure camera\" };\n> > +\t}\n> > +\n> > +\treturn { Results::Pass, \"Configure camera\" };\n> > +}\n> \n> Is returning a Result here the best option. Shouldn't test results be\n> constructed by tests, and helper classes return statuses that let tests\n> decide what to do ? In particular, the success message generated by this\n> function will never be used, as success applies to a full test, not to\n> an invidual step.\n> \n> On the other hand, the helper classes have the most detailed knowledge\n> about the errors they can encounter. I'm not sure what would be best. I\n> wonder if this could be a valid use case for using exceptions. It would\n> allow writing tests with assert-like functions. For instance, the code\n> below that tests\n> \n> \tif (captureCount_ != captureLimit_)\n> \t\treturn { Results::Fail, \"Got \" + std::to_string(captureCount_) + \" request, wanted \" + std::to_string(captureLimit_) };\n> \n> could be replaced with a\n> \n> \tfailIfNotEqual(captureCount_, captureLimit, \"request(s)\");\n> \n> which would generate the message automatically.\n\nI think this is a very interesting idea. And I thought about it when \nwriting this but shrugged it off as i thought it would be hard to get \nacceptance upstream. I have not experimented with a prototype for this, \nbut maybe this is something we can do on-top as we inclemently improve \nthe structure of the tool.\n\n> \n> > +\n> > +Results::Result 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> > +\n> > +\tif (camera_->start())\n> > +\t\treturn { Results::Fail, \"Failed to start camera\" };\n> > +\n> > +\tcamera_->requestCompleted.connect(this, &SimpleCapture::requestComplete);\n> > +\n> > +\treturn { Results::Pass, \"Started camera\" };\n> > +}\n> > +\n> > +Results::Result SimpleCapture::stop()\n> > +{\n> > +\tStream *stream = config_->at(0).stream();\n> > +\n> > +\tcamera_->stop();\n> > +\n> > +\tcamera_->requestCompleted.disconnect(this, &SimpleCapture::requestComplete);\n> > +\n> > +\tallocator_->free(stream);\n> > +\n> > +\treturn { Results::Pass, \"Stopped camera\" };\n> \n> Generically speaking, should this kind of function be made idempotent,\n> and be called from destructors ? It would make error handling easier.\n\nWith an exception test flow I think it would make sens.\n\n> \n> > +}\n> > +\n> > +/* SimpleCaptureBalanced */\n> > +\n> > +SimpleCaptureBalanced::SimpleCaptureBalanced(std::shared_ptr camera)\n> > +\t: SimpleCapture(camera)\n> > +{\n> > +}\n> > +\n> > +Results::Result SimpleCaptureBalanced::capture(unsigned int numRequests)\n> > +{\n> > +\tResults::Result ret = start();\n> > +\tif (ret.first != Results::Pass)\n> > +\t\treturn ret;\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> > +\t\tstop();\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> \n> This makes me wonder what the granularity of a test should be. What is\n> \"one test\", should it be a call to this function with a given number of\n> buffers, or one call to testSingleStream() ? I'm leaning towards the\n> later, defining a test as something that can be selected in a test plan.\n> I can imagine selecting testSingleStream() through a command line\n> argument (or through a configuration file), going for a smaller\n> granularity doesn't seem too useful. We need to log the status of\n> individual steps in a test, but at the top (and TAP) level, I'd limit it\n> to the higher level.\n\nI'm heavily biased by my preference for TAP and the test framework 9pm \n(disclaimer partly developed by me) when it comes to how to structure \ntests.\n\nThe basic structure is a test case is a collection of TAP test points. \nEach case is specifies the number of test points it has and then in \norder lists if the test point is passed, skipped or failed. The test \ncases is then the worst possible aggregation of all test points. That is \nif one TAP is fail the whole test cases is fail. If one TAP is skip and \nthere are no fails the whole case is skip. And only if all TAPs are pass \nis the test case PASS.\n\nThis protects against test cases terminating prematurely as the harness \nknows how may TAPs to expect before the actual testing begins.\n\nThen you collect a list of test cases into a test suite and that is your \ntop level object.\n\nIn this case the testSingleStream is a test case while it could output \nany number of pass/fail/skip TAP points. And the collection of \ntestSingleStream, testRequestBalance, testRequestUnbalance would be a \ntest suite with 3 test-cases.\n\nBut I think the lesson learnt over time is to tailor your test structure \nto the tool/framework you intend to use. For example 9pm would be a \nhorrible match for the needs of lc-compliance. I think the best way \nforward for us is to move forward with a our own (does not have to be \nthis one) implementation of a test harness and once we know which \nenvironment it will be used most frequently pick an existing library and \nswitch to that.\n\n> \n> > +\t}\n> > +\n> > +\tqueueCount_ = 0;\n> > +\tcaptureCount_ = 0;\n> > +\tcaptureLimit_ = numRequests;\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\tif (!request) {\n> > +\t\t\tstop();\n> > +\t\t\treturn { Results::Fail, \"Can't create request\" };\n> > +\t\t}\n> > +\n> > +\t\tif (request->addBuffer(stream, buffer.get())) {\n> > +\t\t\tstop();\n> > +\t\t\treturn { Results::Fail, \"Can't set buffer for request\" };\n> > +\t\t}\n> > +\n> > +\t\tif (queueRequest(request.get()) < 0) {\n> > +\t\t\tstop();\n> > +\t\t\treturn { Results::Fail, \"Failed to queue request\" };\n> > +\t\t}\n> > +\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> Should we construct the event loop in the constructor of the base class,\n> and delete it in its destructor ?\n\nI thought about that too but ruled again it. Mostly because we had other \nissues with the EventLoop at the time and start/stop it did not really \nwork as intended. Maybe we can reopen that when we look at the Exception \ndriven testing and keep it simple and readable for now?\n\n> \n> > +\n> > +\tif (captureCount_ != captureLimit_)\n> > +\t\treturn { Results::Fail, \"Got \" + std::to_string(captureCount_) + \" request, wanted \" + std::to_string(captureLimit_) };\n> \n> This is a very long line.\n\nI opted to keep string constructing on a single line to ease grepping in \nthe code. You are now the n th person commenting about it so I will \nadmit defeat and break them :-)\n\n> \n> > +\n> > +\treturn { Results::Pass, \"Balanced capture of \" + std::to_string(numRequests) + \" requests\" };\n> > +}\n> > +\n> > +int SimpleCaptureBalanced::queueRequest(Request *request)\n> > +{\n> > +\tqueueCount_++;\n> > +\tif (queueCount_ > captureLimit_)\n> > +\t\treturn 0;\n> > +\n> > +\treturn camera_->queueRequest(request);\n> > +}\n> > +\n> > +void SimpleCaptureBalanced::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/lc-compliance/simple_capture.h b/src/lc-compliance/simple_capture.h\n> > new file mode 100644\n> > index 0000000000000000..3a6afc538c623050\n> > --- /dev/null\n> > +++ b/src/lc-compliance/simple_capture.h\n> > @@ -0,0 +1,54 @@\n> > +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> > +/*\n> > + * Copyright (C) 2020, Google Inc.\n> > + *\n> > + * simple_capture.h - Simple capture helper\n> > + */\n> > +#ifndef __LC_COMPLIANCE_SIMPLE_CAPTURE_H__\n> > +#define __LC_COMPLIANCE_SIMPLE_CAPTURE_H__\n> > +\n> > +#include \n> > +\n> > +#include \n> > +\n> > +#include \"../cam/event_loop.h\"\n> > +#include \"results.h\"\n> > +\n> > +class SimpleCapture\n> > +{\n> > +public:\n> > +\tResults::Result configure(libcamera::StreamRole role);\n> > +\n> > +protected:\n> > +\tSimpleCapture(std::shared_ptr camera);\n> \n> You can pass a libcamera::Camera * as a borrowed reference here, as the\n> caller guarantees that the reference stays valid for the lifetime of\n> this class.\n\nWe could, I opted to use the shared_ptr to keep the use-cases as close \nto what simple applications would use to be able to copy/past code form \nthis tool in future when i forgotten the API ;-)\n\nI'm not oppose to changing it but would prefers to keep it the way it \nis.\n\n","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 2B836C32EA\n\tfor ;\n\tMon, 29 Mar 2021 16:44:59 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 80F6A68784;\n\tMon, 29 Mar 2021 18:44:58 +0200 (CEST)","from mail-lj1-x231.google.com (mail-lj1-x231.google.com\n\t[IPv6:2a00:1450:4864:20::231])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D19BF6877D\n\tfor ;\n\tMon, 29 Mar 2021 18:44:56 +0200 (CEST)","by mail-lj1-x231.google.com with SMTP id o16so2645964ljp.3\n\tfor ;\n\tMon, 29 Mar 2021 09:44:56 -0700 (PDT)","from localhost (h-209-203.A463.priv.bahnhof.se. [155.4.209.203])\n\tby smtp.gmail.com with ESMTPSA id\n\ty10sm1977289lfl.240.2021.03.29.09.44.52\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tMon, 29 Mar 2021 09:44:52 -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=\"DQ6zL8SX\"; 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=/6iXE71sW6/7R/bNAExoz+9Mj7ZF+htY75s9ms7WMUQ=;\n\tb=DQ6zL8SX34ZSFg3DHeel8GSDG/7h1ZRx1kbHa50OUBQJJXHrshBrwJh00ifdfRQ+m8\n\tBw0r6gSUtkTRZUkhMfPJegQOPi+o8zt0uPH/lUHpAi2mTXF2lLRZ1+Jo2sLI5yIE0qal\n\tNd5sFQtiGG8tOVhs4znw8P0a5fORa4nVTdZ09uLeZqandAQ0QzUznY96JpCs8zuWZ4yu\n\toC28I528+CjV+S3/Z0W2+e7k+dLlWp7ZTV0Pi812XRcCshyWuUTHnYIufj85z7/nWv0X\n\thfDLJi7X7AN8179PUUXcspxkcm36OUiBTvDJZEf4cexZ+IKDYAl+FFcWnaKOvNvhdnzK\n\tCFFA==","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=/6iXE71sW6/7R/bNAExoz+9Mj7ZF+htY75s9ms7WMUQ=;\n\tb=slQtNfnVIUFAntTWMMEa2kwetfp1EejQ52Uh9Mu4wrJUE0uLNGTln2+ulsa/pNR7YW\n\tiSZHmNipMmY8yir89HmORiD8RI9dc8roYxpYXEl66FlbiornfDoS7ox4eVGvc0k0G83A\n\tUrqNpUK+y4vUDyS12vnStYh1ctGFkVUCzyCGJ6wRF2dvBpocSouqzxUQy+sK5oMgkJ9B\n\tf0WFLvLQ7Hekx2s1D6p5hwyRQOb4F2WrPSCg7hoY8aRIcFSVc69UUUAfXTr2zjR3iIMx\n\tInA5fQHjeTP7bEPBwKouZ5K9+U0WSBHylJjXOvZTh9kmOFeFgxr/s8GCen+AJMhNyAWm\n\tRpMg==","X-Gm-Message-State":"AOAM533NrZuFIHZHjOVSmsSAafnO0iYvTBJ+ptZgIMji8QDfFKbwQANy\n\tPpnqt8hRgQ23zJw0D3k/PvXj6G2Vu9f8+RkJ","X-Google-Smtp-Source":"ABdhPJz7te5uuXvELK9Z59G0QHg0dNUGcsUo/2PQmLvMcLfvMfysIsINYByP4vLGUW6fS492UCE6Bg==","X-Received":"by 2002:a2e:9310:: with SMTP id\n\te16mr18475067ljh.226.1617036294705; \n\tMon, 29 Mar 2021 09:44:54 -0700 (PDT)","Date":"Mon, 29 Mar 2021 18:44:51 +0200","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Laurent Pinchart ","Message-ID":"","References":"<20210310154414.3560115-1-niklas.soderlund@ragnatech.se>\n\t<20210310154414.3560115-2-niklas.soderlund@ragnatech.se>\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v3 1/2] lc-compliance: Add a libcamera\n\tcompliance tool","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","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":16031,"web_url":"https://patchwork.libcamera.org/comment/16031/","msgid":"","date":"2021-03-29T23:31:54","subject":"Re: [libcamera-devel] [PATCH v3 1/2] lc-compliance: Add a libcamera\n\tcompliance tool","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Niklas,\n\nOn Mon, Mar 29, 2021 at 06:44:51PM +0200, Niklas Söderlund wrote:\n> Hi Laurent,\n> \n> Thanks for your feedback.\n> \n> On 2021-03-14 22:47:17 +0200, Laurent Pinchart wrote:\n> \n> \n> \n> > > diff --git a/src/lc-compliance/simple_capture.cpp \n> > > b/src/lc-compliance/simple_capture.cpp\n> > > new file mode 100644\n> > > index 0000000000000000..fac8db379118efbd\n> > > --- /dev/null\n> > > +++ b/src/lc-compliance/simple_capture.cpp\n> > > @@ -0,0 +1,149 @@\n> > > +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> > > +/*\n> > > + * Copyright (C) 2020, Google Inc.\n> > > + *\n> > > + * simple_capture.cpp - Simple capture helper\n> > > + */\n> > > +\n> > > +#include \"simple_capture.h\"\n> > > +\n> > > +using namespace libcamera;\n> > > +\n> > > +SimpleCapture::SimpleCapture(std::shared_ptr camera)\n> > > +\t: camera_(camera), allocator_(std::make_unique(camera))\n> > > +{\n> > > +}\n> > > +\n> > > +SimpleCapture::~SimpleCapture()\n> > > +{\n> > > +}\n> > > +\n> > > +Results::Result 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}\n> > > +\n> > > +\tif (camera_->configure(config_.get())) {\n> > > +\t\tconfig_.reset();\n> > > +\t\treturn { Results::Fail, \"Failed to configure camera\" };\n> > > +\t}\n> > > +\n> > > +\treturn { Results::Pass, \"Configure camera\" };\n> > > +}\n> > \n> > Is returning a Result here the best option. Shouldn't test results be\n> > constructed by tests, and helper classes return statuses that let tests\n> > decide what to do ? In particular, the success message generated by this\n> > function will never be used, as success applies to a full test, not to\n> > an invidual step.\n> > \n> > On the other hand, the helper classes have the most detailed knowledge\n> > about the errors they can encounter. I'm not sure what would be best. I\n> > wonder if this could be a valid use case for using exceptions. It would\n> > allow writing tests with assert-like functions. For instance, the code\n> > below that tests\n> > \n> > \tif (captureCount_ != captureLimit_)\n> > \t\treturn { Results::Fail, \"Got \" + std::to_string(captureCount_) + \" request, wanted \" + std::to_string(captureLimit_) };\n> > \n> > could be replaced with a\n> > \n> > \tfailIfNotEqual(captureCount_, captureLimit, \"request(s)\");\n> > \n> > which would generate the message automatically.\n> \n> I think this is a very interesting idea. And I thought about it when \n> writing this but shrugged it off as i thought it would be hard to get \n> acceptance upstream. I have not experimented with a prototype for this, \n> but maybe this is something we can do on-top as we inclemently improve \n> the structure of the tool.\n\nhttps://bugs.libcamera.org/show_bug.cgi?id=17\n\n> > > +\n> > > +Results::Result 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> > > +\n> > > +\tif (camera_->start())\n> > > +\t\treturn { Results::Fail, \"Failed to start camera\" };\n> > > +\n> > > +\tcamera_->requestCompleted.connect(this, &SimpleCapture::requestComplete);\n> > > +\n> > > +\treturn { Results::Pass, \"Started camera\" };\n> > > +}\n> > > +\n> > > +Results::Result SimpleCapture::stop()\n> > > +{\n> > > +\tStream *stream = config_->at(0).stream();\n> > > +\n> > > +\tcamera_->stop();\n> > > +\n> > > +\tcamera_->requestCompleted.disconnect(this, &SimpleCapture::requestComplete);\n> > > +\n> > > +\tallocator_->free(stream);\n> > > +\n> > > +\treturn { Results::Pass, \"Stopped camera\" };\n> > \n> > Generically speaking, should this kind of function be made idempotent,\n> > and be called from destructors ? It would make error handling easier.\n> \n> With an exception test flow I think it would make sens.\n\nhttps://bugs.libcamera.org/show_bug.cgi?id=18\n\n> > > +}\n> > > +\n> > > +/* SimpleCaptureBalanced */\n> > > +\n> > > +SimpleCaptureBalanced::SimpleCaptureBalanced(std::shared_ptr camera)\n> > > +\t: SimpleCapture(camera)\n> > > +{\n> > > +}\n> > > +\n> > > +Results::Result SimpleCaptureBalanced::capture(unsigned int numRequests)\n> > > +{\n> > > +\tResults::Result ret = start();\n> > > +\tif (ret.first != Results::Pass)\n> > > +\t\treturn ret;\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> > > +\t\tstop();\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> > \n> > This makes me wonder what the granularity of a test should be. What is\n> > \"one test\", should it be a call to this function with a given number of\n> > buffers, or one call to testSingleStream() ? I'm leaning towards the\n> > later, defining a test as something that can be selected in a test plan.\n> > I can imagine selecting testSingleStream() through a command line\n> > argument (or through a configuration file), going for a smaller\n> > granularity doesn't seem too useful. We need to log the status of\n> > individual steps in a test, but at the top (and TAP) level, I'd limit it\n> > to the higher level.\n> \n> I'm heavily biased by my preference for TAP and the test framework 9pm \n> (disclaimer partly developed by me) when it comes to how to structure \n> tests.\n> \n> The basic structure is a test case is a collection of TAP test points. \n> Each case is specifies the number of test points it has and then in \n> order lists if the test point is passed, skipped or failed. The test \n> cases is then the worst possible aggregation of all test points. That is \n> if one TAP is fail the whole test cases is fail. If one TAP is skip and \n> there are no fails the whole case is skip. And only if all TAPs are pass \n> is the test case PASS.\n> \n> This protects against test cases terminating prematurely as the harness \n> knows how may TAPs to expect before the actual testing begins.\n> \n> Then you collect a list of test cases into a test suite and that is your \n> top level object.\n\nSo this means that while test cases can contain multiple test points,\nthe granularity of selecting what tests to run would be at the test case\nlevel, not the test point level ?\n\n> In this case the testSingleStream is a test case while it could output \n> any number of pass/fail/skip TAP points. And the collection of \n> testSingleStream, testRequestBalance, testRequestUnbalance would be a \n> test suite with 3 test-cases.\n\nYou mention that test cases need to report how many test points they\nproduce. How does this work when a test point failure is fatal and\nprevents other test points from running ?\n\n> But I think the lesson learnt over time is to tailor your test structure \n> to the tool/framework you intend to use. For example 9pm would be a \n> horrible match for the needs of lc-compliance. I think the best way \n> forward for us is to move forward with a our own (does not have to be \n> this one) implementation of a test harness and once we know which \n> environment it will be used most frequently pick an existing library and \n> switch to that.\n> \n> > > +\t}\n> > > +\n> > > +\tqueueCount_ = 0;\n> > > +\tcaptureCount_ = 0;\n> > > +\tcaptureLimit_ = numRequests;\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\tif (!request) {\n> > > +\t\t\tstop();\n> > > +\t\t\treturn { Results::Fail, \"Can't create request\" };\n> > > +\t\t}\n> > > +\n> > > +\t\tif (request->addBuffer(stream, buffer.get())) {\n> > > +\t\t\tstop();\n> > > +\t\t\treturn { Results::Fail, \"Can't set buffer for request\" };\n> > > +\t\t}\n> > > +\n> > > +\t\tif (queueRequest(request.get()) < 0) {\n> > > +\t\t\tstop();\n> > > +\t\t\treturn { Results::Fail, \"Failed to queue request\" };\n> > > +\t\t}\n> > > +\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> > Should we construct the event loop in the constructor of the base class,\n> > and delete it in its destructor ?\n> \n> I thought about that too but ruled again it. Mostly because we had other \n> issues with the EventLoop at the time and start/stop it did not really \n> work as intended. Maybe we can reopen that when we look at the Exception \n> driven testing and keep it simple and readable for now?\n> \n> > > +\n> > > +\tif (captureCount_ != captureLimit_)\n> > > +\t\treturn { Results::Fail, \"Got \" + std::to_string(captureCount_) + \" request, wanted \" + std::to_string(captureLimit_) };\n> > \n> > This is a very long line.\n> \n> I opted to keep string constructing on a single line to ease grepping in \n> the code. You are now the n th person commenting about it so I will \n> admit defeat and break them :-)\n> \n> > > +\n> > > +\treturn { Results::Pass, \"Balanced capture of \" + std::to_string(numRequests) + \" requests\" };\n> > > +}\n> > > +\n> > > +int SimpleCaptureBalanced::queueRequest(Request *request)\n> > > +{\n> > > +\tqueueCount_++;\n> > > +\tif (queueCount_ > captureLimit_)\n> > > +\t\treturn 0;\n> > > +\n> > > +\treturn camera_->queueRequest(request);\n> > > +}\n> > > +\n> > > +void SimpleCaptureBalanced::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/lc-compliance/simple_capture.h b/src/lc-compliance/simple_capture.h\n> > > new file mode 100644\n> > > index 0000000000000000..3a6afc538c623050\n> > > --- /dev/null\n> > > +++ b/src/lc-compliance/simple_capture.h\n> > > @@ -0,0 +1,54 @@\n> > > +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> > > +/*\n> > > + * Copyright (C) 2020, Google Inc.\n> > > + *\n> > > + * simple_capture.h - Simple capture helper\n> > > + */\n> > > +#ifndef __LC_COMPLIANCE_SIMPLE_CAPTURE_H__\n> > > +#define __LC_COMPLIANCE_SIMPLE_CAPTURE_H__\n> > > +\n> > > +#include \n> > > +\n> > > +#include \n> > > +\n> > > +#include \"../cam/event_loop.h\"\n> > > +#include \"results.h\"\n> > > +\n> > > +class SimpleCapture\n> > > +{\n> > > +public:\n> > > +\tResults::Result configure(libcamera::StreamRole role);\n> > > +\n> > > +protected:\n> > > +\tSimpleCapture(std::shared_ptr camera);\n> > \n> > You can pass a libcamera::Camera * as a borrowed reference here, as the\n> > caller guarantees that the reference stays valid for the lifetime of\n> > this class.\n> \n> We could, I opted to use the shared_ptr to keep the use-cases as close \n> to what simple applications would use to be able to copy/past code form \n> this tool in future when i forgotten the API ;-)\n\nApplications can also use borrowed pointers :-) I think that using a\nshared_ptr to model a reference that can be held, and a normal pointer\nfor a borrowed reference that must not be stored behind the caller's\nback may make the code more explicit. There's also a small performance\ndifference, which doesn't matter here.\n\nAt this point I don't think this matters much, we can revisit the code\nlater.\n\n> I'm not oppose to changing it but would prefers to keep it the way it \n> is.","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 C31E4C32F0\n\tfor ;\n\tMon, 29 Mar 2021 23:32:42 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 12E1F68784;\n\tTue, 30 Mar 2021 01:32:42 +0200 (CEST)","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 DEA61602D7\n\tfor ;\n\tTue, 30 Mar 2021 01:32:40 +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 283D0292;\n\tTue, 30 Mar 2021 01:32:39 +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=\"Lxui7hox\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1617060760;\n\tbh=fNdgxzjN4+8Bz0oOp5kHYiGGw5R0kdhSRl7Os1z1YNM=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Lxui7hoxcAxUa3A+1OnElzq3mTdCYCa/7nj4ivGJ5SRViU/OAowvlcLAWxU4uh5AB\n\tq2iaL7MEkt2bpqg0ZD8IEIqU5me3cj9UwxwNYJPXrKJoPUARVGo5xFQqIyJ4jZu0cM\n\tRM3c6ptnb221ZvuhNvNdjb+Y4pbhcp/80/EilkJE=","Date":"Tue, 30 Mar 2021 02:31:54 +0300","From":"Laurent Pinchart ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Message-ID":"","References":"<20210310154414.3560115-1-niklas.soderlund@ragnatech.se>\n\t<20210310154414.3560115-2-niklas.soderlund@ragnatech.se>\n\t\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v3 1/2] lc-compliance: Add a libcamera\n\tcompliance tool","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","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16033,"web_url":"https://patchwork.libcamera.org/comment/16033/","msgid":"","date":"2021-03-29T23:45:09","subject":"Re: [libcamera-devel] [PATCH v3 1/2] lc-compliance: Add a libcamera\n\tcompliance tool","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Niklas,\n\nOn Tue, Mar 30, 2021 at 02:31:57AM +0300, Laurent Pinchart wrote:\n> On Mon, Mar 29, 2021 at 06:44:51PM +0200, Niklas Söderlund wrote:\n> > On 2021-03-14 22:47:17 +0200, Laurent Pinchart wrote:\n> > \n> > \n> > \n> > > > diff --git a/src/lc-compliance/simple_capture.cpp \n> > > > b/src/lc-compliance/simple_capture.cpp\n> > > > new file mode 100644\n> > > > index 0000000000000000..fac8db379118efbd\n> > > > --- /dev/null\n> > > > +++ b/src/lc-compliance/simple_capture.cpp\n> > > > @@ -0,0 +1,149 @@\n> > > > +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> > > > +/*\n> > > > + * Copyright (C) 2020, Google Inc.\n> > > > + *\n> > > > + * simple_capture.cpp - Simple capture helper\n> > > > + */\n> > > > +\n> > > > +#include \"simple_capture.h\"\n> > > > +\n> > > > +using namespace libcamera;\n> > > > +\n> > > > +SimpleCapture::SimpleCapture(std::shared_ptr camera)\n> > > > +\t: camera_(camera), allocator_(std::make_unique(camera))\n> > > > +{\n> > > > +}\n> > > > +\n> > > > +SimpleCapture::~SimpleCapture()\n> > > > +{\n> > > > +}\n> > > > +\n> > > > +Results::Result 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}\n> > > > +\n> > > > +\tif (camera_->configure(config_.get())) {\n> > > > +\t\tconfig_.reset();\n> > > > +\t\treturn { Results::Fail, \"Failed to configure camera\" };\n> > > > +\t}\n> > > > +\n> > > > +\treturn { Results::Pass, \"Configure camera\" };\n> > > > +}\n> > > \n> > > Is returning a Result here the best option. Shouldn't test results be\n> > > constructed by tests, and helper classes return statuses that let tests\n> > > decide what to do ? In particular, the success message generated by this\n> > > function will never be used, as success applies to a full test, not to\n> > > an invidual step.\n> > > \n> > > On the other hand, the helper classes have the most detailed knowledge\n> > > about the errors they can encounter. I'm not sure what would be best. I\n> > > wonder if this could be a valid use case for using exceptions. It would\n> > > allow writing tests with assert-like functions. For instance, the code\n> > > below that tests\n> > > \n> > > \tif (captureCount_ != captureLimit_)\n> > > \t\treturn { Results::Fail, \"Got \" + std::to_string(captureCount_) + \" request, wanted \" + std::to_string(captureLimit_) };\n> > > \n> > > could be replaced with a\n> > > \n> > > \tfailIfNotEqual(captureCount_, captureLimit, \"request(s)\");\n> > > \n> > > which would generate the message automatically.\n> > \n> > I think this is a very interesting idea. And I thought about it when \n> > writing this but shrugged it off as i thought it would be hard to get \n> > acceptance upstream. I have not experimented with a prototype for this, \n> > but maybe this is something we can do on-top as we inclemently improve \n> > the structure of the tool.\n> \n> https://bugs.libcamera.org/show_bug.cgi?id=17\n> \n> > > > +\n> > > > +Results::Result 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> > > > +\n> > > > +\tif (camera_->start())\n> > > > +\t\treturn { Results::Fail, \"Failed to start camera\" };\n> > > > +\n> > > > +\tcamera_->requestCompleted.connect(this, &SimpleCapture::requestComplete);\n> > > > +\n> > > > +\treturn { Results::Pass, \"Started camera\" };\n> > > > +}\n> > > > +\n> > > > +Results::Result SimpleCapture::stop()\n> > > > +{\n> > > > +\tStream *stream = config_->at(0).stream();\n> > > > +\n> > > > +\tcamera_->stop();\n> > > > +\n> > > > +\tcamera_->requestCompleted.disconnect(this, &SimpleCapture::requestComplete);\n> > > > +\n> > > > +\tallocator_->free(stream);\n> > > > +\n> > > > +\treturn { Results::Pass, \"Stopped camera\" };\n> > > \n> > > Generically speaking, should this kind of function be made idempotent,\n> > > and be called from destructors ? It would make error handling easier.\n> > \n> > With an exception test flow I think it would make sens.\n> \n> https://bugs.libcamera.org/show_bug.cgi?id=18\n> \n> > > > +}\n> > > > +\n> > > > +/* SimpleCaptureBalanced */\n> > > > +\n> > > > +SimpleCaptureBalanced::SimpleCaptureBalanced(std::shared_ptr camera)\n> > > > +\t: SimpleCapture(camera)\n> > > > +{\n> > > > +}\n> > > > +\n> > > > +Results::Result SimpleCaptureBalanced::capture(unsigned int numRequests)\n> > > > +{\n> > > > +\tResults::Result ret = start();\n> > > > +\tif (ret.first != Results::Pass)\n> > > > +\t\treturn ret;\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> > > > +\t\tstop();\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> > > \n> > > This makes me wonder what the granularity of a test should be. What is\n> > > \"one test\", should it be a call to this function with a given number of\n> > > buffers, or one call to testSingleStream() ? I'm leaning towards the\n> > > later, defining a test as something that can be selected in a test plan.\n> > > I can imagine selecting testSingleStream() through a command line\n> > > argument (or through a configuration file), going for a smaller\n> > > granularity doesn't seem too useful. We need to log the status of\n> > > individual steps in a test, but at the top (and TAP) level, I'd limit it\n> > > to the higher level.\n> > \n> > I'm heavily biased by my preference for TAP and the test framework 9pm \n> > (disclaimer partly developed by me) when it comes to how to structure \n> > tests.\n> > \n> > The basic structure is a test case is a collection of TAP test points. \n> > Each case is specifies the number of test points it has and then in \n> > order lists if the test point is passed, skipped or failed. The test \n> > cases is then the worst possible aggregation of all test points. That is \n> > if one TAP is fail the whole test cases is fail. If one TAP is skip and \n> > there are no fails the whole case is skip. And only if all TAPs are pass \n> > is the test case PASS.\n> > \n> > This protects against test cases terminating prematurely as the harness \n> > knows how may TAPs to expect before the actual testing begins.\n> > \n> > Then you collect a list of test cases into a test suite and that is your \n> > top level object.\n> \n> So this means that while test cases can contain multiple test points,\n> the granularity of selecting what tests to run would be at the test case\n> level, not the test point level ?\n> \n> > In this case the testSingleStream is a test case while it could output \n> > any number of pass/fail/skip TAP points. And the collection of \n> > testSingleStream, testRequestBalance, testRequestUnbalance would be a \n> > test suite with 3 test-cases.\n> \n> You mention that test cases need to report how many test points they\n> produce. How does this work when a test point failure is fatal and\n> prevents other test points from running ?\n\nhttps://bugs.libcamera.org/show_bug.cgi?id=19\n\n> > But I think the lesson learnt over time is to tailor your test structure \n> > to the tool/framework you intend to use. For example 9pm would be a \n> > horrible match for the needs of lc-compliance. I think the best way \n> > forward for us is to move forward with a our own (does not have to be \n> > this one) implementation of a test harness and once we know which \n> > environment it will be used most frequently pick an existing library and \n> > switch to that.\n> > \n> > > > +\t}\n> > > > +\n> > > > +\tqueueCount_ = 0;\n> > > > +\tcaptureCount_ = 0;\n> > > > +\tcaptureLimit_ = numRequests;\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\tif (!request) {\n> > > > +\t\t\tstop();\n> > > > +\t\t\treturn { Results::Fail, \"Can't create request\" };\n> > > > +\t\t}\n> > > > +\n> > > > +\t\tif (request->addBuffer(stream, buffer.get())) {\n> > > > +\t\t\tstop();\n> > > > +\t\t\treturn { Results::Fail, \"Can't set buffer for request\" };\n> > > > +\t\t}\n> > > > +\n> > > > +\t\tif (queueRequest(request.get()) < 0) {\n> > > > +\t\t\tstop();\n> > > > +\t\t\treturn { Results::Fail, \"Failed to queue request\" };\n> > > > +\t\t}\n> > > > +\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> > > Should we construct the event loop in the constructor of the base class,\n> > > and delete it in its destructor ?\n> > \n> > I thought about that too but ruled again it. Mostly because we had other \n> > issues with the EventLoop at the time and start/stop it did not really \n> > work as intended. Maybe we can reopen that when we look at the Exception \n> > driven testing and keep it simple and readable for now?\n> > \n> > > > +\n> > > > +\tif (captureCount_ != captureLimit_)\n> > > > +\t\treturn { Results::Fail, \"Got \" + std::to_string(captureCount_) + \" request, wanted \" + std::to_string(captureLimit_) };\n> > > \n> > > This is a very long line.\n> > \n> > I opted to keep string constructing on a single line to ease grepping in \n> > the code. You are now the n th person commenting about it so I will \n> > admit defeat and break them :-)\n> > \n> > > > +\n> > > > +\treturn { Results::Pass, \"Balanced capture of \" + std::to_string(numRequests) + \" requests\" };\n> > > > +}\n> > > > +\n> > > > +int SimpleCaptureBalanced::queueRequest(Request *request)\n> > > > +{\n> > > > +\tqueueCount_++;\n> > > > +\tif (queueCount_ > captureLimit_)\n> > > > +\t\treturn 0;\n> > > > +\n> > > > +\treturn camera_->queueRequest(request);\n> > > > +}\n> > > > +\n> > > > +void SimpleCaptureBalanced::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/lc-compliance/simple_capture.h b/src/lc-compliance/simple_capture.h\n> > > > new file mode 100644\n> > > > index 0000000000000000..3a6afc538c623050\n> > > > --- /dev/null\n> > > > +++ b/src/lc-compliance/simple_capture.h\n> > > > @@ -0,0 +1,54 @@\n> > > > +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> > > > +/*\n> > > > + * Copyright (C) 2020, Google Inc.\n> > > > + *\n> > > > + * simple_capture.h - Simple capture helper\n> > > > + */\n> > > > +#ifndef __LC_COMPLIANCE_SIMPLE_CAPTURE_H__\n> > > > +#define __LC_COMPLIANCE_SIMPLE_CAPTURE_H__\n> > > > +\n> > > > +#include \n> > > > +\n> > > > +#include \n> > > > +\n> > > > +#include \"../cam/event_loop.h\"\n> > > > +#include \"results.h\"\n> > > > +\n> > > > +class SimpleCapture\n> > > > +{\n> > > > +public:\n> > > > +\tResults::Result configure(libcamera::StreamRole role);\n> > > > +\n> > > > +protected:\n> > > > +\tSimpleCapture(std::shared_ptr camera);\n> > > \n> > > You can pass a libcamera::Camera * as a borrowed reference here, as the\n> > > caller guarantees that the reference stays valid for the lifetime of\n> > > this class.\n> > \n> > We could, I opted to use the shared_ptr to keep the use-cases as close \n> > to what simple applications would use to be able to copy/past code form \n> > this tool in future when i forgotten the API ;-)\n> \n> Applications can also use borrowed pointers :-) I think that using a\n> shared_ptr to model a reference that can be held, and a normal pointer\n> for a borrowed reference that must not be stored behind the caller's\n> back may make the code more explicit. There's also a small performance\n> difference, which doesn't matter here.\n> \n> At this point I don't think this matters much, we can revisit the code\n> later.\n> \n> > I'm not oppose to changing it but would prefers to keep it the way it \n> > is.","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 02E80C32F0\n\tfor ;\n\tMon, 29 Mar 2021 23:45:56 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 68A8968786;\n\tTue, 30 Mar 2021 01:45:55 +0200 (CEST)","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 48EAA602D7\n\tfor ;\n\tTue, 30 Mar 2021 01:45:54 +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 BF1BFDD;\n\tTue, 30 Mar 2021 01:45:53 +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=\"HrClcknU\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1617061554;\n\tbh=akKRkCw/Y2w9TtO8yNucLQqgPHoypBm65lets+aqi2A=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=HrClcknUFal/vzbj4OH0YBR+XNiPR7OWRepSlj34x/nI8jzFpfFw7iH7br27r3wKc\n\tLKrpQ+QSNmsS3fmvLt87Z6/EZm89SD9lU/YXW5mkr0yrEF36plhRGI3nNtrsDrEkZs\n\tyyWM0pxkr4/KRHozLIqnV0XELRDmt9oseOeYqTC4=","Date":"Tue, 30 Mar 2021 02:45:09 +0300","From":"Laurent Pinchart ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Message-ID":"","References":"<20210310154414.3560115-1-niklas.soderlund@ragnatech.se>\n\t<20210310154414.3560115-2-niklas.soderlund@ragnatech.se>\n\t\n\t\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v3 1/2] lc-compliance: Add a libcamera\n\tcompliance tool","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","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16067,"web_url":"https://patchwork.libcamera.org/comment/16067/","msgid":"","date":"2021-03-31T06:01:18","subject":"Re: [libcamera-devel] [PATCH v3 1/2] lc-compliance: Add a libcamera\n\tcompliance tool","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Laurent,\n\nOn 2021-03-30 02:31:54 +0300, Laurent Pinchart wrote:\n> Hi Niklas,\n> \n> On Mon, Mar 29, 2021 at 06:44:51PM +0200, Niklas Söderlund wrote:\n> > Hi Laurent,\n> > \n> > Thanks for your feedback.\n> > \n> > On 2021-03-14 22:47:17 +0200, Laurent Pinchart wrote:\n> > \n> > \n> > \n> > > > diff --git a/src/lc-compliance/simple_capture.cpp \n> > > > b/src/lc-compliance/simple_capture.cpp\n> > > > new file mode 100644\n> > > > index 0000000000000000..fac8db379118efbd\n> > > > --- /dev/null\n> > > > +++ b/src/lc-compliance/simple_capture.cpp\n> > > > @@ -0,0 +1,149 @@\n> > > > +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> > > > +/*\n> > > > + * Copyright (C) 2020, Google Inc.\n> > > > + *\n> > > > + * simple_capture.cpp - Simple capture helper\n> > > > + */\n> > > > +\n> > > > +#include \"simple_capture.h\"\n> > > > +\n> > > > +using namespace libcamera;\n> > > > +\n> > > > +SimpleCapture::SimpleCapture(std::shared_ptr camera)\n> > > > +\t: camera_(camera), allocator_(std::make_unique(camera))\n> > > > +{\n> > > > +}\n> > > > +\n> > > > +SimpleCapture::~SimpleCapture()\n> > > > +{\n> > > > +}\n> > > > +\n> > > > +Results::Result 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}\n> > > > +\n> > > > +\tif (camera_->configure(config_.get())) {\n> > > > +\t\tconfig_.reset();\n> > > > +\t\treturn { Results::Fail, \"Failed to configure camera\" };\n> > > > +\t}\n> > > > +\n> > > > +\treturn { Results::Pass, \"Configure camera\" };\n> > > > +}\n> > > \n> > > Is returning a Result here the best option. Shouldn't test results be\n> > > constructed by tests, and helper classes return statuses that let tests\n> > > decide what to do ? In particular, the success message generated by this\n> > > function will never be used, as success applies to a full test, not to\n> > > an invidual step.\n> > > \n> > > On the other hand, the helper classes have the most detailed knowledge\n> > > about the errors they can encounter. I'm not sure what would be best. I\n> > > wonder if this could be a valid use case for using exceptions. It would\n> > > allow writing tests with assert-like functions. For instance, the code\n> > > below that tests\n> > > \n> > > \tif (captureCount_ != captureLimit_)\n> > > \t\treturn { Results::Fail, \"Got \" + std::to_string(captureCount_) + \" request, wanted \" + std::to_string(captureLimit_) };\n> > > \n> > > could be replaced with a\n> > > \n> > > \tfailIfNotEqual(captureCount_, captureLimit, \"request(s)\");\n> > > \n> > > which would generate the message automatically.\n> > \n> > I think this is a very interesting idea. And I thought about it when \n> > writing this but shrugged it off as i thought it would be hard to get \n> > acceptance upstream. I have not experimented with a prototype for this, \n> > but maybe this is something we can do on-top as we inclemently improve \n> > the structure of the tool.\n> \n> https://bugs.libcamera.org/show_bug.cgi?id=17\n> \n> > > > +\n> > > > +Results::Result 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> > > > +\n> > > > +\tif (camera_->start())\n> > > > +\t\treturn { Results::Fail, \"Failed to start camera\" };\n> > > > +\n> > > > +\tcamera_->requestCompleted.connect(this, &SimpleCapture::requestComplete);\n> > > > +\n> > > > +\treturn { Results::Pass, \"Started camera\" };\n> > > > +}\n> > > > +\n> > > > +Results::Result SimpleCapture::stop()\n> > > > +{\n> > > > +\tStream *stream = config_->at(0).stream();\n> > > > +\n> > > > +\tcamera_->stop();\n> > > > +\n> > > > +\tcamera_->requestCompleted.disconnect(this, &SimpleCapture::requestComplete);\n> > > > +\n> > > > +\tallocator_->free(stream);\n> > > > +\n> > > > +\treturn { Results::Pass, \"Stopped camera\" };\n> > > \n> > > Generically speaking, should this kind of function be made idempotent,\n> > > and be called from destructors ? It would make error handling easier.\n> > \n> > With an exception test flow I think it would make sens.\n> \n> https://bugs.libcamera.org/show_bug.cgi?id=18\n> \n> > > > +}\n> > > > +\n> > > > +/* SimpleCaptureBalanced */\n> > > > +\n> > > > +SimpleCaptureBalanced::SimpleCaptureBalanced(std::shared_ptr camera)\n> > > > +\t: SimpleCapture(camera)\n> > > > +{\n> > > > +}\n> > > > +\n> > > > +Results::Result SimpleCaptureBalanced::capture(unsigned int numRequests)\n> > > > +{\n> > > > +\tResults::Result ret = start();\n> > > > +\tif (ret.first != Results::Pass)\n> > > > +\t\treturn ret;\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> > > > +\t\tstop();\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> > > \n> > > This makes me wonder what the granularity of a test should be. What is\n> > > \"one test\", should it be a call to this function with a given number of\n> > > buffers, or one call to testSingleStream() ? I'm leaning towards the\n> > > later, defining a test as something that can be selected in a test plan.\n> > > I can imagine selecting testSingleStream() through a command line\n> > > argument (or through a configuration file), going for a smaller\n> > > granularity doesn't seem too useful. We need to log the status of\n> > > individual steps in a test, but at the top (and TAP) level, I'd limit it\n> > > to the higher level.\n> > \n> > I'm heavily biased by my preference for TAP and the test framework 9pm \n> > (disclaimer partly developed by me) when it comes to how to structure \n> > tests.\n> > \n> > The basic structure is a test case is a collection of TAP test points. \n> > Each case is specifies the number of test points it has and then in \n> > order lists if the test point is passed, skipped or failed. The test \n> > cases is then the worst possible aggregation of all test points. That is \n> > if one TAP is fail the whole test cases is fail. If one TAP is skip and \n> > there are no fails the whole case is skip. And only if all TAPs are pass \n> > is the test case PASS.\n> > \n> > This protects against test cases terminating prematurely as the harness \n> > knows how may TAPs to expect before the actual testing begins.\n> > \n> > Then you collect a list of test cases into a test suite and that is your \n> > top level object.\n> \n> So this means that while test cases can contain multiple test points,\n> the granularity of selecting what tests to run would be at the test case\n> level, not the test point level ?\n\nYes.\n\n> \n> > In this case the testSingleStream is a test case while it could output \n> > any number of pass/fail/skip TAP points. And the collection of \n> > testSingleStream, testRequestBalance, testRequestUnbalance would be a \n> > test suite with 3 test-cases.\n> \n> You mention that test cases need to report how many test points they\n> produce. How does this work when a test point failure is fatal and\n> prevents other test points from running ?\n\nAny test case that produces less then (or more then) the number of TAP \ntest points then it reported in its test plan is a failed test. This is \none feature of TAP I really like as it acts as a sanity check on the \ntest itself. Naturally if we go with a different test framework this may \nneed to be reconsider if the concept of plans do not exist.\n\nSide note, I'm happy that you caught this special case of test cases as \n'fatal fail' was a feature we took quiet seriously in 9pm and really got \nright in the API using the 'fun' upvar and uplevel constructs of TCL :-)\n\nhttps://github.com/rical/9pm/blob/master/output.tcl#L211\n\n> \n> > But I think the lesson learnt over time is to tailor your test structure \n> > to the tool/framework you intend to use. For example 9pm would be a \n> > horrible match for the needs of lc-compliance. I think the best way \n> > forward for us is to move forward with a our own (does not have to be \n> > this one) implementation of a test harness and once we know which \n> > environment it will be used most frequently pick an existing library and \n> > switch to that.\n> > \n> > > > +\t}\n> > > > +\n> > > > +\tqueueCount_ = 0;\n> > > > +\tcaptureCount_ = 0;\n> > > > +\tcaptureLimit_ = numRequests;\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\tif (!request) {\n> > > > +\t\t\tstop();\n> > > > +\t\t\treturn { Results::Fail, \"Can't create request\" };\n> > > > +\t\t}\n> > > > +\n> > > > +\t\tif (request->addBuffer(stream, buffer.get())) {\n> > > > +\t\t\tstop();\n> > > > +\t\t\treturn { Results::Fail, \"Can't set buffer for request\" };\n> > > > +\t\t}\n> > > > +\n> > > > +\t\tif (queueRequest(request.get()) < 0) {\n> > > > +\t\t\tstop();\n> > > > +\t\t\treturn { Results::Fail, \"Failed to queue request\" };\n> > > > +\t\t}\n> > > > +\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> > > Should we construct the event loop in the constructor of the base class,\n> > > and delete it in its destructor ?\n> > \n> > I thought about that too but ruled again it. Mostly because we had other \n> > issues with the EventLoop at the time and start/stop it did not really \n> > work as intended. Maybe we can reopen that when we look at the Exception \n> > driven testing and keep it simple and readable for now?\n> > \n> > > > +\n> > > > +\tif (captureCount_ != captureLimit_)\n> > > > +\t\treturn { Results::Fail, \"Got \" + std::to_string(captureCount_) + \" request, wanted \" + std::to_string(captureLimit_) };\n> > > \n> > > This is a very long line.\n> > \n> > I opted to keep string constructing on a single line to ease grepping in \n> > the code. You are now the n th person commenting about it so I will \n> > admit defeat and break them :-)\n> > \n> > > > +\n> > > > +\treturn { Results::Pass, \"Balanced capture of \" + std::to_string(numRequests) + \" requests\" };\n> > > > +}\n> > > > +\n> > > > +int SimpleCaptureBalanced::queueRequest(Request *request)\n> > > > +{\n> > > > +\tqueueCount_++;\n> > > > +\tif (queueCount_ > captureLimit_)\n> > > > +\t\treturn 0;\n> > > > +\n> > > > +\treturn camera_->queueRequest(request);\n> > > > +}\n> > > > +\n> > > > +void SimpleCaptureBalanced::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/lc-compliance/simple_capture.h b/src/lc-compliance/simple_capture.h\n> > > > new file mode 100644\n> > > > index 0000000000000000..3a6afc538c623050\n> > > > --- /dev/null\n> > > > +++ b/src/lc-compliance/simple_capture.h\n> > > > @@ -0,0 +1,54 @@\n> > > > +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> > > > +/*\n> > > > + * Copyright (C) 2020, Google Inc.\n> > > > + *\n> > > > + * simple_capture.h - Simple capture helper\n> > > > + */\n> > > > +#ifndef __LC_COMPLIANCE_SIMPLE_CAPTURE_H__\n> > > > +#define __LC_COMPLIANCE_SIMPLE_CAPTURE_H__\n> > > > +\n> > > > +#include \n> > > > +\n> > > > +#include \n> > > > +\n> > > > +#include \"../cam/event_loop.h\"\n> > > > +#include \"results.h\"\n> > > > +\n> > > > +class SimpleCapture\n> > > > +{\n> > > > +public:\n> > > > +\tResults::Result configure(libcamera::StreamRole role);\n> > > > +\n> > > > +protected:\n> > > > +\tSimpleCapture(std::shared_ptr camera);\n> > > \n> > > You can pass a libcamera::Camera * as a borrowed reference here, as the\n> > > caller guarantees that the reference stays valid for the lifetime of\n> > > this class.\n> > \n> > We could, I opted to use the shared_ptr to keep the use-cases as close \n> > to what simple applications would use to be able to copy/past code form \n> > this tool in future when i forgotten the API ;-)\n> \n> Applications can also use borrowed pointers :-) I think that using a\n> shared_ptr to model a reference that can be held, and a normal pointer\n> for a borrowed reference that must not be stored behind the caller's\n> back may make the code more explicit. There's also a small performance\n> difference, which doesn't matter here.\n> \n> At this point I don't think this matters much, we can revisit the code\n> later.\n> \n> > I'm not oppose to changing it but would prefers to keep it the way it \n> > is.\n> \n> -- \n> Regards,\n> \n> Laurent Pinchart","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 786E1C0DA3\n\tfor ;\n\tWed, 31 Mar 2021 06:01:22 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id CE8F46877F;\n\tWed, 31 Mar 2021 08:01:21 +0200 (CEST)","from mail-lj1-x229.google.com (mail-lj1-x229.google.com\n\t[IPv6:2a00:1450:4864:20::229])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 8E438602DB\n\tfor ;\n\tWed, 31 Mar 2021 08:01:20 +0200 (CEST)","by mail-lj1-x229.google.com with SMTP id u9so22419793ljd.11\n\tfor ;\n\tTue, 30 Mar 2021 23:01:20 -0700 (PDT)","from localhost (h-209-203.A463.priv.bahnhof.se. [155.4.209.203])\n\tby smtp.gmail.com with ESMTPSA id\n\tg8sm123645lfb.196.2021.03.30.23.01.18\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tTue, 30 Mar 2021 23:01:18 -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=\"b5XqkG5O\"; 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=leqlSCAydpSajhql079BlaRxLPc7M7bcYcKxCUEY3R0=;\n\tb=b5XqkG5O2X0lLYSzFuCdVbJCgX3hJ0ZKExo6P/HNOYyM60EwybDvNUoh9vDFJ2HTaQ\n\tqeLVDr8Hh3fY4n+6b8lffrOoqanDq1ulHQhz0OpcwQU4+/lY3WAEX3x4uCeKuKsSh94V\n\tVW3SUBo+585lV0SvNAR+ygu9jYrcf7QtOmIyTrClLrADOeIN/9MP9AVd5oJnIUITYnUZ\n\t78HJA3yRGM7MHYaY009wwPHxHi287STwOFoXbtv8lsynhUsmgn7Mn+zLSxEcAArhGegA\n\tFzEBtOiSIus08DojNkNlQMEsSy/bxe56cgJTpkDKmYt/eLwxa4oqrLWI0D57PPdi5ylU\n\tMi+g==","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=leqlSCAydpSajhql079BlaRxLPc7M7bcYcKxCUEY3R0=;\n\tb=pKUatjleQmOt8UIDaBqvagPj5cuuCFHtV58NLSzxyGnrHC5bA/PH/s/aW8rEMtsa7y\n\t8QZxv4y8vcqxvC5vxaKmPNIVDLrb3e4JM0PxUMm5N24jXpUaPzYDcZZEPsbaOYgNxODh\n\tmZOg2k89pBu465ny36O/I5foCzVwHLQUwYZ9rBRS2OA6AYhwUgsel3SmbLhCHbTS8UlF\n\tew/hI5xt/1Ws3wjfEVujDE+9WvwwiuRgrTmJei8LhpUh9uW92FWSuH3bsodPywsuRolT\n\tMK6HxdjcQy2BDrv0Pk5bVvKqF7vZ7cWKc/vImGg66h08uPqBrWdbtmwrcaiOipviUVIz\n\tBeuQ==","X-Gm-Message-State":"AOAM530OoG++3+cqa8YSaLqDhjeZLLoiqtcAUcOoDxMtBdI8CD0Q+qhd\n\tOplHGx2e9m3uiqpSomMzIRZKgQ==","X-Google-Smtp-Source":"ABdhPJyBlMZEBwmr8UFEwypALhAeCNdDqedz2dxvcGJOrkAxT6l8naA8sH5mV9QefdH/mFl/8gvnpQ==","X-Received":"by 2002:a2e:b008:: with SMTP id y8mr969281ljk.233.1617170479704; \n\tTue, 30 Mar 2021 23:01:19 -0700 (PDT)","Date":"Wed, 31 Mar 2021 08:01:18 +0200","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Laurent Pinchart ","Message-ID":"","References":"<20210310154414.3560115-1-niklas.soderlund@ragnatech.se>\n\t<20210310154414.3560115-2-niklas.soderlund@ragnatech.se>\n\t\n\t\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v3 1/2] lc-compliance: Add a libcamera\n\tcompliance tool","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","Content-Type":"text/plain; charset=\"iso-8859-1\"","Content-Transfer-Encoding":"quoted-printable","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]