[{"id":32950,"web_url":"https://patchwork.libcamera.org/comment/32950/","msgid":"<qex6tsqykhbg44taaxlmq2ns5zzipp7hpnwy4kypnbzvpk6qbc@cdqmiac5b7xa>","date":"2025-01-07T17:06:04","subject":"Re: [RFC PATCH v1 08/12] apps: lc-compliance: Remove libevent\n\tdependency","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Barnabás\n\nOn Fri, Dec 20, 2024 at 03:08:37PM +0000, Barnabás Pőcze wrote:\n> libevent is only used as a way to notify the main thread from\n> the CameraManager thread when the capture should be terminated.\n>\n> Not only is libevent not really necessary and instead can be\n> replaced with a simple combination of C++ STL parts, the current\n> usage is prone to race conditions.\n>\n> Specifically, the camera's `queueRequest()` might complete the\n> request before the main thread could set the `loop_` member\n> variable and synchronize with the thread. This is a data race,\n> practically resulting in a nullptr or dangling pointer dereference.\n>\n> This can easily be triggered by inserting a sufficiently large\n> timeout before `loop_ = new EventLoop;`:\n\nTo be frank this is kind of hard to trigger, as it can only happens\nif the main thread does not execute for longer than the frame\ninterval. But yes, who knows, if there is a chance for this to happen,\nit will happen sooner or later, so it's indeed worth addressing it.\n\n>\n>   [46:43:40.529620351] [671833] DEBUG Request request.cpp:129 Request(4:C:0/1:0)\n>   ../src/apps/lc-compliance/helpers/capture.cpp:140:14: runtime error: member call on null pointer of type 'struct EventLoop'\n>     0 0x632c3e82f81a in CaptureBalanced::requestComplete(libcamera::Request*) ../src/apps/lc-compliance/helpers/capture.cpp:140\n>\n> Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com>\n> ---\n>  README.rst                                 |  2 +-\n>  src/apps/lc-compliance/helpers/capture.cpp | 23 ++++++------\n>  src/apps/lc-compliance/helpers/capture.h   | 42 +++++++++++++++++++---\n>  src/apps/lc-compliance/meson.build         |  7 ++--\n>  src/apps/meson.build                       |  5 ---\n>  5 files changed, 53 insertions(+), 26 deletions(-)\n>\n> diff --git a/README.rst b/README.rst\n> index 4068c6cc8..f1749be97 100644\n> --- a/README.rst\n> +++ b/README.rst\n> @@ -97,7 +97,7 @@ for Python bindings: [optional]\n>          pybind11-dev\n>\n>  for lc-compliance: [optional]\n> -        libevent-dev libgtest-dev\n> +        libgtest-dev\n>\n>  for abi-compat.sh: [optional]\n>          abi-compliance-checker\n> diff --git a/src/apps/lc-compliance/helpers/capture.cpp b/src/apps/lc-compliance/helpers/capture.cpp\n> index 91c4d4400..b473e0773 100644\n> --- a/src/apps/lc-compliance/helpers/capture.cpp\n> +++ b/src/apps/lc-compliance/helpers/capture.cpp\n> @@ -12,7 +12,7 @@\n>  using namespace libcamera;\n>\n>  Capture::Capture(std::shared_ptr<Camera> camera)\n> -\t: loop_(nullptr), camera_(std::move(camera)),\n> +\t: camera_(std::move(camera)),\n>  \t  allocator_(camera_)\n\nThis now fits on the previous line\n\n>  {\n>  }\n> @@ -52,6 +52,8 @@ void Capture::start()\n>\n>  \tcamera_->requestCompleted.connect(this, &Capture::requestComplete);\n>\n> +\tresult_.reset();\n> +\n>  \tASSERT_EQ(camera_->start(), 0) << \"Failed to start camera\";\n>  }\n>\n> @@ -62,6 +64,8 @@ void Capture::stop()\n>\n>  \tcamera_->stop();\n>\n> +\tresult_.reset();\n> +\n\nI don't think you could have a stop-stop or start-start sequence, so\nprobably a reset in start() is all you need. However this doesn't hurt\n\n>  \tcamera_->requestCompleted.disconnect(this);\n>\n>  \tStream *stream = config_->at(0).stream();\n> @@ -108,11 +112,10 @@ void CaptureBalanced::capture(unsigned int numRequests)\n>  \t}\n>\n>  \t/* Run capture session. */\n> -\tloop_ = new EventLoop();\n> -\tloop_->exec();\n> +\tint status = result_.wait();\n>  \tstop();\n> -\tdelete loop_;\n>\n> +\tASSERT_EQ(status, 0);\n>  \tASSERT_EQ(captureCount_, captureLimit_);\n>  }\n>\n> @@ -132,13 +135,13 @@ void CaptureBalanced::requestComplete(Request *request)\n>\n>  \tcaptureCount_++;\n>  \tif (captureCount_ >= captureLimit_) {\n> -\t\tloop_->exit(0);\n> +\t\tresult_.set(0);\n>  \t\treturn;\n>  \t}\n>\n>  \trequest->reuse(Request::ReuseBuffers);\n>  \tif (queueRequest(request))\n> -\t\tloop_->exit(-EINVAL);\n> +\t\tresult_.set(-EINVAL);\n>  }\n>\n>  /* CaptureUnbalanced */\n> @@ -171,10 +174,8 @@ void CaptureUnbalanced::capture(unsigned int numRequests)\n>  \t}\n>\n>  \t/* Run capture session. */\n> -\tloop_ = new EventLoop();\n> -\tint status = loop_->exec();\n> +\tint status = result_.wait();\n>  \tstop();\n> -\tdelete loop_;\n>\n>  \tASSERT_EQ(status, 0);\n>  }\n> @@ -183,7 +184,7 @@ void CaptureUnbalanced::requestComplete(Request *request)\n>  {\n>  \tcaptureCount_++;\n>  \tif (captureCount_ >= captureLimit_) {\n> -\t\tloop_->exit(0);\n> +\t\tresult_.set(0);\n>  \t\treturn;\n>  \t}\n>\n> @@ -192,5 +193,5 @@ void CaptureUnbalanced::requestComplete(Request *request)\n>\n>  \trequest->reuse(Request::ReuseBuffers);\n>  \tif (camera_->queueRequest(request))\n> -\t\tloop_->exit(-EINVAL);\n> +\t\tresult_.set(-EINVAL);\n>  }\n> diff --git a/src/apps/lc-compliance/helpers/capture.h b/src/apps/lc-compliance/helpers/capture.h\n> index a4cc3a99e..8582ade8e 100644\n> --- a/src/apps/lc-compliance/helpers/capture.h\n> +++ b/src/apps/lc-compliance/helpers/capture.h\n> @@ -7,12 +7,13 @@\n>\n>  #pragma once\n>\n> +#include <condition_variable>\n>  #include <memory>\n> +#include <mutex>\n> +#include <optional>\n>\n>  #include <libcamera/libcamera.h>\n>\n> -#include \"../common/event_loop.h\"\n> -\n>  class Capture\n>  {\n>  public:\n> @@ -27,12 +28,45 @@ protected:\n>\n>  \tvirtual void requestComplete(libcamera::Request *request) = 0;\n>\n> -\tEventLoop *loop_;\n> -\n>  \tstd::shared_ptr<libcamera::Camera> camera_;\n>  \tlibcamera::FrameBufferAllocator allocator_;\n>  \tstd::unique_ptr<libcamera::CameraConfiguration> config_;\n>  \tstd::vector<std::unique_ptr<libcamera::Request>> requests_;\n> +\n> +\tstruct\n> +\t{\n\nnit: We usually do\n\n        struct {\n\n> +\tprivate:\n\nWith private: after public:\n\n> +\t\tstd::mutex mutex_;\n> +\t\tstd::condition_variable cv_;\n> +\t\tstd::optional<int> value_;\n> +\n> +\tpublic:\n> +\t\tint wait()\n> +\t\t{\n> +\t\t\tstd::unique_lock guard(mutex_);\n> +\n> +\t\t\tcv_.wait(guard, [&] {\n\nDo you need to capture the whole env or &value is enough ?\n\n> +\t\t\t\treturn value_.has_value();\n> +\t\t\t});\n> +\n> +\t\t\treturn *value_;\n> +\t\t}\n> +\n> +\t\tvoid set(int value)\n> +\t\t{\n> +\t\t\tstd::unique_lock guard(mutex_);\n> +\n> +\t\t\tif (!value_)\n\nWhy do you think this can't be overriden ?\n\n> +\t\t\t\tvalue_ = value;\n> +\n> +\t\t\tcv_.notify_all();\n> +\t\t}\n> +\n> +\t\tvoid reset()\n> +\t\t{\n> +\t\t\tvalue_.reset();\n> +\t\t}\n> +\t} result_;\n>  };\n>\n>  class CaptureBalanced : public Capture\n> diff --git a/src/apps/lc-compliance/meson.build b/src/apps/lc-compliance/meson.build\n> index b1f605f33..0884bc6ca 100644\n> --- a/src/apps/lc-compliance/meson.build\n> +++ b/src/apps/lc-compliance/meson.build\n> @@ -4,13 +4,11 @@ libgtest = dependency('gtest', version : '>=1.10.0',\n>                        required : get_option('lc-compliance'),\n>                        fallback : ['gtest', 'gtest_dep'])\n>\n> -if opt_lc_compliance.disabled() or not libevent.found() or not libgtest.found()\n> -    lc_compliance_enabled = false\n> +lc_compliance_enabled = opt_lc_compliance.allowed() and libgtest.found()\n> +if not lc_compliance_enabled\n>      subdir_done()\n>  endif\n>\n> -lc_compliance_enabled = true\n> -\n>  lc_compliance_sources = files([\n>      'environment.cpp',\n>      'helpers/capture.cpp',\n> @@ -29,7 +27,6 @@ lc_compliance  = executable('lc-compliance', lc_compliance_sources,\n>                              dependencies : [\n>                                  libatomic,\n>                                  libcamera_public,\n> -                                libevent,\n>                                  libgtest,\n>                              ],\n>                              include_directories : lc_compliance_includes,\n> diff --git a/src/apps/meson.build b/src/apps/meson.build\n> index af632b9a7..252491441 100644\n> --- a/src/apps/meson.build\n> +++ b/src/apps/meson.build\n> @@ -3,12 +3,7 @@\n>  opt_cam = get_option('cam')\n>  opt_lc_compliance = get_option('lc-compliance')\n>\n> -# libevent is needed by cam and lc-compliance. As they are both feature options,\n> -# they can't be combined with simple boolean logic.\n>  libevent = dependency('libevent_pthreads', required : opt_cam)\n> -if not libevent.found()\n> -    libevent = dependency('libevent_pthreads', required : opt_lc_compliance)\n> -endif\n\nDo you think the EventLoop abstraction is needed at all for the other\napps ?\n\nNice rework overall\nReviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>\n\nThanks\n  j\n\n\n>\n>  libtiff = dependency('libtiff-4', required : false)\n>\n> --\n> 2.47.1\n>\n>","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id B5E6AC32DC\n\tfor <parsemail@patchwork.libcamera.org>;\n\tTue,  7 Jan 2025 17:06:10 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C3AD3684E2;\n\tTue,  7 Jan 2025 18:06:09 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C7A72684CD\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tTue,  7 Jan 2025 18:06:07 +0100 (CET)","from ideasonboard.com (mob-5-90-140-128.net.vodafone.it\n\t[5.90.140.128])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 4C280496;\n\tTue,  7 Jan 2025 18:05:15 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"H4sJj+19\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1736269515;\n\tbh=rs66GOm+Qi4NXilR+356dUzCApKd2/294xZ0vqOAU/Y=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=H4sJj+191tKorBoN4M26NuET3/hwDbyZ5BWvz8eR+lB6uxcuaUxZA4K7WUkiGeS5J\n\tvwtA35aUEaqzolpPq1TCwsP4phIYi7uKlLgT08j5ARpmPw9DAOU8UoVhe4cYt8NqMh\n\t18HfF486pdR+4Z/+z6jAgeVlbH1Wl5oBUdzS3Usk=","Date":"Tue, 7 Jan 2025 18:06:04 +0100","From":"Jacopo Mondi <jacopo.mondi@ideasonboard.com>","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= <pobrn@protonmail.com>","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [RFC PATCH v1 08/12] apps: lc-compliance: Remove libevent\n\tdependency","Message-ID":"<qex6tsqykhbg44taaxlmq2ns5zzipp7hpnwy4kypnbzvpk6qbc@cdqmiac5b7xa>","References":"<20241220150759.709756-1-pobrn@protonmail.com>\n\t<20241220150759.709756-9-pobrn@protonmail.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20241220150759.709756-9-pobrn@protonmail.com>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":33004,"web_url":"https://patchwork.libcamera.org/comment/33004/","msgid":"<20250110003445.GL6159@pendragon.ideasonboard.com>","date":"2025-01-10T00:34:45","subject":"Re: [RFC PATCH v1 08/12] apps: lc-compliance: Remove libevent\n\tdependency","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Tue, Jan 07, 2025 at 06:06:04PM +0100, Jacopo Mondi wrote:\n> On Fri, Dec 20, 2024 at 03:08:37PM +0000, Barnabás Pőcze wrote:\n> > libevent is only used as a way to notify the main thread from\n> > the CameraManager thread when the capture should be terminated.\n\nTrue, but will that always be the case ? The current tests are fairly\nsimple, but what will happen when we'll have more complex tests that\nwill need to perform multiple actions through a capture session ? I'm\nsure we could still handle that manually with condition variables, but I\nthink that will become complex, and it feels like reinventing the wheel.\n\n> >\n> > Not only is libevent not really necessary and instead can be\n> > replaced with a simple combination of C++ STL parts, the current\n> > usage is prone to race conditions.\n> >\n> > Specifically, the camera's `queueRequest()` might complete the\n> > request before the main thread could set the `loop_` member\n> > variable and synchronize with the thread. This is a data race,\n> > practically resulting in a nullptr or dangling pointer dereference.\n> >\n> > This can easily be triggered by inserting a sufficiently large\n> > timeout before `loop_ = new EventLoop;`:\n> \n> To be frank this is kind of hard to trigger, as it can only happens\n> if the main thread does not execute for longer than the frame\n> interval. But yes, who knows, if there is a chance for this to happen,\n> it will happen sooner or later, so it's indeed worth addressing it.\n> \n> >\n> >   [46:43:40.529620351] [671833] DEBUG Request request.cpp:129 Request(4:C:0/1:0)\n> >   ../src/apps/lc-compliance/helpers/capture.cpp:140:14: runtime error: member call on null pointer of type 'struct EventLoop'\n> >     0 0x632c3e82f81a in CaptureBalanced::requestComplete(libcamera::Request*) ../src/apps/lc-compliance/helpers/capture.cpp:140\n> >\n> > Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com>\n> > ---\n> >  README.rst                                 |  2 +-\n> >  src/apps/lc-compliance/helpers/capture.cpp | 23 ++++++------\n> >  src/apps/lc-compliance/helpers/capture.h   | 42 +++++++++++++++++++---\n> >  src/apps/lc-compliance/meson.build         |  7 ++--\n> >  src/apps/meson.build                       |  5 ---\n> >  5 files changed, 53 insertions(+), 26 deletions(-)\n> >\n> > diff --git a/README.rst b/README.rst\n> > index 4068c6cc8..f1749be97 100644\n> > --- a/README.rst\n> > +++ b/README.rst\n> > @@ -97,7 +97,7 @@ for Python bindings: [optional]\n> >          pybind11-dev\n> >\n> >  for lc-compliance: [optional]\n> > -        libevent-dev libgtest-dev\n> > +        libgtest-dev\n> >\n> >  for abi-compat.sh: [optional]\n> >          abi-compliance-checker\n> > diff --git a/src/apps/lc-compliance/helpers/capture.cpp b/src/apps/lc-compliance/helpers/capture.cpp\n> > index 91c4d4400..b473e0773 100644\n> > --- a/src/apps/lc-compliance/helpers/capture.cpp\n> > +++ b/src/apps/lc-compliance/helpers/capture.cpp\n> > @@ -12,7 +12,7 @@\n> >  using namespace libcamera;\n> >\n> >  Capture::Capture(std::shared_ptr<Camera> camera)\n> > -\t: loop_(nullptr), camera_(std::move(camera)),\n> > +\t: camera_(std::move(camera)),\n> >  \t  allocator_(camera_)\n> \n> This now fits on the previous line\n> \n> >  {\n> >  }\n> > @@ -52,6 +52,8 @@ void Capture::start()\n> >\n> >  \tcamera_->requestCompleted.connect(this, &Capture::requestComplete);\n> >\n> > +\tresult_.reset();\n> > +\n> >  \tASSERT_EQ(camera_->start(), 0) << \"Failed to start camera\";\n> >  }\n> >\n> > @@ -62,6 +64,8 @@ void Capture::stop()\n> >\n> >  \tcamera_->stop();\n> >\n> > +\tresult_.reset();\n> > +\n> \n> I don't think you could have a stop-stop or start-start sequence, so\n> probably a reset in start() is all you need. However this doesn't hurt\n> \n> >  \tcamera_->requestCompleted.disconnect(this);\n> >\n> >  \tStream *stream = config_->at(0).stream();\n> > @@ -108,11 +112,10 @@ void CaptureBalanced::capture(unsigned int numRequests)\n> >  \t}\n> >\n> >  \t/* Run capture session. */\n> > -\tloop_ = new EventLoop();\n> > -\tloop_->exec();\n> > +\tint status = result_.wait();\n> >  \tstop();\n> > -\tdelete loop_;\n> >\n> > +\tASSERT_EQ(status, 0);\n> >  \tASSERT_EQ(captureCount_, captureLimit_);\n> >  }\n> >\n> > @@ -132,13 +135,13 @@ void CaptureBalanced::requestComplete(Request *request)\n> >\n> >  \tcaptureCount_++;\n> >  \tif (captureCount_ >= captureLimit_) {\n> > -\t\tloop_->exit(0);\n> > +\t\tresult_.set(0);\n> >  \t\treturn;\n> >  \t}\n> >\n> >  \trequest->reuse(Request::ReuseBuffers);\n> >  \tif (queueRequest(request))\n> > -\t\tloop_->exit(-EINVAL);\n> > +\t\tresult_.set(-EINVAL);\n> >  }\n> >\n> >  /* CaptureUnbalanced */\n> > @@ -171,10 +174,8 @@ void CaptureUnbalanced::capture(unsigned int numRequests)\n> >  \t}\n> >\n> >  \t/* Run capture session. */\n> > -\tloop_ = new EventLoop();\n> > -\tint status = loop_->exec();\n> > +\tint status = result_.wait();\n> >  \tstop();\n> > -\tdelete loop_;\n> >\n> >  \tASSERT_EQ(status, 0);\n> >  }\n> > @@ -183,7 +184,7 @@ void CaptureUnbalanced::requestComplete(Request *request)\n> >  {\n> >  \tcaptureCount_++;\n> >  \tif (captureCount_ >= captureLimit_) {\n> > -\t\tloop_->exit(0);\n> > +\t\tresult_.set(0);\n> >  \t\treturn;\n> >  \t}\n> >\n> > @@ -192,5 +193,5 @@ void CaptureUnbalanced::requestComplete(Request *request)\n> >\n> >  \trequest->reuse(Request::ReuseBuffers);\n> >  \tif (camera_->queueRequest(request))\n> > -\t\tloop_->exit(-EINVAL);\n> > +\t\tresult_.set(-EINVAL);\n> >  }\n> > diff --git a/src/apps/lc-compliance/helpers/capture.h b/src/apps/lc-compliance/helpers/capture.h\n> > index a4cc3a99e..8582ade8e 100644\n> > --- a/src/apps/lc-compliance/helpers/capture.h\n> > +++ b/src/apps/lc-compliance/helpers/capture.h\n> > @@ -7,12 +7,13 @@\n> >\n> >  #pragma once\n> >\n> > +#include <condition_variable>\n> >  #include <memory>\n> > +#include <mutex>\n> > +#include <optional>\n> >\n> >  #include <libcamera/libcamera.h>\n> >\n> > -#include \"../common/event_loop.h\"\n> > -\n> >  class Capture\n> >  {\n> >  public:\n> > @@ -27,12 +28,45 @@ protected:\n> >\n> >  \tvirtual void requestComplete(libcamera::Request *request) = 0;\n> >\n> > -\tEventLoop *loop_;\n> > -\n> >  \tstd::shared_ptr<libcamera::Camera> camera_;\n> >  \tlibcamera::FrameBufferAllocator allocator_;\n> >  \tstd::unique_ptr<libcamera::CameraConfiguration> config_;\n> >  \tstd::vector<std::unique_ptr<libcamera::Request>> requests_;\n> > +\n> > +\tstruct\n> > +\t{\n> \n> nit: We usually do\n> \n>         struct {\n> \n> > +\tprivate:\n> \n> With private: after public:\n> \n> > +\t\tstd::mutex mutex_;\n> > +\t\tstd::condition_variable cv_;\n> > +\t\tstd::optional<int> value_;\n> > +\n> > +\tpublic:\n> > +\t\tint wait()\n> > +\t\t{\n> > +\t\t\tstd::unique_lock guard(mutex_);\n> > +\n> > +\t\t\tcv_.wait(guard, [&] {\n> \n> Do you need to capture the whole env or &value is enough ?\n> \n> > +\t\t\t\treturn value_.has_value();\n> > +\t\t\t});\n> > +\n> > +\t\t\treturn *value_;\n> > +\t\t}\n> > +\n> > +\t\tvoid set(int value)\n> > +\t\t{\n> > +\t\t\tstd::unique_lock guard(mutex_);\n> > +\n> > +\t\t\tif (!value_)\n> \n> Why do you think this can't be overriden ?\n> \n> > +\t\t\t\tvalue_ = value;\n> > +\n> > +\t\t\tcv_.notify_all();\n> > +\t\t}\n> > +\n> > +\t\tvoid reset()\n> > +\t\t{\n> > +\t\t\tvalue_.reset();\n> > +\t\t}\n> > +\t} result_;\n> >  };\n> >\n> >  class CaptureBalanced : public Capture\n> > diff --git a/src/apps/lc-compliance/meson.build b/src/apps/lc-compliance/meson.build\n> > index b1f605f33..0884bc6ca 100644\n> > --- a/src/apps/lc-compliance/meson.build\n> > +++ b/src/apps/lc-compliance/meson.build\n> > @@ -4,13 +4,11 @@ libgtest = dependency('gtest', version : '>=1.10.0',\n> >                        required : get_option('lc-compliance'),\n> >                        fallback : ['gtest', 'gtest_dep'])\n> >\n> > -if opt_lc_compliance.disabled() or not libevent.found() or not libgtest.found()\n> > -    lc_compliance_enabled = false\n> > +lc_compliance_enabled = opt_lc_compliance.allowed() and libgtest.found()\n> > +if not lc_compliance_enabled\n> >      subdir_done()\n> >  endif\n> >\n> > -lc_compliance_enabled = true\n> > -\n> >  lc_compliance_sources = files([\n> >      'environment.cpp',\n> >      'helpers/capture.cpp',\n> > @@ -29,7 +27,6 @@ lc_compliance  = executable('lc-compliance', lc_compliance_sources,\n> >                              dependencies : [\n> >                                  libatomic,\n> >                                  libcamera_public,\n> > -                                libevent,\n> >                                  libgtest,\n> >                              ],\n> >                              include_directories : lc_compliance_includes,\n> > diff --git a/src/apps/meson.build b/src/apps/meson.build\n> > index af632b9a7..252491441 100644\n> > --- a/src/apps/meson.build\n> > +++ b/src/apps/meson.build\n> > @@ -3,12 +3,7 @@\n> >  opt_cam = get_option('cam')\n> >  opt_lc_compliance = get_option('lc-compliance')\n> >\n> > -# libevent is needed by cam and lc-compliance. As they are both feature options,\n> > -# they can't be combined with simple boolean logic.\n> >  libevent = dependency('libevent_pthreads', required : opt_cam)\n> > -if not libevent.found()\n> > -    libevent = dependency('libevent_pthreads', required : opt_lc_compliance)\n> > -endif\n> \n> Do you think the EventLoop abstraction is needed at all for the other\n> apps ?\n> \n> Nice rework overall\n> Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>\n> \n> >\n> >  libtiff = dependency('libtiff-4', required : false)\n> >","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id D97A5C32EF\n\tfor <parsemail@patchwork.libcamera.org>;\n\tFri, 10 Jan 2025 00:34:50 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 466D2684E0;\n\tFri, 10 Jan 2025 01:34:50 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E0598608A9\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri, 10 Jan 2025 01:34:48 +0100 (CET)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id BB3A478C;\n\tFri, 10 Jan 2025 01:33:54 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"bbJC12qx\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1736469234;\n\tbh=MgrdfjtmP94RUJGeydch0bpxtWmHTgcxg1GXgIPqugM=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=bbJC12qx+tp3Xzp29MfRb+qCPzg1aIggr/RHrp6c36B4H033vNT/5iuiYdw2XtxKr\n\tdhfQD1GBAnnX0d90qFJgm+7qWp6dpp57v2MrSYzbBDJnW3gbtqjIRakGPT6zTIhVp4\n\tnEtZqN6GVDaOsv2th8envr54zUF/YYYKeOh6Y0/o=","Date":"Fri, 10 Jan 2025 02:34:45 +0200","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Jacopo Mondi <jacopo.mondi@ideasonboard.com>","Cc":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= <pobrn@protonmail.com>,\n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [RFC PATCH v1 08/12] apps: lc-compliance: Remove libevent\n\tdependency","Message-ID":"<20250110003445.GL6159@pendragon.ideasonboard.com>","References":"<20241220150759.709756-1-pobrn@protonmail.com>\n\t<20241220150759.709756-9-pobrn@protonmail.com>\n\t<qex6tsqykhbg44taaxlmq2ns5zzipp7hpnwy4kypnbzvpk6qbc@cdqmiac5b7xa>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<qex6tsqykhbg44taaxlmq2ns5zzipp7hpnwy4kypnbzvpk6qbc@cdqmiac5b7xa>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":33014,"web_url":"https://patchwork.libcamera.org/comment/33014/","msgid":"<O2RtBLpth6uWwYQQ18jgO7wHmNeKzGUhYFqrT83JUFB_szJxR2AjNvZ3GpcjmX9SkGXoIHTqg0AR1Lkjd-RfnXfVJkxKsExGiryj2DWcqUU=@protonmail.com>","date":"2025-01-10T10:28:22","subject":"Re: [RFC PATCH v1 08/12] apps: lc-compliance: Remove libevent\n\tdependency","submitter":{"id":133,"url":"https://patchwork.libcamera.org/api/people/133/","name":"Pőcze Barnabás","email":"pobrn@protonmail.com"},"content":"Hi\n\n\n2025. január 10., péntek 1:34 keltezéssel, Laurent Pinchart <laurent.pinchart@ideasonboard.com> írta:\n\n> On Tue, Jan 07, 2025 at 06:06:04PM +0100, Jacopo Mondi wrote:\n> > On Fri, Dec 20, 2024 at 03:08:37PM +0000, Barnabás Pőcze wrote:\n> > > libevent is only used as a way to notify the main thread from\n> > > the CameraManager thread when the capture should be terminated.\n> \n> True, but will that always be the case ? The current tests are fairly\n> simple, but what will happen when we'll have more complex tests that\n> will need to perform multiple actions through a capture session ? I'm\n> sure we could still handle that manually with condition variables, but I\n> think that will become complex, and it feels like reinventing the wheel.\n> \n\nThe current libevent usage is not quite correct, so it needs to be fixed in any case.\nThis patch implements probably the simplest option that works for the current\nrequirements. Alternatively, I believe manually triggered events of libevent could\nbe used to achieve the same, but that would easily be more code and touch more\ncomponents (e.g. `event_loop.{cpp,h}`). But I'm not trying to suggest that libevent\nshould not be used anymore. It could be reintroduced in the future easily in my opinion.\nSo which one should it be?\n\n\nRegards,\nBarnabás Pőcze\n\n\n> > >\n> > > Not only is libevent not really necessary and instead can be\n> > > replaced with a simple combination of C++ STL parts, the current\n> > > usage is prone to race conditions.\n> > >\n> > > Specifically, the camera's `queueRequest()` might complete the\n> > > request before the main thread could set the `loop_` member\n> > > variable and synchronize with the thread. This is a data race,\n> > > practically resulting in a nullptr or dangling pointer dereference.\n> > >\n> > > This can easily be triggered by inserting a sufficiently large\n> > > timeout before `loop_ = new EventLoop;`:\n> >\n> > To be frank this is kind of hard to trigger, as it can only happens\n> > if the main thread does not execute for longer than the frame\n> > interval. But yes, who knows, if there is a chance for this to happen,\n> > it will happen sooner or later, so it's indeed worth addressing it.\n> >\n> > >\n> > >   [46:43:40.529620351] [671833] DEBUG Request request.cpp:129 Request(4:C:0/1:0)\n> > >   ../src/apps/lc-compliance/helpers/capture.cpp:140:14: runtime error: member call on null pointer of type 'struct EventLoop'\n> > >     0 0x632c3e82f81a in CaptureBalanced::requestComplete(libcamera::Request*) ../src/apps/lc-compliance/helpers/capture.cpp:140\n> > >\n> > > Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com>\n> > > ---\n> > >  README.rst                                 |  2 +-\n> > >  src/apps/lc-compliance/helpers/capture.cpp | 23 ++++++------\n> > >  src/apps/lc-compliance/helpers/capture.h   | 42 +++++++++++++++++++---\n> > >  src/apps/lc-compliance/meson.build         |  7 ++--\n> > >  src/apps/meson.build                       |  5 ---\n> > >  5 files changed, 53 insertions(+), 26 deletions(-)\n> > >\n> > > diff --git a/README.rst b/README.rst\n> > > index 4068c6cc8..f1749be97 100644\n> > > --- a/README.rst\n> > > +++ b/README.rst\n> > > @@ -97,7 +97,7 @@ for Python bindings: [optional]\n> > >          pybind11-dev\n> > >\n> > >  for lc-compliance: [optional]\n> > > -        libevent-dev libgtest-dev\n> > > +        libgtest-dev\n> > >\n> > >  for abi-compat.sh: [optional]\n> > >          abi-compliance-checker\n> > > diff --git a/src/apps/lc-compliance/helpers/capture.cpp b/src/apps/lc-compliance/helpers/capture.cpp\n> > > index 91c4d4400..b473e0773 100644\n> > > --- a/src/apps/lc-compliance/helpers/capture.cpp\n> > > +++ b/src/apps/lc-compliance/helpers/capture.cpp\n> > > @@ -12,7 +12,7 @@\n> > >  using namespace libcamera;\n> > >\n> > >  Capture::Capture(std::shared_ptr<Camera> camera)\n> > > -\t: loop_(nullptr), camera_(std::move(camera)),\n> > > +\t: camera_(std::move(camera)),\n> > >  \t  allocator_(camera_)\n> >\n> > This now fits on the previous line\n> >\n> > >  {\n> > >  }\n> > > @@ -52,6 +52,8 @@ void Capture::start()\n> > >\n> > >  \tcamera_->requestCompleted.connect(this, &Capture::requestComplete);\n> > >\n> > > +\tresult_.reset();\n> > > +\n> > >  \tASSERT_EQ(camera_->start(), 0) << \"Failed to start camera\";\n> > >  }\n> > >\n> > > @@ -62,6 +64,8 @@ void Capture::stop()\n> > >\n> > >  \tcamera_->stop();\n> > >\n> > > +\tresult_.reset();\n> > > +\n> >\n> > I don't think you could have a stop-stop or start-start sequence, so\n> > probably a reset in start() is all you need. However this doesn't hurt\n> >\n> > >  \tcamera_->requestCompleted.disconnect(this);\n> > >\n> > >  \tStream *stream = config_->at(0).stream();\n> > > @@ -108,11 +112,10 @@ void CaptureBalanced::capture(unsigned int numRequests)\n> > >  \t}\n> > >\n> > >  \t/* Run capture session. */\n> > > -\tloop_ = new EventLoop();\n> > > -\tloop_->exec();\n> > > +\tint status = result_.wait();\n> > >  \tstop();\n> > > -\tdelete loop_;\n> > >\n> > > +\tASSERT_EQ(status, 0);\n> > >  \tASSERT_EQ(captureCount_, captureLimit_);\n> > >  }\n> > >\n> > > @@ -132,13 +135,13 @@ void CaptureBalanced::requestComplete(Request *request)\n> > >\n> > >  \tcaptureCount_++;\n> > >  \tif (captureCount_ >= captureLimit_) {\n> > > -\t\tloop_->exit(0);\n> > > +\t\tresult_.set(0);\n> > >  \t\treturn;\n> > >  \t}\n> > >\n> > >  \trequest->reuse(Request::ReuseBuffers);\n> > >  \tif (queueRequest(request))\n> > > -\t\tloop_->exit(-EINVAL);\n> > > +\t\tresult_.set(-EINVAL);\n> > >  }\n> > >\n> > >  /* CaptureUnbalanced */\n> > > @@ -171,10 +174,8 @@ void CaptureUnbalanced::capture(unsigned int numRequests)\n> > >  \t}\n> > >\n> > >  \t/* Run capture session. */\n> > > -\tloop_ = new EventLoop();\n> > > -\tint status = loop_->exec();\n> > > +\tint status = result_.wait();\n> > >  \tstop();\n> > > -\tdelete loop_;\n> > >\n> > >  \tASSERT_EQ(status, 0);\n> > >  }\n> > > @@ -183,7 +184,7 @@ void CaptureUnbalanced::requestComplete(Request *request)\n> > >  {\n> > >  \tcaptureCount_++;\n> > >  \tif (captureCount_ >= captureLimit_) {\n> > > -\t\tloop_->exit(0);\n> > > +\t\tresult_.set(0);\n> > >  \t\treturn;\n> > >  \t}\n> > >\n> > > @@ -192,5 +193,5 @@ void CaptureUnbalanced::requestComplete(Request *request)\n> > >\n> > >  \trequest->reuse(Request::ReuseBuffers);\n> > >  \tif (camera_->queueRequest(request))\n> > > -\t\tloop_->exit(-EINVAL);\n> > > +\t\tresult_.set(-EINVAL);\n> > >  }\n> > > diff --git a/src/apps/lc-compliance/helpers/capture.h b/src/apps/lc-compliance/helpers/capture.h\n> > > index a4cc3a99e..8582ade8e 100644\n> > > --- a/src/apps/lc-compliance/helpers/capture.h\n> > > +++ b/src/apps/lc-compliance/helpers/capture.h\n> > > @@ -7,12 +7,13 @@\n> > >\n> > >  #pragma once\n> > >\n> > > +#include <condition_variable>\n> > >  #include <memory>\n> > > +#include <mutex>\n> > > +#include <optional>\n> > >\n> > >  #include <libcamera/libcamera.h>\n> > >\n> > > -#include \"../common/event_loop.h\"\n> > > -\n> > >  class Capture\n> > >  {\n> > >  public:\n> > > @@ -27,12 +28,45 @@ protected:\n> > >\n> > >  \tvirtual void requestComplete(libcamera::Request *request) = 0;\n> > >\n> > > -\tEventLoop *loop_;\n> > > -\n> > >  \tstd::shared_ptr<libcamera::Camera> camera_;\n> > >  \tlibcamera::FrameBufferAllocator allocator_;\n> > >  \tstd::unique_ptr<libcamera::CameraConfiguration> config_;\n> > >  \tstd::vector<std::unique_ptr<libcamera::Request>> requests_;\n> > > +\n> > > +\tstruct\n> > > +\t{\n> >\n> > nit: We usually do\n> >\n> >         struct {\n> >\n> > > +\tprivate:\n> >\n> > With private: after public:\n> >\n> > > +\t\tstd::mutex mutex_;\n> > > +\t\tstd::condition_variable cv_;\n> > > +\t\tstd::optional<int> value_;\n> > > +\n> > > +\tpublic:\n> > > +\t\tint wait()\n> > > +\t\t{\n> > > +\t\t\tstd::unique_lock guard(mutex_);\n> > > +\n> > > +\t\t\tcv_.wait(guard, [&] {\n> >\n> > Do you need to capture the whole env or &value is enough ?\n> >\n> > > +\t\t\t\treturn value_.has_value();\n> > > +\t\t\t});\n> > > +\n> > > +\t\t\treturn *value_;\n> > > +\t\t}\n> > > +\n> > > +\t\tvoid set(int value)\n> > > +\t\t{\n> > > +\t\t\tstd::unique_lock guard(mutex_);\n> > > +\n> > > +\t\t\tif (!value_)\n> >\n> > Why do you think this can't be overriden ?\n> >\n> > > +\t\t\t\tvalue_ = value;\n> > > +\n> > > +\t\t\tcv_.notify_all();\n> > > +\t\t}\n> > > +\n> > > +\t\tvoid reset()\n> > > +\t\t{\n> > > +\t\t\tvalue_.reset();\n> > > +\t\t}\n> > > +\t} result_;\n> > >  };\n> > >\n> > >  class CaptureBalanced : public Capture\n> > > diff --git a/src/apps/lc-compliance/meson.build b/src/apps/lc-compliance/meson.build\n> > > index b1f605f33..0884bc6ca 100644\n> > > --- a/src/apps/lc-compliance/meson.build\n> > > +++ b/src/apps/lc-compliance/meson.build\n> > > @@ -4,13 +4,11 @@ libgtest = dependency('gtest', version : '>=1.10.0',\n> > >                        required : get_option('lc-compliance'),\n> > >                        fallback : ['gtest', 'gtest_dep'])\n> > >\n> > > -if opt_lc_compliance.disabled() or not libevent.found() or not libgtest.found()\n> > > -    lc_compliance_enabled = false\n> > > +lc_compliance_enabled = opt_lc_compliance.allowed() and libgtest.found()\n> > > +if not lc_compliance_enabled\n> > >      subdir_done()\n> > >  endif\n> > >\n> > > -lc_compliance_enabled = true\n> > > -\n> > >  lc_compliance_sources = files([\n> > >      'environment.cpp',\n> > >      'helpers/capture.cpp',\n> > > @@ -29,7 +27,6 @@ lc_compliance  = executable('lc-compliance', lc_compliance_sources,\n> > >                              dependencies : [\n> > >                                  libatomic,\n> > >                                  libcamera_public,\n> > > -                                libevent,\n> > >                                  libgtest,\n> > >                              ],\n> > >                              include_directories : lc_compliance_includes,\n> > > diff --git a/src/apps/meson.build b/src/apps/meson.build\n> > > index af632b9a7..252491441 100644\n> > > --- a/src/apps/meson.build\n> > > +++ b/src/apps/meson.build\n> > > @@ -3,12 +3,7 @@\n> > >  opt_cam = get_option('cam')\n> > >  opt_lc_compliance = get_option('lc-compliance')\n> > >\n> > > -# libevent is needed by cam and lc-compliance. As they are both feature options,\n> > > -# they can't be combined with simple boolean logic.\n> > >  libevent = dependency('libevent_pthreads', required : opt_cam)\n> > > -if not libevent.found()\n> > > -    libevent = dependency('libevent_pthreads', required : opt_lc_compliance)\n> > > -endif\n> >\n> > Do you think the EventLoop abstraction is needed at all for the other\n> > apps ?\n> >\n> > Nice rework overall\n> > Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>\n> >\n> > >\n> > >  libtiff = dependency('libtiff-4', required : false)\n> > >\n> \n> --\n> Regards,\n> \n> Laurent Pinchart\n>","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 29967C32F5\n\tfor <parsemail@patchwork.libcamera.org>;\n\tFri, 10 Jan 2025 10:28:31 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 24B2E684EA;\n\tFri, 10 Jan 2025 11:28:30 +0100 (CET)","from mail-40133.protonmail.ch (mail-40133.protonmail.ch\n\t[185.70.40.133])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 82F8561882\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri, 10 Jan 2025 11:28:28 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=protonmail.com header.i=@protonmail.com\n\theader.b=\"edjZJtwJ\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=protonmail.com;\n\ts=protonmail3; t=1736504907; x=1736764107;\n\tbh=tubEt98f4vE9d2uPsJNFmYEEZ1vP0EA3uotgXE8MheA=;\n\th=Date:To:From:Cc:Subject:Message-ID:In-Reply-To:References:\n\tFeedback-ID:From:To:Cc:Date:Subject:Reply-To:Feedback-ID:\n\tMessage-ID:BIMI-Selector:List-Unsubscribe:List-Unsubscribe-Post;\n\tb=edjZJtwJ/jouRcsMu0rtPSbL1Ir6UZC4EPOtTZBtBiq3GaH5vWaQ8I0WGE+0QF+Y/\n\t+LIf//3BNaTctxmkcSekh5Y2ZH+tNYIBHNVYysy3JAOh+bbaD5vYSdRW0ueE/inGNC\n\tfb5S8dfnJtqp9zIOfN1B3MnX3zAY22+IXAje2nrtcPzJMxiVZAim+50CQ3uiaiyzbo\n\tutmAOZU+ig5KyEz2gvu2kgBqvxnXyVa6D+fKdB8/PBRFsy95nX7qyyCpVTCRJJqLym\n\tUS1pgS9RkyVT91G7Qd48csrQ/j7lvpI8a9p0zqvfxknEgvkFTIDFAfie7GZX+3ynV1\n\tdtFb4Jc5qxfkA==","Date":"Fri, 10 Jan 2025 10:28:22 +0000","To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","From":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= <pobrn@protonmail.com>","Cc":"Jacopo Mondi <jacopo.mondi@ideasonboard.com>,\n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [RFC PATCH v1 08/12] apps: lc-compliance: Remove libevent\n\tdependency","Message-ID":"<O2RtBLpth6uWwYQQ18jgO7wHmNeKzGUhYFqrT83JUFB_szJxR2AjNvZ3GpcjmX9SkGXoIHTqg0AR1Lkjd-RfnXfVJkxKsExGiryj2DWcqUU=@protonmail.com>","In-Reply-To":"<20250110003445.GL6159@pendragon.ideasonboard.com>","References":"<20241220150759.709756-1-pobrn@protonmail.com>\n\t<20241220150759.709756-9-pobrn@protonmail.com>\n\t<qex6tsqykhbg44taaxlmq2ns5zzipp7hpnwy4kypnbzvpk6qbc@cdqmiac5b7xa>\n\t<20250110003445.GL6159@pendragon.ideasonboard.com>","Feedback-ID":"20568564:user:proton","X-Pm-Message-ID":"c72e6694ebe929dbc56edba568f49a7d84cb3dbc","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Transfer-Encoding":"quoted-printable","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":33015,"web_url":"https://patchwork.libcamera.org/comment/33015/","msgid":"<aCtdd44ymF7p8qi9FxM-W62S3ZdD5ZmMtaScnHnwaXjjV-45fjr1CWNZXalQeyU2LgGBfxvkyo5s9ofbo-3W5RM8rrHo4p0J3tQJSLYfd2Y=@protonmail.com>","date":"2025-01-10T10:40:52","subject":"Re: [RFC PATCH v1 08/12] apps: lc-compliance: Remove libevent\n\tdependency","submitter":{"id":133,"url":"https://patchwork.libcamera.org/api/people/133/","name":"Pőcze Barnabás","email":"pobrn@protonmail.com"},"content":"Hi\n\n\n2025. január 7., kedd 18:06 keltezéssel, Jacopo Mondi <jacopo.mondi@ideasonboard.com> írta:\n\n> Hi Barnabás\n> \n> On Fri, Dec 20, 2024 at 03:08:37PM +0000, Barnabás Pőcze wrote:\n> > libevent is only used as a way to notify the main thread from\n> > the CameraManager thread when the capture should be terminated.\n> >\n> > Not only is libevent not really necessary and instead can be\n> > replaced with a simple combination of C++ STL parts, the current\n> > usage is prone to race conditions.\n> >\n> > Specifically, the camera's `queueRequest()` might complete the\n> > request before the main thread could set the `loop_` member\n> > variable and synchronize with the thread. This is a data race,\n> > practically resulting in a nullptr or dangling pointer dereference.\n> >\n> > This can easily be triggered by inserting a sufficiently large\n> > timeout before `loop_ = new EventLoop;`:\n> \n> To be frank this is kind of hard to trigger, as it can only happens\n> if the main thread does not execute for longer than the frame\n> interval. But yes, who knows, if there is a chance for this to happen,\n> it will happen sooner or later, so it's indeed worth addressing it.\n> \n> >\n> >   [46:43:40.529620351] [671833] DEBUG Request request.cpp:129 Request(4:C:0/1:0)\n> >   ../src/apps/lc-compliance/helpers/capture.cpp:140:14: runtime error: member call on null pointer of type 'struct EventLoop'\n> >     0 0x632c3e82f81a in CaptureBalanced::requestComplete(libcamera::Request*) ../src/apps/lc-compliance/helpers/capture.cpp:140\n> >\n> > Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com>\n> > ---\n> >  README.rst                                 |  2 +-\n> >  src/apps/lc-compliance/helpers/capture.cpp | 23 ++++++------\n> >  src/apps/lc-compliance/helpers/capture.h   | 42 +++++++++++++++++++---\n> >  src/apps/lc-compliance/meson.build         |  7 ++--\n> >  src/apps/meson.build                       |  5 ---\n> >  5 files changed, 53 insertions(+), 26 deletions(-)\n> >\n> > diff --git a/README.rst b/README.rst\n> > index 4068c6cc8..f1749be97 100644\n> > --- a/README.rst\n> > +++ b/README.rst\n> > @@ -97,7 +97,7 @@ for Python bindings: [optional]\n> >          pybind11-dev\n> >\n> >  for lc-compliance: [optional]\n> > -        libevent-dev libgtest-dev\n> > +        libgtest-dev\n> >\n> >  for abi-compat.sh: [optional]\n> >          abi-compliance-checker\n> > diff --git a/src/apps/lc-compliance/helpers/capture.cpp b/src/apps/lc-compliance/helpers/capture.cpp\n> > index 91c4d4400..b473e0773 100644\n> > --- a/src/apps/lc-compliance/helpers/capture.cpp\n> > +++ b/src/apps/lc-compliance/helpers/capture.cpp\n> > @@ -12,7 +12,7 @@\n> >  using namespace libcamera;\n> >\n> >  Capture::Capture(std::shared_ptr<Camera> camera)\n> > -\t: loop_(nullptr), camera_(std::move(camera)),\n> > +\t: camera_(std::move(camera)),\n> >  \t  allocator_(camera_)\n> \n> This now fits on the previous line\n> \n> >  {\n> >  }\n> > @@ -52,6 +52,8 @@ void Capture::start()\n> >\n> >  \tcamera_->requestCompleted.connect(this, &Capture::requestComplete);\n> >\n> > +\tresult_.reset();\n> > +\n> >  \tASSERT_EQ(camera_->start(), 0) << \"Failed to start camera\";\n> >  }\n> >\n> > @@ -62,6 +64,8 @@ void Capture::stop()\n> >\n> >  \tcamera_->stop();\n> >\n> > +\tresult_.reset();\n> > +\n> \n> I don't think you could have a stop-stop or start-start sequence, so\n> probably a reset in start() is all you need. However this doesn't hurt\n> \n> >  \tcamera_->requestCompleted.disconnect(this);\n> >\n> >  \tStream *stream = config_->at(0).stream();\n> > @@ -108,11 +112,10 @@ void CaptureBalanced::capture(unsigned int numRequests)\n> >  \t}\n> >\n> >  \t/* Run capture session. */\n> > -\tloop_ = new EventLoop();\n> > -\tloop_->exec();\n> > +\tint status = result_.wait();\n> >  \tstop();\n> > -\tdelete loop_;\n> >\n> > +\tASSERT_EQ(status, 0);\n> >  \tASSERT_EQ(captureCount_, captureLimit_);\n> >  }\n> >\n> > @@ -132,13 +135,13 @@ void CaptureBalanced::requestComplete(Request *request)\n> >\n> >  \tcaptureCount_++;\n> >  \tif (captureCount_ >= captureLimit_) {\n> > -\t\tloop_->exit(0);\n> > +\t\tresult_.set(0);\n> >  \t\treturn;\n> >  \t}\n> >\n> >  \trequest->reuse(Request::ReuseBuffers);\n> >  \tif (queueRequest(request))\n> > -\t\tloop_->exit(-EINVAL);\n> > +\t\tresult_.set(-EINVAL);\n> >  }\n> >\n> >  /* CaptureUnbalanced */\n> > @@ -171,10 +174,8 @@ void CaptureUnbalanced::capture(unsigned int numRequests)\n> >  \t}\n> >\n> >  \t/* Run capture session. */\n> > -\tloop_ = new EventLoop();\n> > -\tint status = loop_->exec();\n> > +\tint status = result_.wait();\n> >  \tstop();\n> > -\tdelete loop_;\n> >\n> >  \tASSERT_EQ(status, 0);\n> >  }\n> > @@ -183,7 +184,7 @@ void CaptureUnbalanced::requestComplete(Request *request)\n> >  {\n> >  \tcaptureCount_++;\n> >  \tif (captureCount_ >= captureLimit_) {\n> > -\t\tloop_->exit(0);\n> > +\t\tresult_.set(0);\n> >  \t\treturn;\n> >  \t}\n> >\n> > @@ -192,5 +193,5 @@ void CaptureUnbalanced::requestComplete(Request *request)\n> >\n> >  \trequest->reuse(Request::ReuseBuffers);\n> >  \tif (camera_->queueRequest(request))\n> > -\t\tloop_->exit(-EINVAL);\n> > +\t\tresult_.set(-EINVAL);\n> >  }\n> > diff --git a/src/apps/lc-compliance/helpers/capture.h b/src/apps/lc-compliance/helpers/capture.h\n> > index a4cc3a99e..8582ade8e 100644\n> > --- a/src/apps/lc-compliance/helpers/capture.h\n> > +++ b/src/apps/lc-compliance/helpers/capture.h\n> > @@ -7,12 +7,13 @@\n> >\n> >  #pragma once\n> >\n> > +#include <condition_variable>\n> >  #include <memory>\n> > +#include <mutex>\n> > +#include <optional>\n> >\n> >  #include <libcamera/libcamera.h>\n> >\n> > -#include \"../common/event_loop.h\"\n> > -\n> >  class Capture\n> >  {\n> >  public:\n> > @@ -27,12 +28,45 @@ protected:\n> >\n> >  \tvirtual void requestComplete(libcamera::Request *request) = 0;\n> >\n> > -\tEventLoop *loop_;\n> > -\n> >  \tstd::shared_ptr<libcamera::Camera> camera_;\n> >  \tlibcamera::FrameBufferAllocator allocator_;\n> >  \tstd::unique_ptr<libcamera::CameraConfiguration> config_;\n> >  \tstd::vector<std::unique_ptr<libcamera::Request>> requests_;\n> > +\n> > +\tstruct\n> > +\t{\n> \n> nit: We usually do\n> \n>         struct {\n> \n> > +\tprivate:\n> \n> With private: after public:\n> \n> > +\t\tstd::mutex mutex_;\n> > +\t\tstd::condition_variable cv_;\n> > +\t\tstd::optional<int> value_;\n> > +\n> > +\tpublic:\n> > +\t\tint wait()\n> > +\t\t{\n> > +\t\t\tstd::unique_lock guard(mutex_);\n> > +\n> > +\t\t\tcv_.wait(guard, [&] {\n> \n> Do you need to capture the whole env or &value is enough ?\n\n`value` is enough, but I'm just used to using `[&]`, especially in lambdas\nthat don't escape their scope of creation.\n\n\n> \n> > +\t\t\t\treturn value_.has_value();\n> > +\t\t\t});\n> > +\n> > +\t\t\treturn *value_;\n> > +\t\t}\n> > +\n> > +\t\tvoid set(int value)\n> > +\t\t{\n> > +\t\t\tstd::unique_lock guard(mutex_);\n> > +\n> > +\t\t\tif (!value_)\n> \n> Why do you think this can't be overriden ?\n\nThe question here is which value should be stored in multiple `set()` calls\nhappen. I opted for the earliest but it could easily be the latest.\n\n\n> \n> > +\t\t\t\tvalue_ = value;\n> > +\n> > +\t\t\tcv_.notify_all();\n> > +\t\t}\n> > +\n> > +\t\tvoid reset()\n> > +\t\t{\n> > +\t\t\tvalue_.reset();\n> > +\t\t}\n> > +\t} result_;\n> >  };\n> >\n> >  class CaptureBalanced : public Capture\n> > diff --git a/src/apps/lc-compliance/meson.build b/src/apps/lc-compliance/meson.build\n> > index b1f605f33..0884bc6ca 100644\n> > --- a/src/apps/lc-compliance/meson.build\n> > +++ b/src/apps/lc-compliance/meson.build\n> > @@ -4,13 +4,11 @@ libgtest = dependency('gtest', version : '>=1.10.0',\n> >                        required : get_option('lc-compliance'),\n> >                        fallback : ['gtest', 'gtest_dep'])\n> >\n> > -if opt_lc_compliance.disabled() or not libevent.found() or not libgtest.found()\n> > -    lc_compliance_enabled = false\n> > +lc_compliance_enabled = opt_lc_compliance.allowed() and libgtest.found()\n> > +if not lc_compliance_enabled\n> >      subdir_done()\n> >  endif\n> >\n> > -lc_compliance_enabled = true\n> > -\n> >  lc_compliance_sources = files([\n> >      'environment.cpp',\n> >      'helpers/capture.cpp',\n> > @@ -29,7 +27,6 @@ lc_compliance  = executable('lc-compliance', lc_compliance_sources,\n> >                              dependencies : [\n> >                                  libatomic,\n> >                                  libcamera_public,\n> > -                                libevent,\n> >                                  libgtest,\n> >                              ],\n> >                              include_directories : lc_compliance_includes,\n> > diff --git a/src/apps/meson.build b/src/apps/meson.build\n> > index af632b9a7..252491441 100644\n> > --- a/src/apps/meson.build\n> > +++ b/src/apps/meson.build\n> > @@ -3,12 +3,7 @@\n> >  opt_cam = get_option('cam')\n> >  opt_lc_compliance = get_option('lc-compliance')\n> >\n> > -# libevent is needed by cam and lc-compliance. As they are both feature options,\n> > -# they can't be combined with simple boolean logic.\n> >  libevent = dependency('libevent_pthreads', required : opt_cam)\n> > -if not libevent.found()\n> > -    libevent = dependency('libevent_pthreads', required : opt_lc_compliance)\n> > -endif\n> \n> Do you think the EventLoop abstraction is needed at all for the other\n> apps ?\n\nIt certainly seems to be needed for other parts.\n\n\nRegards,\nBarnabás Pőcze\n\n\n> \n> Nice rework overall\n> Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>\n> \n> Thanks\n>   j\n> \n> \n> >\n> >  libtiff = dependency('libtiff-4', required : false)\n> >\n> > --\n> > 2.47.1\n> >\n> >\n>","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 492B5C32EF\n\tfor <parsemail@patchwork.libcamera.org>;\n\tFri, 10 Jan 2025 10:41:00 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0B61261882;\n\tFri, 10 Jan 2025 11:40:59 +0100 (CET)","from mail-4322.protonmail.ch (mail-4322.protonmail.ch\n\t[185.70.43.22])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id CD45461882\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri, 10 Jan 2025 11:40:56 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=protonmail.com header.i=@protonmail.com\n\theader.b=\"YyjZ6rAH\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=protonmail.com;\n\ts=protonmail3; t=1736505655; x=1736764855;\n\tbh=4YCmJUaUs/g6+FLnSZxyY33yZMUyZ9teDJvCPFn6wyo=;\n\th=Date:To:From:Cc:Subject:Message-ID:In-Reply-To:References:\n\tFeedback-ID:From:To:Cc:Date:Subject:Reply-To:Feedback-ID:\n\tMessage-ID:BIMI-Selector:List-Unsubscribe:List-Unsubscribe-Post;\n\tb=YyjZ6rAHMv0FYoGDlI+jDP01Ldkkf/0/i3RqHsd3pkxRYA94DOcMUtUOu+ZtQMdMQ\n\tsiLwN7SA1muuXKpQDeGtfAoUNrOLNNoVmX0fVB6mNIfrsYI2jxdVwdwLbbQid9/PNT\n\t/JJXM6UxDtR+Uc9KSKsiC67sRPPfG0DfSBCk6L6DaSxw2p8SkjgwX/yn7sb2SZdW/S\n\tS1T9x/fjGMnihtP+3KSc2epwbc6ulczBbwbsghUM9ukHWFlmbbM6t1rCf7fZQkjRlB\n\tHqlNQupP3j3s/aB+Uz8Wj7VuXpJxaFjclvOhthiWZf60tFfXWUDUNYR0XTEXWH2hpK\n\tb1Aaz0R9BHZrw==","Date":"Fri, 10 Jan 2025 10:40:52 +0000","To":"Jacopo Mondi <jacopo.mondi@ideasonboard.com>","From":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= <pobrn@protonmail.com>","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [RFC PATCH v1 08/12] apps: lc-compliance: Remove libevent\n\tdependency","Message-ID":"<aCtdd44ymF7p8qi9FxM-W62S3ZdD5ZmMtaScnHnwaXjjV-45fjr1CWNZXalQeyU2LgGBfxvkyo5s9ofbo-3W5RM8rrHo4p0J3tQJSLYfd2Y=@protonmail.com>","In-Reply-To":"<qex6tsqykhbg44taaxlmq2ns5zzipp7hpnwy4kypnbzvpk6qbc@cdqmiac5b7xa>","References":"<20241220150759.709756-1-pobrn@protonmail.com>\n\t<20241220150759.709756-9-pobrn@protonmail.com>\n\t<qex6tsqykhbg44taaxlmq2ns5zzipp7hpnwy4kypnbzvpk6qbc@cdqmiac5b7xa>","Feedback-ID":"20568564:user:proton","X-Pm-Message-ID":"e399146028ed2fbc1229f6f02cf8986f038239db","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Transfer-Encoding":"quoted-printable","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":33016,"web_url":"https://patchwork.libcamera.org/comment/33016/","msgid":"<e_makuRYDfqmC-a2BL9xEfhSDito_1n_hT2nECpP-yiTK1u46paybFmfqM3vxnyIM8c3prNI0J2vDEhrqjYrfVQBptQGttFEmHzrS4F2bMs=@protonmail.com>","date":"2025-01-10T10:44:07","subject":"Re: [RFC PATCH v1 08/12] apps: lc-compliance: Remove libevent\n\tdependency","submitter":{"id":133,"url":"https://patchwork.libcamera.org/api/people/133/","name":"Pőcze Barnabás","email":"pobrn@protonmail.com"},"content":"2025. január 10., péntek 11:28 keltezéssel, Barnabás Pőcze <pobrn@protonmail.com> írta:\n\n> 2025. január 10., péntek 1:34 keltezéssel, Laurent Pinchart <laurent.pinchart@ideasonboard.com> írta:\n> \n> > On Tue, Jan 07, 2025 at 06:06:04PM +0100, Jacopo Mondi wrote:\n> > > On Fri, Dec 20, 2024 at 03:08:37PM +0000, Barnabás Pőcze wrote:\n> > > > libevent is only used as a way to notify the main thread from\n> > > > the CameraManager thread when the capture should be terminated.\n> >\n> > True, but will that always be the case ? The current tests are fairly\n> > simple, but what will happen when we'll have more complex tests that\n> > will need to perform multiple actions through a capture session ? I'm\n> > sure we could still handle that manually with condition variables, but I\n> > think that will become complex, and it feels like reinventing the wheel.\n> >\n> \n> The current libevent usage is not quite correct, so it needs to be fixed in any case.\n> This patch implements probably the simplest option that works for the current\n> requirements. Alternatively, I believe manually triggered events of libevent could\n> be used to achieve the same, but that would easily be more code and touch more\n> components (e.g. `event_loop.{cpp,h}`). But I'm not trying to suggest that libevent\n> should not be used anymore. It could be reintroduced in the future easily in my opinion.\n> So which one should it be?\n\nIt would appear that `EventLoop::callLater()` might be an option as well.\n\n\n> \n> \n> Regards,\n> Barnabás Pőcze\n> \n> \n> > > >\n> > > > Not only is libevent not really necessary and instead can be\n> > > > replaced with a simple combination of C++ STL parts, the current\n> > > > usage is prone to race conditions.\n> > > >\n> > > > Specifically, the camera's `queueRequest()` might complete the\n> > > > request before the main thread could set the `loop_` member\n> > > > variable and synchronize with the thread. This is a data race,\n> > > > practically resulting in a nullptr or dangling pointer dereference.\n> > > >\n> > > > This can easily be triggered by inserting a sufficiently large\n> > > > timeout before `loop_ = new EventLoop;`:\n> > >\n> > > To be frank this is kind of hard to trigger, as it can only happens\n> > > if the main thread does not execute for longer than the frame\n> > > interval. But yes, who knows, if there is a chance for this to happen,\n> > > it will happen sooner or later, so it's indeed worth addressing it.\n> > >\n> > > >\n> > > >   [46:43:40.529620351] [671833] DEBUG Request request.cpp:129 Request(4:C:0/1:0)\n> > > >   ../src/apps/lc-compliance/helpers/capture.cpp:140:14: runtime error: member call on null pointer of type 'struct EventLoop'\n> > > >     0 0x632c3e82f81a in CaptureBalanced::requestComplete(libcamera::Request*) ../src/apps/lc-compliance/helpers/capture.cpp:140\n> > > >\n> > > > Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com>\n> > > > ---\n> > > > [...]","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 3C927C32EF\n\tfor <parsemail@patchwork.libcamera.org>;\n\tFri, 10 Jan 2025 10:44:20 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4D70B684EA;\n\tFri, 10 Jan 2025 11:44:19 +0100 (CET)","from mail-4316.protonmail.ch (mail-4316.protonmail.ch\n\t[185.70.43.16])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B1BB661882\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri, 10 Jan 2025 11:44:16 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=protonmail.com header.i=@protonmail.com\n\theader.b=\"wWHGkfmD\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=protonmail.com;\n\ts=protonmail3; t=1736505853; x=1736765053;\n\tbh=alyd1RbUJ792OHy3qgd3a6xCf+9a91OV8hP0X5YNAzA=;\n\th=Date:To:From:Cc:Subject:Message-ID:In-Reply-To:References:\n\tFeedback-ID:From:To:Cc:Date:Subject:Reply-To:Feedback-ID:\n\tMessage-ID:BIMI-Selector:List-Unsubscribe:List-Unsubscribe-Post;\n\tb=wWHGkfmD28OgR1SXnuDbd7mCugox5lJ+L9W40i8umdsdoPuYpT4uuZjplI3vjdSRv\n\tf46r3ZRk6dPNxE+zAp16ebXZJHDXajGE72HCGFWtkMXdLamb1Dr0A3suRND3ZLmrlq\n\tsIQLqegztnYghh11U1qlXpgnB6Q4dSz6MjXqK4vW+J3/TT4afv+zRizVogpNOPsgLm\n\tkeolCQOxY1GTlK743lA4+05+xU/t8cCzjrHrZKDLX8iEBEypn07d2ntyD2EDOLPCwQ\n\tPd69rZA2Bunf0cTpPiVcZdkrZ2cHEVfSzKK9YuFLQeQA6dCGhIuNBay81+VIg2kXQR\n\tPZoF9PvKLhZpg==","Date":"Fri, 10 Jan 2025 10:44:07 +0000","To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","From":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= <pobrn@protonmail.com>","Cc":"Jacopo Mondi <jacopo.mondi@ideasonboard.com>,\n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [RFC PATCH v1 08/12] apps: lc-compliance: Remove libevent\n\tdependency","Message-ID":"<e_makuRYDfqmC-a2BL9xEfhSDito_1n_hT2nECpP-yiTK1u46paybFmfqM3vxnyIM8c3prNI0J2vDEhrqjYrfVQBptQGttFEmHzrS4F2bMs=@protonmail.com>","In-Reply-To":"<O2RtBLpth6uWwYQQ18jgO7wHmNeKzGUhYFqrT83JUFB_szJxR2AjNvZ3GpcjmX9SkGXoIHTqg0AR1Lkjd-RfnXfVJkxKsExGiryj2DWcqUU=@protonmail.com>","References":"<20241220150759.709756-1-pobrn@protonmail.com>\n\t<20241220150759.709756-9-pobrn@protonmail.com>\n\t<qex6tsqykhbg44taaxlmq2ns5zzipp7hpnwy4kypnbzvpk6qbc@cdqmiac5b7xa>\n\t<20250110003445.GL6159@pendragon.ideasonboard.com>\n\t<O2RtBLpth6uWwYQQ18jgO7wHmNeKzGUhYFqrT83JUFB_szJxR2AjNvZ3GpcjmX9SkGXoIHTqg0AR1Lkjd-RfnXfVJkxKsExGiryj2DWcqUU=@protonmail.com>","Feedback-ID":"20568564:user:proton","X-Pm-Message-ID":"7740fbcc7b6932490fe5d784b37ecac08b4b4e8f","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Transfer-Encoding":"quoted-printable","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":33020,"web_url":"https://patchwork.libcamera.org/comment/33020/","msgid":"<20250110143034.GD7733@pendragon.ideasonboard.com>","date":"2025-01-10T14:30:34","subject":"Re: [RFC PATCH v1 08/12] apps: lc-compliance: Remove libevent\n\tdependency","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Fri, Jan 10, 2025 at 10:44:07AM +0000, Barnabás Pőcze wrote:\n> 2025. január 10., péntek 11:28 keltezéssel, Barnabás Pőcze írta:\n> > 2025. január 10., péntek 1:34 keltezéssel, Laurent Pinchart írta:\n> > > On Tue, Jan 07, 2025 at 06:06:04PM +0100, Jacopo Mondi wrote:\n> > > > On Fri, Dec 20, 2024 at 03:08:37PM +0000, Barnabás Pőcze wrote:\n> > > > > libevent is only used as a way to notify the main thread from\n> > > > > the CameraManager thread when the capture should be terminated.\n> > >\n> > > True, but will that always be the case ? The current tests are fairly\n> > > simple, but what will happen when we'll have more complex tests that\n> > > will need to perform multiple actions through a capture session ? I'm\n> > > sure we could still handle that manually with condition variables, but I\n> > > think that will become complex, and it feels like reinventing the wheel.\n> > \n> > The current libevent usage is not quite correct, so it needs to be fixed in any case.\n> > This patch implements probably the simplest option that works for the current\n> > requirements. Alternatively, I believe manually triggered events of libevent could\n> > be used to achieve the same, but that would easily be more code and touch more\n> > components (e.g. `event_loop.{cpp,h}`). But I'm not trying to suggest that libevent\n> > should not be used anymore. It could be reintroduced in the future easily in my opinion.\n> > So which one should it be?\n> \n> It would appear that `EventLoop::callLater()` might be an option as well.\n\nI suppose the issue isn't just that loop_ could be null when the request\ncompletion handler calls loop_->exit(), but also the fact that exit()\ncould be called before exec(). Using callLater() as done in the cam\napplication should work I think, it the fix should then be simple and\nnot too intrusive. Unless I'm missing something :-) Could you try it out\n?\n\n> > > > > Not only is libevent not really necessary and instead can be\n> > > > > replaced with a simple combination of C++ STL parts, the current\n> > > > > usage is prone to race conditions.\n> > > > >\n> > > > > Specifically, the camera's `queueRequest()` might complete the\n> > > > > request before the main thread could set the `loop_` member\n> > > > > variable and synchronize with the thread. This is a data race,\n> > > > > practically resulting in a nullptr or dangling pointer dereference.\n> > > > >\n> > > > > This can easily be triggered by inserting a sufficiently large\n> > > > > timeout before `loop_ = new EventLoop;`:\n> > > >\n> > > > To be frank this is kind of hard to trigger, as it can only happens\n> > > > if the main thread does not execute for longer than the frame\n> > > > interval. But yes, who knows, if there is a chance for this to happen,\n> > > > it will happen sooner or later, so it's indeed worth addressing it.\n> > > >\n> > > > >   [46:43:40.529620351] [671833] DEBUG Request request.cpp:129 Request(4:C:0/1:0)\n> > > > >   ../src/apps/lc-compliance/helpers/capture.cpp:140:14: runtime error: member call on null pointer of type 'struct EventLoop'\n> > > > >     0 0x632c3e82f81a in CaptureBalanced::requestComplete(libcamera::Request*) ../src/apps/lc-compliance/helpers/capture.cpp:140\n> > > > >\n> > > > > Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com>\n> > > > > ---\n> > > > > [...]","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 76648C32EF\n\tfor <parsemail@patchwork.libcamera.org>;\n\tFri, 10 Jan 2025 14:30:41 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2B48E684E7;\n\tFri, 10 Jan 2025 15:30:41 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 7B3B6608AA\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri, 10 Jan 2025 15:30:39 +0100 (CET)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id B687D9FC;\n\tFri, 10 Jan 2025 15:29:44 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"Q6Ge8plR\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1736519384;\n\tbh=lRi4iVDKllMIbmnu8oAvUWkQDwHjJzXRFb+1WdkSZUw=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Q6Ge8plRkPwwchqM7Bnn9/i9dugp4FYybZhjP3CgSQ5scnchyIXKYfSWYeFVFTyIC\n\tGPUGTBtU9ojyM+MRgCWj39hLPUWfAQp9kn33IrFpDGoZqat3mJSkcaa7gbZC98EtSR\n\tGVJLOZQMkwBqUOvQ0QcMF7pYYGnMgJ1FEXs1I5Sk=","Date":"Fri, 10 Jan 2025 16:30:34 +0200","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= <pobrn@protonmail.com>","Cc":"Jacopo Mondi <jacopo.mondi@ideasonboard.com>,\n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [RFC PATCH v1 08/12] apps: lc-compliance: Remove libevent\n\tdependency","Message-ID":"<20250110143034.GD7733@pendragon.ideasonboard.com>","References":"<20241220150759.709756-1-pobrn@protonmail.com>\n\t<20241220150759.709756-9-pobrn@protonmail.com>\n\t<qex6tsqykhbg44taaxlmq2ns5zzipp7hpnwy4kypnbzvpk6qbc@cdqmiac5b7xa>\n\t<20250110003445.GL6159@pendragon.ideasonboard.com>\n\t<O2RtBLpth6uWwYQQ18jgO7wHmNeKzGUhYFqrT83JUFB_szJxR2AjNvZ3GpcjmX9SkGXoIHTqg0AR1Lkjd-RfnXfVJkxKsExGiryj2DWcqUU=@protonmail.com>\n\t<e_makuRYDfqmC-a2BL9xEfhSDito_1n_hT2nECpP-yiTK1u46paybFmfqM3vxnyIM8c3prNI0J2vDEhrqjYrfVQBptQGttFEmHzrS4F2bMs=@protonmail.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<e_makuRYDfqmC-a2BL9xEfhSDito_1n_hT2nECpP-yiTK1u46paybFmfqM3vxnyIM8c3prNI0J2vDEhrqjYrfVQBptQGttFEmHzrS4F2bMs=@protonmail.com>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}}]