[libcamera-devel] [PATCH 1/1] fix: pipeline handlers: Stop exponential explosive calls to createPipelineHandlers

[Laurent Pinchart]

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 at flowerpot.me>
> > 
> > LGTM
> > 
> > Reviewed-by: Umang Jain <umang.jain at ideasonboard.com>
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart at 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()

-- 
Regards,

Laurent Pinchart