| Message ID | 20210503193307.108607-4-nfraprado@collabora.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Nícolas, Thanks for your work! I really like how gtest helps make the actual tests to be easier to read. On 2021-05-03 16:33:07 -0300, Nícolas F. R. A. Prado wrote: > Refactor lc-compliance using Googletest as the test framework. > > Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com> > --- > Changes in v2: > - Changed from static to dynamic test registration > - Removed -c flag > > There's still an issue with the refactoring that the shared_ptr of the tests > apparently aren't being deleted on time, which causes: > > [20:26:08.744507935] [103243] ERROR DeviceEnumerator device_enumerator.cpp:165 Removing media device /dev/media0 while still in use > [20:26:08.744548175] [103243] ERROR DeviceEnumerator device_enumerator.cpp:165 Removing media device /dev/media1 while still in use > Segmentation fault (core dumped) > > I tried explicitly resetting the shared_ptr on the destructor of the tests but > that didn't work. In fact, when just listing the tests, the constructor for the > tests isn't even called. So I think the issue has to do with the passing of the > camera shared pointer to the REGISTER_TESTS() macro through that functor (?): > > [=]() -> testsuite* { return new testcase(c, r, rq); }); \ > > Not sure how to solve this. Any tip would be welcome. I will leave this for now as I have a different worry about REGISTER_TESTS() design below. > > src/lc-compliance/main.cpp | 99 +++++++++--------- > src/lc-compliance/meson.build | 3 + > src/lc-compliance/simple_capture.cpp | 74 +++++-------- > src/lc-compliance/simple_capture.h | 8 +- > src/lc-compliance/single_stream.cpp | 151 ++++++++++++++------------- > src/lc-compliance/tests.h | 15 ++- > 6 files changed, 175 insertions(+), 175 deletions(-) > > diff --git a/src/lc-compliance/main.cpp b/src/lc-compliance/main.cpp > index 54cee54aa978..add0d7729aec 100644 > --- a/src/lc-compliance/main.cpp > +++ b/src/lc-compliance/main.cpp > @@ -9,6 +9,8 @@ > #include <iostream> > #include <string.h> > > +#include <gtest/gtest.h> > + > #include <libcamera/libcamera.h> > > #include "../cam/options.h" > @@ -17,7 +19,6 @@ > using namespace libcamera; > > enum { > - OptCamera = 'c', > OptHelp = 'h', > }; > > @@ -28,14 +29,15 @@ public: > ~Harness(); > > int exec(); > + int init(); > + void registerTests(); > > private: > - int init(); > void listCameras(); > > OptionsParser::Options options_; > std::unique_ptr<CameraManager> cm_; > - std::shared_ptr<Camera> camera_; > + std::vector<std::shared_ptr<Camera>> cameras_; > }; > > Harness::Harness(const OptionsParser::Options &options) > @@ -46,33 +48,14 @@ Harness::Harness(const OptionsParser::Options &options) > > Harness::~Harness() > { > - if (camera_) { > - camera_->release(); > - camera_.reset(); > + for (auto &c : cameras_) { > + c->release(); > + c.reset(); > } > > cm_->stop(); > } > > -int Harness::exec() > -{ > - int ret = init(); > - if (ret) > - return ret; > - > - std::vector<Results> results; > - > - results.push_back(testSingleStream(camera_)); > - > - for (const Results &result : results) { > - ret = result.summary(); > - if (ret) > - return ret; > - } > - > - return 0; > -} > - > int Harness::init() > { > int ret = cm_->start(); > @@ -82,42 +65,26 @@ int Harness::init() > return ret; > } > > - if (!options_.isSet(OptCamera)) { > - std::cout << "No camera specified, available cameras:" << std::endl; > - listCameras(); > - return -ENODEV; > - } > - > - const std::string &cameraId = options_[OptCamera]; > - camera_ = cm_->get(cameraId); > - if (!camera_) { > - std::cout << "Camera " << cameraId << " not found, available cameras:" << std::endl; > - listCameras(); > - return -ENODEV; > + for (auto cam : cm_->cameras()) { > + if (cam->acquire()) { > + std::cout << "Failed to acquire camera" << std::endl; > + return -EINVAL; > + } > + cameras_.push_back(cam); I don't like this, I think we need to retain the ability to select which camera to test. And further down the line maybe allow more then one camera to be selected if we want to test concurrent use of two specific ones. But for the work in this series I think you can ignore multiple cameras. If I understand the coverletter correctly the correct thing here the cli arguments should be feed to gtest directly to be able to filter on what tests are run. Is there support in getst to also pass arguments to tests? Maybe we could remove the need to parse arguments ourself completely? > } > > - if (camera_->acquire()) { > - std::cout << "Failed to acquire camera" << std::endl; > - return -EINVAL; > - } > - > - std::cout << "Using camera " << cameraId << std::endl; > - > return 0; > } > > void Harness::listCameras() > { > - for (const std::shared_ptr<Camera> &cam : cm_->cameras()) > - std::cout << "- " << cam.get()->id() << std::endl; > + for (const std::shared_ptr<Camera> &c : cm_->cameras()) > + std::cout << "- " << c.get()->id() << std::endl; > } > > static int parseOptions(int argc, char **argv, OptionsParser::Options *options) > { > OptionsParser parser; > - parser.addOption(OptCamera, OptionString, > - "Specify which camera to operate on, by id", "camera", > - ArgumentRequired, "camera"); > parser.addOption(OptHelp, OptionNone, "Display this help message", > "help"); > > @@ -133,6 +100,31 @@ static int parseOptions(int argc, char **argv, OptionsParser::Options *options) > return 0; > } > > +/* > + * Make asserts act like exceptions, otherwise they only fail (or skip) the > + * current function. From gtest documentation: > + * https://google.github.io/googletest/advanced.html#asserting-on-subroutines-with-an-exception > + */ > +class ThrowListener : public testing::EmptyTestEventListener { > + void OnTestPartResult(const testing::TestPartResult& result) override { > + if (result.type() == testing::TestPartResult::kFatalFailure > + || result.type() == testing::TestPartResult::kSkip) { > + throw testing::AssertionException(result); > + } > + } > +}; > + > + > +void Harness::registerTests() { > + std::map<StreamRole, std::string> roles = {{Raw, "Raw"}, > + {StillCapture, "Still"}, > + {VideoRecording, "Video"}, > + {Viewfinder, "Viewfinder"}}; > + std::vector<int> requests = {1, 2, 3, 5, 8, 13, 21, 34, 55, 89}; > + > + registerSingleStreamTests(cameras_, roles, requests); > +} > + > int main(int argc, char **argv) > { > OptionsParser::Options options; > @@ -143,6 +135,13 @@ int main(int argc, char **argv) > return EXIT_FAILURE; > > Harness harness(options); > + ret = harness.init(); > + if (ret) > + return ret; > + > + harness.registerTests(); > > - return harness.exec() ? EXIT_FAILURE : EXIT_SUCCESS; > + ::testing::InitGoogleTest(&argc, argv); > + testing::UnitTest::GetInstance()->listeners().Append(new ThrowListener); > + return RUN_ALL_TESTS(); > } > diff --git a/src/lc-compliance/meson.build b/src/lc-compliance/meson.build > index a2bfcceb1259..704bc18af3e1 100644 > --- a/src/lc-compliance/meson.build > +++ b/src/lc-compliance/meson.build > @@ -18,10 +18,13 @@ lc_compliance_sources = files([ > 'single_stream.cpp', > ]) > > +gtest_dep = dependency('gtest') > + > lc_compliance = executable('lc-compliance', lc_compliance_sources, > dependencies : [ > libatomic, > libcamera_dep, > libevent, > + gtest_dep, > ], > install : true) > diff --git a/src/lc-compliance/simple_capture.cpp b/src/lc-compliance/simple_capture.cpp > index f90fe6d0f9aa..7731eb16f8c2 100644 > --- a/src/lc-compliance/simple_capture.cpp > +++ b/src/lc-compliance/simple_capture.cpp > @@ -5,6 +5,8 @@ > * simple_capture.cpp - Simple capture helper > */ > > +#include <gtest/gtest.h> > + > #include "simple_capture.h" > > using namespace libcamera; > @@ -20,38 +22,34 @@ SimpleCapture::~SimpleCapture() > stop(); > } > > -Results::Result SimpleCapture::configure(StreamRole role) > +void SimpleCapture::configure(StreamRole role) > { > config_ = camera_->generateConfiguration({ role }); > > - if (!config_) > - return { Results::Skip, "Role not supported by camera" }; > + if (!config_) { > + std::cout << "Role not supported by camera" << std::endl; > + GTEST_SKIP(); > + } > > if (config_->validate() != CameraConfiguration::Valid) { > config_.reset(); > - return { Results::Fail, "Configuration not valid" }; > + FAIL() << "Configuration not valid"; > } > > if (camera_->configure(config_.get())) { > config_.reset(); > - return { Results::Fail, "Failed to configure camera" }; > + FAIL() << "Failed to configure camera"; > } > - > - return { Results::Pass, "Configure camera" }; > } > > -Results::Result SimpleCapture::start() > +void SimpleCapture::start() > { > Stream *stream = config_->at(0).stream(); > - if (allocator_->allocate(stream) < 0) > - return { Results::Fail, "Failed to allocate buffers" }; > + ASSERT_GE(allocator_->allocate(stream), 0) << "Failed to allocate buffers"; > > - if (camera_->start()) > - return { Results::Fail, "Failed to start camera" }; > + ASSERT_TRUE(!camera_->start()) << "Failed to start camera"; > > camera_->requestCompleted.connect(this, &SimpleCapture::requestComplete); > - > - return { Results::Pass, "Started camera" }; > } > > void SimpleCapture::stop() > @@ -77,22 +75,19 @@ SimpleCaptureBalanced::SimpleCaptureBalanced(std::shared_ptr<Camera> camera) > { > } > > -Results::Result SimpleCaptureBalanced::capture(unsigned int numRequests) > +void SimpleCaptureBalanced::capture(unsigned int numRequests) > { > - Results::Result ret = start(); > - if (ret.first != Results::Pass) > - return ret; > + start(); > > Stream *stream = config_->at(0).stream(); > const std::vector<std::unique_ptr<FrameBuffer>> &buffers = allocator_->buffers(stream); > > /* No point in testing less requests then the camera depth. */ > if (buffers.size() > numRequests) { > - /* Cache buffers.size() before we destroy it in stop() */ > - int buffers_size = buffers.size(); > - > - return { Results::Skip, "Camera needs " + std::to_string(buffers_size) > - + " requests, can't test only " + std::to_string(numRequests) }; > + std::cout << "Camera needs " + std::to_string(buffers.size()) > + + " requests, can't test only " > + + std::to_string(numRequests) << std::endl; > + GTEST_SKIP(); > } > > queueCount_ = 0; > @@ -103,14 +98,11 @@ Results::Result SimpleCaptureBalanced::capture(unsigned int numRequests) > std::vector<std::unique_ptr<libcamera::Request>> requests; > for (const std::unique_ptr<FrameBuffer> &buffer : buffers) { > std::unique_ptr<Request> request = camera_->createRequest(); > - if (!request) > - return { Results::Fail, "Can't create request" }; > + ASSERT_TRUE(request) << "Can't create request"; > > - if (request->addBuffer(stream, buffer.get())) > - return { Results::Fail, "Can't set buffer for request" }; > + ASSERT_FALSE(request->addBuffer(stream, buffer.get())) << "Can't set buffer for request"; > > - if (queueRequest(request.get()) < 0) > - return { Results::Fail, "Failed to queue request" }; > + ASSERT_GE(queueRequest(request.get()), 0) << "Failed to queue request"; > > requests.push_back(std::move(request)); > } > @@ -121,12 +113,7 @@ Results::Result SimpleCaptureBalanced::capture(unsigned int numRequests) > stop(); > delete loop_; > > - if (captureCount_ != captureLimit_) > - return { Results::Fail, "Got " + std::to_string(captureCount_) + > - " request, wanted " + std::to_string(captureLimit_) }; > - > - return { Results::Pass, "Balanced capture of " + > - std::to_string(numRequests) + " requests" }; > + ASSERT_EQ(captureCount_, captureLimit_); > } > > int SimpleCaptureBalanced::queueRequest(Request *request) > @@ -158,11 +145,9 @@ SimpleCaptureUnbalanced::SimpleCaptureUnbalanced(std::shared_ptr<Camera> camera) > { > } > > -Results::Result SimpleCaptureUnbalanced::capture(unsigned int numRequests) > +void SimpleCaptureUnbalanced::capture(unsigned int numRequests) > { > - Results::Result ret = start(); > - if (ret.first != Results::Pass) > - return ret; > + start(); > > Stream *stream = config_->at(0).stream(); > const std::vector<std::unique_ptr<FrameBuffer>> &buffers = allocator_->buffers(stream); > @@ -174,14 +159,11 @@ Results::Result SimpleCaptureUnbalanced::capture(unsigned int numRequests) > std::vector<std::unique_ptr<libcamera::Request>> requests; > for (const std::unique_ptr<FrameBuffer> &buffer : buffers) { > std::unique_ptr<Request> request = camera_->createRequest(); > - if (!request) > - return { Results::Fail, "Can't create request" }; > + ASSERT_TRUE(request) << "Can't create request"; > > - if (request->addBuffer(stream, buffer.get())) > - return { Results::Fail, "Can't set buffer for request" }; > + ASSERT_FALSE(request->addBuffer(stream, buffer.get())) << "Can't set buffer for request"; > > - if (camera_->queueRequest(request.get()) < 0) > - return { Results::Fail, "Failed to queue request" }; > + ASSERT_GE(camera_->queueRequest(request.get()), 0) << "Failed to queue request"; > > requests.push_back(std::move(request)); > } > @@ -192,7 +174,7 @@ Results::Result SimpleCaptureUnbalanced::capture(unsigned int numRequests) > stop(); > delete loop_; > > - return { status ? Results::Fail : Results::Pass, "Unbalanced capture of " + std::to_string(numRequests) + " requests" }; > + ASSERT_FALSE(status); > } > > void SimpleCaptureUnbalanced::requestComplete(Request *request) > diff --git a/src/lc-compliance/simple_capture.h b/src/lc-compliance/simple_capture.h > index d9de53fb63a3..0f8465083456 100644 > --- a/src/lc-compliance/simple_capture.h > +++ b/src/lc-compliance/simple_capture.h > @@ -17,13 +17,13 @@ > class SimpleCapture > { > public: > - Results::Result configure(libcamera::StreamRole role); > + void configure(libcamera::StreamRole role); > > protected: > SimpleCapture(std::shared_ptr<libcamera::Camera> camera); > virtual ~SimpleCapture(); > > - Results::Result start(); > + void start(); > void stop(); > > virtual void requestComplete(libcamera::Request *request) = 0; > @@ -40,7 +40,7 @@ class SimpleCaptureBalanced : public SimpleCapture > public: > SimpleCaptureBalanced(std::shared_ptr<libcamera::Camera> camera); > > - Results::Result capture(unsigned int numRequests); > + void capture(unsigned int numRequests); > > private: > int queueRequest(libcamera::Request *request); > @@ -56,7 +56,7 @@ class SimpleCaptureUnbalanced : public SimpleCapture > public: > SimpleCaptureUnbalanced(std::shared_ptr<libcamera::Camera> camera); > > - Results::Result capture(unsigned int numRequests); > + void capture(unsigned int numRequests); > > private: > void requestComplete(libcamera::Request *request) override; > diff --git a/src/lc-compliance/single_stream.cpp b/src/lc-compliance/single_stream.cpp > index 8318b42f42d6..eb6a6f305826 100644 > --- a/src/lc-compliance/single_stream.cpp > +++ b/src/lc-compliance/single_stream.cpp > @@ -7,91 +7,94 @@ > > #include <iostream> > > +#include <gtest/gtest.h> > + > #include "simple_capture.h" > #include "tests.h" > > using namespace libcamera; > > -Results::Result testRequestBalance(std::shared_ptr<Camera> camera, > - StreamRole role, unsigned int startCycles, > - unsigned int numRequests) > -{ > - SimpleCaptureBalanced capture(camera); > +class SingleStream : public testing::Test { > +public: > + explicit SingleStream(std::shared_ptr<Camera> camera, StreamRole role, unsigned int numRequests) > + : camera_(camera), role_(role), numRequests_(numRequests) {} > > - Results::Result ret = capture.configure(role); > - if (ret.first != Results::Pass) > - return ret; > +protected: > + std::shared_ptr<Camera> camera_; > + StreamRole role_; > + unsigned int numRequests_; > +}; > > - for (unsigned int starts = 0; starts < startCycles; starts++) { > - ret = capture.capture(numRequests); > - if (ret.first != Results::Pass) > - return ret; > - } > +/* > + * Test single capture cycles > + * > + * Makes sure the camera completes the exact number of requests queued. Example > + * failure is a camera that needs N+M requests queued to complete N requests to > + * the application. > + */ > +class BalancedSingleCycle : public SingleStream { > +public: > + explicit BalancedSingleCycle(std::shared_ptr<Camera> camera, StreamRole role, unsigned int numRequests) > + : SingleStream(camera, role, numRequests) {} > > - return { Results::Pass, "Balanced capture of " + > - std::to_string(numRequests) + " requests with " + > - std::to_string(startCycles) + " start cycles" }; > -} > + void TestBody() override { > + SimpleCaptureBalanced capture(camera_); > > -Results::Result testRequestUnbalance(std::shared_ptr<Camera> camera, > - StreamRole role, unsigned int numRequests) > -{ > - SimpleCaptureUnbalanced capture(camera); > + capture.configure(role_); > > - Results::Result ret = capture.configure(role); > - if (ret.first != Results::Pass) > - return ret; > + capture.capture(numRequests_); > + }; > +}; > > - return capture.capture(numRequests); > -} > +/* > + * Test multiple start/stop cycles > + * > + * Makes sure the camera supports multiple start/stop cycles. Example failure is > + * a camera that does not clean up correctly in its error path but is only > + * tested by single-capture applications. > + */ > +class BalancedMultiCycle : public SingleStream { > +public: > + explicit BalancedMultiCycle(std::shared_ptr<Camera> camera, StreamRole role, unsigned int numRequests) > + : SingleStream(camera, role, numRequests) {} > > -Results testSingleStream(std::shared_ptr<Camera> camera) > -{ > - static const std::vector<std::pair<std::string, StreamRole>> roles = { > - { "raw", Raw }, > - { "still", StillCapture }, > - { "video", VideoRecording }, > - { "viewfinder", Viewfinder }, > + void TestBody() override { > + unsigned int numRepeats = 3; > + > + SimpleCaptureBalanced capture(camera_); > + > + capture.configure(role_); > + > + for (unsigned int starts = 0; starts < numRepeats; starts++) > + capture.capture(numRequests_); > }; > - static const std::vector<unsigned int> numRequests = { 1, 2, 3, 5, 8, 13, 21, 34, 55, 89 }; > - > - Results results(numRequests.size() * roles.size() * 3); > - > - for (const auto &role : roles) { > - std::cout << "= Test role " << role.first << std::endl; > - /* > - * Test single capture cycles > - * > - * Makes sure the camera completes the exact number of requests queued. > - * Example failure is a camera that needs N+M requests queued to > - * complete N requests to the application. > - */ > - std::cout << "* Test single capture cycles" << std::endl; > - for (unsigned int num : numRequests) > - results.add(testRequestBalance(camera, role.second, 1, num)); > - > - /* > - * Test multiple start/stop cycles > - * > - * Makes sure the camera supports multiple start/stop cycles. > - * Example failure is a camera that does not clean up correctly in its > - * error path but is only tested by single-capture applications. > - */ > - std::cout << "* Test multiple start/stop cycles" << std::endl; > - for (unsigned int num : numRequests) > - results.add(testRequestBalance(camera, role.second, 3, num)); > - > - /* > - * Test unbalanced stop > - * > - * Makes sure the camera supports a stop with requests queued. > - * Example failure is a camera that does not handle cancelation > - * of buffers coming back from the video device while stopping. > - */ > - std::cout << "* Test unbalanced stop" << std::endl; > - for (unsigned int num : numRequests) > - results.add(testRequestUnbalance(camera, role.second, num)); > - } > - > - return results; > +}; > + > +/* > + * Test unbalanced stop > + * > + * Makes sure the camera supports a stop with requests queued. Example failure > + * is a camera that does not handle cancelation of buffers coming back from the > + * video device while stopping. > + */ > +class Unbalanced : public SingleStream { > +public: > + explicit Unbalanced(std::shared_ptr<Camera> camera, StreamRole role, unsigned int numRequests) > + : SingleStream(camera, role, numRequests) {} > + > + void TestBody() override { > + SimpleCaptureUnbalanced capture(camera_); > + > + capture.configure(role_); > + > + capture.capture(numRequests_); > + }; > +}; > + > +void registerSingleStreamTests(std::vector<std::shared_ptr<Camera>> cameras, > + std::map<StreamRole, std::string> roles, std::vector<int> requests) > +{ > + REGISTER_TESTS(SingleStream, BalancedSingleCycle, cameras, roles, requests); > + REGISTER_TESTS(SingleStream, BalancedMultiCycle, cameras, roles, requests); > + REGISTER_TESTS(SingleStream, Unbalanced, cameras, roles, requests); > } > diff --git a/src/lc-compliance/tests.h b/src/lc-compliance/tests.h > index 396605214e4b..6d5a8b88c287 100644 > --- a/src/lc-compliance/tests.h > +++ b/src/lc-compliance/tests.h > @@ -11,6 +11,19 @@ > > #include "results.h" > > -Results testSingleStream(std::shared_ptr<libcamera::Camera> camera); > +void registerSingleStreamTests(std::vector<std::shared_ptr<libcamera::Camera>> cameras, > + std::map<libcamera::StreamRole, std::string> roles, std::vector<int> requests); > + > +#define REGISTER_TESTS(testsuite, testcase, cameras, roles, requests) \ > +for (auto c : cameras) { \ > + for (auto [r, rn] : roles) { \ > + for (auto rq : requests) { \ > + testing::RegisterTest( \ > + #testsuite, (std::string(#testcase) + "/" + c->id() + "/" + rn + "/" + std::to_string(rq)).c_str(), \ > + nullptr, nullptr, __FILE__, __LINE__, \ > + [=]() -> testsuite* { return new testcase(c, r, rq); }); \ > + } \ > + } \ > +} > > #endif /* __LC_COMPLIANCE_TESTS_H__ */ > -- > 2.31.1 >
Hi Niklas, thanks for the feedback. Em 2021-05-06 05:48, Niklas Söderlund escreveu: > Hi Nícolas, > > Thanks for your work! > > I really like how gtest helps make the actual tests to be easier to > read. > > On 2021-05-03 16:33:07 -0300, Nícolas F. R. A. Prado wrote: > > Refactor lc-compliance using Googletest as the test framework. > > > > Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com> > > --- > > Changes in v2: > > - Changed from static to dynamic test registration > > - Removed -c flag > > > > There's still an issue with the refactoring that the shared_ptr of the tests > > apparently aren't being deleted on time, which causes: > > > > [20:26:08.744507935] [103243] ERROR DeviceEnumerator device_enumerator.cpp:165 Removing media device /dev/media0 while still in use > > [20:26:08.744548175] [103243] ERROR DeviceEnumerator device_enumerator.cpp:165 Removing media device /dev/media1 while still in use > > Segmentation fault (core dumped) > > > > I tried explicitly resetting the shared_ptr on the destructor of the tests but > > that didn't work. In fact, when just listing the tests, the constructor for the > > tests isn't even called. So I think the issue has to do with the passing of the > > camera shared pointer to the REGISTER_TESTS() macro through that functor (?): > > > > [=]() -> testsuite* { return new testcase(c, r, rq); }); \ > > > > Not sure how to solve this. Any tip would be welcome. > > I will leave this for now as I have a different worry about > REGISTER_TESTS() design below. > > > > > src/lc-compliance/main.cpp | 99 +++++++++--------- > > src/lc-compliance/meson.build | 3 + > > src/lc-compliance/simple_capture.cpp | 74 +++++-------- > > src/lc-compliance/simple_capture.h | 8 +- > > src/lc-compliance/single_stream.cpp | 151 ++++++++++++++------------- > > src/lc-compliance/tests.h | 15 ++- > > 6 files changed, 175 insertions(+), 175 deletions(-) > > > > diff --git a/src/lc-compliance/main.cpp b/src/lc-compliance/main.cpp > > index 54cee54aa978..add0d7729aec 100644 > > --- a/src/lc-compliance/main.cpp > > +++ b/src/lc-compliance/main.cpp > > @@ -9,6 +9,8 @@ > > #include <iostream> > > #include <string.h> > > > > +#include <gtest/gtest.h> > > + > > #include <libcamera/libcamera.h> > > > > #include "../cam/options.h" > > @@ -17,7 +19,6 @@ > > using namespace libcamera; > > > > enum { > > - OptCamera = 'c', > > OptHelp = 'h', > > }; > > > > @@ -28,14 +29,15 @@ public: > > ~Harness(); > > > > int exec(); > > + int init(); > > + void registerTests(); > > > > private: > > - int init(); > > void listCameras(); > > > > OptionsParser::Options options_; > > std::unique_ptr<CameraManager> cm_; > > - std::shared_ptr<Camera> camera_; > > + std::vector<std::shared_ptr<Camera>> cameras_; > > }; > > > > Harness::Harness(const OptionsParser::Options &options) > > @@ -46,33 +48,14 @@ Harness::Harness(const OptionsParser::Options &options) > > > > Harness::~Harness() > > { > > - if (camera_) { > > - camera_->release(); > > - camera_.reset(); > > + for (auto &c : cameras_) { > > + c->release(); > > + c.reset(); > > } > > > > cm_->stop(); > > } > > > > -int Harness::exec() > > -{ > > - int ret = init(); > > - if (ret) > > - return ret; > > - > > - std::vector<Results> results; > > - > > - results.push_back(testSingleStream(camera_)); > > - > > - for (const Results &result : results) { > > - ret = result.summary(); > > - if (ret) > > - return ret; > > - } > > - > > - return 0; > > -} > > - > > int Harness::init() > > { > > int ret = cm_->start(); > > @@ -82,42 +65,26 @@ int Harness::init() > > return ret; > > } > > > > - if (!options_.isSet(OptCamera)) { > > - std::cout << "No camera specified, available cameras:" << std::endl; > > - listCameras(); > > - return -ENODEV; > > - } > > - > > - const std::string &cameraId = options_[OptCamera]; > > - camera_ = cm_->get(cameraId); > > - if (!camera_) { > > - std::cout << "Camera " << cameraId << " not found, available cameras:" << std::endl; > > - listCameras(); > > - return -ENODEV; > > + for (auto cam : cm_->cameras()) { > > + if (cam->acquire()) { > > + std::cout << "Failed to acquire camera" << std::endl; > > + return -EINVAL; > > + } > > + cameras_.push_back(cam); > > I don't like this, I think we need to retain the ability to select which > camera to test. And further down the line maybe allow more then one > camera to be selected if we want to test concurrent use of two specific > ones. But for the work in this series I think you can ignore multiple > cameras. Actually, that doesn't mean that multiple cameras will be tested, that's determined by the test filter, so doing --gtest_filter='.*idCamera1.*' would only test that camera. But I think I see what you mean, I shouldn't be acquiring all cameras, only the ones that I will actually test (and in this code there's a big issue where if a single camera is already busy, even if it won't be tested, lc-compliance as a whole fails). > > If I understand the coverletter correctly the correct thing here the cli > arguments should be feed to gtest directly to be able to filter on what > tests are run. Is there support in getst to also pass arguments to > tests? Maybe we could remove the need to parse arguments ourself > completely? There isn't any CLI argument in gtest to pass arguments to tests, but I think we could solve both problems (this and the one above) like this: - In the main(), query the id of each camera from libcamera. - Register each of the tests dynamically in gtest passing as an argument the camera id, rather than a camera pointer (which is the current behavior). - To select which tests to run, and on which camera(s), the user just uses --gtest_filter='testToRun.*idCamera' (or '.*idCamera.*' to run all tests for the camera). - In the startup of each test the camera is acquired for that test, and when it's done it is released. Implications: - We no longer need to parse arguments and can drop that code, as both camera and test are specified directly with gtest's filter. - We only acquire the cameras that we'll actually test and only while they're being tested. - As a downside, a single camera will probably be acquired multiple times during a single run as lc-compliance moves from one test to the next, but I don't think this would be a big issue. (Maybe we could do this at the test suite level instead to greatly decrease the number of times of acquire(), but I'm not sure if it's possible, since we instantiate, by passing the camera id, at the test level, I'll have to test that). What do you think? Thanks, Nícolas > > > } > > > > - if (camera_->acquire()) { > > - std::cout << "Failed to acquire camera" << std::endl; > > - return -EINVAL; > > - } > > - > > - std::cout << "Using camera " << cameraId << std::endl; > > - > > return 0; > > } > > > > void Harness::listCameras() > > { > > - for (const std::shared_ptr<Camera> &cam : cm_->cameras()) > > - std::cout << "- " << cam.get()->id() << std::endl; > > + for (const std::shared_ptr<Camera> &c : cm_->cameras()) > > + std::cout << "- " << c.get()->id() << std::endl; > > } > > > > static int parseOptions(int argc, char **argv, OptionsParser::Options *options) > > { > > OptionsParser parser; > > - parser.addOption(OptCamera, OptionString, > > - "Specify which camera to operate on, by id", "camera", > > - ArgumentRequired, "camera"); > > parser.addOption(OptHelp, OptionNone, "Display this help message", > > "help"); > > > > @@ -133,6 +100,31 @@ static int parseOptions(int argc, char **argv, OptionsParser::Options *options) > > return 0; > > } > > > > +/* > > + * Make asserts act like exceptions, otherwise they only fail (or skip) the > > + * current function. From gtest documentation: > > + * https://google.github.io/googletest/advanced.html#asserting-on-subroutines-with-an-exception > > + */ > > +class ThrowListener : public testing::EmptyTestEventListener { > > + void OnTestPartResult(const testing::TestPartResult& result) override { > > + if (result.type() == testing::TestPartResult::kFatalFailure > > + || result.type() == testing::TestPartResult::kSkip) { > > + throw testing::AssertionException(result); > > + } > > + } > > +}; > > + > > + > > +void Harness::registerTests() { > > + std::map<StreamRole, std::string> roles = {{Raw, "Raw"}, > > + {StillCapture, "Still"}, > > + {VideoRecording, "Video"}, > > + {Viewfinder, "Viewfinder"}}; > > + std::vector<int> requests = {1, 2, 3, 5, 8, 13, 21, 34, 55, 89}; > > + > > + registerSingleStreamTests(cameras_, roles, requests); > > +} > > + > > int main(int argc, char **argv) > > { > > OptionsParser::Options options; > > @@ -143,6 +135,13 @@ int main(int argc, char **argv) > > return EXIT_FAILURE; > > > > Harness harness(options); > > + ret = harness.init(); > > + if (ret) > > + return ret; > > + > > + harness.registerTests(); > > > > - return harness.exec() ? EXIT_FAILURE : EXIT_SUCCESS; > > + ::testing::InitGoogleTest(&argc, argv); > > + testing::UnitTest::GetInstance()->listeners().Append(new ThrowListener); > > + return RUN_ALL_TESTS(); > > } > > diff --git a/src/lc-compliance/meson.build b/src/lc-compliance/meson.build > > index a2bfcceb1259..704bc18af3e1 100644 > > --- a/src/lc-compliance/meson.build > > +++ b/src/lc-compliance/meson.build > > @@ -18,10 +18,13 @@ lc_compliance_sources = files([ > > 'single_stream.cpp', > > ]) > > > > +gtest_dep = dependency('gtest') > > + > > lc_compliance = executable('lc-compliance', lc_compliance_sources, > > dependencies : [ > > libatomic, > > libcamera_dep, > > libevent, > > + gtest_dep, > > ], > > install : true) > > diff --git a/src/lc-compliance/simple_capture.cpp b/src/lc-compliance/simple_capture.cpp > > index f90fe6d0f9aa..7731eb16f8c2 100644 > > --- a/src/lc-compliance/simple_capture.cpp > > +++ b/src/lc-compliance/simple_capture.cpp > > @@ -5,6 +5,8 @@ > > * simple_capture.cpp - Simple capture helper > > */ > > > > +#include <gtest/gtest.h> > > + > > #include "simple_capture.h" > > > > using namespace libcamera; > > @@ -20,38 +22,34 @@ SimpleCapture::~SimpleCapture() > > stop(); > > } > > > > -Results::Result SimpleCapture::configure(StreamRole role) > > +void SimpleCapture::configure(StreamRole role) > > { > > config_ = camera_->generateConfiguration({ role }); > > > > - if (!config_) > > - return { Results::Skip, "Role not supported by camera" }; > > + if (!config_) { > > + std::cout << "Role not supported by camera" << std::endl; > > + GTEST_SKIP(); > > + } > > > > if (config_->validate() != CameraConfiguration::Valid) { > > config_.reset(); > > - return { Results::Fail, "Configuration not valid" }; > > + FAIL() << "Configuration not valid"; > > } > > > > if (camera_->configure(config_.get())) { > > config_.reset(); > > - return { Results::Fail, "Failed to configure camera" }; > > + FAIL() << "Failed to configure camera"; > > } > > - > > - return { Results::Pass, "Configure camera" }; > > } > > > > -Results::Result SimpleCapture::start() > > +void SimpleCapture::start() > > { > > Stream *stream = config_->at(0).stream(); > > - if (allocator_->allocate(stream) < 0) > > - return { Results::Fail, "Failed to allocate buffers" }; > > + ASSERT_GE(allocator_->allocate(stream), 0) << "Failed to allocate buffers"; > > > > - if (camera_->start()) > > - return { Results::Fail, "Failed to start camera" }; > > + ASSERT_TRUE(!camera_->start()) << "Failed to start camera"; > > > > camera_->requestCompleted.connect(this, &SimpleCapture::requestComplete); > > - > > - return { Results::Pass, "Started camera" }; > > } > > > > void SimpleCapture::stop() > > @@ -77,22 +75,19 @@ SimpleCaptureBalanced::SimpleCaptureBalanced(std::shared_ptr<Camera> camera) > > { > > } > > > > -Results::Result SimpleCaptureBalanced::capture(unsigned int numRequests) > > +void SimpleCaptureBalanced::capture(unsigned int numRequests) > > { > > - Results::Result ret = start(); > > - if (ret.first != Results::Pass) > > - return ret; > > + start(); > > > > Stream *stream = config_->at(0).stream(); > > const std::vector<std::unique_ptr<FrameBuffer>> &buffers = allocator_->buffers(stream); > > > > /* No point in testing less requests then the camera depth. */ > > if (buffers.size() > numRequests) { > > - /* Cache buffers.size() before we destroy it in stop() */ > > - int buffers_size = buffers.size(); > > - > > - return { Results::Skip, "Camera needs " + std::to_string(buffers_size) > > - + " requests, can't test only " + std::to_string(numRequests) }; > > + std::cout << "Camera needs " + std::to_string(buffers.size()) > > + + " requests, can't test only " > > + + std::to_string(numRequests) << std::endl; > > + GTEST_SKIP(); > > } > > > > queueCount_ = 0; > > @@ -103,14 +98,11 @@ Results::Result SimpleCaptureBalanced::capture(unsigned int numRequests) > > std::vector<std::unique_ptr<libcamera::Request>> requests; > > for (const std::unique_ptr<FrameBuffer> &buffer : buffers) { > > std::unique_ptr<Request> request = camera_->createRequest(); > > - if (!request) > > - return { Results::Fail, "Can't create request" }; > > + ASSERT_TRUE(request) << "Can't create request"; > > > > - if (request->addBuffer(stream, buffer.get())) > > - return { Results::Fail, "Can't set buffer for request" }; > > + ASSERT_FALSE(request->addBuffer(stream, buffer.get())) << "Can't set buffer for request"; > > > > - if (queueRequest(request.get()) < 0) > > - return { Results::Fail, "Failed to queue request" }; > > + ASSERT_GE(queueRequest(request.get()), 0) << "Failed to queue request"; > > > > requests.push_back(std::move(request)); > > } > > @@ -121,12 +113,7 @@ Results::Result SimpleCaptureBalanced::capture(unsigned int numRequests) > > stop(); > > delete loop_; > > > > - if (captureCount_ != captureLimit_) > > - return { Results::Fail, "Got " + std::to_string(captureCount_) + > > - " request, wanted " + std::to_string(captureLimit_) }; > > - > > - return { Results::Pass, "Balanced capture of " + > > - std::to_string(numRequests) + " requests" }; > > + ASSERT_EQ(captureCount_, captureLimit_); > > } > > > > int SimpleCaptureBalanced::queueRequest(Request *request) > > @@ -158,11 +145,9 @@ SimpleCaptureUnbalanced::SimpleCaptureUnbalanced(std::shared_ptr<Camera> camera) > > { > > } > > > > -Results::Result SimpleCaptureUnbalanced::capture(unsigned int numRequests) > > +void SimpleCaptureUnbalanced::capture(unsigned int numRequests) > > { > > - Results::Result ret = start(); > > - if (ret.first != Results::Pass) > > - return ret; > > + start(); > > > > Stream *stream = config_->at(0).stream(); > > const std::vector<std::unique_ptr<FrameBuffer>> &buffers = allocator_->buffers(stream); > > @@ -174,14 +159,11 @@ Results::Result SimpleCaptureUnbalanced::capture(unsigned int numRequests) > > std::vector<std::unique_ptr<libcamera::Request>> requests; > > for (const std::unique_ptr<FrameBuffer> &buffer : buffers) { > > std::unique_ptr<Request> request = camera_->createRequest(); > > - if (!request) > > - return { Results::Fail, "Can't create request" }; > > + ASSERT_TRUE(request) << "Can't create request"; > > > > - if (request->addBuffer(stream, buffer.get())) > > - return { Results::Fail, "Can't set buffer for request" }; > > + ASSERT_FALSE(request->addBuffer(stream, buffer.get())) << "Can't set buffer for request"; > > > > - if (camera_->queueRequest(request.get()) < 0) > > - return { Results::Fail, "Failed to queue request" }; > > + ASSERT_GE(camera_->queueRequest(request.get()), 0) << "Failed to queue request"; > > > > requests.push_back(std::move(request)); > > } > > @@ -192,7 +174,7 @@ Results::Result SimpleCaptureUnbalanced::capture(unsigned int numRequests) > > stop(); > > delete loop_; > > > > - return { status ? Results::Fail : Results::Pass, "Unbalanced capture of " + std::to_string(numRequests) + " requests" }; > > + ASSERT_FALSE(status); > > } > > > > void SimpleCaptureUnbalanced::requestComplete(Request *request) > > diff --git a/src/lc-compliance/simple_capture.h b/src/lc-compliance/simple_capture.h > > index d9de53fb63a3..0f8465083456 100644 > > --- a/src/lc-compliance/simple_capture.h > > +++ b/src/lc-compliance/simple_capture.h > > @@ -17,13 +17,13 @@ > > class SimpleCapture > > { > > public: > > - Results::Result configure(libcamera::StreamRole role); > > + void configure(libcamera::StreamRole role); > > > > protected: > > SimpleCapture(std::shared_ptr<libcamera::Camera> camera); > > virtual ~SimpleCapture(); > > > > - Results::Result start(); > > + void start(); > > void stop(); > > > > virtual void requestComplete(libcamera::Request *request) = 0; > > @@ -40,7 +40,7 @@ class SimpleCaptureBalanced : public SimpleCapture > > public: > > SimpleCaptureBalanced(std::shared_ptr<libcamera::Camera> camera); > > > > - Results::Result capture(unsigned int numRequests); > > + void capture(unsigned int numRequests); > > > > private: > > int queueRequest(libcamera::Request *request); > > @@ -56,7 +56,7 @@ class SimpleCaptureUnbalanced : public SimpleCapture > > public: > > SimpleCaptureUnbalanced(std::shared_ptr<libcamera::Camera> camera); > > > > - Results::Result capture(unsigned int numRequests); > > + void capture(unsigned int numRequests); > > > > private: > > void requestComplete(libcamera::Request *request) override; > > diff --git a/src/lc-compliance/single_stream.cpp b/src/lc-compliance/single_stream.cpp > > index 8318b42f42d6..eb6a6f305826 100644 > > --- a/src/lc-compliance/single_stream.cpp > > +++ b/src/lc-compliance/single_stream.cpp > > @@ -7,91 +7,94 @@ > > > > #include <iostream> > > > > +#include <gtest/gtest.h> > > + > > #include "simple_capture.h" > > #include "tests.h" > > > > using namespace libcamera; > > > > -Results::Result testRequestBalance(std::shared_ptr<Camera> camera, > > - StreamRole role, unsigned int startCycles, > > - unsigned int numRequests) > > -{ > > - SimpleCaptureBalanced capture(camera); > > +class SingleStream : public testing::Test { > > +public: > > + explicit SingleStream(std::shared_ptr<Camera> camera, StreamRole role, unsigned int numRequests) > > + : camera_(camera), role_(role), numRequests_(numRequests) {} > > > > - Results::Result ret = capture.configure(role); > > - if (ret.first != Results::Pass) > > - return ret; > > +protected: > > + std::shared_ptr<Camera> camera_; > > + StreamRole role_; > > + unsigned int numRequests_; > > +}; > > > > - for (unsigned int starts = 0; starts < startCycles; starts++) { > > - ret = capture.capture(numRequests); > > - if (ret.first != Results::Pass) > > - return ret; > > - } > > +/* > > + * Test single capture cycles > > + * > > + * Makes sure the camera completes the exact number of requests queued. Example > > + * failure is a camera that needs N+M requests queued to complete N requests to > > + * the application. > > + */ > > +class BalancedSingleCycle : public SingleStream { > > +public: > > + explicit BalancedSingleCycle(std::shared_ptr<Camera> camera, StreamRole role, unsigned int numRequests) > > + : SingleStream(camera, role, numRequests) {} > > > > - return { Results::Pass, "Balanced capture of " + > > - std::to_string(numRequests) + " requests with " + > > - std::to_string(startCycles) + " start cycles" }; > > -} > > + void TestBody() override { > > + SimpleCaptureBalanced capture(camera_); > > > > -Results::Result testRequestUnbalance(std::shared_ptr<Camera> camera, > > - StreamRole role, unsigned int numRequests) > > -{ > > - SimpleCaptureUnbalanced capture(camera); > > + capture.configure(role_); > > > > - Results::Result ret = capture.configure(role); > > - if (ret.first != Results::Pass) > > - return ret; > > + capture.capture(numRequests_); > > + }; > > +}; > > > > - return capture.capture(numRequests); > > -} > > +/* > > + * Test multiple start/stop cycles > > + * > > + * Makes sure the camera supports multiple start/stop cycles. Example failure is > > + * a camera that does not clean up correctly in its error path but is only > > + * tested by single-capture applications. > > + */ > > +class BalancedMultiCycle : public SingleStream { > > +public: > > + explicit BalancedMultiCycle(std::shared_ptr<Camera> camera, StreamRole role, unsigned int numRequests) > > + : SingleStream(camera, role, numRequests) {} > > > > -Results testSingleStream(std::shared_ptr<Camera> camera) > > -{ > > - static const std::vector<std::pair<std::string, StreamRole>> roles = { > > - { "raw", Raw }, > > - { "still", StillCapture }, > > - { "video", VideoRecording }, > > - { "viewfinder", Viewfinder }, > > + void TestBody() override { > > + unsigned int numRepeats = 3; > > + > > + SimpleCaptureBalanced capture(camera_); > > + > > + capture.configure(role_); > > + > > + for (unsigned int starts = 0; starts < numRepeats; starts++) > > + capture.capture(numRequests_); > > }; > > - static const std::vector<unsigned int> numRequests = { 1, 2, 3, 5, 8, 13, 21, 34, 55, 89 }; > > - > > - Results results(numRequests.size() * roles.size() * 3); > > - > > - for (const auto &role : roles) { > > - std::cout << "= Test role " << role.first << std::endl; > > - /* > > - * Test single capture cycles > > - * > > - * Makes sure the camera completes the exact number of requests queued. > > - * Example failure is a camera that needs N+M requests queued to > > - * complete N requests to the application. > > - */ > > - std::cout << "* Test single capture cycles" << std::endl; > > - for (unsigned int num : numRequests) > > - results.add(testRequestBalance(camera, role.second, 1, num)); > > - > > - /* > > - * Test multiple start/stop cycles > > - * > > - * Makes sure the camera supports multiple start/stop cycles. > > - * Example failure is a camera that does not clean up correctly in its > > - * error path but is only tested by single-capture applications. > > - */ > > - std::cout << "* Test multiple start/stop cycles" << std::endl; > > - for (unsigned int num : numRequests) > > - results.add(testRequestBalance(camera, role.second, 3, num)); > > - > > - /* > > - * Test unbalanced stop > > - * > > - * Makes sure the camera supports a stop with requests queued. > > - * Example failure is a camera that does not handle cancelation > > - * of buffers coming back from the video device while stopping. > > - */ > > - std::cout << "* Test unbalanced stop" << std::endl; > > - for (unsigned int num : numRequests) > > - results.add(testRequestUnbalance(camera, role.second, num)); > > - } > > - > > - return results; > > +}; > > + > > +/* > > + * Test unbalanced stop > > + * > > + * Makes sure the camera supports a stop with requests queued. Example failure > > + * is a camera that does not handle cancelation of buffers coming back from the > > + * video device while stopping. > > + */ > > +class Unbalanced : public SingleStream { > > +public: > > + explicit Unbalanced(std::shared_ptr<Camera> camera, StreamRole role, unsigned int numRequests) > > + : SingleStream(camera, role, numRequests) {} > > + > > + void TestBody() override { > > + SimpleCaptureUnbalanced capture(camera_); > > + > > + capture.configure(role_); > > + > > + capture.capture(numRequests_); > > + }; > > +}; > > + > > +void registerSingleStreamTests(std::vector<std::shared_ptr<Camera>> cameras, > > + std::map<StreamRole, std::string> roles, std::vector<int> requests) > > +{ > > + REGISTER_TESTS(SingleStream, BalancedSingleCycle, cameras, roles, requests); > > + REGISTER_TESTS(SingleStream, BalancedMultiCycle, cameras, roles, requests); > > + REGISTER_TESTS(SingleStream, Unbalanced, cameras, roles, requests); > > } > > diff --git a/src/lc-compliance/tests.h b/src/lc-compliance/tests.h > > index 396605214e4b..6d5a8b88c287 100644 > > --- a/src/lc-compliance/tests.h > > +++ b/src/lc-compliance/tests.h > > @@ -11,6 +11,19 @@ > > > > #include "results.h" > > > > -Results testSingleStream(std::shared_ptr<libcamera::Camera> camera); > > +void registerSingleStreamTests(std::vector<std::shared_ptr<libcamera::Camera>> cameras, > > + std::map<libcamera::StreamRole, std::string> roles, std::vector<int> requests); > > + > > +#define REGISTER_TESTS(testsuite, testcase, cameras, roles, requests) \ > > +for (auto c : cameras) { \ > > + for (auto [r, rn] : roles) { \ > > + for (auto rq : requests) { \ > > + testing::RegisterTest( \ > > + #testsuite, (std::string(#testcase) + "/" + c->id() + "/" + rn + "/" + std::to_string(rq)).c_str(), \ > > + nullptr, nullptr, __FILE__, __LINE__, \ > > + [=]() -> testsuite* { return new testcase(c, r, rq); }); \ > > + } \ > > + } \ > > +} > > > > #endif /* __LC_COMPLIANCE_TESTS_H__ */ > > -- > > 2.31.1 > > > > -- > Regards, > Niklas Söderlund > > -- > To unsubscribe, send mail to kernel-unsubscribe@lists.collabora.co.uk.
Hi Nícolas, On 2021-05-06 17:20:29 -0300, Nícolas F. R. A. Prado wrote: > Hi Niklas, > > thanks for the feedback. > > Em 2021-05-06 05:48, Niklas Söderlund escreveu: > > > Hi Nícolas, > > > > Thanks for your work! > > > > I really like how gtest helps make the actual tests to be easier to > > read. > > > > On 2021-05-03 16:33:07 -0300, Nícolas F. R. A. Prado wrote: > > > Refactor lc-compliance using Googletest as the test framework. > > > > > > Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com> > > > --- > > > Changes in v2: > > > - Changed from static to dynamic test registration > > > - Removed -c flag > > > > > > There's still an issue with the refactoring that the shared_ptr of the tests > > > apparently aren't being deleted on time, which causes: > > > > > > [20:26:08.744507935] [103243] ERROR DeviceEnumerator device_enumerator.cpp:165 Removing media device /dev/media0 while still in use > > > [20:26:08.744548175] [103243] ERROR DeviceEnumerator device_enumerator.cpp:165 Removing media device /dev/media1 while still in use > > > Segmentation fault (core dumped) > > > > > > I tried explicitly resetting the shared_ptr on the destructor of the tests but > > > that didn't work. In fact, when just listing the tests, the constructor for the > > > tests isn't even called. So I think the issue has to do with the passing of the > > > camera shared pointer to the REGISTER_TESTS() macro through that functor (?): > > > > > > [=]() -> testsuite* { return new testcase(c, r, rq); }); \ > > > > > > Not sure how to solve this. Any tip would be welcome. > > > > I will leave this for now as I have a different worry about > > REGISTER_TESTS() design below. > > > > > > > > src/lc-compliance/main.cpp | 99 +++++++++--------- > > > src/lc-compliance/meson.build | 3 + > > > src/lc-compliance/simple_capture.cpp | 74 +++++-------- > > > src/lc-compliance/simple_capture.h | 8 +- > > > src/lc-compliance/single_stream.cpp | 151 ++++++++++++++------------- > > > src/lc-compliance/tests.h | 15 ++- > > > 6 files changed, 175 insertions(+), 175 deletions(-) > > > > > > diff --git a/src/lc-compliance/main.cpp b/src/lc-compliance/main.cpp > > > index 54cee54aa978..add0d7729aec 100644 > > > --- a/src/lc-compliance/main.cpp > > > +++ b/src/lc-compliance/main.cpp > > > @@ -9,6 +9,8 @@ > > > #include <iostream> > > > #include <string.h> > > > > > > +#include <gtest/gtest.h> > > > + > > > #include <libcamera/libcamera.h> > > > > > > #include "../cam/options.h" > > > @@ -17,7 +19,6 @@ > > > using namespace libcamera; > > > > > > enum { > > > - OptCamera = 'c', > > > OptHelp = 'h', > > > }; > > > > > > @@ -28,14 +29,15 @@ public: > > > ~Harness(); > > > > > > int exec(); > > > + int init(); > > > + void registerTests(); > > > > > > private: > > > - int init(); > > > void listCameras(); > > > > > > OptionsParser::Options options_; > > > std::unique_ptr<CameraManager> cm_; > > > - std::shared_ptr<Camera> camera_; > > > + std::vector<std::shared_ptr<Camera>> cameras_; > > > }; > > > > > > Harness::Harness(const OptionsParser::Options &options) > > > @@ -46,33 +48,14 @@ Harness::Harness(const OptionsParser::Options &options) > > > > > > Harness::~Harness() > > > { > > > - if (camera_) { > > > - camera_->release(); > > > - camera_.reset(); > > > + for (auto &c : cameras_) { > > > + c->release(); > > > + c.reset(); > > > } > > > > > > cm_->stop(); > > > } > > > > > > -int Harness::exec() > > > -{ > > > - int ret = init(); > > > - if (ret) > > > - return ret; > > > - > > > - std::vector<Results> results; > > > - > > > - results.push_back(testSingleStream(camera_)); > > > - > > > - for (const Results &result : results) { > > > - ret = result.summary(); > > > - if (ret) > > > - return ret; > > > - } > > > - > > > - return 0; > > > -} > > > - > > > int Harness::init() > > > { > > > int ret = cm_->start(); > > > @@ -82,42 +65,26 @@ int Harness::init() > > > return ret; > > > } > > > > > > - if (!options_.isSet(OptCamera)) { > > > - std::cout << "No camera specified, available cameras:" << std::endl; > > > - listCameras(); > > > - return -ENODEV; > > > - } > > > - > > > - const std::string &cameraId = options_[OptCamera]; > > > - camera_ = cm_->get(cameraId); > > > - if (!camera_) { > > > - std::cout << "Camera " << cameraId << " not found, available cameras:" << std::endl; > > > - listCameras(); > > > - return -ENODEV; > > > + for (auto cam : cm_->cameras()) { > > > + if (cam->acquire()) { > > > + std::cout << "Failed to acquire camera" << std::endl; > > > + return -EINVAL; > > > + } > > > + cameras_.push_back(cam); > > > > I don't like this, I think we need to retain the ability to select which > > camera to test. And further down the line maybe allow more then one > > camera to be selected if we want to test concurrent use of two specific > > ones. But for the work in this series I think you can ignore multiple > > cameras. > > Actually, that doesn't mean that multiple cameras will be tested, that's > determined by the test filter, so doing --gtest_filter='.*idCamera1.*' would > only test that camera. But I think I see what you mean, I shouldn't be acquiring > all cameras, only the ones that I will actually test (and in this code there's a > big issue where if a single camera is already busy, even if it won't be tested, > lc-compliance as a whole fails). Ahh I understand. > > > > > If I understand the coverletter correctly the correct thing here the cli > > arguments should be feed to gtest directly to be able to filter on what > > tests are run. Is there support in getst to also pass arguments to > > tests? Maybe we could remove the need to parse arguments ourself > > completely? > > There isn't any CLI argument in gtest to pass arguments to tests, but I think we > could solve both problems (this and the one above) like this: > > - In the main(), query the id of each camera from libcamera. > - Register each of the tests dynamically in gtest passing as an argument the > camera id, rather than a camera pointer (which is the current behavior). > - To select which tests to run, and on which camera(s), the user just uses > --gtest_filter='testToRun.*idCamera' (or '.*idCamera.*' to run all tests for > the camera). > - In the startup of each test the camera is acquired for that test, and when > it's done it is released. > > Implications: > - We no longer need to parse arguments and can drop that code, as both camera > and test are specified directly with gtest's filter. > - We only acquire the cameras that we'll actually test and only while they're > being tested. > - As a downside, a single camera will probably be acquired multiple times during > a single run as lc-compliance moves from one test to the next, but I don't > think this would be a big issue. (Maybe we could do this at the test suite > level instead to greatly decrease the number of times of acquire(), but I'm > not sure if it's possible, since we instantiate, by passing the camera id, at > the test level, I'll have to test that). > > What do you think? I understand what you are trying to do but I see two issues. 1. It depends on the implicit knowledge of users how to build the filter string and is rather unintuitive compared to having two distinct arguments, one to select camera and one to express the test filter. This could possibly be worked around by keeping the cam options code and building the gtest filter string internally by injecitng the selected camera name in the correct location, but the we have the next issue, 2. I don't think it will be practically possible to express a test filter that selects a specific combination of two cameras. I do think we need to be able to parameterize test cases. If gtest does not allow to do so using function arguments there are other ways of doing so. I'm sure there are many solutions to this problem and maybe someone more familiar with gtest knows of a neat way to piggyback on its infrastrcuture? Since I know very little about the gtest framework the following is just an idea and I'm happy to yield. How about using a singleton class that carries arguments constructed from cli arguments (camera id) before the gtest are executed. Each test case could then consult this singleton if it needs to access parameters. I think you could take inspiration from the libcamera CameraManager class. This could (at lest for now) imply keeping the cam option parser. If so I think it would be nice if somewhen before we integrate this work that the gtest filter string could be supplied as an argument using that parser. Of course if we can do it the other way around and add arguments to the gtest parser that would be even cooler as it would be less code for us to maintain ;-) > > Thanks, > Nícolas > > > > > > } > > > > > > - if (camera_->acquire()) { > > > - std::cout << "Failed to acquire camera" << std::endl; > > > - return -EINVAL; > > > - } > > > - > > > - std::cout << "Using camera " << cameraId << std::endl; > > > - > > > return 0; > > > } > > > > > > void Harness::listCameras() > > > { > > > - for (const std::shared_ptr<Camera> &cam : cm_->cameras()) > > > - std::cout << "- " << cam.get()->id() << std::endl; > > > + for (const std::shared_ptr<Camera> &c : cm_->cameras()) > > > + std::cout << "- " << c.get()->id() << std::endl; > > > } > > > > > > static int parseOptions(int argc, char **argv, OptionsParser::Options *options) > > > { > > > OptionsParser parser; > > > - parser.addOption(OptCamera, OptionString, > > > - "Specify which camera to operate on, by id", "camera", > > > - ArgumentRequired, "camera"); > > > parser.addOption(OptHelp, OptionNone, "Display this help message", > > > "help"); > > > > > > @@ -133,6 +100,31 @@ static int parseOptions(int argc, char **argv, OptionsParser::Options *options) > > > return 0; > > > } > > > > > > +/* > > > + * Make asserts act like exceptions, otherwise they only fail (or skip) the > > > + * current function. From gtest documentation: > > > + * https://google.github.io/googletest/advanced.html#asserting-on-subroutines-with-an-exception > > > + */ > > > +class ThrowListener : public testing::EmptyTestEventListener { > > > + void OnTestPartResult(const testing::TestPartResult& result) override { > > > + if (result.type() == testing::TestPartResult::kFatalFailure > > > + || result.type() == testing::TestPartResult::kSkip) { > > > + throw testing::AssertionException(result); > > > + } > > > + } > > > +}; > > > + > > > + > > > +void Harness::registerTests() { > > > + std::map<StreamRole, std::string> roles = {{Raw, "Raw"}, > > > + {StillCapture, "Still"}, > > > + {VideoRecording, "Video"}, > > > + {Viewfinder, "Viewfinder"}}; > > > + std::vector<int> requests = {1, 2, 3, 5, 8, 13, 21, 34, 55, 89}; > > > + > > > + registerSingleStreamTests(cameras_, roles, requests); > > > +} > > > + > > > int main(int argc, char **argv) > > > { > > > OptionsParser::Options options; > > > @@ -143,6 +135,13 @@ int main(int argc, char **argv) > > > return EXIT_FAILURE; > > > > > > Harness harness(options); > > > + ret = harness.init(); > > > + if (ret) > > > + return ret; > > > + > > > + harness.registerTests(); > > > > > > - return harness.exec() ? EXIT_FAILURE : EXIT_SUCCESS; > > > + ::testing::InitGoogleTest(&argc, argv); > > > + testing::UnitTest::GetInstance()->listeners().Append(new ThrowListener); > > > + return RUN_ALL_TESTS(); > > > } > > > diff --git a/src/lc-compliance/meson.build b/src/lc-compliance/meson.build > > > index a2bfcceb1259..704bc18af3e1 100644 > > > --- a/src/lc-compliance/meson.build > > > +++ b/src/lc-compliance/meson.build > > > @@ -18,10 +18,13 @@ lc_compliance_sources = files([ > > > 'single_stream.cpp', > > > ]) > > > > > > +gtest_dep = dependency('gtest') > > > + > > > lc_compliance = executable('lc-compliance', lc_compliance_sources, > > > dependencies : [ > > > libatomic, > > > libcamera_dep, > > > libevent, > > > + gtest_dep, > > > ], > > > install : true) > > > diff --git a/src/lc-compliance/simple_capture.cpp b/src/lc-compliance/simple_capture.cpp > > > index f90fe6d0f9aa..7731eb16f8c2 100644 > > > --- a/src/lc-compliance/simple_capture.cpp > > > +++ b/src/lc-compliance/simple_capture.cpp > > > @@ -5,6 +5,8 @@ > > > * simple_capture.cpp - Simple capture helper > > > */ > > > > > > +#include <gtest/gtest.h> > > > + > > > #include "simple_capture.h" > > > > > > using namespace libcamera; > > > @@ -20,38 +22,34 @@ SimpleCapture::~SimpleCapture() > > > stop(); > > > } > > > > > > -Results::Result SimpleCapture::configure(StreamRole role) > > > +void SimpleCapture::configure(StreamRole role) > > > { > > > config_ = camera_->generateConfiguration({ role }); > > > > > > - if (!config_) > > > - return { Results::Skip, "Role not supported by camera" }; > > > + if (!config_) { > > > + std::cout << "Role not supported by camera" << std::endl; > > > + GTEST_SKIP(); > > > + } > > > > > > if (config_->validate() != CameraConfiguration::Valid) { > > > config_.reset(); > > > - return { Results::Fail, "Configuration not valid" }; > > > + FAIL() << "Configuration not valid"; > > > } > > > > > > if (camera_->configure(config_.get())) { > > > config_.reset(); > > > - return { Results::Fail, "Failed to configure camera" }; > > > + FAIL() << "Failed to configure camera"; > > > } > > > - > > > - return { Results::Pass, "Configure camera" }; > > > } > > > > > > -Results::Result SimpleCapture::start() > > > +void SimpleCapture::start() > > > { > > > Stream *stream = config_->at(0).stream(); > > > - if (allocator_->allocate(stream) < 0) > > > - return { Results::Fail, "Failed to allocate buffers" }; > > > + ASSERT_GE(allocator_->allocate(stream), 0) << "Failed to allocate buffers"; > > > > > > - if (camera_->start()) > > > - return { Results::Fail, "Failed to start camera" }; > > > + ASSERT_TRUE(!camera_->start()) << "Failed to start camera"; > > > > > > camera_->requestCompleted.connect(this, &SimpleCapture::requestComplete); > > > - > > > - return { Results::Pass, "Started camera" }; > > > } > > > > > > void SimpleCapture::stop() > > > @@ -77,22 +75,19 @@ SimpleCaptureBalanced::SimpleCaptureBalanced(std::shared_ptr<Camera> camera) > > > { > > > } > > > > > > -Results::Result SimpleCaptureBalanced::capture(unsigned int numRequests) > > > +void SimpleCaptureBalanced::capture(unsigned int numRequests) > > > { > > > - Results::Result ret = start(); > > > - if (ret.first != Results::Pass) > > > - return ret; > > > + start(); > > > > > > Stream *stream = config_->at(0).stream(); > > > const std::vector<std::unique_ptr<FrameBuffer>> &buffers = allocator_->buffers(stream); > > > > > > /* No point in testing less requests then the camera depth. */ > > > if (buffers.size() > numRequests) { > > > - /* Cache buffers.size() before we destroy it in stop() */ > > > - int buffers_size = buffers.size(); > > > - > > > - return { Results::Skip, "Camera needs " + std::to_string(buffers_size) > > > - + " requests, can't test only " + std::to_string(numRequests) }; > > > + std::cout << "Camera needs " + std::to_string(buffers.size()) > > > + + " requests, can't test only " > > > + + std::to_string(numRequests) << std::endl; > > > + GTEST_SKIP(); > > > } > > > > > > queueCount_ = 0; > > > @@ -103,14 +98,11 @@ Results::Result SimpleCaptureBalanced::capture(unsigned int numRequests) > > > std::vector<std::unique_ptr<libcamera::Request>> requests; > > > for (const std::unique_ptr<FrameBuffer> &buffer : buffers) { > > > std::unique_ptr<Request> request = camera_->createRequest(); > > > - if (!request) > > > - return { Results::Fail, "Can't create request" }; > > > + ASSERT_TRUE(request) << "Can't create request"; > > > > > > - if (request->addBuffer(stream, buffer.get())) > > > - return { Results::Fail, "Can't set buffer for request" }; > > > + ASSERT_FALSE(request->addBuffer(stream, buffer.get())) << "Can't set buffer for request"; > > > > > > - if (queueRequest(request.get()) < 0) > > > - return { Results::Fail, "Failed to queue request" }; > > > + ASSERT_GE(queueRequest(request.get()), 0) << "Failed to queue request"; > > > > > > requests.push_back(std::move(request)); > > > } > > > @@ -121,12 +113,7 @@ Results::Result SimpleCaptureBalanced::capture(unsigned int numRequests) > > > stop(); > > > delete loop_; > > > > > > - if (captureCount_ != captureLimit_) > > > - return { Results::Fail, "Got " + std::to_string(captureCount_) + > > > - " request, wanted " + std::to_string(captureLimit_) }; > > > - > > > - return { Results::Pass, "Balanced capture of " + > > > - std::to_string(numRequests) + " requests" }; > > > + ASSERT_EQ(captureCount_, captureLimit_); > > > } > > > > > > int SimpleCaptureBalanced::queueRequest(Request *request) > > > @@ -158,11 +145,9 @@ SimpleCaptureUnbalanced::SimpleCaptureUnbalanced(std::shared_ptr<Camera> camera) > > > { > > > } > > > > > > -Results::Result SimpleCaptureUnbalanced::capture(unsigned int numRequests) > > > +void SimpleCaptureUnbalanced::capture(unsigned int numRequests) > > > { > > > - Results::Result ret = start(); > > > - if (ret.first != Results::Pass) > > > - return ret; > > > + start(); > > > > > > Stream *stream = config_->at(0).stream(); > > > const std::vector<std::unique_ptr<FrameBuffer>> &buffers = allocator_->buffers(stream); > > > @@ -174,14 +159,11 @@ Results::Result SimpleCaptureUnbalanced::capture(unsigned int numRequests) > > > std::vector<std::unique_ptr<libcamera::Request>> requests; > > > for (const std::unique_ptr<FrameBuffer> &buffer : buffers) { > > > std::unique_ptr<Request> request = camera_->createRequest(); > > > - if (!request) > > > - return { Results::Fail, "Can't create request" }; > > > + ASSERT_TRUE(request) << "Can't create request"; > > > > > > - if (request->addBuffer(stream, buffer.get())) > > > - return { Results::Fail, "Can't set buffer for request" }; > > > + ASSERT_FALSE(request->addBuffer(stream, buffer.get())) << "Can't set buffer for request"; > > > > > > - if (camera_->queueRequest(request.get()) < 0) > > > - return { Results::Fail, "Failed to queue request" }; > > > + ASSERT_GE(camera_->queueRequest(request.get()), 0) << "Failed to queue request"; > > > > > > requests.push_back(std::move(request)); > > > } > > > @@ -192,7 +174,7 @@ Results::Result SimpleCaptureUnbalanced::capture(unsigned int numRequests) > > > stop(); > > > delete loop_; > > > > > > - return { status ? Results::Fail : Results::Pass, "Unbalanced capture of " + std::to_string(numRequests) + " requests" }; > > > + ASSERT_FALSE(status); > > > } > > > > > > void SimpleCaptureUnbalanced::requestComplete(Request *request) > > > diff --git a/src/lc-compliance/simple_capture.h b/src/lc-compliance/simple_capture.h > > > index d9de53fb63a3..0f8465083456 100644 > > > --- a/src/lc-compliance/simple_capture.h > > > +++ b/src/lc-compliance/simple_capture.h > > > @@ -17,13 +17,13 @@ > > > class SimpleCapture > > > { > > > public: > > > - Results::Result configure(libcamera::StreamRole role); > > > + void configure(libcamera::StreamRole role); > > > > > > protected: > > > SimpleCapture(std::shared_ptr<libcamera::Camera> camera); > > > virtual ~SimpleCapture(); > > > > > > - Results::Result start(); > > > + void start(); > > > void stop(); > > > > > > virtual void requestComplete(libcamera::Request *request) = 0; > > > @@ -40,7 +40,7 @@ class SimpleCaptureBalanced : public SimpleCapture > > > public: > > > SimpleCaptureBalanced(std::shared_ptr<libcamera::Camera> camera); > > > > > > - Results::Result capture(unsigned int numRequests); > > > + void capture(unsigned int numRequests); > > > > > > private: > > > int queueRequest(libcamera::Request *request); > > > @@ -56,7 +56,7 @@ class SimpleCaptureUnbalanced : public SimpleCapture > > > public: > > > SimpleCaptureUnbalanced(std::shared_ptr<libcamera::Camera> camera); > > > > > > - Results::Result capture(unsigned int numRequests); > > > + void capture(unsigned int numRequests); > > > > > > private: > > > void requestComplete(libcamera::Request *request) override; > > > diff --git a/src/lc-compliance/single_stream.cpp b/src/lc-compliance/single_stream.cpp > > > index 8318b42f42d6..eb6a6f305826 100644 > > > --- a/src/lc-compliance/single_stream.cpp > > > +++ b/src/lc-compliance/single_stream.cpp > > > @@ -7,91 +7,94 @@ > > > > > > #include <iostream> > > > > > > +#include <gtest/gtest.h> > > > + > > > #include "simple_capture.h" > > > #include "tests.h" > > > > > > using namespace libcamera; > > > > > > -Results::Result testRequestBalance(std::shared_ptr<Camera> camera, > > > - StreamRole role, unsigned int startCycles, > > > - unsigned int numRequests) > > > -{ > > > - SimpleCaptureBalanced capture(camera); > > > +class SingleStream : public testing::Test { > > > +public: > > > + explicit SingleStream(std::shared_ptr<Camera> camera, StreamRole role, unsigned int numRequests) > > > + : camera_(camera), role_(role), numRequests_(numRequests) {} > > > > > > - Results::Result ret = capture.configure(role); > > > - if (ret.first != Results::Pass) > > > - return ret; > > > +protected: > > > + std::shared_ptr<Camera> camera_; > > > + StreamRole role_; > > > + unsigned int numRequests_; > > > +}; > > > > > > - for (unsigned int starts = 0; starts < startCycles; starts++) { > > > - ret = capture.capture(numRequests); > > > - if (ret.first != Results::Pass) > > > - return ret; > > > - } > > > +/* > > > + * Test single capture cycles > > > + * > > > + * Makes sure the camera completes the exact number of requests queued. Example > > > + * failure is a camera that needs N+M requests queued to complete N requests to > > > + * the application. > > > + */ > > > +class BalancedSingleCycle : public SingleStream { > > > +public: > > > + explicit BalancedSingleCycle(std::shared_ptr<Camera> camera, StreamRole role, unsigned int numRequests) > > > + : SingleStream(camera, role, numRequests) {} > > > > > > - return { Results::Pass, "Balanced capture of " + > > > - std::to_string(numRequests) + " requests with " + > > > - std::to_string(startCycles) + " start cycles" }; > > > -} > > > + void TestBody() override { > > > + SimpleCaptureBalanced capture(camera_); > > > > > > -Results::Result testRequestUnbalance(std::shared_ptr<Camera> camera, > > > - StreamRole role, unsigned int numRequests) > > > -{ > > > - SimpleCaptureUnbalanced capture(camera); > > > + capture.configure(role_); > > > > > > - Results::Result ret = capture.configure(role); > > > - if (ret.first != Results::Pass) > > > - return ret; > > > + capture.capture(numRequests_); > > > + }; > > > +}; > > > > > > - return capture.capture(numRequests); > > > -} > > > +/* > > > + * Test multiple start/stop cycles > > > + * > > > + * Makes sure the camera supports multiple start/stop cycles. Example failure is > > > + * a camera that does not clean up correctly in its error path but is only > > > + * tested by single-capture applications. > > > + */ > > > +class BalancedMultiCycle : public SingleStream { > > > +public: > > > + explicit BalancedMultiCycle(std::shared_ptr<Camera> camera, StreamRole role, unsigned int numRequests) > > > + : SingleStream(camera, role, numRequests) {} > > > > > > -Results testSingleStream(std::shared_ptr<Camera> camera) > > > -{ > > > - static const std::vector<std::pair<std::string, StreamRole>> roles = { > > > - { "raw", Raw }, > > > - { "still", StillCapture }, > > > - { "video", VideoRecording }, > > > - { "viewfinder", Viewfinder }, > > > + void TestBody() override { > > > + unsigned int numRepeats = 3; > > > + > > > + SimpleCaptureBalanced capture(camera_); > > > + > > > + capture.configure(role_); > > > + > > > + for (unsigned int starts = 0; starts < numRepeats; starts++) > > > + capture.capture(numRequests_); > > > }; > > > - static const std::vector<unsigned int> numRequests = { 1, 2, 3, 5, 8, 13, 21, 34, 55, 89 }; > > > - > > > - Results results(numRequests.size() * roles.size() * 3); > > > - > > > - for (const auto &role : roles) { > > > - std::cout << "= Test role " << role.first << std::endl; > > > - /* > > > - * Test single capture cycles > > > - * > > > - * Makes sure the camera completes the exact number of requests queued. > > > - * Example failure is a camera that needs N+M requests queued to > > > - * complete N requests to the application. > > > - */ > > > - std::cout << "* Test single capture cycles" << std::endl; > > > - for (unsigned int num : numRequests) > > > - results.add(testRequestBalance(camera, role.second, 1, num)); > > > - > > > - /* > > > - * Test multiple start/stop cycles > > > - * > > > - * Makes sure the camera supports multiple start/stop cycles. > > > - * Example failure is a camera that does not clean up correctly in its > > > - * error path but is only tested by single-capture applications. > > > - */ > > > - std::cout << "* Test multiple start/stop cycles" << std::endl; > > > - for (unsigned int num : numRequests) > > > - results.add(testRequestBalance(camera, role.second, 3, num)); > > > - > > > - /* > > > - * Test unbalanced stop > > > - * > > > - * Makes sure the camera supports a stop with requests queued. > > > - * Example failure is a camera that does not handle cancelation > > > - * of buffers coming back from the video device while stopping. > > > - */ > > > - std::cout << "* Test unbalanced stop" << std::endl; > > > - for (unsigned int num : numRequests) > > > - results.add(testRequestUnbalance(camera, role.second, num)); > > > - } > > > - > > > - return results; > > > +}; > > > + > > > +/* > > > + * Test unbalanced stop > > > + * > > > + * Makes sure the camera supports a stop with requests queued. Example failure > > > + * is a camera that does not handle cancelation of buffers coming back from the > > > + * video device while stopping. > > > + */ > > > +class Unbalanced : public SingleStream { > > > +public: > > > + explicit Unbalanced(std::shared_ptr<Camera> camera, StreamRole role, unsigned int numRequests) > > > + : SingleStream(camera, role, numRequests) {} > > > + > > > + void TestBody() override { > > > + SimpleCaptureUnbalanced capture(camera_); > > > + > > > + capture.configure(role_); > > > + > > > + capture.capture(numRequests_); > > > + }; > > > +}; > > > + > > > +void registerSingleStreamTests(std::vector<std::shared_ptr<Camera>> cameras, > > > + std::map<StreamRole, std::string> roles, std::vector<int> requests) > > > +{ > > > + REGISTER_TESTS(SingleStream, BalancedSingleCycle, cameras, roles, requests); > > > + REGISTER_TESTS(SingleStream, BalancedMultiCycle, cameras, roles, requests); > > > + REGISTER_TESTS(SingleStream, Unbalanced, cameras, roles, requests); > > > } > > > diff --git a/src/lc-compliance/tests.h b/src/lc-compliance/tests.h > > > index 396605214e4b..6d5a8b88c287 100644 > > > --- a/src/lc-compliance/tests.h > > > +++ b/src/lc-compliance/tests.h > > > @@ -11,6 +11,19 @@ > > > > > > #include "results.h" > > > > > > -Results testSingleStream(std::shared_ptr<libcamera::Camera> camera); > > > +void registerSingleStreamTests(std::vector<std::shared_ptr<libcamera::Camera>> cameras, > > > + std::map<libcamera::StreamRole, std::string> roles, std::vector<int> requests); > > > + > > > +#define REGISTER_TESTS(testsuite, testcase, cameras, roles, requests) \ > > > +for (auto c : cameras) { \ > > > + for (auto [r, rn] : roles) { \ > > > + for (auto rq : requests) { \ > > > + testing::RegisterTest( \ > > > + #testsuite, (std::string(#testcase) + "/" + c->id() + "/" + rn + "/" + std::to_string(rq)).c_str(), \ > > > + nullptr, nullptr, __FILE__, __LINE__, \ > > > + [=]() -> testsuite* { return new testcase(c, r, rq); }); \ > > > + } \ > > > + } \ > > > +} > > > > > > #endif /* __LC_COMPLIANCE_TESTS_H__ */ > > > -- > > > 2.31.1 > > > > > > > -- > > Regards, > > Niklas Söderlund > > > > -- > > To unsubscribe, send mail to kernel-unsubscribe@lists.collabora.co.uk. > >
Hi Niklas, Em 2021-05-07 06:11, Niklas Söderlund escreveu: > Hi Nícolas, > > On 2021-05-06 17:20:29 -0300, Nícolas F. R. A. Prado wrote: > > Hi Niklas, > > > > thanks for the feedback. > > > > Em 2021-05-06 05:48, Niklas Söderlund escreveu: > > > > > Hi Nícolas, > > > > > > Thanks for your work! > > > > > > I really like how gtest helps make the actual tests to be easier to > > > read. > > > > > > On 2021-05-03 16:33:07 -0300, Nícolas F. R. A. Prado wrote: > > > > Refactor lc-compliance using Googletest as the test framework. > > > > > > > > Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com> > > > > --- > > > > Changes in v2: > > > > - Changed from static to dynamic test registration > > > > - Removed -c flag > > > > > > > > There's still an issue with the refactoring that the shared_ptr of the tests > > > > apparently aren't being deleted on time, which causes: > > > > > > > > [20:26:08.744507935] [103243] ERROR DeviceEnumerator device_enumerator.cpp:165 Removing media device /dev/media0 while still in use > > > > [20:26:08.744548175] [103243] ERROR DeviceEnumerator device_enumerator.cpp:165 Removing media device /dev/media1 while still in use > > > > Segmentation fault (core dumped) > > > > > > > > I tried explicitly resetting the shared_ptr on the destructor of the tests but > > > > that didn't work. In fact, when just listing the tests, the constructor for the > > > > tests isn't even called. So I think the issue has to do with the passing of the > > > > camera shared pointer to the REGISTER_TESTS() macro through that functor (?): > > > > > > > > [=]() -> testsuite* { return new testcase(c, r, rq); }); \ > > > > > > > > Not sure how to solve this. Any tip would be welcome. > > > > > > I will leave this for now as I have a different worry about > > > REGISTER_TESTS() design below. > > > > > > > > > > > src/lc-compliance/main.cpp | 99 +++++++++--------- > > > > src/lc-compliance/meson.build | 3 + > > > > src/lc-compliance/simple_capture.cpp | 74 +++++-------- > > > > src/lc-compliance/simple_capture.h | 8 +- > > > > src/lc-compliance/single_stream.cpp | 151 ++++++++++++++------------- > > > > src/lc-compliance/tests.h | 15 ++- > > > > 6 files changed, 175 insertions(+), 175 deletions(-) > > > > > > > > diff --git a/src/lc-compliance/main.cpp b/src/lc-compliance/main.cpp > > > > index 54cee54aa978..add0d7729aec 100644 > > > > --- a/src/lc-compliance/main.cpp > > > > +++ b/src/lc-compliance/main.cpp > > > > @@ -9,6 +9,8 @@ > > > > #include <iostream> > > > > #include <string.h> > > > > > > > > +#include <gtest/gtest.h> > > > > + > > > > #include <libcamera/libcamera.h> > > > > > > > > #include "../cam/options.h" > > > > @@ -17,7 +19,6 @@ > > > > using namespace libcamera; > > > > > > > > enum { > > > > - OptCamera = 'c', > > > > OptHelp = 'h', > > > > }; > > > > > > > > @@ -28,14 +29,15 @@ public: > > > > ~Harness(); > > > > > > > > int exec(); > > > > + int init(); > > > > + void registerTests(); > > > > > > > > private: > > > > - int init(); > > > > void listCameras(); > > > > > > > > OptionsParser::Options options_; > > > > std::unique_ptr<CameraManager> cm_; > > > > - std::shared_ptr<Camera> camera_; > > > > + std::vector<std::shared_ptr<Camera>> cameras_; > > > > }; > > > > > > > > Harness::Harness(const OptionsParser::Options &options) > > > > @@ -46,33 +48,14 @@ Harness::Harness(const OptionsParser::Options &options) > > > > > > > > Harness::~Harness() > > > > { > > > > - if (camera_) { > > > > - camera_->release(); > > > > - camera_.reset(); > > > > + for (auto &c : cameras_) { > > > > + c->release(); > > > > + c.reset(); > > > > } > > > > > > > > cm_->stop(); > > > > } > > > > > > > > -int Harness::exec() > > > > -{ > > > > - int ret = init(); > > > > - if (ret) > > > > - return ret; > > > > - > > > > - std::vector<Results> results; > > > > - > > > > - results.push_back(testSingleStream(camera_)); > > > > - > > > > - for (const Results &result : results) { > > > > - ret = result.summary(); > > > > - if (ret) > > > > - return ret; > > > > - } > > > > - > > > > - return 0; > > > > -} > > > > - > > > > int Harness::init() > > > > { > > > > int ret = cm_->start(); > > > > @@ -82,42 +65,26 @@ int Harness::init() > > > > return ret; > > > > } > > > > > > > > - if (!options_.isSet(OptCamera)) { > > > > - std::cout << "No camera specified, available cameras:" << std::endl; > > > > - listCameras(); > > > > - return -ENODEV; > > > > - } > > > > - > > > > - const std::string &cameraId = options_[OptCamera]; > > > > - camera_ = cm_->get(cameraId); > > > > - if (!camera_) { > > > > - std::cout << "Camera " << cameraId << " not found, available cameras:" << std::endl; > > > > - listCameras(); > > > > - return -ENODEV; > > > > + for (auto cam : cm_->cameras()) { > > > > + if (cam->acquire()) { > > > > + std::cout << "Failed to acquire camera" << std::endl; > > > > + return -EINVAL; > > > > + } > > > > + cameras_.push_back(cam); > > > > > > I don't like this, I think we need to retain the ability to select which > > > camera to test. And further down the line maybe allow more then one > > > camera to be selected if we want to test concurrent use of two specific > > > ones. But for the work in this series I think you can ignore multiple > > > cameras. > > > > Actually, that doesn't mean that multiple cameras will be tested, that's > > determined by the test filter, so doing --gtest_filter='.*idCamera1.*' would > > only test that camera. But I think I see what you mean, I shouldn't be acquiring > > all cameras, only the ones that I will actually test (and in this code there's a > > big issue where if a single camera is already busy, even if it won't be tested, > > lc-compliance as a whole fails). > > Ahh I understand. > > > > > > > > > If I understand the coverletter correctly the correct thing here the cli > > > arguments should be feed to gtest directly to be able to filter on what > > > tests are run. Is there support in getst to also pass arguments to > > > tests? Maybe we could remove the need to parse arguments ourself > > > completely? > > > > There isn't any CLI argument in gtest to pass arguments to tests, but I think we > > could solve both problems (this and the one above) like this: > > > > - In the main(), query the id of each camera from libcamera. > > - Register each of the tests dynamically in gtest passing as an argument the > > camera id, rather than a camera pointer (which is the current behavior). > > - To select which tests to run, and on which camera(s), the user just uses > > --gtest_filter='testToRun.*idCamera' (or '.*idCamera.*' to run all tests for > > the camera). > > - In the startup of each test the camera is acquired for that test, and when > > it's done it is released. > > > > Implications: > > - We no longer need to parse arguments and can drop that code, as both camera > > and test are specified directly with gtest's filter. > > - We only acquire the cameras that we'll actually test and only while they're > > being tested. > > - As a downside, a single camera will probably be acquired multiple times during > > a single run as lc-compliance moves from one test to the next, but I don't > > think this would be a big issue. (Maybe we could do this at the test suite > > level instead to greatly decrease the number of times of acquire(), but I'm > > not sure if it's possible, since we instantiate, by passing the camera id, at > > the test level, I'll have to test that). > > > > What do you think? > > I understand what you are trying to do but I see two issues. > > 1. It depends on the implicit knowledge of users how to build the filter > string and is rather unintuitive compared to having two distinct > arguments, one to select camera and one to express the test filter. > > This could possibly be worked around by keeping the cam options code > and building the gtest filter string internally by injecitng the > selected camera name in the correct location, but the we have the > next issue, > > 2. I don't think it will be practically possible to express a test > filter that selects a specific combination of two cameras. > > I do think we need to be able to parameterize test cases. If gtest does > not allow to do so using function arguments there are other ways of > doing so. I think I didn't express myself well. I meant to say that gtest's CLI doesn't have a flag to allow passing parameters to tests, meaning that it would not be possible to parametrize the tests from the command line and also remove our own parser and rely on gtest's. But gtest does allow test parametrization at runtime and I'm already relying on it since v2. Let me go a bit more over the way I'm doing this. - I have a test defined, eg BalancedSingleCycle, which takes some parameters: std::shared_ptr<Camera> camera, StreamRole role, unsigned int numRequests - Since we may want to run the same test for all cameras, roles and numRequests, I register it, at runtime, using my REGISTER_TESTS() macro, which calls gtest's testing::RegisterTest() with all possible combinations of parameters. - Based on the --gtest_filter CLI argument from gtest, it decides which tests are actually run So the key point is that even though I parametrize the tests, I do it for all combinations in order to later rely on gtest_filter to select which tests to run. Now, I do agree with both of your points. And if we want to make the interface more intuitive to use (which I also find important), there's no way around having our own parser. And if we're using our own camera flag, then it makes no sense to register tests for all cameras, we can register tests only with the specified camera. I think here it would be helpful to think if the interface suits our problem. To me we're looking at having: lc-compliance --camera 'cameraId' --filter 'BalancedSingleCycle' to only test camera 'cameraId' with a single test 'BalancedSingleCycle'. Here, the camera flag is used to only register tests with that camera, and the filter is passed as --gtest_filter, possibly a bit adapted, to gtest. And if we imagine we have a test ConcurrentCameras, which receives two cameras as parameters, and we call lc-compliance --camera 'cameraId1,cameraId2' it would test both each of the cameras separately in all current tests which accept a single camera, like BalancedSingleCycle, as well as test both at the same time in the ConcurrentCameras test (and we could make only the former or the latter occur by using the filter parameter). I know we don't need to implement support for multiple cameras right now, I just want to make sure it's possible and thinking about the interface and the scenarios help me picture how they'd work. Thoughts? Thanks, Nícolas > > I'm sure there are many solutions to this problem and maybe someone more > familiar with gtest knows of a neat way to piggyback on its > infrastrcuture? Since I know very little about the gtest framework the > following is just an idea and I'm happy to yield. > > How about using a singleton class that carries arguments constructed > from cli arguments (camera id) before the gtest are executed. Each test > case could then consult this singleton if it needs to access parameters. > I think you could take inspiration from the libcamera CameraManager > class. > > This could (at lest for now) imply keeping the cam option parser. If so > I think it would be nice if somewhen before we integrate this work that > the gtest filter string could be supplied as an argument using that > parser. Of course if we can do it the other way around and add arguments > to the gtest parser that would be even cooler as it would be less code > for us to maintain ;-) > > > > > Thanks, > > Nícolas > > > > > > > > > } > > > > > > > > - if (camera_->acquire()) { > > > > - std::cout << "Failed to acquire camera" << std::endl; > > > > - return -EINVAL; > > > > - } > > > > - > > > > - std::cout << "Using camera " << cameraId << std::endl; > > > > - > > > > return 0; > > > > } > > > > > > > > void Harness::listCameras() > > > > { > > > > - for (const std::shared_ptr<Camera> &cam : cm_->cameras()) > > > > - std::cout << "- " << cam.get()->id() << std::endl; > > > > + for (const std::shared_ptr<Camera> &c : cm_->cameras()) > > > > + std::cout << "- " << c.get()->id() << std::endl; > > > > } > > > > > > > > static int parseOptions(int argc, char **argv, OptionsParser::Options *options) > > > > { > > > > OptionsParser parser; > > > > - parser.addOption(OptCamera, OptionString, > > > > - "Specify which camera to operate on, by id", "camera", > > > > - ArgumentRequired, "camera"); > > > > parser.addOption(OptHelp, OptionNone, "Display this help message", > > > > "help"); > > > > > > > > @@ -133,6 +100,31 @@ static int parseOptions(int argc, char **argv, OptionsParser::Options *options) > > > > return 0; > > > > } > > > > > > > > +/* > > > > + * Make asserts act like exceptions, otherwise they only fail (or skip) the > > > > + * current function. From gtest documentation: > > > > + * https://google.github.io/googletest/advanced.html#asserting-on-subroutines-with-an-exception > > > > + */ > > > > +class ThrowListener : public testing::EmptyTestEventListener { > > > > + void OnTestPartResult(const testing::TestPartResult& result) override { > > > > + if (result.type() == testing::TestPartResult::kFatalFailure > > > > + || result.type() == testing::TestPartResult::kSkip) { > > > > + throw testing::AssertionException(result); > > > > + } > > > > + } > > > > +}; > > > > + > > > > + > > > > +void Harness::registerTests() { > > > > + std::map<StreamRole, std::string> roles = {{Raw, "Raw"}, > > > > + {StillCapture, "Still"}, > > > > + {VideoRecording, "Video"}, > > > > + {Viewfinder, "Viewfinder"}}; > > > > + std::vector<int> requests = {1, 2, 3, 5, 8, 13, 21, 34, 55, 89}; > > > > + > > > > + registerSingleStreamTests(cameras_, roles, requests); > > > > +} > > > > + > > > > int main(int argc, char **argv) > > > > { > > > > OptionsParser::Options options; > > > > @@ -143,6 +135,13 @@ int main(int argc, char **argv) > > > > return EXIT_FAILURE; > > > > > > > > Harness harness(options); > > > > + ret = harness.init(); > > > > + if (ret) > > > > + return ret; > > > > + > > > > + harness.registerTests(); > > > > > > > > - return harness.exec() ? EXIT_FAILURE : EXIT_SUCCESS; > > > > + ::testing::InitGoogleTest(&argc, argv); > > > > + testing::UnitTest::GetInstance()->listeners().Append(new ThrowListener); > > > > + return RUN_ALL_TESTS(); > > > > } > > > > diff --git a/src/lc-compliance/meson.build b/src/lc-compliance/meson.build > > > > index a2bfcceb1259..704bc18af3e1 100644 > > > > --- a/src/lc-compliance/meson.build > > > > +++ b/src/lc-compliance/meson.build > > > > @@ -18,10 +18,13 @@ lc_compliance_sources = files([ > > > > 'single_stream.cpp', > > > > ]) > > > > > > > > +gtest_dep = dependency('gtest') > > > > + > > > > lc_compliance = executable('lc-compliance', lc_compliance_sources, > > > > dependencies : [ > > > > libatomic, > > > > libcamera_dep, > > > > libevent, > > > > + gtest_dep, > > > > ], > > > > install : true) > > > > diff --git a/src/lc-compliance/simple_capture.cpp b/src/lc-compliance/simple_capture.cpp > > > > index f90fe6d0f9aa..7731eb16f8c2 100644 > > > > --- a/src/lc-compliance/simple_capture.cpp > > > > +++ b/src/lc-compliance/simple_capture.cpp > > > > @@ -5,6 +5,8 @@ > > > > * simple_capture.cpp - Simple capture helper > > > > */ > > > > > > > > +#include <gtest/gtest.h> > > > > + > > > > #include "simple_capture.h" > > > > > > > > using namespace libcamera; > > > > @@ -20,38 +22,34 @@ SimpleCapture::~SimpleCapture() > > > > stop(); > > > > } > > > > > > > > -Results::Result SimpleCapture::configure(StreamRole role) > > > > +void SimpleCapture::configure(StreamRole role) > > > > { > > > > config_ = camera_->generateConfiguration({ role }); > > > > > > > > - if (!config_) > > > > - return { Results::Skip, "Role not supported by camera" }; > > > > + if (!config_) { > > > > + std::cout << "Role not supported by camera" << std::endl; > > > > + GTEST_SKIP(); > > > > + } > > > > > > > > if (config_->validate() != CameraConfiguration::Valid) { > > > > config_.reset(); > > > > - return { Results::Fail, "Configuration not valid" }; > > > > + FAIL() << "Configuration not valid"; > > > > } > > > > > > > > if (camera_->configure(config_.get())) { > > > > config_.reset(); > > > > - return { Results::Fail, "Failed to configure camera" }; > > > > + FAIL() << "Failed to configure camera"; > > > > } > > > > - > > > > - return { Results::Pass, "Configure camera" }; > > > > } > > > > > > > > -Results::Result SimpleCapture::start() > > > > +void SimpleCapture::start() > > > > { > > > > Stream *stream = config_->at(0).stream(); > > > > - if (allocator_->allocate(stream) < 0) > > > > - return { Results::Fail, "Failed to allocate buffers" }; > > > > + ASSERT_GE(allocator_->allocate(stream), 0) << "Failed to allocate buffers"; > > > > > > > > - if (camera_->start()) > > > > - return { Results::Fail, "Failed to start camera" }; > > > > + ASSERT_TRUE(!camera_->start()) << "Failed to start camera"; > > > > > > > > camera_->requestCompleted.connect(this, &SimpleCapture::requestComplete); > > > > - > > > > - return { Results::Pass, "Started camera" }; > > > > } > > > > > > > > void SimpleCapture::stop() > > > > @@ -77,22 +75,19 @@ SimpleCaptureBalanced::SimpleCaptureBalanced(std::shared_ptr<Camera> camera) > > > > { > > > > } > > > > > > > > -Results::Result SimpleCaptureBalanced::capture(unsigned int numRequests) > > > > +void SimpleCaptureBalanced::capture(unsigned int numRequests) > > > > { > > > > - Results::Result ret = start(); > > > > - if (ret.first != Results::Pass) > > > > - return ret; > > > > + start(); > > > > > > > > Stream *stream = config_->at(0).stream(); > > > > const std::vector<std::unique_ptr<FrameBuffer>> &buffers = allocator_->buffers(stream); > > > > > > > > /* No point in testing less requests then the camera depth. */ > > > > if (buffers.size() > numRequests) { > > > > - /* Cache buffers.size() before we destroy it in stop() */ > > > > - int buffers_size = buffers.size(); > > > > - > > > > - return { Results::Skip, "Camera needs " + std::to_string(buffers_size) > > > > - + " requests, can't test only " + std::to_string(numRequests) }; > > > > + std::cout << "Camera needs " + std::to_string(buffers.size()) > > > > + + " requests, can't test only " > > > > + + std::to_string(numRequests) << std::endl; > > > > + GTEST_SKIP(); > > > > } > > > > > > > > queueCount_ = 0; > > > > @@ -103,14 +98,11 @@ Results::Result SimpleCaptureBalanced::capture(unsigned int numRequests) > > > > std::vector<std::unique_ptr<libcamera::Request>> requests; > > > > for (const std::unique_ptr<FrameBuffer> &buffer : buffers) { > > > > std::unique_ptr<Request> request = camera_->createRequest(); > > > > - if (!request) > > > > - return { Results::Fail, "Can't create request" }; > > > > + ASSERT_TRUE(request) << "Can't create request"; > > > > > > > > - if (request->addBuffer(stream, buffer.get())) > > > > - return { Results::Fail, "Can't set buffer for request" }; > > > > + ASSERT_FALSE(request->addBuffer(stream, buffer.get())) << "Can't set buffer for request"; > > > > > > > > - if (queueRequest(request.get()) < 0) > > > > - return { Results::Fail, "Failed to queue request" }; > > > > + ASSERT_GE(queueRequest(request.get()), 0) << "Failed to queue request"; > > > > > > > > requests.push_back(std::move(request)); > > > > } > > > > @@ -121,12 +113,7 @@ Results::Result SimpleCaptureBalanced::capture(unsigned int numRequests) > > > > stop(); > > > > delete loop_; > > > > > > > > - if (captureCount_ != captureLimit_) > > > > - return { Results::Fail, "Got " + std::to_string(captureCount_) + > > > > - " request, wanted " + std::to_string(captureLimit_) }; > > > > - > > > > - return { Results::Pass, "Balanced capture of " + > > > > - std::to_string(numRequests) + " requests" }; > > > > + ASSERT_EQ(captureCount_, captureLimit_); > > > > } > > > > > > > > int SimpleCaptureBalanced::queueRequest(Request *request) > > > > @@ -158,11 +145,9 @@ SimpleCaptureUnbalanced::SimpleCaptureUnbalanced(std::shared_ptr<Camera> camera) > > > > { > > > > } > > > > > > > > -Results::Result SimpleCaptureUnbalanced::capture(unsigned int numRequests) > > > > +void SimpleCaptureUnbalanced::capture(unsigned int numRequests) > > > > { > > > > - Results::Result ret = start(); > > > > - if (ret.first != Results::Pass) > > > > - return ret; > > > > + start(); > > > > > > > > Stream *stream = config_->at(0).stream(); > > > > const std::vector<std::unique_ptr<FrameBuffer>> &buffers = allocator_->buffers(stream); > > > > @@ -174,14 +159,11 @@ Results::Result SimpleCaptureUnbalanced::capture(unsigned int numRequests) > > > > std::vector<std::unique_ptr<libcamera::Request>> requests; > > > > for (const std::unique_ptr<FrameBuffer> &buffer : buffers) { > > > > std::unique_ptr<Request> request = camera_->createRequest(); > > > > - if (!request) > > > > - return { Results::Fail, "Can't create request" }; > > > > + ASSERT_TRUE(request) << "Can't create request"; > > > > > > > > - if (request->addBuffer(stream, buffer.get())) > > > > - return { Results::Fail, "Can't set buffer for request" }; > > > > + ASSERT_FALSE(request->addBuffer(stream, buffer.get())) << "Can't set buffer for request"; > > > > > > > > - if (camera_->queueRequest(request.get()) < 0) > > > > - return { Results::Fail, "Failed to queue request" }; > > > > + ASSERT_GE(camera_->queueRequest(request.get()), 0) << "Failed to queue request"; > > > > > > > > requests.push_back(std::move(request)); > > > > } > > > > @@ -192,7 +174,7 @@ Results::Result SimpleCaptureUnbalanced::capture(unsigned int numRequests) > > > > stop(); > > > > delete loop_; > > > > > > > > - return { status ? Results::Fail : Results::Pass, "Unbalanced capture of " + std::to_string(numRequests) + " requests" }; > > > > + ASSERT_FALSE(status); > > > > } > > > > > > > > void SimpleCaptureUnbalanced::requestComplete(Request *request) > > > > diff --git a/src/lc-compliance/simple_capture.h b/src/lc-compliance/simple_capture.h > > > > index d9de53fb63a3..0f8465083456 100644 > > > > --- a/src/lc-compliance/simple_capture.h > > > > +++ b/src/lc-compliance/simple_capture.h > > > > @@ -17,13 +17,13 @@ > > > > class SimpleCapture > > > > { > > > > public: > > > > - Results::Result configure(libcamera::StreamRole role); > > > > + void configure(libcamera::StreamRole role); > > > > > > > > protected: > > > > SimpleCapture(std::shared_ptr<libcamera::Camera> camera); > > > > virtual ~SimpleCapture(); > > > > > > > > - Results::Result start(); > > > > + void start(); > > > > void stop(); > > > > > > > > virtual void requestComplete(libcamera::Request *request) = 0; > > > > @@ -40,7 +40,7 @@ class SimpleCaptureBalanced : public SimpleCapture > > > > public: > > > > SimpleCaptureBalanced(std::shared_ptr<libcamera::Camera> camera); > > > > > > > > - Results::Result capture(unsigned int numRequests); > > > > + void capture(unsigned int numRequests); > > > > > > > > private: > > > > int queueRequest(libcamera::Request *request); > > > > @@ -56,7 +56,7 @@ class SimpleCaptureUnbalanced : public SimpleCapture > > > > public: > > > > SimpleCaptureUnbalanced(std::shared_ptr<libcamera::Camera> camera); > > > > > > > > - Results::Result capture(unsigned int numRequests); > > > > + void capture(unsigned int numRequests); > > > > > > > > private: > > > > void requestComplete(libcamera::Request *request) override; > > > > diff --git a/src/lc-compliance/single_stream.cpp b/src/lc-compliance/single_stream.cpp > > > > index 8318b42f42d6..eb6a6f305826 100644 > > > > --- a/src/lc-compliance/single_stream.cpp > > > > +++ b/src/lc-compliance/single_stream.cpp > > > > @@ -7,91 +7,94 @@ > > > > > > > > #include <iostream> > > > > > > > > +#include <gtest/gtest.h> > > > > + > > > > #include "simple_capture.h" > > > > #include "tests.h" > > > > > > > > using namespace libcamera; > > > > > > > > -Results::Result testRequestBalance(std::shared_ptr<Camera> camera, > > > > - StreamRole role, unsigned int startCycles, > > > > - unsigned int numRequests) > > > > -{ > > > > - SimpleCaptureBalanced capture(camera); > > > > +class SingleStream : public testing::Test { > > > > +public: > > > > + explicit SingleStream(std::shared_ptr<Camera> camera, StreamRole role, unsigned int numRequests) > > > > + : camera_(camera), role_(role), numRequests_(numRequests) {} > > > > > > > > - Results::Result ret = capture.configure(role); > > > > - if (ret.first != Results::Pass) > > > > - return ret; > > > > +protected: > > > > + std::shared_ptr<Camera> camera_; > > > > + StreamRole role_; > > > > + unsigned int numRequests_; > > > > +}; > > > > > > > > - for (unsigned int starts = 0; starts < startCycles; starts++) { > > > > - ret = capture.capture(numRequests); > > > > - if (ret.first != Results::Pass) > > > > - return ret; > > > > - } > > > > +/* > > > > + * Test single capture cycles > > > > + * > > > > + * Makes sure the camera completes the exact number of requests queued. Example > > > > + * failure is a camera that needs N+M requests queued to complete N requests to > > > > + * the application. > > > > + */ > > > > +class BalancedSingleCycle : public SingleStream { > > > > +public: > > > > + explicit BalancedSingleCycle(std::shared_ptr<Camera> camera, StreamRole role, unsigned int numRequests) > > > > + : SingleStream(camera, role, numRequests) {} > > > > > > > > - return { Results::Pass, "Balanced capture of " + > > > > - std::to_string(numRequests) + " requests with " + > > > > - std::to_string(startCycles) + " start cycles" }; > > > > -} > > > > + void TestBody() override { > > > > + SimpleCaptureBalanced capture(camera_); > > > > > > > > -Results::Result testRequestUnbalance(std::shared_ptr<Camera> camera, > > > > - StreamRole role, unsigned int numRequests) > > > > -{ > > > > - SimpleCaptureUnbalanced capture(camera); > > > > + capture.configure(role_); > > > > > > > > - Results::Result ret = capture.configure(role); > > > > - if (ret.first != Results::Pass) > > > > - return ret; > > > > + capture.capture(numRequests_); > > > > + }; > > > > +}; > > > > > > > > - return capture.capture(numRequests); > > > > -} > > > > +/* > > > > + * Test multiple start/stop cycles > > > > + * > > > > + * Makes sure the camera supports multiple start/stop cycles. Example failure is > > > > + * a camera that does not clean up correctly in its error path but is only > > > > + * tested by single-capture applications. > > > > + */ > > > > +class BalancedMultiCycle : public SingleStream { > > > > +public: > > > > + explicit BalancedMultiCycle(std::shared_ptr<Camera> camera, StreamRole role, unsigned int numRequests) > > > > + : SingleStream(camera, role, numRequests) {} > > > > > > > > -Results testSingleStream(std::shared_ptr<Camera> camera) > > > > -{ > > > > - static const std::vector<std::pair<std::string, StreamRole>> roles = { > > > > - { "raw", Raw }, > > > > - { "still", StillCapture }, > > > > - { "video", VideoRecording }, > > > > - { "viewfinder", Viewfinder }, > > > > + void TestBody() override { > > > > + unsigned int numRepeats = 3; > > > > + > > > > + SimpleCaptureBalanced capture(camera_); > > > > + > > > > + capture.configure(role_); > > > > + > > > > + for (unsigned int starts = 0; starts < numRepeats; starts++) > > > > + capture.capture(numRequests_); > > > > }; > > > > - static const std::vector<unsigned int> numRequests = { 1, 2, 3, 5, 8, 13, 21, 34, 55, 89 }; > > > > - > > > > - Results results(numRequests.size() * roles.size() * 3); > > > > - > > > > - for (const auto &role : roles) { > > > > - std::cout << "= Test role " << role.first << std::endl; > > > > - /* > > > > - * Test single capture cycles > > > > - * > > > > - * Makes sure the camera completes the exact number of requests queued. > > > > - * Example failure is a camera that needs N+M requests queued to > > > > - * complete N requests to the application. > > > > - */ > > > > - std::cout << "* Test single capture cycles" << std::endl; > > > > - for (unsigned int num : numRequests) > > > > - results.add(testRequestBalance(camera, role.second, 1, num)); > > > > - > > > > - /* > > > > - * Test multiple start/stop cycles > > > > - * > > > > - * Makes sure the camera supports multiple start/stop cycles. > > > > - * Example failure is a camera that does not clean up correctly in its > > > > - * error path but is only tested by single-capture applications. > > > > - */ > > > > - std::cout << "* Test multiple start/stop cycles" << std::endl; > > > > - for (unsigned int num : numRequests) > > > > - results.add(testRequestBalance(camera, role.second, 3, num)); > > > > - > > > > - /* > > > > - * Test unbalanced stop > > > > - * > > > > - * Makes sure the camera supports a stop with requests queued. > > > > - * Example failure is a camera that does not handle cancelation > > > > - * of buffers coming back from the video device while stopping. > > > > - */ > > > > - std::cout << "* Test unbalanced stop" << std::endl; > > > > - for (unsigned int num : numRequests) > > > > - results.add(testRequestUnbalance(camera, role.second, num)); > > > > - } > > > > - > > > > - return results; > > > > +}; > > > > + > > > > +/* > > > > + * Test unbalanced stop > > > > + * > > > > + * Makes sure the camera supports a stop with requests queued. Example failure > > > > + * is a camera that does not handle cancelation of buffers coming back from the > > > > + * video device while stopping. > > > > + */ > > > > +class Unbalanced : public SingleStream { > > > > +public: > > > > + explicit Unbalanced(std::shared_ptr<Camera> camera, StreamRole role, unsigned int numRequests) > > > > + : SingleStream(camera, role, numRequests) {} > > > > + > > > > + void TestBody() override { > > > > + SimpleCaptureUnbalanced capture(camera_); > > > > + > > > > + capture.configure(role_); > > > > + > > > > + capture.capture(numRequests_); > > > > + }; > > > > +}; > > > > + > > > > +void registerSingleStreamTests(std::vector<std::shared_ptr<Camera>> cameras, > > > > + std::map<StreamRole, std::string> roles, std::vector<int> requests) > > > > +{ > > > > + REGISTER_TESTS(SingleStream, BalancedSingleCycle, cameras, roles, requests); > > > > + REGISTER_TESTS(SingleStream, BalancedMultiCycle, cameras, roles, requests); > > > > + REGISTER_TESTS(SingleStream, Unbalanced, cameras, roles, requests); > > > > } > > > > diff --git a/src/lc-compliance/tests.h b/src/lc-compliance/tests.h > > > > index 396605214e4b..6d5a8b88c287 100644 > > > > --- a/src/lc-compliance/tests.h > > > > +++ b/src/lc-compliance/tests.h > > > > @@ -11,6 +11,19 @@ > > > > > > > > #include "results.h" > > > > > > > > -Results testSingleStream(std::shared_ptr<libcamera::Camera> camera); > > > > +void registerSingleStreamTests(std::vector<std::shared_ptr<libcamera::Camera>> cameras, > > > > + std::map<libcamera::StreamRole, std::string> roles, std::vector<int> requests); > > > > + > > > > +#define REGISTER_TESTS(testsuite, testcase, cameras, roles, requests) \ > > > > +for (auto c : cameras) { \ > > > > + for (auto [r, rn] : roles) { \ > > > > + for (auto rq : requests) { \ > > > > + testing::RegisterTest( \ > > > > + #testsuite, (std::string(#testcase) + "/" + c->id() + "/" + rn + "/" + std::to_string(rq)).c_str(), \ > > > > + nullptr, nullptr, __FILE__, __LINE__, \ > > > > + [=]() -> testsuite* { return new testcase(c, r, rq); }); \ > > > > + } \ > > > > + } \ > > > > +} > > > > > > > > #endif /* __LC_COMPLIANCE_TESTS_H__ */ > > > > -- > > > > 2.31.1 > > > > > > > > > > -- > > > Regards, > > > Niklas Söderlund > > > > > > -- > > > To unsubscribe, send mail to kernel-unsubscribe@lists.collabora.co.uk. > > > > > > -- > Regards, > Niklas Söderlund > > -- > To unsubscribe, send mail to kernel-unsubscribe@lists.collabora.co.uk.
Hi Nícolas, Thanks for your work on this. I really think gtest is a step in the right direction! On 2021-05-07 12:52:38 -0300, Nícolas F. R. A. Prado wrote: > Hi Niklas, > > Em 2021-05-07 06:11, Niklas Söderlund escreveu: > > > Hi Nícolas, > > > > On 2021-05-06 17:20:29 -0300, Nícolas F. R. A. Prado wrote: > > > Hi Niklas, > > > > > > thanks for the feedback. > > > > > > Em 2021-05-06 05:48, Niklas Söderlund escreveu: > > > > > > > Hi Nícolas, > > > > > > > > Thanks for your work! > > > > > > > > I really like how gtest helps make the actual tests to be easier to > > > > read. > > > > > > > > On 2021-05-03 16:33:07 -0300, Nícolas F. R. A. Prado wrote: > > > > > Refactor lc-compliance using Googletest as the test framework. > > > > > > > > > > Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com> > > > > > --- > > > > > Changes in v2: > > > > > - Changed from static to dynamic test registration > > > > > - Removed -c flag > > > > > > > > > > There's still an issue with the refactoring that the shared_ptr of the tests > > > > > apparently aren't being deleted on time, which causes: > > > > > > > > > > [20:26:08.744507935] [103243] ERROR DeviceEnumerator device_enumerator.cpp:165 Removing media device /dev/media0 while still in use > > > > > [20:26:08.744548175] [103243] ERROR DeviceEnumerator device_enumerator.cpp:165 Removing media device /dev/media1 while still in use > > > > > Segmentation fault (core dumped) > > > > > > > > > > I tried explicitly resetting the shared_ptr on the destructor of the tests but > > > > > that didn't work. In fact, when just listing the tests, the constructor for the > > > > > tests isn't even called. So I think the issue has to do with the passing of the > > > > > camera shared pointer to the REGISTER_TESTS() macro through that functor (?): > > > > > > > > > > [=]() -> testsuite* { return new testcase(c, r, rq); }); \ > > > > > > > > > > Not sure how to solve this. Any tip would be welcome. > > > > > > > > I will leave this for now as I have a different worry about > > > > REGISTER_TESTS() design below. > > > > > > > > > > > > > > src/lc-compliance/main.cpp | 99 +++++++++--------- > > > > > src/lc-compliance/meson.build | 3 + > > > > > src/lc-compliance/simple_capture.cpp | 74 +++++-------- > > > > > src/lc-compliance/simple_capture.h | 8 +- > > > > > src/lc-compliance/single_stream.cpp | 151 ++++++++++++++------------- > > > > > src/lc-compliance/tests.h | 15 ++- > > > > > 6 files changed, 175 insertions(+), 175 deletions(-) > > > > > > > > > > diff --git a/src/lc-compliance/main.cpp b/src/lc-compliance/main.cpp > > > > > index 54cee54aa978..add0d7729aec 100644 > > > > > --- a/src/lc-compliance/main.cpp > > > > > +++ b/src/lc-compliance/main.cpp > > > > > @@ -9,6 +9,8 @@ > > > > > #include <iostream> > > > > > #include <string.h> > > > > > > > > > > +#include <gtest/gtest.h> > > > > > + > > > > > #include <libcamera/libcamera.h> > > > > > > > > > > #include "../cam/options.h" > > > > > @@ -17,7 +19,6 @@ > > > > > using namespace libcamera; > > > > > > > > > > enum { > > > > > - OptCamera = 'c', > > > > > OptHelp = 'h', > > > > > }; > > > > > > > > > > @@ -28,14 +29,15 @@ public: > > > > > ~Harness(); > > > > > > > > > > int exec(); > > > > > + int init(); > > > > > + void registerTests(); > > > > > > > > > > private: > > > > > - int init(); > > > > > void listCameras(); > > > > > > > > > > OptionsParser::Options options_; > > > > > std::unique_ptr<CameraManager> cm_; > > > > > - std::shared_ptr<Camera> camera_; > > > > > + std::vector<std::shared_ptr<Camera>> cameras_; > > > > > }; > > > > > > > > > > Harness::Harness(const OptionsParser::Options &options) > > > > > @@ -46,33 +48,14 @@ Harness::Harness(const OptionsParser::Options &options) > > > > > > > > > > Harness::~Harness() > > > > > { > > > > > - if (camera_) { > > > > > - camera_->release(); > > > > > - camera_.reset(); > > > > > + for (auto &c : cameras_) { > > > > > + c->release(); > > > > > + c.reset(); > > > > > } > > > > > > > > > > cm_->stop(); > > > > > } > > > > > > > > > > -int Harness::exec() > > > > > -{ > > > > > - int ret = init(); > > > > > - if (ret) > > > > > - return ret; > > > > > - > > > > > - std::vector<Results> results; > > > > > - > > > > > - results.push_back(testSingleStream(camera_)); > > > > > - > > > > > - for (const Results &result : results) { > > > > > - ret = result.summary(); > > > > > - if (ret) > > > > > - return ret; > > > > > - } > > > > > - > > > > > - return 0; > > > > > -} > > > > > - > > > > > int Harness::init() > > > > > { > > > > > int ret = cm_->start(); > > > > > @@ -82,42 +65,26 @@ int Harness::init() > > > > > return ret; > > > > > } > > > > > > > > > > - if (!options_.isSet(OptCamera)) { > > > > > - std::cout << "No camera specified, available cameras:" << std::endl; > > > > > - listCameras(); > > > > > - return -ENODEV; > > > > > - } > > > > > - > > > > > - const std::string &cameraId = options_[OptCamera]; > > > > > - camera_ = cm_->get(cameraId); > > > > > - if (!camera_) { > > > > > - std::cout << "Camera " << cameraId << " not found, available cameras:" << std::endl; > > > > > - listCameras(); > > > > > - return -ENODEV; > > > > > + for (auto cam : cm_->cameras()) { > > > > > + if (cam->acquire()) { > > > > > + std::cout << "Failed to acquire camera" << std::endl; > > > > > + return -EINVAL; > > > > > + } > > > > > + cameras_.push_back(cam); > > > > > > > > I don't like this, I think we need to retain the ability to select which > > > > camera to test. And further down the line maybe allow more then one > > > > camera to be selected if we want to test concurrent use of two specific > > > > ones. But for the work in this series I think you can ignore multiple > > > > cameras. > > > > > > Actually, that doesn't mean that multiple cameras will be tested, that's > > > determined by the test filter, so doing --gtest_filter='.*idCamera1.*' would > > > only test that camera. But I think I see what you mean, I shouldn't be acquiring > > > all cameras, only the ones that I will actually test (and in this code there's a > > > big issue where if a single camera is already busy, even if it won't be tested, > > > lc-compliance as a whole fails). > > > > Ahh I understand. > > > > > > > > > > > > > If I understand the coverletter correctly the correct thing here the cli > > > > arguments should be feed to gtest directly to be able to filter on what > > > > tests are run. Is there support in getst to also pass arguments to > > > > tests? Maybe we could remove the need to parse arguments ourself > > > > completely? > > > > > > There isn't any CLI argument in gtest to pass arguments to tests, but I think we > > > could solve both problems (this and the one above) like this: > > > > > > - In the main(), query the id of each camera from libcamera. > > > - Register each of the tests dynamically in gtest passing as an argument the > > > camera id, rather than a camera pointer (which is the current behavior). > > > - To select which tests to run, and on which camera(s), the user just uses > > > --gtest_filter='testToRun.*idCamera' (or '.*idCamera.*' to run all tests for > > > the camera). > > > - In the startup of each test the camera is acquired for that test, and when > > > it's done it is released. > > > > > > Implications: > > > - We no longer need to parse arguments and can drop that code, as both camera > > > and test are specified directly with gtest's filter. > > > - We only acquire the cameras that we'll actually test and only while they're > > > being tested. > > > - As a downside, a single camera will probably be acquired multiple times during > > > a single run as lc-compliance moves from one test to the next, but I don't > > > think this would be a big issue. (Maybe we could do this at the test suite > > > level instead to greatly decrease the number of times of acquire(), but I'm > > > not sure if it's possible, since we instantiate, by passing the camera id, at > > > the test level, I'll have to test that). > > > > > > What do you think? > > > > I understand what you are trying to do but I see two issues. > > > > 1. It depends on the implicit knowledge of users how to build the filter > > string and is rather unintuitive compared to having two distinct > > arguments, one to select camera and one to express the test filter. > > > > This could possibly be worked around by keeping the cam options code > > and building the gtest filter string internally by injecitng the > > selected camera name in the correct location, but the we have the > > next issue, > > > > 2. I don't think it will be practically possible to express a test > > filter that selects a specific combination of two cameras. > > > > I do think we need to be able to parameterize test cases. If gtest does > > not allow to do so using function arguments there are other ways of > > doing so. > > I think I didn't express myself well. I meant to say that gtest's CLI doesn't > have a flag to allow passing parameters to tests, meaning that it would not be > possible to parametrize the tests from the command line and also remove our > own parser and rely on gtest's. Check, I think we need to keep our own parser for now as we need to be able to parametrize the tests from the CLI. > But gtest does allow test parametrization at runtime and I'm already relying on > it since v2. Let me go a bit more over the way I'm doing this. > > - I have a test defined, eg BalancedSingleCycle, which takes some parameters: > > std::shared_ptr<Camera> camera, StreamRole role, unsigned int numRequests > > - Since we may want to run the same test for all cameras, roles and numRequests, > I register it, at runtime, using my REGISTER_TESTS() macro, which calls > gtest's testing::RegisterTest() with all possible combinations of parameters. > > - Based on the --gtest_filter CLI argument from gtest, it decides which tests > are actually run > > So the key point is that even though I parametrize the tests, I do it for all > combinations in order to later rely on gtest_filter to select which tests to > run. I see, that explains how the parameters get filled. I noticed it but did not dig further, see below for an expansion of my thoughts now that I understand this detail :-) > > Now, I do agree with both of your points. And if we want to make the interface > more intuitive to use (which I also find important), there's no way around > having our own parser. > > And if we're using our own camera flag, then it makes no sense to register tests > for all cameras, we can register tests only with the specified camera. This feels wrong. It feels like tests should be register without a camera component. The camera (and possible other arguments) should come from either arguments [1] or a singleton parameter class [2]. 1. This do not seem to be possible with gtests as test parameters needs to be registerd. 2. This would be similar to the global variable used in earlier versions but instead modeled as a class singleton like CameraManager. Maybe there are other options tho. > > I think here it would be helpful to think if the interface suits our problem. > To me we're looking at having: > > lc-compliance --camera 'cameraId' --filter 'BalancedSingleCycle' This matches my view as well. > > to only test camera 'cameraId' with a single test 'BalancedSingleCycle'. Here, > the camera flag is used to only register tests with that camera, and the filter > is passed as --gtest_filter, possibly a bit adapted, to gtest. > > And if we imagine we have a test ConcurrentCameras, which receives two cameras > as parameters, and we call > > lc-compliance --camera 'cameraId1,cameraId2' This also looks good. But for now lets focus on one camera, I think it will give us enough headache :-) > > it would test both each of the cameras separately in all current tests which > accept a single camera, like BalancedSingleCycle, as well as test both at the > same time in the ConcurrentCameras test (and we could make only the former or > the latter occur by using the filter parameter). I have not thought enough about this to know if that is a good idea or not. At the top of my head I still tend to think that lc-compliance tests a single 'primary' camera and any additional camera supplied are only used for tests where two (or more) cameras are needed to test some specific feature of the primary camera. That is only the 'primary' camera is testes, the other camera(s) are just there to help/provoke the testing. But maybe I'm to narrow minded. > > I know we don't need to implement support for multiple cameras right now, I just > want to make sure it's possible and thinking about the interface and the > scenarios help me picture how they'd work. > > Thoughts? > > Thanks, > Nícolas > > > > > I'm sure there are many solutions to this problem and maybe someone more > > familiar with gtest knows of a neat way to piggyback on its > > infrastrcuture? Since I know very little about the gtest framework the > > following is just an idea and I'm happy to yield. > > > > How about using a singleton class that carries arguments constructed > > from cli arguments (camera id) before the gtest are executed. Each test > > case could then consult this singleton if it needs to access parameters. > > I think you could take inspiration from the libcamera CameraManager > > class. > > > > This could (at lest for now) imply keeping the cam option parser. If so > > I think it would be nice if somewhen before we integrate this work that > > the gtest filter string could be supplied as an argument using that > > parser. Of course if we can do it the other way around and add arguments > > to the gtest parser that would be even cooler as it would be less code > > for us to maintain ;-) > > > > > > > > Thanks, > > > Nícolas > > > > > > > > > > > > } > > > > > > > > > > - if (camera_->acquire()) { > > > > > - std::cout << "Failed to acquire camera" << std::endl; > > > > > - return -EINVAL; > > > > > - } > > > > > - > > > > > - std::cout << "Using camera " << cameraId << std::endl; > > > > > - > > > > > return 0; > > > > > } > > > > > > > > > > void Harness::listCameras() > > > > > { > > > > > - for (const std::shared_ptr<Camera> &cam : cm_->cameras()) > > > > > - std::cout << "- " << cam.get()->id() << std::endl; > > > > > + for (const std::shared_ptr<Camera> &c : cm_->cameras()) > > > > > + std::cout << "- " << c.get()->id() << std::endl; > > > > > } > > > > > > > > > > static int parseOptions(int argc, char **argv, OptionsParser::Options *options) > > > > > { > > > > > OptionsParser parser; > > > > > - parser.addOption(OptCamera, OptionString, > > > > > - "Specify which camera to operate on, by id", "camera", > > > > > - ArgumentRequired, "camera"); > > > > > parser.addOption(OptHelp, OptionNone, "Display this help message", > > > > > "help"); > > > > > > > > > > @@ -133,6 +100,31 @@ static int parseOptions(int argc, char **argv, OptionsParser::Options *options) > > > > > return 0; > > > > > } > > > > > > > > > > +/* > > > > > + * Make asserts act like exceptions, otherwise they only fail (or skip) the > > > > > + * current function. From gtest documentation: > > > > > + * https://google.github.io/googletest/advanced.html#asserting-on-subroutines-with-an-exception > > > > > + */ > > > > > +class ThrowListener : public testing::EmptyTestEventListener { > > > > > + void OnTestPartResult(const testing::TestPartResult& result) override { > > > > > + if (result.type() == testing::TestPartResult::kFatalFailure > > > > > + || result.type() == testing::TestPartResult::kSkip) { > > > > > + throw testing::AssertionException(result); > > > > > + } > > > > > + } > > > > > +}; > > > > > + > > > > > + > > > > > +void Harness::registerTests() { > > > > > + std::map<StreamRole, std::string> roles = {{Raw, "Raw"}, > > > > > + {StillCapture, "Still"}, > > > > > + {VideoRecording, "Video"}, > > > > > + {Viewfinder, "Viewfinder"}}; > > > > > + std::vector<int> requests = {1, 2, 3, 5, 8, 13, 21, 34, 55, 89}; > > > > > + > > > > > + registerSingleStreamTests(cameras_, roles, requests); > > > > > +} > > > > > + > > > > > int main(int argc, char **argv) > > > > > { > > > > > OptionsParser::Options options; > > > > > @@ -143,6 +135,13 @@ int main(int argc, char **argv) > > > > > return EXIT_FAILURE; > > > > > > > > > > Harness harness(options); > > > > > + ret = harness.init(); > > > > > + if (ret) > > > > > + return ret; > > > > > + > > > > > + harness.registerTests(); > > > > > > > > > > - return harness.exec() ? EXIT_FAILURE : EXIT_SUCCESS; > > > > > + ::testing::InitGoogleTest(&argc, argv); > > > > > + testing::UnitTest::GetInstance()->listeners().Append(new ThrowListener); > > > > > + return RUN_ALL_TESTS(); > > > > > } > > > > > diff --git a/src/lc-compliance/meson.build b/src/lc-compliance/meson.build > > > > > index a2bfcceb1259..704bc18af3e1 100644 > > > > > --- a/src/lc-compliance/meson.build > > > > > +++ b/src/lc-compliance/meson.build > > > > > @@ -18,10 +18,13 @@ lc_compliance_sources = files([ > > > > > 'single_stream.cpp', > > > > > ]) > > > > > > > > > > +gtest_dep = dependency('gtest') > > > > > + > > > > > lc_compliance = executable('lc-compliance', lc_compliance_sources, > > > > > dependencies : [ > > > > > libatomic, > > > > > libcamera_dep, > > > > > libevent, > > > > > + gtest_dep, > > > > > ], > > > > > install : true) > > > > > diff --git a/src/lc-compliance/simple_capture.cpp b/src/lc-compliance/simple_capture.cpp > > > > > index f90fe6d0f9aa..7731eb16f8c2 100644 > > > > > --- a/src/lc-compliance/simple_capture.cpp > > > > > +++ b/src/lc-compliance/simple_capture.cpp > > > > > @@ -5,6 +5,8 @@ > > > > > * simple_capture.cpp - Simple capture helper > > > > > */ > > > > > > > > > > +#include <gtest/gtest.h> > > > > > + > > > > > #include "simple_capture.h" > > > > > > > > > > using namespace libcamera; > > > > > @@ -20,38 +22,34 @@ SimpleCapture::~SimpleCapture() > > > > > stop(); > > > > > } > > > > > > > > > > -Results::Result SimpleCapture::configure(StreamRole role) > > > > > +void SimpleCapture::configure(StreamRole role) > > > > > { > > > > > config_ = camera_->generateConfiguration({ role }); > > > > > > > > > > - if (!config_) > > > > > - return { Results::Skip, "Role not supported by camera" }; > > > > > + if (!config_) { > > > > > + std::cout << "Role not supported by camera" << std::endl; > > > > > + GTEST_SKIP(); > > > > > + } > > > > > > > > > > if (config_->validate() != CameraConfiguration::Valid) { > > > > > config_.reset(); > > > > > - return { Results::Fail, "Configuration not valid" }; > > > > > + FAIL() << "Configuration not valid"; > > > > > } > > > > > > > > > > if (camera_->configure(config_.get())) { > > > > > config_.reset(); > > > > > - return { Results::Fail, "Failed to configure camera" }; > > > > > + FAIL() << "Failed to configure camera"; > > > > > } > > > > > - > > > > > - return { Results::Pass, "Configure camera" }; > > > > > } > > > > > > > > > > -Results::Result SimpleCapture::start() > > > > > +void SimpleCapture::start() > > > > > { > > > > > Stream *stream = config_->at(0).stream(); > > > > > - if (allocator_->allocate(stream) < 0) > > > > > - return { Results::Fail, "Failed to allocate buffers" }; > > > > > + ASSERT_GE(allocator_->allocate(stream), 0) << "Failed to allocate buffers"; > > > > > > > > > > - if (camera_->start()) > > > > > - return { Results::Fail, "Failed to start camera" }; > > > > > + ASSERT_TRUE(!camera_->start()) << "Failed to start camera"; > > > > > > > > > > camera_->requestCompleted.connect(this, &SimpleCapture::requestComplete); > > > > > - > > > > > - return { Results::Pass, "Started camera" }; > > > > > } > > > > > > > > > > void SimpleCapture::stop() > > > > > @@ -77,22 +75,19 @@ SimpleCaptureBalanced::SimpleCaptureBalanced(std::shared_ptr<Camera> camera) > > > > > { > > > > > } > > > > > > > > > > -Results::Result SimpleCaptureBalanced::capture(unsigned int numRequests) > > > > > +void SimpleCaptureBalanced::capture(unsigned int numRequests) > > > > > { > > > > > - Results::Result ret = start(); > > > > > - if (ret.first != Results::Pass) > > > > > - return ret; > > > > > + start(); > > > > > > > > > > Stream *stream = config_->at(0).stream(); > > > > > const std::vector<std::unique_ptr<FrameBuffer>> &buffers = allocator_->buffers(stream); > > > > > > > > > > /* No point in testing less requests then the camera depth. */ > > > > > if (buffers.size() > numRequests) { > > > > > - /* Cache buffers.size() before we destroy it in stop() */ > > > > > - int buffers_size = buffers.size(); > > > > > - > > > > > - return { Results::Skip, "Camera needs " + std::to_string(buffers_size) > > > > > - + " requests, can't test only " + std::to_string(numRequests) }; > > > > > + std::cout << "Camera needs " + std::to_string(buffers.size()) > > > > > + + " requests, can't test only " > > > > > + + std::to_string(numRequests) << std::endl; > > > > > + GTEST_SKIP(); > > > > > } > > > > > > > > > > queueCount_ = 0; > > > > > @@ -103,14 +98,11 @@ Results::Result SimpleCaptureBalanced::capture(unsigned int numRequests) > > > > > std::vector<std::unique_ptr<libcamera::Request>> requests; > > > > > for (const std::unique_ptr<FrameBuffer> &buffer : buffers) { > > > > > std::unique_ptr<Request> request = camera_->createRequest(); > > > > > - if (!request) > > > > > - return { Results::Fail, "Can't create request" }; > > > > > + ASSERT_TRUE(request) << "Can't create request"; > > > > > > > > > > - if (request->addBuffer(stream, buffer.get())) > > > > > - return { Results::Fail, "Can't set buffer for request" }; > > > > > + ASSERT_FALSE(request->addBuffer(stream, buffer.get())) << "Can't set buffer for request"; > > > > > > > > > > - if (queueRequest(request.get()) < 0) > > > > > - return { Results::Fail, "Failed to queue request" }; > > > > > + ASSERT_GE(queueRequest(request.get()), 0) << "Failed to queue request"; > > > > > > > > > > requests.push_back(std::move(request)); > > > > > } > > > > > @@ -121,12 +113,7 @@ Results::Result SimpleCaptureBalanced::capture(unsigned int numRequests) > > > > > stop(); > > > > > delete loop_; > > > > > > > > > > - if (captureCount_ != captureLimit_) > > > > > - return { Results::Fail, "Got " + std::to_string(captureCount_) + > > > > > - " request, wanted " + std::to_string(captureLimit_) }; > > > > > - > > > > > - return { Results::Pass, "Balanced capture of " + > > > > > - std::to_string(numRequests) + " requests" }; > > > > > + ASSERT_EQ(captureCount_, captureLimit_); > > > > > } > > > > > > > > > > int SimpleCaptureBalanced::queueRequest(Request *request) > > > > > @@ -158,11 +145,9 @@ SimpleCaptureUnbalanced::SimpleCaptureUnbalanced(std::shared_ptr<Camera> camera) > > > > > { > > > > > } > > > > > > > > > > -Results::Result SimpleCaptureUnbalanced::capture(unsigned int numRequests) > > > > > +void SimpleCaptureUnbalanced::capture(unsigned int numRequests) > > > > > { > > > > > - Results::Result ret = start(); > > > > > - if (ret.first != Results::Pass) > > > > > - return ret; > > > > > + start(); > > > > > > > > > > Stream *stream = config_->at(0).stream(); > > > > > const std::vector<std::unique_ptr<FrameBuffer>> &buffers = allocator_->buffers(stream); > > > > > @@ -174,14 +159,11 @@ Results::Result SimpleCaptureUnbalanced::capture(unsigned int numRequests) > > > > > std::vector<std::unique_ptr<libcamera::Request>> requests; > > > > > for (const std::unique_ptr<FrameBuffer> &buffer : buffers) { > > > > > std::unique_ptr<Request> request = camera_->createRequest(); > > > > > - if (!request) > > > > > - return { Results::Fail, "Can't create request" }; > > > > > + ASSERT_TRUE(request) << "Can't create request"; > > > > > > > > > > - if (request->addBuffer(stream, buffer.get())) > > > > > - return { Results::Fail, "Can't set buffer for request" }; > > > > > + ASSERT_FALSE(request->addBuffer(stream, buffer.get())) << "Can't set buffer for request"; > > > > > > > > > > - if (camera_->queueRequest(request.get()) < 0) > > > > > - return { Results::Fail, "Failed to queue request" }; > > > > > + ASSERT_GE(camera_->queueRequest(request.get()), 0) << "Failed to queue request"; > > > > > > > > > > requests.push_back(std::move(request)); > > > > > } > > > > > @@ -192,7 +174,7 @@ Results::Result SimpleCaptureUnbalanced::capture(unsigned int numRequests) > > > > > stop(); > > > > > delete loop_; > > > > > > > > > > - return { status ? Results::Fail : Results::Pass, "Unbalanced capture of " + std::to_string(numRequests) + " requests" }; > > > > > + ASSERT_FALSE(status); > > > > > } > > > > > > > > > > void SimpleCaptureUnbalanced::requestComplete(Request *request) > > > > > diff --git a/src/lc-compliance/simple_capture.h b/src/lc-compliance/simple_capture.h > > > > > index d9de53fb63a3..0f8465083456 100644 > > > > > --- a/src/lc-compliance/simple_capture.h > > > > > +++ b/src/lc-compliance/simple_capture.h > > > > > @@ -17,13 +17,13 @@ > > > > > class SimpleCapture > > > > > { > > > > > public: > > > > > - Results::Result configure(libcamera::StreamRole role); > > > > > + void configure(libcamera::StreamRole role); > > > > > > > > > > protected: > > > > > SimpleCapture(std::shared_ptr<libcamera::Camera> camera); > > > > > virtual ~SimpleCapture(); > > > > > > > > > > - Results::Result start(); > > > > > + void start(); > > > > > void stop(); > > > > > > > > > > virtual void requestComplete(libcamera::Request *request) = 0; > > > > > @@ -40,7 +40,7 @@ class SimpleCaptureBalanced : public SimpleCapture > > > > > public: > > > > > SimpleCaptureBalanced(std::shared_ptr<libcamera::Camera> camera); > > > > > > > > > > - Results::Result capture(unsigned int numRequests); > > > > > + void capture(unsigned int numRequests); > > > > > > > > > > private: > > > > > int queueRequest(libcamera::Request *request); > > > > > @@ -56,7 +56,7 @@ class SimpleCaptureUnbalanced : public SimpleCapture > > > > > public: > > > > > SimpleCaptureUnbalanced(std::shared_ptr<libcamera::Camera> camera); > > > > > > > > > > - Results::Result capture(unsigned int numRequests); > > > > > + void capture(unsigned int numRequests); > > > > > > > > > > private: > > > > > void requestComplete(libcamera::Request *request) override; > > > > > diff --git a/src/lc-compliance/single_stream.cpp b/src/lc-compliance/single_stream.cpp > > > > > index 8318b42f42d6..eb6a6f305826 100644 > > > > > --- a/src/lc-compliance/single_stream.cpp > > > > > +++ b/src/lc-compliance/single_stream.cpp > > > > > @@ -7,91 +7,94 @@ > > > > > > > > > > #include <iostream> > > > > > > > > > > +#include <gtest/gtest.h> > > > > > + > > > > > #include "simple_capture.h" > > > > > #include "tests.h" > > > > > > > > > > using namespace libcamera; > > > > > > > > > > -Results::Result testRequestBalance(std::shared_ptr<Camera> camera, > > > > > - StreamRole role, unsigned int startCycles, > > > > > - unsigned int numRequests) > > > > > -{ > > > > > - SimpleCaptureBalanced capture(camera); > > > > > +class SingleStream : public testing::Test { > > > > > +public: > > > > > + explicit SingleStream(std::shared_ptr<Camera> camera, StreamRole role, unsigned int numRequests) > > > > > + : camera_(camera), role_(role), numRequests_(numRequests) {} > > > > > > > > > > - Results::Result ret = capture.configure(role); > > > > > - if (ret.first != Results::Pass) > > > > > - return ret; > > > > > +protected: > > > > > + std::shared_ptr<Camera> camera_; > > > > > + StreamRole role_; > > > > > + unsigned int numRequests_; > > > > > +}; > > > > > > > > > > - for (unsigned int starts = 0; starts < startCycles; starts++) { > > > > > - ret = capture.capture(numRequests); > > > > > - if (ret.first != Results::Pass) > > > > > - return ret; > > > > > - } > > > > > +/* > > > > > + * Test single capture cycles > > > > > + * > > > > > + * Makes sure the camera completes the exact number of requests queued. Example > > > > > + * failure is a camera that needs N+M requests queued to complete N requests to > > > > > + * the application. > > > > > + */ > > > > > +class BalancedSingleCycle : public SingleStream { > > > > > +public: > > > > > + explicit BalancedSingleCycle(std::shared_ptr<Camera> camera, StreamRole role, unsigned int numRequests) > > > > > + : SingleStream(camera, role, numRequests) {} > > > > > > > > > > - return { Results::Pass, "Balanced capture of " + > > > > > - std::to_string(numRequests) + " requests with " + > > > > > - std::to_string(startCycles) + " start cycles" }; > > > > > -} > > > > > + void TestBody() override { > > > > > + SimpleCaptureBalanced capture(camera_); > > > > > > > > > > -Results::Result testRequestUnbalance(std::shared_ptr<Camera> camera, > > > > > - StreamRole role, unsigned int numRequests) > > > > > -{ > > > > > - SimpleCaptureUnbalanced capture(camera); > > > > > + capture.configure(role_); > > > > > > > > > > - Results::Result ret = capture.configure(role); > > > > > - if (ret.first != Results::Pass) > > > > > - return ret; > > > > > + capture.capture(numRequests_); > > > > > + }; > > > > > +}; > > > > > > > > > > - return capture.capture(numRequests); > > > > > -} > > > > > +/* > > > > > + * Test multiple start/stop cycles > > > > > + * > > > > > + * Makes sure the camera supports multiple start/stop cycles. Example failure is > > > > > + * a camera that does not clean up correctly in its error path but is only > > > > > + * tested by single-capture applications. > > > > > + */ > > > > > +class BalancedMultiCycle : public SingleStream { > > > > > +public: > > > > > + explicit BalancedMultiCycle(std::shared_ptr<Camera> camera, StreamRole role, unsigned int numRequests) > > > > > + : SingleStream(camera, role, numRequests) {} > > > > > > > > > > -Results testSingleStream(std::shared_ptr<Camera> camera) > > > > > -{ > > > > > - static const std::vector<std::pair<std::string, StreamRole>> roles = { > > > > > - { "raw", Raw }, > > > > > - { "still", StillCapture }, > > > > > - { "video", VideoRecording }, > > > > > - { "viewfinder", Viewfinder }, > > > > > + void TestBody() override { > > > > > + unsigned int numRepeats = 3; > > > > > + > > > > > + SimpleCaptureBalanced capture(camera_); > > > > > + > > > > > + capture.configure(role_); > > > > > + > > > > > + for (unsigned int starts = 0; starts < numRepeats; starts++) > > > > > + capture.capture(numRequests_); > > > > > }; > > > > > - static const std::vector<unsigned int> numRequests = { 1, 2, 3, 5, 8, 13, 21, 34, 55, 89 }; > > > > > - > > > > > - Results results(numRequests.size() * roles.size() * 3); > > > > > - > > > > > - for (const auto &role : roles) { > > > > > - std::cout << "= Test role " << role.first << std::endl; > > > > > - /* > > > > > - * Test single capture cycles > > > > > - * > > > > > - * Makes sure the camera completes the exact number of requests queued. > > > > > - * Example failure is a camera that needs N+M requests queued to > > > > > - * complete N requests to the application. > > > > > - */ > > > > > - std::cout << "* Test single capture cycles" << std::endl; > > > > > - for (unsigned int num : numRequests) > > > > > - results.add(testRequestBalance(camera, role.second, 1, num)); > > > > > - > > > > > - /* > > > > > - * Test multiple start/stop cycles > > > > > - * > > > > > - * Makes sure the camera supports multiple start/stop cycles. > > > > > - * Example failure is a camera that does not clean up correctly in its > > > > > - * error path but is only tested by single-capture applications. > > > > > - */ > > > > > - std::cout << "* Test multiple start/stop cycles" << std::endl; > > > > > - for (unsigned int num : numRequests) > > > > > - results.add(testRequestBalance(camera, role.second, 3, num)); > > > > > - > > > > > - /* > > > > > - * Test unbalanced stop > > > > > - * > > > > > - * Makes sure the camera supports a stop with requests queued. > > > > > - * Example failure is a camera that does not handle cancelation > > > > > - * of buffers coming back from the video device while stopping. > > > > > - */ > > > > > - std::cout << "* Test unbalanced stop" << std::endl; > > > > > - for (unsigned int num : numRequests) > > > > > - results.add(testRequestUnbalance(camera, role.second, num)); > > > > > - } > > > > > - > > > > > - return results; > > > > > +}; > > > > > + > > > > > +/* > > > > > + * Test unbalanced stop > > > > > + * > > > > > + * Makes sure the camera supports a stop with requests queued. Example failure > > > > > + * is a camera that does not handle cancelation of buffers coming back from the > > > > > + * video device while stopping. > > > > > + */ > > > > > +class Unbalanced : public SingleStream { > > > > > +public: > > > > > + explicit Unbalanced(std::shared_ptr<Camera> camera, StreamRole role, unsigned int numRequests) > > > > > + : SingleStream(camera, role, numRequests) {} > > > > > + > > > > > + void TestBody() override { > > > > > + SimpleCaptureUnbalanced capture(camera_); > > > > > + > > > > > + capture.configure(role_); > > > > > + > > > > > + capture.capture(numRequests_); > > > > > + }; > > > > > +}; > > > > > + > > > > > +void registerSingleStreamTests(std::vector<std::shared_ptr<Camera>> cameras, > > > > > + std::map<StreamRole, std::string> roles, std::vector<int> requests) > > > > > +{ > > > > > + REGISTER_TESTS(SingleStream, BalancedSingleCycle, cameras, roles, requests); > > > > > + REGISTER_TESTS(SingleStream, BalancedMultiCycle, cameras, roles, requests); > > > > > + REGISTER_TESTS(SingleStream, Unbalanced, cameras, roles, requests); > > > > > } > > > > > diff --git a/src/lc-compliance/tests.h b/src/lc-compliance/tests.h > > > > > index 396605214e4b..6d5a8b88c287 100644 > > > > > --- a/src/lc-compliance/tests.h > > > > > +++ b/src/lc-compliance/tests.h > > > > > @@ -11,6 +11,19 @@ > > > > > > > > > > #include "results.h" > > > > > > > > > > -Results testSingleStream(std::shared_ptr<libcamera::Camera> camera); > > > > > +void registerSingleStreamTests(std::vector<std::shared_ptr<libcamera::Camera>> cameras, > > > > > + std::map<libcamera::StreamRole, std::string> roles, std::vector<int> requests); > > > > > + > > > > > +#define REGISTER_TESTS(testsuite, testcase, cameras, roles, requests) \ > > > > > +for (auto c : cameras) { \ > > > > > + for (auto [r, rn] : roles) { \ > > > > > + for (auto rq : requests) { \ > > > > > + testing::RegisterTest( \ > > > > > + #testsuite, (std::string(#testcase) + "/" + c->id() + "/" + rn + "/" + std::to_string(rq)).c_str(), \ > > > > > + nullptr, nullptr, __FILE__, __LINE__, \ > > > > > + [=]() -> testsuite* { return new testcase(c, r, rq); }); \ > > > > > + } \ > > > > > + } \ > > > > > +} > > > > > > > > > > #endif /* __LC_COMPLIANCE_TESTS_H__ */ > > > > > -- > > > > > 2.31.1 > > > > > > > > > > > > > -- > > > > Regards, > > > > Niklas Söderlund > > > > > > > > -- > > > > To unsubscribe, send mail to kernel-unsubscribe@lists.collabora.co.uk. > > > > > > > > > > -- > > Regards, > > Niklas Söderlund > > > > -- > > To unsubscribe, send mail to kernel-unsubscribe@lists.collabora.co.uk. > >
Hi Niklas, Em 2021-05-10 18:04, Niklas Söderlund escreveu: > Hi Nícolas, > > Thanks for your work on this. I really think gtest is a step in the > right direction! > > On 2021-05-07 12:52:38 -0300, Nícolas F. R. A. Prado wrote: > > Hi Niklas, > > > > Em 2021-05-07 06:11, Niklas Söderlund escreveu: > > > > > Hi Nícolas, > > > > > > On 2021-05-06 17:20:29 -0300, Nícolas F. R. A. Prado wrote: > > > > Hi Niklas, > > > > > > > > thanks for the feedback. > > > > > > > > Em 2021-05-06 05:48, Niklas Söderlund escreveu: > > > > > > > > > Hi Nícolas, > > > > > > > > > > Thanks for your work! > > > > > > > > > > I really like how gtest helps make the actual tests to be easier to > > > > > read. > > > > > > > > > > On 2021-05-03 16:33:07 -0300, Nícolas F. R. A. Prado wrote: > > > > > > Refactor lc-compliance using Googletest as the test framework. > > > > > > > > > > > > Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com> > > > > > > --- > > > > > > Changes in v2: > > > > > > - Changed from static to dynamic test registration > > > > > > - Removed -c flag > > > > > > > > > > > > There's still an issue with the refactoring that the shared_ptr of the tests > > > > > > apparently aren't being deleted on time, which causes: > > > > > > > > > > > > [20:26:08.744507935] [103243] ERROR DeviceEnumerator device_enumerator.cpp:165 Removing media device /dev/media0 while still in use > > > > > > [20:26:08.744548175] [103243] ERROR DeviceEnumerator device_enumerator.cpp:165 Removing media device /dev/media1 while still in use > > > > > > Segmentation fault (core dumped) > > > > > > > > > > > > I tried explicitly resetting the shared_ptr on the destructor of the tests but > > > > > > that didn't work. In fact, when just listing the tests, the constructor for the > > > > > > tests isn't even called. So I think the issue has to do with the passing of the > > > > > > camera shared pointer to the REGISTER_TESTS() macro through that functor (?): > > > > > > > > > > > > [=]() -> testsuite* { return new testcase(c, r, rq); }); \ > > > > > > > > > > > > Not sure how to solve this. Any tip would be welcome. > > > > > > > > > > I will leave this for now as I have a different worry about > > > > > REGISTER_TESTS() design below. > > > > > > > > > > > > > > > > > src/lc-compliance/main.cpp | 99 +++++++++--------- > > > > > > src/lc-compliance/meson.build | 3 + > > > > > > src/lc-compliance/simple_capture.cpp | 74 +++++-------- > > > > > > src/lc-compliance/simple_capture.h | 8 +- > > > > > > src/lc-compliance/single_stream.cpp | 151 ++++++++++++++------------- > > > > > > src/lc-compliance/tests.h | 15 ++- > > > > > > 6 files changed, 175 insertions(+), 175 deletions(-) > > > > > > > > > > > > diff --git a/src/lc-compliance/main.cpp b/src/lc-compliance/main.cpp > > > > > > index 54cee54aa978..add0d7729aec 100644 > > > > > > --- a/src/lc-compliance/main.cpp > > > > > > +++ b/src/lc-compliance/main.cpp > > > > > > @@ -9,6 +9,8 @@ > > > > > > #include <iostream> > > > > > > #include <string.h> > > > > > > > > > > > > +#include <gtest/gtest.h> > > > > > > + > > > > > > #include <libcamera/libcamera.h> > > > > > > > > > > > > #include "../cam/options.h" > > > > > > @@ -17,7 +19,6 @@ > > > > > > using namespace libcamera; > > > > > > > > > > > > enum { > > > > > > - OptCamera = 'c', > > > > > > OptHelp = 'h', > > > > > > }; > > > > > > > > > > > > @@ -28,14 +29,15 @@ public: > > > > > > ~Harness(); > > > > > > > > > > > > int exec(); > > > > > > + int init(); > > > > > > + void registerTests(); > > > > > > > > > > > > private: > > > > > > - int init(); > > > > > > void listCameras(); > > > > > > > > > > > > OptionsParser::Options options_; > > > > > > std::unique_ptr<CameraManager> cm_; > > > > > > - std::shared_ptr<Camera> camera_; > > > > > > + std::vector<std::shared_ptr<Camera>> cameras_; > > > > > > }; > > > > > > > > > > > > Harness::Harness(const OptionsParser::Options &options) > > > > > > @@ -46,33 +48,14 @@ Harness::Harness(const OptionsParser::Options &options) > > > > > > > > > > > > Harness::~Harness() > > > > > > { > > > > > > - if (camera_) { > > > > > > - camera_->release(); > > > > > > - camera_.reset(); > > > > > > + for (auto &c : cameras_) { > > > > > > + c->release(); > > > > > > + c.reset(); > > > > > > } > > > > > > > > > > > > cm_->stop(); > > > > > > } > > > > > > > > > > > > -int Harness::exec() > > > > > > -{ > > > > > > - int ret = init(); > > > > > > - if (ret) > > > > > > - return ret; > > > > > > - > > > > > > - std::vector<Results> results; > > > > > > - > > > > > > - results.push_back(testSingleStream(camera_)); > > > > > > - > > > > > > - for (const Results &result : results) { > > > > > > - ret = result.summary(); > > > > > > - if (ret) > > > > > > - return ret; > > > > > > - } > > > > > > - > > > > > > - return 0; > > > > > > -} > > > > > > - > > > > > > int Harness::init() > > > > > > { > > > > > > int ret = cm_->start(); > > > > > > @@ -82,42 +65,26 @@ int Harness::init() > > > > > > return ret; > > > > > > } > > > > > > > > > > > > - if (!options_.isSet(OptCamera)) { > > > > > > - std::cout << "No camera specified, available cameras:" << std::endl; > > > > > > - listCameras(); > > > > > > - return -ENODEV; > > > > > > - } > > > > > > - > > > > > > - const std::string &cameraId = options_[OptCamera]; > > > > > > - camera_ = cm_->get(cameraId); > > > > > > - if (!camera_) { > > > > > > - std::cout << "Camera " << cameraId << " not found, available cameras:" << std::endl; > > > > > > - listCameras(); > > > > > > - return -ENODEV; > > > > > > + for (auto cam : cm_->cameras()) { > > > > > > + if (cam->acquire()) { > > > > > > + std::cout << "Failed to acquire camera" << std::endl; > > > > > > + return -EINVAL; > > > > > > + } > > > > > > + cameras_.push_back(cam); > > > > > > > > > > I don't like this, I think we need to retain the ability to select which > > > > > camera to test. And further down the line maybe allow more then one > > > > > camera to be selected if we want to test concurrent use of two specific > > > > > ones. But for the work in this series I think you can ignore multiple > > > > > cameras. > > > > > > > > Actually, that doesn't mean that multiple cameras will be tested, that's > > > > determined by the test filter, so doing --gtest_filter='.*idCamera1.*' would > > > > only test that camera. But I think I see what you mean, I shouldn't be acquiring > > > > all cameras, only the ones that I will actually test (and in this code there's a > > > > big issue where if a single camera is already busy, even if it won't be tested, > > > > lc-compliance as a whole fails). > > > > > > Ahh I understand. > > > > > > > > > > > > > > > > > If I understand the coverletter correctly the correct thing here the cli > > > > > arguments should be feed to gtest directly to be able to filter on what > > > > > tests are run. Is there support in getst to also pass arguments to > > > > > tests? Maybe we could remove the need to parse arguments ourself > > > > > completely? > > > > > > > > There isn't any CLI argument in gtest to pass arguments to tests, but I think we > > > > could solve both problems (this and the one above) like this: > > > > > > > > - In the main(), query the id of each camera from libcamera. > > > > - Register each of the tests dynamically in gtest passing as an argument the > > > > camera id, rather than a camera pointer (which is the current behavior). > > > > - To select which tests to run, and on which camera(s), the user just uses > > > > --gtest_filter='testToRun.*idCamera' (or '.*idCamera.*' to run all tests for > > > > the camera). > > > > - In the startup of each test the camera is acquired for that test, and when > > > > it's done it is released. > > > > > > > > Implications: > > > > - We no longer need to parse arguments and can drop that code, as both camera > > > > and test are specified directly with gtest's filter. > > > > - We only acquire the cameras that we'll actually test and only while they're > > > > being tested. > > > > - As a downside, a single camera will probably be acquired multiple times during > > > > a single run as lc-compliance moves from one test to the next, but I don't > > > > think this would be a big issue. (Maybe we could do this at the test suite > > > > level instead to greatly decrease the number of times of acquire(), but I'm > > > > not sure if it's possible, since we instantiate, by passing the camera id, at > > > > the test level, I'll have to test that). > > > > > > > > What do you think? > > > > > > I understand what you are trying to do but I see two issues. > > > > > > 1. It depends on the implicit knowledge of users how to build the filter > > > string and is rather unintuitive compared to having two distinct > > > arguments, one to select camera and one to express the test filter. > > > > > > This could possibly be worked around by keeping the cam options code > > > and building the gtest filter string internally by injecitng the > > > selected camera name in the correct location, but the we have the > > > next issue, > > > > > > 2. I don't think it will be practically possible to express a test > > > filter that selects a specific combination of two cameras. > > > > > > I do think we need to be able to parameterize test cases. If gtest does > > > not allow to do so using function arguments there are other ways of > > > doing so. > > > > I think I didn't express myself well. I meant to say that gtest's CLI doesn't > > have a flag to allow passing parameters to tests, meaning that it would not be > > possible to parametrize the tests from the command line and also remove our > > own parser and rely on gtest's. > > Check, I think we need to keep our own parser for now as we need to be > able to parametrize the tests from the CLI. > > > But gtest does allow test parametrization at runtime and I'm already relying on > > it since v2. Let me go a bit more over the way I'm doing this. > > > > - I have a test defined, eg BalancedSingleCycle, which takes some parameters: > > > > std::shared_ptr<Camera> camera, StreamRole role, unsigned int numRequests > > > > - Since we may want to run the same test for all cameras, roles and numRequests, > > I register it, at runtime, using my REGISTER_TESTS() macro, which calls > > gtest's testing::RegisterTest() with all possible combinations of parameters. > > > > - Based on the --gtest_filter CLI argument from gtest, it decides which tests > > are actually run > > > > So the key point is that even though I parametrize the tests, I do it for all > > combinations in order to later rely on gtest_filter to select which tests to > > run. > > I see, that explains how the parameters get filled. I noticed it but did > not dig further, see below for an expansion of my thoughts now that I > understand this detail :-) > > > > > Now, I do agree with both of your points. And if we want to make the interface > > more intuitive to use (which I also find important), there's no way around > > having our own parser. > > > > And if we're using our own camera flag, then it makes no sense to register tests > > for all cameras, we can register tests only with the specified camera. > > This feels wrong. It feels like tests should be register without a > camera component. The camera (and possible other arguments) should come > from either arguments [1] or a singleton parameter class [2]. > > 1. This do not seem to be possible with gtests as test parameters needs > to be registerd. > > 2. This would be similar to the global variable used in earlier versions > but instead modeled as a class singleton like CameraManager. > > Maybe there are other options tho. > > > > > I think here it would be helpful to think if the interface suits our problem. > > To me we're looking at having: > > > > lc-compliance --camera 'cameraId' --filter 'BalancedSingleCycle' > > This matches my view as well. > > > > > to only test camera 'cameraId' with a single test 'BalancedSingleCycle'. Here, > > the camera flag is used to only register tests with that camera, and the filter > > is passed as --gtest_filter, possibly a bit adapted, to gtest. > > > > And if we imagine we have a test ConcurrentCameras, which receives two cameras > > as parameters, and we call > > > > lc-compliance --camera 'cameraId1,cameraId2' > > This also looks good. But for now lets focus on one camera, I think it > will give us enough headache :-) > > > > > it would test both each of the cameras separately in all current tests which > > accept a single camera, like BalancedSingleCycle, as well as test both at the > > same time in the ConcurrentCameras test (and we could make only the former or > > the latter occur by using the filter parameter). > > I have not thought enough about this to know if that is a good idea or > not. At the top of my head I still tend to think that lc-compliance > tests a single 'primary' camera and any additional camera supplied are > only used for tests where two (or more) cameras are needed to test some > specific feature of the primary camera. > > That is only the 'primary' camera is testes, the other camera(s) are > just there to help/provoke the testing. But maybe I'm to narrow minded. Okay, so it now comes down to how lc-compliance will get used. Either: 1. lc-compliance will be called to test a single camera, with optionally other secondary cameras 2. lc-compliance will be called to test multiple cameras at once (not necessarily simultaneously, but in a single run), with the default case being "Test all the cameras in this system". Given that the final objective of my internship is to have lc-compliance running on real boards by KernelCI, to catch any kernel regressions that affect camera usage, I'm trying to think how it would work there. (I wonder if this discussion should be happening together with KernelCI to decide on this...) So I'm imagining a RockPi4 board with a camera sensor plugged in, running lc-compliance to catch any kernel regressions. The limitation I see in 1 is that if a system has multiple cameras and want to ensure that all keep working, it will need to either hardcode all camera ids and manually call lc-compliance once for each one, or have a separate program that lists all cameras from libcamera (using 'cam -l' for example) and calls lc-compliance with each one. That's to say, running on all cameras is not as easy. And then there are multiple reports, one for each camera, instead of a single one for all cameras, which might be good or bad, not sure. That said, since the RockPi4 (and also the Raspberry Pi 4) has a single CSI-2 connector, I believe it only supports a single camera anyway, so this shouldn't be a problem for this board, but I'm not sure about others. Phones for example seem to have at least two independent cameras... Regarding the benefit of 1, I'm not sure. If we were to use a global variable like in v1, it would make the code a lot simpler by making the test registration static. But that doesn't seem to be an option :). I'm not familiar with using a Singleton, so I'll take a look at CameraManager. But I assume it wouldn't be accessible statically, so the dynamic test registration complexities would have to stay. Also, just to make sure we're on the same page, by using a Singleton, and again assuming it isn't stored in a global variable to be accessible from the tests statically, even though we would eliminate the camera as a parameter for the tests, the reference to the singleton itself would need to be added as a parameter for the tests. Thanks, Nícolas > > > > > I know we don't need to implement support for multiple cameras right now, I just > > want to make sure it's possible and thinking about the interface and the > > scenarios help me picture how they'd work. > > > > Thoughts? > > > > Thanks, > > Nícolas > > > > > > > > I'm sure there are many solutions to this problem and maybe someone more > > > familiar with gtest knows of a neat way to piggyback on its > > > infrastrcuture? Since I know very little about the gtest framework the > > > following is just an idea and I'm happy to yield. > > > > > > How about using a singleton class that carries arguments constructed > > > from cli arguments (camera id) before the gtest are executed. Each test > > > case could then consult this singleton if it needs to access parameters. > > > I think you could take inspiration from the libcamera CameraManager > > > class. > > > > > > This could (at lest for now) imply keeping the cam option parser. If so > > > I think it would be nice if somewhen before we integrate this work that > > > the gtest filter string could be supplied as an argument using that > > > parser. Of course if we can do it the other way around and add arguments > > > to the gtest parser that would be even cooler as it would be less code > > > for us to maintain ;-)
Hello again, Em 2021-05-11 14:17, Nícolas F. R. A. Prado escreveu: > Hi Niklas, > > Em 2021-05-10 18:04, Niklas Söderlund escreveu: > > > Hi Nícolas, > > > > Thanks for your work on this. I really think gtest is a step in the > > right direction! > > > > On 2021-05-07 12:52:38 -0300, Nícolas F. R. A. Prado wrote: > > > Hi Niklas, > > > > > > Em 2021-05-07 06:11, Niklas Söderlund escreveu: > > > > > > > Hi Nícolas, > > > > > > > > On 2021-05-06 17:20:29 -0300, Nícolas F. R. A. Prado wrote: > > > > > Hi Niklas, > > > > > > > > > > thanks for the feedback. > > > > > > > > > > Em 2021-05-06 05:48, Niklas Söderlund escreveu: > > > > > > > > > > > Hi Nícolas, > > > > > > > > > > > > Thanks for your work! > > > > > > > > > > > > I really like how gtest helps make the actual tests to be easier to > > > > > > read. > > > > > > > > > > > > On 2021-05-03 16:33:07 -0300, Nícolas F. R. A. Prado wrote: > > > > > > > Refactor lc-compliance using Googletest as the test framework. > > > > > > > > > > > > > > Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com> > > > > > > > --- > > > > > > > Changes in v2: > > > > > > > - Changed from static to dynamic test registration > > > > > > > - Removed -c flag > > > > > > > > > > > > > > There's still an issue with the refactoring that the shared_ptr of the tests > > > > > > > apparently aren't being deleted on time, which causes: > > > > > > > > > > > > > > [20:26:08.744507935] [103243] ERROR DeviceEnumerator device_enumerator.cpp:165 Removing media device /dev/media0 while still in use > > > > > > > [20:26:08.744548175] [103243] ERROR DeviceEnumerator device_enumerator.cpp:165 Removing media device /dev/media1 while still in use > > > > > > > Segmentation fault (core dumped) > > > > > > > > > > > > > > I tried explicitly resetting the shared_ptr on the destructor of the tests but > > > > > > > that didn't work. In fact, when just listing the tests, the constructor for the > > > > > > > tests isn't even called. So I think the issue has to do with the passing of the > > > > > > > camera shared pointer to the REGISTER_TESTS() macro through that functor (?): > > > > > > > > > > > > > > [=]() -> testsuite* { return new testcase(c, r, rq); }); \ > > > > > > > > > > > > > > Not sure how to solve this. Any tip would be welcome. > > > > > > > > > > > > I will leave this for now as I have a different worry about > > > > > > REGISTER_TESTS() design below. > > > > > > > > > > > > > > > > > > > > src/lc-compliance/main.cpp | 99 +++++++++--------- > > > > > > > src/lc-compliance/meson.build | 3 + > > > > > > > src/lc-compliance/simple_capture.cpp | 74 +++++-------- > > > > > > > src/lc-compliance/simple_capture.h | 8 +- > > > > > > > src/lc-compliance/single_stream.cpp | 151 ++++++++++++++------------- > > > > > > > src/lc-compliance/tests.h | 15 ++- > > > > > > > 6 files changed, 175 insertions(+), 175 deletions(-) > > > > > > > > > > > > > > diff --git a/src/lc-compliance/main.cpp b/src/lc-compliance/main.cpp > > > > > > > index 54cee54aa978..add0d7729aec 100644 > > > > > > > --- a/src/lc-compliance/main.cpp > > > > > > > +++ b/src/lc-compliance/main.cpp > > > > > > > @@ -9,6 +9,8 @@ > > > > > > > #include <iostream> > > > > > > > #include <string.h> > > > > > > > > > > > > > > +#include <gtest/gtest.h> > > > > > > > + > > > > > > > #include <libcamera/libcamera.h> > > > > > > > > > > > > > > #include "../cam/options.h" > > > > > > > @@ -17,7 +19,6 @@ > > > > > > > using namespace libcamera; > > > > > > > > > > > > > > enum { > > > > > > > - OptCamera = 'c', > > > > > > > OptHelp = 'h', > > > > > > > }; > > > > > > > > > > > > > > @@ -28,14 +29,15 @@ public: > > > > > > > ~Harness(); > > > > > > > > > > > > > > int exec(); > > > > > > > + int init(); > > > > > > > + void registerTests(); > > > > > > > > > > > > > > private: > > > > > > > - int init(); > > > > > > > void listCameras(); > > > > > > > > > > > > > > OptionsParser::Options options_; > > > > > > > std::unique_ptr<CameraManager> cm_; > > > > > > > - std::shared_ptr<Camera> camera_; > > > > > > > + std::vector<std::shared_ptr<Camera>> cameras_; > > > > > > > }; > > > > > > > > > > > > > > Harness::Harness(const OptionsParser::Options &options) > > > > > > > @@ -46,33 +48,14 @@ Harness::Harness(const OptionsParser::Options &options) > > > > > > > > > > > > > > Harness::~Harness() > > > > > > > { > > > > > > > - if (camera_) { > > > > > > > - camera_->release(); > > > > > > > - camera_.reset(); > > > > > > > + for (auto &c : cameras_) { > > > > > > > + c->release(); > > > > > > > + c.reset(); > > > > > > > } > > > > > > > > > > > > > > cm_->stop(); > > > > > > > } > > > > > > > > > > > > > > -int Harness::exec() > > > > > > > -{ > > > > > > > - int ret = init(); > > > > > > > - if (ret) > > > > > > > - return ret; > > > > > > > - > > > > > > > - std::vector<Results> results; > > > > > > > - > > > > > > > - results.push_back(testSingleStream(camera_)); > > > > > > > - > > > > > > > - for (const Results &result : results) { > > > > > > > - ret = result.summary(); > > > > > > > - if (ret) > > > > > > > - return ret; > > > > > > > - } > > > > > > > - > > > > > > > - return 0; > > > > > > > -} > > > > > > > - > > > > > > > int Harness::init() > > > > > > > { > > > > > > > int ret = cm_->start(); > > > > > > > @@ -82,42 +65,26 @@ int Harness::init() > > > > > > > return ret; > > > > > > > } > > > > > > > > > > > > > > - if (!options_.isSet(OptCamera)) { > > > > > > > - std::cout << "No camera specified, available cameras:" << std::endl; > > > > > > > - listCameras(); > > > > > > > - return -ENODEV; > > > > > > > - } > > > > > > > - > > > > > > > - const std::string &cameraId = options_[OptCamera]; > > > > > > > - camera_ = cm_->get(cameraId); > > > > > > > - if (!camera_) { > > > > > > > - std::cout << "Camera " << cameraId << " not found, available cameras:" << std::endl; > > > > > > > - listCameras(); > > > > > > > - return -ENODEV; > > > > > > > + for (auto cam : cm_->cameras()) { > > > > > > > + if (cam->acquire()) { > > > > > > > + std::cout << "Failed to acquire camera" << std::endl; > > > > > > > + return -EINVAL; > > > > > > > + } > > > > > > > + cameras_.push_back(cam); > > > > > > > > > > > > I don't like this, I think we need to retain the ability to select which > > > > > > camera to test. And further down the line maybe allow more then one > > > > > > camera to be selected if we want to test concurrent use of two specific > > > > > > ones. But for the work in this series I think you can ignore multiple > > > > > > cameras. > > > > > > > > > > Actually, that doesn't mean that multiple cameras will be tested, that's > > > > > determined by the test filter, so doing --gtest_filter='.*idCamera1.*' would > > > > > only test that camera. But I think I see what you mean, I shouldn't be acquiring > > > > > all cameras, only the ones that I will actually test (and in this code there's a > > > > > big issue where if a single camera is already busy, even if it won't be tested, > > > > > lc-compliance as a whole fails). > > > > > > > > Ahh I understand. > > > > > > > > > > > > > > > > > > > > > If I understand the coverletter correctly the correct thing here the cli > > > > > > arguments should be feed to gtest directly to be able to filter on what > > > > > > tests are run. Is there support in getst to also pass arguments to > > > > > > tests? Maybe we could remove the need to parse arguments ourself > > > > > > completely? > > > > > > > > > > There isn't any CLI argument in gtest to pass arguments to tests, but I think we > > > > > could solve both problems (this and the one above) like this: > > > > > > > > > > - In the main(), query the id of each camera from libcamera. > > > > > - Register each of the tests dynamically in gtest passing as an argument the > > > > > camera id, rather than a camera pointer (which is the current behavior). > > > > > - To select which tests to run, and on which camera(s), the user just uses > > > > > --gtest_filter='testToRun.*idCamera' (or '.*idCamera.*' to run all tests for > > > > > the camera). > > > > > - In the startup of each test the camera is acquired for that test, and when > > > > > it's done it is released. > > > > > > > > > > Implications: > > > > > - We no longer need to parse arguments and can drop that code, as both camera > > > > > and test are specified directly with gtest's filter. > > > > > - We only acquire the cameras that we'll actually test and only while they're > > > > > being tested. > > > > > - As a downside, a single camera will probably be acquired multiple times during > > > > > a single run as lc-compliance moves from one test to the next, but I don't > > > > > think this would be a big issue. (Maybe we could do this at the test suite > > > > > level instead to greatly decrease the number of times of acquire(), but I'm > > > > > not sure if it's possible, since we instantiate, by passing the camera id, at > > > > > the test level, I'll have to test that). > > > > > > > > > > What do you think? > > > > > > > > I understand what you are trying to do but I see two issues. > > > > > > > > 1. It depends on the implicit knowledge of users how to build the filter > > > > string and is rather unintuitive compared to having two distinct > > > > arguments, one to select camera and one to express the test filter. > > > > > > > > This could possibly be worked around by keeping the cam options code > > > > and building the gtest filter string internally by injecitng the > > > > selected camera name in the correct location, but the we have the > > > > next issue, > > > > > > > > 2. I don't think it will be practically possible to express a test > > > > filter that selects a specific combination of two cameras. > > > > > > > > I do think we need to be able to parameterize test cases. If gtest does > > > > not allow to do so using function arguments there are other ways of > > > > doing so. > > > > > > I think I didn't express myself well. I meant to say that gtest's CLI doesn't > > > have a flag to allow passing parameters to tests, meaning that it would not be > > > possible to parametrize the tests from the command line and also remove our > > > own parser and rely on gtest's. > > > > Check, I think we need to keep our own parser for now as we need to be > > able to parametrize the tests from the CLI. > > > > > But gtest does allow test parametrization at runtime and I'm already relying on > > > it since v2. Let me go a bit more over the way I'm doing this. > > > > > > - I have a test defined, eg BalancedSingleCycle, which takes some parameters: > > > > > > std::shared_ptr<Camera> camera, StreamRole role, unsigned int numRequests > > > > > > - Since we may want to run the same test for all cameras, roles and numRequests, > > > I register it, at runtime, using my REGISTER_TESTS() macro, which calls > > > gtest's testing::RegisterTest() with all possible combinations of parameters. > > > > > > - Based on the --gtest_filter CLI argument from gtest, it decides which tests > > > are actually run > > > > > > So the key point is that even though I parametrize the tests, I do it for all > > > combinations in order to later rely on gtest_filter to select which tests to > > > run. > > > > I see, that explains how the parameters get filled. I noticed it but did > > not dig further, see below for an expansion of my thoughts now that I > > understand this detail :-) > > > > > > > > Now, I do agree with both of your points. And if we want to make the interface > > > more intuitive to use (which I also find important), there's no way around > > > having our own parser. > > > > > > And if we're using our own camera flag, then it makes no sense to register tests > > > for all cameras, we can register tests only with the specified camera. > > > > This feels wrong. It feels like tests should be register without a > > camera component. The camera (and possible other arguments) should come > > from either arguments [1] or a singleton parameter class [2]. > > > > 1. This do not seem to be possible with gtests as test parameters needs > > to be registerd. > > > > 2. This would be similar to the global variable used in earlier versions > > but instead modeled as a class singleton like CameraManager. > > > > Maybe there are other options tho. > > > > > > > > I think here it would be helpful to think if the interface suits our problem. > > > To me we're looking at having: > > > > > > lc-compliance --camera 'cameraId' --filter 'BalancedSingleCycle' > > > > This matches my view as well. > > > > > > > > to only test camera 'cameraId' with a single test 'BalancedSingleCycle'. Here, > > > the camera flag is used to only register tests with that camera, and the filter > > > is passed as --gtest_filter, possibly a bit adapted, to gtest. > > > > > > And if we imagine we have a test ConcurrentCameras, which receives two cameras > > > as parameters, and we call > > > > > > lc-compliance --camera 'cameraId1,cameraId2' > > > > This also looks good. But for now lets focus on one camera, I think it > > will give us enough headache :-) > > > > > > > > it would test both each of the cameras separately in all current tests which > > > accept a single camera, like BalancedSingleCycle, as well as test both at the > > > same time in the ConcurrentCameras test (and we could make only the former or > > > the latter occur by using the filter parameter). > > > > I have not thought enough about this to know if that is a good idea or > > not. At the top of my head I still tend to think that lc-compliance > > tests a single 'primary' camera and any additional camera supplied are > > only used for tests where two (or more) cameras are needed to test some > > specific feature of the primary camera. > > > > That is only the 'primary' camera is testes, the other camera(s) are > > just there to help/provoke the testing. But maybe I'm to narrow minded. > > Okay, so it now comes down to how lc-compliance will get used. Either: > > 1. lc-compliance will be called to test a single camera, with optionally other > secondary cameras > > 2. lc-compliance will be called to test multiple cameras at once (not > necessarily simultaneously, but in a single run), with the default case being > "Test all the cameras in this system". > > Given that the final objective of my internship is to have lc-compliance running > on real boards by KernelCI, to catch any kernel regressions that affect camera > usage, I'm trying to think how it would work there. (I wonder if this discussion > should be happening together with KernelCI to decide on this...) > > So I'm imagining a RockPi4 board with a camera sensor plugged in, running > lc-compliance to catch any kernel regressions. > > The limitation I see in 1 is that if a system has multiple cameras and want to > ensure that all keep working, it will need to either hardcode all camera ids and > manually call lc-compliance once for each one, or have a separate program that > lists all cameras from libcamera (using 'cam -l' for example) and calls > lc-compliance with each one. That's to say, running on all cameras is not as > easy. And then there are multiple reports, one for each camera, instead of a > single one for all cameras, which might be good or bad, not sure. > > That said, since the RockPi4 (and also the Raspberry Pi 4) has a single CSI-2 > connector, I believe it only supports a single camera anyway, so this shouldn't > be a problem for this board, but I'm not sure about others. Phones for example > seem to have at least two independent cameras... So, I just read about the Singleton pattern on Wikipedia and checked out the CameraManager class (should've done it before replying :) ). From what I can tell, there's a big difference between the CameraManager class and a Singleton. The CameraManager just makes sure that there's a single instance of it by saving a reference to it in an internal static variable. A Singleton apparently, in addition to doing that, makes its instatiation through a public static function. This means that not only there's a single instance, it's also available from the global scope. So I wanted to ask you to clarify which one you meant. If you meant that we should have a class like CameraManager, which just makes sure it's the single instance, my previous comments below are still valid. On the other hand, if you meant that we should a have singleton, which is available globally, then it is basically the same thing as having a global variable, only a bit more organized, and my comments below are no longer valid. We could indeed go back to static test registration, since the camera instance could be accessed from any test as something like TestEnvironment::getInstance().camera() making the alternative 1 have the benefit of making the test registration and definition code simpler. Thanks, Nícolas > > Regarding the benefit of 1, I'm not sure. If we were to use a global variable > like in v1, it would make the code a lot simpler by making the test registration > static. But that doesn't seem to be an option :). I'm not familiar with using a > Singleton, so I'll take a look at CameraManager. But I assume it wouldn't be > accessible statically, so the dynamic test registration complexities would have > to stay. > > Also, just to make sure we're on the same page, by using a Singleton, and again > assuming it isn't stored in a global variable to be accessible from the tests > statically, even though we would eliminate the camera as a parameter for the > tests, the reference to the singleton itself would need to be added as a > parameter for the tests. > > Thanks, > Nícolas > > > > > > > > > I know we don't need to implement support for multiple cameras right now, I just > > > want to make sure it's possible and thinking about the interface and the > > > scenarios help me picture how they'd work. > > > > > > Thoughts? > > > > > > Thanks, > > > Nícolas > > > > > > > > > > > I'm sure there are many solutions to this problem and maybe someone more > > > > familiar with gtest knows of a neat way to piggyback on its > > > > infrastrcuture? Since I know very little about the gtest framework the > > > > following is just an idea and I'm happy to yield. > > > > > > > > How about using a singleton class that carries arguments constructed > > > > from cli arguments (camera id) before the gtest are executed. Each test > > > > case could then consult this singleton if it needs to access parameters. > > > > I think you could take inspiration from the libcamera CameraManager > > > > class. > > > > > > > > This could (at lest for now) imply keeping the cam option parser. If so > > > > I think it would be nice if somewhen before we integrate this work that > > > > the gtest filter string could be supplied as an argument using that > > > > parser. Of course if we can do it the other way around and add arguments > > > > to the gtest parser that would be even cooler as it would be less code > > > > for us to maintain ;-)
Hi Nícolas, On 2021-05-11 14:17:56 -0300, Nícolas F. R. A. Prado wrote: > Hi Niklas, > > Em 2021-05-10 18:04, Niklas Söderlund escreveu: > > > Hi Nícolas, > > > > Thanks for your work on this. I really think gtest is a step in the > > right direction! > > > > On 2021-05-07 12:52:38 -0300, Nícolas F. R. A. Prado wrote: > > > Hi Niklas, > > > > > > Em 2021-05-07 06:11, Niklas Söderlund escreveu: > > > > > > > Hi Nícolas, > > > > > > > > On 2021-05-06 17:20:29 -0300, Nícolas F. R. A. Prado wrote: > > > > > Hi Niklas, > > > > > > > > > > thanks for the feedback. > > > > > > > > > > Em 2021-05-06 05:48, Niklas Söderlund escreveu: > > > > > > > > > > > Hi Nícolas, > > > > > > > > > > > > Thanks for your work! > > > > > > > > > > > > I really like how gtest helps make the actual tests to be easier to > > > > > > read. > > > > > > > > > > > > On 2021-05-03 16:33:07 -0300, Nícolas F. R. A. Prado wrote: > > > > > > > Refactor lc-compliance using Googletest as the test framework. > > > > > > > > > > > > > > Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com> > > > > > > > --- > > > > > > > Changes in v2: > > > > > > > - Changed from static to dynamic test registration > > > > > > > - Removed -c flag > > > > > > > > > > > > > > There's still an issue with the refactoring that the shared_ptr of the tests > > > > > > > apparently aren't being deleted on time, which causes: > > > > > > > > > > > > > > [20:26:08.744507935] [103243] ERROR DeviceEnumerator device_enumerator.cpp:165 Removing media device /dev/media0 while still in use > > > > > > > [20:26:08.744548175] [103243] ERROR DeviceEnumerator device_enumerator.cpp:165 Removing media device /dev/media1 while still in use > > > > > > > Segmentation fault (core dumped) > > > > > > > > > > > > > > I tried explicitly resetting the shared_ptr on the destructor of the tests but > > > > > > > that didn't work. In fact, when just listing the tests, the constructor for the > > > > > > > tests isn't even called. So I think the issue has to do with the passing of the > > > > > > > camera shared pointer to the REGISTER_TESTS() macro through that functor (?): > > > > > > > > > > > > > > [=]() -> testsuite* { return new testcase(c, r, rq); }); \ > > > > > > > > > > > > > > Not sure how to solve this. Any tip would be welcome. > > > > > > > > > > > > I will leave this for now as I have a different worry about > > > > > > REGISTER_TESTS() design below. > > > > > > > > > > > > > > > > > > > > src/lc-compliance/main.cpp | 99 +++++++++--------- > > > > > > > src/lc-compliance/meson.build | 3 + > > > > > > > src/lc-compliance/simple_capture.cpp | 74 +++++-------- > > > > > > > src/lc-compliance/simple_capture.h | 8 +- > > > > > > > src/lc-compliance/single_stream.cpp | 151 ++++++++++++++------------- > > > > > > > src/lc-compliance/tests.h | 15 ++- > > > > > > > 6 files changed, 175 insertions(+), 175 deletions(-) > > > > > > > > > > > > > > diff --git a/src/lc-compliance/main.cpp b/src/lc-compliance/main.cpp > > > > > > > index 54cee54aa978..add0d7729aec 100644 > > > > > > > --- a/src/lc-compliance/main.cpp > > > > > > > +++ b/src/lc-compliance/main.cpp > > > > > > > @@ -9,6 +9,8 @@ > > > > > > > #include <iostream> > > > > > > > #include <string.h> > > > > > > > > > > > > > > +#include <gtest/gtest.h> > > > > > > > + > > > > > > > #include <libcamera/libcamera.h> > > > > > > > > > > > > > > #include "../cam/options.h" > > > > > > > @@ -17,7 +19,6 @@ > > > > > > > using namespace libcamera; > > > > > > > > > > > > > > enum { > > > > > > > - OptCamera = 'c', > > > > > > > OptHelp = 'h', > > > > > > > }; > > > > > > > > > > > > > > @@ -28,14 +29,15 @@ public: > > > > > > > ~Harness(); > > > > > > > > > > > > > > int exec(); > > > > > > > + int init(); > > > > > > > + void registerTests(); > > > > > > > > > > > > > > private: > > > > > > > - int init(); > > > > > > > void listCameras(); > > > > > > > > > > > > > > OptionsParser::Options options_; > > > > > > > std::unique_ptr<CameraManager> cm_; > > > > > > > - std::shared_ptr<Camera> camera_; > > > > > > > + std::vector<std::shared_ptr<Camera>> cameras_; > > > > > > > }; > > > > > > > > > > > > > > Harness::Harness(const OptionsParser::Options &options) > > > > > > > @@ -46,33 +48,14 @@ Harness::Harness(const OptionsParser::Options &options) > > > > > > > > > > > > > > Harness::~Harness() > > > > > > > { > > > > > > > - if (camera_) { > > > > > > > - camera_->release(); > > > > > > > - camera_.reset(); > > > > > > > + for (auto &c : cameras_) { > > > > > > > + c->release(); > > > > > > > + c.reset(); > > > > > > > } > > > > > > > > > > > > > > cm_->stop(); > > > > > > > } > > > > > > > > > > > > > > -int Harness::exec() > > > > > > > -{ > > > > > > > - int ret = init(); > > > > > > > - if (ret) > > > > > > > - return ret; > > > > > > > - > > > > > > > - std::vector<Results> results; > > > > > > > - > > > > > > > - results.push_back(testSingleStream(camera_)); > > > > > > > - > > > > > > > - for (const Results &result : results) { > > > > > > > - ret = result.summary(); > > > > > > > - if (ret) > > > > > > > - return ret; > > > > > > > - } > > > > > > > - > > > > > > > - return 0; > > > > > > > -} > > > > > > > - > > > > > > > int Harness::init() > > > > > > > { > > > > > > > int ret = cm_->start(); > > > > > > > @@ -82,42 +65,26 @@ int Harness::init() > > > > > > > return ret; > > > > > > > } > > > > > > > > > > > > > > - if (!options_.isSet(OptCamera)) { > > > > > > > - std::cout << "No camera specified, available cameras:" << std::endl; > > > > > > > - listCameras(); > > > > > > > - return -ENODEV; > > > > > > > - } > > > > > > > - > > > > > > > - const std::string &cameraId = options_[OptCamera]; > > > > > > > - camera_ = cm_->get(cameraId); > > > > > > > - if (!camera_) { > > > > > > > - std::cout << "Camera " << cameraId << " not found, available cameras:" << std::endl; > > > > > > > - listCameras(); > > > > > > > - return -ENODEV; > > > > > > > + for (auto cam : cm_->cameras()) { > > > > > > > + if (cam->acquire()) { > > > > > > > + std::cout << "Failed to acquire camera" << std::endl; > > > > > > > + return -EINVAL; > > > > > > > + } > > > > > > > + cameras_.push_back(cam); > > > > > > > > > > > > I don't like this, I think we need to retain the ability to select which > > > > > > camera to test. And further down the line maybe allow more then one > > > > > > camera to be selected if we want to test concurrent use of two specific > > > > > > ones. But for the work in this series I think you can ignore multiple > > > > > > cameras. > > > > > > > > > > Actually, that doesn't mean that multiple cameras will be tested, that's > > > > > determined by the test filter, so doing --gtest_filter='.*idCamera1.*' would > > > > > only test that camera. But I think I see what you mean, I shouldn't be acquiring > > > > > all cameras, only the ones that I will actually test (and in this code there's a > > > > > big issue where if a single camera is already busy, even if it won't be tested, > > > > > lc-compliance as a whole fails). > > > > > > > > Ahh I understand. > > > > > > > > > > > > > > > > > > > > > If I understand the coverletter correctly the correct thing here the cli > > > > > > arguments should be feed to gtest directly to be able to filter on what > > > > > > tests are run. Is there support in getst to also pass arguments to > > > > > > tests? Maybe we could remove the need to parse arguments ourself > > > > > > completely? > > > > > > > > > > There isn't any CLI argument in gtest to pass arguments to tests, but I think we > > > > > could solve both problems (this and the one above) like this: > > > > > > > > > > - In the main(), query the id of each camera from libcamera. > > > > > - Register each of the tests dynamically in gtest passing as an argument the > > > > > camera id, rather than a camera pointer (which is the current behavior). > > > > > - To select which tests to run, and on which camera(s), the user just uses > > > > > --gtest_filter='testToRun.*idCamera' (or '.*idCamera.*' to run all tests for > > > > > the camera). > > > > > - In the startup of each test the camera is acquired for that test, and when > > > > > it's done it is released. > > > > > > > > > > Implications: > > > > > - We no longer need to parse arguments and can drop that code, as both camera > > > > > and test are specified directly with gtest's filter. > > > > > - We only acquire the cameras that we'll actually test and only while they're > > > > > being tested. > > > > > - As a downside, a single camera will probably be acquired multiple times during > > > > > a single run as lc-compliance moves from one test to the next, but I don't > > > > > think this would be a big issue. (Maybe we could do this at the test suite > > > > > level instead to greatly decrease the number of times of acquire(), but I'm > > > > > not sure if it's possible, since we instantiate, by passing the camera id, at > > > > > the test level, I'll have to test that). > > > > > > > > > > What do you think? > > > > > > > > I understand what you are trying to do but I see two issues. > > > > > > > > 1. It depends on the implicit knowledge of users how to build the filter > > > > string and is rather unintuitive compared to having two distinct > > > > arguments, one to select camera and one to express the test filter. > > > > > > > > This could possibly be worked around by keeping the cam options code > > > > and building the gtest filter string internally by injecitng the > > > > selected camera name in the correct location, but the we have the > > > > next issue, > > > > > > > > 2. I don't think it will be practically possible to express a test > > > > filter that selects a specific combination of two cameras. > > > > > > > > I do think we need to be able to parameterize test cases. If gtest does > > > > not allow to do so using function arguments there are other ways of > > > > doing so. > > > > > > I think I didn't express myself well. I meant to say that gtest's CLI doesn't > > > have a flag to allow passing parameters to tests, meaning that it would not be > > > possible to parametrize the tests from the command line and also remove our > > > own parser and rely on gtest's. > > > > Check, I think we need to keep our own parser for now as we need to be > > able to parametrize the tests from the CLI. > > > > > But gtest does allow test parametrization at runtime and I'm already relying on > > > it since v2. Let me go a bit more over the way I'm doing this. > > > > > > - I have a test defined, eg BalancedSingleCycle, which takes some parameters: > > > > > > std::shared_ptr<Camera> camera, StreamRole role, unsigned int numRequests > > > > > > - Since we may want to run the same test for all cameras, roles and numRequests, > > > I register it, at runtime, using my REGISTER_TESTS() macro, which calls > > > gtest's testing::RegisterTest() with all possible combinations of parameters. > > > > > > - Based on the --gtest_filter CLI argument from gtest, it decides which tests > > > are actually run > > > > > > So the key point is that even though I parametrize the tests, I do it for all > > > combinations in order to later rely on gtest_filter to select which tests to > > > run. > > > > I see, that explains how the parameters get filled. I noticed it but did > > not dig further, see below for an expansion of my thoughts now that I > > understand this detail :-) > > > > > > > > Now, I do agree with both of your points. And if we want to make the interface > > > more intuitive to use (which I also find important), there's no way around > > > having our own parser. > > > > > > And if we're using our own camera flag, then it makes no sense to register tests > > > for all cameras, we can register tests only with the specified camera. > > > > This feels wrong. It feels like tests should be register without a > > camera component. The camera (and possible other arguments) should come > > from either arguments [1] or a singleton parameter class [2]. > > > > 1. This do not seem to be possible with gtests as test parameters needs > > to be registerd. > > > > 2. This would be similar to the global variable used in earlier versions > > but instead modeled as a class singleton like CameraManager. > > > > Maybe there are other options tho. > > > > > > > > I think here it would be helpful to think if the interface suits our problem. > > > To me we're looking at having: > > > > > > lc-compliance --camera 'cameraId' --filter 'BalancedSingleCycle' > > > > This matches my view as well. > > > > > > > > to only test camera 'cameraId' with a single test 'BalancedSingleCycle'. Here, > > > the camera flag is used to only register tests with that camera, and the filter > > > is passed as --gtest_filter, possibly a bit adapted, to gtest. > > > > > > And if we imagine we have a test ConcurrentCameras, which receives two cameras > > > as parameters, and we call > > > > > > lc-compliance --camera 'cameraId1,cameraId2' > > > > This also looks good. But for now lets focus on one camera, I think it > > will give us enough headache :-) > > > > > > > > it would test both each of the cameras separately in all current tests which > > > accept a single camera, like BalancedSingleCycle, as well as test both at the > > > same time in the ConcurrentCameras test (and we could make only the former or > > > the latter occur by using the filter parameter). > > > > I have not thought enough about this to know if that is a good idea or > > not. At the top of my head I still tend to think that lc-compliance > > tests a single 'primary' camera and any additional camera supplied are > > only used for tests where two (or more) cameras are needed to test some > > specific feature of the primary camera. > > > > That is only the 'primary' camera is testes, the other camera(s) are > > just there to help/provoke the testing. But maybe I'm to narrow minded. > > Okay, so it now comes down to how lc-compliance will get used. Either: > > 1. lc-compliance will be called to test a single camera, with optionally other > secondary cameras > > 2. lc-compliance will be called to test multiple cameras at once (not > necessarily simultaneously, but in a single run), with the default case being > "Test all the cameras in this system". > > Given that the final objective of my internship is to have lc-compliance running > on real boards by KernelCI, to catch any kernel regressions that affect camera > usage, I'm trying to think how it would work there. (I wonder if this discussion > should be happening together with KernelCI to decide on this...) > > So I'm imagining a RockPi4 board with a camera sensor plugged in, running > lc-compliance to catch any kernel regressions. > > The limitation I see in 1 is that if a system has multiple cameras and want to > ensure that all keep working, it will need to either hardcode all camera ids and > manually call lc-compliance once for each one, or have a separate program that > lists all cameras from libcamera (using 'cam -l' for example) and calls > lc-compliance with each one. That's to say, running on all cameras is not as > easy. And then there are multiple reports, one for each camera, instead of a > single one for all cameras, which might be good or bad, not sure. > > That said, since the RockPi4 (and also the Raspberry Pi 4) has a single CSI-2 > connector, I believe it only supports a single camera anyway, so this shouldn't > be a problem for this board, but I'm not sure about others. Phones for example > seem to have at least two independent cameras... I now understand your point of view better, thanks for brining up the KernelCI angle. I agree from a KernelCI perspective the goal is likely to test all cameras of the system to get the widest test base. But from a (current) libcamera perspective the lc-complinance tool primary usage is to be able to test a particular camera (or pipeline) to prove changes do not break things, kind of like v4l2-complinace is for V4L2. That being said I think it's trivial for us to make lc-complinance satisfy both use-cases. If the arg parsing is structured something like this, if (cliargs.continas("--camera")) { run_tests_for_camera(cliargs.value_for("--camera"); } else if (cliargs.continas("--run-all-cameras")) { for (camera : camer_manager.list_all_cameras()) { run_tests_for_camera(camera) } } Or am I missing something? > > Regarding the benefit of 1, I'm not sure. If we were to use a global variable > like in v1, it would make the code a lot simpler by making the test registration > static. But that doesn't seem to be an option :). I'm not familiar with using a > Singleton, so I'll take a look at CameraManager. But I assume it wouldn't be > accessible statically, so the dynamic test registration complexities would have > to stay. > > Also, just to make sure we're on the same page, by using a Singleton, and again > assuming it isn't stored in a global variable to be accessible from the tests > statically, even though we would eliminate the camera as a parameter for the > tests, the reference to the singleton itself would need to be added as a > parameter for the tests. I will follow up on this in your second mail. > > Thanks, > Nícolas > > > > > > > > > I know we don't need to implement support for multiple cameras right now, I just > > > want to make sure it's possible and thinking about the interface and the > > > scenarios help me picture how they'd work. > > > > > > Thoughts? > > > > > > Thanks, > > > Nícolas > > > > > > > > > > > I'm sure there are many solutions to this problem and maybe someone more > > > > familiar with gtest knows of a neat way to piggyback on its > > > > infrastrcuture? Since I know very little about the gtest framework the > > > > following is just an idea and I'm happy to yield. > > > > > > > > How about using a singleton class that carries arguments constructed > > > > from cli arguments (camera id) before the gtest are executed. Each test > > > > case could then consult this singleton if it needs to access parameters. > > > > I think you could take inspiration from the libcamera CameraManager > > > > class. > > > > > > > > This could (at lest for now) imply keeping the cam option parser. If so > > > > I think it would be nice if somewhen before we integrate this work that > > > > the gtest filter string could be supplied as an argument using that > > > > parser. Of course if we can do it the other way around and add arguments > > > > to the gtest parser that would be even cooler as it would be less code > > > > for us to maintain ;-)
On 2021-05-11 16:06:04 -0300, Nícolas F. R. A. Prado wrote: > Hello again, Hello :-) > > Em 2021-05-11 14:17, Nícolas F. R. A. Prado escreveu: > > > Hi Niklas, > > > > Em 2021-05-10 18:04, Niklas Söderlund escreveu: > > > > > Hi Nícolas, > > > > > > Thanks for your work on this. I really think gtest is a step in the > > > right direction! > > > > > > On 2021-05-07 12:52:38 -0300, Nícolas F. R. A. Prado wrote: > > > > Hi Niklas, > > > > > > > > Em 2021-05-07 06:11, Niklas Söderlund escreveu: > > > > > > > > > Hi Nícolas, > > > > > > > > > > On 2021-05-06 17:20:29 -0300, Nícolas F. R. A. Prado wrote: > > > > > > Hi Niklas, > > > > > > > > > > > > thanks for the feedback. > > > > > > > > > > > > Em 2021-05-06 05:48, Niklas Söderlund escreveu: > > > > > > > > > > > > > Hi Nícolas, > > > > > > > > > > > > > > Thanks for your work! > > > > > > > > > > > > > > I really like how gtest helps make the actual tests to be easier to > > > > > > > read. > > > > > > > > > > > > > > On 2021-05-03 16:33:07 -0300, Nícolas F. R. A. Prado wrote: > > > > > > > > Refactor lc-compliance using Googletest as the test framework. > > > > > > > > > > > > > > > > Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com> > > > > > > > > --- > > > > > > > > Changes in v2: > > > > > > > > - Changed from static to dynamic test registration > > > > > > > > - Removed -c flag > > > > > > > > > > > > > > > > There's still an issue with the refactoring that the shared_ptr of the tests > > > > > > > > apparently aren't being deleted on time, which causes: > > > > > > > > > > > > > > > > [20:26:08.744507935] [103243] ERROR DeviceEnumerator device_enumerator.cpp:165 Removing media device /dev/media0 while still in use > > > > > > > > [20:26:08.744548175] [103243] ERROR DeviceEnumerator device_enumerator.cpp:165 Removing media device /dev/media1 while still in use > > > > > > > > Segmentation fault (core dumped) > > > > > > > > > > > > > > > > I tried explicitly resetting the shared_ptr on the destructor of the tests but > > > > > > > > that didn't work. In fact, when just listing the tests, the constructor for the > > > > > > > > tests isn't even called. So I think the issue has to do with the passing of the > > > > > > > > camera shared pointer to the REGISTER_TESTS() macro through that functor (?): > > > > > > > > > > > > > > > > [=]() -> testsuite* { return new testcase(c, r, rq); }); \ > > > > > > > > > > > > > > > > Not sure how to solve this. Any tip would be welcome. > > > > > > > > > > > > > > I will leave this for now as I have a different worry about > > > > > > > REGISTER_TESTS() design below. > > > > > > > > > > > > > > > > > > > > > > > src/lc-compliance/main.cpp | 99 +++++++++--------- > > > > > > > > src/lc-compliance/meson.build | 3 + > > > > > > > > src/lc-compliance/simple_capture.cpp | 74 +++++-------- > > > > > > > > src/lc-compliance/simple_capture.h | 8 +- > > > > > > > > src/lc-compliance/single_stream.cpp | 151 ++++++++++++++------------- > > > > > > > > src/lc-compliance/tests.h | 15 ++- > > > > > > > > 6 files changed, 175 insertions(+), 175 deletions(-) > > > > > > > > > > > > > > > > diff --git a/src/lc-compliance/main.cpp b/src/lc-compliance/main.cpp > > > > > > > > index 54cee54aa978..add0d7729aec 100644 > > > > > > > > --- a/src/lc-compliance/main.cpp > > > > > > > > +++ b/src/lc-compliance/main.cpp > > > > > > > > @@ -9,6 +9,8 @@ > > > > > > > > #include <iostream> > > > > > > > > #include <string.h> > > > > > > > > > > > > > > > > +#include <gtest/gtest.h> > > > > > > > > + > > > > > > > > #include <libcamera/libcamera.h> > > > > > > > > > > > > > > > > #include "../cam/options.h" > > > > > > > > @@ -17,7 +19,6 @@ > > > > > > > > using namespace libcamera; > > > > > > > > > > > > > > > > enum { > > > > > > > > - OptCamera = 'c', > > > > > > > > OptHelp = 'h', > > > > > > > > }; > > > > > > > > > > > > > > > > @@ -28,14 +29,15 @@ public: > > > > > > > > ~Harness(); > > > > > > > > > > > > > > > > int exec(); > > > > > > > > + int init(); > > > > > > > > + void registerTests(); > > > > > > > > > > > > > > > > private: > > > > > > > > - int init(); > > > > > > > > void listCameras(); > > > > > > > > > > > > > > > > OptionsParser::Options options_; > > > > > > > > std::unique_ptr<CameraManager> cm_; > > > > > > > > - std::shared_ptr<Camera> camera_; > > > > > > > > + std::vector<std::shared_ptr<Camera>> cameras_; > > > > > > > > }; > > > > > > > > > > > > > > > > Harness::Harness(const OptionsParser::Options &options) > > > > > > > > @@ -46,33 +48,14 @@ Harness::Harness(const OptionsParser::Options &options) > > > > > > > > > > > > > > > > Harness::~Harness() > > > > > > > > { > > > > > > > > - if (camera_) { > > > > > > > > - camera_->release(); > > > > > > > > - camera_.reset(); > > > > > > > > + for (auto &c : cameras_) { > > > > > > > > + c->release(); > > > > > > > > + c.reset(); > > > > > > > > } > > > > > > > > > > > > > > > > cm_->stop(); > > > > > > > > } > > > > > > > > > > > > > > > > -int Harness::exec() > > > > > > > > -{ > > > > > > > > - int ret = init(); > > > > > > > > - if (ret) > > > > > > > > - return ret; > > > > > > > > - > > > > > > > > - std::vector<Results> results; > > > > > > > > - > > > > > > > > - results.push_back(testSingleStream(camera_)); > > > > > > > > - > > > > > > > > - for (const Results &result : results) { > > > > > > > > - ret = result.summary(); > > > > > > > > - if (ret) > > > > > > > > - return ret; > > > > > > > > - } > > > > > > > > - > > > > > > > > - return 0; > > > > > > > > -} > > > > > > > > - > > > > > > > > int Harness::init() > > > > > > > > { > > > > > > > > int ret = cm_->start(); > > > > > > > > @@ -82,42 +65,26 @@ int Harness::init() > > > > > > > > return ret; > > > > > > > > } > > > > > > > > > > > > > > > > - if (!options_.isSet(OptCamera)) { > > > > > > > > - std::cout << "No camera specified, available cameras:" << std::endl; > > > > > > > > - listCameras(); > > > > > > > > - return -ENODEV; > > > > > > > > - } > > > > > > > > - > > > > > > > > - const std::string &cameraId = options_[OptCamera]; > > > > > > > > - camera_ = cm_->get(cameraId); > > > > > > > > - if (!camera_) { > > > > > > > > - std::cout << "Camera " << cameraId << " not found, available cameras:" << std::endl; > > > > > > > > - listCameras(); > > > > > > > > - return -ENODEV; > > > > > > > > + for (auto cam : cm_->cameras()) { > > > > > > > > + if (cam->acquire()) { > > > > > > > > + std::cout << "Failed to acquire camera" << std::endl; > > > > > > > > + return -EINVAL; > > > > > > > > + } > > > > > > > > + cameras_.push_back(cam); > > > > > > > > > > > > > > I don't like this, I think we need to retain the ability to select which > > > > > > > camera to test. And further down the line maybe allow more then one > > > > > > > camera to be selected if we want to test concurrent use of two specific > > > > > > > ones. But for the work in this series I think you can ignore multiple > > > > > > > cameras. > > > > > > > > > > > > Actually, that doesn't mean that multiple cameras will be tested, that's > > > > > > determined by the test filter, so doing --gtest_filter='.*idCamera1.*' would > > > > > > only test that camera. But I think I see what you mean, I shouldn't be acquiring > > > > > > all cameras, only the ones that I will actually test (and in this code there's a > > > > > > big issue where if a single camera is already busy, even if it won't be tested, > > > > > > lc-compliance as a whole fails). > > > > > > > > > > Ahh I understand. > > > > > > > > > > > > > > > > > > > > > > > > > If I understand the coverletter correctly the correct thing here the cli > > > > > > > arguments should be feed to gtest directly to be able to filter on what > > > > > > > tests are run. Is there support in getst to also pass arguments to > > > > > > > tests? Maybe we could remove the need to parse arguments ourself > > > > > > > completely? > > > > > > > > > > > > There isn't any CLI argument in gtest to pass arguments to tests, but I think we > > > > > > could solve both problems (this and the one above) like this: > > > > > > > > > > > > - In the main(), query the id of each camera from libcamera. > > > > > > - Register each of the tests dynamically in gtest passing as an argument the > > > > > > camera id, rather than a camera pointer (which is the current behavior). > > > > > > - To select which tests to run, and on which camera(s), the user just uses > > > > > > --gtest_filter='testToRun.*idCamera' (or '.*idCamera.*' to run all tests for > > > > > > the camera). > > > > > > - In the startup of each test the camera is acquired for that test, and when > > > > > > it's done it is released. > > > > > > > > > > > > Implications: > > > > > > - We no longer need to parse arguments and can drop that code, as both camera > > > > > > and test are specified directly with gtest's filter. > > > > > > - We only acquire the cameras that we'll actually test and only while they're > > > > > > being tested. > > > > > > - As a downside, a single camera will probably be acquired multiple times during > > > > > > a single run as lc-compliance moves from one test to the next, but I don't > > > > > > think this would be a big issue. (Maybe we could do this at the test suite > > > > > > level instead to greatly decrease the number of times of acquire(), but I'm > > > > > > not sure if it's possible, since we instantiate, by passing the camera id, at > > > > > > the test level, I'll have to test that). > > > > > > > > > > > > What do you think? > > > > > > > > > > I understand what you are trying to do but I see two issues. > > > > > > > > > > 1. It depends on the implicit knowledge of users how to build the filter > > > > > string and is rather unintuitive compared to having two distinct > > > > > arguments, one to select camera and one to express the test filter. > > > > > > > > > > This could possibly be worked around by keeping the cam options code > > > > > and building the gtest filter string internally by injecitng the > > > > > selected camera name in the correct location, but the we have the > > > > > next issue, > > > > > > > > > > 2. I don't think it will be practically possible to express a test > > > > > filter that selects a specific combination of two cameras. > > > > > > > > > > I do think we need to be able to parameterize test cases. If gtest does > > > > > not allow to do so using function arguments there are other ways of > > > > > doing so. > > > > > > > > I think I didn't express myself well. I meant to say that gtest's CLI doesn't > > > > have a flag to allow passing parameters to tests, meaning that it would not be > > > > possible to parametrize the tests from the command line and also remove our > > > > own parser and rely on gtest's. > > > > > > Check, I think we need to keep our own parser for now as we need to be > > > able to parametrize the tests from the CLI. > > > > > > > But gtest does allow test parametrization at runtime and I'm already relying on > > > > it since v2. Let me go a bit more over the way I'm doing this. > > > > > > > > - I have a test defined, eg BalancedSingleCycle, which takes some parameters: > > > > > > > > std::shared_ptr<Camera> camera, StreamRole role, unsigned int numRequests > > > > > > > > - Since we may want to run the same test for all cameras, roles and numRequests, > > > > I register it, at runtime, using my REGISTER_TESTS() macro, which calls > > > > gtest's testing::RegisterTest() with all possible combinations of parameters. > > > > > > > > - Based on the --gtest_filter CLI argument from gtest, it decides which tests > > > > are actually run > > > > > > > > So the key point is that even though I parametrize the tests, I do it for all > > > > combinations in order to later rely on gtest_filter to select which tests to > > > > run. > > > > > > I see, that explains how the parameters get filled. I noticed it but did > > > not dig further, see below for an expansion of my thoughts now that I > > > understand this detail :-) > > > > > > > > > > > Now, I do agree with both of your points. And if we want to make the interface > > > > more intuitive to use (which I also find important), there's no way around > > > > having our own parser. > > > > > > > > And if we're using our own camera flag, then it makes no sense to register tests > > > > for all cameras, we can register tests only with the specified camera. > > > > > > This feels wrong. It feels like tests should be register without a > > > camera component. The camera (and possible other arguments) should come > > > from either arguments [1] or a singleton parameter class [2]. > > > > > > 1. This do not seem to be possible with gtests as test parameters needs > > > to be registerd. > > > > > > 2. This would be similar to the global variable used in earlier versions > > > but instead modeled as a class singleton like CameraManager. > > > > > > Maybe there are other options tho. > > > > > > > > > > > I think here it would be helpful to think if the interface suits our problem. > > > > To me we're looking at having: > > > > > > > > lc-compliance --camera 'cameraId' --filter 'BalancedSingleCycle' > > > > > > This matches my view as well. > > > > > > > > > > > to only test camera 'cameraId' with a single test 'BalancedSingleCycle'. Here, > > > > the camera flag is used to only register tests with that camera, and the filter > > > > is passed as --gtest_filter, possibly a bit adapted, to gtest. > > > > > > > > And if we imagine we have a test ConcurrentCameras, which receives two cameras > > > > as parameters, and we call > > > > > > > > lc-compliance --camera 'cameraId1,cameraId2' > > > > > > This also looks good. But for now lets focus on one camera, I think it > > > will give us enough headache :-) > > > > > > > > > > > it would test both each of the cameras separately in all current tests which > > > > accept a single camera, like BalancedSingleCycle, as well as test both at the > > > > same time in the ConcurrentCameras test (and we could make only the former or > > > > the latter occur by using the filter parameter). > > > > > > I have not thought enough about this to know if that is a good idea or > > > not. At the top of my head I still tend to think that lc-compliance > > > tests a single 'primary' camera and any additional camera supplied are > > > only used for tests where two (or more) cameras are needed to test some > > > specific feature of the primary camera. > > > > > > That is only the 'primary' camera is testes, the other camera(s) are > > > just there to help/provoke the testing. But maybe I'm to narrow minded. > > > > Okay, so it now comes down to how lc-compliance will get used. Either: > > > > 1. lc-compliance will be called to test a single camera, with optionally other > > secondary cameras > > > > 2. lc-compliance will be called to test multiple cameras at once (not > > necessarily simultaneously, but in a single run), with the default case being > > "Test all the cameras in this system". > > > > Given that the final objective of my internship is to have lc-compliance running > > on real boards by KernelCI, to catch any kernel regressions that affect camera > > usage, I'm trying to think how it would work there. (I wonder if this discussion > > should be happening together with KernelCI to decide on this...) > > > > So I'm imagining a RockPi4 board with a camera sensor plugged in, running > > lc-compliance to catch any kernel regressions. > > > > The limitation I see in 1 is that if a system has multiple cameras and want to > > ensure that all keep working, it will need to either hardcode all camera ids and > > manually call lc-compliance once for each one, or have a separate program that > > lists all cameras from libcamera (using 'cam -l' for example) and calls > > lc-compliance with each one. That's to say, running on all cameras is not as > > easy. And then there are multiple reports, one for each camera, instead of a > > single one for all cameras, which might be good or bad, not sure. > > > > That said, since the RockPi4 (and also the Raspberry Pi 4) has a single CSI-2 > > connector, I believe it only supports a single camera anyway, so this shouldn't > > be a problem for this board, but I'm not sure about others. Phones for example > > seem to have at least two independent cameras... > > So, I just read about the Singleton pattern on Wikipedia and checked out the > CameraManager class (should've done it before replying :) ). > > From what I can tell, there's a big difference between the CameraManager class > and a Singleton. The CameraManager just makes sure that there's a single > instance of it by saving a reference to it in an internal static variable. A > Singleton apparently, in addition to doing that, makes its instatiation through > a public static function. This means that not only there's a single instance, > it's also available from the global scope. You are correct, my bad. I thought CameraManager implemented the Singleton pattern. > > So I wanted to ask you to clarify which one you meant. Always a good idea as I easily get lost ;-) > If you meant that we > should have a class like CameraManager, which just makes sure it's the single > instance, my previous comments below are still valid. > > On the other hand, if you meant that we should a have singleton, which is > available globally, then it is basically the same thing as having a global > variable, only a bit more organized, and my comments below are no longer valid. > We could indeed go back to static test registration, since the camera instance > could be accessed from any test as something like > > TestEnvironment::getInstance().camera() This is what I meant, a more organized way to reach the global variable to allow for static test registration while still being able to parametrize the tests. Sorry for the confusion by bringing up CameraManager. > > making the alternative 1 have the benefit of making the test registration and > definition code simpler. > > Thanks, > Nícolas > > > > > Regarding the benefit of 1, I'm not sure. If we were to use a global variable > > like in v1, it would make the code a lot simpler by making the test registration > > static. But that doesn't seem to be an option :). I'm not familiar with using a > > Singleton, so I'll take a look at CameraManager. But I assume it wouldn't be > > accessible statically, so the dynamic test registration complexities would have > > to stay. > > > > Also, just to make sure we're on the same page, by using a Singleton, and again > > assuming it isn't stored in a global variable to be accessible from the tests > > statically, even though we would eliminate the camera as a parameter for the > > tests, the reference to the singleton itself would need to be added as a > > parameter for the tests. > > > > Thanks, > > Nícolas > > > > > > > > > > > > > I know we don't need to implement support for multiple cameras right now, I just > > > > want to make sure it's possible and thinking about the interface and the > > > > scenarios help me picture how they'd work. > > > > > > > > Thoughts? > > > > > > > > Thanks, > > > > Nícolas > > > > > > > > > > > > > > I'm sure there are many solutions to this problem and maybe someone more > > > > > familiar with gtest knows of a neat way to piggyback on its > > > > > infrastrcuture? Since I know very little about the gtest framework the > > > > > following is just an idea and I'm happy to yield. > > > > > > > > > > How about using a singleton class that carries arguments constructed > > > > > from cli arguments (camera id) before the gtest are executed. Each test > > > > > case could then consult this singleton if it needs to access parameters. > > > > > I think you could take inspiration from the libcamera CameraManager > > > > > class. > > > > > > > > > > This could (at lest for now) imply keeping the cam option parser. If so > > > > > I think it would be nice if somewhen before we integrate this work that > > > > > the gtest filter string could be supplied as an argument using that > > > > > parser. Of course if we can do it the other way around and add arguments > > > > > to the gtest parser that would be even cooler as it would be less code > > > > > for us to maintain ;-)
Hi Niklas, Em 2021-05-12 05:49, Niklas Söderlund escreveu: > Hi Nícolas, > > On 2021-05-11 14:17:56 -0300, Nícolas F. R. A. Prado wrote: > > Hi Niklas, > > > > Em 2021-05-10 18:04, Niklas Söderlund escreveu: > > > > > Hi Nícolas, > > > > > > Thanks for your work on this. I really think gtest is a step in the > > > right direction! > > > > > > On 2021-05-07 12:52:38 -0300, Nícolas F. R. A. Prado wrote: > > > > Hi Niklas, > > > > > > > > Em 2021-05-07 06:11, Niklas Söderlund escreveu: > > > > > > > > > Hi Nícolas, > > > > > > > > > > On 2021-05-06 17:20:29 -0300, Nícolas F. R. A. Prado wrote: > > > > > > Hi Niklas, > > > > > > > > > > > > thanks for the feedback. > > > > > > > > > > > > Em 2021-05-06 05:48, Niklas Söderlund escreveu: > > > > > > > > > > > > > Hi Nícolas, > > > > > > > > > > > > > > Thanks for your work! > > > > > > > > > > > > > > I really like how gtest helps make the actual tests to be easier to > > > > > > > read. > > > > > > > > > > > > > > On 2021-05-03 16:33:07 -0300, Nícolas F. R. A. Prado wrote: > > > > > > > > Refactor lc-compliance using Googletest as the test framework. > > > > > > > > > > > > > > > > Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com> > > > > > > > > --- > > > > > > > > Changes in v2: > > > > > > > > - Changed from static to dynamic test registration > > > > > > > > - Removed -c flag > > > > > > > > > > > > > > > > There's still an issue with the refactoring that the shared_ptr of the tests > > > > > > > > apparently aren't being deleted on time, which causes: > > > > > > > > > > > > > > > > [20:26:08.744507935] [103243] ERROR DeviceEnumerator device_enumerator.cpp:165 Removing media device /dev/media0 while still in use > > > > > > > > [20:26:08.744548175] [103243] ERROR DeviceEnumerator device_enumerator.cpp:165 Removing media device /dev/media1 while still in use > > > > > > > > Segmentation fault (core dumped) > > > > > > > > > > > > > > > > I tried explicitly resetting the shared_ptr on the destructor of the tests but > > > > > > > > that didn't work. In fact, when just listing the tests, the constructor for the > > > > > > > > tests isn't even called. So I think the issue has to do with the passing of the > > > > > > > > camera shared pointer to the REGISTER_TESTS() macro through that functor (?): > > > > > > > > > > > > > > > > [=]() -> testsuite* { return new testcase(c, r, rq); }); \ > > > > > > > > > > > > > > > > Not sure how to solve this. Any tip would be welcome. > > > > > > > > > > > > > > I will leave this for now as I have a different worry about > > > > > > > REGISTER_TESTS() design below. > > > > > > > > > > > > > > > > > > > > > > > src/lc-compliance/main.cpp | 99 +++++++++--------- > > > > > > > > src/lc-compliance/meson.build | 3 + > > > > > > > > src/lc-compliance/simple_capture.cpp | 74 +++++-------- > > > > > > > > src/lc-compliance/simple_capture.h | 8 +- > > > > > > > > src/lc-compliance/single_stream.cpp | 151 ++++++++++++++------------- > > > > > > > > src/lc-compliance/tests.h | 15 ++- > > > > > > > > 6 files changed, 175 insertions(+), 175 deletions(-) > > > > > > > > > > > > > > > > diff --git a/src/lc-compliance/main.cpp b/src/lc-compliance/main.cpp > > > > > > > > index 54cee54aa978..add0d7729aec 100644 > > > > > > > > --- a/src/lc-compliance/main.cpp > > > > > > > > +++ b/src/lc-compliance/main.cpp > > > > > > > > @@ -9,6 +9,8 @@ > > > > > > > > #include <iostream> > > > > > > > > #include <string.h> > > > > > > > > > > > > > > > > +#include <gtest/gtest.h> > > > > > > > > + > > > > > > > > #include <libcamera/libcamera.h> > > > > > > > > > > > > > > > > #include "../cam/options.h" > > > > > > > > @@ -17,7 +19,6 @@ > > > > > > > > using namespace libcamera; > > > > > > > > > > > > > > > > enum { > > > > > > > > - OptCamera = 'c', > > > > > > > > OptHelp = 'h', > > > > > > > > }; > > > > > > > > > > > > > > > > @@ -28,14 +29,15 @@ public: > > > > > > > > ~Harness(); > > > > > > > > > > > > > > > > int exec(); > > > > > > > > + int init(); > > > > > > > > + void registerTests(); > > > > > > > > > > > > > > > > private: > > > > > > > > - int init(); > > > > > > > > void listCameras(); > > > > > > > > > > > > > > > > OptionsParser::Options options_; > > > > > > > > std::unique_ptr<CameraManager> cm_; > > > > > > > > - std::shared_ptr<Camera> camera_; > > > > > > > > + std::vector<std::shared_ptr<Camera>> cameras_; > > > > > > > > }; > > > > > > > > > > > > > > > > Harness::Harness(const OptionsParser::Options &options) > > > > > > > > @@ -46,33 +48,14 @@ Harness::Harness(const OptionsParser::Options &options) > > > > > > > > > > > > > > > > Harness::~Harness() > > > > > > > > { > > > > > > > > - if (camera_) { > > > > > > > > - camera_->release(); > > > > > > > > - camera_.reset(); > > > > > > > > + for (auto &c : cameras_) { > > > > > > > > + c->release(); > > > > > > > > + c.reset(); > > > > > > > > } > > > > > > > > > > > > > > > > cm_->stop(); > > > > > > > > } > > > > > > > > > > > > > > > > -int Harness::exec() > > > > > > > > -{ > > > > > > > > - int ret = init(); > > > > > > > > - if (ret) > > > > > > > > - return ret; > > > > > > > > - > > > > > > > > - std::vector<Results> results; > > > > > > > > - > > > > > > > > - results.push_back(testSingleStream(camera_)); > > > > > > > > - > > > > > > > > - for (const Results &result : results) { > > > > > > > > - ret = result.summary(); > > > > > > > > - if (ret) > > > > > > > > - return ret; > > > > > > > > - } > > > > > > > > - > > > > > > > > - return 0; > > > > > > > > -} > > > > > > > > - > > > > > > > > int Harness::init() > > > > > > > > { > > > > > > > > int ret = cm_->start(); > > > > > > > > @@ -82,42 +65,26 @@ int Harness::init() > > > > > > > > return ret; > > > > > > > > } > > > > > > > > > > > > > > > > - if (!options_.isSet(OptCamera)) { > > > > > > > > - std::cout << "No camera specified, available cameras:" << std::endl; > > > > > > > > - listCameras(); > > > > > > > > - return -ENODEV; > > > > > > > > - } > > > > > > > > - > > > > > > > > - const std::string &cameraId = options_[OptCamera]; > > > > > > > > - camera_ = cm_->get(cameraId); > > > > > > > > - if (!camera_) { > > > > > > > > - std::cout << "Camera " << cameraId << " not found, available cameras:" << std::endl; > > > > > > > > - listCameras(); > > > > > > > > - return -ENODEV; > > > > > > > > + for (auto cam : cm_->cameras()) { > > > > > > > > + if (cam->acquire()) { > > > > > > > > + std::cout << "Failed to acquire camera" << std::endl; > > > > > > > > + return -EINVAL; > > > > > > > > + } > > > > > > > > + cameras_.push_back(cam); > > > > > > > > > > > > > > I don't like this, I think we need to retain the ability to select which > > > > > > > camera to test. And further down the line maybe allow more then one > > > > > > > camera to be selected if we want to test concurrent use of two specific > > > > > > > ones. But for the work in this series I think you can ignore multiple > > > > > > > cameras. > > > > > > > > > > > > Actually, that doesn't mean that multiple cameras will be tested, that's > > > > > > determined by the test filter, so doing --gtest_filter='.*idCamera1.*' would > > > > > > only test that camera. But I think I see what you mean, I shouldn't be acquiring > > > > > > all cameras, only the ones that I will actually test (and in this code there's a > > > > > > big issue where if a single camera is already busy, even if it won't be tested, > > > > > > lc-compliance as a whole fails). > > > > > > > > > > Ahh I understand. > > > > > > > > > > > > > > > > > > > > > > > > > If I understand the coverletter correctly the correct thing here the cli > > > > > > > arguments should be feed to gtest directly to be able to filter on what > > > > > > > tests are run. Is there support in getst to also pass arguments to > > > > > > > tests? Maybe we could remove the need to parse arguments ourself > > > > > > > completely? > > > > > > > > > > > > There isn't any CLI argument in gtest to pass arguments to tests, but I think we > > > > > > could solve both problems (this and the one above) like this: > > > > > > > > > > > > - In the main(), query the id of each camera from libcamera. > > > > > > - Register each of the tests dynamically in gtest passing as an argument the > > > > > > camera id, rather than a camera pointer (which is the current behavior). > > > > > > - To select which tests to run, and on which camera(s), the user just uses > > > > > > --gtest_filter='testToRun.*idCamera' (or '.*idCamera.*' to run all tests for > > > > > > the camera). > > > > > > - In the startup of each test the camera is acquired for that test, and when > > > > > > it's done it is released. > > > > > > > > > > > > Implications: > > > > > > - We no longer need to parse arguments and can drop that code, as both camera > > > > > > and test are specified directly with gtest's filter. > > > > > > - We only acquire the cameras that we'll actually test and only while they're > > > > > > being tested. > > > > > > - As a downside, a single camera will probably be acquired multiple times during > > > > > > a single run as lc-compliance moves from one test to the next, but I don't > > > > > > think this would be a big issue. (Maybe we could do this at the test suite > > > > > > level instead to greatly decrease the number of times of acquire(), but I'm > > > > > > not sure if it's possible, since we instantiate, by passing the camera id, at > > > > > > the test level, I'll have to test that). > > > > > > > > > > > > What do you think? > > > > > > > > > > I understand what you are trying to do but I see two issues. > > > > > > > > > > 1. It depends on the implicit knowledge of users how to build the filter > > > > > string and is rather unintuitive compared to having two distinct > > > > > arguments, one to select camera and one to express the test filter. > > > > > > > > > > This could possibly be worked around by keeping the cam options code > > > > > and building the gtest filter string internally by injecitng the > > > > > selected camera name in the correct location, but the we have the > > > > > next issue, > > > > > > > > > > 2. I don't think it will be practically possible to express a test > > > > > filter that selects a specific combination of two cameras. > > > > > > > > > > I do think we need to be able to parameterize test cases. If gtest does > > > > > not allow to do so using function arguments there are other ways of > > > > > doing so. > > > > > > > > I think I didn't express myself well. I meant to say that gtest's CLI doesn't > > > > have a flag to allow passing parameters to tests, meaning that it would not be > > > > possible to parametrize the tests from the command line and also remove our > > > > own parser and rely on gtest's. > > > > > > Check, I think we need to keep our own parser for now as we need to be > > > able to parametrize the tests from the CLI. > > > > > > > But gtest does allow test parametrization at runtime and I'm already relying on > > > > it since v2. Let me go a bit more over the way I'm doing this. > > > > > > > > - I have a test defined, eg BalancedSingleCycle, which takes some parameters: > > > > > > > > std::shared_ptr<Camera> camera, StreamRole role, unsigned int numRequests > > > > > > > > - Since we may want to run the same test for all cameras, roles and numRequests, > > > > I register it, at runtime, using my REGISTER_TESTS() macro, which calls > > > > gtest's testing::RegisterTest() with all possible combinations of parameters. > > > > > > > > - Based on the --gtest_filter CLI argument from gtest, it decides which tests > > > > are actually run > > > > > > > > So the key point is that even though I parametrize the tests, I do it for all > > > > combinations in order to later rely on gtest_filter to select which tests to > > > > run. > > > > > > I see, that explains how the parameters get filled. I noticed it but did > > > not dig further, see below for an expansion of my thoughts now that I > > > understand this detail :-) > > > > > > > > > > > Now, I do agree with both of your points. And if we want to make the interface > > > > more intuitive to use (which I also find important), there's no way around > > > > having our own parser. > > > > > > > > And if we're using our own camera flag, then it makes no sense to register tests > > > > for all cameras, we can register tests only with the specified camera. > > > > > > This feels wrong. It feels like tests should be register without a > > > camera component. The camera (and possible other arguments) should come > > > from either arguments [1] or a singleton parameter class [2]. > > > > > > 1. This do not seem to be possible with gtests as test parameters needs > > > to be registerd. > > > > > > 2. This would be similar to the global variable used in earlier versions > > > but instead modeled as a class singleton like CameraManager. > > > > > > Maybe there are other options tho. > > > > > > > > > > > I think here it would be helpful to think if the interface suits our problem. > > > > To me we're looking at having: > > > > > > > > lc-compliance --camera 'cameraId' --filter 'BalancedSingleCycle' > > > > > > This matches my view as well. > > > > > > > > > > > to only test camera 'cameraId' with a single test 'BalancedSingleCycle'. Here, > > > > the camera flag is used to only register tests with that camera, and the filter > > > > is passed as --gtest_filter, possibly a bit adapted, to gtest. > > > > > > > > And if we imagine we have a test ConcurrentCameras, which receives two cameras > > > > as parameters, and we call > > > > > > > > lc-compliance --camera 'cameraId1,cameraId2' > > > > > > This also looks good. But for now lets focus on one camera, I think it > > > will give us enough headache :-) > > > > > > > > > > > it would test both each of the cameras separately in all current tests which > > > > accept a single camera, like BalancedSingleCycle, as well as test both at the > > > > same time in the ConcurrentCameras test (and we could make only the former or > > > > the latter occur by using the filter parameter). > > > > > > I have not thought enough about this to know if that is a good idea or > > > not. At the top of my head I still tend to think that lc-compliance > > > tests a single 'primary' camera and any additional camera supplied are > > > only used for tests where two (or more) cameras are needed to test some > > > specific feature of the primary camera. > > > > > > That is only the 'primary' camera is testes, the other camera(s) are > > > just there to help/provoke the testing. But maybe I'm to narrow minded. > > > > Okay, so it now comes down to how lc-compliance will get used. Either: > > > > 1. lc-compliance will be called to test a single camera, with optionally other > > secondary cameras > > > > 2. lc-compliance will be called to test multiple cameras at once (not > > necessarily simultaneously, but in a single run), with the default case being > > "Test all the cameras in this system". > > > > Given that the final objective of my internship is to have lc-compliance running > > on real boards by KernelCI, to catch any kernel regressions that affect camera > > usage, I'm trying to think how it would work there. (I wonder if this discussion > > should be happening together with KernelCI to decide on this...) > > > > So I'm imagining a RockPi4 board with a camera sensor plugged in, running > > lc-compliance to catch any kernel regressions. > > > > The limitation I see in 1 is that if a system has multiple cameras and want to > > ensure that all keep working, it will need to either hardcode all camera ids and > > manually call lc-compliance once for each one, or have a separate program that > > lists all cameras from libcamera (using 'cam -l' for example) and calls > > lc-compliance with each one. That's to say, running on all cameras is not as > > easy. And then there are multiple reports, one for each camera, instead of a > > single one for all cameras, which might be good or bad, not sure. > > > > That said, since the RockPi4 (and also the Raspberry Pi 4) has a single CSI-2 > > connector, I believe it only supports a single camera anyway, so this shouldn't > > be a problem for this board, but I'm not sure about others. Phones for example > > seem to have at least two independent cameras... > > I now understand your point of view better, thanks for brining up the > KernelCI angle. I agree from a KernelCI perspective the goal is likely > to test all cameras of the system to get the widest test base. But from > a (current) libcamera perspective the lc-complinance tool primary usage > is to be able to test a particular camera (or pipeline) to prove changes > do not break things, kind of like v4l2-complinace is for V4L2. > > That being said I think it's trivial for us to make lc-complinance > satisfy both use-cases. If the arg parsing is structured something like > this, > > if (cliargs.continas("--camera")) { > run_tests_for_camera(cliargs.value_for("--camera"); > } else if (cliargs.continas("--run-all-cameras")) { > for (camera : camer_manager.list_all_cameras()) { > run_tests_for_camera(camera) > } > } > > Or am I missing something? Unfortunately I don't think that would work. All tests would run fine, but the issue is in the results reporting at the end. Instead of having a single report, with the right number of tests failed, skipped or succeeded and details for each one, there would be N consecutive reports, where N is the number of cameras, each one reporting about its own camera. From a user perspective that would be very confusing, since you'd need to scroll through each of the reports to check the result for each camera. Another issue is that for KernelCI we need machine-parsable results, which we can do with --gtest_output=json to write the result in the JSON format into a file, but then I bet each camera's result would overwrite the previous one (haven't tried it yet, though). (Although thinking a bit more about it now, we could take advantage of the fact that --gtest_output=(json|xml)[:DIRECTORY_PATH/|:FILE_PATH] allows to set the file name to make a sure each camera result is written to a different file, but this still doesn't solve the first case with the text output to the terminal for the user) So I don't see any way to have lc-compliance work with multiple cameras and have a single complete results report without relying on dynamic registration of tests for each camera. In any case, given your answer in the second email that we could indeed use the singleton pattern and have the camera globally available, meaning we could go back to static registration of tests, I think it's worth it to have lc-compliance focused on a single camera (with optional secondary ones) and push the burden of how to test all cameras to KernelCI (which I'll probably be the one to solve anyway :P) in exchange for a lot easier test registration (and getting rid of that REGISTER_TESTS macro I wrote that I hated from the start :) ). So assuming you're okay with that, I'll get working on it for v3. Thanks, Nícolas > > > > > Regarding the benefit of 1, I'm not sure. If we were to use a global variable > > like in v1, it would make the code a lot simpler by making the test registration > > static. But that doesn't seem to be an option :). I'm not familiar with using a > > Singleton, so I'll take a look at CameraManager. But I assume it wouldn't be > > accessible statically, so the dynamic test registration complexities would have > > to stay. > > > > Also, just to make sure we're on the same page, by using a Singleton, and again > > assuming it isn't stored in a global variable to be accessible from the tests > > statically, even though we would eliminate the camera as a parameter for the > > tests, the reference to the singleton itself would need to be added as a > > parameter for the tests. > > I will follow up on this in your second mail. > > > > > Thanks, > > Nícolas > > > > > > > > > > > > > I know we don't need to implement support for multiple cameras right now, I just > > > > want to make sure it's possible and thinking about the interface and the > > > > scenarios help me picture how they'd work. > > > > > > > > Thoughts? > > > > > > > > Thanks, > > > > Nícolas > > > > > > > > > > > > > > I'm sure there are many solutions to this problem and maybe someone more > > > > > familiar with gtest knows of a neat way to piggyback on its > > > > > infrastrcuture? Since I know very little about the gtest framework the > > > > > following is just an idea and I'm happy to yield. > > > > > > > > > > How about using a singleton class that carries arguments constructed > > > > > from cli arguments (camera id) before the gtest are executed. Each test > > > > > case could then consult this singleton if it needs to access parameters. > > > > > I think you could take inspiration from the libcamera CameraManager > > > > > class. > > > > > > > > > > This could (at lest for now) imply keeping the cam option parser. If so > > > > > I think it would be nice if somewhen before we integrate this work that > > > > > the gtest filter string could be supplied as an argument using that > > > > > parser. Of course if we can do it the other way around and add arguments > > > > > to the gtest parser that would be even cooler as it would be less code > > > > > for us to maintain ;-) > > -- > Regards, > Niklas Söderlund > > -- > To unsubscribe, send mail to kernel-unsubscribe@lists.collabora.co.uk.
Hello Nícolas, On 2021-05-12 10:43:35 -0300, Nícolas F. R. A. Prado wrote: > Hi Niklas, > > Em 2021-05-12 05:49, Niklas Söderlund escreveu: > > > Hi Nícolas, > > > > On 2021-05-11 14:17:56 -0300, Nícolas F. R. A. Prado wrote: > > > Hi Niklas, > > > > > > Em 2021-05-10 18:04, Niklas Söderlund escreveu: > > > > > > > Hi Nícolas, > > > > > > > > Thanks for your work on this. I really think gtest is a step in the > > > > right direction! > > > > > > > > On 2021-05-07 12:52:38 -0300, Nícolas F. R. A. Prado wrote: > > > > > Hi Niklas, > > > > > > > > > > Em 2021-05-07 06:11, Niklas Söderlund escreveu: > > > > > > > > > > > Hi Nícolas, > > > > > > > > > > > > On 2021-05-06 17:20:29 -0300, Nícolas F. R. A. Prado wrote: > > > > > > > Hi Niklas, > > > > > > > > > > > > > > thanks for the feedback. > > > > > > > > > > > > > > Em 2021-05-06 05:48, Niklas Söderlund escreveu: > > > > > > > > > > > > > > > Hi Nícolas, > > > > > > > > > > > > > > > > Thanks for your work! > > > > > > > > > > > > > > > > I really like how gtest helps make the actual tests to be easier to > > > > > > > > read. > > > > > > > > > > > > > > > > On 2021-05-03 16:33:07 -0300, Nícolas F. R. A. Prado wrote: > > > > > > > > > Refactor lc-compliance using Googletest as the test framework. > > > > > > > > > > > > > > > > > > Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com> > > > > > > > > > --- > > > > > > > > > Changes in v2: > > > > > > > > > - Changed from static to dynamic test registration > > > > > > > > > - Removed -c flag > > > > > > > > > > > > > > > > > > There's still an issue with the refactoring that the shared_ptr of the tests > > > > > > > > > apparently aren't being deleted on time, which causes: > > > > > > > > > > > > > > > > > > [20:26:08.744507935] [103243] ERROR DeviceEnumerator device_enumerator.cpp:165 Removing media device /dev/media0 while still in use > > > > > > > > > [20:26:08.744548175] [103243] ERROR DeviceEnumerator device_enumerator.cpp:165 Removing media device /dev/media1 while still in use > > > > > > > > > Segmentation fault (core dumped) > > > > > > > > > > > > > > > > > > I tried explicitly resetting the shared_ptr on the destructor of the tests but > > > > > > > > > that didn't work. In fact, when just listing the tests, the constructor for the > > > > > > > > > tests isn't even called. So I think the issue has to do with the passing of the > > > > > > > > > camera shared pointer to the REGISTER_TESTS() macro through that functor (?): > > > > > > > > > > > > > > > > > > [=]() -> testsuite* { return new testcase(c, r, rq); }); \ > > > > > > > > > > > > > > > > > > Not sure how to solve this. Any tip would be welcome. > > > > > > > > > > > > > > > > I will leave this for now as I have a different worry about > > > > > > > > REGISTER_TESTS() design below. > > > > > > > > > > > > > > > > > > > > > > > > > > src/lc-compliance/main.cpp | 99 +++++++++--------- > > > > > > > > > src/lc-compliance/meson.build | 3 + > > > > > > > > > src/lc-compliance/simple_capture.cpp | 74 +++++-------- > > > > > > > > > src/lc-compliance/simple_capture.h | 8 +- > > > > > > > > > src/lc-compliance/single_stream.cpp | 151 ++++++++++++++------------- > > > > > > > > > src/lc-compliance/tests.h | 15 ++- > > > > > > > > > 6 files changed, 175 insertions(+), 175 deletions(-) > > > > > > > > > > > > > > > > > > diff --git a/src/lc-compliance/main.cpp b/src/lc-compliance/main.cpp > > > > > > > > > index 54cee54aa978..add0d7729aec 100644 > > > > > > > > > --- a/src/lc-compliance/main.cpp > > > > > > > > > +++ b/src/lc-compliance/main.cpp > > > > > > > > > @@ -9,6 +9,8 @@ > > > > > > > > > #include <iostream> > > > > > > > > > #include <string.h> > > > > > > > > > > > > > > > > > > +#include <gtest/gtest.h> > > > > > > > > > + > > > > > > > > > #include <libcamera/libcamera.h> > > > > > > > > > > > > > > > > > > #include "../cam/options.h" > > > > > > > > > @@ -17,7 +19,6 @@ > > > > > > > > > using namespace libcamera; > > > > > > > > > > > > > > > > > > enum { > > > > > > > > > - OptCamera = 'c', > > > > > > > > > OptHelp = 'h', > > > > > > > > > }; > > > > > > > > > > > > > > > > > > @@ -28,14 +29,15 @@ public: > > > > > > > > > ~Harness(); > > > > > > > > > > > > > > > > > > int exec(); > > > > > > > > > + int init(); > > > > > > > > > + void registerTests(); > > > > > > > > > > > > > > > > > > private: > > > > > > > > > - int init(); > > > > > > > > > void listCameras(); > > > > > > > > > > > > > > > > > > OptionsParser::Options options_; > > > > > > > > > std::unique_ptr<CameraManager> cm_; > > > > > > > > > - std::shared_ptr<Camera> camera_; > > > > > > > > > + std::vector<std::shared_ptr<Camera>> cameras_; > > > > > > > > > }; > > > > > > > > > > > > > > > > > > Harness::Harness(const OptionsParser::Options &options) > > > > > > > > > @@ -46,33 +48,14 @@ Harness::Harness(const OptionsParser::Options &options) > > > > > > > > > > > > > > > > > > Harness::~Harness() > > > > > > > > > { > > > > > > > > > - if (camera_) { > > > > > > > > > - camera_->release(); > > > > > > > > > - camera_.reset(); > > > > > > > > > + for (auto &c : cameras_) { > > > > > > > > > + c->release(); > > > > > > > > > + c.reset(); > > > > > > > > > } > > > > > > > > > > > > > > > > > > cm_->stop(); > > > > > > > > > } > > > > > > > > > > > > > > > > > > -int Harness::exec() > > > > > > > > > -{ > > > > > > > > > - int ret = init(); > > > > > > > > > - if (ret) > > > > > > > > > - return ret; > > > > > > > > > - > > > > > > > > > - std::vector<Results> results; > > > > > > > > > - > > > > > > > > > - results.push_back(testSingleStream(camera_)); > > > > > > > > > - > > > > > > > > > - for (const Results &result : results) { > > > > > > > > > - ret = result.summary(); > > > > > > > > > - if (ret) > > > > > > > > > - return ret; > > > > > > > > > - } > > > > > > > > > - > > > > > > > > > - return 0; > > > > > > > > > -} > > > > > > > > > - > > > > > > > > > int Harness::init() > > > > > > > > > { > > > > > > > > > int ret = cm_->start(); > > > > > > > > > @@ -82,42 +65,26 @@ int Harness::init() > > > > > > > > > return ret; > > > > > > > > > } > > > > > > > > > > > > > > > > > > - if (!options_.isSet(OptCamera)) { > > > > > > > > > - std::cout << "No camera specified, available cameras:" << std::endl; > > > > > > > > > - listCameras(); > > > > > > > > > - return -ENODEV; > > > > > > > > > - } > > > > > > > > > - > > > > > > > > > - const std::string &cameraId = options_[OptCamera]; > > > > > > > > > - camera_ = cm_->get(cameraId); > > > > > > > > > - if (!camera_) { > > > > > > > > > - std::cout << "Camera " << cameraId << " not found, available cameras:" << std::endl; > > > > > > > > > - listCameras(); > > > > > > > > > - return -ENODEV; > > > > > > > > > + for (auto cam : cm_->cameras()) { > > > > > > > > > + if (cam->acquire()) { > > > > > > > > > + std::cout << "Failed to acquire camera" << std::endl; > > > > > > > > > + return -EINVAL; > > > > > > > > > + } > > > > > > > > > + cameras_.push_back(cam); > > > > > > > > > > > > > > > > I don't like this, I think we need to retain the ability to select which > > > > > > > > camera to test. And further down the line maybe allow more then one > > > > > > > > camera to be selected if we want to test concurrent use of two specific > > > > > > > > ones. But for the work in this series I think you can ignore multiple > > > > > > > > cameras. > > > > > > > > > > > > > > Actually, that doesn't mean that multiple cameras will be tested, that's > > > > > > > determined by the test filter, so doing --gtest_filter='.*idCamera1.*' would > > > > > > > only test that camera. But I think I see what you mean, I shouldn't be acquiring > > > > > > > all cameras, only the ones that I will actually test (and in this code there's a > > > > > > > big issue where if a single camera is already busy, even if it won't be tested, > > > > > > > lc-compliance as a whole fails). > > > > > > > > > > > > Ahh I understand. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > If I understand the coverletter correctly the correct thing here the cli > > > > > > > > arguments should be feed to gtest directly to be able to filter on what > > > > > > > > tests are run. Is there support in getst to also pass arguments to > > > > > > > > tests? Maybe we could remove the need to parse arguments ourself > > > > > > > > completely? > > > > > > > > > > > > > > There isn't any CLI argument in gtest to pass arguments to tests, but I think we > > > > > > > could solve both problems (this and the one above) like this: > > > > > > > > > > > > > > - In the main(), query the id of each camera from libcamera. > > > > > > > - Register each of the tests dynamically in gtest passing as an argument the > > > > > > > camera id, rather than a camera pointer (which is the current behavior). > > > > > > > - To select which tests to run, and on which camera(s), the user just uses > > > > > > > --gtest_filter='testToRun.*idCamera' (or '.*idCamera.*' to run all tests for > > > > > > > the camera). > > > > > > > - In the startup of each test the camera is acquired for that test, and when > > > > > > > it's done it is released. > > > > > > > > > > > > > > Implications: > > > > > > > - We no longer need to parse arguments and can drop that code, as both camera > > > > > > > and test are specified directly with gtest's filter. > > > > > > > - We only acquire the cameras that we'll actually test and only while they're > > > > > > > being tested. > > > > > > > - As a downside, a single camera will probably be acquired multiple times during > > > > > > > a single run as lc-compliance moves from one test to the next, but I don't > > > > > > > think this would be a big issue. (Maybe we could do this at the test suite > > > > > > > level instead to greatly decrease the number of times of acquire(), but I'm > > > > > > > not sure if it's possible, since we instantiate, by passing the camera id, at > > > > > > > the test level, I'll have to test that). > > > > > > > > > > > > > > What do you think? > > > > > > > > > > > > I understand what you are trying to do but I see two issues. > > > > > > > > > > > > 1. It depends on the implicit knowledge of users how to build the filter > > > > > > string and is rather unintuitive compared to having two distinct > > > > > > arguments, one to select camera and one to express the test filter. > > > > > > > > > > > > This could possibly be worked around by keeping the cam options code > > > > > > and building the gtest filter string internally by injecitng the > > > > > > selected camera name in the correct location, but the we have the > > > > > > next issue, > > > > > > > > > > > > 2. I don't think it will be practically possible to express a test > > > > > > filter that selects a specific combination of two cameras. > > > > > > > > > > > > I do think we need to be able to parameterize test cases. If gtest does > > > > > > not allow to do so using function arguments there are other ways of > > > > > > doing so. > > > > > > > > > > I think I didn't express myself well. I meant to say that gtest's CLI doesn't > > > > > have a flag to allow passing parameters to tests, meaning that it would not be > > > > > possible to parametrize the tests from the command line and also remove our > > > > > own parser and rely on gtest's. > > > > > > > > Check, I think we need to keep our own parser for now as we need to be > > > > able to parametrize the tests from the CLI. > > > > > > > > > But gtest does allow test parametrization at runtime and I'm already relying on > > > > > it since v2. Let me go a bit more over the way I'm doing this. > > > > > > > > > > - I have a test defined, eg BalancedSingleCycle, which takes some parameters: > > > > > > > > > > std::shared_ptr<Camera> camera, StreamRole role, unsigned int numRequests > > > > > > > > > > - Since we may want to run the same test for all cameras, roles and numRequests, > > > > > I register it, at runtime, using my REGISTER_TESTS() macro, which calls > > > > > gtest's testing::RegisterTest() with all possible combinations of parameters. > > > > > > > > > > - Based on the --gtest_filter CLI argument from gtest, it decides which tests > > > > > are actually run > > > > > > > > > > So the key point is that even though I parametrize the tests, I do it for all > > > > > combinations in order to later rely on gtest_filter to select which tests to > > > > > run. > > > > > > > > I see, that explains how the parameters get filled. I noticed it but did > > > > not dig further, see below for an expansion of my thoughts now that I > > > > understand this detail :-) > > > > > > > > > > > > > > Now, I do agree with both of your points. And if we want to make the interface > > > > > more intuitive to use (which I also find important), there's no way around > > > > > having our own parser. > > > > > > > > > > And if we're using our own camera flag, then it makes no sense to register tests > > > > > for all cameras, we can register tests only with the specified camera. > > > > > > > > This feels wrong. It feels like tests should be register without a > > > > camera component. The camera (and possible other arguments) should come > > > > from either arguments [1] or a singleton parameter class [2]. > > > > > > > > 1. This do not seem to be possible with gtests as test parameters needs > > > > to be registerd. > > > > > > > > 2. This would be similar to the global variable used in earlier versions > > > > but instead modeled as a class singleton like CameraManager. > > > > > > > > Maybe there are other options tho. > > > > > > > > > > > > > > I think here it would be helpful to think if the interface suits our problem. > > > > > To me we're looking at having: > > > > > > > > > > lc-compliance --camera 'cameraId' --filter 'BalancedSingleCycle' > > > > > > > > This matches my view as well. > > > > > > > > > > > > > > to only test camera 'cameraId' with a single test 'BalancedSingleCycle'. Here, > > > > > the camera flag is used to only register tests with that camera, and the filter > > > > > is passed as --gtest_filter, possibly a bit adapted, to gtest. > > > > > > > > > > And if we imagine we have a test ConcurrentCameras, which receives two cameras > > > > > as parameters, and we call > > > > > > > > > > lc-compliance --camera 'cameraId1,cameraId2' > > > > > > > > This also looks good. But for now lets focus on one camera, I think it > > > > will give us enough headache :-) > > > > > > > > > > > > > > it would test both each of the cameras separately in all current tests which > > > > > accept a single camera, like BalancedSingleCycle, as well as test both at the > > > > > same time in the ConcurrentCameras test (and we could make only the former or > > > > > the latter occur by using the filter parameter). > > > > > > > > I have not thought enough about this to know if that is a good idea or > > > > not. At the top of my head I still tend to think that lc-compliance > > > > tests a single 'primary' camera and any additional camera supplied are > > > > only used for tests where two (or more) cameras are needed to test some > > > > specific feature of the primary camera. > > > > > > > > That is only the 'primary' camera is testes, the other camera(s) are > > > > just there to help/provoke the testing. But maybe I'm to narrow minded. > > > > > > Okay, so it now comes down to how lc-compliance will get used. Either: > > > > > > 1. lc-compliance will be called to test a single camera, with optionally other > > > secondary cameras > > > > > > 2. lc-compliance will be called to test multiple cameras at once (not > > > necessarily simultaneously, but in a single run), with the default case being > > > "Test all the cameras in this system". > > > > > > Given that the final objective of my internship is to have lc-compliance running > > > on real boards by KernelCI, to catch any kernel regressions that affect camera > > > usage, I'm trying to think how it would work there. (I wonder if this discussion > > > should be happening together with KernelCI to decide on this...) > > > > > > So I'm imagining a RockPi4 board with a camera sensor plugged in, running > > > lc-compliance to catch any kernel regressions. > > > > > > The limitation I see in 1 is that if a system has multiple cameras and want to > > > ensure that all keep working, it will need to either hardcode all camera ids and > > > manually call lc-compliance once for each one, or have a separate program that > > > lists all cameras from libcamera (using 'cam -l' for example) and calls > > > lc-compliance with each one. That's to say, running on all cameras is not as > > > easy. And then there are multiple reports, one for each camera, instead of a > > > single one for all cameras, which might be good or bad, not sure. > > > > > > That said, since the RockPi4 (and also the Raspberry Pi 4) has a single CSI-2 > > > connector, I believe it only supports a single camera anyway, so this shouldn't > > > be a problem for this board, but I'm not sure about others. Phones for example > > > seem to have at least two independent cameras... > > > > I now understand your point of view better, thanks for brining up the > > KernelCI angle. I agree from a KernelCI perspective the goal is likely > > to test all cameras of the system to get the widest test base. But from > > a (current) libcamera perspective the lc-complinance tool primary usage > > is to be able to test a particular camera (or pipeline) to prove changes > > do not break things, kind of like v4l2-complinace is for V4L2. > > > > That being said I think it's trivial for us to make lc-complinance > > satisfy both use-cases. If the arg parsing is structured something like > > this, > > > > if (cliargs.continas("--camera")) { > > run_tests_for_camera(cliargs.value_for("--camera"); > > } else if (cliargs.continas("--run-all-cameras")) { > > for (camera : camer_manager.list_all_cameras()) { > > run_tests_for_camera(camera) > > } > > } > > > > Or am I missing something? > > Unfortunately I don't think that would work. All tests would run fine, but the > issue is in the results reporting at the end. Instead of having a single report, > with the right number of tests failed, skipped or succeeded and details for each > one, there would be N consecutive reports, where N is the number of cameras, > each one reporting about its own camera. From a user perspective that would be > very confusing, since you'd need to scroll through each of the reports to check > the result for each camera. Good point, for some reason I thought we could aggregate each test run and create a lc-compliance report at the end. But as you point out maybe it's best to keep a one camera test view. > > Another issue is that for KernelCI we need machine-parsable results, which we > can do with --gtest_output=json to write the result in the JSON format into a > file, but then I bet each camera's result would overwrite the previous one > (haven't tried it yet, though). > > (Although thinking a bit more about it now, we could take advantage of the fact > that --gtest_output=(json|xml)[:DIRECTORY_PATH/|:FILE_PATH] allows to set the > file name to make a sure each camera result is written to a different file, but > this still doesn't solve the first case with the text output to the terminal for > the user) For KernelCI this could be the way forward. A worst case maybe the output files from multiple lc-compliance runs can be aggregated into a single report file and then feed back to KernelCI? > > So I don't see any way to have lc-compliance work with multiple cameras and have > a single complete results report without relying on dynamic registration of > tests for each camera. > > In any case, given your answer in the second email that we could indeed use the > singleton pattern and have the camera globally available, meaning we could go > back to static registration of tests, I think it's worth it to have > lc-compliance focused on a single camera (with optional secondary ones) and push > the burden of how to test all cameras to KernelCI (which I'll probably be the > one to solve anyway :P) in exchange for a lot easier test registration (and > getting rid of that REGISTER_TESTS macro I wrote that I hated from the start :) > ). > > So assuming you're okay with that, I'll get working on it for v3. I think this sounds like a good way forward! Thanks again for working on this, I think switching to gtest really is a good step in the right direction. > > Thanks, > Nícolas > > > > > > > > > Regarding the benefit of 1, I'm not sure. If we were to use a global variable > > > like in v1, it would make the code a lot simpler by making the test registration > > > static. But that doesn't seem to be an option :). I'm not familiar with using a > > > Singleton, so I'll take a look at CameraManager. But I assume it wouldn't be > > > accessible statically, so the dynamic test registration complexities would have > > > to stay. > > > > > > Also, just to make sure we're on the same page, by using a Singleton, and again > > > assuming it isn't stored in a global variable to be accessible from the tests > > > statically, even though we would eliminate the camera as a parameter for the > > > tests, the reference to the singleton itself would need to be added as a > > > parameter for the tests. > > > > I will follow up on this in your second mail. > > > > > > > > Thanks, > > > Nícolas > > > > > > > > > > > > > > > > > I know we don't need to implement support for multiple cameras right now, I just > > > > > want to make sure it's possible and thinking about the interface and the > > > > > scenarios help me picture how they'd work. > > > > > > > > > > Thoughts? > > > > > > > > > > Thanks, > > > > > Nícolas > > > > > > > > > > > > > > > > > I'm sure there are many solutions to this problem and maybe someone more > > > > > > familiar with gtest knows of a neat way to piggyback on its > > > > > > infrastrcuture? Since I know very little about the gtest framework the > > > > > > following is just an idea and I'm happy to yield. > > > > > > > > > > > > How about using a singleton class that carries arguments constructed > > > > > > from cli arguments (camera id) before the gtest are executed. Each test > > > > > > case could then consult this singleton if it needs to access parameters. > > > > > > I think you could take inspiration from the libcamera CameraManager > > > > > > class. > > > > > > > > > > > > This could (at lest for now) imply keeping the cam option parser. If so > > > > > > I think it would be nice if somewhen before we integrate this work that > > > > > > the gtest filter string could be supplied as an argument using that > > > > > > parser. Of course if we can do it the other way around and add arguments > > > > > > to the gtest parser that would be even cooler as it would be less code > > > > > > for us to maintain ;-) > > > > -- > > Regards, > > Niklas Söderlund > > > > -- > > To unsubscribe, send mail to kernel-unsubscribe@lists.collabora.co.uk. > >
diff --git a/src/lc-compliance/main.cpp b/src/lc-compliance/main.cpp index 54cee54aa978..add0d7729aec 100644 --- a/src/lc-compliance/main.cpp +++ b/src/lc-compliance/main.cpp @@ -9,6 +9,8 @@ #include <iostream> #include <string.h> +#include <gtest/gtest.h> + #include <libcamera/libcamera.h> #include "../cam/options.h" @@ -17,7 +19,6 @@ using namespace libcamera; enum { - OptCamera = 'c', OptHelp = 'h', }; @@ -28,14 +29,15 @@ public: ~Harness(); int exec(); + int init(); + void registerTests(); private: - int init(); void listCameras(); OptionsParser::Options options_; std::unique_ptr<CameraManager> cm_; - std::shared_ptr<Camera> camera_; + std::vector<std::shared_ptr<Camera>> cameras_; }; Harness::Harness(const OptionsParser::Options &options) @@ -46,33 +48,14 @@ Harness::Harness(const OptionsParser::Options &options) Harness::~Harness() { - if (camera_) { - camera_->release(); - camera_.reset(); + for (auto &c : cameras_) { + c->release(); + c.reset(); } cm_->stop(); } -int Harness::exec() -{ - int ret = init(); - if (ret) - return ret; - - std::vector<Results> results; - - results.push_back(testSingleStream(camera_)); - - for (const Results &result : results) { - ret = result.summary(); - if (ret) - return ret; - } - - return 0; -} - int Harness::init() { int ret = cm_->start(); @@ -82,42 +65,26 @@ int Harness::init() return ret; } - if (!options_.isSet(OptCamera)) { - std::cout << "No camera specified, available cameras:" << std::endl; - listCameras(); - return -ENODEV; - } - - const std::string &cameraId = options_[OptCamera]; - camera_ = cm_->get(cameraId); - if (!camera_) { - std::cout << "Camera " << cameraId << " not found, available cameras:" << std::endl; - listCameras(); - return -ENODEV; + for (auto cam : cm_->cameras()) { + if (cam->acquire()) { + std::cout << "Failed to acquire camera" << std::endl; + return -EINVAL; + } + cameras_.push_back(cam); } - if (camera_->acquire()) { - std::cout << "Failed to acquire camera" << std::endl; - return -EINVAL; - } - - std::cout << "Using camera " << cameraId << std::endl; - return 0; } void Harness::listCameras() { - for (const std::shared_ptr<Camera> &cam : cm_->cameras()) - std::cout << "- " << cam.get()->id() << std::endl; + for (const std::shared_ptr<Camera> &c : cm_->cameras()) + std::cout << "- " << c.get()->id() << std::endl; } static int parseOptions(int argc, char **argv, OptionsParser::Options *options) { OptionsParser parser; - parser.addOption(OptCamera, OptionString, - "Specify which camera to operate on, by id", "camera", - ArgumentRequired, "camera"); parser.addOption(OptHelp, OptionNone, "Display this help message", "help"); @@ -133,6 +100,31 @@ static int parseOptions(int argc, char **argv, OptionsParser::Options *options) return 0; } +/* + * Make asserts act like exceptions, otherwise they only fail (or skip) the + * current function. From gtest documentation: + * https://google.github.io/googletest/advanced.html#asserting-on-subroutines-with-an-exception + */ +class ThrowListener : public testing::EmptyTestEventListener { + void OnTestPartResult(const testing::TestPartResult& result) override { + if (result.type() == testing::TestPartResult::kFatalFailure + || result.type() == testing::TestPartResult::kSkip) { + throw testing::AssertionException(result); + } + } +}; + + +void Harness::registerTests() { + std::map<StreamRole, std::string> roles = {{Raw, "Raw"}, + {StillCapture, "Still"}, + {VideoRecording, "Video"}, + {Viewfinder, "Viewfinder"}}; + std::vector<int> requests = {1, 2, 3, 5, 8, 13, 21, 34, 55, 89}; + + registerSingleStreamTests(cameras_, roles, requests); +} + int main(int argc, char **argv) { OptionsParser::Options options; @@ -143,6 +135,13 @@ int main(int argc, char **argv) return EXIT_FAILURE; Harness harness(options); + ret = harness.init(); + if (ret) + return ret; + + harness.registerTests(); - return harness.exec() ? EXIT_FAILURE : EXIT_SUCCESS; + ::testing::InitGoogleTest(&argc, argv); + testing::UnitTest::GetInstance()->listeners().Append(new ThrowListener); + return RUN_ALL_TESTS(); } diff --git a/src/lc-compliance/meson.build b/src/lc-compliance/meson.build index a2bfcceb1259..704bc18af3e1 100644 --- a/src/lc-compliance/meson.build +++ b/src/lc-compliance/meson.build @@ -18,10 +18,13 @@ lc_compliance_sources = files([ 'single_stream.cpp', ]) +gtest_dep = dependency('gtest') + lc_compliance = executable('lc-compliance', lc_compliance_sources, dependencies : [ libatomic, libcamera_dep, libevent, + gtest_dep, ], install : true) diff --git a/src/lc-compliance/simple_capture.cpp b/src/lc-compliance/simple_capture.cpp index f90fe6d0f9aa..7731eb16f8c2 100644 --- a/src/lc-compliance/simple_capture.cpp +++ b/src/lc-compliance/simple_capture.cpp @@ -5,6 +5,8 @@ * simple_capture.cpp - Simple capture helper */ +#include <gtest/gtest.h> + #include "simple_capture.h" using namespace libcamera; @@ -20,38 +22,34 @@ SimpleCapture::~SimpleCapture() stop(); } -Results::Result SimpleCapture::configure(StreamRole role) +void SimpleCapture::configure(StreamRole role) { config_ = camera_->generateConfiguration({ role }); - if (!config_) - return { Results::Skip, "Role not supported by camera" }; + if (!config_) { + std::cout << "Role not supported by camera" << std::endl; + GTEST_SKIP(); + } if (config_->validate() != CameraConfiguration::Valid) { config_.reset(); - return { Results::Fail, "Configuration not valid" }; + FAIL() << "Configuration not valid"; } if (camera_->configure(config_.get())) { config_.reset(); - return { Results::Fail, "Failed to configure camera" }; + FAIL() << "Failed to configure camera"; } - - return { Results::Pass, "Configure camera" }; } -Results::Result SimpleCapture::start() +void SimpleCapture::start() { Stream *stream = config_->at(0).stream(); - if (allocator_->allocate(stream) < 0) - return { Results::Fail, "Failed to allocate buffers" }; + ASSERT_GE(allocator_->allocate(stream), 0) << "Failed to allocate buffers"; - if (camera_->start()) - return { Results::Fail, "Failed to start camera" }; + ASSERT_TRUE(!camera_->start()) << "Failed to start camera"; camera_->requestCompleted.connect(this, &SimpleCapture::requestComplete); - - return { Results::Pass, "Started camera" }; } void SimpleCapture::stop() @@ -77,22 +75,19 @@ SimpleCaptureBalanced::SimpleCaptureBalanced(std::shared_ptr<Camera> camera) { } -Results::Result SimpleCaptureBalanced::capture(unsigned int numRequests) +void SimpleCaptureBalanced::capture(unsigned int numRequests) { - Results::Result ret = start(); - if (ret.first != Results::Pass) - return ret; + start(); Stream *stream = config_->at(0).stream(); const std::vector<std::unique_ptr<FrameBuffer>> &buffers = allocator_->buffers(stream); /* No point in testing less requests then the camera depth. */ if (buffers.size() > numRequests) { - /* Cache buffers.size() before we destroy it in stop() */ - int buffers_size = buffers.size(); - - return { Results::Skip, "Camera needs " + std::to_string(buffers_size) - + " requests, can't test only " + std::to_string(numRequests) }; + std::cout << "Camera needs " + std::to_string(buffers.size()) + + " requests, can't test only " + + std::to_string(numRequests) << std::endl; + GTEST_SKIP(); } queueCount_ = 0; @@ -103,14 +98,11 @@ Results::Result SimpleCaptureBalanced::capture(unsigned int numRequests) std::vector<std::unique_ptr<libcamera::Request>> requests; for (const std::unique_ptr<FrameBuffer> &buffer : buffers) { std::unique_ptr<Request> request = camera_->createRequest(); - if (!request) - return { Results::Fail, "Can't create request" }; + ASSERT_TRUE(request) << "Can't create request"; - if (request->addBuffer(stream, buffer.get())) - return { Results::Fail, "Can't set buffer for request" }; + ASSERT_FALSE(request->addBuffer(stream, buffer.get())) << "Can't set buffer for request"; - if (queueRequest(request.get()) < 0) - return { Results::Fail, "Failed to queue request" }; + ASSERT_GE(queueRequest(request.get()), 0) << "Failed to queue request"; requests.push_back(std::move(request)); } @@ -121,12 +113,7 @@ Results::Result SimpleCaptureBalanced::capture(unsigned int numRequests) stop(); delete loop_; - if (captureCount_ != captureLimit_) - return { Results::Fail, "Got " + std::to_string(captureCount_) + - " request, wanted " + std::to_string(captureLimit_) }; - - return { Results::Pass, "Balanced capture of " + - std::to_string(numRequests) + " requests" }; + ASSERT_EQ(captureCount_, captureLimit_); } int SimpleCaptureBalanced::queueRequest(Request *request) @@ -158,11 +145,9 @@ SimpleCaptureUnbalanced::SimpleCaptureUnbalanced(std::shared_ptr<Camera> camera) { } -Results::Result SimpleCaptureUnbalanced::capture(unsigned int numRequests) +void SimpleCaptureUnbalanced::capture(unsigned int numRequests) { - Results::Result ret = start(); - if (ret.first != Results::Pass) - return ret; + start(); Stream *stream = config_->at(0).stream(); const std::vector<std::unique_ptr<FrameBuffer>> &buffers = allocator_->buffers(stream); @@ -174,14 +159,11 @@ Results::Result SimpleCaptureUnbalanced::capture(unsigned int numRequests) std::vector<std::unique_ptr<libcamera::Request>> requests; for (const std::unique_ptr<FrameBuffer> &buffer : buffers) { std::unique_ptr<Request> request = camera_->createRequest(); - if (!request) - return { Results::Fail, "Can't create request" }; + ASSERT_TRUE(request) << "Can't create request"; - if (request->addBuffer(stream, buffer.get())) - return { Results::Fail, "Can't set buffer for request" }; + ASSERT_FALSE(request->addBuffer(stream, buffer.get())) << "Can't set buffer for request"; - if (camera_->queueRequest(request.get()) < 0) - return { Results::Fail, "Failed to queue request" }; + ASSERT_GE(camera_->queueRequest(request.get()), 0) << "Failed to queue request"; requests.push_back(std::move(request)); } @@ -192,7 +174,7 @@ Results::Result SimpleCaptureUnbalanced::capture(unsigned int numRequests) stop(); delete loop_; - return { status ? Results::Fail : Results::Pass, "Unbalanced capture of " + std::to_string(numRequests) + " requests" }; + ASSERT_FALSE(status); } void SimpleCaptureUnbalanced::requestComplete(Request *request) diff --git a/src/lc-compliance/simple_capture.h b/src/lc-compliance/simple_capture.h index d9de53fb63a3..0f8465083456 100644 --- a/src/lc-compliance/simple_capture.h +++ b/src/lc-compliance/simple_capture.h @@ -17,13 +17,13 @@ class SimpleCapture { public: - Results::Result configure(libcamera::StreamRole role); + void configure(libcamera::StreamRole role); protected: SimpleCapture(std::shared_ptr<libcamera::Camera> camera); virtual ~SimpleCapture(); - Results::Result start(); + void start(); void stop(); virtual void requestComplete(libcamera::Request *request) = 0; @@ -40,7 +40,7 @@ class SimpleCaptureBalanced : public SimpleCapture public: SimpleCaptureBalanced(std::shared_ptr<libcamera::Camera> camera); - Results::Result capture(unsigned int numRequests); + void capture(unsigned int numRequests); private: int queueRequest(libcamera::Request *request); @@ -56,7 +56,7 @@ class SimpleCaptureUnbalanced : public SimpleCapture public: SimpleCaptureUnbalanced(std::shared_ptr<libcamera::Camera> camera); - Results::Result capture(unsigned int numRequests); + void capture(unsigned int numRequests); private: void requestComplete(libcamera::Request *request) override; diff --git a/src/lc-compliance/single_stream.cpp b/src/lc-compliance/single_stream.cpp index 8318b42f42d6..eb6a6f305826 100644 --- a/src/lc-compliance/single_stream.cpp +++ b/src/lc-compliance/single_stream.cpp @@ -7,91 +7,94 @@ #include <iostream> +#include <gtest/gtest.h> + #include "simple_capture.h" #include "tests.h" using namespace libcamera; -Results::Result testRequestBalance(std::shared_ptr<Camera> camera, - StreamRole role, unsigned int startCycles, - unsigned int numRequests) -{ - SimpleCaptureBalanced capture(camera); +class SingleStream : public testing::Test { +public: + explicit SingleStream(std::shared_ptr<Camera> camera, StreamRole role, unsigned int numRequests) + : camera_(camera), role_(role), numRequests_(numRequests) {} - Results::Result ret = capture.configure(role); - if (ret.first != Results::Pass) - return ret; +protected: + std::shared_ptr<Camera> camera_; + StreamRole role_; + unsigned int numRequests_; +}; - for (unsigned int starts = 0; starts < startCycles; starts++) { - ret = capture.capture(numRequests); - if (ret.first != Results::Pass) - return ret; - } +/* + * Test single capture cycles + * + * Makes sure the camera completes the exact number of requests queued. Example + * failure is a camera that needs N+M requests queued to complete N requests to + * the application. + */ +class BalancedSingleCycle : public SingleStream { +public: + explicit BalancedSingleCycle(std::shared_ptr<Camera> camera, StreamRole role, unsigned int numRequests) + : SingleStream(camera, role, numRequests) {} - return { Results::Pass, "Balanced capture of " + - std::to_string(numRequests) + " requests with " + - std::to_string(startCycles) + " start cycles" }; -} + void TestBody() override { + SimpleCaptureBalanced capture(camera_); -Results::Result testRequestUnbalance(std::shared_ptr<Camera> camera, - StreamRole role, unsigned int numRequests) -{ - SimpleCaptureUnbalanced capture(camera); + capture.configure(role_); - Results::Result ret = capture.configure(role); - if (ret.first != Results::Pass) - return ret; + capture.capture(numRequests_); + }; +}; - return capture.capture(numRequests); -} +/* + * Test multiple start/stop cycles + * + * Makes sure the camera supports multiple start/stop cycles. Example failure is + * a camera that does not clean up correctly in its error path but is only + * tested by single-capture applications. + */ +class BalancedMultiCycle : public SingleStream { +public: + explicit BalancedMultiCycle(std::shared_ptr<Camera> camera, StreamRole role, unsigned int numRequests) + : SingleStream(camera, role, numRequests) {} -Results testSingleStream(std::shared_ptr<Camera> camera) -{ - static const std::vector<std::pair<std::string, StreamRole>> roles = { - { "raw", Raw }, - { "still", StillCapture }, - { "video", VideoRecording }, - { "viewfinder", Viewfinder }, + void TestBody() override { + unsigned int numRepeats = 3; + + SimpleCaptureBalanced capture(camera_); + + capture.configure(role_); + + for (unsigned int starts = 0; starts < numRepeats; starts++) + capture.capture(numRequests_); }; - static const std::vector<unsigned int> numRequests = { 1, 2, 3, 5, 8, 13, 21, 34, 55, 89 }; - - Results results(numRequests.size() * roles.size() * 3); - - for (const auto &role : roles) { - std::cout << "= Test role " << role.first << std::endl; - /* - * Test single capture cycles - * - * Makes sure the camera completes the exact number of requests queued. - * Example failure is a camera that needs N+M requests queued to - * complete N requests to the application. - */ - std::cout << "* Test single capture cycles" << std::endl; - for (unsigned int num : numRequests) - results.add(testRequestBalance(camera, role.second, 1, num)); - - /* - * Test multiple start/stop cycles - * - * Makes sure the camera supports multiple start/stop cycles. - * Example failure is a camera that does not clean up correctly in its - * error path but is only tested by single-capture applications. - */ - std::cout << "* Test multiple start/stop cycles" << std::endl; - for (unsigned int num : numRequests) - results.add(testRequestBalance(camera, role.second, 3, num)); - - /* - * Test unbalanced stop - * - * Makes sure the camera supports a stop with requests queued. - * Example failure is a camera that does not handle cancelation - * of buffers coming back from the video device while stopping. - */ - std::cout << "* Test unbalanced stop" << std::endl; - for (unsigned int num : numRequests) - results.add(testRequestUnbalance(camera, role.second, num)); - } - - return results; +}; + +/* + * Test unbalanced stop + * + * Makes sure the camera supports a stop with requests queued. Example failure + * is a camera that does not handle cancelation of buffers coming back from the + * video device while stopping. + */ +class Unbalanced : public SingleStream { +public: + explicit Unbalanced(std::shared_ptr<Camera> camera, StreamRole role, unsigned int numRequests) + : SingleStream(camera, role, numRequests) {} + + void TestBody() override { + SimpleCaptureUnbalanced capture(camera_); + + capture.configure(role_); + + capture.capture(numRequests_); + }; +}; + +void registerSingleStreamTests(std::vector<std::shared_ptr<Camera>> cameras, + std::map<StreamRole, std::string> roles, std::vector<int> requests) +{ + REGISTER_TESTS(SingleStream, BalancedSingleCycle, cameras, roles, requests); + REGISTER_TESTS(SingleStream, BalancedMultiCycle, cameras, roles, requests); + REGISTER_TESTS(SingleStream, Unbalanced, cameras, roles, requests); } diff --git a/src/lc-compliance/tests.h b/src/lc-compliance/tests.h index 396605214e4b..6d5a8b88c287 100644 --- a/src/lc-compliance/tests.h +++ b/src/lc-compliance/tests.h @@ -11,6 +11,19 @@ #include "results.h" -Results testSingleStream(std::shared_ptr<libcamera::Camera> camera); +void registerSingleStreamTests(std::vector<std::shared_ptr<libcamera::Camera>> cameras, + std::map<libcamera::StreamRole, std::string> roles, std::vector<int> requests); + +#define REGISTER_TESTS(testsuite, testcase, cameras, roles, requests) \ +for (auto c : cameras) { \ + for (auto [r, rn] : roles) { \ + for (auto rq : requests) { \ + testing::RegisterTest( \ + #testsuite, (std::string(#testcase) + "/" + c->id() + "/" + rn + "/" + std::to_string(rq)).c_str(), \ + nullptr, nullptr, __FILE__, __LINE__, \ + [=]() -> testsuite* { return new testcase(c, r, rq); }); \ + } \ + } \ +} #endif /* __LC_COMPLIANCE_TESTS_H__ */
Refactor lc-compliance using Googletest as the test framework. Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com> --- Changes in v2: - Changed from static to dynamic test registration - Removed -c flag There's still an issue with the refactoring that the shared_ptr of the tests apparently aren't being deleted on time, which causes: [20:26:08.744507935] [103243] ERROR DeviceEnumerator device_enumerator.cpp:165 Removing media device /dev/media0 while still in use [20:26:08.744548175] [103243] ERROR DeviceEnumerator device_enumerator.cpp:165 Removing media device /dev/media1 while still in use Segmentation fault (core dumped) I tried explicitly resetting the shared_ptr on the destructor of the tests but that didn't work. In fact, when just listing the tests, the constructor for the tests isn't even called. So I think the issue has to do with the passing of the camera shared pointer to the REGISTER_TESTS() macro through that functor (?): [=]() -> testsuite* { return new testcase(c, r, rq); }); \ Not sure how to solve this. Any tip would be welcome. src/lc-compliance/main.cpp | 99 +++++++++--------- src/lc-compliance/meson.build | 3 + src/lc-compliance/simple_capture.cpp | 74 +++++-------- src/lc-compliance/simple_capture.h | 8 +- src/lc-compliance/single_stream.cpp | 151 ++++++++++++++------------- src/lc-compliance/tests.h | 15 ++- 6 files changed, 175 insertions(+), 175 deletions(-)