[RFC PATCH v1 08/12] apps: lc-compliance: Remove libevent dependency
Barnabás Pőcze
pobrn at protonmail.com
Fri Jan 10 11:28:22 CET 2025
Hi
2025. január 10., péntek 1:34 keltezéssel, Laurent Pinchart <laurent.pinchart at ideasonboard.com> írta:
> On Tue, Jan 07, 2025 at 06:06:04PM +0100, Jacopo Mondi wrote:
> > 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.
>
> True, but will that always be the case ? The current tests are fairly
> simple, but what will happen when we'll have more complex tests that
> will need to perform multiple actions through a capture session ? I'm
> sure we could still handle that manually with condition variables, but I
> think that will become complex, and it feels like reinventing the wheel.
>
The current libevent usage is not quite correct, so it needs to be fixed in any case.
This patch implements probably the simplest option that works for the current
requirements. Alternatively, I believe manually triggered events of libevent could
be used to achieve the same, but that would easily be more code and touch more
components (e.g. `event_loop.{cpp,h}`). But I'm not trying to suggest that libevent
should not be used anymore. It could be reintroduced in the future easily in my opinion.
So which one should it be?
Regards,
Barnabás Pőcze
> > >
> > > 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>
> >
> > >
> > > libtiff = dependency('libtiff-4', required : false)
> > >
>
> --
> Regards,
>
> Laurent Pinchart
>
More information about the libcamera-devel
mailing list