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

[Kieran Bingham]

Quoting Laurent Pinchart (2025-01-24 20:33:17)
> 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.
> > > 

Centralising all this was going to be my review comment to Stefan's
patch, 

> > > 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 :-)

I find almost every use of std::transform in libcamera hard to read :_(

but whatever works,


Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>

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