| Message ID | 20250303142010.714280-1-barnabas.pocze@ideasonboard.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
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@chromium.org> BR, Harvey On Mon, Mar 3, 2025 at 10:20 PM Barnabás Pőcze <barnabas.pocze@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@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 >
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@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 >
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@chromium.org> > > BR, > Harvey > > On Mon, Mar 3, 2025 at 10:20 PM Barnabás Pőcze > <barnabas.pocze@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@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 >>
Hi 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@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. > >> 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. Regards, Barnabás Pőcze > >> } >> >> /** >> @@ -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 >>
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@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@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)); > >> } > >> > >> /**
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)); } /**
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> --- src/libcamera/camera_manager.cpp | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-)