[libcamera-devel,v3,3/4] lc-compliance: Add test to queue more requests than hardware depth

12052 diff mbox series

A pipeline handler should be able to handle an arbitrary amount of
simultaneous requests by submitting what it can to the video device and
queuing the rest internally until resources are available. This isn't
currently done by some pipeline handlers however [1].

Add a new test to lc-compliance that submits a lot of requests at once
to check if the pipeline handler is behaving well in this situation.

[1] https://bugs.libcamera.org/show_bug.cgi?id=24

Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>
---
 src/lc-compliance/simple_capture.cpp | 70 ++++++++++++++++++++++++++++
 src/lc-compliance/simple_capture.h   | 15 ++++++
 src/lc-compliance/single_stream.cpp  | 31 +++++++++++-
 3 files changed, 115 insertions(+), 1 deletion(-)

Hi "Nícolas,

Thanks for your work.

On 2021-04-21 13:51:38 -0300, Nícolas F. R. A. Prado wrote:
> A pipeline handler should be able to handle an arbitrary amount of
> simultaneous requests by submitting what it can to the video device and
> queuing the rest internally until resources are available. This isn't
> currently done by some pipeline handlers however [1].
> 
> Add a new test to lc-compliance that submits a lot of requests at once
> to check if the pipeline handler is behaving well in this situation.
> 
> [1] https://bugs.libcamera.org/show_bug.cgi?id=24
> 
> Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>
> ---
>  src/lc-compliance/simple_capture.cpp | 70 ++++++++++++++++++++++++++++
>  src/lc-compliance/simple_capture.h   | 15 ++++++
>  src/lc-compliance/single_stream.cpp  | 31 +++++++++++-
>  3 files changed, 115 insertions(+), 1 deletion(-)
> 
> diff --git a/src/lc-compliance/simple_capture.cpp b/src/lc-compliance/simple_capture.cpp
> index 875772a80c27..01d76380e998 100644
> --- a/src/lc-compliance/simple_capture.cpp
> +++ b/src/lc-compliance/simple_capture.cpp
> @@ -223,3 +223,73 @@ void SimpleCaptureUnbalanced::requestComplete(Request *request)
>  	if (camera_->queueRequest(request))
>  		loop_->exit(-EINVAL);
>  }
> +
> +SimpleCaptureOverflow::SimpleCaptureOverflow(std::shared_ptr<Camera> camera)
> +	: SimpleCapture(camera)
> +{
> +}
> +
> +Results::Result SimpleCaptureOverflow::capture()
> +{
> +	Results::Result ret = start();
> +	if (ret.first != Results::Pass)
> +		return ret;
> +
> +	Stream *stream = config_->at(0).stream();
> +	const std::vector<std::unique_ptr<FrameBuffer>> &buffers = allocator_->buffers(stream);
> +
> +	captureCount_ = 0;
> +	captureLimit_ = buffers.size();
> +
> +	std::vector<std::unique_ptr<libcamera::Request>> requests;
> +	for (const std::unique_ptr<FrameBuffer> &buffer : buffers) {
> +		std::unique_ptr<Request> request = camera_->createRequest();
> +		if (!request) {
> +			stop();
> +			return { Results::Fail, "Can't create request" };
> +		}
> +
> +		if (request->addBuffer(stream, buffer.get())) {
> +			stop();
> +			return { Results::Fail, "Can't set buffer for request" };
> +		}
> +
> +		if (camera_->queueRequest(request.get()) < 0) {
> +			stop();
> +			return { Results::Fail, "Failed to queue request" };
> +		}
> +
> +		requests.push_back(std::move(request));
> +	}
> +
> +	/* Run capture session. */
> +	loop_ = new EventLoop();
> +	loop_->exec();
> +	stop();
> +	delete loop_;
> +
> +	if (captureCount_ != captureLimit_)
> +		return { Results::Fail, "Got " + std::to_string(captureCount_) +
> +			" request, wanted " + std::to_string(captureLimit_) };
> +
> +	return { Results::Pass, "Overflow capture of " +
> +		std::to_string(captureLimit_) + " requests" };
> +}

Reading this I now see there is quiet a bit of duplication between 
SimpleCapture*::capture() implementations. This existed before this 
patch and can be addressed on-top.

> +
> +void SimpleCaptureOverflow::requestComplete([[maybe_unused]]Request *request)
> +{
> +	captureCount_++;
> +	if (captureCount_ >= captureLimit_) {
> +		loop_->exit(0);
> +		return;
> +	}
> +}

Like above I now see this is could be moved to SimpleCapture without 
much work and the code can then be shared between all SimpleCapture* 
tests. Maybe this one is so straight forward you could do it as a pre 
patch to this one? If you like it can also be done on-top.

> +
> +Results::Result SimpleCaptureOverflow::allocateBuffers(unsigned int count)
> +{
> +	Stream *stream = config_->at(0).stream();
> +	if (allocator_->allocate(stream, count) < 0)
> +		return { Results::Fail, "Failed to allocate buffers" };
> +
> +	return { Results::Pass, "Allocated buffers" };
> +}
> diff --git a/src/lc-compliance/simple_capture.h b/src/lc-compliance/simple_capture.h
> index 82e2c56a55f1..cafcdafe10c2 100644
> --- a/src/lc-compliance/simple_capture.h
> +++ b/src/lc-compliance/simple_capture.h
> @@ -66,4 +66,19 @@ private:
>  	unsigned int captureLimit_;
>  };
>  
> +class SimpleCaptureOverflow : public SimpleCapture
> +{
> +public:
> +	SimpleCaptureOverflow(std::shared_ptr<libcamera::Camera> camera);
> +
> +	Results::Result allocateBuffers(unsigned int count);
> +	Results::Result capture();
> +
> +private:
> +	void requestComplete(libcamera::Request *request) override;
> +
> +	unsigned int captureCount_;
> +	unsigned int captureLimit_;
> +};
> +
>  #endif /* __LC_COMPLIANCE_SIMPLE_CAPTURE_H__ */
> diff --git a/src/lc-compliance/single_stream.cpp b/src/lc-compliance/single_stream.cpp
> index 649291c7bb73..61979f66d92d 100644
> --- a/src/lc-compliance/single_stream.cpp
> +++ b/src/lc-compliance/single_stream.cpp
> @@ -12,6 +12,23 @@
>  
>  using namespace libcamera;
>  
> +static const unsigned int MAX_SIMULTANEOUS_REQUESTS = 8;
> +
> +Results::Result testRequestOverflow(std::shared_ptr<Camera> camera,
> +				   StreamRole role, unsigned int numRequests)
> +{
> +	SimpleCaptureOverflow capture(camera);
> +
> +	Results::Result ret = capture.configure(role);
> +	if (ret.first != Results::Pass)
> +		return ret;
> +	capture.allocateBuffers(numRequests);
> +	if (ret.first != Results::Pass)
> +		return ret;
> +
> +	return capture.capture();
> +}
> +
>  Results::Result testRequestBalance(std::shared_ptr<Camera> camera,
>  				   StreamRole role, unsigned int startCycles,
>  				   unsigned int numRequests)
> @@ -61,7 +78,7 @@ Results testSingleStream(std::shared_ptr<Camera> camera)
>  	};
>  	static const std::vector<unsigned int> numRequests = { 1, 2, 3, 5, 8, 13, 21, 34, 55, 89 };
>  
> -	Results results(numRequests.size() * roles.size() * 3);
> +	Results results(numRequests.size() * roles.size() * 3 + roles.size() * 1);

I think you should drop the '* 1' at the end.

>  
>  	for (const auto &role : roles) {
>  		std::cout << "= Test role " << role.first << std::endl;
> @@ -97,6 +114,18 @@ Results testSingleStream(std::shared_ptr<Camera> camera)
>  		std::cout << "* Test unbalanced stop" << std::endl;
>  		for (unsigned int num : numRequests)
>  			results.add(testRequestUnbalance(camera, role.second, num));
> +
> +		/*
> +		 * Test overflowing pipeline with requests
> +		 *
> +		 * Makes sure that the camera supports receiving a large amount
> +		 * of requests at once. Example failure is a camera that doesn't
> +		 * check if there are available resources (free internal
> +		 * buffers, free buffers in the video devices) before handling a
> +		 * request.
> +		 */
> +		std::cout << "* Test overflowing requests" << std::endl;
> +		results.add(testRequestOverflow(camera, role.second, MAX_SIMULTANEOUS_REQUESTS));
>  	}
>  
>  	return results;
> -- 
> 2.31.1
>

Hi Niklas,

thank you for the feedback.

Em 2021-04-26 06:40, Niklas Söderlund escreveu:

