[libcamera-devel,03/38] libcamera: PipelineHandler: Move printing pipeline names to CameraManager

9708 diff mbox series

Since pipeline registration is done with declaring static factory
objects, there is a risk that pipeline factories will be constructed
before libcamera facilities are ready. For example, logging in the
constructor of a pipeline handler factory may cause a segfault if
threading isn't ready yet. Avoid this issue by moving printing the
registration of the pipeline handler to the camera manager.

Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

---
Cosmetic changes in v2
---
 src/libcamera/camera_manager.cpp   | 3 +++
 src/libcamera/pipeline_handler.cpp | 3 ---
 2 files changed, 3 insertions(+), 3 deletions(-)

Hi Paul,

On Tue, Sep 22, 2020 at 10:35:02PM +0900, Paul Elder wrote:
> Since pipeline registration is done with declaring static factory
> objects, there is a risk that pipeline factories will be constructed
> before libcamera facilities are ready. For example, logging in the
> constructor of a pipeline handler factory may cause a segfault if
> threading isn't ready yet. Avoid this issue by moving printing the
> registration of the pipeline handler to the camera manager.

Did you ever see this happening ?
>
> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>

Thanks
  j

>
> ---
> Cosmetic changes in v2
> ---
>  src/libcamera/camera_manager.cpp   | 3 +++
>  src/libcamera/pipeline_handler.cpp | 3 ---
>  2 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp
> index b8d3ccc8..756f5b2b 100644
> --- a/src/libcamera/camera_manager.cpp
> +++ b/src/libcamera/camera_manager.cpp
> @@ -142,6 +142,9 @@ void CameraManager::Private::createPipelineHandlers()
>  		PipelineHandlerFactory::factories();
>
>  	for (PipelineHandlerFactory *factory : factories) {
> +		LOG(Camera, Debug)
> +			<< "Found registered pipeline handler '"
> +			<< factory->name() << "'";
>  		/*
>  		 * Try each pipeline handler until it exhaust
>  		 * all pipelines it can provide.
> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
> index 918aea1e..894200ee 100644
> --- a/src/libcamera/pipeline_handler.cpp
> +++ b/src/libcamera/pipeline_handler.cpp
> @@ -689,9 +689,6 @@ void PipelineHandlerFactory::registerType(PipelineHandlerFactory *factory)
>  	std::vector<PipelineHandlerFactory *> &factories = PipelineHandlerFactory::factories();
>
>  	factories.push_back(factory);
> -
> -	LOG(Pipeline, Debug)
> -		<< "Registered pipeline handler \"" << factory->name() << "\"";
>  }
>
>  /**
> --
> 2.27.0
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

Hi Jacopo,

On Wed, Sep 23, 2020 at 12:41:15PM +0200, Jacopo Mondi wrote:
> Hi Paul,
> 
> On Tue, Sep 22, 2020 at 10:35:02PM +0900, Paul Elder wrote:
> > Since pipeline registration is done with declaring static factory
> > objects, there is a risk that pipeline factories will be constructed
> > before libcamera facilities are ready. For example, logging in the
> > constructor of a pipeline handler factory may cause a segfault if
> > threading isn't ready yet. Avoid this issue by moving printing the
> > registration of the pipeline handler to the camera manager.
> 
> Did you ever see this happening ?

Yes, this did actually happen...

> > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>


Thanks,

Paul

> >
> > ---
> > Cosmetic changes in v2
> > ---
> >  src/libcamera/camera_manager.cpp   | 3 +++
> >  src/libcamera/pipeline_handler.cpp | 3 ---
> >  2 files changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp
> > index b8d3ccc8..756f5b2b 100644
> > --- a/src/libcamera/camera_manager.cpp
> > +++ b/src/libcamera/camera_manager.cpp
> > @@ -142,6 +142,9 @@ void CameraManager::Private::createPipelineHandlers()
> >  		PipelineHandlerFactory::factories();
> >
> >  	for (PipelineHandlerFactory *factory : factories) {
> > +		LOG(Camera, Debug)
> > +			<< "Found registered pipeline handler '"
> > +			<< factory->name() << "'";
> >  		/*
> >  		 * Try each pipeline handler until it exhaust
> >  		 * all pipelines it can provide.
> > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
> > index 918aea1e..894200ee 100644
> > --- a/src/libcamera/pipeline_handler.cpp
> > +++ b/src/libcamera/pipeline_handler.cpp
> > @@ -689,9 +689,6 @@ void PipelineHandlerFactory::registerType(PipelineHandlerFactory *factory)
> >  	std::vector<PipelineHandlerFactory *> &factories = PipelineHandlerFactory::factories();
> >
> >  	factories.push_back(factory);
> > -
> > -	LOG(Pipeline, Debug)
> > -		<< "Registered pipeline handler \"" << factory->name() << "\"";
> >  }
> >
> >  /**
> > --
> > 2.27.0
> >
> > _______________________________________________
> > libcamera-devel mailing list
> > libcamera-devel@lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel