[RFC PATCH v3 17/21] apps: lc-compliance: Support multiple streams in helpers

[Jacopo Mondi]

Hi Barnabás

On Thu, Jan 30, 2025 at 11:51:28AM +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>
> Reviewed-by: Paul Elder <paul.elder at ideasonboard.com>
> ---
>  src/apps/lc-compliance/helpers/capture.cpp    | 77 +++++++++++++++----
>  src/apps/lc-compliance/helpers/capture.h      |  2 +-
>  src/apps/lc-compliance/tests/capture_test.cpp |  6 +-
>  3 files changed, 66 insertions(+), 19 deletions(-)
>
> diff --git a/src/apps/lc-compliance/helpers/capture.cpp b/src/apps/lc-compliance/helpers/capture.cpp
> index 940646f6c..4a8627662 100644
> --- a/src/apps/lc-compliance/helpers/capture.cpp
> +++ b/src/apps/lc-compliance/helpers/capture.cpp
> @@ -24,13 +24,31 @@ 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";
>
> +	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 ?

The way we currently handle bufferCount is sub-optimal, so for the
time being I would leave the \todo in place

> +	 */
> +	auto largest =
> +		std::max_element(config_->begin(), config_->end(),
> +				 [](const StreamConfiguration &l, const StreamConfiguration &r)
> +				 { return l.bufferCount < r.bufferCount; });
> +
> +	assert(largest != config_->end());

Can this happen ?

> +
> +	for (auto &cfg : *config_)
> +		cfg.bufferCount = largest->bufferCount;
> +

I presume having all streams with the same buffer count makes it way
easier to handle request queuing etc. However there might be system
where this might not be possible ? I guess we'll revisit if they
appear

>  	if (config_->validate() != CameraConfiguration::Valid) {
>  		config_.reset();
>  		FAIL() << "Configuration not valid";
> @@ -74,20 +92,36 @@ void Capture::prepareRequests()
>  	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;
> +
> +	for (const auto &cfg : *config_) {
> +		const auto &buffers = allocator_.buffers(cfg.stream());
> +		ASSERT_FALSE(buffers.empty()) << "Zero buffers allocated for stream";
> +
> +		maxBuffers = std::max(maxBuffers, buffers.size());

Give the above, I guess all streams have the same buffer count ?
If that's the case, can we record it in a comment ? Otherwise when
we'll re-look at this in a few months we'll wonder why we have to
compute maxBuffers if all streams have the same buffer count.

(actually the choice of how many buffers to allocate is left to
PipelineHandler::exportBuffers(). All (?) our implementations use
bufferCount at the moment. If we want this to be enforced we should
check that all streams have allocated the same buffers, as we forced
bufferCount to have the same value for all streams ?) This can also be
recorded in a todo comment if you agree ?

> +	}
>
>  	/* No point in testing less requests then the camera depth. */
> -	if (queueLimit_ && *queueLimit_ < buffers.size()) {
> -		GTEST_SKIP() << "Camera needs " << buffers.size()
> +	if (queueLimit_ && *queueLimit_ < maxBuffers) {
> +		GTEST_SKIP() << "Camera needs " << maxBuffers
>  			     << " requests, can't test only " << *queueLimit_;
>  	}

No need for {}, right ?

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

As per the above, we could assert i < buffer.size() ?

> +
> +			ASSERT_EQ(request->addBuffer(stream, buffers[i].get()), 0)
> +				<< "Can't add buffer to request";
> +		}
>
>  		requests_.push_back(std::move(request));
>  	}
> @@ -124,11 +158,19 @@ void Capture::requestComplete(Request *request)
>
>  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";

This last check includes the above GE(0)

> +	}
>
> -	ASSERT_GE(count, 0) << "Failed to allocate buffers";
> -	EXPECT_EQ(count, config_->at(0).bufferCount) << "Allocated less buffers than expected";
> +	ASSERT_TRUE(allocator_.allocated());
>
>  	camera_->requestCompleted.connect(this, &Capture::requestComplete);
>
> @@ -144,7 +186,12 @@ void Capture::stop()
>
>  	camera_->requestCompleted.disconnect(this);
>
> -	Stream *stream = config_->at(0).stream();
>  	requests_.clear();
> -	allocator_.free(stream);
> +
> +	for (const auto &cfg : *config_) {
> +		int ret = allocator_.free(cfg.stream());
> +		EXPECT_EQ(ret, 0) << "Failed to free buffers associated with stream";

If ret doesn't have to be returned

	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 ede395e2a..48a8dadcb 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..147e17019 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(std::array{ role });

Is there any advantage in passing in a Span<StreamRole> compared to
passing a const reference to the container (it's an std::array<> in
this patch, an std::vector<> since the next one).

>
>  	capture.run(numRequests, numRequests);
>  }
> @@ -108,7 +108,7 @@ TEST_P(SingleStream, CaptureStartStop)
>
>  	Capture capture(camera_);
>
> -	capture.configure(role);
> +	capture.configure(std::array{ 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(std::array{ role });
>
>  	capture.run(numRequests);
>  }
> --
> 2.48.1
>
>