[libcamera-devel] [PATCH v3 1/2] lc-compliance: Add a libcamera compliance tool

[Niklas Söderlund]

Hi Laurent,

Thanks for your feedback.

On 2021-03-14 22:47:17 +0200, Laurent Pinchart wrote:

<snip>

> > diff --git a/src/lc-compliance/simple_capture.cpp 
> > b/src/lc-compliance/simple_capture.cpp
> > new file mode 100644
> > index 0000000000000000..fac8db379118efbd
> > --- /dev/null
> > +++ b/src/lc-compliance/simple_capture.cpp
> > @@ -0,0 +1,149 @@
> > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > +/*
> > + * Copyright (C) 2020, Google Inc.
> > + *
> > + * simple_capture.cpp - Simple capture helper
> > + */
> > +
> > +#include "simple_capture.h"
> > +
> > +using namespace libcamera;
> > +
> > +SimpleCapture::SimpleCapture(std::shared_ptr<Camera> camera)
> > +	: camera_(camera), allocator_(std::make_unique<FrameBufferAllocator>(camera))
> > +{
> > +}
> > +
> > +SimpleCapture::~SimpleCapture()
> > +{
> > +}
> > +
> > +Results::Result SimpleCapture::configure(StreamRole role)
> > +{
> > +	config_ = camera_->generateConfiguration({ role });
> > +
> > +	if (config_->validate() != CameraConfiguration::Valid) {
> > +		config_.reset();
> > +		return { Results::Fail, "Configuration not valid" };
> > +	}
> > +
> > +	if (camera_->configure(config_.get())) {
> > +		config_.reset();
> > +		return { Results::Fail, "Failed to configure camera" };
> > +	}
> > +
> > +	return { Results::Pass, "Configure camera" };
> > +}
> 
> Is returning a Result here the best option. Shouldn't test results be
> constructed by tests, and helper classes return statuses that let tests
> decide what to do ? In particular, the success message generated by this
> function will never be used, as success applies to a full test, not to
> an invidual step.
> 
> On the other hand, the helper classes have the most detailed knowledge
> about the errors they can encounter. I'm not sure what would be best. I
> wonder if this could be a valid use case for using exceptions. It would
> allow writing tests with assert-like functions. For instance, the code
> below that tests
> 
> 	if (captureCount_ != captureLimit_)
> 		return { Results::Fail, "Got " + std::to_string(captureCount_) + " request, wanted " + std::to_string(captureLimit_) };
> 
> could be replaced with a
> 
> 	failIfNotEqual(captureCount_, captureLimit, "request(s)");
> 
> which would generate the message automatically.

I think this is a very interesting idea. And I thought about it when 
writing this but shrugged it off as i thought it would be hard to get 
acceptance upstream. I have not experimented with a prototype for this, 
but maybe this is something we can do on-top as we inclemently improve 
the structure of the tool.

> 
> > +
> > +Results::Result SimpleCapture::start()
> > +{
> > +	Stream *stream = config_->at(0).stream();
> > +	if (allocator_->allocate(stream) < 0)
> > +		return { Results::Fail, "Failed to allocate buffers" };
> > +
> > +	if (camera_->start())
> > +		return { Results::Fail, "Failed to start camera" };
> > +
> > +	camera_->requestCompleted.connect(this, &SimpleCapture::requestComplete);
> > +
> > +	return { Results::Pass, "Started camera" };
> > +}
> > +
> > +Results::Result SimpleCapture::stop()
> > +{
> > +	Stream *stream = config_->at(0).stream();
> > +
> > +	camera_->stop();
> > +
> > +	camera_->requestCompleted.disconnect(this, &SimpleCapture::requestComplete);
> > +
> > +	allocator_->free(stream);
> > +
> > +	return { Results::Pass, "Stopped camera" };
> 
> Generically speaking, should this kind of function be made idempotent,
> and be called from destructors ? It would make error handling easier.

With an exception test flow I think it would make sens.

> 
> > +}
> > +
> > +/* SimpleCaptureBalanced */
> > +
> > +SimpleCaptureBalanced::SimpleCaptureBalanced(std::shared_ptr<Camera> camera)
> > +	: SimpleCapture(camera)
> > +{
> > +}
> > +
> > +Results::Result SimpleCaptureBalanced::capture(unsigned int numRequests)
> > +{
> > +	Results::Result ret = start();
> > +	if (ret.first != Results::Pass)
> > +		return ret;
> > +
> > +	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();
> > +		stop();
> > +
> > +		return { Results::Skip, "Camera needs " + std::to_string(buffers_size)
> > +			+ " requests, can't test only " + std::to_string(numRequests) };
> 
> This makes me wonder what the granularity of a test should be. What is
> "one test", should it be a call to this function with a given number of
> buffers, or one call to testSingleStream() ? I'm leaning towards the
> later, defining a test as something that can be selected in a test plan.
> I can imagine selecting testSingleStream() through a command line
> argument (or through a configuration file), going for a smaller
> granularity doesn't seem too useful. We need to log the status of
> individual steps in a test, but at the top (and TAP) level, I'd limit it
> to the higher level.

I'm heavily biased by my preference for TAP and the test framework 9pm 
(disclaimer partly developed by me) when it comes to how to structure 
tests.

The basic structure is a test case is a collection of TAP test points. 
Each case is specifies the number of test points it has and then in 
order lists if the test point is passed, skipped or failed. The test 
cases is then the worst possible aggregation of all test points. That is 
if one TAP is fail the whole test cases is fail. If one TAP is skip and 
there are no fails the whole case is skip. And only if all TAPs are pass 
is the test case PASS.

This protects against test cases terminating prematurely as the harness 
knows how may TAPs to expect before the actual testing begins.

Then you collect a list of test cases into a test suite and that is your 
top level object.

In this case the testSingleStream is a test case while it could output 
any number of pass/fail/skip TAP points. And the collection of 
testSingleStream, testRequestBalance, testRequestUnbalance would be a 
test suite with 3 test-cases.

But I think the lesson learnt over time is to tailor your test structure 
to the tool/framework you intend to use. For example 9pm would be a 
horrible match for the needs of lc-compliance. I think the best way 
forward for us is to move forward with a our own (does not have to be 
this one) implementation of a test harness and once we know which 
environment it will be used most frequently pick an existing library and 
switch to that.

> 
> > +	}
> > +
> > +	queueCount_ = 0;
> > +	captureCount_ = 0;
> > +	captureLimit_ = numRequests;
> > +
> > +	/* Queue the recommended number of reqeuests. */
> > +	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) {
> > +			stop();
> > +			return { Results::Fail, "Can't create request" };
> > +		}
> > +
> > +		if (request->addBuffer(stream, buffer.get())) {
> > +			stop();
> > +			return { Results::Fail, "Can't set buffer for request" };
> > +		}
> > +
> > +		if (queueRequest(request.get()) < 0) {
> > +			stop();
> > +			return { Results::Fail, "Failed to queue request" };
> > +		}
> > +
> > +		requests.push_back(std::move(request));
> > +	}
> > +
> > +	/* Run capture session. */
> > +	loop_ = new EventLoop();
> > +	loop_->exec();
> > +	stop();
> > +	delete loop_;
> 
> Should we construct the event loop in the constructor of the base class,
> and delete it in its destructor ?

