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

[Julien Vuillaumier]

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 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.
>> +
>> +   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
>>