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

[Julien Vuillaumier]

Hi Kieran,

On 23/04/2024 11:09, Kieran Bingham wrote:

> 
> 
> Quoting Julien Vuillaumier (2024-03-27 17:27:31)
>> To match the enumerated media devices, each 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 to give the option to define an ordered list of pipelines to
>> match on.
>>
>> LIBCAMERA_PIPELINES_MATCH_LIST="<name1>[,<name2>[,<name3>...]]]"
>>
>> Example:
>> LIBCAMERA_PIPELINES_MATCH_LIST="PipelineHandlerRkISP1,SimplePipelineHandler"
> 
> I still think this env variable (and presumably also possible to set in
> a configuration file with Milan's configuration file work) makes sense.

Definitely, adding the option to configure the pipelines list using 
Milan's global configuration file would be useful.

> I do wish the match list used the short names that are equivalent to how
> the meson configuration would name them though. Perhaps as you suggested
> by using a parameter to the pipeline handler registration instead so
> that a more suitable name is used for the references.
> 
> That would still then work with the previous patch I believe.
> 
> So ... I think it's worth making the naming easier for users if we set
> this ... but I also think this patch is ok.
> 
> 
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> 
> So the question is - if we merge this - does it become some sort of
> expected ABI and mean we /shouldn't/ then change the name to the more
> user-friendly ones? And therefore we should do that first? or just do it
> on top ?

If using the short names is the preferred way to go, then I suppose it 
is better adding that change first. It should work the same with the 
other patches.
So, if nobody objects, I will add to those patches the change in the 
pipeline registration macro, in order to assign to each pipeline the 
short name used in meson files.

Thanks,
Julien

> 
> --
> Kieran
> 
> 
>>
>> Signed-off-by: Julien Vuillaumier <julien.vuillaumier at nxp.com>
>> Reviewed-by: Jacopo Mondi <jacopo.mondi at ideasonboard.com>
>> ---
>>   Documentation/environment_variables.rst     |  8 ++++
>>   include/libcamera/internal/camera_manager.h |  1 +
>>   src/libcamera/camera_manager.cpp            | 53 ++++++++++++++++-----
>>   3 files changed, 50 insertions(+), 12 deletions(-)
>>
>> diff --git a/Documentation/environment_variables.rst b/Documentation/environment_variables.rst
>> index a9b230bc..c763435c 100644
>> --- a/Documentation/environment_variables.rst
>> +++ b/Documentation/environment_variables.rst
>> @@ -37,6 +37,14 @@ 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. The pipeline handler names used to populate the
>> +   variable are the ones passed to the REGISTER_PIPELINE_HANDLER() macro in the
>> +   source code.
>> +
>> +   Example value: ``PipelineHandlerRkISP1,SimplePipelineHandler``
>> +
>>   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..5694d40d 100644
>> --- a/include/libcamera/internal/camera_manager.h
>> +++ b/include/libcamera/internal/camera_manager.h
>> @@ -44,6 +44,7 @@ protected:
>>   private:
>>          int init();
>>          void createPipelineHandlers();
>> +       void pipelineFactoryMatch(const PipelineHandlerFactoryBase *factory);
>>          void cleanup() LIBCAMERA_TSA_EXCLUDES(mutex_);
>>
>>          /*
>> diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp
>> index 355f3ada..ce6607e1 100644
>> --- a/src/libcamera/camera_manager.cpp
>> +++ b/src/libcamera/camera_manager.cpp
>> @@ -99,16 +99,37 @@ 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.
>>           */
>> +       const char *pipesList =
>> +               utils::secure_getenv("LIBCAMERA_PIPELINES_MATCH_LIST");
>> +       if (pipesList) {
>> +               /*
>> +                * When a list of preferred pipelines is defined, iterate
>> +                * through the ordered list to match the enumerated devices.
>> +                */
>> +               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();
>>
>> +       /* Match all the registered pipeline handlers. */
>>          for (const PipelineHandlerFactoryBase *factory : factories) {
>>                  LOG(Camera, Debug)
>>                          << "Found registered pipeline handler '"
>> @@ -117,15 +138,23 @@ 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. */
>> +       while (1) {
>> +               std::shared_ptr<PipelineHandler> pipe = factory->create(o);
>> +               if (!pipe->match(enumerator_.get()))
>> +                       break;
>> +
>> +               LOG(Camera, Debug)
>> +                       << "Pipeline handler \"" << factory->name()
>> +                       << "\" matched";
>>          }
>>   }
>>
>> --
>> 2.34.1
>>