[libcamera-devel] [RFC PATCH v2 3/3] lc-compliance: Refactor using Googletest

Niklas Söderlund niklas.soderlund at ragnatech.se
Fri May 7 11:11:53 CEST 2021


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 at 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 at lists.collabora.co.uk.
> 
> 

-- 
Regards,
Niklas Söderlund


More information about the libcamera-devel mailing list