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

[Kieran Bingham]

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

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