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

[Kieran Bingham]

Quoting Barnabás Pőcze (2025-03-03 14:20:10)
> 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.
> 

This all sounds quite reasonable to me, so only a stylistic comment
below...

> 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);
>  
> +{

Having the inner scope at the leftmost layer is my only niggle here. I
think I would suggest that the MutexLocker scope should be indented

>         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));

It feels a bit weird that we move the camera to the signal... It's a
shared pointer, so I guess it doesn't matter ... it's not actually
transferring ownership to the transient signal emitter ?

I guess this is because it's not used locally after this so it avoids a
copy of the shared_ptr? 

>  }
>  
>  /**
> @@ -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);
> +}
> +

Same thoughts on the indent and std::move usage below too.

>         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
>