Message ID | 20250130115001.1129305-17-pobrn@protonmail.com |
---|---|
State | New |
Headers | show |
Series |
|
Related | show |
Hi Barnabás On Thu, Jan 30, 2025 at 11:51:22AM +0000, Barnabás Pőcze wrote: > The above two classes have very similar implementations, 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> Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> Thanks j > --- > src/apps/lc-compliance/helpers/capture.cpp | 142 ++++++------------ > src/apps/lc-compliance/helpers/capture.h | 50 ++---- > src/apps/lc-compliance/tests/capture_test.cpp | 12 +- > 3 files changed, 71 insertions(+), 133 deletions(-) > > diff --git a/src/apps/lc-compliance/helpers/capture.cpp b/src/apps/lc-compliance/helpers/capture.cpp > index 43db15d2d..940646f6c 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_) This now fits on the previous line > { > } > @@ -40,153 +42,109 @@ 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; > - > - camera_->stop(); > + start(); > + prepareRequests(); > > - camera_->requestCompleted.disconnect(this); > + for (const auto &request : requests_) > + queueRequest(request.get()); > > - Stream *stream = config_->at(0).stream(); > - requests_.clear(); > - allocator_.free(stream); > -} > + EXPECT_EQ(loop_->exec(), 0); > > -/* CaptureBalanced */ > + stop(); > > -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() > { > - 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); > + int ret = camera_->queueRequest(request); > + if (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..ede395e2a 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(); > + 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.1 > >
diff --git a/src/apps/lc-compliance/helpers/capture.cpp b/src/apps/lc-compliance/helpers/capture.cpp index 43db15d2d..940646f6c 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,109 @@ 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; - - camera_->stop(); + start(); + prepareRequests(); - camera_->requestCompleted.disconnect(this); + for (const auto &request : requests_) + queueRequest(request.get()); - Stream *stream = config_->at(0).stream(); - requests_.clear(); - allocator_.free(stream); -} + EXPECT_EQ(loop_->exec(), 0); -/* CaptureBalanced */ + stop(); -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() { - 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); + int ret = camera_->queueRequest(request); + if (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..ede395e2a 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(); + 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 have very similar implementations, 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 | 142 ++++++------------ src/apps/lc-compliance/helpers/capture.h | 50 ++---- src/apps/lc-compliance/tests/capture_test.cpp | 12 +- 3 files changed, 71 insertions(+), 133 deletions(-)