[RFC PATCH v2 13/16] apps: lc-compliance: Merge `CaptureBalanced` and `CaptureUnbalanced`
Barnabás Pőcze
pobrn at protonmail.com
Fri Jan 24 11:06:23 CET 2025
Hi
2025. január 22., szerda 13:20 keltezéssel, Jacopo Mondi <jacopo.mondi at ideasonboard.com> írta:
> Hi Barnabás
>
> On Tue, Jan 14, 2025 at 06:22:52PM +0000, Barnabás Pőcze wrote:
> > The above two classes implement very similar, in fact, the only essential
>
> s/implement/implementations are/ ?
>
> > difference is how many requests are queued. `CaptureBalanced` queues
> > a predetermined number of requests, while `CaptureUnbalanced` queues
> > requests without limit.
> >
> > This can be addressed by introducing a "capture" and a "queue" limit
> > into the `Capture` class, which determine at most how many requests
> > can be queued, and how many request completions are expected before
> > stopping.
> >
> > Signed-off-by: Barnabás Pőcze <pobrn at protonmail.com>
> > ---
> > src/apps/lc-compliance/helpers/capture.cpp | 141 +++++++-----------
> > src/apps/lc-compliance/helpers/capture.h | 50 ++-----
> > src/apps/lc-compliance/tests/capture_test.cpp | 12 +-
> > 3 files changed, 71 insertions(+), 132 deletions(-)
> >
> > diff --git a/src/apps/lc-compliance/helpers/capture.cpp b/src/apps/lc-compliance/helpers/capture.cpp
> > index 43db15d2d..77e87c9e4 100644
> > --- a/src/apps/lc-compliance/helpers/capture.cpp
> > +++ b/src/apps/lc-compliance/helpers/capture.cpp
> > @@ -7,12 +7,14 @@
> >
> > #include "capture.h"
> >
> > +#include <assert.h>
> > +
> > #include <gtest/gtest.h>
> >
> > using namespace libcamera;
> >
> > Capture::Capture(std::shared_ptr<Camera> camera)
> > - : loop_(nullptr), camera_(std::move(camera)),
> > + : camera_(std::move(camera)),
> > allocator_(camera_)
> > {
> > }
> > @@ -40,153 +42,110 @@ void Capture::configure(StreamRole role)
> > }
> > }
> >
> > -void Capture::start()
> > +void Capture::run(unsigned int captureLimit, std::optional<unsigned int> queueLimit)
> > {
> > - Stream *stream = config_->at(0).stream();
> > - int count = allocator_.allocate(stream);
> > + assert(!queueLimit || captureLimit <= *queueLimit);
> >
> > - ASSERT_GE(count, 0) << "Failed to allocate buffers";
> > - EXPECT_EQ(count, config_->at(0).bufferCount) << "Allocated less buffers than expected";
> > + captureLimit_ = captureLimit;
> > + queueLimit_ = queueLimit;
> >
> > - camera_->requestCompleted.connect(this, &Capture::requestComplete);
> > + captureCount_ = queueCount_ = 0;
> >
> > - ASSERT_EQ(camera_->start(), 0) << "Failed to start camera";
> > -}
> > + EventLoop loop;
> > + loop_ = &loop;
>
> Can loop_ be made a class member instead of being created here ?
> Each test run will create an instance of the Capture class if I'm not
> mistaken
I agree it would be better, I would actually prefer using a single event loop
during all tests if possible. But in order to do that, there would need to be a
way to cancel the callbacks submitted with `EventLoop::callLater()`.
One potential way to implement it would be to add a second argument to `callLater()`,
named something like "cookie", and then add a new function like `cancelLater(cookie)`.
Any ideas, comments?
>
> >
> > -void Capture::stop()
> > -{
> > - if (!config_ || !allocator_.allocated())
> > - return;
> > + start();
> > + prepareRequests(queueLimit_);
> >
> > - camera_->stop();
> > + for (const auto &request : requests_)
> > + queueRequest(request.get());
> >
> > - camera_->requestCompleted.disconnect(this);
> > + EXPECT_EQ(loop_->exec(), 0);
> >
> > - Stream *stream = config_->at(0).stream();
> > - requests_.clear();
> > - allocator_.free(stream);
> > -}
> > + stop();
> >
> > -/* CaptureBalanced */
> > + loop_ = nullptr;
> >
> > -CaptureBalanced::CaptureBalanced(std::shared_ptr<Camera> camera)
> > - : Capture(std::move(camera))
> > -{
> > + EXPECT_LE(captureLimit_, captureCount_);
> > + EXPECT_LE(captureCount_, queueCount_);
> > + EXPECT_TRUE(!queueLimit_ || queueCount_ <= *queueLimit_);
> > }
> >
> > -void CaptureBalanced::capture(unsigned int numRequests)
> > +void Capture::prepareRequests(std::optional<unsigned int> queueLimit)
>
> queueLimit_ is a class member, initialized by the caller of this
> function. Do you need to pass it in here ?
Probably not.
>
> > {
> > - 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) {
> > + if (queueLimit && *queueLimit < buffers.size()) {
> > GTEST_SKIP() << "Camera needs " << buffers.size()
> > - << " requests, can't test only " << numRequests;
> > + << " requests, can't test only " << *queueLimit;
> > }
> >
> > - 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));
> > }
> > -
> > - /* Run capture session. */
> > - loop_ = new EventLoop();
> > - loop_->exec();
> > - stop();
> > - delete loop_;
> > -
> > - ASSERT_EQ(captureCount_, captureLimit_);
> > }
> >
> > -int CaptureBalanced::queueRequest(Request *request)
> > +int Capture::queueRequest(libcamera::Request *request)
> > {
> > - queueCount_++;
> > - if (queueCount_ > captureLimit_)
> > + if (queueLimit_ && queueCount_ >= *queueLimit_)
> > return 0;
> >
> > - return camera_->queueRequest(request);
> > + if (int ret = camera_->queueRequest(request); ret < 0)
> > + return ret;
>
> This is valid, but a bit unusual for the code base.
>
> int ret = camera_->queueRequest(request);
> if (ret)
> return ret;
>
> is more commmon.
ACK
>
> > +
> > + queueCount_ += 1;
> > + return 0;
> > }
> >
> > -void CaptureBalanced::requestComplete(Request *request)
> > +void Capture::requestComplete(Request *request)
> > {
> > - EXPECT_EQ(request->status(), Request::Status::RequestComplete)
> > - << "Request didn't complete successfully";
> > -
> > captureCount_++;
> > if (captureCount_ >= captureLimit_) {
> > loop_->exit(0);
> > return;
> > }
> >
> > + EXPECT_EQ(request->status(), Request::Status::RequestComplete)
> > + << "Request didn't complete successfully";
> > +
> > request->reuse(Request::ReuseBuffers);
> > if (queueRequest(request))
> > loop_->exit(-EINVAL);
> > }
> >
> > -/* CaptureUnbalanced */
> > -
> > -CaptureUnbalanced::CaptureUnbalanced(std::shared_ptr<Camera> camera)
> > - : Capture(std::move(camera))
> > -{
> > -}
> > -
> > -void CaptureUnbalanced::capture(unsigned int numRequests)
> > +void Capture::start()
> > {
> > - 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";
> > -
> > - 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";
> > + int count = allocator_.allocate(stream);
> >
> > - requests_.push_back(std::move(request));
> > - }
> > + ASSERT_GE(count, 0) << "Failed to allocate buffers";
> > + EXPECT_EQ(count, config_->at(0).bufferCount) << "Allocated less buffers than expected";
> >
> > - /* Run capture session. */
> > - loop_ = new EventLoop();
> > - int status = loop_->exec();
> > - stop();
> > - delete loop_;
> > + camera_->requestCompleted.connect(this, &Capture::requestComplete);
> >
> > - ASSERT_EQ(status, 0);
> > + ASSERT_EQ(camera_->start(), 0) << "Failed to start camera";
> > }
> >
> > -void CaptureUnbalanced::requestComplete(Request *request)
> > +void Capture::stop()
> > {
> > - captureCount_++;
> > - if (captureCount_ >= captureLimit_) {
> > - loop_->exit(0);
> > + if (!config_ || !allocator_.allocated())
> > return;
> > - }
> >
> > - EXPECT_EQ(request->status(), Request::Status::RequestComplete)
> > - << "Request didn't complete successfully";
> > + camera_->stop();
> >
> > - request->reuse(Request::ReuseBuffers);
> > - if (camera_->queueRequest(request))
> > - loop_->exit(-EINVAL);
> > + camera_->requestCompleted.disconnect(this);
> > +
> > + Stream *stream = config_->at(0).stream();
> > + requests_.clear();
> > + allocator_.free(stream);
> > }
> > diff --git a/src/apps/lc-compliance/helpers/capture.h b/src/apps/lc-compliance/helpers/capture.h
> > index a4cc3a99e..173421fd2 100644
> > --- a/src/apps/lc-compliance/helpers/capture.h
> > +++ b/src/apps/lc-compliance/helpers/capture.h
> > @@ -8,6 +8,7 @@
> > #pragma once
> >
> > #include <memory>
> > +#include <optional>
> >
> > #include <libcamera/libcamera.h>
> >
> > @@ -16,51 +17,30 @@
> > class Capture
> > {
> > public:
> > + Capture(std::shared_ptr<libcamera::Camera> camera);
> > + ~Capture();
> > +
> > void configure(libcamera::StreamRole role);
> > + void run(unsigned int captureLimit, std::optional<unsigned int> queueLimit = {});
> >
> > -protected:
> > - Capture(std::shared_ptr<libcamera::Camera> camera);
> > - virtual ~Capture();
> > +private:
> > + LIBCAMERA_DISABLE_COPY_AND_MOVE(Capture)
> >
> > void start();
> > void stop();
> >
> > - virtual void requestComplete(libcamera::Request *request) = 0;
> > -
> > - EventLoop *loop_;
> > + void prepareRequests(std::optional<unsigned int> queueLimit = {});
> > + int queueRequest(libcamera::Request *request);
> > + void requestComplete(libcamera::Request *request);
> >
> > std::shared_ptr<libcamera::Camera> camera_;
> > libcamera::FrameBufferAllocator allocator_;
> > std::unique_ptr<libcamera::CameraConfiguration> config_;
> > std::vector<std::unique_ptr<libcamera::Request>> requests_;
> > -};
> > -
> > -class CaptureBalanced : public Capture
> > -{
> > -public:
> > - CaptureBalanced(std::shared_ptr<libcamera::Camera> camera);
> > -
> > - void capture(unsigned int numRequests);
> > -
> > -private:
> > - int queueRequest(libcamera::Request *request);
> > - void requestComplete(libcamera::Request *request) override;
> > -
> > - unsigned int queueCount_;
> > - unsigned int captureCount_;
> > - unsigned int captureLimit_;
> > -};
> > -
> > -class CaptureUnbalanced : public Capture
> > -{
> > -public:
> > - CaptureUnbalanced(std::shared_ptr<libcamera::Camera> camera);
> > -
> > - void capture(unsigned int numRequests);
> > -
> > -private:
> > - void requestComplete(libcamera::Request *request) override;
> >
> > - unsigned int captureCount_;
> > - unsigned int captureLimit_;
> > + EventLoop *loop_ = nullptr;
> > + unsigned int captureLimit_ = 0;
> > + std::optional<unsigned int> queueLimit_;
> > + unsigned int captureCount_ = 0;
> > + unsigned int queueCount_ = 0;
> > };
> > diff --git a/src/apps/lc-compliance/tests/capture_test.cpp b/src/apps/lc-compliance/tests/capture_test.cpp
> > index 97465a612..93bed48f0 100644
> > --- a/src/apps/lc-compliance/tests/capture_test.cpp
> > +++ b/src/apps/lc-compliance/tests/capture_test.cpp
> > @@ -87,11 +87,11 @@ TEST_P(SingleStream, Capture)
> > {
> > auto [role, numRequests] = GetParam();
> >
> > - CaptureBalanced capture(camera_);
> > + Capture capture(camera_);
> >
> > capture.configure(role);
> >
> > - capture.capture(numRequests);
> > + capture.run(numRequests, numRequests);
> > }
> >
> > /*
> > @@ -106,12 +106,12 @@ TEST_P(SingleStream, CaptureStartStop)
> > auto [role, numRequests] = GetParam();
> > unsigned int numRepeats = 3;
> >
> > - CaptureBalanced capture(camera_);
> > + Capture capture(camera_);
> >
> > capture.configure(role);
> >
> > for (unsigned int starts = 0; starts < numRepeats; starts++)
> > - capture.capture(numRequests);
> > + capture.run(numRequests, numRequests);
> > }
> >
> > /*
> > @@ -125,11 +125,11 @@ TEST_P(SingleStream, UnbalancedStop)
> > {
> > auto [role, numRequests] = GetParam();
> >
> > - CaptureUnbalanced capture(camera_);
> > + Capture capture(camera_);
> >
> > capture.configure(role);
> >
> > - capture.capture(numRequests);
> > + capture.run(numRequests);
>
> Can we remove "Unbalanced" from the test comment ? I always found it a
> bit weird and now that we have removed the class with the
> corresponding name I think the comment can be
>
> /*
> * Test stop with queued requests
> *
> * Makes sure the camera supports a stop with requests queued. Example failure
> * is a camera that does not handle cancelation of buffers coming back from the
> * video device while stopping.
> */
>
> The rest looks good, thanks
> Reviewed-by: Jacopo Mondi <jacopo.mondi at ideasonboard.com>
And what about the name (`CaptureUnbalanced`)? Should that change as well?
Regards,
Barnabás Pőcze
>
> > }
> >
> > INSTANTIATE_TEST_SUITE_P(CaptureTests,
> > --
> > 2.48.0
> >
> >
>
More information about the libcamera-devel
mailing list