[libcamera-devel] [PATCH v3 3/4] lc-compliance: Add test to queue more requests than hardware depth
Niklas Söderlund
niklas.soderlund at ragnatech.se
Thu Apr 29 13:23:43 CEST 2021
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 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.
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 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
>
--
Regards,
Niklas Söderlund
More information about the libcamera-devel
mailing list