[v1,1/1] libcamera: camera_manager: Add environment variable to order pipelines match
diff mbox series

Message ID 20240304181816.2361334-2-julien.vuillaumier@nxp.com
State Superseded
Headers show
Series
  • Add environment variable to order pipelines match
Related show

Commit Message

Julien Vuillaumier March 4, 2024, 6:18 p.m. UTC
To match the enumerated media devices, each pipeline handler registered
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
to invoke during the match process.

LIBCAMERA_PIPELINES_MATCH_LIST="<name1>[,<name2>[,<name3>...]]]"

Example:
LIBCAMERA_PIPELINES_MATCH_LIST="PipelineHandlerRkISP1,SimplePipelineHandler"

Signed-off-by: Julien Vuillaumier <julien.vuillaumier@nxp.com>
---
 Documentation/environment_variables.rst |  5 +++
 src/libcamera/camera_manager.cpp        | 51 +++++++++++++++++++++----
 2 files changed, 48 insertions(+), 8 deletions(-)

Comments

Kieran Bingham March 5, 2024, 6:54 a.m. UTC | #1
Quoting Julien Vuillaumier (2024-03-04 18:18:16)
> To match the enumerated media devices, each pipeline handler registered
> 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
> to invoke during the match process.
> 
> LIBCAMERA_PIPELINES_MATCH_LIST="<name1>[,<name2>[,<name3>...]]]"
> 
> Example:
> LIBCAMERA_PIPELINES_MATCH_LIST="PipelineHandlerRkISP1,SimplePipelineHandler"
> 
> Signed-off-by: Julien Vuillaumier <julien.vuillaumier@nxp.com>
> ---
>  Documentation/environment_variables.rst |  5 +++
>  src/libcamera/camera_manager.cpp        | 51 +++++++++++++++++++++----
>  2 files changed, 48 insertions(+), 8 deletions(-)
> 
> diff --git a/Documentation/environment_variables.rst b/Documentation/environment_variables.rst
> index a9b230bc..f3a0d431 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 ordered list of pipelines to be used to match the media devices.
> +
> +   Example value: ``PipelineHandlerRkISP1,SimplePipelineHandler``
> +
>  LIBCAMERA_RPI_CONFIG_FILE
>     Define a custom configuration file to use in the Raspberry Pi pipeline handler.
>  
> diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp
> index 355f3ada..9568801e 100644
> --- a/src/libcamera/camera_manager.cpp
> +++ b/src/libcamera/camera_manager.cpp
> @@ -109,14 +109,7 @@ void CameraManager::Private::createPipelineHandlers()
>         const std::vector<PipelineHandlerFactoryBase *> &factories =
>                 PipelineHandlerFactoryBase::factories();
>  
> -       for (const PipelineHandlerFactoryBase *factory : factories) {
> -               LOG(Camera, Debug)
> -                       << "Found registered pipeline handler '"
> -                       << factory->name() << "'";
> -               /*
> -                * Try each pipeline handler until it exhaust
> -                * all pipelines it can provide.
> -                */
> +       auto pipeMatch = [&](const PipelineHandlerFactoryBase *factory) {

Does this need to be a lambda? is there a specific benefit, or can it be
a function? I find lambda's hard to parse :-( but perhaps that's just
me, so lets see what others say.


>                 while (1) {
>                         std::shared_ptr<PipelineHandler> pipe = factory->create(o);
>                         if (!pipe->match(enumerator_.get()))
> @@ -126,6 +119,48 @@ void CameraManager::Private::createPipelineHandlers()
>                                 << "Pipeline handler \"" << factory->name()
>                                 << "\" matched";
>                 }
> +       };
> +
> +       /*
> +        * When a list of preferred pipelines is defined, iterate through the
> +        * ordered list to match the devices enumerated.
> +        * Otherwise, devices matching is done in no specific order with each

s/devices/device/

> +        * pipeline handler registered.
> +        */
> +       const char *pipesLists =
> +               utils::secure_getenv("LIBCAMERA_PIPELINES_MATCH_LIST");
> +       if (pipesLists) {
> +               for (const auto &pipeName : utils::split(pipesLists, ",")) {
> +                       if (pipeName.empty())
> +                               continue;
> +
> +                       auto nameMatch = [pipeName](const PipelineHandlerFactoryBase *f) {
> +                               return (f->name() == pipeName);
> +                       };
> +
> +                       auto iter = std::find_if(factories.begin(),
> +                                                factories.end(),
> +                                                nameMatch);

If we do this - I'd probably add support to the PipelineHandlerFactory
to support getting by name as a preceeding patch.

> +
> +                       if (iter != factories.end()) {
> +                               LOG(Camera, Debug)
> +                                       << "Found pipeline handler from list '"
> +                                       << (*iter)->name() << "'";
> +                               pipeMatch(*iter);
> +                       }
> +               }
> +               return;
> +       }
> +
> +       for (const PipelineHandlerFactoryBase *factory : factories) {
> +               LOG(Camera, Debug)
> +                       << "Found registered pipeline handler '"
> +                       << factory->name() << "'";
> +               /*
> +                * Try each pipeline handler until it exhausts
> +                * all pipelines it can provide.
> +                */
> +               pipeMatch(factory);
>         }
>  }
>  
> -- 
> 2.34.1
>
Barnabás Pőcze March 5, 2024, 4:42 p.m. UTC | #2
Hi


