[RFC,6/7] libcamera: ipa: simple: Separate saturation from CCM

25004 diff mbox series

Saturation adjustments are implemented using matrix operations.  They
are currently applied to the colour correction matrix.  Let's move them
to a newly introduced separate "Adjust" algorithm.

This separation has the following advantages:

- It allows disabling general colour adjustments algorithms without
  disabling the CCM algorithm.

- It keeps the CCM separated from other corrections.

- It's generally cleaner.

Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
---
 src/ipa/simple/algorithms/adjust.cpp  | 111 ++++++++++++++++++++++++++
 src/ipa/simple/algorithms/adjust.h    |  52 ++++++++++++
 src/ipa/simple/algorithms/ccm.cpp     |  58 +-------------
 src/ipa/simple/algorithms/ccm.h       |  10 +--
 src/ipa/simple/algorithms/meson.build |   1 +
 src/ipa/simple/data/uncalibrated.yaml |   1 +
 6 files changed, 169 insertions(+), 64 deletions(-)
 create mode 100644 src/ipa/simple/algorithms/adjust.cpp
 create mode 100644 src/ipa/simple/algorithms/adjust.h

Quoting Milan Zamazal (2025-11-12 08:27:14)
> Saturation adjustments are implemented using matrix operations.  They
> are currently applied to the colour correction matrix.  Let's move them
> to a newly introduced separate "Adjust" algorithm.
> 
> This separation has the following advantages:
> 
> - It allows disabling general colour adjustments algorithms without
>   disabling the CCM algorithm.
> 
> - It keeps the CCM separated from other corrections.
> 
> - It's generally cleaner.
> 
> Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
> ---
>  src/ipa/simple/algorithms/adjust.cpp  | 111 ++++++++++++++++++++++++++
>  src/ipa/simple/algorithms/adjust.h    |  52 ++++++++++++
>  src/ipa/simple/algorithms/ccm.cpp     |  58 +-------------
>  src/ipa/simple/algorithms/ccm.h       |  10 +--
>  src/ipa/simple/algorithms/meson.build |   1 +
>  src/ipa/simple/data/uncalibrated.yaml |   1 +
>  6 files changed, 169 insertions(+), 64 deletions(-)
>  create mode 100644 src/ipa/simple/algorithms/adjust.cpp
>  create mode 100644 src/ipa/simple/algorithms/adjust.h
> 
> diff --git a/src/ipa/simple/algorithms/adjust.cpp b/src/ipa/simple/algorithms/adjust.cpp
> new file mode 100644
> index 000000000..282b3ccb0
> --- /dev/null
> +++ b/src/ipa/simple/algorithms/adjust.cpp
> @@ -0,0 +1,111 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2024, Ideas On Board
> + * Copyright (C) 2024-2025, Red Hat Inc.
> + *
> + * Common image adjustments
> + */
> +
> +#include "adjust.h"
> +
> +#include <libcamera/base/log.h>
> +#include <libcamera/base/utils.h>
> +
> +#include <libcamera/control_ids.h>
> +
> +#include "libcamera/internal/matrix.h"
> +
> +namespace libcamera {
> +
> +namespace ipa::soft::algorithms {
> +
> +LOG_DEFINE_CATEGORY(IPASoftAdjust)
> +
> +int Adjust::init(IPAContext &context, [[maybe_unused]] const YamlObject &tuningData)
> +{
> +       if (context.ccmEnabled)
> +               context.ctrlMap[&controls::Saturation] = ControlInfo(0.0f, 2.0f, 1.0f);

Nice, I like the tight coupling of the controls to where they are
provided, and only reported if they are possible.


> +       return 0;
> +}
> +
> +int Adjust::configure(IPAContext &context,
> +                     [[maybe_unused]] const IPAConfigInfo &configInfo)
> +{
> +       context.activeState.knobs.saturation = std::optional<double>();
> +       context.activeState.correctionMatrix =
> +               Matrix<float, 3, 3>{ { 1, 0, 0, 0, 1, 0, 0, 0, 1 } };

Real nit pick but can we style this in three lines ? Does it help?

		Matrix<float, 3, 3>{ { 1, 0, 0,
				       0, 1, 0,
				       0, 0, 1 }
		};

or 
		Matrix<float, 3, 3>{
			{ 1, 0, 0,
			  0, 1, 0,
			  0, 0, 1 }
		};

?

> +
> +       return 0;
> +}
> +
> +void Adjust::queueRequest(typename Module::Context &context,
> +                         [[maybe_unused]] const uint32_t frame,
> +                         [[maybe_unused]] typename Module::FrameContext &frameContext,
> +                         const ControlList &controls)
> +{
> +       const auto &saturation = controls.get(controls::Saturation);
> +       if (saturation.has_value()) {
> +               context.activeState.knobs.saturation = saturation;
> +               LOG(IPASoftAdjust, Debug) << "Setting saturation to " << saturation.value();
> +       }
> +}
> +
> +void Adjust::applySaturation(Matrix<float, 3, 3> &matrix, float saturation)
> +{
> +       /* https://en.wikipedia.org/wiki/YCbCr#ITU-R_BT.601_conversion */
> +       const Matrix<float, 3, 3> rgb2ycbcr{
> +               { 0.256788235294, 0.504129411765, 0.0979058823529,
> +                 -0.148223529412, -0.290992156863, 0.439215686275,
> +                 0.439215686275, -0.367788235294, -0.0714274509804 }
> +       };
> +       const Matrix<float, 3, 3> ycbcr2rgb{
> +               { 1.16438356164, 0, 1.59602678571,
> +                 1.16438356164, -0.391762290094, -0.812967647235,
> +                 1.16438356164, 2.01723214285, 0 }
> +       };

I think this is duplicating these const Matrix definitions from the ccm
file now.

Can you move them to src/ipa/libipa/colours{.cpp,.h} as a pre-cursor
please?

Perhaps they need to be in a colours::bt601::rgb2ycbcr,ycbcr2rgb
namespace if we expect bt709,bt2020 versions or such to be explicit.

Edit: Discovered this is a move, not a duplication see below:

> +       const Matrix<float, 3, 3> saturationMatrix{
> +               { 1, 0, 0,
> +                 0, saturation, 0,
> +                 0, 0, saturation }
> +       };
> +       matrix =
> +               ycbcr2rgb * saturationMatrix * rgb2ycbcr * matrix;
> +}
> +
> +void Adjust::prepare(IPAContext &context,
> +                    const uint32_t frame,
> +                    IPAFrameContext &frameContext,
> +                    [[maybe_unused]] DebayerParams *params)
> +{
> +       if (!context.ccmEnabled)
> +               return;
> +
> +       auto &saturation = context.activeState.knobs.saturation;
> +
> +       if (frame > 0 && saturation == lastSaturation_)
> +               return;
> +
> +       context.activeState.correctionMatrix = context.activeState.ccm;
> +       context.activeState.matrixChanged = true;
> +       lastSaturation_ = saturation;
> +       if (saturation)
> +               applySaturation(context.activeState.correctionMatrix, saturation.value());
> +
> +       frameContext.saturation = saturation;
> +}
> +
> +void Adjust::process([[maybe_unused]] IPAContext &context,
> +                    [[maybe_unused]] const uint32_t frame,
> +                    IPAFrameContext &frameContext,
> +                    [[maybe_unused]] const SwIspStats *stats,
> +                    ControlList &metadata)
> +{
> +       const auto &saturation = frameContext.saturation;
> +       metadata.set(controls::Saturation, saturation.value_or(1.0));
> +}
> +
> +REGISTER_IPA_ALGORITHM(Adjust, "Adjust")
> +
> +} /* namespace ipa::soft::algorithms */
> +
> +} /* namespace libcamera */
> diff --git a/src/ipa/simple/algorithms/adjust.h b/src/ipa/simple/algorithms/adjust.h
> new file mode 100644
> index 000000000..c4baa2503
> --- /dev/null
> +++ b/src/ipa/simple/algorithms/adjust.h
> @@ -0,0 +1,52 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2024-2025, Red Hat Inc.
> + *
> + * Color correction matrix
> + */
> +
> +#pragma once
> +
> +#include <optional>
> +
> +#include "libcamera/internal/matrix.h"
> +
> +#include <libipa/interpolator.h>
> +
> +#include "algorithm.h"
> +
> +namespace libcamera {
> +
> +namespace ipa::soft::algorithms {
> +
> +class Adjust : public Algorithm
> +{
> +public:
> +       Adjust() = default;
> +       ~Adjust() = default;
> +
> +       int init(IPAContext &context, const YamlObject &tuningData) override;
> +       int configure(IPAContext &context,
> +                     const IPAConfigInfo &configInfo) override;
> +       void queueRequest(typename Module::Context &context,
> +                         const uint32_t frame,
> +                         typename Module::FrameContext &frameContext,
> +                         const ControlList &controls) override;
> +       void prepare(IPAContext &context,
> +                    const uint32_t frame,
> +                    IPAFrameContext &frameContext,
> +                    DebayerParams *params) override;
> +       void process(IPAContext &context, const uint32_t frame,
> +                    IPAFrameContext &frameContext,
> +                    const SwIspStats *stats,
> +                    ControlList &metadata) override;
> +
> +private:
> +       void applySaturation(Matrix<float, 3, 3> &ccm, float saturation);
> +
> +       std::optional<float> lastSaturation_;
> +};
> +
> +} /* namespace ipa::soft::algorithms */
> +
> +} /* namespace libcamera */
> diff --git a/src/ipa/simple/algorithms/ccm.cpp b/src/ipa/simple/algorithms/ccm.cpp
> index 3e9528a91..c541ef466 100644
> --- a/src/ipa/simple/algorithms/ccm.cpp
> +++ b/src/ipa/simple/algorithms/ccm.cpp
> @@ -3,7 +3,7 @@
>   * Copyright (C) 2024, Ideas On Board
>   * Copyright (C) 2024-2025, Red Hat Inc.
>   *
> - * Color correction matrix + saturation
> + * Color correction matrix

