[RFC PATCH v2 13/16] apps: lc-compliance: Merge `CaptureBalanced` and `CaptureUnbalanced`
Jacopo Mondi
jacopo.mondi at ideasonboard.com
Fri Jan 24 12:21:52 CET 2025
Hi Barnabás
On Fri, Jan 24, 2025 at 10:06:23AM +0000, Barnabás Pőcze wrote:
> 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?
If cancelling the pending callbacks should happen at EvenLoop::exit()
time, can't we simply clear the calls_ queue there ?
>
>
> >
> > >
> > > -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