[EXT] Re: [PATCH v1 1/1] libcamera: camera_manager: Add environment variable to order pipelines match
Kieran Bingham
kieran.bingham at ideasonboard.com
Thu Mar 7 10:49:48 CET 2024
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
> >>
>
More information about the libcamera-devel
mailing list