I very much like this ;-)


>   */
>  
>  #include "ccm.h"
> @@ -37,76 +37,27 @@ int Ccm::init([[maybe_unused]] IPAContext &context, const YamlObject &tuningData
>         }
>  
>         context.ccmEnabled = true;
> -       context.ctrlMap[&controls::Saturation] = ControlInfo(0.0f, 2.0f, 1.0f);
>  
>         return 0;
>  }
>  
> -int Ccm::configure(IPAContext &context,
> -                  [[maybe_unused]] const IPAConfigInfo &configInfo)
> -{
> -       context.activeState.knobs.saturation = std::optional<double>();
> -
> -       return 0;
> -}
> -
> -void Ccm::queueRequest(typename Module::Context &context,
> -                      [[maybe_unused]] const uint32_t frame,
> -                      [[maybe_unused]] typename Module::FrameContext &frameContext,
> -                      const ControlList &controls)
> -{
> -       const auto &saturation = controls.get(controls::Saturation);
> -       if (saturation.has_value()) {
> -               context.activeState.knobs.saturation = saturation;
> -               LOG(IPASoftCcm, Debug) << "Setting saturation to " << saturation.value();
> -       }
> -}
> -
> -void Ccm::applySaturation(Matrix<float, 3, 3> &ccm, float saturation)
> -{
> -       /* https://en.wikipedia.org/wiki/YCbCr#ITU-R_BT.601_conversion */
> -       const Matrix<float, 3, 3> rgb2ycbcr{
> -               { 0.256788235294, 0.504129411765, 0.0979058823529,
> -                 -0.148223529412, -0.290992156863, 0.439215686275,
> -                 0.439215686275, -0.367788235294, -0.0714274509804 }
> -       };
> -       const Matrix<float, 3, 3> ycbcr2rgb{
> -               { 1.16438356164, 0, 1.59602678571,
> -                 1.16438356164, -0.391762290094, -0.812967647235,
> -                 1.16438356164, 2.01723214285, 0 }
> -       };
> -       const Matrix<float, 3, 3> saturationMatrix{
> -               { 1, 0, 0,
> -                 0, saturation, 0,
> -                 0, 0, saturation }
> -       };
> -       ccm = ycbcr2rgb * saturationMatrix * rgb2ycbcr * ccm;
> -}


