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

9633 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>
---
 src/libcamera/camera_manager.cpp   | 2 ++
 src/libcamera/pipeline_handler.cpp | 3 ---
 2 files changed, 2 insertions(+), 3 deletions(-)

Hi Paul,

Thank you for the patch.

On Tue, Sep 15, 2020 at 11:20:38PM +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.

This is an important issue, that should be kept in mind when adding any
global variable (especially global variables that end up running
constructors). Could I convince you to document the problem in
Documentation/coding-style.rst ? :-) Maybe in a new section after
"Object Ownership". It should summarize the problem as we have
previously discussed on privately (the order of global initializations
*and* destructions can't be reasonably controlled, thus causing
potential issues when different global variables depend on each other),
and the solution (avoid global variables when possible, a good example
is patch 01/23 in this series, and when required, such as when
implementing factories with auto-registration, avoid dependencies by
running the minimum amount of code in the constructor and destructor,
and moving code that contains dependencies to a later point of time, as
done in this patch).

> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> ---
>  src/libcamera/camera_manager.cpp   | 2 ++
>  src/libcamera/pipeline_handler.cpp | 3 ---
>  2 files changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp
> index b8d3ccc8..e5af271d 100644
> --- a/src/libcamera/camera_manager.cpp
> +++ b/src/libcamera/camera_manager.cpp
> @@ -142,6 +142,8 @@ void CameraManager::Private::createPipelineHandlers()
>  		PipelineHandlerFactory::factories();
>  
>  	for (PipelineHandlerFactory *factory : factories) {
> +		LOG(Camera, Debug)
> +			<< "Found registered pipeline handler \"" << factory->name() << "\"";

With the line wrapped

			<< "Found registered pipeline handler \""
			<< factory->name() << "\"";

and a blank line added here,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

You may also want to replace \" with '.

>  		/*
>  		 * 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() << "\"";
>  }
>  
>  /**