> Hi "Nícolas,
>
> Thanks for your work.
>
> On 2021-04-21 13:51:38 -0300, Nícolas F. R. A. Prado wrote:
> > A pipeline handler should be able to handle an arbitrary amount of
> > simultaneous requests by submitting what it can to the video device and
> > queuing the rest internally until resources are available. This isn't
> > currently done by some pipeline handlers however [1].
> > 
> > Add a new test to lc-compliance that submits a lot of requests at once
> > to check if the pipeline handler is behaving well in this situation.
> > 
> > [1] https://bugs.libcamera.org/show_bug.cgi?id=24
> > 
> > Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>
> > ---
> >  src/lc-compliance/simple_capture.cpp | 70 ++++++++++++++++++++++++++++
> >  src/lc-compliance/simple_capture.h   | 15 ++++++
> >  src/lc-compliance/single_stream.cpp  | 31 +++++++++++-
> >  3 files changed, 115 insertions(+), 1 deletion(-)
> > 
> > diff --git a/src/lc-compliance/simple_capture.cpp b/src/lc-compliance/simple_capture.cpp
> > index 875772a80c27..01d76380e998 100644
> > --- a/src/lc-compliance/simple_capture.cpp
> > +++ b/src/lc-compliance/simple_capture.cpp
> > @@ -223,3 +223,73 @@ void SimpleCaptureUnbalanced::requestComplete(Request *request)
> >  	if (camera_->queueRequest(request))
> >  		loop_->exit(-EINVAL);
> >  }
> > +
> > +SimpleCaptureOverflow::SimpleCaptureOverflow(std::shared_ptr<Camera> camera)
> > +	: SimpleCapture(camera)
> > +{
> > +}
> > +
> > +Results::Result SimpleCaptureOverflow::capture()
> > +{
> > +	Results::Result ret = start();
> > +	if (ret.first != Results::Pass)
> > +		return ret;
> > +
> > +	Stream *stream = config_->at(0).stream();
> > +	const std::vector<std::unique_ptr<FrameBuffer>> &buffers = allocator_->buffers(stream);
> > +
> > +	captureCount_ = 0;
> > +	captureLimit_ = buffers.size();
> > +
> > +	std::vector<std::unique_ptr<libcamera::Request>> requests;
> > +	for (const std::unique_ptr<FrameBuffer> &buffer : buffers) {
> > +		std::unique_ptr<Request> request = camera_->createRequest();
> > +		if (!request) {
> > +			stop();
> > +			return { Results::Fail, "Can't create request" };
> > +		}
> > +
> > +		if (request->addBuffer(stream, buffer.get())) {
> > +			stop();
> > +			return { Results::Fail, "Can't set buffer for request" };
> > +		}
> > +
> > +		if (camera_->queueRequest(request.get()) < 0) {
> > +			stop();
> > +			return { Results::Fail, "Failed to queue request" };
> > +		}
> > +
> > +		requests.push_back(std::move(request));
> > +	}
> > +
> > +	/* Run capture session. */
> > +	loop_ = new EventLoop();
> > +	loop_->exec();
> > +	stop();
> > +	delete loop_;
> > +
> > +	if (captureCount_ != captureLimit_)
> > +		return { Results::Fail, "Got " + std::to_string(captureCount_) +
> > +			" request, wanted " + std::to_string(captureLimit_) };
> > +
> > +	return { Results::Pass, "Overflow capture of " +
> > +		std::to_string(captureLimit_) + " requests" };
> > +}
>
> Reading this I now see there is quiet a bit of duplication between
> SimpleCapture*::capture() implementations. This existed before this
> patch and can be addressed on-top.
>
> > +
> > +void SimpleCaptureOverflow::requestComplete([[maybe_unused]]Request *request)
> > +{
> > +	captureCount_++;
> > +	if (captureCount_ >= captureLimit_) {
> > +		loop_->exit(0);
> > +		return;
> > +	}
> > +}
>
> Like above I now see this is could be moved to SimpleCapture without
> much work and the code can then be shared between all SimpleCapture*
> tests. Maybe this one is so straight forward you could do it as a pre
> patch to this one? If you like it can also be done on-top.

I agree that there's duplicated code, but each class does things a little
differently, just enough that we can't have a single common function for all of
them, from what I can tell.

For example, in SimpleCaptureOverflow::requestComplete() I don't need to requeue
buffers as they complete since I queue them all at once, so even though part of
it is similar to other requestComplete()'s they aren't the same.

For SimpleCapture*::capture() I also don't see how they could all be shared,
since they have different variables to initialize and different success asserts
at the end. But at least the requests queueing loop could be shared, if we can
have a default queueRequest() that is overrided by SimpleCaptureBalanced.

Please see below my attempt at removing code duplication. Note that it doesn't
work because SimpleCapture::queueRequest() gets called instead of
SimpleCaptureBalanced::queueRequest() even on the SimpleCaptureBalanced
instance. Some OOP concept that I'm missing... Any pointers on how to fix it or
on a better way to reduce the duplication would be great :).

Thanks,
Nícolas

From 4166a9b1b0a3005ef5a077a4e2f10267d89bf375 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?N=C3=ADcolas=20F=2E=20R=2E=20A=2E=20Prado?=
 <nfraprado@collabora.com>
Date: Tue, 27 Apr 2021 18:10:03 -0300
Subject: [PATCH] tmp2

---
 src/lc-compliance/simple_capture.cpp | 126 ++++++++++-----------------
 src/lc-compliance/simple_capture.h   |  23 +++--
 2 files changed, 57 insertions(+), 92 deletions(-)

diff --git a/src/lc-compliance/simple_capture.cpp b/src/lc-compliance/simple_capture.cpp
index e4f6c6723f4d..34e116852d0b 100644
--- a/src/lc-compliance/simple_capture.cpp
+++ b/src/lc-compliance/simple_capture.cpp
@@ -73,6 +73,24 @@ void SimpleCapture::stop()
 	allocator_->free(stream);
 }
 
