[libcamera-devel,v3,2/2] lc-compliance: Add test stopping single stream with requests queued

11544 diff mbox series

Add a test which stops a camera while requests are still queued. This
intents to test cleanup paths where requests are dequeued from video
devices in an uncompleted state.

Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
---
 src/lc-compliance/simple_capture.cpp | 63 ++++++++++++++++++++++++++++
 src/lc-compliance/simple_capture.h   | 14 +++++++
 src/lc-compliance/single_stream.cpp  | 25 ++++++++++-
 3 files changed, 101 insertions(+), 1 deletion(-)

Hi Niklas,

Thank you for the patch.

On Wed, Mar 10, 2021 at 04:44:14PM +0100, Niklas Söderlund wrote:
> Add a test which stops a camera while requests are still queued. This
> intents to test cleanup paths where requests are dequeued from video

s/intents/intends/

> devices in an uncompleted state.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> ---
>  src/lc-compliance/simple_capture.cpp | 63 ++++++++++++++++++++++++++++
>  src/lc-compliance/simple_capture.h   | 14 +++++++
>  src/lc-compliance/single_stream.cpp  | 25 ++++++++++-
>  3 files changed, 101 insertions(+), 1 deletion(-)
> 
> diff --git a/src/lc-compliance/simple_capture.cpp b/src/lc-compliance/simple_capture.cpp
> index fac8db379118efbd..88fb6a8187cc5155 100644
> --- a/src/lc-compliance/simple_capture.cpp
> +++ b/src/lc-compliance/simple_capture.cpp
> @@ -147,3 +147,66 @@ void SimpleCaptureBalanced::requestComplete(Request *request)
>  	if (queueRequest(request))
>  		loop_->exit(-EINVAL);
>  }
> +
> +/* SimpleCaptureUnbalanced */
> +
> +SimpleCaptureUnbalanced::SimpleCaptureUnbalanced(std::shared_ptr<Camera> camera)
> +	: SimpleCapture(camera)
> +{
> +}
> +
> +Results::Result SimpleCaptureUnbalanced::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);
> +
> +	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 (camera_->queueRequest(request.get()) < 0) {
> +			stop();
> +			return { Results::Fail, "Failed to queue request" };
> +		}
> +
> +		requests.push_back(std::move(request));
> +	}
> +
> +	/* Run capture session. */
> +	loop_ = new EventLoop();
> +	int status = loop_->exec();
> +	stop();
> +	delete loop_;
> +
> +	return { status ? Results::Fail : Results::Pass, "Unbalanced capture of " + std::to_string(numRequests) + " requests" };
> +}
> +
> +void SimpleCaptureUnbalanced::requestComplete(Request *request)
> +{
> +	captureCount_++;
> +	if (captureCount_ >= captureLimit_) {
> +		loop_->exit(0);
> +		return;
> +	}
> +
> +	request->reuse(Request::ReuseBuffers);
> +	if (camera_->queueRequest(request))
> +		loop_->exit(-EINVAL);
> +}
> diff --git a/src/lc-compliance/simple_capture.h b/src/lc-compliance/simple_capture.h
> index 3a6afc538c623050..4693c13404cee7d2 100644
> --- a/src/lc-compliance/simple_capture.h
> +++ b/src/lc-compliance/simple_capture.h
> @@ -51,4 +51,18 @@ private:
>  	unsigned int captureLimit_;
>  };
>  
> +class SimpleCaptureUnbalanced : public SimpleCapture
> +{
> +public:
> +	SimpleCaptureUnbalanced(std::shared_ptr<libcamera::Camera> camera);
> +
> +	Results::Result capture(unsigned int numRequests);
> +
> +private:
> +	void requestComplete(libcamera::Request *request) override;
> +
> +	unsigned int captureCount_;
> +	unsigned int captureLimit_;
> +};
> +
>  #endif /* __LC_COMPLIANCE_SIMPLE_CAPTURE_H__ */
> diff --git a/src/lc-compliance/single_stream.cpp b/src/lc-compliance/single_stream.cpp
> index 0ed6f5dcfb5a516d..90903b06b255a104 100644
> --- a/src/lc-compliance/single_stream.cpp
> +++ b/src/lc-compliance/single_stream.cpp

I foresee a reorganization of the code where we'll clearly separate
helpers used by multiple tests from the actual tests (this will ask the
question of which parts from simple_capture.{cpp,h} are helpers, and
which parts are tests). Should we maybe already start naming the files
that implement the tests with a test_ prefix ?

> @@ -31,6 +31,18 @@ Results::Result testRequestBalance(std::shared_ptr<Camera> camera,
>  	return { Results::Pass, "Balanced capture of " + std::to_string(numRequests) + " requests with " + std::to_string(startCycles) + " start cycles" };
>  }
>  
> +Results::Result testRequestUnbalance(std::shared_ptr<Camera> camera,
> +				     StreamRole role, unsigned int numRequests)
> +{
> +	SimpleCaptureUnbalanced capture(camera);
> +
> +	Results::Result ret = capture.configure(role);
> +	if (ret.first != Results::Pass)
> +		return ret;
> +
> +	return capture.capture(numRequests);
> +}
> +
>  Results testSingleStream(std::shared_ptr<Camera> camera)
>  {
>  	const std::vector<std::pair<std::string, StreamRole>> roles = {
> @@ -41,7 +53,7 @@ Results testSingleStream(std::shared_ptr<Camera> camera)
>  	};
>  	const std::vector<unsigned int> numRequests = { 1, 2, 3, 5, 8, 13, 21, 34, 55, 89 };
>  
> -	Results results(numRequests.size() * roles.size() * 2);
> +	Results results(numRequests.size() * roles.size() * 3);
>  
>  	if (!camera)
>  		return results;
> @@ -69,6 +81,17 @@ Results testSingleStream(std::shared_ptr<Camera> camera)
>  		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;

Following the comments on 1/2, I think this would qualify as a separate
test from the balanced capture. I'm not sure if we need one top-level
entry point (e.g. testSingleStream()) per test, or if we should support
multiple tests per entry point.

It would be nice to have a system to auto-register tests, with names.
Not something needed right away, but we can record it in a \todo.