[v2,26/35] pipeline: rkisp1: Enable the dewarper based on the tuning file

24753 diff mbox series

To do actual lens dewarping, the dewarper will be configured based on
the tuning file. As a first step implement the basic loading of the
tuning file and enable/disable the dewarper for the given camera based
on the existence of the "Dewarp" algorithm in the tuning file.

Todo: This is an backwards incompatible change in that the dewarper is
currently included in the chain unconditionally. Some users may want to
not use the dewarper, so it is sensible to make that configurable. If it
should be in the algorithms section or in a different one is open for
debate.

Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>

---

Changes in v2:
- Drop unused params variable
- Moved patch a bit earlier so canUseDewarper is available for handling
  the orientation
---
 src/ipa/libipa/module.h                  |  4 ++
 src/libcamera/pipeline/rkisp1/rkisp1.cpp | 48 +++++++++++++++++++++++-
 2 files changed, 51 insertions(+), 1 deletion(-)

Quoting Stefan Klug (2025-10-23 23:48:27)
> To do actual lens dewarping, the dewarper will be configured based on
> the tuning file. As a first step implement the basic loading of the
> tuning file and enable/disable the dewarper for the given camera based
> on the existence of the "Dewarp" algorithm in the tuning file.
> 
> Todo: This is an backwards incompatible change in that the dewarper is
> currently included in the chain unconditionally. Some users may want to
> not use the dewarper, so it is sensible to make that configurable. If it
> should be in the algorithms section or in a different one is open for
> debate.

It indeed does not feel like an algorithm; it's part of the pipeline that
implements ScalerCrop... although I suppose in the future it will do actual
dewarping and then it will become an algorithm. Thus I think it's fine to put
in the algorithms section.

As for backwards compatibility, I would imagine that most users would want the
dewarper on, but so do they other algorithms that need to be specified in the
tuning file to be enabled. Therefore I think it's fine do be backwards
incompatible. It's debatable whether or not it's an API change (I would think
not). If you're worried about users complaining we could add a warning for a
couple versions?

> 
> Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>

I'm not going to bikeshed variable names; they're good enough :)

Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>

> 
> ---
> 
> Changes in v2:
> - Drop unused params variable
> - Moved patch a bit earlier so canUseDewarper is available for handling
>   the orientation
> ---
>  src/ipa/libipa/module.h                  |  4 ++
>  src/libcamera/pipeline/rkisp1/rkisp1.cpp | 48 +++++++++++++++++++++++-
>  2 files changed, 51 insertions(+), 1 deletion(-)
> 
> diff --git a/src/ipa/libipa/module.h b/src/ipa/libipa/module.h
> index 0fb51916fff6..84386f901534 100644
> --- a/src/ipa/libipa/module.h
> +++ b/src/ipa/libipa/module.h
> @@ -74,6 +74,10 @@ private:
>         int createAlgorithm(Context &context, const YamlObject &data)
>         {
>                 const auto &[name, algoData] = *data.asDict().begin();
> +
> +               if (name == "Dewarp")
> +                       return 0;
> +
>                 std::unique_ptr<Algorithm<Module>> algo = createAlgorithm(name);
>                 if (!algo) {
>                         LOG(IPAModuleAlgo, Error)
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index d7bb30f20668..943f26ece974 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -16,6 +16,7 @@
>  #include <linux/media-bus-format.h>
>  #include <linux/rkisp1-config.h>
>  
> +#include <libcamera/base/file.h>
>  #include <libcamera/base/log.h>
>  #include <libcamera/base/utils.h>
>  
> @@ -47,6 +48,7 @@
>  #include "libcamera/internal/v4l2_request.h"
>  #include "libcamera/internal/v4l2_subdevice.h"
>  #include "libcamera/internal/v4l2_videodevice.h"
> +#include "libcamera/internal/yaml_parser.h"
>  
>  #include "rkisp1_path.h"
>  
> @@ -123,6 +125,7 @@ public:
>          */
>         MediaPipeline pipe_;
>  
> +       bool canUseDewarper_;
>         bool usesDewarper_;
>  
>  private:
> @@ -131,6 +134,7 @@ private:
>                                const ControlList &sensorControls);
>  
>         void metadataReady(unsigned int frame, const ControlList &metadata);
> +       int loadTuningFile(const std::string &file);
>  };
>  
>  class RkISP1CameraConfiguration : public CameraConfiguration
> @@ -416,6 +420,48 @@ int RkISP1CameraData::loadIPA(unsigned int hwRevision, uint32_t supportedBlocks)
>                 return ret;
>         }
>  
> +       ret = loadTuningFile(ipaTuningFile);
> +       if (ret < 0) {
> +               LOG(RkISP1, Error) << "Failed to load tuning file";
> +               return ret;
> +       }
> +
> +       return 0;
> +}
> +
> +int RkISP1CameraData::loadTuningFile(const std::string &path)
> +{
> +       if (!pipe()->dewarper_)
> +               /* Nothing to do without dewarper */
> +               return 0;
> +
> +       LOG(RkISP1, Debug) << "Load tuning file " << path;
> +
> +       File file(path);
> +       if (!file.open(File::OpenModeFlag::ReadOnly)) {
> +               int ret = file.error();
> +               LOG(RkISP1, Error)
> +                       << "Failed to open tuning file "
> +                       << path << ": " << strerror(-ret);
> +               return ret;
> +       }
> +
> +       std::unique_ptr<libcamera::YamlObject> data = YamlParser::parse(file);
> +       if (!data)
> +               return -EINVAL;
> +
> +       if (!data->contains("algorithms")) {
> +               LOG(RkISP1, Error)
> +                       << "Tuning file doesn't contain any algorithm";
> +               return -EINVAL;
> +       }
> +
> +       const auto &algos = (*data)["algorithms"].asList();
> +       for (const auto &algo : algos) {
> +               if (algo.contains("Dewarp"))
> +                       canUseDewarper_ = true;
> +       }
> +
>         return 0;
>  }
>  
> @@ -573,7 +619,7 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()
>         }
>  
>         bool useDewarper = false;
> -       if (pipe->dewarper_ && !isRaw)
> +       if (data_->canUseDewarper_ && !isRaw)
>                 useDewarper = true;
>  
>         /*
> -- 
> 2.48.1
>