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

[Barnabás Pőcze]

Hi


2025. február 6., csütörtök 20:24 keltezéssel, Jacopo Mondi <jacopo.mondi at 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 at 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 at ideasonboard.com>
> > > > Signed-off-by: Jacopo Mondi <jacopo.mondi at ideasonboard.com>
> > > > Signed-off-by: Barnabás Pőcze <pobrn at protonmail.com>
> > > > Reviewed-by: Paul Elder <paul.elder at ideasonboard.com>
> > > > ---
> > > >  src/apps/lc-compliance/helpers/capture.cpp    | 77 +++++++++++++++----
> > > >  src/apps/lc-compliance/helpers/capture.h      |  2 +-
> > > >  src/apps/lc-compliance/tests/capture_test.cpp |  6 +-
> > > >  3 files changed, 66 insertions(+), 19 deletions(-)
> > > >
> > > > diff --git a/src/apps/lc-compliance/helpers/capture.cpp b/src/apps/lc-compliance/helpers/capture.cpp
> > > > index 940646f6c..4a8627662 100644
> > > > --- a/src/apps/lc-compliance/helpers/capture.cpp
> > > > +++ b/src/apps/lc-compliance/helpers/capture.cpp
> > > > @@ -24,13 +24,31 @@ Capture::~Capture()
> > > >  	stop();
> > > >  }
> > > >
> > > > -void Capture::configure(StreamRole role)
> > > > +void Capture::configure(libcamera::Span<const libcamera::StreamRole> roles)
> > > >  {
> > > > -	config_ = camera_->generateConfiguration({ role });
> > > > +	assert(!roles.empty());
> > > > +
> > > > +	config_ = camera_->generateConfiguration(roles);
> > > >
> > > >  	if (!config_)
> > > >  		GTEST_SKIP() << "Role not supported by camera";
> > > >
> > > > +	ASSERT_EQ(config_->size(), roles.size()) << "Unexpected number of streams in configuration";
> > > > +
> > > > +	/*
> > > > +	 * Set the buffers count to the largest value across all streams.
> > > > +	 * \todo: Should all streams from a Camera have the same buffer count ?
> > >
> > > The way we currently handle bufferCount is sub-optimal, so for the
> > > time being I would leave the \todo in place
> >
> > 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
> > > >
> > > >