+int SimpleCapture::queueRequest(Request *request)
+{
+	return camera_->queueRequest(request);
+}
+
+void SimpleCapture::requestComplete(Request *request)
+{
+	captureCount_++;
+	if (captureCount_ >= captureLimit_) {
+		loop_->exit(0);
+		return;
+	}
+
+	request->reuse(Request::ReuseBuffers);
+	if (queueRequest(request))
+		loop_->exit(-EINVAL);
+}
+
 /* SimpleCaptureBalanced */
 
 SimpleCaptureBalanced::SimpleCaptureBalanced(std::shared_ptr<Camera> camera)
@@ -103,27 +121,10 @@ Results::Result SimpleCaptureBalanced::capture(unsigned int numRequests)
 	captureCount_ = 0;
 	captureLimit_ = numRequests;
 
-	/* Queue the recommended number of reqeuests. */
 	std::vector<std::unique_ptr<libcamera::Request>> requests;
-	for (const std::unique_ptr<FrameBuffer> &buffer : buffers) {
-		std::unique_ptr<Request> request = camera_->createRequest();
-		if (!request) {
-			stop();
-			return { Results::Fail, "Can't create request" };
-		}
-
-		if (request->addBuffer(stream, buffer.get())) {
-			stop();
-			return { Results::Fail, "Can't set buffer for request" };
-		}
-
-		if (queueRequest(request.get()) < 0) {
-			stop();
-			return { Results::Fail, "Failed to queue request" };
-		}
-
-		requests.push_back(std::move(request));
-	}
+	ret = queueRequests(stream, requests, buffers);
+	if (ret.first != Results::Pass)
+		return ret;
 
 	/* Run capture session. */
 	loop_ = new EventLoop();
@@ -148,19 +149,6 @@ int SimpleCaptureBalanced::queueRequest(Request *request)
 	return camera_->queueRequest(request);
 }
 
-void SimpleCaptureBalanced::requestComplete(Request *request)
-{
-	captureCount_++;
-	if (captureCount_ >= captureLimit_) {
-		loop_->exit(0);
-		return;
-	}
-
-	request->reuse(Request::ReuseBuffers);
-	if (queueRequest(request))
-		loop_->exit(-EINVAL);
-}
-
 /* SimpleCaptureUnbalanced */
 
 SimpleCaptureUnbalanced::SimpleCaptureUnbalanced(std::shared_ptr<Camera> camera)
@@ -180,27 +168,10 @@ Results::Result SimpleCaptureUnbalanced::capture(unsigned int numRequests)
 	captureCount_ = 0;
 	captureLimit_ = numRequests;
 
-	/* Queue the recommended number of reqeuests. */
 	std::vector<std::unique_ptr<libcamera::Request>> requests;
-	for (const std::unique_ptr<FrameBuffer> &buffer : buffers) {
-		std::unique_ptr<Request> request = camera_->createRequest();
-		if (!request) {
-			stop();
-			return { Results::Fail, "Can't create request" };
-		}
-
-		if (request->addBuffer(stream, buffer.get())) {
-			stop();
-			return { Results::Fail, "Can't set buffer for request" };
-		}
-
-		if (camera_->queueRequest(request.get()) < 0) {
-			stop();
-			return { Results::Fail, "Failed to queue request" };
-		}
-
-		requests.push_back(std::move(request));
-	}
+	ret = queueRequests(stream, requests, buffers);
+	if (ret.first != Results::Pass)
+		return ret;
 
 	/* Run capture session. */
 	loop_ = new EventLoop();
@@ -211,37 +182,14 @@ Results::Result SimpleCaptureUnbalanced::capture(unsigned int numRequests)
 	return { status ? Results::Fail : Results::Pass, "Unbalanced capture of " + std::to_string(numRequests) + " requests" };
 }
 
-void SimpleCaptureUnbalanced::requestComplete(Request *request)
-{
-	captureCount_++;
-	if (captureCount_ >= captureLimit_) {
-		loop_->exit(0);
-		return;
-	}
-
-	request->reuse(Request::ReuseBuffers);
-	if (camera_->queueRequest(request))
-		loop_->exit(-EINVAL);
-}
-
 SimpleCaptureOverflow::SimpleCaptureOverflow(std::shared_ptr<Camera> camera)
 	: SimpleCapture(camera)
 {
 }
 
