[RFC,v3,17/21] apps: lc-compliance: Support multiple streams in helpers
diff mbox series

Message ID 20250130115001.1129305-18-pobrn@protonmail.com
State New
Headers show
Series
  • apps: lc-compliance: Multi-stream tests
Related show

Commit Message

Barnabás Pőcze Jan. 30, 2025, 11:51 a.m. UTC
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@ideasonboard.com>
Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com>
Reviewed-by: Paul Elder <paul.elder@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(-)

Comments

Jacopo Mondi Feb. 6, 2025, 5:11 p.m. UTC | #1
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@ideasonboard.com>
> Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com>
> Reviewed-by: Paul Elder <paul.elder@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
>
>
Barnabás Pőcze Feb. 6, 2025, 6:23 p.m. UTC | #2
Hi


2025. február 6., csütörtök 18:11 keltezéssel, Jacopo Mondi <jacopo.mondi@ideasonboard.com> írta:

> 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@ideasonboard.com>
> > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com>
> > Reviewed-by: Paul Elder <paul.elder@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

Sorry, I don't quite understand what you mean by "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 ?

I don't think so.


> 
> > +
> > +	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 ?

I have opened https://bugs.libcamera.org/show_bug.cgi?id=251 in relation with this topic.
The semantics of that field are not entirely clear to me. So I tried to handle
mismatching buffer counts gracefully. Nonetheless, I would not consider the
implementation here good by any means. Maybe it would indeed be better
to require the same buffer count for now.


> 
> > +	}
> >
> >  	/* 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 ?

I usually use `{}` for when there are multiple lines and checkstyle.py did not
complain, so should I change it?


> 
> >
> > -	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)

`EXPECT_*()` checks fail the test but they do not abort the execution.


> 
> > +	}
> >
> > -	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

ACK


> 
> 	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).

Well, I did not see a good enough reason not to use 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
> >
> >
Jacopo Mondi Feb. 6, 2025, 7:24 p.m. UTC | #3
Hi Barnabás

On Thu, Feb 06, 2025 at 06:23:57PM +0000, Barnabás Pőcze wrote:
> Hi
>
>
> 2025. február 6., csütörtök 18:11 keltezéssel, Jacopo Mondi <jacopo.mondi@ideasonboard.com> írta:
>
> > 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@ideasonboard.com>
> > > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > > Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com>
> > > Reviewed-by: Paul Elder <paul.elder@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
>
> Sorry, I don't quite understand what you mean by "leave the \todo in place".
>

Sorry, I was just reinforcing that bufferCount is poorly handled, and
I agree with the todo being there

>
> >
> > > +	 */
> > > +	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 ?
>
> I don't think so.
>
>
> >
> > > +
> > > +	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 ?
>
> I have opened https://bugs.libcamera.org/show_bug.cgi?id=251 in relation with this topic.

Thanks

> The semantics of that field are not entirely clear to me. So I tried to handle
> mismatching buffer counts gracefully. Nonetheless, I would not consider the
> implementation here good by any means. Maybe it would indeed be better
> to require the same buffer count for now.

Ok, then let's compute the 'largest' bufferCount as above and set all
cfg.bufferCount to that as you're doing already.

Then just make sure in prepareRequests() that all streams have the
same number of buffers and that should be it ?

>
>
> >
> > > +	}
> > >
> > >  	/* 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 ?
>
> I usually use `{}` for when there are multiple lines and checkstyle.py did not
> complain, so should I change it?
>

it's a single statement, so theoretically, yes. Practically, whatever :)

>
> >
> > >
> > > -	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() ?

If we validate that all streams have the same number of buffers you
can remove this

> >
> > > +
> > > +			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)
>
> `EXPECT_*()` checks fail the test but they do not abort the execution.

ASSERT_EQ() then ?

>
>
> >
> > > +	}
> > >
> > > -	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
>
> ACK
>
>
> >
> > 	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).
>
> Well, I did not see a good enough reason not to use one. :)
>

True that :)

Thanks
  j

>
> >
> > >
> > >  	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
> > >
> > >
Barnabás Pőcze Feb. 10, 2025, 8:03 a.m. UTC | #4
Hi


2025. február 6., csütörtök 20:24 keltezéssel, Jacopo Mondi <jacopo.mondi@ideasonboard.com> írta:

> Hi Barnabás
>
> On Thu, Feb 06, 2025 at 06:23:57PM +0000, Barnabás Pőcze wrote:
> > Hi
> >
> >
> > 2025. február 6., csütörtök 18:11 keltezéssel, Jacopo Mondi <jacopo.mondi@ideasonboard.com> írta:
> >
> > > 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@ideasonboard.com>
> > > > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > > > Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com>
> > > > Reviewed-by: Paul Elder <paul.elder@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
> >
> > Sorry, I don't quite understand what you mean by "leave the \todo in place".
> >
>
> Sorry, I was just reinforcing that bufferCount is poorly handled, and
> I agree with the todo being there
>
> >
> > >
> > > > +	 */
> > > > +	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 ?
> >
> > I don't think so.
> >
> >
> > >
> > > > +
> > > > +	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 ?
> >
> > I have opened https://bugs.libcamera.org/show_bug.cgi?id=251 in relation with this topic.
>
> Thanks
>
> > The semantics of that field are not entirely clear to me. So I tried to handle
> > mismatching buffer counts gracefully. Nonetheless, I would not consider the
> > implementation here good by any means. Maybe it would indeed be better
> > to require the same buffer count for now.
>
> Ok, then let's compute the 'largest' bufferCount as above and set all
> cfg.bufferCount to that as you're doing already.
>
> Then just make sure in prepareRequests() that all streams have the
> same number of buffers and that should be it ?

I think so. I'll rewrite this part to require matching buffer counts.


>
> >
> >
> > >
> > > > +	}
> > > >
> > > >  	/* 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 ?
> >
> > I usually use `{}` for when there are multiple lines and checkstyle.py did not
> > complain, so should I change it?
> >
>
> it's a single statement, so theoretically, yes. Practically, whatever :)
>
> >
> > >
> > > >
> > > > -	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() ?
>
> If we validate that all streams have the same number of buffers you
> can remove this
>
> > >
> > > > +
> > > > +			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)
> >
> > `EXPECT_*()` checks fail the test but they do not abort the execution.
>
> ASSERT_EQ() then ?

Oops, sorry, this was a bit of an incomplete reply. `ASSERT_*()` will cause an
exception to be thrown, so the execution of the test is aborted. So removing
either one will change the behaviour. I also think it's nice to get a different
message for the error case.


>
> >
> >
> > >
> > > > +	}
> > > >
> > > > -	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
> >
> > ACK
> >
> >
> > >
> > > 	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).
> >
> > Well, I did not see a good enough reason not to use one. :)
> >
>
> True that :)
>
> Thanks
>   j
>
> >
> > >
> > > >
> > > >  	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
> > > >
> > > >

Patch
diff mbox series

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 ?
+	 */
+	auto largest =
+		std::max_element(config_->begin(), config_->end(),
+				 [](const StreamConfiguration &l, const StreamConfiguration &r)
+				 { return l.bufferCount < r.bufferCount; });
+
+	assert(largest != config_->end());
+
+	for (auto &cfg : *config_)
+		cfg.bufferCount = largest->bufferCount;
+
 	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());
+	}
 
 	/* 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_;
 	}
 
-	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;
+
+			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";
+	}
 
-	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";
+	}
+
+	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 });
 
 	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);
 }