2024. március 4., hétfő 19:18 keltezéssel, Julien Vuillaumier <julien.vuillaumier@nxp.com> írta:

> To match the enumerated media devices, each pipeline handler registered
> 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
> to invoke during the match process.
> 
> LIBCAMERA_PIPELINES_MATCH_LIST="<name1>[,<name2>[,<name3>...]]]"
> 
> Example:
> LIBCAMERA_PIPELINES_MATCH_LIST="PipelineHandlerRkISP1,SimplePipelineHandler"
> 
> Signed-off-by: Julien Vuillaumier <julien.vuillaumier@nxp.com>
> ---
> [...]
> +
> +	/*
> +	 * When a list of preferred pipelines is defined, iterate through the
> +	 * ordered list to match the devices enumerated.
> +	 * Otherwise, devices matching is done in no specific order with each
> +	 * pipeline handler registered.
> +	 */
> +	const char *pipesLists =
> +		utils::secure_getenv("LIBCAMERA_PIPELINES_MATCH_LIST");
> +	if (pipesLists) {
> +		for (const auto &pipeName : utils::split(pipesLists, ",")) {
> +			if (pipeName.empty())
> +				continue;
> +
> +			auto nameMatch = [pipeName](const PipelineHandlerFactoryBase *f) {

`[&pipeName]` to avoid the copy.


> +				return (f->name() == pipeName);
> +			};
> +
> +			auto iter = std::find_if(factories.begin(),
> +						 factories.end(),
> +						 nameMatch);
> +
> +			if (iter != factories.end()) {
> +				LOG(Camera, Debug)
> +					<< "Found pipeline handler from list '"
> +					<< (*iter)->name() << "'";
> +				pipeMatch(*iter);
> +			}
> +		}
> +		return;
> +	}
> +
> +	for (const PipelineHandlerFactoryBase *factory : factories) {
> +		LOG(Camera, Debug)
> +			<< "Found registered pipeline handler '"
> +			<< factory->name() << "'";
> +		/*
> +		 * Try each pipeline handler until it exhausts
> +		 * all pipelines it can provide.
> +		 */
> +		pipeMatch(factory);
>  	}
>  }
> 
> --
> 2.34.1
> 


Regards,
Barnabás Pőcze
Jacopo Mondi March 5, 2024, 5:38 p.m. UTC | #3
Hi Julien

On Mon, Mar 04, 2024 at 07:18:16PM +0100, Julien Vuillaumier wrote:
> To match the enumerated media devices, each pipeline handler registered
> 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
> to invoke during the match process.
>
> LIBCAMERA_PIPELINES_MATCH_LIST="<name1>[,<name2>[,<name3>...]]]"
>
> Example:
> LIBCAMERA_PIPELINES_MATCH_LIST="PipelineHandlerRkISP1,SimplePipelineHandler"

I think that's a good idea. I expect some bikeshedding on the env
variable name :)

>
> Signed-off-by: Julien Vuillaumier <julien.vuillaumier@nxp.com>
> ---
>  Documentation/environment_variables.rst |  5 +++
>  src/libcamera/camera_manager.cpp        | 51 +++++++++++++++++++++----
>  2 files changed, 48 insertions(+), 8 deletions(-)
>
> diff --git a/Documentation/environment_variables.rst b/Documentation/environment_variables.rst
> index a9b230bc..f3a0d431 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 ordered list of pipelines to be used to match the media devices.

nit:

