| Message ID | 20240708123803.1006689-4-mzamazal@redhat.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
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@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@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(),
Quoting Laurent Pinchart (2024-07-31 10:46:59) > 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@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@ideasonboard.com> Reviewed-by: Kieran Bingham <kieran.bingham@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
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; 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 { 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 << "'"; + } else { + LOG(IPAProxy, Warning) + << "Configuration file '" << name + << "' not found for IPA module '" << ipaName + << "', trying '" << fallbackName << "'"; + return configurationFile(fallbackName); + } 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(),
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@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(-)