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

Message ID 20251112082715.17823-7-mzamazal@redhat.com
State New
Headers show
Series
  • Simple pipeline IPA cleanup
Related show

Commit Message

Milan Zamazal Nov. 12, 2025, 8:27 a.m. UTC 
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

Comments

Kieran Bingham Nov. 12, 2025, 10:13 a.m. UTC | #1 
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
>
Milan Zamazal Nov. 13, 2025, 2:24 p.m. UTC | #2 
Hi Kieran,

Kieran Bingham <kieran.bingham@ideasonboard.com> writes:

> 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 }
> 		};
>
> ?

I probably had some disagreement with the autoformatter but we should
agree to each other in v2.

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

Not exactly `Ccm' I think but yes.  It's also used in rkisp1.  Not for
this series but it can be done as a follow-up.

> 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
>>
Laurent Pinchart Nov. 19, 2025, 4:26 a.m. UTC | #3 
On Wed, Nov 12, 2025 at 10:13:15AM +0000, Kieran Bingham wrote:
> 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 ++++++++++++

I'm not too fond of the name as it's very generic. In rkisp1 we call
this cproc, short for colour processing, which is the name of the
hardware block where those adjustments are applied. Should we use the
same name here ? Or is "adjust" the right term to cover contrast,
brightness, hue and saturation adjustments ?

> >  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 }
> 		};
> 
> ?

		Matrix<float, 3, 3>::identity();

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

static const

> > +               { 0.256788235294, 0.504129411765, 0.0979058823529,
> > +                 -0.148223529412, -0.290992156863, 0.439215686275,
> > +                 0.439215686275, -0.367788235294, -0.0714274509804 }
> > +       };
> > +       const Matrix<float, 3, 3> ycbcr2rgb{

Ditto.

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

We should then use the values from utils/rkisp1/gen-csc-table.py.

> 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

You forgot to rename this.

> > + */
> > +
> > +#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 ;-)

Not now, there will be too much bikeshedding :-)

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

Patch
diff mbox series 

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);
+	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 } };
+
+	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 }
+	};
+	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
  */
 
 #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;
-}
-
 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_;
 };
 
 } /* 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:
 ...