'an ordered list of pipeline names'
'the media devices in the system'

?

> +
> +   Example value: ``PipelineHandlerRkISP1,SimplePipelineHandler``
> +
>  LIBCAMERA_RPI_CONFIG_FILE
>     Define a custom configuration file to use in the Raspberry Pi pipeline handler.
>
> diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp
> index 355f3ada..9568801e 100644
> --- a/src/libcamera/camera_manager.cpp
> +++ b/src/libcamera/camera_manager.cpp
> @@ -109,14 +109,7 @@ void CameraManager::Private::createPipelineHandlers()
>  	const std::vector<PipelineHandlerFactoryBase *> &factories =
>  		PipelineHandlerFactoryBase::factories();
>
> -	for (const PipelineHandlerFactoryBase *factory : factories) {
> -		LOG(Camera, Debug)
> -			<< "Found registered pipeline handler '"
> -			<< factory->name() << "'";
> -		/*
> -		 * Try each pipeline handler until it exhaust
> -		 * all pipelines it can provide.
> -		 */
> +	auto pipeMatch = [&](const PipelineHandlerFactoryBase *factory) {
>  		while (1) {
>  			std::shared_ptr<PipelineHandler> pipe = factory->create(o);
>  			if (!pipe->match(enumerator_.get()))
> @@ -126,6 +119,48 @@ void CameraManager::Private::createPipelineHandlers()
>  				<< "Pipeline handler \"" << factory->name()
>  				<< "\" matched";
>  		}
> +	};
> +
> +	/*
> +	 * When a list of preferred pipelines is defined, iterate through the
> +	 * ordered list to match the devices enumerated.
> +	 * Otherwise, devices matching is done in no specific order with each
> +	 * pipeline handler registered.

nit: 'registered pipeline handler'

> +	 */
> +	const char *pipesLists =

nit: pipesList

> +		utils::secure_getenv("LIBCAMERA_PIPELINES_MATCH_LIST");
> +	if (pipesLists) {
> +		for (const auto &pipeName : utils::split(pipesLists, ",")) {
> +			if (pipeName.empty())
> +				continue;

Not sure if this can be empty, but better safe than sorry

> +
> +			auto nameMatch = [pipeName](const PipelineHandlerFactoryBase *f) {
> +				return (f->name() == pipeName);

no need for ()

> +			};
> +
> +			auto iter = std::find_if(factories.begin(),
> +						 factories.end(),
> +						 nameMatch);

You could also inline the above here

			auto iter = std::find_if(factories.begin(),
						 factories.end(),
						 [&pipeName](const PipelineHandlerFactoryBase *f)
                                                 {
                                                        return f->name() == pipeName;
                                                 });

Up to you

> +
> +			if (iter != factories.end()) {
> +				LOG(Camera, Debug)
> +					<< "Found pipeline handler from list '"
> +					<< (*iter)->name() << "'";
> +				pipeMatch(*iter);
> +			}
> +		}
> +		return;
> +	}
> +
> +	for (const PipelineHandlerFactoryBase *factory : factories) {
> +		LOG(Camera, Debug)
> +			<< "Found registered pipeline handler '"
> +			<< factory->name() << "'";

If order was not relevant, you could have simply searched
factory->name() in pipesLists here, but I presume the order in which
pipelines are specified in the env variable and matched is relevant,
right ?

> +		/*
> +		 * Try each pipeline handler until it exhausts
> +		 * all pipelines it can provide.
> +		 */
> +		pipeMatch(factory);
>  	}
>  }
>
> --
> 2.34.1
>
Julien Vuillaumier March 6, 2024, 5:57 p.m. UTC | #4
Hi Kieran,

