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

[Stefan Klug]

Hi Laurent,

Thank you for the patch.

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?

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

> 
> -	/* 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.

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

Cheers,
Stefan

> 
> base-commit: fdc01dc3e0969423d65ee87f118164ec74373e5d
> --
> Regards,
> 
> Laurent Pinchart
>