[PATCH v4 1/2] apps: lc-compliance: Support multiple streams in helpers

[Jacopo Mondi]

Hi Barnabás

On Fri, Mar 14, 2025 at 06:42:18PM +0100, 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. The buffer count

s/capture()/start()/ ?

> of each stream is asserted to be the same.
>
> Multi-stream support will be added in next patches.
>
> Signed-off-by: Barnabás Pőcze <barnabas.pocze at ideasonboard.com>
> ---
>  src/apps/lc-compliance/helpers/capture.cpp    | 64 ++++++++++++++-----
>  src/apps/lc-compliance/helpers/capture.h      |  2 +-
>  src/apps/lc-compliance/tests/capture_test.cpp |  6 +-
>  3 files changed, 51 insertions(+), 21 deletions(-)
>
> diff --git a/src/apps/lc-compliance/helpers/capture.cpp b/src/apps/lc-compliance/helpers/capture.cpp
> index f2c6d58ce..2a3fa3b68 100644
> --- a/src/apps/lc-compliance/helpers/capture.cpp
> +++ b/src/apps/lc-compliance/helpers/capture.cpp
> @@ -23,12 +23,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_)
> -		GTEST_SKIP() << "Role not supported by camera";
> +		GTEST_SKIP() << "Roles not supported by camera";
> +
> +	ASSERT_EQ(config_->size(), roles.size()) << "Unexpected number of streams in configuration";
> +
> +	/*
> +	 * 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; });
> +
> +	assert(largest != config_->end());

The only way this could happen if config is empty, something you check
for a few lines above

> +
> +	for (auto &cfg : *config_)
> +		cfg.bufferCount = largest->bufferCount;
>
>  	if (config_->validate() != CameraConfiguration::Valid) {
>  		config_.reset();
> @@ -103,29 +120,37 @@ void Capture::start()
>  	assert(!allocator_.allocated());
>  	assert(requests_.empty());
>
> -	Stream *stream = config_->at(0).stream();
> -	int count = allocator_.allocate(stream);
> -
> -	ASSERT_GE(count, 0) << "Failed to allocate buffers";
> -	EXPECT_EQ(count, config_->at(0).bufferCount) << "Allocated less buffers than expected";
> -
> -	const std::vector<std::unique_ptr<FrameBuffer>> &buffers = allocator_.buffers(stream);
> +	const auto bufferCount = config_->at(0).bufferCount;

This hunk changes the logic to use bufferCount in place of buffers.size()

However, the code made sure that
        EXPECT_EQ(count, config_->at(0).bufferCount)

so indeed the two are equivalent

>
>  	/* No point in testing less requests then the camera depth. */
> -	if (queueLimit_ && *queueLimit_ < buffers.size()) {
> -		GTEST_SKIP() << "Camera needs " << buffers.size()
> +	if (queueLimit_ && *queueLimit_ < bufferCount) {
> +		GTEST_SKIP() << "Camera needs " << bufferCount
>  			     << " requests, can't test only " << *queueLimit_;
>  	}
>
> -	for (const std::unique_ptr<FrameBuffer> &buffer : buffers) {
> +	for (std::size_t i = 0; i < bufferCount; i++) {
>  		std::unique_ptr<Request> request = camera_->createRequest();
>  		ASSERT_TRUE(request) << "Can't create request";
> +		requests_.push_back(std::move(request));
> +	}
>
> -		ASSERT_EQ(request->addBuffer(stream, buffer.get()), 0) << "Can't set buffer for request";
> +	for (const auto &cfg : *config_) {
> +		Stream *stream = cfg.stream();
>
> -		requests_.push_back(std::move(request));
> +		int count = allocator_.allocate(stream);
> +		ASSERT_GE(count, 0) << "Failed to allocate buffers";
> +
> +		const auto &buffers = allocator_.buffers(stream);
> +		ASSERT_EQ(buffers.size(), bufferCount) << "Mismatching buffer count";
> +
> +		for (std::size_t i = 0; i < bufferCount; i++) {
> +			ASSERT_EQ(requests_[i]->addBuffer(stream, buffers[i].get()), 0)
> +				<< "Failed to add buffer to request";
> +		}
>  	}
> +	ASSERT_TRUE(allocator_.allocated());

This has been validated by the above assretions for count > 0

> +
>  	camera_->requestCompleted.connect(this, &Capture::requestComplete);
>
>  	ASSERT_EQ(camera_->start(), 0) << "Failed to start camera";
> @@ -140,7 +165,12 @@ void Capture::stop()
>
>  	camera_->requestCompleted.disconnect(this);
>
> -	Stream *stream = config_->at(0).stream();
>  	requests_.clear();
> -	allocator_.free(stream);
> +
> +	for (const auto &cfg : *config_) {
> +		EXPECT_EQ(allocator_.free(cfg.stream()), 0)
> +			<< "Failed to free buffers associated with stream";
> +	}
> +
> +	EXPECT_FALSE(allocator_.allocated());

>  }
> diff --git a/src/apps/lc-compliance/helpers/capture.h b/src/apps/lc-compliance/helpers/capture.h
> index 0e7b848fb..ea01c11c1 100644
> --- a/src/apps/lc-compliance/helpers/capture.h
> +++ b/src/apps/lc-compliance/helpers/capture.h
> @@ -20,7 +20,7 @@ public:
>  	Capture(std::shared_ptr<libcamera::Camera> camera);
>  	~Capture();
>
> -	void configure(libcamera::StreamRole role);
> +	void configure(libcamera::Span<const libcamera::StreamRole> roles);
>  	void run(unsigned int captureLimit, std::optional<unsigned int> queueLimit = {});
>
>  private:
> diff --git a/src/apps/lc-compliance/tests/capture_test.cpp b/src/apps/lc-compliance/tests/capture_test.cpp
> index 93bed48f0..06f15bdb4 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)
>
>  	Capture capture(camera_);
>
> -	capture.configure(role);
> +	capture.configure({ { role } });
>
>  	capture.run(numRequests, numRequests);
>  }
> @@ -108,7 +108,7 @@ TEST_P(SingleStream, CaptureStartStop)
>
>  	Capture capture(camera_);
>
> -	capture.configure(role);
> +	capture.configure({ { role } });
>
>  	for (unsigned int starts = 0; starts < numRepeats; starts++)
>  		capture.run(numRequests, numRequests);
> @@ -127,7 +127,7 @@ TEST_P(SingleStream, UnbalancedStop)
>
>  	Capture capture(camera_);
>
> -	capture.configure(role);
> +	capture.configure({ { role } });
>
>  	capture.run(numRequests);

Patch looks good!

Reviewed-by: Jacopo Mondi <jacopo.mondi at ideasonboard.com>

Thanks
  j

>  }
> --
> 2.48.1
>