[RFC PATCH v1 08/12] apps: lc-compliance: Remove libevent dependency
Jacopo Mondi
jacopo.mondi at ideasonboard.com
Tue Jan 7 18:06:04 CET 2025
Hi Barnabás
On Fri, Dec 20, 2024 at 03:08:37PM +0000, Barnabás Pőcze wrote:
> libevent is only used as a way to notify the main thread from
> the CameraManager thread when the capture should be terminated.
>
> Not only is libevent not really necessary and instead can be
> replaced with a simple combination of C++ STL parts, the current
> usage is prone to race conditions.
>
> Specifically, the camera's `queueRequest()` might complete the
> request before the main thread could set the `loop_` member
> variable and synchronize with the thread. This is a data race,
> practically resulting in a nullptr or dangling pointer dereference.
>
> This can easily be triggered by inserting a sufficiently large
> timeout before `loop_ = new EventLoop;`:
To be frank this is kind of hard to trigger, as it can only happens
if the main thread does not execute for longer than the frame
interval. But yes, who knows, if there is a chance for this to happen,
it will happen sooner or later, so it's indeed worth addressing it.
>
> [46:43:40.529620351] [671833] DEBUG Request request.cpp:129 Request(4:C:0/1:0)
> ../src/apps/lc-compliance/helpers/capture.cpp:140:14: runtime error: member call on null pointer of type 'struct EventLoop'
> 0 0x632c3e82f81a in CaptureBalanced::requestComplete(libcamera::Request*) ../src/apps/lc-compliance/helpers/capture.cpp:140
>
> Signed-off-by: Barnabás Pőcze <pobrn at protonmail.com>
> ---
> README.rst | 2 +-
> src/apps/lc-compliance/helpers/capture.cpp | 23 ++++++------
> src/apps/lc-compliance/helpers/capture.h | 42 +++++++++++++++++++---
> src/apps/lc-compliance/meson.build | 7 ++--
> src/apps/meson.build | 5 ---
> 5 files changed, 53 insertions(+), 26 deletions(-)
>
> diff --git a/README.rst b/README.rst
> index 4068c6cc8..f1749be97 100644
> --- a/README.rst
> +++ b/README.rst
> @@ -97,7 +97,7 @@ for Python bindings: [optional]
> pybind11-dev
>
> for lc-compliance: [optional]
> - libevent-dev libgtest-dev
> + libgtest-dev
>
> for abi-compat.sh: [optional]
> abi-compliance-checker
> diff --git a/src/apps/lc-compliance/helpers/capture.cpp b/src/apps/lc-compliance/helpers/capture.cpp
> index 91c4d4400..b473e0773 100644
> --- a/src/apps/lc-compliance/helpers/capture.cpp
> +++ b/src/apps/lc-compliance/helpers/capture.cpp
> @@ -12,7 +12,7 @@
> 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
> {
> }
> @@ -52,6 +52,8 @@ void Capture::start()
>
> camera_->requestCompleted.connect(this, &Capture::requestComplete);
>
> + result_.reset();
> +
> ASSERT_EQ(camera_->start(), 0) << "Failed to start camera";
> }
>
> @@ -62,6 +64,8 @@ void Capture::stop()
>
> camera_->stop();
>
> + result_.reset();
> +
I don't think you could have a stop-stop or start-start sequence, so
probably a reset in start() is all you need. However this doesn't hurt
> camera_->requestCompleted.disconnect(this);
>
> Stream *stream = config_->at(0).stream();
> @@ -108,11 +112,10 @@ void CaptureBalanced::capture(unsigned int numRequests)
> }
>
> /* Run capture session. */
> - loop_ = new EventLoop();
> - loop_->exec();
> + int status = result_.wait();
> stop();
> - delete loop_;
>
> + ASSERT_EQ(status, 0);
> ASSERT_EQ(captureCount_, captureLimit_);
> }
>
> @@ -132,13 +135,13 @@ void CaptureBalanced::requestComplete(Request *request)
>
> captureCount_++;
> if (captureCount_ >= captureLimit_) {
> - loop_->exit(0);
> + result_.set(0);
> return;
> }
>
> request->reuse(Request::ReuseBuffers);
> if (queueRequest(request))
> - loop_->exit(-EINVAL);
> + result_.set(-EINVAL);
> }
>
> /* CaptureUnbalanced */
> @@ -171,10 +174,8 @@ void CaptureUnbalanced::capture(unsigned int numRequests)
> }
>
> /* Run capture session. */
> - loop_ = new EventLoop();
> - int status = loop_->exec();
> + int status = result_.wait();
> stop();
> - delete loop_;
>
> ASSERT_EQ(status, 0);
> }
> @@ -183,7 +184,7 @@ void CaptureUnbalanced::requestComplete(Request *request)
> {
> captureCount_++;
> if (captureCount_ >= captureLimit_) {
> - loop_->exit(0);
> + result_.set(0);
> return;
> }
>
> @@ -192,5 +193,5 @@ void CaptureUnbalanced::requestComplete(Request *request)
>
> request->reuse(Request::ReuseBuffers);
> if (camera_->queueRequest(request))
> - loop_->exit(-EINVAL);
> + result_.set(-EINVAL);
> }
> diff --git a/src/apps/lc-compliance/helpers/capture.h b/src/apps/lc-compliance/helpers/capture.h
> index a4cc3a99e..8582ade8e 100644
> --- a/src/apps/lc-compliance/helpers/capture.h
> +++ b/src/apps/lc-compliance/helpers/capture.h
> @@ -7,12 +7,13 @@
>
> #pragma once
>
> +#include <condition_variable>
> #include <memory>
> +#include <mutex>
> +#include <optional>
>
> #include <libcamera/libcamera.h>
>
> -#include "../common/event_loop.h"
> -
> class Capture
> {
> public:
> @@ -27,12 +28,45 @@ protected:
>
> virtual void requestComplete(libcamera::Request *request) = 0;
>
> - EventLoop *loop_;
> -
> std::shared_ptr<libcamera::Camera> camera_;
> libcamera::FrameBufferAllocator allocator_;
> std::unique_ptr<libcamera::CameraConfiguration> config_;
> std::vector<std::unique_ptr<libcamera::Request>> requests_;
> +
> + struct
> + {
nit: We usually do
struct {
> + private:
With private: after public:
> + std::mutex mutex_;
> + std::condition_variable cv_;
> + std::optional<int> value_;
> +
> + public:
> + int wait()
> + {
> + std::unique_lock guard(mutex_);
> +
> + cv_.wait(guard, [&] {
Do you need to capture the whole env or &value is enough ?
> + return value_.has_value();
> + });
> +
> + return *value_;
> + }
> +
> + void set(int value)
> + {
> + std::unique_lock guard(mutex_);
> +
> + if (!value_)
Why do you think this can't be overriden ?
> + value_ = value;
> +
> + cv_.notify_all();
> + }
> +
> + void reset()
> + {
> + value_.reset();
> + }
> + } result_;
> };
>
> class CaptureBalanced : public Capture
> diff --git a/src/apps/lc-compliance/meson.build b/src/apps/lc-compliance/meson.build
> index b1f605f33..0884bc6ca 100644
> --- a/src/apps/lc-compliance/meson.build
> +++ b/src/apps/lc-compliance/meson.build
> @@ -4,13 +4,11 @@ libgtest = dependency('gtest', version : '>=1.10.0',
> required : get_option('lc-compliance'),
> fallback : ['gtest', 'gtest_dep'])
>
> -if opt_lc_compliance.disabled() or not libevent.found() or not libgtest.found()
> - lc_compliance_enabled = false
> +lc_compliance_enabled = opt_lc_compliance.allowed() and libgtest.found()
> +if not lc_compliance_enabled
> subdir_done()
> endif
>
> -lc_compliance_enabled = true
> -
> lc_compliance_sources = files([
> 'environment.cpp',
> 'helpers/capture.cpp',
> @@ -29,7 +27,6 @@ lc_compliance = executable('lc-compliance', lc_compliance_sources,
> dependencies : [
> libatomic,
> libcamera_public,
> - libevent,
> libgtest,
> ],
> include_directories : lc_compliance_includes,
> diff --git a/src/apps/meson.build b/src/apps/meson.build
> index af632b9a7..252491441 100644
> --- a/src/apps/meson.build
> +++ b/src/apps/meson.build
> @@ -3,12 +3,7 @@
> opt_cam = get_option('cam')
> opt_lc_compliance = get_option('lc-compliance')
>
> -# libevent is needed by cam and lc-compliance. As they are both feature options,
> -# they can't be combined with simple boolean logic.
> libevent = dependency('libevent_pthreads', required : opt_cam)
> -if not libevent.found()
> - libevent = dependency('libevent_pthreads', required : opt_lc_compliance)
> -endif
Do you think the EventLoop abstraction is needed at all for the other
apps ?
Nice rework overall
Reviewed-by: Jacopo Mondi <jacopo.mondi at ideasonboard.com>
Thanks
j
>
> libtiff = dependency('libtiff-4', required : false)
>
> --
> 2.47.1
>
>
More information about the libcamera-devel
mailing list