-Results::Result SimpleCaptureOverflow::capture()
+Results::Result SimpleCapture::queueRequests(Stream *stream, std::vector<std::unique_ptr<libcamera::Request>> &requests,
+					     const std::vector<std::unique_ptr<FrameBuffer>> &buffers)
 {
-	Results::Result ret = start();
-	if (ret.first != Results::Pass)
-		return ret;
-
-	Stream *stream = config_->at(0).stream();
-	const std::vector<std::unique_ptr<FrameBuffer>> &buffers = allocator_->buffers(stream);
-
-	captureCount_ = 0;
-	captureLimit_ = buffers.size();
-
-	std::vector<std::unique_ptr<libcamera::Request>> requests;
 	for (const std::unique_ptr<FrameBuffer> &buffer : buffers) {
 		std::unique_ptr<Request> request = camera_->createRequest();
 		if (!request) {
@@ -254,7 +202,7 @@ Results::Result SimpleCaptureOverflow::capture()
 			return { Results::Fail, "Can't set buffer for request" };
 		}
 
-		if (camera_->queueRequest(request.get()) < 0) {
+		if (queueRequest(request.get()) < 0) {
 			stop();
 			return { Results::Fail, "Failed to queue request" };
 		}
@@ -262,6 +210,26 @@ Results::Result SimpleCaptureOverflow::capture()
 		requests.push_back(std::move(request));
 	}
 
+	return { Results::Pass, "Succesfully queued requests" };
+}
+
+Results::Result SimpleCaptureOverflow::capture()
+{
+	Results::Result ret = start();
+	if (ret.first != Results::Pass)
+		return ret;
+
+	Stream *stream = config_->at(0).stream();
+	const std::vector<std::unique_ptr<FrameBuffer>> &buffers = allocator_->buffers(stream);
+
+	captureCount_ = 0;
+	captureLimit_ = buffers.size();
+
+	std::vector<std::unique_ptr<libcamera::Request>> requests;
+	ret = queueRequests(stream, requests, buffers);
+	if (ret.first != Results::Pass)
+		return ret;
+
 	/* Run capture session. */
 	loop_ = new EventLoop();
 	loop_->exec();
diff --git a/src/lc-compliance/simple_capture.h b/src/lc-compliance/simple_capture.h
index cafcdafe10c2..754dc6ea0db8 100644
--- a/src/lc-compliance/simple_capture.h
+++ b/src/lc-compliance/simple_capture.h
@@ -26,11 +26,18 @@ protected:
 
 	Results::Result start();
 	void stop();
+	int queueRequest(libcamera::Request *request);
+	Results::Result queueRequests(libcamera::Stream *stream,
+				      std::vector<std::unique_ptr<libcamera::Request>> &requests,
+				      const std::vector<std::unique_ptr<libcamera::FrameBuffer>> &buffers);
 
-	virtual void requestComplete(libcamera::Request *request) = 0;
+	void requestComplete(libcamera::Request *request);
 
 	EventLoop *loop_;
 
+	unsigned int captureCount_;
+	unsigned int captureLimit_;
+
 	std::shared_ptr<libcamera::Camera> camera_;
 	std::unique_ptr<libcamera::FrameBufferAllocator> allocator_;
 	std::unique_ptr<libcamera::CameraConfiguration> config_;
@@ -45,11 +52,9 @@ public:
 
 private:
 	int queueRequest(libcamera::Request *request);
-	void requestComplete(libcamera::Request *request) override;
+	void requestComplete(libcamera::Request *request);
 
 	unsigned int queueCount_;
-	unsigned int captureCount_;
-	unsigned int captureLimit_;
 };
 
 class SimpleCaptureUnbalanced : public SimpleCapture
@@ -59,11 +64,6 @@ public:
 
 	Results::Result capture(unsigned int numRequests);
 
-private:
-	void requestComplete(libcamera::Request *request) override;
-
-	unsigned int captureCount_;
-	unsigned int captureLimit_;
 };
 
 class SimpleCaptureOverflow : public SimpleCapture
@@ -75,10 +75,7 @@ public:
 	Results::Result capture();
 
 private:
-	void requestComplete(libcamera::Request *request) override;
-
-	unsigned int captureCount_;
-	unsigned int captureLimit_;
+	void requestComplete(libcamera::Request *request);
 };
 
 #endif /* __LC_COMPLIANCE_SIMPLE_CAPTURE_H__ */

Hello Nícolas,

