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

23000 diff mbox series

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@ideasonboard.com>
Reviewed-by: Harvey Yang <chenghaoyang@chromium.org>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
changes in v2:
  * indent scopes properly
  * drop `std::move()` calls


v1: https://patchwork.libcamera.org/patch/22905/
---
 src/libcamera/camera_manager.cpp | 44 +++++++++++++++++---------------
 1 file changed, 23 insertions(+), 21 deletions(-)

--
2.49.0

Quoting Barnabás Pőcze (2025-03-24 11:51:23)
> 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@ideasonboard.com>
> Reviewed-by: Harvey Yang <chenghaoyang@chromium.org>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
> changes in v2:
>   * indent scopes properly
>   * drop `std::move()` calls
> 
> 
> v1: https://patchwork.libcamera.org/patch/22905/
> ---
>  src/libcamera/camera_manager.cpp | 44 +++++++++++++++++---------------
>  1 file changed, 23 insertions(+), 21 deletions(-)
> 
> diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp
> index 87e6717ec..400109f12 100644
> --- a/src/libcamera/camera_manager.cpp
> +++ b/src/libcamera/camera_manager.cpp
> @@ -202,24 +202,24 @@ void CameraManager::Private::addCamera(std::shared_ptr<Camera> camera)
>  {
>         ASSERT(Thread::current() == this);
> 
> -       MutexLocker locker(mutex_);
> +       {
> +               MutexLocker locker(mutex_);

Thanks, the indents make patches harder to parse, but the code clearer
to read :-)



Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>



> 
> -       for (const std::shared_ptr<Camera> &c : cameras_) {
> -               if (c->id() == camera->id()) {
> -                       LOG(Camera, Fatal)
> -                               << "Trying to register a camera with a duplicated ID '"
> -                               << camera->id() << "'";
> -                       return;
> +               for (const std::shared_ptr<Camera> &c : cameras_) {
> +                       if (c->id() == camera->id()) {
> +                               LOG(Camera, Fatal)
> +                                       << "Trying to register a camera with a duplicated ID '"
> +                                       << camera->id() << "'";
> +                               return;
> +                       }
>                 }
> -       }
> 
> -       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(camera);
>  }
> 
>  /**
> @@ -236,20 +236,22 @@ void CameraManager::Private::removeCamera(std::shared_ptr<Camera> camera)
>  {
>         ASSERT(Thread::current() == this);
> 
> -       MutexLocker locker(mutex_);
> +       {
> +               MutexLocker locker(mutex_);
> 
> -       auto iter = std::find_if(cameras_.begin(), cameras_.end(),
> -                                [camera](std::shared_ptr<Camera> &c) {
> -                                        return c.get() == camera.get();
> -                                });
> -       if (iter == cameras_.end())
> -               return;
> +               auto iter = std::find_if(cameras_.begin(), cameras_.end(),
> +                                        [camera](std::shared_ptr<Camera> &c) {
> +                                                return c.get() == camera.get();
> +                                        });
> +               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);
> --
> 2.49.0