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

[Laurent Pinchart]

On Tue, Mar 04, 2025 at 02:34:51PM +0100, Barnabás Pőcze wrote:
> 2025. 03. 04. 11:17 keltezéssel, Kieran Bingham írta:
> > 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
> 
> Okay, I'll indent it; my thinking was to avoid the excessive diff that
> it causes; but I agree it does look odd.

With that fixed,

Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

> >>          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?
> 
> Yes, the `CameraManager` already has a reference to it in `cameras_`, so
> the local reference in `camera` can be transferred.
> 
> >>   }
> >>   
> >>   /**
> >> @@ -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));
> >>   }
> >>   
> >>   /**

-- 
Regards,

Laurent Pinchart