[PATCH] libcamera: pipeline: Move tuning file override handling to IPAProxy

[Laurent Pinchart]

Hi Stefan,

On Fri, Jan 24, 2025 at 07:19:47PM +0100, Stefan Klug wrote:
> On Thu, Jan 23, 2025 at 04:38:18PM +0200, Laurent Pinchart wrote:
> > The rkisp1 and rpi pipeline handlers duplicate code to handle the
> > LIBCAMERA_RKISP1_TUNING_FILE and LIBCAMERA_RPI_TUNING_FILE environment
> > variables that override tuning file selection. Move the common code to
> > IPAProxy::configurationFile() to avoid the duplication, and make the
> > feature available to all pipeline handlers with the same behaviour.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > ---
> >  Documentation/environment_variables.rst       |  4 +--
> >  src/libcamera/ipa_proxy.cpp                   | 27 +++++++++++++++----
> >  src/libcamera/pipeline/rkisp1/rkisp1.cpp      | 15 +++--------
> >  .../pipeline/rpi/common/pipeline_base.cpp     | 19 ++++---------
> >  4 files changed, 32 insertions(+), 33 deletions(-)
> > 
> > diff --git a/Documentation/environment_variables.rst b/Documentation/environment_variables.rst
> > index 7da9883a8380..6f1235587a40 100644
> > --- a/Documentation/environment_variables.rst
> > +++ b/Documentation/environment_variables.rst
> > @@ -57,8 +57,8 @@ LIBCAMERA_RPI_CONFIG_FILE
> > 
> >     Example value: ``/usr/local/share/libcamera/pipeline/rpi/vc4/minimal_mem.yaml``
> > 
> > -LIBCAMERA_RPI_TUNING_FILE
> > -   Define a custom JSON tuning file to use in the Raspberry Pi.
> > +LIBCAMERA_<NAME>_TUNING_FILE
> > +   Define a custom IPA tuning file to use with the pipeline handler `NAME`.
> > 
> >     Example value: ``/usr/local/share/libcamera/ipa/rpi/vc4/custom_sensor.json``
> 
> Not directly related to this patch but anyways. This variable is quite
> useful while developing, but fails if there are multiple sensors
> attached to the device. Should we add a LIBCAMERA_<NAME>_TUNING_DIR to
> add the ability to run multiple sensors with custom tuning files?

We could, but I think I would prefer exploring a configuration file
instead of environment variables.

> > diff --git a/src/libcamera/ipa_proxy.cpp b/src/libcamera/ipa_proxy.cpp
> > index 85004737c171..25f772a41bf8 100644
> > --- a/src/libcamera/ipa_proxy.cpp
> > +++ b/src/libcamera/ipa_proxy.cpp
> > @@ -98,16 +98,33 @@ IPAProxy::~IPAProxy()
> >  std::string IPAProxy::configurationFile(const std::string &name,
> >  					const std::string &fallbackName) const
> >  {
> > -	struct stat statbuf;
> > -	int ret;
> > -
> >  	/*
> >  	 * The IPA module name can be used as-is to build directory names as it
> >  	 * has been validated when loading the module.
> >  	 */
> > -	std::string ipaName = ipam_->info().name;
> > +	const std::string ipaName = ipam_->info().name;
> 
> Should this be a string_view?

We can't, because the operators that concatenate a std::string and a
std::string_view have only been added in C++20 :-( See
https://en.cppreference.com/w/cpp/string/basic_string/operator%2B.
Workarounds would be possible by defining our own operators (I actually
have a patch that does just that), but that's out of scope for this
patch.

> > -	/* Check the environment variable first. */
> > +	/*
> > +	 * Start with any user override through the module-specific environment
> > +	 * variable. Use the name of the IPA module up to the first '/' to
> > +	 * construct the variable name.
> > +	 */
> > +	std::string ipaEnvName = ipaName.substr(0, ipaName.find('/'));
> > +	std::transform(ipaEnvName.begin(), ipaEnvName.end(), ipaEnvName.begin(),
> > +		       [](unsigned char c) { return std::toupper(c); });
> 
> Sometimes c++ is really awful :-). Googling for this revealed
> 
> for (auto & c: ipaEnvName) c = std::toupper(c);
> 
> as short alternative.

I kind of like std::transform(), but it may be a case of Stockholm
syndrome :-)

