[PATCH v2 3/3] libcamera: ipa_proxy: Report a missing configuration as a warning

[Laurent Pinchart]

On Mon, Jul 08, 2024 at 02:38:03PM +0200, Milan Zamazal wrote:
> When the configuration file for an IPA module is missing, it is reported
> as an error in the log, for example:
> 
>   ERROR IPAProxy ipa_proxy.cpp:149 Configuration file 'imx219.yaml' not found for IPA module 'simple'
> 
> This is misleading because several pipelines use uncalibrated.yaml in
> such a case and can continue working.  And in case of software ISP,
> there is currently no other configuration file so the error is always
> reported.
> 
> On the other hand, in some other cases the presence of the configuration
> file is required and it is an error if it is missing.
> 
> Let's introduce a new optional argument to IPAProxy::configurationFile
> that specifies a fallback file if the requested file is not found.  If
> the primary requested file is not found and a non-empty fallback file is
> specified then a warning is logged and the fallback file is looked up.
> If neither the fallback file can be found then only then an error is
> logged and the method returns an empty string.  This change has also the
> benefit of putting the common fallback file ("uncalibrated.yaml")
> pattern to a single place.
> 
> Signed-off-by: Milan Zamazal <mzamazal at redhat.com>
> ---
>  include/libcamera/internal/ipa_proxy.h      |  4 +++-
>  src/libcamera/ipa_proxy.cpp                 | 22 +++++++++++++++++----
>  src/libcamera/pipeline/ipu3/ipu3.cpp        |  5 ++---
>  src/libcamera/pipeline/rkisp1/rkisp1.cpp    |  9 ++-------
>  src/libcamera/software_isp/software_isp.cpp |  5 ++---
>  5 files changed, 27 insertions(+), 18 deletions(-)
> 
> diff --git a/include/libcamera/internal/ipa_proxy.h b/include/libcamera/internal/ipa_proxy.h
> index 5240f69f..88068de7 100644
> --- a/include/libcamera/internal/ipa_proxy.h
> +++ b/include/libcamera/internal/ipa_proxy.h
> @@ -31,7 +31,9 @@ public:
>  
>  	bool isValid() const { return valid_; }
>  
> -	std::string configurationFile(const std::string &name) const;
> +	std::string configurationFile(
> +		const std::string &name,
> +		const std::string &fallbackName = std::string()) const;

	std::string configurationFile(const std::string &name,
				      const std::string &fallbackName = std::string()) const;

>  
>  protected:
>  	std::string resolvePath(const std::string &file) const;
> diff --git a/src/libcamera/ipa_proxy.cpp b/src/libcamera/ipa_proxy.cpp
> index 6c17c456..247c6fa0 100644
> --- a/src/libcamera/ipa_proxy.cpp
> +++ b/src/libcamera/ipa_proxy.cpp
> @@ -72,6 +72,7 @@ IPAProxy::~IPAProxy()
>  /**
>   * \brief Retrieve the absolute path to an IPA configuration file
>   * \param[in] name The configuration file name
> + * \param[in] fallbackName The name of a fallback configuration file
>   *
>   * This function locates the configuration file for an IPA and returns its
>   * absolute path. It searches the following directories, in order:
> @@ -89,10 +90,15 @@ IPAProxy::~IPAProxy()
>   * named after the IPA module name, as reported in IPAModuleInfo::name, and for
>   * a file named \a name within that directory. The \a name is IPA-specific.
>   *
> + * If the file named \a name is not found and \a fallbackName is non-empty then
> + * the whole search is repeated for \a fallbackName.
> + *
>   * \return The full path to the IPA configuration file, or an empty string if
>   * no configuration file can be found
>   */
> -std::string IPAProxy::configurationFile(const std::string &name) const
> +std::string IPAProxy::configurationFile(
> +	const std::string &name,
> +	const std::string &fallbackName) const

std::string IPAProxy::configurationFile(const std::string &name,
					const std::string &fallbackName) const

