[PATCH v2 2/2] libcamera: camera_manager: Add environment variable to order pipelines match

[Jacopo Mondi]

Hi Julien

On Fri, Mar 08, 2024 at 12:00:56PM +0100, Julien Vuillaumier wrote:
> To match the enumerated media devices, each pipeline handler registered

s/pipeline handler registered/registered pipeline handler/

> is used in no specific order. It is a limitation when several pipelines
> can match the devices, and user has to select a specific pipeline.
>
> For this purpose, environment variable LIBCAMERA_PIPELINES_MATCH_LIST is
> created that gives the option to define an ordered list of pipelines

s/that gives/to give/

> to invoke during the match process.

Or just: "to match on."

>
> LIBCAMERA_PIPELINES_MATCH_LIST="<name1>[,<name2>[,<name3>...]]]"
>
> Example:
> LIBCAMERA_PIPELINES_MATCH_LIST="PipelineHandlerRkISP1,SimplePipelineHandler"
>
> Signed-off-by: Julien Vuillaumier <julien.vuillaumier at nxp.com>
> ---
>  Documentation/environment_variables.rst     |  5 ++
>  include/libcamera/internal/camera_manager.h |  1 +
>  src/libcamera/camera_manager.cpp            | 57 ++++++++++++++++-----
>  3 files changed, 51 insertions(+), 12 deletions(-)
>
> diff --git a/Documentation/environment_variables.rst b/Documentation/environment_variables.rst
> index a9b230bc..ea4da3c9 100644
> --- a/Documentation/environment_variables.rst
> +++ b/Documentation/environment_variables.rst
> @@ -37,6 +37,11 @@ LIBCAMERA_IPA_MODULE_PATH
>
>     Example value: ``${HOME}/.libcamera/lib:/opt/libcamera/vendor/lib``
>
> +LIBCAMERA_PIPELINES_MATCH_LIST
> +   Define an ordered list of pipeline names to be used to match the media devices in the system.

long line over 80 cols which could eaily be broken in 2 lines

> +
> +   Example value: ``PipelineHandlerRkISP1,SimplePipelineHandler``

These are the names of the PipelineHandlers classes, which is fine,
but maybe for users it is better to use something they can more easily
reference, like the meson option associated with the pipeline ?

This will require a map though...

> +
>  LIBCAMERA_RPI_CONFIG_FILE
>     Define a custom configuration file to use in the Raspberry Pi pipeline handler.
>
> diff --git a/include/libcamera/internal/camera_manager.h b/include/libcamera/internal/camera_manager.h
> index 33ebe069..c57e509a 100644
> --- a/include/libcamera/internal/camera_manager.h
> +++ b/include/libcamera/internal/camera_manager.h
> @@ -45,6 +45,7 @@ private:
>  	int init();
>  	void createPipelineHandlers();
>  	void cleanup() LIBCAMERA_TSA_EXCLUDES(mutex_);
> +	void pipelineFactoryMatch(const PipelineHandlerFactoryBase *factory);

Please move this one line up to match the function definition order in
the .cpp file

>
>  	/*
>  	 * This mutex protects
> diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp
> index 355f3ada..620be1c8 100644
> --- a/src/libcamera/camera_manager.cpp
> +++ b/src/libcamera/camera_manager.cpp
> @@ -99,13 +99,36 @@ int CameraManager::Private::init()
>
>  void CameraManager::Private::createPipelineHandlers()
>  {
> -	CameraManager *const o = LIBCAMERA_O_PTR();
> -
>  	/*
>  	 * \todo Try to read handlers and order from configuration
> -	 * file and only fallback on all handlers if there is no
> -	 * configuration file.
> +	 * file and only fallback on environment variable or all handlers, if
> +	 * there is no configuration file.
> +	 */
> +
> +	/*
> +	 * When a list of preferred pipelines is defined, iterate through the
> +	 * ordered list to match the devices enumerated.

s/devices enumerated/enumerated devices/

> +	 * Otherwise, device matching is done in no specific order with each
> +	 * registered pipeline handler.
>  	 */

What about moving this comment block in the below if (pipesList)
branch ?

> +	const char *pipesList =
> +		utils::secure_getenv("LIBCAMERA_PIPELINES_MATCH_LIST");
> +	if (pipesList) {
> +		for (const auto &pipeName : utils::split(pipesList, ",")) {
> +			const PipelineHandlerFactoryBase *factory;
> +			factory = PipelineHandlerFactoryBase::getFactoryByName(pipeName);
> +			if (!factory)
> +				continue;
> +
> +			LOG(Camera, Debug)
> +				<< "Found listed pipeline handler '"
> +				<< pipeName << "'";
> +			pipelineFactoryMatch(factory);
> +		}
> +
> +		return;
> +	}
> +
>  	const std::vector<PipelineHandlerFactoryBase *> &factories =
>  		PipelineHandlerFactoryBase::factories();
>
> @@ -117,15 +140,25 @@ void CameraManager::Private::createPipelineHandlers()
>  		 * Try each pipeline handler until it exhaust
>  		 * all pipelines it can provide.
>  		 */
> -		while (1) {
> -			std::shared_ptr<PipelineHandler> pipe = factory->create(o);
> -			if (!pipe->match(enumerator_.get()))
> -				break;
> +		pipelineFactoryMatch(factory);
> +	}
> +}
>
> -			LOG(Camera, Debug)
> -				<< "Pipeline handler \"" << factory->name()
> -				<< "\" matched";
> -		}
> +void CameraManager::Private::pipelineFactoryMatch(const PipelineHandlerFactoryBase *factory)
> +{
> +	CameraManager *const o = LIBCAMERA_O_PTR();
> +
> +	/*
> +	 * Provide as many matching pipelines as possible
> +	 */

Fits on a single line

Also, I would
        /* Match all the registed pipeline handlers. */

> +	while (1) {
> +		std::shared_ptr<PipelineHandler> pipe = factory->create(o);
> +		if (!pipe->match(enumerator_.get()))
> +			break;
> +
> +		LOG(Camera, Debug)
> +			<< "Pipeline handler \"" << factory->name()
> +			<< "\" matched";
>  	}
>  }

Overall, I concur this is an useful addition. I know there are ideas
about assigning a priority to pipeline handlers at creation time as
currently the only conflict is on the ISI which can be
matched both by the imx8-isi pipeline and the simple pipeline.

For the time being, with the above issues fixed
Reviewed-by: Jacopo Mondi <jacopo.mondi at ideasonboard.com>

Thanks
  j

>
> --
> 2.34.1
>