Message ID | 20250114182143.1773762-14-pobrn@protonmail.com |
---|---|
State | Superseded |
Headers | show |
Series |
|
Related | show |
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@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 > > -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 ? > { > - 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. > + > + 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@ideasonboard.com> > } > > INSTANTIATE_TEST_SUITE_P(CaptureTests, > -- > 2.48.0 > >
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 > 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@protonmail.com> With Jacopo's comments addressed, Reviewed-by: Paul Elder <paul.elder@ideasonboard.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; > > -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) > { > - 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; > + > + 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); > } > > INSTANTIATE_TEST_SUITE_P(CaptureTests, > -- > 2.48.0 > >
Hi 2025. január 22., szerda 13:20 keltezéssel, Jacopo Mondi <jacopo.mondi@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@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@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 > > > > >
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@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@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@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 > > > > > > > >
2025. január 24., péntek 12:21 keltezéssel, Jacopo Mondi <jacopo.mondi@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@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@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. 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@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 > > > > > > > > > > > >
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@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@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@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 ? 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@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. > > > > > > Regards, > > > Barnabás Pőcze > > > > > > > > > > > > > > > } > > > > > > > > > > INSTANTIATE_TEST_SUITE_P(CaptureTests, > > > > > -- > > > > > 2.48.0 > > > > > > > > > > > > > > > >
2025. január 24., péntek 12:59 keltezéssel, Jacopo Mondi <jacopo.mondi@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@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@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@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. 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. > > 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@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 > > > > > > > > > > > > > > > > > > > > >
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@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@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@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@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 ? > > > > > 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@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 > > > > > > > > > > > > > > > > > > > > > > > > > >
2025. január 24., péntek 14:16 keltezéssel, Jacopo Mondi <jacopo.mondi@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@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@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@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@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@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 > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
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; -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) { - 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; + + 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); } INSTANTIATE_TEST_SUITE_P(CaptureTests,
The above two classes implement very similar, in fact, the only essential 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@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(-)