On 05/03/2024 07:54, Kieran Bingham wrote:
> Caution: This is an external email. Please take care when clicking links or opening attachments. When in doubt, report the message using the 'Report this email' button
> 
> 
> Quoting Julien Vuillaumier (2024-03-04 18:18:16)
>> To match the enumerated media devices, each pipeline handler registered
>> 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
>> to invoke during the match process.
>>
>> LIBCAMERA_PIPELINES_MATCH_LIST="<name1>[,<name2>[,<name3>...]]]"
>>
>> Example:
>> LIBCAMERA_PIPELINES_MATCH_LIST="PipelineHandlerRkISP1,SimplePipelineHandler"
>>
>> Signed-off-by: Julien Vuillaumier <julien.vuillaumier@nxp.com>
>> ---
>>   Documentation/environment_variables.rst |  5 +++
>>   src/libcamera/camera_manager.cpp        | 51 +++++++++++++++++++++----
>>   2 files changed, 48 insertions(+), 8 deletions(-)
>>
>> diff --git a/Documentation/environment_variables.rst b/Documentation/environment_variables.rst
>> index a9b230bc..f3a0d431 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 ordered list of pipelines to be used to match the media devices.
>> +
>> +   Example value: ``PipelineHandlerRkISP1,SimplePipelineHandler``
>> +
>>   LIBCAMERA_RPI_CONFIG_FILE
>>      Define a custom configuration file to use in the Raspberry Pi pipeline handler.
>>
>> diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp
>> index 355f3ada..9568801e 100644
>> --- a/src/libcamera/camera_manager.cpp
>> +++ b/src/libcamera/camera_manager.cpp
>> @@ -109,14 +109,7 @@ void CameraManager::Private::createPipelineHandlers()
>>          const std::vector<PipelineHandlerFactoryBase *> &factories =
>>                  PipelineHandlerFactoryBase::factories();
>>
>> -       for (const PipelineHandlerFactoryBase *factory : factories) {
>> -               LOG(Camera, Debug)
>> -                       << "Found registered pipeline handler '"
>> -                       << factory->name() << "'";
>> -               /*
>> -                * Try each pipeline handler until it exhaust
>> -                * all pipelines it can provide.
>> -                */
>> +       auto pipeMatch = [&](const PipelineHandlerFactoryBase *factory) {
> 
> Does this need to be a lambda? is there a specific benefit, or can it be
> a function? I find lambda's hard to parse :-( but perhaps that's just
> me, so lets see what others say.

This lambda can be moved to a function, but this function would probably 
never be used from anywhere else - for that reason it was made local as 
a lambda. But if consensus is to limit usage of lambda, a function can 
be used instead.

> 
>>                  while (1) {
>>                          std::shared_ptr<PipelineHandler> pipe = factory->create(o);
>>                          if (!pipe->match(enumerator_.get()))
>> @@ -126,6 +119,48 @@ void CameraManager::Private::createPipelineHandlers()
>>                                  << "Pipeline handler \"" << factory->name()
>>                                  << "\" matched";
>>                  }
>> +       };
>> +
>> +       /*
>> +        * When a list of preferred pipelines is defined, iterate through the
>> +        * ordered list to match the devices enumerated.
>> +        * Otherwise, devices matching is done in no specific order with each
> 
> s/devices/device/
> 

I will update in v2 - thanks.

>> +        * pipeline handler registered.
>> +        */
>> +       const char *pipesLists =
>> +               utils::secure_getenv("LIBCAMERA_PIPELINES_MATCH_LIST");
>> +       if (pipesLists) {
>> +               for (const auto &pipeName : utils::split(pipesLists, ",")) {
>> +                       if (pipeName.empty())
>> +                               continue;
>> +
>> +                       auto nameMatch = [pipeName](const PipelineHandlerFactoryBase *f) {
>> +                               return (f->name() == pipeName);
>> +                       };
>> +
>> +                       auto iter = std::find_if(factories.begin(),
>> +                                                factories.end(),
>> +                                                nameMatch);
> 
> If we do this - I'd probably add support to the PipelineHandlerFactory
> to support getting by name as a preceeding patch.
> 

Sure, I can do - will update in v2.

>> +
>> +                       if (iter != factories.end()) {
>> +                               LOG(Camera, Debug)
>> +                                       << "Found pipeline handler from list '"
>> +                                       << (*iter)->name() << "'";
>> +                               pipeMatch(*iter);
>> +                       }
>> +               }
>> +               return;
>> +       }
>> +
>> +       for (const PipelineHandlerFactoryBase *factory : factories) {
>> +               LOG(Camera, Debug)
>> +                       << "Found registered pipeline handler '"
>> +                       << factory->name() << "'";
>> +               /*
>> +                * Try each pipeline handler until it exhausts
>> +                * all pipelines it can provide.
>> +                */
>> +               pipeMatch(factory);
>>          }
>>   }
>>
>> --
>> 2.34.1
>>
Julien Vuillaumier March 6, 2024, 6:04 p.m. UTC | #5
Hi Barnabás,