> > +	ipaEnvName = "LIBCAMERA_" + ipaEnvName + "_TUNING_FILE";
> > +
> > +	char const *configFromEnv = utils::secure_getenv(ipaEnvName.c_str());
> > +	if (configFromEnv && *configFromEnv == '\0')
> > +		return { configFromEnv };
> > +
> > +	struct stat statbuf;
> > +	int ret;
> > +
> > +	/*
> > +	 * Check the directory pointed to by the IPA config path environment
> > +	 * variable next.
> > +	 */
> >  	const char *confPaths = utils::secure_getenv("LIBCAMERA_IPA_CONFIG_PATH");
> >  	if (confPaths) {
> >  		for (const auto &dir : utils::split(confPaths, ":")) {
> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > index 35c793da9bba..1ac8d8ae7ed9 100644
> > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > @@ -380,18 +380,9 @@ int RkISP1CameraData::loadIPA(unsigned int hwRevision)
> >  	ipa_->paramsComputed.connect(this, &RkISP1CameraData::paramsComputed);
> >  	ipa_->metadataReady.connect(this, &RkISP1CameraData::metadataReady);
> > 
> > -	/*
> > -	 * The API tuning file is made from the sensor name unless the
> > -	 * environment variable overrides it.
> > -	 */
> > -	std::string ipaTuningFile;
> > -	char const *configFromEnv = utils::secure_getenv("LIBCAMERA_RKISP1_TUNING_FILE");
> > -	if (!configFromEnv || *configFromEnv == '\0') {
> > -		ipaTuningFile =
> > -			ipa_->configurationFile(sensor_->model() + ".yaml", "uncalibrated.yaml");
> > -	} else {
> > -		ipaTuningFile = std::string(configFromEnv);
> > -	}
> > +	/* The IPA tuning file is made from the sensor name. */
> > +	std::string ipaTuningFile =
> > +		ipa_->configurationFile(sensor_->model() + ".yaml", "uncalibrated.yaml");
> 
> Oh I like this file getting shorter.
> 
> > 
> >  	IPACameraSensorInfo sensorInfo{};
> >  	int ret = sensor_->sensorInfo(&sensorInfo);
> > diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> > index 4b147fdb379a..1f13e5230fae 100644
> > --- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> > +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> > @@ -1156,20 +1156,11 @@ int CameraData::loadIPA(ipa::RPi::InitResult *result)
> >  	if (!ipa_)
> >  		return -ENOENT;
> > 
> > -	/*
> > -	 * 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");
> > -	if (!configFromEnv || *configFromEnv == '\0') {
> > -		std::string model = sensor_->model();
> > -		if (isMonoSensor(sensor_))
> > -			model += "_mono";
> > -		configurationFile = ipa_->configurationFile(model + ".json");
> > -	} else {
> > -		configurationFile = std::string(configFromEnv);
> > -	}
> > +	/* The configuration (tuning file) is made from the sensor name. */
> > +	std::string model = sensor_->model();
> > +	if (isMonoSensor(sensor_))
> > +		model += "_mono";
> > +	std::string configurationFile = ipa_->configurationFile(model + ".json");
> > 
> >  	IPASettings settings(configurationFile, sensor_->model());
> >  	ipa::RPi::InitParams params;
> 
> The other comments were nits, so
> Reviewed-by: Stefan Klug <stefan.klug at ideasonboard.com> 
> 
> > base-commit: fdc01dc3e0969423d65ee87f118164ec74373e5d

-- 
Regards,

Laurent Pinchart