[RFC,v1,11/12] apps: lc-compliance: Support multiple streams in helpers
diff mbox series

Message ID 20241220150759.709756-12-pobrn@protonmail.com
State Superseded
Headers show
Series
  • apps: lc-compliance: Multi-stream tests
Related show

Commit Message

Barnabás Pőcze Dec. 20, 2024, 3:08 p.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>
---
 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(-)

Comments

Laurent Pinchart Jan. 10, 2025, 1:32 a.m. UTC | #1 
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@ideasonboard.com>
> Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> Signed-off-by: Barnabás Pőcze <pobrn@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@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);
>  }
Barnabás Pőcze Jan. 10, 2025, 9:27 a.m. UTC | #2 
Hi


2025. január 10., péntek 2:32 keltezéssel, Laurent Pinchart <laurent.pinchart@ideasonboard.com> írta:

> 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@ideasonboard.com>
> > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > Signed-off-by: Barnabás Pőcze <pobrn@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);
> 			    });

To me both look about the same in terms of readability.


> 
> > +
> > +	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";

I think that changes the behaviour, but I believe `continue` would work.


Regards,
Barnabás Pőcze

> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@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
>
Laurent Pinchart Jan. 10, 2025, 9:41 a.m. UTC | #3 
On Fri, Jan 10, 2025 at 09:27:24AM +0000, Barnabás Pőcze wrote:
> Hi
> 
> 
> 2025. január 10., péntek 2:32 keltezéssel, Laurent Pinchart <laurent.pinchart@ideasonboard.com> írta:
> 
> > 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@ideasonboard.com>
> > > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > > Signed-off-by: Barnabás Pőcze <pobrn@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);
> > 			    });
> 
> To me both look about the same in terms of readability.
> 
> 
> > 
> > > +
> > > +	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";
> 
> I think that changes the behaviour, but I believe `continue` would
> work.

Oops, indeed, I meant continue.

> > Reviewed-by: Laurent Pinchart <laurent.pinchart@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);
> > >  }

Patch
diff mbox series 

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; });
+
+	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());
+		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();
+		}
+
+		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()) {
+				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);
 }