On 05/03/2024 17:42, Barnabás Pőcze wrote:
> Caution: This is an external email. Please take care when clicking links or opening attachments. When in doubt, report the message using the 'Report this email' button
> 
> 
> Hi
> 
> 
> 2024. március 4., hétfő 19:18 keltezéssel, Julien Vuillaumier <julien.vuillaumier@nxp.com> írta:
> 
>> To match the enumerated media devices, each pipeline handler registered
>> 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
>> to invoke during the match process.
>>
>> LIBCAMERA_PIPELINES_MATCH_LIST="<name1>[,<name2>[,<name3>...]]]"
>>
>> Example:
>> LIBCAMERA_PIPELINES_MATCH_LIST="PipelineHandlerRkISP1,SimplePipelineHandler"
>>
>> Signed-off-by: Julien Vuillaumier <julien.vuillaumier@nxp.com>
>> ---
>> [...]
>> +
>> +     /*
>> +      * When a list of preferred pipelines is defined, iterate through the
>> +      * ordered list to match the devices enumerated.
>> +      * Otherwise, devices matching is done in no specific order with each
>> +      * pipeline handler registered.
>> +      */
>> +     const char *pipesLists =
>> +             utils::secure_getenv("LIBCAMERA_PIPELINES_MATCH_LIST");
>> +     if (pipesLists) {
>> +             for (const auto &pipeName : utils::split(pipesLists, ",")) {
>> +                     if (pipeName.empty())
>> +                             continue;
>> +
>> +                     auto nameMatch = [pipeName](const PipelineHandlerFactoryBase *f) {
> 
> `[&pipeName]` to avoid the copy.

I will update in v2 - thanks

> 
>> +                             return (f->name() == pipeName);
>> +                     };
>> +
>> +                     auto iter = std::find_if(factories.begin(),
>> +                                              factories.end(),
>> +                                              nameMatch);
>> +
>> +                     if (iter != factories.end()) {
>> +                             LOG(Camera, Debug)
>> +                                     << "Found pipeline handler from list '"
>> +                                     << (*iter)->name() << "'";
>> +                             pipeMatch(*iter);
>> +                     }
>> +             }
>> +             return;
>> +     }
>> +
>> +     for (const PipelineHandlerFactoryBase *factory : factories) {
>> +             LOG(Camera, Debug)
>> +                     << "Found registered pipeline handler '"
>> +                     << factory->name() << "'";
>> +             /*
>> +              * Try each pipeline handler until it exhausts
>> +              * all pipelines it can provide.
>> +              */
>> +             pipeMatch(factory);
>>        }
>>   }
>>
>> --
>> 2.34.1
>>
> 
> 
> Regards,
> Barnabás Pőcze
Julien Vuillaumier March 6, 2024, 6:45 p.m. UTC | #6
Hi Jacopo,


On 05/03/2024 18:38, Jacopo Mondi wrote:
> Caution: This is an external email. Please take care when clicking links or opening attachments. When in doubt, report the message using the 'Report this email' button
> 
> 
> Hi Julien
> 
> On Mon, Mar 04, 2024 at 07:18:16PM +0100, Julien Vuillaumier wrote:
>> To match the enumerated media devices, each pipeline handler registered
>> 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
>> to invoke during the match process.
>>
>> LIBCAMERA_PIPELINES_MATCH_LIST="<name1>[,<name2>[,<name3>...]]]"
>>
>> Example:
>> LIBCAMERA_PIPELINES_MATCH_LIST="PipelineHandlerRkISP1,SimplePipelineHandler"
> 
> I think that's a good idea. I expect some bikeshedding on the env
> variable name :)
> 

No worries, env variable name can be changed if needed :)

>>
>> Signed-off-by: Julien Vuillaumier <julien.vuillaumier@nxp.com>
>> ---
>>   Documentation/environment_variables.rst |  5 +++
>>   src/libcamera/camera_manager.cpp        | 51 +++++++++++++++++++++----
>>   2 files changed, 48 insertions(+), 8 deletions(-)
>>
>> diff --git a/Documentation/environment_variables.rst b/Documentation/environment_variables.rst
>> index a9b230bc..f3a0d431 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 ordered list of pipelines to be used to match the media devices.
> 
> nit:
> 
> 'an ordered list of pipeline names'
> 'the media devices in the system'
> 
> ?

Sure, I will update for the v2 with:
'Define an ordered list of pipeline names to be used to match the media 
devices in the system.'

