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

[Barnabás Pőcze]

Hi


2025. 03. 03. 15:31 keltezéssel, Cheng-Hao Yang írta:
> 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.

They are not necessary, but I thought I'll make that change
if I am already here.


Regards,
Barnabás Pőcze

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