[RFC PATCH v1 08/12] apps: lc-compliance: Remove libevent dependency
Barnabás Pőcze
pobrn at protonmail.com
Fri Jan 10 11:40:52 CET 2025
Hi
2025. január 7., kedd 18:06 keltezéssel, Jacopo Mondi <jacopo.mondi at ideasonboard.com> írta:
> 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 ?
`value` is enough, but I'm just used to using `[&]`, especially in lambdas
that don't escape their scope of creation.
>
> > + 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 ?
The question here is which value should be stored in multiple `set()` calls
happen. I opted for the earliest but it could easily be the latest.
>
> > + 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 ?
It certainly seems to be needed for other parts.
Regards,
Barnabás Pőcze
>
> 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