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

Jacopo Mondi jacopo.mondi at ideasonboard.com
Tue Mar 19 16:59:30 CET 2024


Hi Stefan

On Fri, Mar 15, 2024 at 02:00:35PM +0100, Stefan Klug wrote:
> 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.
>

This is clearly wrong, I'll fix

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

Theoretically they could, as bufferCount is part of
StreamConfiguration. In practice the bufferCount field is ill-defined
and gets populated with the number of min buffers required by the
video device node, which doesn't differ between capture devices (from
which each stream is generated from) in any platform I know of.

I can add a \todo to keep track of this

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


More information about the libcamera-devel mailing list