>  {
>  	struct stat statbuf;
>  	int ret;
> @@ -146,9 +152,17 @@ std::string IPAProxy::configurationFile(const std::string &name) const
>  		}
>  	}
>  
> -	LOG(IPAProxy, Error)
> -		<< "Configuration file '" << name
> -		<< "' not found for IPA module '" << ipaName << "'";
> +	if (fallbackName.empty()) {
> +		LOG(IPAProxy, Error)
> +			<< "Configuration file '" << name
> +			<< "' not found for IPA module '" << ipaName << "'";

		return std::string();

> +	} else {
> +		LOG(IPAProxy, Warning)
> +			<< "Configuration file '" << name
> +			<< "' not found for IPA module '" << ipaName
> +			<< "', trying '" << fallbackName << "'";

s/trying/falling back to/

> +		return configurationFile(fallbackName);

and drop and else and lower the indentation here.

Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

> +	}
>  
>  	return std::string();
>  }
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index 066fd4a2..2071c338 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -1186,9 +1186,8 @@ int IPU3CameraData::loadIPA()
>  	 * The API tuning file is made from the sensor name. If the tuning file
>  	 * isn't found, fall back to the 'uncalibrated' file.
>  	 */
> -	std::string ipaTuningFile = ipa_->configurationFile(sensor->model() + ".yaml");
> -	if (ipaTuningFile.empty())
> -		ipaTuningFile = ipa_->configurationFile("uncalibrated.yaml");
> +	std::string ipaTuningFile =
> +		ipa_->configurationFile(sensor->model() + ".yaml", "uncalibrated.yaml");
>  
>  	ret = ipa_->init(IPASettings{ ipaTuningFile, sensor->model() },
>  			 sensorInfo, sensor->controls(), &ipaControls_);
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index 97cd78a7..c3dae003 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -350,13 +350,8 @@ int RkISP1CameraData::loadIPA(unsigned int hwRevision)
>  	std::string ipaTuningFile;
>  	char const *configFromEnv = utils::secure_getenv("LIBCAMERA_RKISP1_TUNING_FILE");
>  	if (!configFromEnv || *configFromEnv == '\0') {
> -		ipaTuningFile = ipa_->configurationFile(sensor_->model() + ".yaml");
> -		/*
> -		 * If the tuning file isn't found, fall back to the
> -		 * 'uncalibrated' configuration file.
> -		 */
> -		if (ipaTuningFile.empty())
> -			ipaTuningFile = ipa_->configurationFile("uncalibrated.yaml");
> +		ipaTuningFile =
> +			ipa_->configurationFile(sensor_->model() + ".yaml", "uncalibrated.yaml");
>  	} else {
>  		ipaTuningFile = std::string(configFromEnv);
>  	}
> diff --git a/src/libcamera/software_isp/software_isp.cpp b/src/libcamera/software_isp/software_isp.cpp
> index 20fb6f48..aeef28e8 100644
> --- a/src/libcamera/software_isp/software_isp.cpp
> +++ b/src/libcamera/software_isp/software_isp.cpp
> @@ -121,9 +121,8 @@ SoftwareIsp::SoftwareIsp(PipelineHandler *pipe, const CameraSensor *sensor)
>  	 * The API tuning file is made from the sensor name. If the tuning file
>  	 * isn't found, fall back to the 'uncalibrated' file.
>  	 */
> -	std::string ipaTuningFile = ipa_->configurationFile(sensor->model() + ".yaml");
> -	if (ipaTuningFile.empty())
> -		ipaTuningFile = ipa_->configurationFile("uncalibrated.yaml");
> +	std::string ipaTuningFile =
> +		ipa_->configurationFile(sensor->model() + ".yaml", "uncalibrated.yaml");
>  
>  	int ret = ipa_->init(IPASettings{ ipaTuningFile, sensor->model() },
>  			     debayer_->getStatsFD(),

-- 
Regards,

Laurent Pinchart