aha, never mind - we're not duplciating - we're moving. So ignore the
colours namespace requirement for now.


> -
>  void Ccm::prepare(IPAContext &context, const uint32_t frame,
>                   IPAFrameContext &frameContext, [[maybe_unused]] DebayerParams *params)
>  {
> -       auto &saturation = context.activeState.knobs.saturation;
> -
>         const unsigned int ct = context.activeState.awb.temperatureK;
>  
> -       /* Change CCM only on saturation or bigger temperature changes. */
> +       /* Change CCM only on bigger temperature changes. */
>         if (frame > 0 &&
> -           utils::abs_diff(ct, lastCt_) < kTemperatureThreshold &&
> -           saturation == lastSaturation_) {
> +           utils::abs_diff(ct, lastCt_) < kTemperatureThreshold) {
>                 frameContext.ccm = context.activeState.ccm;
>                 return;
>         }
>  
>         lastCt_ = ct;
> -       lastSaturation_ = saturation;
>         Matrix<float, 3, 3> ccm = ccm_.getInterpolated(ct);
> -       if (saturation)
> -               applySaturation(ccm, saturation.value());
>  
>         context.activeState.correctionMatrix = ccm;
>         context.activeState.ccm = ccm;
> -       frameContext.saturation = saturation;
>         context.activeState.matrixChanged = true;
>         frameContext.ccm = ccm;
>  }
> @@ -118,9 +69,6 @@ void Ccm::process([[maybe_unused]] IPAContext &context,
>                   ControlList &metadata)
>  {
>         metadata.set(controls::ColourCorrectionMatrix, frameContext.ccm.data());
> -
> -       const auto &saturation = frameContext.saturation;
> -       metadata.set(controls::Saturation, saturation.value_or(1.0));
>  }
>  
>  REGISTER_IPA_ALGORITHM(Ccm, "Ccm")
> diff --git a/src/ipa/simple/algorithms/ccm.h b/src/ipa/simple/algorithms/ccm.h
> index 8279a3d59..10c9829f5 100644
> --- a/src/ipa/simple/algorithms/ccm.h
> +++ b/src/ipa/simple/algorithms/ccm.h
> @@ -26,12 +26,6 @@ public:
>         ~Ccm() = default;
>  
>         int init(IPAContext &context, const YamlObject &tuningData) override;
> -       int configure(IPAContext &context,
> -                     const IPAConfigInfo &configInfo) override;
> -       void queueRequest(typename Module::Context &context,
> -                         const uint32_t frame,
> -                         typename Module::FrameContext &frameContext,
> -                         const ControlList &controls) override;
>         void prepare(IPAContext &context,
>                      const uint32_t frame,
>                      IPAFrameContext &frameContext,
> @@ -42,11 +36,9 @@ public:
>                      ControlList &metadata) override;
>  
>  private:
> -       void applySaturation(Matrix<float, 3, 3> &ccm, float saturation);
> -
>         unsigned int lastCt_;
> -       std::optional<float> lastSaturation_;
>         Interpolator<Matrix<float, 3, 3>> ccm_;
> +       Matrix<float, 3, 3> lastCcm_;

