[PATCH v2 3/3] libcamera: ipa_proxy: Report a missing configuration as a warning
Kieran Bingham
kieran.bingham at ideasonboard.com
Wed Jul 31 13:42:40 CEST 2024
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 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>
Reviewed-by: Kieran Bingham <kieran.bingham 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
More information about the libcamera-devel
mailing list