[PATCH v3 8/9] libcamera: software_isp: Track whether CCM is enabled

Kieran Bingham kieran.bingham at ideasonboard.com
Thu Jan 9 18:32:35 CET 2025


Quoting Milan Zamazal (2024-12-10 15:34:38)
> Applying color correction matrix (CCM) in software ISP is optional due
> to performance reasons.  CCM is applied if and only if `Ccm' algorithm
> is present in the tuning file and it defines some CCM.
> Software ISP debayering is a performance critical piece of code and we
> do not want to use dynamic conditionals there.  Therefore we pass
> information about CCM application to debayering configuration and let it
> select the right versions of debayering functions using templates.  This
> is a similar trick as the previously used one for adding or not adding
> an alpha channel to the output.
> 
> Debayering gets this information but it ignores it in this patch.
> Actual processing with CCM is added in the followup patch.
> 
> Signed-off-by: Milan Zamazal <mzamazal at redhat.com>
> ---
>  include/libcamera/ipa/soft.mojom            |  2 +-
>  src/ipa/simple/algorithms/ccm.cpp           | 18 +++++------
>  src/ipa/simple/algorithms/ccm.h             |  1 +
>  src/ipa/simple/soft_simple.cpp              |  8 +++--
>  src/libcamera/software_isp/debayer.cpp      |  3 +-
>  src/libcamera/software_isp/debayer.h        |  3 +-
>  src/libcamera/software_isp/debayer_cpu.cpp  | 35 ++++++++++++---------
>  src/libcamera/software_isp/debayer_cpu.h    | 24 +++++++-------
>  src/libcamera/software_isp/software_isp.cpp |  5 +--
>  9 files changed, 56 insertions(+), 43 deletions(-)
> 
> diff --git a/include/libcamera/ipa/soft.mojom b/include/libcamera/ipa/soft.mojom
> index d52e6f1a..b5ed3905 100644
> --- a/include/libcamera/ipa/soft.mojom
> +++ b/include/libcamera/ipa/soft.mojom
> @@ -21,7 +21,7 @@ interface IPASoftInterface {
>         start() => (int32 ret);
>         stop();
>         configure(IPAConfigInfo configInfo)
> -               => (int32 ret);
> +               => (int32 ret, bool ccmEnabled);
>  
>         [async] queueRequest(uint32 frame, libcamera.ControlList sensorControls);
>         [async] computeParams(uint32 frame);
> diff --git a/src/ipa/simple/algorithms/ccm.cpp b/src/ipa/simple/algorithms/ccm.cpp
> index 40fe12c8..26bf8ff1 100644
> --- a/src/ipa/simple/algorithms/ccm.cpp
> +++ b/src/ipa/simple/algorithms/ccm.cpp
> @@ -25,26 +25,24 @@ unsigned int Ccm::kTemperatureThreshold = 100;
>  int Ccm::init([[maybe_unused]] IPAContext &context, const YamlObject &tuningData)
>  {
>         int ret = ccm_.readYaml(tuningData["ccms"], "ct", "ccm");
> -       if (ret < 0) {
> +       if (ret < 0)
>                 LOG(IPASoftCcm, Warning)
>                         << "Failed to parse 'ccm' "
>                         << "parameter from tuning file; falling back to unit matrix";
> -               ccmEnabled_ = false;
> -       } else {
> -               ccmEnabled_ = true;
> -       }

I'm not seeing why we're not tracking this...

>  
>         return 0;
>  }
>  
> +int Ccm::configure(IPAContext &context,
> +                  [[maybe_unused]] const IPAConfigInfo &configInfo)
> +{
> +       context.activeState.ccm.enabled = true;
> +       return 0;
> +}
> +
>  void Ccm::prepare(IPAContext &context, const uint32_t frame,
>                   IPAFrameContext &frameContext, [[maybe_unused]] DebayerParams *params)
>  {
> -       context.activeState.ccm.enabled = ccmEnabled_;
> -
> -       if (!ccmEnabled_)
> -               return;
> -

I'm not sure I understand why we're not returning and doing nothing if !ccmEnabled_

>         unsigned int ct = context.activeState.awb.temperatureK;
>  
>         /* Change CCM only on bigger temperature changes. */
> diff --git a/src/ipa/simple/algorithms/ccm.h b/src/ipa/simple/algorithms/ccm.h
> index 23481a08..fe81ebd5 100644
> --- a/src/ipa/simple/algorithms/ccm.h
> +++ b/src/ipa/simple/algorithms/ccm.h
> @@ -24,6 +24,7 @@ public:
>         ~Ccm() = default;
>  
>         int init(IPAContext &context, const YamlObject &tuningData) override;
> +       int configure(IPAContext &context, const IPAConfigInfo &configInfo) override;
>         void prepare(IPAContext &context,
>                      const uint32_t frame,
>                      IPAFrameContext &frameContext,
> diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp
> index b26e4e15..545773fa 100644
> --- a/src/ipa/simple/soft_simple.cpp
> +++ b/src/ipa/simple/soft_simple.cpp
> @@ -52,7 +52,7 @@ public:
>                  const SharedFD &fdParams,
>                  const ControlInfoMap &sensorInfoMap,
>                  ControlInfoMap *ipaControls) override;
> -       int configure(const IPAConfigInfo &configInfo) override;
> +       int configure(const IPAConfigInfo &configInfo, bool *ccmEnabled) override;
>  
>         int start() override;
>         void stop() override;
> @@ -182,7 +182,7 @@ int IPASoftSimple::init(const IPASettings &settings,
>         return 0;
>  }
>  
> -int IPASoftSimple::configure(const IPAConfigInfo &configInfo)
> +int IPASoftSimple::configure(const IPAConfigInfo &configInfo, bool *ccmEnabled)
>  {
>         sensorInfoMap_ = configInfo.sensorControls;
>  
> @@ -242,12 +242,16 @@ int IPASoftSimple::configure(const IPAConfigInfo &configInfo)
>                 context_.configuration.agc.againMinStep = 1.0;
>         }
>  
> +       context_.activeState.ccm.enabled = false;

I would hope/expect this to be initialised to false already.

> +
>         for (auto const &algo : algorithms()) {
>                 int ret = algo->configure(context_, configInfo);
>                 if (ret)
>                         return ret;
>         }
>  
> +       *ccmEnabled = context_.activeState.ccm.enabled;

And if there's no ccm enabled in the tuning file - it should never
update it so it would still be possible to use this line, without
changing the current state tracking in the ccm module itself.

> +
>         LOG(IPASoft, Info)
>                 << "Exposure " << context_.configuration.agc.exposureMin << "-"
>                 << context_.configuration.agc.exposureMax
> diff --git a/src/libcamera/software_isp/debayer.cpp b/src/libcamera/software_isp/debayer.cpp
> index f0b83261..595f73f6 100644
> --- a/src/libcamera/software_isp/debayer.cpp
> +++ b/src/libcamera/software_isp/debayer.cpp
> @@ -57,10 +57,11 @@ Debayer::~Debayer()
>  }
>  
>  /**
> - * \fn int Debayer::configure(const StreamConfiguration &inputCfg, const std::vector<std::reference_wrapper<StreamConfiguration>> &outputCfgs)
> + * \fn int Debayer::configure(const StreamConfiguration &inputCfg, const std::vector<std::reference_wrapper<StreamConfiguration>> &outputCfgs, ccmEnabled)
>   * \brief Configure the debayer object according to the passed in parameters
>   * \param[in] inputCfg The input configuration
>   * \param[in] outputCfgs The output configurations
> + * \param[in] ccmEnabled Whether a color correction matrix is applied
>   *
>   * \return 0 on success, a negative errno on failure
>   */
> diff --git a/src/libcamera/software_isp/debayer.h b/src/libcamera/software_isp/debayer.h
> index d7ca060d..ba033d44 100644
> --- a/src/libcamera/software_isp/debayer.h
> +++ b/src/libcamera/software_isp/debayer.h
> @@ -33,7 +33,8 @@ public:
>         virtual ~Debayer() = 0;
>  
>         virtual int configure(const StreamConfiguration &inputCfg,
> -                             const std::vector<std::reference_wrapper<StreamConfiguration>> &outputCfgs) = 0;
> +                             const std::vector<std::reference_wrapper<StreamConfiguration>> &outputCfgs,
> +                             bool ccmEnabled) = 0;
>  
>         virtual std::vector<PixelFormat> formats(PixelFormat inputFormat) = 0;
>  
> diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp
> index 01cfb36b..3c6597f9 100644
> --- a/src/libcamera/software_isp/debayer_cpu.cpp
> +++ b/src/libcamera/software_isp/debayer_cpu.cpp
> @@ -114,7 +114,7 @@ DebayerCpu::~DebayerCpu() = default;
>                 (prev[x] + curr[x - p] + curr[x + n] + next[x]) / (4 * (div)),         \
>                 curr[x] / (div))
>  
> -template<bool addAlphaByte>
> +template<bool addAlphaByte, bool ccmEnabled>
>  void DebayerCpu::debayer8_BGBG_BGR888(uint8_t *dst, const uint8_t *src[])
>  {
>         DECLARE_SRC_POINTERS(uint8_t)
> @@ -125,7 +125,7 @@ void DebayerCpu::debayer8_BGBG_BGR888(uint8_t *dst, const uint8_t *src[])
>         }
>  }
>  
> -template<bool addAlphaByte>
> +template<bool addAlphaByte, bool ccmEnabled>
>  void DebayerCpu::debayer8_GRGR_BGR888(uint8_t *dst, const uint8_t *src[])
>  {
>         DECLARE_SRC_POINTERS(uint8_t)
> @@ -136,7 +136,7 @@ void DebayerCpu::debayer8_GRGR_BGR888(uint8_t *dst, const uint8_t *src[])
>         }
>  }
>  
> -template<bool addAlphaByte>
> +template<bool addAlphaByte, bool ccmEnabled>
>  void DebayerCpu::debayer10_BGBG_BGR888(uint8_t *dst, const uint8_t *src[])
>  {
>         DECLARE_SRC_POINTERS(uint16_t)
> @@ -148,7 +148,7 @@ void DebayerCpu::debayer10_BGBG_BGR888(uint8_t *dst, const uint8_t *src[])
>         }
>  }
>  
> -template<bool addAlphaByte>
> +template<bool addAlphaByte, bool ccmEnabled>
>  void DebayerCpu::debayer10_GRGR_BGR888(uint8_t *dst, const uint8_t *src[])
>  {
>         DECLARE_SRC_POINTERS(uint16_t)
> @@ -160,7 +160,7 @@ void DebayerCpu::debayer10_GRGR_BGR888(uint8_t *dst, const uint8_t *src[])
>         }
>  }
>  
> -template<bool addAlphaByte>
> +template<bool addAlphaByte, bool ccmEnabled>
>  void DebayerCpu::debayer12_BGBG_BGR888(uint8_t *dst, const uint8_t *src[])
>  {
>         DECLARE_SRC_POINTERS(uint16_t)
> @@ -172,7 +172,7 @@ void DebayerCpu::debayer12_BGBG_BGR888(uint8_t *dst, const uint8_t *src[])
>         }
>  }
>  
> -template<bool addAlphaByte>
> +template<bool addAlphaByte, bool ccmEnabled>
>  void DebayerCpu::debayer12_GRGR_BGR888(uint8_t *dst, const uint8_t *src[])
>  {
>         DECLARE_SRC_POINTERS(uint16_t)
> @@ -184,7 +184,7 @@ void DebayerCpu::debayer12_GRGR_BGR888(uint8_t *dst, const uint8_t *src[])
>         }
>  }
>  
> -template<bool addAlphaByte>
> +template<bool addAlphaByte, bool ccmEnabled>
>  void DebayerCpu::debayer10P_BGBG_BGR888(uint8_t *dst, const uint8_t *src[])
>  {
>         const int widthInBytes = window_.width * 5 / 4;
> @@ -210,7 +210,7 @@ void DebayerCpu::debayer10P_BGBG_BGR888(uint8_t *dst, const uint8_t *src[])
>         }
>  }
>  
> -template<bool addAlphaByte>
> +template<bool addAlphaByte, bool ccmEnabled>
>  void DebayerCpu::debayer10P_GRGR_BGR888(uint8_t *dst, const uint8_t *src[])
>  {
>         const int widthInBytes = window_.width * 5 / 4;
> @@ -231,7 +231,7 @@ void DebayerCpu::debayer10P_GRGR_BGR888(uint8_t *dst, const uint8_t *src[])
>         }
>  }
>  
> -template<bool addAlphaByte>
> +template<bool addAlphaByte, bool ccmEnabled>
>  void DebayerCpu::debayer10P_GBGB_BGR888(uint8_t *dst, const uint8_t *src[])
>  {
>         const int widthInBytes = window_.width * 5 / 4;
> @@ -252,7 +252,7 @@ void DebayerCpu::debayer10P_GBGB_BGR888(uint8_t *dst, const uint8_t *src[])
>         }
>  }
>  
> -template<bool addAlphaByte>
> +template<bool addAlphaByte, bool ccmEnabled>
>  void DebayerCpu::debayer10P_RGRG_BGR888(uint8_t *dst, const uint8_t *src[])
>  {
>         const int widthInBytes = window_.width * 5 / 4;
> @@ -368,10 +368,11 @@ int DebayerCpu::setupStandardBayerOrder(BayerFormat::Order order)
>         return 0;
>  }
>  
> -#define SET_DEBAYER_METHODS(method0, method1)                                                \
> -       debayer0_ = addAlphaByte ? &DebayerCpu::method0<true> : &DebayerCpu::method0<false>; \
> -       debayer1_ = addAlphaByte ? &DebayerCpu::method1<true> : &DebayerCpu::method1<false>;
> +#define SET_DEBAYER_METHODS(method0, method1)                                                                        \
> +       debayer0_ = addAlphaByte ? &DebayerCpu::method0<true, ccmEnabled> : &DebayerCpu::method0<false, ccmEnabled>; \
> +       debayer1_ = addAlphaByte ? &DebayerCpu::method1<true, ccmEnabled> : &DebayerCpu::method1<false, ccmEnabled>;
>  
> +template<bool ccmEnabled>
>  int DebayerCpu::setDebayerFunctions(PixelFormat inputFormat, PixelFormat outputFormat)
>  {
>         BayerFormat bayerFormat =
> @@ -464,7 +465,8 @@ int DebayerCpu::setDebayerFunctions(PixelFormat inputFormat, PixelFormat outputF
>  }
>  
>  int DebayerCpu::configure(const StreamConfiguration &inputCfg,
> -                         const std::vector<std::reference_wrapper<StreamConfiguration>> &outputCfgs)
> +                         const std::vector<std::reference_wrapper<StreamConfiguration>> &outputCfgs,
> +                         bool ccmEnabled)
>  {
>         if (getInputConfig(inputCfg.pixelFormat, inputConfig_) != 0)
>                 return -EINVAL;
> @@ -503,7 +505,10 @@ int DebayerCpu::configure(const StreamConfiguration &inputCfg,
>                 return -EINVAL;
>         }
>  
> -       if (setDebayerFunctions(inputCfg.pixelFormat, outputCfg.pixelFormat) != 0)
> +       int ret = ccmEnabled
> +                         ? setDebayerFunctions<true>(inputCfg.pixelFormat, outputCfg.pixelFormat)
> +                         : setDebayerFunctions<false>(inputCfg.pixelFormat, outputCfg.pixelFormat);
> +       if (ret != 0)
>                 return -EINVAL;
>  
>         window_.x = ((inputCfg.size.width - outputCfg.size.width) / 2) &
> diff --git a/src/libcamera/software_isp/debayer_cpu.h b/src/libcamera/software_isp/debayer_cpu.h
> index 2c47e7c6..b2ec8f1b 100644
> --- a/src/libcamera/software_isp/debayer_cpu.h
> +++ b/src/libcamera/software_isp/debayer_cpu.h
> @@ -31,7 +31,8 @@ public:
>         ~DebayerCpu();
>  
>         int configure(const StreamConfiguration &inputCfg,
> -                     const std::vector<std::reference_wrapper<StreamConfiguration>> &outputCfgs);
> +                     const std::vector<std::reference_wrapper<StreamConfiguration>> &outputCfgs,
> +                     bool ccmEnabled);
>         Size patternSize(PixelFormat inputFormat);
>         std::vector<PixelFormat> formats(PixelFormat input);
>         std::tuple<unsigned int, unsigned int>
> @@ -85,28 +86,28 @@ private:
>         using debayerFn = void (DebayerCpu::*)(uint8_t *dst, const uint8_t *src[]);
>  
>         /* 8-bit raw bayer format */
> -       template<bool addAlphaByte>
> +       template<bool addAlphaByte, bool ccmEnabled>
>         void debayer8_BGBG_BGR888(uint8_t *dst, const uint8_t *src[]);
> -       template<bool addAlphaByte>
> +       template<bool addAlphaByte, bool ccmEnabled>
>         void debayer8_GRGR_BGR888(uint8_t *dst, const uint8_t *src[]);
>         /* unpacked 10-bit raw bayer format */
> -       template<bool addAlphaByte>
> +       template<bool addAlphaByte, bool ccmEnabled>
>         void debayer10_BGBG_BGR888(uint8_t *dst, const uint8_t *src[]);
> -       template<bool addAlphaByte>
> +       template<bool addAlphaByte, bool ccmEnabled>
>         void debayer10_GRGR_BGR888(uint8_t *dst, const uint8_t *src[]);
>         /* unpacked 12-bit raw bayer format */
> -       template<bool addAlphaByte>
> +       template<bool addAlphaByte, bool ccmEnabled>
>         void debayer12_BGBG_BGR888(uint8_t *dst, const uint8_t *src[]);
> -       template<bool addAlphaByte>
> +       template<bool addAlphaByte, bool ccmEnabled>
>         void debayer12_GRGR_BGR888(uint8_t *dst, const uint8_t *src[]);
>         /* CSI-2 packed 10-bit raw bayer format (all the 4 orders) */
> -       template<bool addAlphaByte>
> +       template<bool addAlphaByte, bool ccmEnabled>
>         void debayer10P_BGBG_BGR888(uint8_t *dst, const uint8_t *src[]);
> -       template<bool addAlphaByte>
> +       template<bool addAlphaByte, bool ccmEnabled>
>         void debayer10P_GRGR_BGR888(uint8_t *dst, const uint8_t *src[]);
> -       template<bool addAlphaByte>
> +       template<bool addAlphaByte, bool ccmEnabled>
>         void debayer10P_GBGB_BGR888(uint8_t *dst, const uint8_t *src[]);
> -       template<bool addAlphaByte>
> +       template<bool addAlphaByte, bool ccmEnabled>
>         void debayer10P_RGRG_BGR888(uint8_t *dst, const uint8_t *src[]);
>  
>         struct DebayerInputConfig {
> @@ -125,6 +126,7 @@ private:
>         int getInputConfig(PixelFormat inputFormat, DebayerInputConfig &config);
>         int getOutputConfig(PixelFormat outputFormat, DebayerOutputConfig &config);
>         int setupStandardBayerOrder(BayerFormat::Order order);
> +       template<bool ccmEnabled>
>         int setDebayerFunctions(PixelFormat inputFormat, PixelFormat outputFormat);
>         void setupInputMemcpy(const uint8_t *linePointers[]);
>         void shiftLinePointers(const uint8_t *linePointers[], const uint8_t *src);
> diff --git a/src/libcamera/software_isp/software_isp.cpp b/src/libcamera/software_isp/software_isp.cpp
> index 2bea64d9..6a07de85 100644
> --- a/src/libcamera/software_isp/software_isp.cpp
> +++ b/src/libcamera/software_isp/software_isp.cpp
> @@ -236,11 +236,12 @@ int SoftwareIsp::configure(const StreamConfiguration &inputCfg,
>  {
>         ASSERT(ipa_ && debayer_);
>  
> -       int ret = ipa_->configure(configInfo);
> +       bool ccmEnabled;
> +       int ret = ipa_->configure(configInfo, &ccmEnabled);
>         if (ret < 0)
>                 return ret;
>  
> -       return debayer_->configure(inputCfg, outputCfgs);
> +       return debayer_->configure(inputCfg, outputCfgs, ccmEnabled);
>  }
>  
>  /**
> -- 
> 2.44.2
>


More information about the libcamera-devel mailing list