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

[Niklas Söderlund]

Hi Laurent,

On 2021-03-30 02:31:54 +0300, Laurent Pinchart wrote:
> Hi Niklas,
> 
> On Mon, Mar 29, 2021 at 06:44:51PM +0200, Niklas Söderlund wrote:
> > 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.
> 
> https://bugs.libcamera.org/show_bug.cgi?id=17
> 
> > > > +
> > > > +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.
> 
> https://bugs.libcamera.org/show_bug.cgi?id=18
> 
> > > > +}
> > > > +
> > > > +/* 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.
> 
> So this means that while test cases can contain multiple test points,
> the granularity of selecting what tests to run would be at the test case
> level, not the test point level ?

Yes.

> 
> > 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.
> 
> You mention that test cases need to report how many test points they
> produce. How does this work when a test point failure is fatal and
> prevents other test points from running ?

Any test case that produces less then (or more then) the number of TAP 
test points then it reported in its test plan is a failed test. This is 
one feature of TAP I really like as it acts as a sanity check on the 
test itself. Naturally if we go with a different test framework this may 
need to be reconsider if the concept of plans do not exist.

Side note, I'm happy that you caught this special case of test cases as 
'fatal fail' was a feature we took quiet seriously in 9pm and really got 
right in the API using the 'fun' upvar and uplevel constructs of TCL :-)

https://github.com/rical/9pm/blob/master/output.tcl#L211

> 
> > 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 ;-)
> 
> Applications can also use borrowed pointers :-) I think that using a
> shared_ptr to model a reference that can be held, and a normal pointer
> for a borrowed reference that must not be stored behind the caller's
> back may make the code more explicit. There's also a small performance
> difference, which doesn't matter here.
> 
> At this point I don't think this matters much, we can revisit the code
> later.
> 
> > I'm not oppose to changing it but would prefers to keep it the way it 
> > is.
> 
> -- 
> Regards,
> 
> Laurent Pinchart

-- 
Regards,
Niklas Söderlund