[PATCH v1 1/1] libcamera: camera_manager: Add environment variable to order pipelines match

[Julien Vuillaumier]

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 at 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
>>