> 
>> +
>> +   Example value: ``PipelineHandlerRkISP1,SimplePipelineHandler``
>> +
>>   LIBCAMERA_RPI_CONFIG_FILE
>>      Define a custom configuration file to use in the Raspberry Pi pipeline handler.
>>
>> diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp
>> index 355f3ada..9568801e 100644
>> --- a/src/libcamera/camera_manager.cpp
>> +++ b/src/libcamera/camera_manager.cpp
>> @@ -109,14 +109,7 @@ void CameraManager::Private::createPipelineHandlers()
>>        const std::vector<PipelineHandlerFactoryBase *> &factories =
>>                PipelineHandlerFactoryBase::factories();
>>
>> -     for (const PipelineHandlerFactoryBase *factory : factories) {
>> -             LOG(Camera, Debug)
>> -                     << "Found registered pipeline handler '"
>> -                     << factory->name() << "'";
>> -             /*
>> -              * Try each pipeline handler until it exhaust
>> -              * all pipelines it can provide.
>> -              */
>> +     auto pipeMatch = [&](const PipelineHandlerFactoryBase *factory) {
>>                while (1) {
>>                        std::shared_ptr<PipelineHandler> pipe = factory->create(o);
>>                        if (!pipe->match(enumerator_.get()))
>> @@ -126,6 +119,48 @@ void CameraManager::Private::createPipelineHandlers()
>>                                << "Pipeline handler \"" << factory->name()
>>                                << "\" matched";
>>                }
>> +     };
>> +
>> +     /*
>> +      * When a list of preferred pipelines is defined, iterate through the
>> +      * ordered list to match the devices enumerated.
>> +      * Otherwise, devices matching is done in no specific order with each
>> +      * pipeline handler registered.
> 
> nit: 'registered pipeline handler'
> 

I will update the comment in v2

>> +      */
>> +     const char *pipesLists =
> 
> nit: pipesList >

I will update the variable name in v2


>> +             utils::secure_getenv("LIBCAMERA_PIPELINES_MATCH_LIST");
>> +     if (pipesLists) {
>> +             for (const auto &pipeName : utils::split(pipesLists, ",")) {
>> +                     if (pipeName.empty())
>> +                             continue;
> 
> Not sure if this can be empty, but better safe than sorry
> 

Agreed, not sure empty() test is strictly necessary. Intent was 
presumably to catch early (odd) definitions like:
var='pipe1,,pipe2'
But things should work without. I will double check and remove in v2 if 
not needed.

>> +
>> +                     auto nameMatch = [pipeName](const PipelineHandlerFactoryBase *f) {
>> +                             return (f->name() == pipeName);
> 
> no need for ()
> 

I will remove the extra () in v2

>> +                     };
>> +
>> +                     auto iter = std::find_if(factories.begin(),
>> +                                              factories.end(),
>> +                                              nameMatch);
> 
> You could also inline the above here
> 
>                          auto iter = std::find_if(factories.begin(),
>                                                   factories.end(),
>                                                   [&pipeName](const PipelineHandlerFactoryBase *f)
>                                                   {
>                                                          return f->name() == pipeName;
>                                                   });
> 
> Up to you
> 

That was an attempt to adhere to coding style and stay within the 80 
columns - but the lambda declaration failed to do so anyway...
Thank you for the suggestion - I will see how that fits after moving 
this block to a getting by name method of the PipelineHandlerFactory, as 
Kieran suggested.

>> +
>> +                     if (iter != factories.end()) {
>> +                             LOG(Camera, Debug)
>> +                                     << "Found pipeline handler from list '"
>> +                                     << (*iter)->name() << "'";
>> +                             pipeMatch(*iter);
>> +                     }
>> +             }
>> +             return;
>> +     }
>> +
>> +     for (const PipelineHandlerFactoryBase *factory : factories) {
>> +             LOG(Camera, Debug)
>> +                     << "Found registered pipeline handler '"
>> +                     << factory->name() << "'";
> 
> If order was not relevant, you could have simply searched
> factory->name() in pipesLists here, but I presume the order in which
> pipelines are specified in the env variable and matched is relevant,
> right ?
> 

The order of the pipelines listed in the env variable is relevant as it 
defines their respective priorities for the match.
For that reason, if a list exists, the preceding 'for' block iterates in 
order through every pipeline name and attempts match with relevant pipeline.

That second 'for' block is the default case when there is no list 
defined. It corresponds to the current implementation where match is 
attempted with every pipeline registered in PipelineHandlerFactoryBase, 
but without specific order.

>> +             /*
>> +              * Try each pipeline handler until it exhausts
>> +              * all pipelines it can provide.
>> +              */
>> +             pipeMatch(factory);
>>        }
>>   }
>>
>> --
>> 2.34.1
>>
Kieran Bingham March 7, 2024, 9:49 a.m. UTC | #7
Quoting Julien Vuillaumier (2024-03-06 17:57:58)
> Hi Kieran,
> 
> On 05/03/2024 07:54, Kieran Bingham wrote:
> > Caution: This is an external email. Please take care when clicking links or opening attachments. When in doubt, report the message using the 'Report this email' button
> > 
> > 
> > Quoting Julien Vuillaumier (2024-03-04 18:18:16)
> >> To match the enumerated media devices, each pipeline handler registered
> >> 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
> >> to invoke during the match process.
> >>
> >> LIBCAMERA_PIPELINES_MATCH_LIST="<name1>[,<name2>[,<name3>...]]]"
> >>
> >> Example:
> >> LIBCAMERA_PIPELINES_MATCH_LIST="PipelineHandlerRkISP1,SimplePipelineHandler"
> >>
> >> Signed-off-by: Julien Vuillaumier <julien.vuillaumier@nxp.com>
> >> ---
> >>   Documentation/environment_variables.rst |  5 +++
> >>   src/libcamera/camera_manager.cpp        | 51 +++++++++++++++++++++----
> >>   2 files changed, 48 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/Documentation/environment_variables.rst b/Documentation/environment_variables.rst
> >> index a9b230bc..f3a0d431 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 ordered list of pipelines to be used to match the media devices.
> >> +
> >> +   Example value: ``PipelineHandlerRkISP1,SimplePipelineHandler``
> >> +
> >>   LIBCAMERA_RPI_CONFIG_FILE
> >>      Define a custom configuration file to use in the Raspberry Pi pipeline handler.
> >>
> >> diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp
> >> index 355f3ada..9568801e 100644
> >> --- a/src/libcamera/camera_manager.cpp
> >> +++ b/src/libcamera/camera_manager.cpp
> >> @@ -109,14 +109,7 @@ void CameraManager::Private::createPipelineHandlers()
> >>          const std::vector<PipelineHandlerFactoryBase *> &factories =
> >>                  PipelineHandlerFactoryBase::factories();
> >>
> >> -       for (const PipelineHandlerFactoryBase *factory : factories) {
> >> -               LOG(Camera, Debug)
> >> -                       << "Found registered pipeline handler '"
> >> -                       << factory->name() << "'";
> >> -               /*
> >> -                * Try each pipeline handler until it exhaust
> >> -                * all pipelines it can provide.
> >> -                */
> >> +       auto pipeMatch = [&](const PipelineHandlerFactoryBase *factory) {
> > 
> > Does this need to be a lambda? is there a specific benefit, or can it be
> > a function? I find lambda's hard to parse :-( but perhaps that's just
> > me, so lets see what others say.
> 
> This lambda can be moved to a function, but this function would probably 
> never be used from anywhere else - for that reason it was made local as 
> a lambda. But if consensus is to limit usage of lambda, a function can 
> be used instead.

Indeed, that makes sense. We've often used private class functions where
needed though. I don't think what you've done is wrong, just not what
I'm used to ;-) so I don't specifically mind if it stays, but just to
ease readibility a block comment above the lambda might help.

--
Kieran

> 
> > 
> >>                  while (1) {
> >>                          std::shared_ptr<PipelineHandler> pipe = factory->create(o);
> >>                          if (!pipe->match(enumerator_.get()))
> >> @@ -126,6 +119,48 @@ void CameraManager::Private::createPipelineHandlers()
> >>                                  << "Pipeline handler \"" << factory->name()
> >>                                  << "\" matched";
> >>                  }
> >> +       };
> >> +
> >> +       /*
> >> +        * When a list of preferred pipelines is defined, iterate through the
> >> +        * ordered list to match the devices enumerated.
> >> +        * Otherwise, devices matching is done in no specific order with each
> > 
> > s/devices/device/
> > 
> 
> I will update in v2 - thanks.
> 
> >> +        * pipeline handler registered.
> >> +        */
> >> +       const char *pipesLists =
> >> +               utils::secure_getenv("LIBCAMERA_PIPELINES_MATCH_LIST");
> >> +       if (pipesLists) {
> >> +               for (const auto &pipeName : utils::split(pipesLists, ",")) {
> >> +                       if (pipeName.empty())
> >> +                               continue;
> >> +
> >> +                       auto nameMatch = [pipeName](const PipelineHandlerFactoryBase *f) {
> >> +                               return (f->name() == pipeName);
> >> +                       };
> >> +
> >> +                       auto iter = std::find_if(factories.begin(),
> >> +                                                factories.end(),
> >> +                                                nameMatch);
> > 
> > If we do this - I'd probably add support to the PipelineHandlerFactory
> > to support getting by name as a preceeding patch.
> > 
> 
> Sure, I can do - will update in v2.
> 
> >> +
> >> +                       if (iter != factories.end()) {
> >> +                               LOG(Camera, Debug)
> >> +                                       << "Found pipeline handler from list '"
> >> +                                       << (*iter)->name() << "'";
> >> +                               pipeMatch(*iter);
> >> +                       }
> >> +               }
> >> +               return;
> >> +       }
> >> +
> >> +       for (const PipelineHandlerFactoryBase *factory : factories) {
> >> +               LOG(Camera, Debug)
> >> +                       << "Found registered pipeline handler '"
> >> +                       << factory->name() << "'";
> >> +               /*
> >> +                * Try each pipeline handler until it exhausts
> >> +                * all pipelines it can provide.
> >> +                */
> >> +               pipeMatch(factory);
> >>          }
> >>   }
> >>
> >> --
> >> 2.34.1
> >>
>