I thought about that too but ruled again it. Mostly because we had other 
issues with the EventLoop at the time and start/stop it did not really 
work as intended. Maybe we can reopen that when we look at the Exception 
driven testing and keep it simple and readable for now?

> 
> > +
> > +	if (captureCount_ != captureLimit_)
> > +		return { Results::Fail, "Got " + std::to_string(captureCount_) + " request, wanted " + std::to_string(captureLimit_) };
> 
> This is a very long line.

I opted to keep string constructing on a single line to ease grepping in 
the code. You are now the n th person commenting about it so I will 
admit defeat and break them :-)

> 
> > +
> > +	return { Results::Pass, "Balanced capture of " + std::to_string(numRequests) + " requests" };
> > +}
> > +
> > +int SimpleCaptureBalanced::queueRequest(Request *request)
> > +{
> > +	queueCount_++;
> > +	if (queueCount_ > captureLimit_)
> > +		return 0;
> > +
> > +	return camera_->queueRequest(request);
> > +}
> > +
> > +void SimpleCaptureBalanced::requestComplete(Request *request)
> > +{
> > +	captureCount_++;
> > +	if (captureCount_ >= captureLimit_) {
> > +		loop_->exit(0);
> > +		return;
> > +	}
> > +
> > +	request->reuse(Request::ReuseBuffers);
> > +	if (queueRequest(request))
> > +		loop_->exit(-EINVAL);
> > +}
> > diff --git a/src/lc-compliance/simple_capture.h b/src/lc-compliance/simple_capture.h
> > new file mode 100644
> > index 0000000000000000..3a6afc538c623050
> > --- /dev/null
> > +++ b/src/lc-compliance/simple_capture.h
> > @@ -0,0 +1,54 @@
> > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > +/*
> > + * Copyright (C) 2020, Google Inc.
> > + *
> > + * simple_capture.h - Simple capture helper
> > + */
> > +#ifndef __LC_COMPLIANCE_SIMPLE_CAPTURE_H__
> > +#define __LC_COMPLIANCE_SIMPLE_CAPTURE_H__
> > +
> > +#include <memory>
> > +
> > +#include <libcamera/libcamera.h>
> > +
> > +#include "../cam/event_loop.h"
> > +#include "results.h"
> > +
> > +class SimpleCapture
> > +{
> > +public:
> > +	Results::Result configure(libcamera::StreamRole role);
> > +
> > +protected:
> > +	SimpleCapture(std::shared_ptr<libcamera::Camera> camera);
> 
> You can pass a libcamera::Camera * as a borrowed reference here, as the
> caller guarantees that the reference stays valid for the lifetime of
> this class.

We could, I opted to use the shared_ptr to keep the use-cases as close 
to what simple applications would use to be able to copy/past code form 
this tool in future when i forgotten the API ;-)

I'm not oppose to changing it but would prefers to keep it the way it 
is.

<snip>

-- 
Regards,
Niklas Söderlund