On 2021-04-28 10:42:48 -0300, Nícolas F. R. A. Prado wrote:
> Hi Niklas,
> 
> thank you for the feedback.
> 
> Em 2021-04-26 06:40, Niklas Söderlund escreveu:
> 
> > Hi "Nícolas,
> >
> > Thanks for your work.
> >
> > On 2021-04-21 13:51:38 -0300, Nícolas F. R. A. Prado wrote:
> > > A pipeline handler should be able to handle an arbitrary amount of
> > > simultaneous requests by submitting what it can to the video device and
> > > queuing the rest internally until resources are available. This isn't
> > > currently done by some pipeline handlers however [1].
> > > 
> > > Add a new test to lc-compliance that submits a lot of requests at once
> > > to check if the pipeline handler is behaving well in this situation.
> > > 
> > > [1] https://bugs.libcamera.org/show_bug.cgi?id=24
> > > 
> > > Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>
> > > ---
> > >  src/lc-compliance/simple_capture.cpp | 70 ++++++++++++++++++++++++++++
> > >  src/lc-compliance/simple_capture.h   | 15 ++++++
> > >  src/lc-compliance/single_stream.cpp  | 31 +++++++++++-
> > >  3 files changed, 115 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/src/lc-compliance/simple_capture.cpp b/src/lc-compliance/simple_capture.cpp
> > > index 875772a80c27..01d76380e998 100644
> > > --- a/src/lc-compliance/simple_capture.cpp
> > > +++ b/src/lc-compliance/simple_capture.cpp
> > > @@ -223,3 +223,73 @@ void SimpleCaptureUnbalanced::requestComplete(Request *request)
> > >  	if (camera_->queueRequest(request))
> > >  		loop_->exit(-EINVAL);
> > >  }
> > > +
> > > +SimpleCaptureOverflow::SimpleCaptureOverflow(std::shared_ptr<Camera> camera)
> > > +	: SimpleCapture(camera)
> > > +{
> > > +}
> > > +
> > > +Results::Result SimpleCaptureOverflow::capture()
> > > +{
> > > +	Results::Result ret = start();
> > > +	if (ret.first != Results::Pass)
> > > +		return ret;
> > > +
> > > +	Stream *stream = config_->at(0).stream();
> > > +	const std::vector<std::unique_ptr<FrameBuffer>> &buffers = allocator_->buffers(stream);
> > > +
> > > +	captureCount_ = 0;
> > > +	captureLimit_ = buffers.size();
> > > +
> > > +	std::vector<std::unique_ptr<libcamera::Request>> requests;
> > > +	for (const std::unique_ptr<FrameBuffer> &buffer : buffers) {
> > > +		std::unique_ptr<Request> request = camera_->createRequest();
> > > +		if (!request) {
> > > +			stop();
> > > +			return { Results::Fail, "Can't create request" };
> > > +		}
> > > +
> > > +		if (request->addBuffer(stream, buffer.get())) {
> > > +			stop();
> > > +			return { Results::Fail, "Can't set buffer for request" };
> > > +		}
> > > +
> > > +		if (camera_->queueRequest(request.get()) < 0) {
> > > +			stop();
> > > +			return { Results::Fail, "Failed to queue request" };
> > > +		}
> > > +
> > > +		requests.push_back(std::move(request));
> > > +	}
> > > +
> > > +	/* Run capture session. */
> > > +	loop_ = new EventLoop();
> > > +	loop_->exec();
> > > +	stop();
> > > +	delete loop_;
> > > +
> > > +	if (captureCount_ != captureLimit_)
> > > +		return { Results::Fail, "Got " + std::to_string(captureCount_) +
> > > +			" request, wanted " + std::to_string(captureLimit_) };
> > > +
> > > +	return { Results::Pass, "Overflow capture of " +
> > > +		std::to_string(captureLimit_) + " requests" };
> > > +}
> >
> > Reading this I now see there is quiet a bit of duplication between
> > SimpleCapture*::capture() implementations. This existed before this
> > patch and can be addressed on-top.
> >
> > > +
> > > +void SimpleCaptureOverflow::requestComplete([[maybe_unused]]Request *request)
> > > +{
> > > +	captureCount_++;
> > > +	if (captureCount_ >= captureLimit_) {
> > > +		loop_->exit(0);
> > > +		return;
> > > +	}
> > > +}
> >
> > Like above I now see this is could be moved to SimpleCapture without
> > much work and the code can then be shared between all SimpleCapture*
> > tests. Maybe this one is so straight forward you could do it as a pre
> > patch to this one? If you like it can also be done on-top.
> 
> I agree that there's duplicated code, but each class does things a little
> differently, just enough that we can't have a single common function for all of
> them, from what I can tell.

As always the devils in the details :-) I still think we can improve the 
code reuse here. But please only consider my suggestions as possible 
ways forward, they could very well turn out to be bad once tried.

> 
> For example, in SimpleCaptureOverflow::requestComplete() I don't need to requeue
> buffers as they complete since I queue them all at once, so even though part of
> it is similar to other requestComplete()'s they aren't the same.

Maybe a flag could be added to control if requests should be request or 
not?

> 
> For SimpleCapture*::capture() I also don't see how they could all be shared,
> since they have different variables to initialize and different success asserts
> at the end. But at least the requests queueing loop could be shared, if we can
> have a default queueRequest() that is overrided by SimpleCaptureBalanced.

Maybe the bulk of the code that can be reused can be broken out to a 
possible parameterized helper functions (or functions) in the base class 
that is then called from the specialized case?

