| Message ID | 20230305230603.3697024-2-dev@flowerpot.me |
|---|---|
| State | Accepted |
| Commit | a146e05125fdac75b8ffb6a818e00a446cec65dd |
| Headers | show |
| Series |
|
| Related | show |
HI Sophie, Thank you for the patch On 3/6/23 4:36 AM, Sophie Friedrich via libcamera-devel wrote: > Currently the method `createPipelineHandlers` registered itself to the > `devicesAdded` signal at the end of each call. As the Signal object > supports multiple non-unique listeners connected to it, the former s/listeners/slots maybe ? > method would be called exponentially often with each new emitted event > on `devicesAdded` (i.e. with udev plugging in a new camera) indeed, quite a bug! > > By attaching the `createPipelineHandlers` function to `devicesAdded` during > the first call to it / init of the `CameraManager::Private` this effect > is prevented. Would repharase a bit to : Fix it by registering the createPipelineHandlers() slot to `devicesAdded` signal in CameraManager::Private::init() instead. This will prevent the slot getting registered multiple times to the `devicesAdded` signal. > > Signed-off-by: Sophie Friedrich <dev@flowerpot.me> LGTM Reviewed-by: Umang Jain <umang.jain@ideasonboard.com> > --- > src/libcamera/camera_manager.cpp | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp > index b1785f75..c1edefda 100644 > --- a/src/libcamera/camera_manager.cpp > +++ b/src/libcamera/camera_manager.cpp > @@ -131,6 +131,7 @@ int CameraManager::Private::init() > return -ENODEV; > > createPipelineHandlers(); > + enumerator_->devicesAdded.connect(this, &Private::createPipelineHandlers); > > return 0; > } > @@ -165,8 +166,6 @@ void CameraManager::Private::createPipelineHandlers() > << "\" matched"; > } > } > - > - enumerator_->devicesAdded.connect(this, &Private::createPipelineHandlers); > } > > void CameraManager::Private::cleanup()
Hello, On Mon, Mar 06, 2023 at 10:39:59AM +0530, Umang Jain via libcamera-devel wrote: > HI Sophie, > > Thank you for the patch > > On 3/6/23 4:36 AM, Sophie Friedrich via libcamera-devel wrote: > > Currently the method `createPipelineHandlers` registered itself to the We use "function" instead of "method" in libcamera, as the term "method" comes from Java, and the C++ standard uses "function". > > `devicesAdded` signal at the end of each call. As the Signal object > > supports multiple non-unique listeners connected to it, the former > > s/listeners/slots maybe ? > > > method would be called exponentially often with each new emitted event > > on `devicesAdded` (i.e. with udev plugging in a new camera) > > indeed, quite a bug! Oops, indeed. > > By attaching the `createPipelineHandlers` function to `devicesAdded` during > > the first call to it / init of the `CameraManager::Private` this effect > > is prevented. > > Would repharase a bit to : > > Fix it by registering the createPipelineHandlers() slot to s/registering/connecting/ > `devicesAdded` signal in > CameraManager::Private::init() instead. This will prevent the slot > getting registered s/registered/connected/ > multiple times to the `devicesAdded` signal. > > > Signed-off-by: Sophie Friedrich <dev@flowerpot.me> > > LGTM > > Reviewed-by: Umang Jain <umang.jain@ideasonboard.com> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> I'll apply the minor changes mentioned above locally, no need to submit a v2. > > --- > > src/libcamera/camera_manager.cpp | 3 +-- > > 1 file changed, 1 insertion(+), 2 deletions(-) > > > > diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp > > index b1785f75..c1edefda 100644 > > --- a/src/libcamera/camera_manager.cpp > > +++ b/src/libcamera/camera_manager.cpp > > @@ -131,6 +131,7 @@ int CameraManager::Private::init() > > return -ENODEV; > > > > createPipelineHandlers(); > > + enumerator_->devicesAdded.connect(this, &Private::createPipelineHandlers); > > > > return 0; > > } > > @@ -165,8 +166,6 @@ void CameraManager::Private::createPipelineHandlers() > > << "\" matched"; > > } > > } > > - > > - enumerator_->devicesAdded.connect(this, &Private::createPipelineHandlers); > > } > > > > void CameraManager::Private::cleanup()
Also, the commit title should be start with the "libcamera: camera_manager:" prefix: libcamera: camera_manager: Stop exponential explosive calls to createPipelineHandlers We also try to keep the title within a (soft) limit of 72 characters. The following could work: libcamera: camera_manager: Connect deviceAdded signal once only Would that be OK with you ? On Mon, Mar 06, 2023 at 12:45:14PM +0200, Laurent Pinchart wrote: > Hello, > > On Mon, Mar 06, 2023 at 10:39:59AM +0530, Umang Jain via libcamera-devel wrote: > > HI Sophie, > > > > Thank you for the patch > > > > On 3/6/23 4:36 AM, Sophie Friedrich via libcamera-devel wrote: > > > Currently the method `createPipelineHandlers` registered itself to the > > We use "function" instead of "method" in libcamera, as the term "method" > comes from Java, and the C++ standard uses "function". > > > > `devicesAdded` signal at the end of each call. As the Signal object > > > supports multiple non-unique listeners connected to it, the former > > > > s/listeners/slots maybe ? > > > > > method would be called exponentially often with each new emitted event > > > on `devicesAdded` (i.e. with udev plugging in a new camera) > > > > indeed, quite a bug! > > Oops, indeed. > > > > By attaching the `createPipelineHandlers` function to `devicesAdded` during > > > the first call to it / init of the `CameraManager::Private` this effect > > > is prevented. > > > > Would repharase a bit to : > > > > Fix it by registering the createPipelineHandlers() slot to > > s/registering/connecting/ > > > `devicesAdded` signal in > > CameraManager::Private::init() instead. This will prevent the slot > > getting registered > > s/registered/connected/ > > > multiple times to the `devicesAdded` signal. > > > > > Signed-off-by: Sophie Friedrich <dev@flowerpot.me> > > > > LGTM > > > > Reviewed-by: Umang Jain <umang.jain@ideasonboard.com> > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > I'll apply the minor changes mentioned above locally, no need to submit > a v2. > > > > --- > > > src/libcamera/camera_manager.cpp | 3 +-- > > > 1 file changed, 1 insertion(+), 2 deletions(-) > > > > > > diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp > > > index b1785f75..c1edefda 100644 > > > --- a/src/libcamera/camera_manager.cpp > > > +++ b/src/libcamera/camera_manager.cpp > > > @@ -131,6 +131,7 @@ int CameraManager::Private::init() > > > return -ENODEV; > > > > > > createPipelineHandlers(); > > > + enumerator_->devicesAdded.connect(this, &Private::createPipelineHandlers); > > > > > > return 0; > > > } > > > @@ -165,8 +166,6 @@ void CameraManager::Private::createPipelineHandlers() > > > << "\" matched"; > > > } > > > } > > > - > > > - enumerator_->devicesAdded.connect(this, &Private::createPipelineHandlers); > > > } > > > > > > void CameraManager::Private::cleanup()
diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp index b1785f75..c1edefda 100644 --- a/src/libcamera/camera_manager.cpp +++ b/src/libcamera/camera_manager.cpp @@ -131,6 +131,7 @@ int CameraManager::Private::init() return -ENODEV; createPipelineHandlers(); + enumerator_->devicesAdded.connect(this, &Private::createPipelineHandlers); return 0; } @@ -165,8 +166,6 @@ void CameraManager::Private::createPipelineHandlers() << "\" matched"; } } - - enumerator_->devicesAdded.connect(this, &Private::createPipelineHandlers); } void CameraManager::Private::cleanup()
Currently the method `createPipelineHandlers` registered itself to the `devicesAdded` signal at the end of each call. As the Signal object supports multiple non-unique listeners connected to it, the former method would be called exponentially often with each new emitted event on `devicesAdded` (i.e. with udev plugging in a new camera) By attaching the `createPipelineHandlers` function to `devicesAdded` during the first call to it / init of the `CameraManager::Private` this effect is prevented. Signed-off-by: Sophie Friedrich <dev@flowerpot.me> --- src/libcamera/camera_manager.cpp | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)