[libcamera-devel] [PATCH v3 3/4] lc-compliance: Add test to queue more requests than hardware depth
Nícolas F. R. A. Prado
nfraprado at collabora.com
Wed Apr 28 15:42:48 CEST 2021
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 at 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 at 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
More information about the libcamera-devel
mailing list