[libcamera-devel] [PATCH] libcamera: raspberrypi: Allow the tuning file to be set by an environment variable

[Naushir Patuck]

Hi Laurent,

David's away this week, so I'll respond on his behalf.

On Sun, 11 Jul 2021 at 21:34, Laurent Pinchart <
laurent.pinchart at ideasonboard.com> wrote:

> Hello,
>
> On Fri, Jul 09, 2021 at 04:04:22PM +0100, Kieran Bingham wrote:
> > On 07/07/2021 14:40, David Plowman wrote:
> > > The configuration (camera tuning) file used by the Raspberry Pi comes
> > > by default from the sensor name. However, we now allow this to be
> > > overridden by the LIBCAMERA_RPI_TUNING_FILE environment variable.
> >
> > Some how I had it in my head that this would be an override occurring in
> > the IPA itself, but I think this is ok too.
>
> I like it better in the pipeline handler, as the IPA module isolation
> mechanisms may limit the ability to pass environmen variables.
>
> > > Signed-off-by: David Plowman <david.plowman at raspberrypi.com>
> > > ---
> > >  src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 14 ++++++++++++--
> > >  1 file changed, 12 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > index 082eb1ee..a738770a 100644
> > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > @@ -1232,8 +1232,18 @@ int
> RPiCameraData::loadIPA(ipa::RPi::SensorConfig *sensorConfig)
> > >     ipa_->setIspControls.connect(this, &RPiCameraData::setIspControls);
> > >     ipa_->setDelayedControls.connect(this,
> &RPiCameraData::setDelayedControls);
> > >
> > > -   IPASettings settings(ipa_->configurationFile(sensor_->model() +
> ".json"),
> > > -                        sensor_->model());
> > > +   /*
> > > +    * The configuration (tuning file) is made from the sensor name
> unless
> > > +    * the environment variable overrides it.
> > > +    */
> > > +   std::string configurationFile;
> > > +   char const *configFromEnv =
> utils::secure_getenv("LIBCAMERA_RPI_TUNING_FILE");
> >
> > We likely need to document this somewhere...
> >
> > We have Documentation/environment_variables.rst
> >
> > But that could be a follow up patch
> >
> > > +   if (!configFromEnv || *configFromEnv == '\0')
> > > +           configurationFile =
> ipa_->configurationFile(sensor_->model() + ".json");
> >
> > I'm not a particular fan of the current ipa_->configurationFile
> > implementation, and I think I see that it should have changes in the
> future.
> >
> > If this were to be generalised, then the override should go there - but
> > given that this is quite specific to RPi needs right now - I think this
> > is ok.
>
> I'd like to see this being generalized indeed. We'll have to figure out
> how to specific a per-camera configuration file though.
>
> > However I could envisage that in the future this environment variable
> > gets deprecated, but I don't yet know what would replace it - so I
> > believe it's worth while to have this for now.
>
> That's the important question: David, if we later create a more generic
> mechanism to select a different IPA configuration file, will it be fine
> to drop support for LIBCAMERA_RPI_TUNING_FILE ?


Yes, I think that should be fine.  I assume any new mechanism would look
like a string parameter passed into the pipeline handler directly through
the application, hence able to be set on the command line.

Regards,
Naush



> If so,
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>
> > Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> >
> > > +   else
> > > +           configurationFile = std::string(configFromEnv);
> > > +
> > > +   IPASettings settings(configurationFile, sensor_->model());
> > >
> > >     return ipa_->init(settings, sensorConfig);
> > >  }
>
> --
> Regards,
>
> Laurent Pinchart
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20210712/e79aec00/attachment.htm>