[libcamera-devel] [PATCH v3 6/7] apps: lc-compliance: Support multiple streams in helpers

[Stefan Klug]

Hi Jacopo,

thanks for the patch. I did not have a setup with support for
multistream. I compiled it and ran the tests which then got skipped (as
extpected)

On Sat, Dec 30, 2023 at 05:29:11PM +0100, Jacopo Mondi 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.
> 
> Signed-off-by: Jacopo Mondi <jacopo.mondi at ideasonboard.com>
> ---
>  src/apps/lc-compliance/helpers/capture.cpp    | 85 ++++++++++++++-----
>  src/apps/lc-compliance/helpers/capture.h      |  2 +-
>  src/apps/lc-compliance/tests/capture_test.cpp | 26 +++---
>  3 files changed, 76 insertions(+), 37 deletions(-)
> 
> diff --git a/src/apps/lc-compliance/helpers/capture.cpp b/src/apps/lc-compliance/helpers/capture.cpp
> index 5aab973f0392..bb95af3d758c 100644
> --- a/src/apps/lc-compliance/helpers/capture.cpp
> +++ b/src/apps/lc-compliance/helpers/capture.cpp
> @@ -7,6 +7,8 @@
>  
>  #include "capture.h"
>  
> +#include <algorithm>
> +
>  #include <gtest/gtest.h>
>  
>  using namespace libcamera;
> @@ -22,15 +24,27 @@ Capture::~Capture()
>  	stop();
>  }
>  
> -void Capture::configure(StreamRole role)
> +void Capture::configure(Span<const StreamRole> roles)
>  {
> -	config_ = camera_->generateConfiguration({ role });
> +	config_ = camera_->generateConfiguration(roles);
>  
>  	if (!config_) {
> -		std::cout << "Role not supported by camera" << std::endl;
> +		std::cout << "Roles 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 ?
> +	 * */

Nit: Not sure about the rules here, but every comment I saw just ended with */ on the last line.

> +	auto largest =
> +		std::max_element(config_->begin(), config_->end(),
> +				 [](StreamConfiguration &l, StreamConfiguration &r)
> +				 { return l.bufferCount < r.bufferCount; });
> +
> +	for (auto &stream : *config_)
> +		stream.bufferCount = largest->bufferCount;
> +
>  	if (config_->validate() != CameraConfiguration::Valid) {
>  		config_.reset();
>  		FAIL() << "Configuration not valid";
> @@ -44,11 +58,17 @@ void Capture::configure(StreamRole role)
>  
>  void Capture::start()
>  {
> -	Stream *stream = config_->at(0).stream();
> -	int count = allocator_->allocate(stream);
> +	unsigned int i = 0;
> +	for (auto const &config : *config_) {
> +		Stream *stream = config.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";
> +		ASSERT_GE(count, 0) << "Failed to allocate buffers for stream " << i;
> +		EXPECT_EQ(count, config.bufferCount)
> +			<< "Allocated less buffers than expected for stream " << i;
> +
> +		++i;
> +	}
>  
>  	camera_->requestCompleted.connect(this, &Capture::requestComplete);
>  
> @@ -64,9 +84,12 @@ void Capture::stop()
>  
>  	camera_->requestCompleted.disconnect(this);
>  
> -	Stream *stream = config_->at(0).stream();
>  	requests_.clear();
> -	allocator_->free(stream);
> +
> +	for (auto const &config : *config_) {
> +		Stream *stream = config.stream();
> +		allocator_->free(stream);
> +	}
>  }
>  
>  /* CaptureBalanced */
> @@ -80,14 +103,12 @@ void CaptureBalanced::capture(unsigned int numRequests)
>  {
>  	start();
>  
> -	Stream *stream = config_->at(0).stream();
> -	const std::vector<std::unique_ptr<FrameBuffer>> &buffers = allocator_->buffers(stream);
> -
>  	/* No point in testing less requests then the camera depth. */
> -	if (buffers.size() > numRequests) {
> -		std::cout << "Camera needs " + std::to_string(buffers.size())
> -			+ " requests, can't test only "
> -			+ std::to_string(numRequests) << std::endl;
> +	const unsigned int bufferCount = config_->at(0).bufferCount;
> +	if (bufferCount > numRequests) {
> +		std::cout << "Camera needs " << bufferCount
> +			  << " requests, can't test only " << numRequests
> +			  << std::endl;

I don't know the details here. Can different streams have different
bufferCount? I guess that is unlikely. Maybe add a comment?

>  		GTEST_SKIP();
>  	}
>  
> @@ -96,11 +117,21 @@ void CaptureBalanced::capture(unsigned int numRequests)
>  	captureLimit_ = numRequests;
>  
>  	/* Queue the recommended number of requests. */
> -	for (const std::unique_ptr<FrameBuffer> &buffer : buffers) {
> +	for (unsigned int bufferIdx = 0; bufferIdx < bufferCount; ++bufferIdx) {
>  		std::unique_ptr<Request> request = camera_->createRequest();
>  		ASSERT_TRUE(request) << "Can't create request";
>  
> -		ASSERT_EQ(request->addBuffer(stream, buffer.get()), 0) << "Can't set buffer for request";
> +		/* Add buffers for each stream. */
> +		unsigned int i = 0;
> +		for (const auto &config : *config_) {
> +			Stream *stream = config.stream();
> +			const auto &buffers = allocator_->buffers(stream);
> +
> +			ASSERT_EQ(request->addBuffer(stream, buffers[bufferIdx].get()), 0)
> +				<< "Can't add buffers for stream " << i;
> +
> +			++i;
> +		}
>  
>  		ASSERT_EQ(queueRequest(request.get()), 0) << "Failed to queue request";
>  
> @@ -152,18 +183,26 @@ void CaptureUnbalanced::capture(unsigned int numRequests)
>  {
>  	start();
>  
> -	Stream *stream = config_->at(0).stream();
> -	const std::vector<std::unique_ptr<FrameBuffer>> &buffers = allocator_->buffers(stream);
> -
>  	captureCount_ = 0;
>  	captureLimit_ = numRequests;
>  
>  	/* Queue the recommended number of requests. */
> -	for (const std::unique_ptr<FrameBuffer> &buffer : buffers) {
> +	const unsigned int bufferCount = config_->at(0).bufferCount;
> +	for (unsigned int bufferIdx = 0; bufferIdx < bufferCount; ++bufferIdx) {
>  		std::unique_ptr<Request> request = camera_->createRequest();
>  		ASSERT_TRUE(request) << "Can't create request";
>  
> -		ASSERT_EQ(request->addBuffer(stream, buffer.get()), 0) << "Can't set buffer for request";
> +		/* Add buffers for each stream. */
> +		unsigned int i = 0;
> +		for (const auto &config : *config_) {
> +			Stream *stream = config.stream();
> +			const auto &buffers = allocator_->buffers(stream);
> +
> +			ASSERT_EQ(request->addBuffer(stream, buffers[bufferIdx].get()), 0)
> +				<< "Can't add buffers for stream " << i;
> +
> +			++i;
> +		}

Same question as above. If the bufferCount could differ between the
streams, this might be problematic.

Cheers,
Stefan

>  
>  		ASSERT_EQ(camera_->queueRequest(request.get()), 0) << "Failed to queue request";
>  
> diff --git a/src/apps/lc-compliance/helpers/capture.h b/src/apps/lc-compliance/helpers/capture.h
> index 0574ab1c7ac7..3e2b2889244d 100644
> --- a/src/apps/lc-compliance/helpers/capture.h
> +++ b/src/apps/lc-compliance/helpers/capture.h
> @@ -16,7 +16,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 284d36307619..3d3cc97791d9 100644
> --- a/src/apps/lc-compliance/tests/capture_test.cpp
> +++ b/src/apps/lc-compliance/tests/capture_test.cpp
> @@ -17,14 +17,14 @@
>  using namespace libcamera;
>  
>  const std::vector<int> NUMREQUESTS = { 1, 2, 3, 5, 8, 13, 21, 34, 55, 89 };
> -const std::vector<StreamRole> ROLES = {
> -	StreamRole::Raw,
> -	StreamRole::StillCapture,
> -	StreamRole::VideoRecording,
> -	StreamRole::Viewfinder
> +const std::vector<std::vector<StreamRole>> ROLES = {
> +	{ StreamRole::Raw },
> +	{ StreamRole::StillCapture },
> +	{ StreamRole::VideoRecording },
> +	{ StreamRole::Viewfinder },
>  };
>  
> -class SingleStream : public testing::TestWithParam<std::tuple<StreamRole, int>>
> +class SingleStream : public testing::TestWithParam<std::tuple<std::vector<StreamRole>, int>>
>  {
>  public:
>  	static std::string nameParameters(const testing::TestParamInfo<SingleStream::ParamType> &info);
> @@ -67,7 +67,7 @@ std::string SingleStream::nameParameters(const testing::TestParamInfo<SingleStre
>  		{ StreamRole::Viewfinder, "Viewfinder" }
>  	};
>  
> -	std::string roleName = rolesMap[std::get<0>(info.param)];
> +	std::string roleName = rolesMap[std::get<0>(info.param)[0]];
>  	std::string numRequestsName = std::to_string(std::get<1>(info.param));
>  
>  	return roleName + "_" + numRequestsName;
> @@ -82,11 +82,11 @@ std::string SingleStream::nameParameters(const testing::TestParamInfo<SingleStre
>   */
>  TEST_P(SingleStream, Capture)
>  {
> -	auto [role, numRequests] = GetParam();
> +	const auto [role, numRequests] = GetParam();
>  
>  	CaptureBalanced capture(camera_);
>  
> -	capture.configure(role);
> +	capture.configure(Span<const StreamRole>{ role });
>  
>  	capture.capture(numRequests);
>  }
> @@ -100,12 +100,12 @@ TEST_P(SingleStream, Capture)
>   */
>  TEST_P(SingleStream, CaptureStartStop)
>  {
> -	auto [role, numRequests] = GetParam();
> +	const auto [role, numRequests] = GetParam();
>  	unsigned int numRepeats = 3;
>  
>  	CaptureBalanced capture(camera_);
>  
> -	capture.configure(role);
> +	capture.configure(Span<const StreamRole>{ role });
>  
>  	for (unsigned int starts = 0; starts < numRepeats; starts++)
>  		capture.capture(numRequests);
> @@ -120,11 +120,11 @@ TEST_P(SingleStream, CaptureStartStop)
>   */
>  TEST_P(SingleStream, UnbalancedStop)
>  {
> -	auto [role, numRequests] = GetParam();
> +	const auto [role, numRequests] = GetParam();
>  
>  	CaptureUnbalanced capture(camera_);
>  
> -	capture.configure(role);
> +	capture.configure(Span<const StreamRole>{ role });
>  
>  	capture.capture(numRequests);
>  }