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

[Laurent Pinchart]

Hi Naush,

On Mon, Jul 12, 2021 at 02:46:48PM +0100, Naushir Patuck wrote:
> On Sun, 11 Jul 2021 at 21:34, Laurent Pinchart wrote:
> > 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.

I'm not sure yet what form that mechanism will take. An API that can be
called by applications is certainly tempting. In any case, it will have
to support per-camera configuration files, as a pipeline handler can
expose multiple cameras.

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