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

[Laurent Pinchart]

Hi Niklas,

On Tue, Mar 30, 2021 at 02:31:57AM +0300, Laurent Pinchart wrote:
> On Mon, Mar 29, 2021 at 06:44:51PM +0200, Niklas Söderlund wrote:
> > 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 ?
> 
> > 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 ?

https://bugs.libcamera.org/show_bug.cgi?id=19

> > 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