[RFC PATCH v2 13/16] apps: lc-compliance: Merge `CaptureBalanced` and `CaptureUnbalanced`
Barnabás Pőcze
pobrn at protonmail.com
Fri Jan 24 14:34:39 CET 2025
2025. január 24., péntek 14:16 keltezéssel, Jacopo Mondi <jacopo.mondi at ideasonboard.com> írta:
> Hi Barnabás
>
> On Fri, Jan 24, 2025 at 12:25:57PM +0000, Barnabás Pőcze wrote:
> > 2025. január 24., péntek 12:59 keltezéssel, Jacopo Mondi <jacopo.mondi at ideasonboard.com> írta:
> >
> > > Hi Barnabás
> > >
> > > On Fri, Jan 24, 2025 at 11:31:22AM +0000, Barnabás Pőcze wrote:
> > > > 2025. január 24., péntek 12:21 keltezéssel, Jacopo Mondi <jacopo.mondi at ideasonboard.com> írta:
> > > >
> > > > > 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 ?
> > > >
> > > > I suppose it depends on how generic or how lc-compliance specific solution is desired.
> > > >
> > >
> > > What makes you think the solution would be lc-compliance specific ?
> > > Every event loop will evenually be terminated by a call to exit(), am
> > > I wrong ? And we should also clean up the queue in the destructor.
> > > What am I missing ?
> >
> > The queue is already cleared when an `EventLoop` is destroyed.
> >
> > I can imagine scenarios where temporarily stopping and then restarting the event
> > loop - in such a way that it is mostly transparent to the code running in it -
> > might be desirable (e.g. migrating between threads).
> >
> > There is nothing that would prevent the restarting of the event loop today,
> > and thus clearing the call queue may not always be desirable. But I suppose
> > we could wait until there is an actual use case before addressing this.
>
> True that. I presume as long as we document that stopping an event
> loop clears all the pending callback, I think it's fine for now.
>
> >
> > Another thing, I think it would be a good thing if every component could use
> > a single event loop implementation. In my opinion it is not ideal that libcamera
> > proper has an event loop that is completely different from that of cam, lc-compliance.
> >
>
> mmm, maybe I miss what you mean by "component", but cam and
> lc-compliance are effectively applications using the libcamera API and
> we don't want applications to run in the same even loop as libcamera ?
Oops, maybe I wasn't too clear. My point is that libcamera proper has an event
loop implementation (`EventDispatcher[Poll]`), and the applications also have a
separate implementation (`EventLoop`). I think having two completely separate
implementations is likely not necessary.
>
> >
> > >
> > > For sure, I missed your question at the end of the previous email. See
> > > below.
> > >
> > > >
> > > > Regards,
> > > > Barnabás Pőcze
> > > >
> > > >
> > > > >
> > > > > >
> > > > > >
> > > > > > >
> > > > > > > >
> > > > > > > > -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?
> > > > > >
> > >
> > > Up to you. These tests have always been 'unbounded' rather than
> > > 'unbalanced' to me.
> >
> > ACK
> >
> >
> > Regards,
> > Barnabás Pőcze
> >
> >
> > >
> > > > > >
> > > > > > Regards,
> > > > > > Barnabás Pőcze
> > > > > >
> > > > > >
> > > > > > >
> > > > > > > > }
> > > > > > > >
> > > > > > > > INSTANTIATE_TEST_SUITE_P(CaptureTests,
> > > > > > > > --
> > > > > > > > 2.48.0
> > > > > > > >
> > > > > > > >
> > > > > > >
> > > > >
> > >
>
More information about the libcamera-devel
mailing list