[RFC PATCH v1 11/12] apps: lc-compliance: Support multiple streams in helpers

[Laurent Pinchart]

Hi Barnabás,

Thank you for the patch.

On Fri, Dec 20, 2024 at 03:08:51PM +0000, Barnabás Pőcze wrote:
> Prepare to add a test suite for capture operations with multiple
> streams.
> 
> Modify the Capture helper class to support multiple roles and streams
> in the configure() and capture() operations.
> 
> Multi-stream support will be added in next patches.
> 
> Co-developed-by: Jacopo Mondi <jacopo.mondi at ideasonboard.com>
> Signed-off-by: Jacopo Mondi <jacopo.mondi at ideasonboard.com>
> Signed-off-by: Barnabás Pőcze <pobrn at protonmail.com>
> ---
>  src/apps/lc-compliance/helpers/capture.cpp    | 83 ++++++++++++++-----
>  src/apps/lc-compliance/helpers/capture.h      |  2 +-
>  src/apps/lc-compliance/tests/capture_test.cpp |  6 +-
>  3 files changed, 66 insertions(+), 25 deletions(-)
> 
> diff --git a/src/apps/lc-compliance/helpers/capture.cpp b/src/apps/lc-compliance/helpers/capture.cpp
> index 7a05be9a3..38edb6f28 100644
> --- a/src/apps/lc-compliance/helpers/capture.cpp
> +++ b/src/apps/lc-compliance/helpers/capture.cpp
> @@ -24,15 +24,29 @@ Capture::~Capture()
>  	stop();
>  }
>  
> -void Capture::configure(StreamRole role)
> +void Capture::configure(libcamera::Span<const libcamera::StreamRole> roles)
>  {
> -	config_ = camera_->generateConfiguration({ role });
> +	assert(!roles.empty());
> +
> +	config_ = camera_->generateConfiguration(roles);
>  
>  	if (!config_) {
>  		std::cout << "Role not supported by camera" << std::endl;
>  		GTEST_SKIP();
>  	}
>  
> +	/*
> +	 * Set the buffers count to the largest value across all streams.
> +	 * \todo: Should all streams from a Camera have the same buffer count ?
> +	 */
> +	auto largest =
> +		std::max_element(config_->begin(), config_->end(),
> +				 [](const StreamConfiguration &l, const StreamConfiguration &r)
> +				 { return l.bufferCount < r.bufferCount; });

Would this (untested) be clearer/simpler ?

	unsigned int maxBufferCount =
		std::reduce(config_->begin(), config_->end(), 0,
			    [](unsigned int a, const StreamConfiguration &b) {
				    return std::max(a, b->bufferCount);
			    });

> +
> +	for (auto &cfg : *config_)
> +		cfg.bufferCount = largest->bufferCount;
> +
>  	if (config_->validate() != CameraConfiguration::Valid) {
>  		config_.reset();
>  		FAIL() << "Configuration not valid";
> @@ -46,12 +60,20 @@ void Capture::configure(StreamRole role)
>  
>  void Capture::start()
>  {
> -	Stream *stream = config_->at(0).stream();
> -	int count = allocator_.allocate(stream);
> +	assert(config_);
> +	assert(!config_->empty());
> +	assert(!allocator_.allocated());
> +
> +	for (const auto &cfg : *config_) {
> +		Stream *stream = cfg.stream();
> +		int count = allocator_.allocate(stream);
> +
> +		ASSERT_GE(count, 0) << "Failed to allocate buffers";
> +		EXPECT_EQ(count, cfg.bufferCount) << "Allocated less buffers than expected";
> +		ASSERT_EQ(count, allocator_.buffers(stream).size()) << "Unexpected number of buffers in allocator";
> +	}
>  
> -	ASSERT_GE(count, 0) << "Failed to allocate buffers";
> -	EXPECT_EQ(count, config_->at(0).bufferCount) << "Allocated less buffers than expected";
> -	ASSERT_EQ(count, allocator_.buffers(stream).size()) << "Unexpected number of buffers in allocator";
> +	ASSERT_TRUE(allocator_.allocated());
>  
>  	camera_->requestCompleted.connect(this, &Capture::requestComplete);
>  
> @@ -71,9 +93,14 @@ void Capture::stop()
>  
>  	camera_->requestCompleted.disconnect(this);
>  
> -	Stream *stream = config_->at(0).stream();
>  	requests_.clear();
> -	allocator_.free(stream);
> +
> +	for (const auto &cfg : *config_) {
> +		int res = allocator_.free(cfg.stream());

s/res/ret/ for consistency.

> +		ASSERT_EQ(res, 0) << "Failed to free buffers associated with stream";
> +	}
> +
> +	ASSERT_FALSE(allocator_.allocated());
>  }
>  
>  void Capture::prepareRequests(unsigned int plannedRequests)
> @@ -81,22 +108,36 @@ void Capture::prepareRequests(unsigned int plannedRequests)
>  	assert(config_);
>  	assert(requests_.empty());
>  
> -	Stream *stream = config_->at(0).stream();
> -	const std::vector<std::unique_ptr<FrameBuffer>> &buffers = allocator_.buffers(stream);
> +	std::size_t maxBuffers = 0;
>  
> -	/* No point in testing less requests then the camera depth. */
> -	if (plannedRequests < buffers.size()) {
> -		std::cout << "Camera needs " << buffers.size()
> -			  << " requests, can't test only "
> -			  << plannedRequests << std::endl;
> -		GTEST_SKIP();
> +	for (const auto &cfg : *config_) {
> +		const auto &buffers = allocator_.buffers(cfg.stream());
> +		ASSERT_FALSE(buffers.empty()) << "Zero buffers allocated for stream";
> +
> +		/* No point in testing less requests then the camera depth. */
> +		if (plannedRequests < buffers.size()) {
> +			std::cout << "Camera needs " << buffers.size()
> +				  << " requests, can't test only "
> +				  << plannedRequests << std::endl;
> +			GTEST_SKIP();
> +		}

I think this could be moved after the loop, testing

		if (plannedRequests < maxBuffers)

> +
> +		maxBuffers = std::max(maxBuffers, buffers.size());
>  	}
>  
> -	for (const std::unique_ptr<FrameBuffer> &buffer : buffers) {
> -		std::unique_ptr<Request> request = camera_->createRequest();
> -		ASSERT_TRUE(request) << "Can't create request";
> +	for (std::size_t i = 0; i < maxBuffers; i++) {
> +		std::unique_ptr<Request> request = camera_->createRequest(i);
> +
> +		for (const auto &cfg : *config_) {
> +			Stream *stream = cfg.stream();
> +			const auto &buffers = allocator_.buffers(stream);
> +			assert(!buffers.empty());
>  
> -		ASSERT_EQ(request->addBuffer(stream, buffer.get()), 0) << "Can't set buffer for request";
> +			if (i < buffers.size()) {

			if (i >= buffers.size())
				break;

			ASSERT_EQ(request->addBuffer(stream, buffers[i].get()), 0)
				<< "Can't add buffer to request";

Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

> +				ASSERT_EQ(request->addBuffer(stream, buffers[i].get()), 0)
> +					<< "Can't add buffer to request";
> +			}
> +		}
>  
>  		requests_.push_back(std::move(request));
>  	}
> diff --git a/src/apps/lc-compliance/helpers/capture.h b/src/apps/lc-compliance/helpers/capture.h
> index 67c29021b..b3a390941 100644
> --- a/src/apps/lc-compliance/helpers/capture.h
> +++ b/src/apps/lc-compliance/helpers/capture.h
> @@ -17,7 +17,7 @@
>  class Capture
>  {
>  public:
> -	void configure(libcamera::StreamRole role);
> +	void configure(libcamera::Span<const libcamera::StreamRole> roles);
>  
>  protected:
>  	Capture(std::shared_ptr<libcamera::Camera> camera);
> diff --git a/src/apps/lc-compliance/tests/capture_test.cpp b/src/apps/lc-compliance/tests/capture_test.cpp
> index 97465a612..c382fcf47 100644
> --- a/src/apps/lc-compliance/tests/capture_test.cpp
> +++ b/src/apps/lc-compliance/tests/capture_test.cpp
> @@ -89,7 +89,7 @@ TEST_P(SingleStream, Capture)
>  
>  	CaptureBalanced capture(camera_);
>  
> -	capture.configure(role);
> +	capture.configure(std::array{ role });
>  
>  	capture.capture(numRequests);
>  }
> @@ -108,7 +108,7 @@ TEST_P(SingleStream, CaptureStartStop)
>  
>  	CaptureBalanced capture(camera_);
>  
> -	capture.configure(role);
> +	capture.configure(std::array{ role });
>  
>  	for (unsigned int starts = 0; starts < numRepeats; starts++)
>  		capture.capture(numRequests);
> @@ -127,7 +127,7 @@ TEST_P(SingleStream, UnbalancedStop)
>  
>  	CaptureUnbalanced capture(camera_);
>  
> -	capture.configure(role);
> +	capture.configure(std::array{ role });
>  
>  	capture.capture(numRequests);
>  }

-- 
Regards,

Laurent Pinchart