Some point maybe we should do a "using Ccm = Matrix<float, 3, 3>;" as a
libipa type ;-)


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

>  };
>  
>  } /* namespace ipa::soft::algorithms */
> diff --git a/src/ipa/simple/algorithms/meson.build b/src/ipa/simple/algorithms/meson.build
> index 2d0adb059..ebe9f20dd 100644
> --- a/src/ipa/simple/algorithms/meson.build
> +++ b/src/ipa/simple/algorithms/meson.build
> @@ -1,6 +1,7 @@
>  # SPDX-License-Identifier: CC0-1.0
>  
>  soft_simple_ipa_algorithms = files([
> +    'adjust.cpp',
>      'awb.cpp',
>      'agc.cpp',
>      'blc.cpp',
> diff --git a/src/ipa/simple/data/uncalibrated.yaml b/src/ipa/simple/data/uncalibrated.yaml
> index 5508e6686..f0410fe61 100644
> --- a/src/ipa/simple/data/uncalibrated.yaml
> +++ b/src/ipa/simple/data/uncalibrated.yaml
> @@ -14,6 +14,7 @@ algorithms:
>    #         ccm: [ 1, 0, 0,
>    #                0, 1, 0,
>    #                0, 0, 1]
> +  - Adjust:
>    - Lut:
>    - Agc:
>  ...
> -- 
> 2.51.1
>