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

[Laurent Pinchart]

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