[RFC PATCH v1 10/12] apps: lc-compliance: Move request creation into common function

[Jacopo Mondi]

Hi Barnabas

On Fri, Dec 20, 2024 at 03:08:46PM +0000, Barnabás Pőcze wrote:
> `CaptureBalanced` and `CaptureUnbalanced` share the same logic
> for creating requests and assigning buffers, only the queueing
> of requests is slightly different. Move this common part into
> a separate function in the base class.
>
> Signed-off-by: Barnabás Pőcze <pobrn at protonmail.com>
> ---
>  src/apps/lc-compliance/helpers/capture.cpp | 66 +++++++++++-----------
>  src/apps/lc-compliance/helpers/capture.h   |  2 +
>  2 files changed, 35 insertions(+), 33 deletions(-)
>
> diff --git a/src/apps/lc-compliance/helpers/capture.cpp b/src/apps/lc-compliance/helpers/capture.cpp
> index 5cc7635f7..7a05be9a3 100644
> --- a/src/apps/lc-compliance/helpers/capture.cpp
> +++ b/src/apps/lc-compliance/helpers/capture.cpp
> @@ -7,6 +7,8 @@
>
>  #include "capture.h"
>
> +#include <assert.h>
> +
>  #include <gtest/gtest.h>
>
>  using namespace libcamera;
> @@ -74,43 +76,51 @@ void Capture::stop()
>  	allocator_.free(stream);
>  }
>
> -/* CaptureBalanced */
> -
> -CaptureBalanced::CaptureBalanced(std::shared_ptr<Camera> camera)
> -	: Capture(std::move(camera))
> +void Capture::prepareRequests(unsigned int plannedRequests)
>  {
> -}
> -
> -void CaptureBalanced::capture(unsigned int numRequests)
> -{
> -	start();
> +	assert(config_);
> +	assert(requests_.empty());
>
>  	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) {
> -		std::cout << "Camera needs " + std::to_string(buffers.size())
> -			+ " requests, can't test only "
> -			+ std::to_string(numRequests) << std::endl;
> +	if (plannedRequests < buffers.size()) {
> +		std::cout << "Camera needs " << buffers.size()
> +			  << " requests, can't test only "
> +			  << plannedRequests << std::endl;

I don't see this check being present in the original implementation of
CaptureUnbalanced::capture(). What am I missing here ?

Thanks
  j

>  		GTEST_SKIP();
>  	}
>
> -	queueCount_ = 0;
> -	captureCount_ = 0;
> -	captureLimit_ = numRequests;
> -
> -	/* Queue the recommended number of requests. */
>  	for (const std::unique_ptr<FrameBuffer> &buffer : buffers) {
>  		std::unique_ptr<Request> request = camera_->createRequest();
>  		ASSERT_TRUE(request) << "Can't create request";
>
>  		ASSERT_EQ(request->addBuffer(stream, buffer.get()), 0) << "Can't set buffer for request";
>
> -		ASSERT_EQ(queueRequest(request.get()), 0) << "Failed to queue request";
> -
>  		requests_.push_back(std::move(request));
>  	}
> +}
> +
> +/* CaptureBalanced */
> +
> +CaptureBalanced::CaptureBalanced(std::shared_ptr<Camera> camera)
> +	: Capture(std::move(camera))
> +{
> +}
> +
> +void CaptureBalanced::capture(unsigned int numRequests)
> +{
> +	start();
> +
> +	queueCount_ = 0;
> +	captureCount_ = 0;
> +	captureLimit_ = numRequests;
> +
> +	prepareRequests(numRequests);
> +
> +	for (const auto &request : requests_)
> +		queueRequest(request.get());
>
>  	/* Run capture session. */
>  	int status = result_.wait();
> @@ -156,23 +166,13 @@ void CaptureUnbalanced::capture(unsigned int numRequests)
>  {
>  	start();
>
> -	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 requests. */
> -	for (const std::unique_ptr<FrameBuffer> &buffer : buffers) {
> -		std::unique_ptr<Request> request = camera_->createRequest();
> -		ASSERT_TRUE(request) << "Can't create request";
> +	prepareRequests(numRequests);
>
> -		ASSERT_EQ(request->addBuffer(stream, buffer.get()), 0) << "Can't set buffer for request";
> -
> -		ASSERT_EQ(camera_->queueRequest(request.get()), 0) << "Failed to queue request";
> -
> -		requests_.push_back(std::move(request));
> -	}
> +	for (const auto &request : requests_)
> +		camera_->queueRequest(request.get());
>
>  	/* Run capture session. */
>  	int status = result_.wait();
> diff --git a/src/apps/lc-compliance/helpers/capture.h b/src/apps/lc-compliance/helpers/capture.h
> index 8582ade8e..67c29021b 100644
> --- a/src/apps/lc-compliance/helpers/capture.h
> +++ b/src/apps/lc-compliance/helpers/capture.h
> @@ -26,6 +26,8 @@ protected:
>  	void start();
>  	void stop();
>
> +	void prepareRequests(unsigned int plannedRequests);
> +
>  	virtual void requestComplete(libcamera::Request *request) = 0;
>
>  	std::shared_ptr<libcamera::Camera> camera_;
> --
> 2.47.1
>
>