Patch
diff mbox series

diff --git a/Documentation/environment_variables.rst b/Documentation/environment_variables.rst
index a9b230bc..f3a0d431 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 ordered list of pipelines to be used to match the media devices.
+
+   Example value: ``PipelineHandlerRkISP1,SimplePipelineHandler``
+
 LIBCAMERA_RPI_CONFIG_FILE
    Define a custom configuration file to use in the Raspberry Pi pipeline handler.
 
diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp
index 355f3ada..9568801e 100644
--- a/src/libcamera/camera_manager.cpp
+++ b/src/libcamera/camera_manager.cpp
@@ -109,14 +109,7 @@  void CameraManager::Private::createPipelineHandlers()
 	const std::vector<PipelineHandlerFactoryBase *> &factories =
 		PipelineHandlerFactoryBase::factories();
 
-	for (const PipelineHandlerFactoryBase *factory : factories) {
-		LOG(Camera, Debug)
-			<< "Found registered pipeline handler '"
-			<< factory->name() << "'";
-		/*
-		 * Try each pipeline handler until it exhaust
-		 * all pipelines it can provide.
-		 */
+	auto pipeMatch = [&](const PipelineHandlerFactoryBase *factory) {
 		while (1) {
 			std::shared_ptr<PipelineHandler> pipe = factory->create(o);
 			if (!pipe->match(enumerator_.get()))
@@ -126,6 +119,48 @@  void CameraManager::Private::createPipelineHandlers()
 				<< "Pipeline handler \"" << factory->name()
 				<< "\" matched";
 		}
+	};
+
+	/*
+	 * When a list of preferred pipelines is defined, iterate through the
+	 * ordered list to match the devices enumerated.
+	 * Otherwise, devices matching is done in no specific order with each
+	 * pipeline handler registered.
+	 */
+	const char *pipesLists =
+		utils::secure_getenv("LIBCAMERA_PIPELINES_MATCH_LIST");
+	if (pipesLists) {
+		for (const auto &pipeName : utils::split(pipesLists, ",")) {
+			if (pipeName.empty())
+				continue;
+
+			auto nameMatch = [pipeName](const PipelineHandlerFactoryBase *f) {
+				return (f->name() == pipeName);
+			};
+
+			auto iter = std::find_if(factories.begin(),
+						 factories.end(),
+						 nameMatch);
+
+			if (iter != factories.end()) {
+				LOG(Camera, Debug)
+					<< "Found pipeline handler from list '"
+					<< (*iter)->name() << "'";
+				pipeMatch(*iter);
+			}
+		}
+		return;
+	}
+
+	for (const PipelineHandlerFactoryBase *factory : factories) {
+		LOG(Camera, Debug)
+			<< "Found registered pipeline handler '"
+			<< factory->name() << "'";
+		/*
+		 * Try each pipeline handler until it exhausts
+		 * all pipelines it can provide.
+		 */
+		pipeMatch(factory);
 	}
 }