Hi Barnabás
On Tue, Jan 14, 2025 at 06:22:52PM +0000, Barnabás Pőcze wrote:
> The above two classes implement very similar, in fact, the only essential
s/implement/implementations are/ ?
> difference is how many requests are queued. `CaptureBalanced` queues
> a predetermined number of requests, while `CaptureUnbalanced` queues
> requests without limit.
>
> This can be addressed by introducing a "capture" and a "queue" limit
> into the `Capture` class, which determine at most how many requests
> can be queued, and how many request completions are expected before
> stopping.
>
> Signed-off-by: Barnabás Pőcze <pobrn at protonmail.com>
> ---
> src/apps/lc-compliance/helpers/capture.cpp | 141 +++++++-----------
> src/apps/lc-compliance/helpers/capture.h | 50 ++-----
> src/apps/lc-compliance/tests/capture_test.cpp | 12 +-
> 3 files changed, 71 insertions(+), 132 deletions(-)
>
> diff --git a/src/apps/lc-compliance/helpers/capture.cpp b/src/apps/lc-compliance/helpers/capture.cpp
> index 43db15d2d..77e87c9e4 100644
> --- a/src/apps/lc-compliance/helpers/capture.cpp
> +++ b/src/apps/lc-compliance/helpers/capture.cpp
> @@ -7,12 +7,14 @@
>
> #include "capture.h"
>
> +#include <assert.h>
> +
> #include <gtest/gtest.h>
>
> using namespace libcamera;
>
> Capture::Capture(std::shared_ptr<Camera> camera)
> - : loop_(nullptr), camera_(std::move(camera)),
> + : camera_(std::move(camera)),
> allocator_(camera_)
> {
> }
> @@ -40,153 +42,110 @@ void Capture::configure(StreamRole role)
> }
> }
>
> -void Capture::start()
> +void Capture::run(unsigned int captureLimit, std::optional<unsigned int> queueLimit)
> {
> - Stream *stream = config_->at(0).stream();
> - int count = allocator_.allocate(stream);
> + assert(!queueLimit || captureLimit <= *queueLimit);
>
> - ASSERT_GE(count, 0) << "Failed to allocate buffers";
> - EXPECT_EQ(count, config_->at(0).bufferCount) << "Allocated less buffers than expected";
> + captureLimit_ = captureLimit;
> + queueLimit_ = queueLimit;
>
> - camera_->requestCompleted.connect(this, &Capture::requestComplete);
> + captureCount_ = queueCount_ = 0;
>
> - ASSERT_EQ(camera_->start(), 0) << "Failed to start camera";
> -}
> + EventLoop loop;
> + loop_ = &loop;
Can loop_ be made a class member instead of being created here ?
Each test run will create an instance of the Capture class if I'm not
mistaken
>
> -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 at ideasonboard.com>
> }
>
> INSTANTIATE_TEST_SUITE_P(CaptureTests,
> --
> 2.48.0
>
>