| Message ID | 20241220150759.709756-9-pobrn@protonmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
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@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@ideasonboard.com> Thanks j > > libtiff = dependency('libtiff-4', required : false) > > -- > 2.47.1 > >
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. > > > > 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@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@ideasonboard.com> > > > > > libtiff = dependency('libtiff-4', required : false) > >
Hi 2025. január 10., péntek 1:34 keltezéssel, Laurent Pinchart <laurent.pinchart@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@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@ideasonboard.com> > > > > > > > > libtiff = dependency('libtiff-4', required : false) > > > > > -- > Regards, > > Laurent Pinchart >
Hi 2025. január 7., kedd 18:06 keltezéssel, Jacopo Mondi <jacopo.mondi@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@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@ideasonboard.com> > > Thanks > j > > > > > > libtiff = dependency('libtiff-4', required : false) > > > > -- > > 2.47.1 > > > > >
2025. január 10., péntek 11:28 keltezéssel, Barnabás Pőcze <pobrn@protonmail.com> írta: > 2025. január 10., péntek 1:34 keltezéssel, Laurent Pinchart <laurent.pinchart@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? It would appear that `EventLoop::callLater()` might be an option as well. > > > 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@protonmail.com> > > > > --- > > > > [...]
On Fri, Jan 10, 2025 at 10:44:07AM +0000, Barnabás Pőcze wrote: > 2025. január 10., péntek 11:28 keltezéssel, Barnabás Pőcze írta: > > 2025. január 10., péntek 1:34 keltezéssel, Laurent Pinchart í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? > > It would appear that `EventLoop::callLater()` might be an option as well. I suppose the issue isn't just that loop_ could be null when the request completion handler calls loop_->exit(), but also the fact that exit() could be called before exec(). Using callLater() as done in the cam application should work I think, it the fix should then be simple and not too intrusive. Unless I'm missing something :-) Could you try it out ? > > > > > 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@protonmail.com> > > > > > --- > > > > > [...]
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_) { } @@ -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(); + 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 + { + private: + std::mutex mutex_; + std::condition_variable cv_; + std::optional<int> value_; + + public: + int wait() + { + std::unique_lock guard(mutex_); + + cv_.wait(guard, [&] { + return value_.has_value(); + }); + + return *value_; + } + + void set(int value) + { + std::unique_lock guard(mutex_); + + if (!value_) + 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 libtiff = dependency('libtiff-4', required : false)
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;`: [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@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(-)