[RFC PATCH v1 10/12] apps: lc-compliance: Move request creation into common function

[Barnabás Pőcze]

2025. január 8., szerda 12:15 keltezéssel, Jacopo Mondi <jacopo.mondi at ideasonboard.com> írta:

> Hi Barnabas
> 
> On Fri, Dec 20, 2024 at 03:08:46PM +0000, Barnabás Pőcze wrote:
> > `CaptureBalanced` and `CaptureUnbalanced` share the same logic
> > for creating requests and assigning buffers, only the queueing
> > of requests is slightly different. Move this common part into
> > a separate function in the base class.
> >
> > Signed-off-by: Barnabás Pőcze <pobrn at protonmail.com>
> > ---
> >  src/apps/lc-compliance/helpers/capture.cpp | 66 +++++++++++-----------
> >  src/apps/lc-compliance/helpers/capture.h   |  2 +
> >  2 files changed, 35 insertions(+), 33 deletions(-)
> >
> > diff --git a/src/apps/lc-compliance/helpers/capture.cpp b/src/apps/lc-compliance/helpers/capture.cpp
> > index 5cc7635f7..7a05be9a3 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 <assert.h>
> > +
> >  #include <gtest/gtest.h>
> >
> >  using namespace libcamera;
> > @@ -74,43 +76,51 @@ void Capture::stop()
> >  	allocator_.free(stream);
> >  }
> >
> > -/* CaptureBalanced */
> > -
> > -CaptureBalanced::CaptureBalanced(std::shared_ptr<Camera> camera)
> > -	: Capture(std::move(camera))
> > +void Capture::prepareRequests(unsigned int plannedRequests)
> >  {
> > -}
> > -
> > -void CaptureBalanced::capture(unsigned int numRequests)
> > -{
> > -	start();
> > +	assert(config_);
> > +	assert(requests_.empty());
> >
> >  	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;
> > +	if (plannedRequests < buffers.size()) {
> > +		std::cout << "Camera needs " << buffers.size()
> > +			  << " requests, can't test only "
> > +			  << plannedRequests << std::endl;
> 
> I don't see this check being present in the original implementation of
> CaptureUnbalanced::capture(). What am I missing here ?

You're not missing anything, it was an oversight on my part. I did fix it for the
next version, but I am now thinking of removing this check altogether. I would
think that no matter how many requests are queued, the camera should behave, so
testing even just one request, for example, could make sense since that is not
likely to happen during normal operations. As well as it would simplify the code
even more. What do you think?


Regards,
Barnabás Pőcze


> 
> Thanks
>   j
> 
> >  		GTEST_SKIP();
> >  	}
> >
> > -	queueCount_ = 0;
> > -	captureCount_ = 0;
> > -	captureLimit_ = numRequests;
> > -
> > -	/* Queue the recommended number of requests. */
> >  	for (const std::unique_ptr<FrameBuffer> &buffer : buffers) {
> >  		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";
> >
> > -		ASSERT_EQ(queueRequest(request.get()), 0) << "Failed to queue request";
> > -
> >  		requests_.push_back(std::move(request));
> >  	}
> > +}
> > +
> > +/* CaptureBalanced */
> > +
> > +CaptureBalanced::CaptureBalanced(std::shared_ptr<Camera> camera)
> > +	: Capture(std::move(camera))
> > +{
> > +}
> > +
> > +void CaptureBalanced::capture(unsigned int numRequests)
> > +{
> > +	start();
> > +
> > +	queueCount_ = 0;
> > +	captureCount_ = 0;
> > +	captureLimit_ = numRequests;
> > +
> > +	prepareRequests(numRequests);
> > +
> > +	for (const auto &request : requests_)
> > +		queueRequest(request.get());
> >
> >  	/* Run capture session. */
> >  	int status = result_.wait();
> > @@ -156,23 +166,13 @@ 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) {
> > -		std::unique_ptr<Request> request = camera_->createRequest();
> > -		ASSERT_TRUE(request) << "Can't create request";
> > +	prepareRequests(numRequests);
> >
> > -		ASSERT_EQ(request->addBuffer(stream, buffer.get()), 0) << "Can't set buffer for request";
> > -
> > -		ASSERT_EQ(camera_->queueRequest(request.get()), 0) << "Failed to queue request";
> > -
> > -		requests_.push_back(std::move(request));
> > -	}
> > +	for (const auto &request : requests_)
> > +		camera_->queueRequest(request.get());
> >
> >  	/* Run capture session. */
> >  	int status = result_.wait();
> > diff --git a/src/apps/lc-compliance/helpers/capture.h b/src/apps/lc-compliance/helpers/capture.h
> > index 8582ade8e..67c29021b 100644
> > --- a/src/apps/lc-compliance/helpers/capture.h
> > +++ b/src/apps/lc-compliance/helpers/capture.h
> > @@ -26,6 +26,8 @@ protected:
> >  	void start();
> >  	void stop();
> >
> > +	void prepareRequests(unsigned int plannedRequests);
> > +
> >  	virtual void requestComplete(libcamera::Request *request) = 0;
> >
> >  	std::shared_ptr<libcamera::Camera> camera_;
> > --
> > 2.47.1
> >
> >
>