> 
> Please see below my attempt at removing code duplication. Note that it doesn't
> work because SimpleCapture::queueRequest() gets called instead of
> SimpleCaptureBalanced::queueRequest() even on the SimpleCaptureBalanced
> instance. Some OOP concept that I'm missing... Any pointers on how to fix it or
> on a better way to reduce the duplication would be great :).
> 
> Thanks,
> Nícolas
> 
> From 4166a9b1b0a3005ef5a077a4e2f10267d89bf375 Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?N=C3=ADcolas=20F=2E=20R=2E=20A=2E=20Prado?=
>  <nfraprado@collabora.com>
> Date: Tue, 27 Apr 2021 18:10:03 -0300
> Subject: [PATCH] tmp2
> 
> ---
>  src/lc-compliance/simple_capture.cpp | 126 ++++++++++-----------------
>  src/lc-compliance/simple_capture.h   |  23 +++--
>  2 files changed, 57 insertions(+), 92 deletions(-)
> 
> diff --git a/src/lc-compliance/simple_capture.cpp b/src/lc-compliance/simple_capture.cpp
> index e4f6c6723f4d..34e116852d0b 100644
> --- a/src/lc-compliance/simple_capture.cpp
> +++ b/src/lc-compliance/simple_capture.cpp
> @@ -73,6 +73,24 @@ void SimpleCapture::stop()
>  	allocator_->free(stream);
>  }
>  
> +int SimpleCapture::queueRequest(Request *request)
> +{
> +	return camera_->queueRequest(request);
> +}
> +
> +void SimpleCapture::requestComplete(Request *request)
> +{
> +	captureCount_++;
> +	if (captureCount_ >= captureLimit_) {
> +		loop_->exit(0);
> +		return;
> +	}
> +
> +	request->reuse(Request::ReuseBuffers);
> +	if (queueRequest(request))
> +		loop_->exit(-EINVAL);
> +}
> +
>  /* SimpleCaptureBalanced */
>  
>  SimpleCaptureBalanced::SimpleCaptureBalanced(std::shared_ptr<Camera> camera)
> @@ -103,27 +121,10 @@ Results::Result SimpleCaptureBalanced::capture(unsigned int numRequests)
>  	captureCount_ = 0;
>  	captureLimit_ = numRequests;
>  
> -	/* Queue the recommended number of reqeuests. */
>  	std::vector<std::unique_ptr<libcamera::Request>> requests;
> -	for (const std::unique_ptr<FrameBuffer> &buffer : buffers) {
> -		std::unique_ptr<Request> request = camera_->createRequest();
> -		if (!request) {
> -			stop();
> -			return { Results::Fail, "Can't create request" };
> -		}
> -
> -		if (request->addBuffer(stream, buffer.get())) {
> -			stop();
> -			return { Results::Fail, "Can't set buffer for request" };
> -		}
> -
> -		if (queueRequest(request.get()) < 0) {
> -			stop();
> -			return { Results::Fail, "Failed to queue request" };
> -		}
> -
> -		requests.push_back(std::move(request));
> -	}
> +	ret = queueRequests(stream, requests, buffers);
> +	if (ret.first != Results::Pass)
> +		return ret;
>  
>  	/* Run capture session. */
>  	loop_ = new EventLoop();
> @@ -148,19 +149,6 @@ int SimpleCaptureBalanced::queueRequest(Request *request)
>  	return camera_->queueRequest(request);
>  }
>  
> -void SimpleCaptureBalanced::requestComplete(Request *request)
> -{
> -	captureCount_++;
> -	if (captureCount_ >= captureLimit_) {
> -		loop_->exit(0);
> -		return;
> -	}
> -
> -	request->reuse(Request::ReuseBuffers);
> -	if (queueRequest(request))
> -		loop_->exit(-EINVAL);
> -}
> -
>  /* SimpleCaptureUnbalanced */
>  
>  SimpleCaptureUnbalanced::SimpleCaptureUnbalanced(std::shared_ptr<Camera> camera)
> @@ -180,27 +168,10 @@ Results::Result SimpleCaptureUnbalanced::capture(unsigned int numRequests)
>  	captureCount_ = 0;
>  	captureLimit_ = numRequests;
>  
> -	/* Queue the recommended number of reqeuests. */
>  	std::vector<std::unique_ptr<libcamera::Request>> requests;
> -	for (const std::unique_ptr<FrameBuffer> &buffer : buffers) {
> -		std::unique_ptr<Request> request = camera_->createRequest();
> -		if (!request) {
> -			stop();
> -			return { Results::Fail, "Can't create request" };
> -		}
> -
> -		if (request->addBuffer(stream, buffer.get())) {
> -			stop();
> -			return { Results::Fail, "Can't set buffer for request" };
> -		}
> -
> -		if (camera_->queueRequest(request.get()) < 0) {
> -			stop();
> -			return { Results::Fail, "Failed to queue request" };
> -		}
> -
> -		requests.push_back(std::move(request));
> -	}
> +	ret = queueRequests(stream, requests, buffers);
> +	if (ret.first != Results::Pass)
> +		return ret;
>  
>  	/* Run capture session. */
>  	loop_ = new EventLoop();
> @@ -211,37 +182,14 @@ Results::Result SimpleCaptureUnbalanced::capture(unsigned int numRequests)
>  	return { status ? Results::Fail : Results::Pass, "Unbalanced capture of " + std::to_string(numRequests) + " requests" };
>  }
>  
> -void SimpleCaptureUnbalanced::requestComplete(Request *request)
> -{
> -	captureCount_++;
> -	if (captureCount_ >= captureLimit_) {
> -		loop_->exit(0);
> -		return;
> -	}
> -
> -	request->reuse(Request::ReuseBuffers);
> -	if (camera_->queueRequest(request))
> -		loop_->exit(-EINVAL);
> -}
> -
>  SimpleCaptureOverflow::SimpleCaptureOverflow(std::shared_ptr<Camera> camera)
>  	: SimpleCapture(camera)
>  {
>  }
>  
> -Results::Result SimpleCaptureOverflow::capture()
> +Results::Result SimpleCapture::queueRequests(Stream *stream, std::vector<std::unique_ptr<libcamera::Request>> &requests,
> +					     const std::vector<std::unique_ptr<FrameBuffer>> &buffers)
>  {
> -	Results::Result ret = start();
> -	if (ret.first != Results::Pass)
> -		return ret;
> -
> -	Stream *stream = config_->at(0).stream();
> -	const std::vector<std::unique_ptr<FrameBuffer>> &buffers = allocator_->buffers(stream);
> -
> -	captureCount_ = 0;
> -	captureLimit_ = buffers.size();
> -
> -	std::vector<std::unique_ptr<libcamera::Request>> requests;
>  	for (const std::unique_ptr<FrameBuffer> &buffer : buffers) {
>  		std::unique_ptr<Request> request = camera_->createRequest();
>  		if (!request) {
> @@ -254,7 +202,7 @@ Results::Result SimpleCaptureOverflow::capture()
>  			return { Results::Fail, "Can't set buffer for request" };
>  		}
>  
> -		if (camera_->queueRequest(request.get()) < 0) {
> +		if (queueRequest(request.get()) < 0) {
>  			stop();
>  			return { Results::Fail, "Failed to queue request" };
>  		}
> @@ -262,6 +210,26 @@ Results::Result SimpleCaptureOverflow::capture()
>  		requests.push_back(std::move(request));
>  	}
>  
> +	return { Results::Pass, "Succesfully queued requests" };
> +}
> +
> +Results::Result SimpleCaptureOverflow::capture()
> +{
> +	Results::Result ret = start();
> +	if (ret.first != Results::Pass)
> +		return ret;
> +
> +	Stream *stream = config_->at(0).stream();
> +	const std::vector<std::unique_ptr<FrameBuffer>> &buffers = allocator_->buffers(stream);
> +
> +	captureCount_ = 0;
> +	captureLimit_ = buffers.size();
> +
> +	std::vector<std::unique_ptr<libcamera::Request>> requests;
> +	ret = queueRequests(stream, requests, buffers);
> +	if (ret.first != Results::Pass)
> +		return ret;
> +
>  	/* Run capture session. */
>  	loop_ = new EventLoop();
>  	loop_->exec();
> diff --git a/src/lc-compliance/simple_capture.h b/src/lc-compliance/simple_capture.h
> index cafcdafe10c2..754dc6ea0db8 100644
> --- a/src/lc-compliance/simple_capture.h
> +++ b/src/lc-compliance/simple_capture.h
> @@ -26,11 +26,18 @@ protected:
>  
>  	Results::Result start();
>  	void stop();
> +	int queueRequest(libcamera::Request *request);
> +	Results::Result queueRequests(libcamera::Stream *stream,
> +				      std::vector<std::unique_ptr<libcamera::Request>> &requests,
> +				      const std::vector<std::unique_ptr<libcamera::FrameBuffer>> &buffers);
>  
> -	virtual void requestComplete(libcamera::Request *request) = 0;
> +	void requestComplete(libcamera::Request *request);
>  
>  	EventLoop *loop_;
>  
> +	unsigned int captureCount_;
> +	unsigned int captureLimit_;
> +
>  	std::shared_ptr<libcamera::Camera> camera_;
>  	std::unique_ptr<libcamera::FrameBufferAllocator> allocator_;
>  	std::unique_ptr<libcamera::CameraConfiguration> config_;
> @@ -45,11 +52,9 @@ public:
>  
>  private:
>  	int queueRequest(libcamera::Request *request);
> -	void requestComplete(libcamera::Request *request) override;
> +	void requestComplete(libcamera::Request *request);
>  
>  	unsigned int queueCount_;
> -	unsigned int captureCount_;
> -	unsigned int captureLimit_;
>  };
>  
>  class SimpleCaptureUnbalanced : public SimpleCapture
> @@ -59,11 +64,6 @@ public:
>  
>  	Results::Result capture(unsigned int numRequests);
>  
> -private:
> -	void requestComplete(libcamera::Request *request) override;
> -
> -	unsigned int captureCount_;
> -	unsigned int captureLimit_;
>  };
>  
>  class SimpleCaptureOverflow : public SimpleCapture
> @@ -75,10 +75,7 @@ public:
>  	Results::Result capture();
>  
>  private:
> -	void requestComplete(libcamera::Request *request) override;
> -
> -	unsigned int captureCount_;
> -	unsigned int captureLimit_;
> +	void requestComplete(libcamera::Request *request);
>  };
>  
>  #endif /* __LC_COMPLIANCE_SIMPLE_CAPTURE_H__ */
> -- 
> 2.31.1
>