[PATCH v1] libcamera: camera_manager: Do not emit signals while holding lock

[Cheng-Hao Yang]

Hi Barnabás,

Thank you for the patch. It makes a lot of sense to me, only that I'm
not sure if `std::move`s are necessary. It probably saves new
instances of shared_ptr though.

Reviewed-by: Harvey Yang <chenghaoyang at chromium.org>

BR,
Harvey

On Mon, Mar 3, 2025 at 10:20 PM Barnabás Pőcze
<barnabas.pocze at ideasonboard.com> wrote:
>
> Both `CameraManager::Private::{add,remove}Camera()` emit the
> `camera{Added,Removed}` signals, respectively, while holding the
> lock protecting the list of cameras.
>
> This is problematic because if a callback tries to call `cameras()`,
> then the same (non-recursive) lock would be locked again.
>
> Furthermore, there is no real need to hold the lock while user code
> is running, so release the lock as soon as possible.
>
> Signed-off-by: Barnabás Pőcze <barnabas.pocze at ideasonboard.com>
> ---
>  src/libcamera/camera_manager.cpp | 16 +++++++++-------
>  1 file changed, 9 insertions(+), 7 deletions(-)
>
> diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp
> index 87e6717ec..d728ac44a 100644
> --- a/src/libcamera/camera_manager.cpp
> +++ b/src/libcamera/camera_manager.cpp
> @@ -202,6 +202,7 @@ void CameraManager::Private::addCamera(std::shared_ptr<Camera> camera)
>  {
>         ASSERT(Thread::current() == this);
>
> +{
>         MutexLocker locker(mutex_);
>
>         for (const std::shared_ptr<Camera> &c : cameras_) {
> @@ -213,13 +214,12 @@ void CameraManager::Private::addCamera(std::shared_ptr<Camera> camera)
>                 }
>         }
>
> -       cameras_.push_back(std::move(camera));
> -
> -       unsigned int index = cameras_.size() - 1;
> +       cameras_.push_back(camera);
> +}
>
>         /* Report the addition to the public signal */
>         CameraManager *const o = LIBCAMERA_O_PTR();
> -       o->cameraAdded.emit(cameras_[index]);
> +       o->cameraAdded.emit(std::move(camera));
>  }
>
>  /**
> @@ -236,6 +236,7 @@ void CameraManager::Private::removeCamera(std::shared_ptr<Camera> camera)
>  {
>         ASSERT(Thread::current() == this);
>
> +{
>         MutexLocker locker(mutex_);
>
>         auto iter = std::find_if(cameras_.begin(), cameras_.end(),
> @@ -245,14 +246,15 @@ void CameraManager::Private::removeCamera(std::shared_ptr<Camera> camera)
>         if (iter == cameras_.end())
>                 return;
>
> +       cameras_.erase(iter);
> +}
> +
>         LOG(Camera, Debug)
>                 << "Unregistering camera '" << camera->id() << "'";
>
> -       cameras_.erase(iter);
> -
>         /* Report the removal to the public signal */
>         CameraManager *const o = LIBCAMERA_O_PTR();
> -       o->cameraRemoved.emit(camera);
> +       o->cameraRemoved.emit(std::move(camera));
>  }
>
>  /**
> --
> 2.48.1
>