[{"id":36783,"web_url":"https://patchwork.libcamera.org/comment/36783/","msgid":"<176294239515.567526.4505528295042314471@ping.linuxembedded.co.uk>","date":"2025-11-12T10:13:15","subject":"Re: [RFC PATCH 6/7] libcamera: ipa: simple: Separate saturation from\n\tCCM","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Milan Zamazal (2025-11-12 08:27:14)\n> Saturation adjustments are implemented using matrix operations.  They\n> are currently applied to the colour correction matrix.  Let's move them\n> to a newly introduced separate \"Adjust\" algorithm.\n> \n> This separation has the following advantages:\n> \n> - It allows disabling general colour adjustments algorithms without\n>   disabling the CCM algorithm.\n> \n> - It keeps the CCM separated from other corrections.\n> \n> - It's generally cleaner.\n> \n> Signed-off-by: Milan Zamazal <mzamazal@redhat.com>\n> ---\n>  src/ipa/simple/algorithms/adjust.cpp  | 111 ++++++++++++++++++++++++++\n>  src/ipa/simple/algorithms/adjust.h    |  52 ++++++++++++\n>  src/ipa/simple/algorithms/ccm.cpp     |  58 +-------------\n>  src/ipa/simple/algorithms/ccm.h       |  10 +--\n>  src/ipa/simple/algorithms/meson.build |   1 +\n>  src/ipa/simple/data/uncalibrated.yaml |   1 +\n>  6 files changed, 169 insertions(+), 64 deletions(-)\n>  create mode 100644 src/ipa/simple/algorithms/adjust.cpp\n>  create mode 100644 src/ipa/simple/algorithms/adjust.h\n> \n> diff --git a/src/ipa/simple/algorithms/adjust.cpp b/src/ipa/simple/algorithms/adjust.cpp\n> new file mode 100644\n> index 000000000..282b3ccb0\n> --- /dev/null\n> +++ b/src/ipa/simple/algorithms/adjust.cpp\n> @@ -0,0 +1,111 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2024, Ideas On Board\n> + * Copyright (C) 2024-2025, Red Hat Inc.\n> + *\n> + * Common image adjustments\n> + */\n> +\n> +#include \"adjust.h\"\n> +\n> +#include <libcamera/base/log.h>\n> +#include <libcamera/base/utils.h>\n> +\n> +#include <libcamera/control_ids.h>\n> +\n> +#include \"libcamera/internal/matrix.h\"\n> +\n> +namespace libcamera {\n> +\n> +namespace ipa::soft::algorithms {\n> +\n> +LOG_DEFINE_CATEGORY(IPASoftAdjust)\n> +\n> +int Adjust::init(IPAContext &context, [[maybe_unused]] const YamlObject &tuningData)\n> +{\n> +       if (context.ccmEnabled)\n> +               context.ctrlMap[&controls::Saturation] = ControlInfo(0.0f, 2.0f, 1.0f);\n\nNice, I like the tight coupling of the controls to where they are\nprovided, and only reported if they are possible.\n\n\n> +       return 0;\n> +}\n> +\n> +int Adjust::configure(IPAContext &context,\n> +                     [[maybe_unused]] const IPAConfigInfo &configInfo)\n> +{\n> +       context.activeState.knobs.saturation = std::optional<double>();\n> +       context.activeState.correctionMatrix =\n> +               Matrix<float, 3, 3>{ { 1, 0, 0, 0, 1, 0, 0, 0, 1 } };\n\nReal nit pick but can we style this in three lines ? Does it help?\n\n\t\tMatrix<float, 3, 3>{ { 1, 0, 0,\n\t\t\t\t       0, 1, 0,\n\t\t\t\t       0, 0, 1 }\n\t\t};\n\nor \n\t\tMatrix<float, 3, 3>{\n\t\t\t{ 1, 0, 0,\n\t\t\t  0, 1, 0,\n\t\t\t  0, 0, 1 }\n\t\t};\n\n?\n\n> +\n> +       return 0;\n> +}\n> +\n> +void Adjust::queueRequest(typename Module::Context &context,\n> +                         [[maybe_unused]] const uint32_t frame,\n> +                         [[maybe_unused]] typename Module::FrameContext &frameContext,\n> +                         const ControlList &controls)\n> +{\n> +       const auto &saturation = controls.get(controls::Saturation);\n> +       if (saturation.has_value()) {\n> +               context.activeState.knobs.saturation = saturation;\n> +               LOG(IPASoftAdjust, Debug) << \"Setting saturation to \" << saturation.value();\n> +       }\n> +}\n> +\n> +void Adjust::applySaturation(Matrix<float, 3, 3> &matrix, float saturation)\n> +{\n> +       /* https://en.wikipedia.org/wiki/YCbCr#ITU-R_BT.601_conversion */\n> +       const Matrix<float, 3, 3> rgb2ycbcr{\n> +               { 0.256788235294, 0.504129411765, 0.0979058823529,\n> +                 -0.148223529412, -0.290992156863, 0.439215686275,\n> +                 0.439215686275, -0.367788235294, -0.0714274509804 }\n> +       };\n> +       const Matrix<float, 3, 3> ycbcr2rgb{\n> +               { 1.16438356164, 0, 1.59602678571,\n> +                 1.16438356164, -0.391762290094, -0.812967647235,\n> +                 1.16438356164, 2.01723214285, 0 }\n> +       };\n\nI think this is duplicating these const Matrix definitions from the ccm\nfile now.\n\nCan you move them to src/ipa/libipa/colours{.cpp,.h} as a pre-cursor\nplease?\n\nPerhaps they need to be in a colours::bt601::rgb2ycbcr,ycbcr2rgb\nnamespace if we expect bt709,bt2020 versions or such to be explicit.\n\nEdit: Discovered this is a move, not a duplication see below:\n\n> +       const Matrix<float, 3, 3> saturationMatrix{\n> +               { 1, 0, 0,\n> +                 0, saturation, 0,\n> +                 0, 0, saturation }\n> +       };\n> +       matrix =\n> +               ycbcr2rgb * saturationMatrix * rgb2ycbcr * matrix;\n> +}\n> +\n> +void Adjust::prepare(IPAContext &context,\n> +                    const uint32_t frame,\n> +                    IPAFrameContext &frameContext,\n> +                    [[maybe_unused]] DebayerParams *params)\n> +{\n> +       if (!context.ccmEnabled)\n> +               return;\n> +\n> +       auto &saturation = context.activeState.knobs.saturation;\n> +\n> +       if (frame > 0 && saturation == lastSaturation_)\n> +               return;\n> +\n> +       context.activeState.correctionMatrix = context.activeState.ccm;\n> +       context.activeState.matrixChanged = true;\n> +       lastSaturation_ = saturation;\n> +       if (saturation)\n> +               applySaturation(context.activeState.correctionMatrix, saturation.value());\n> +\n> +       frameContext.saturation = saturation;\n> +}\n> +\n> +void Adjust::process([[maybe_unused]] IPAContext &context,\n> +                    [[maybe_unused]] const uint32_t frame,\n> +                    IPAFrameContext &frameContext,\n> +                    [[maybe_unused]] const SwIspStats *stats,\n> +                    ControlList &metadata)\n> +{\n> +       const auto &saturation = frameContext.saturation;\n> +       metadata.set(controls::Saturation, saturation.value_or(1.0));\n> +}\n> +\n> +REGISTER_IPA_ALGORITHM(Adjust, \"Adjust\")\n> +\n> +} /* namespace ipa::soft::algorithms */\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/ipa/simple/algorithms/adjust.h b/src/ipa/simple/algorithms/adjust.h\n> new file mode 100644\n> index 000000000..c4baa2503\n> --- /dev/null\n> +++ b/src/ipa/simple/algorithms/adjust.h\n> @@ -0,0 +1,52 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2024-2025, Red Hat Inc.\n> + *\n> + * Color correction matrix\n> + */\n> +\n> +#pragma once\n> +\n> +#include <optional>\n> +\n> +#include \"libcamera/internal/matrix.h\"\n> +\n> +#include <libipa/interpolator.h>\n> +\n> +#include \"algorithm.h\"\n> +\n> +namespace libcamera {\n> +\n> +namespace ipa::soft::algorithms {\n> +\n> +class Adjust : public Algorithm\n> +{\n> +public:\n> +       Adjust() = default;\n> +       ~Adjust() = default;\n> +\n> +       int init(IPAContext &context, const YamlObject &tuningData) override;\n> +       int configure(IPAContext &context,\n> +                     const IPAConfigInfo &configInfo) override;\n> +       void queueRequest(typename Module::Context &context,\n> +                         const uint32_t frame,\n> +                         typename Module::FrameContext &frameContext,\n> +                         const ControlList &controls) override;\n> +       void prepare(IPAContext &context,\n> +                    const uint32_t frame,\n> +                    IPAFrameContext &frameContext,\n> +                    DebayerParams *params) override;\n> +       void process(IPAContext &context, const uint32_t frame,\n> +                    IPAFrameContext &frameContext,\n> +                    const SwIspStats *stats,\n> +                    ControlList &metadata) override;\n> +\n> +private:\n> +       void applySaturation(Matrix<float, 3, 3> &ccm, float saturation);\n> +\n> +       std::optional<float> lastSaturation_;\n> +};\n> +\n> +} /* namespace ipa::soft::algorithms */\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/ipa/simple/algorithms/ccm.cpp b/src/ipa/simple/algorithms/ccm.cpp\n> index 3e9528a91..c541ef466 100644\n> --- a/src/ipa/simple/algorithms/ccm.cpp\n> +++ b/src/ipa/simple/algorithms/ccm.cpp\n> @@ -3,7 +3,7 @@\n>   * Copyright (C) 2024, Ideas On Board\n>   * Copyright (C) 2024-2025, Red Hat Inc.\n>   *\n> - * Color correction matrix + saturation\n> + * Color correction matrix\n\nI very much like this ;-)\n\n\n>   */\n>  \n>  #include \"ccm.h\"\n> @@ -37,76 +37,27 @@ int Ccm::init([[maybe_unused]] IPAContext &context, const YamlObject &tuningData\n>         }\n>  \n>         context.ccmEnabled = true;\n> -       context.ctrlMap[&controls::Saturation] = ControlInfo(0.0f, 2.0f, 1.0f);\n>  \n>         return 0;\n>  }\n>  \n> -int Ccm::configure(IPAContext &context,\n> -                  [[maybe_unused]] const IPAConfigInfo &configInfo)\n> -{\n> -       context.activeState.knobs.saturation = std::optional<double>();\n> -\n> -       return 0;\n> -}\n> -\n> -void Ccm::queueRequest(typename Module::Context &context,\n> -                      [[maybe_unused]] const uint32_t frame,\n> -                      [[maybe_unused]] typename Module::FrameContext &frameContext,\n> -                      const ControlList &controls)\n> -{\n> -       const auto &saturation = controls.get(controls::Saturation);\n> -       if (saturation.has_value()) {\n> -               context.activeState.knobs.saturation = saturation;\n> -               LOG(IPASoftCcm, Debug) << \"Setting saturation to \" << saturation.value();\n> -       }\n> -}\n> -\n> -void Ccm::applySaturation(Matrix<float, 3, 3> &ccm, float saturation)\n> -{\n> -       /* https://en.wikipedia.org/wiki/YCbCr#ITU-R_BT.601_conversion */\n> -       const Matrix<float, 3, 3> rgb2ycbcr{\n> -               { 0.256788235294, 0.504129411765, 0.0979058823529,\n> -                 -0.148223529412, -0.290992156863, 0.439215686275,\n> -                 0.439215686275, -0.367788235294, -0.0714274509804 }\n> -       };\n> -       const Matrix<float, 3, 3> ycbcr2rgb{\n> -               { 1.16438356164, 0, 1.59602678571,\n> -                 1.16438356164, -0.391762290094, -0.812967647235,\n> -                 1.16438356164, 2.01723214285, 0 }\n> -       };\n> -       const Matrix<float, 3, 3> saturationMatrix{\n> -               { 1, 0, 0,\n> -                 0, saturation, 0,\n> -                 0, 0, saturation }\n> -       };\n> -       ccm = ycbcr2rgb * saturationMatrix * rgb2ycbcr * ccm;\n> -}\n\n\naha, never mind - we're not duplciating - we're moving. So ignore the\ncolours namespace requirement for now.\n\n\n> -\n>  void Ccm::prepare(IPAContext &context, const uint32_t frame,\n>                   IPAFrameContext &frameContext, [[maybe_unused]] DebayerParams *params)\n>  {\n> -       auto &saturation = context.activeState.knobs.saturation;\n> -\n>         const unsigned int ct = context.activeState.awb.temperatureK;\n>  \n> -       /* Change CCM only on saturation or bigger temperature changes. */\n> +       /* Change CCM only on bigger temperature changes. */\n>         if (frame > 0 &&\n> -           utils::abs_diff(ct, lastCt_) < kTemperatureThreshold &&\n> -           saturation == lastSaturation_) {\n> +           utils::abs_diff(ct, lastCt_) < kTemperatureThreshold) {\n>                 frameContext.ccm = context.activeState.ccm;\n>                 return;\n>         }\n>  \n>         lastCt_ = ct;\n> -       lastSaturation_ = saturation;\n>         Matrix<float, 3, 3> ccm = ccm_.getInterpolated(ct);\n> -       if (saturation)\n> -               applySaturation(ccm, saturation.value());\n>  \n>         context.activeState.correctionMatrix = ccm;\n>         context.activeState.ccm = ccm;\n> -       frameContext.saturation = saturation;\n>         context.activeState.matrixChanged = true;\n>         frameContext.ccm = ccm;\n>  }\n> @@ -118,9 +69,6 @@ void Ccm::process([[maybe_unused]] IPAContext &context,\n>                   ControlList &metadata)\n>  {\n>         metadata.set(controls::ColourCorrectionMatrix, frameContext.ccm.data());\n> -\n> -       const auto &saturation = frameContext.saturation;\n> -       metadata.set(controls::Saturation, saturation.value_or(1.0));\n>  }\n>  \n>  REGISTER_IPA_ALGORITHM(Ccm, \"Ccm\")\n> diff --git a/src/ipa/simple/algorithms/ccm.h b/src/ipa/simple/algorithms/ccm.h\n> index 8279a3d59..10c9829f5 100644\n> --- a/src/ipa/simple/algorithms/ccm.h\n> +++ b/src/ipa/simple/algorithms/ccm.h\n> @@ -26,12 +26,6 @@ public:\n>         ~Ccm() = default;\n>  \n>         int init(IPAContext &context, const YamlObject &tuningData) override;\n> -       int configure(IPAContext &context,\n> -                     const IPAConfigInfo &configInfo) override;\n> -       void queueRequest(typename Module::Context &context,\n> -                         const uint32_t frame,\n> -                         typename Module::FrameContext &frameContext,\n> -                         const ControlList &controls) override;\n>         void prepare(IPAContext &context,\n>                      const uint32_t frame,\n>                      IPAFrameContext &frameContext,\n> @@ -42,11 +36,9 @@ public:\n>                      ControlList &metadata) override;\n>  \n>  private:\n> -       void applySaturation(Matrix<float, 3, 3> &ccm, float saturation);\n> -\n>         unsigned int lastCt_;\n> -       std::optional<float> lastSaturation_;\n>         Interpolator<Matrix<float, 3, 3>> ccm_;\n> +       Matrix<float, 3, 3> lastCcm_;\n\nSome point maybe we should do a \"using Ccm = Matrix<float, 3, 3>;\" as a\nlibipa type ;-)\n\n\nReviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\n\n>  };\n>  \n>  } /* namespace ipa::soft::algorithms */\n> diff --git a/src/ipa/simple/algorithms/meson.build b/src/ipa/simple/algorithms/meson.build\n> index 2d0adb059..ebe9f20dd 100644\n> --- a/src/ipa/simple/algorithms/meson.build\n> +++ b/src/ipa/simple/algorithms/meson.build\n> @@ -1,6 +1,7 @@\n>  # SPDX-License-Identifier: CC0-1.0\n>  \n>  soft_simple_ipa_algorithms = files([\n> +    'adjust.cpp',\n>      'awb.cpp',\n>      'agc.cpp',\n>      'blc.cpp',\n> diff --git a/src/ipa/simple/data/uncalibrated.yaml b/src/ipa/simple/data/uncalibrated.yaml\n> index 5508e6686..f0410fe61 100644\n> --- a/src/ipa/simple/data/uncalibrated.yaml\n> +++ b/src/ipa/simple/data/uncalibrated.yaml\n> @@ -14,6 +14,7 @@ algorithms:\n>    #         ccm: [ 1, 0, 0,\n>    #                0, 1, 0,\n>    #                0, 0, 1]\n> +  - Adjust:\n>    - Lut:\n>    - Agc:\n>  ...\n> -- \n> 2.51.1\n>","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 184E2C3241\n\tfor <parsemail@patchwork.libcamera.org>;\n\tWed, 12 Nov 2025 10:13:22 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1EC1A60A8B;\n\tWed, 12 Nov 2025 11:13:21 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A4FC4606E6\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed, 12 Nov 2025 11:13:18 +0100 (CET)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust6594.18-1.cable.virginm.net [86.31.185.195])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 3A4F0E77;\n\tWed, 12 Nov 2025 11:11:19 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"Va/1EZr5\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1762942279;\n\tbh=cYWAk49nYeoeVJDBIxor8srY0Ur+f7lYm5G0oaPd8B0=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=Va/1EZr5y1ExnFQ0CGrpYTDXATPyvMfjurtBnvz/HhS6IRNSeWAJSVa85p9Xl48FR\n\tLSYCw8PjnCZtdqmiufPZ5RWmHx1DURineSMxqNu04JMrvdVTlTUfe3NXzS6muD6x0p\n\t6o56kyaoBcZ6+l7MuUuhuGKZ5YRNoVLaWtq7BeGQ=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20251112082715.17823-7-mzamazal@redhat.com>","References":"<20251112082715.17823-1-mzamazal@redhat.com>\n\t<20251112082715.17823-7-mzamazal@redhat.com>","Subject":"Re: [RFC PATCH 6/7] libcamera: ipa: simple: Separate saturation from\n\tCCM","From":"Kieran Bingham <kieran.bingham@ideasonboard.com>","Cc":"Milan Zamazal <mzamazal@redhat.com>","To":"Milan Zamazal <mzamazal@redhat.com>, libcamera-devel@lists.libcamera.org","Date":"Wed, 12 Nov 2025 10:13:15 +0000","Message-ID":"<176294239515.567526.4505528295042314471@ping.linuxembedded.co.uk>","User-Agent":"alot/0.9.1","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":36800,"web_url":"https://patchwork.libcamera.org/comment/36800/","msgid":"<85seei1041.fsf@mzamazal-thinkpadp1gen7.tpbc.csb>","date":"2025-11-13T14:24:46","subject":"Re: [RFC PATCH 6/7] libcamera: ipa: simple: Separate saturation\n\tfrom CCM","submitter":{"id":177,"url":"https://patchwork.libcamera.org/api/people/177/","name":"Milan Zamazal","email":"mzamazal@redhat.com"},"content":"Hi Kieran,\n\nKieran Bingham <kieran.bingham@ideasonboard.com> writes:\n\n> Quoting Milan Zamazal (2025-11-12 08:27:14)\n>> Saturation adjustments are implemented using matrix operations.  They\n>> are currently applied to the colour correction matrix.  Let's move them\n>\n>> to a newly introduced separate \"Adjust\" algorithm.\n>> \n>> This separation has the following advantages:\n>> \n>> - It allows disabling general colour adjustments algorithms without\n>>   disabling the CCM algorithm.\n>> \n>> - It keeps the CCM separated from other corrections.\n>> \n>> - It's generally cleaner.\n>> \n>> Signed-off-by: Milan Zamazal <mzamazal@redhat.com>\n>> ---\n>>  src/ipa/simple/algorithms/adjust.cpp  | 111 ++++++++++++++++++++++++++\n>>  src/ipa/simple/algorithms/adjust.h    |  52 ++++++++++++\n>>  src/ipa/simple/algorithms/ccm.cpp     |  58 +-------------\n>>  src/ipa/simple/algorithms/ccm.h       |  10 +--\n>>  src/ipa/simple/algorithms/meson.build |   1 +\n>>  src/ipa/simple/data/uncalibrated.yaml |   1 +\n>>  6 files changed, 169 insertions(+), 64 deletions(-)\n>>  create mode 100644 src/ipa/simple/algorithms/adjust.cpp\n>>  create mode 100644 src/ipa/simple/algorithms/adjust.h\n>> \n>> diff --git a/src/ipa/simple/algorithms/adjust.cpp b/src/ipa/simple/algorithms/adjust.cpp\n>> new file mode 100644\n>> index 000000000..282b3ccb0\n>> --- /dev/null\n>> +++ b/src/ipa/simple/algorithms/adjust.cpp\n>> @@ -0,0 +1,111 @@\n>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n>> +/*\n>> + * Copyright (C) 2024, Ideas On Board\n>> + * Copyright (C) 2024-2025, Red Hat Inc.\n>> + *\n>> + * Common image adjustments\n>> + */\n>> +\n>> +#include \"adjust.h\"\n>> +\n>> +#include <libcamera/base/log.h>\n>> +#include <libcamera/base/utils.h>\n>> +\n>> +#include <libcamera/control_ids.h>\n>> +\n>> +#include \"libcamera/internal/matrix.h\"\n>> +\n>> +namespace libcamera {\n>> +\n>> +namespace ipa::soft::algorithms {\n>> +\n>> +LOG_DEFINE_CATEGORY(IPASoftAdjust)\n>> +\n>> +int Adjust::init(IPAContext &context, [[maybe_unused]] const YamlObject &tuningData)\n>> +{\n>> +       if (context.ccmEnabled)\n>> +               context.ctrlMap[&controls::Saturation] = ControlInfo(0.0f, 2.0f, 1.0f);\n>\n> Nice, I like the tight coupling of the controls to where they are\n> provided, and only reported if they are possible.\n>\n>\n>> +       return 0;\n>> +}\n>> +\n>> +int Adjust::configure(IPAContext &context,\n>> +                     [[maybe_unused]] const IPAConfigInfo &configInfo)\n>> +{\n>> +       context.activeState.knobs.saturation = std::optional<double>();\n>> +       context.activeState.correctionMatrix =\n>> +               Matrix<float, 3, 3>{ { 1, 0, 0, 0, 1, 0, 0, 0, 1 } };\n>\n> Real nit pick but can we style this in three lines ? Does it help?\n>\n> \t\tMatrix<float, 3, 3>{ { 1, 0, 0,\n> \t\t\t\t       0, 1, 0,\n> \t\t\t\t       0, 0, 1 }\n> \t\t};\n>\n> or \n> \t\tMatrix<float, 3, 3>{\n> \t\t\t{ 1, 0, 0,\n> \t\t\t  0, 1, 0,\n> \t\t\t  0, 0, 1 }\n> \t\t};\n>\n> ?\n\nI probably had some disagreement with the autoformatter but we should\nagree to each other in v2.\n\n>> +\n>> +       return 0;\n>> +}\n>> +\n>> +void Adjust::queueRequest(typename Module::Context &context,\n>> +                         [[maybe_unused]] const uint32_t frame,\n>> +                         [[maybe_unused]] typename Module::FrameContext &frameContext,\n>> +                         const ControlList &controls)\n>> +{\n>> +       const auto &saturation = controls.get(controls::Saturation);\n>> +       if (saturation.has_value()) {\n>> +               context.activeState.knobs.saturation = saturation;\n>> +               LOG(IPASoftAdjust, Debug) << \"Setting saturation to \" << saturation.value();\n>> +       }\n>> +}\n>> +\n>> +void Adjust::applySaturation(Matrix<float, 3, 3> &matrix, float saturation)\n>> +{\n>> +       /* https://en.wikipedia.org/wiki/YCbCr#ITU-R_BT.601_conversion */\n>> +       const Matrix<float, 3, 3> rgb2ycbcr{\n>> +               { 0.256788235294, 0.504129411765, 0.0979058823529,\n>> +                 -0.148223529412, -0.290992156863, 0.439215686275,\n>> +                 0.439215686275, -0.367788235294, -0.0714274509804 }\n>> +       };\n>> +       const Matrix<float, 3, 3> ycbcr2rgb{\n>> +               { 1.16438356164, 0, 1.59602678571,\n>> +                 1.16438356164, -0.391762290094, -0.812967647235,\n>> +                 1.16438356164, 2.01723214285, 0 }\n>> +       };\n>\n> I think this is duplicating these const Matrix definitions from the ccm\n> file now.\n>\n> Can you move them to src/ipa/libipa/colours{.cpp,.h} as a pre-cursor\n> please?\n>\n> Perhaps they need to be in a colours::bt601::rgb2ycbcr,ycbcr2rgb\n> namespace if we expect bt709,bt2020 versions or such to be explicit.\n>\n> Edit: Discovered this is a move, not a duplication see below:\n>\n>> +       const Matrix<float, 3, 3> saturationMatrix{\n>> +               { 1, 0, 0,\n>> +                 0, saturation, 0,\n>> +                 0, 0, saturation }\n>> +       };\n>> +       matrix =\n>> +               ycbcr2rgb * saturationMatrix * rgb2ycbcr * matrix;\n>> +}\n>> +\n>> +void Adjust::prepare(IPAContext &context,\n>> +                    const uint32_t frame,\n>> +                    IPAFrameContext &frameContext,\n>> +                    [[maybe_unused]] DebayerParams *params)\n>> +{\n>> +       if (!context.ccmEnabled)\n>> +               return;\n>> +\n>> +       auto &saturation = context.activeState.knobs.saturation;\n>> +\n>> +       if (frame > 0 && saturation == lastSaturation_)\n>> +               return;\n>> +\n>> +       context.activeState.correctionMatrix = context.activeState.ccm;\n>> +       context.activeState.matrixChanged = true;\n>> +       lastSaturation_ = saturation;\n>> +       if (saturation)\n>> +               applySaturation(context.activeState.correctionMatrix, saturation.value());\n>> +\n>> +       frameContext.saturation = saturation;\n>> +}\n>> +\n>> +void Adjust::process([[maybe_unused]] IPAContext &context,\n>> +                    [[maybe_unused]] const uint32_t frame,\n>> +                    IPAFrameContext &frameContext,\n>> +                    [[maybe_unused]] const SwIspStats *stats,\n>> +                    ControlList &metadata)\n>> +{\n>> +       const auto &saturation = frameContext.saturation;\n>> +       metadata.set(controls::Saturation, saturation.value_or(1.0));\n>> +}\n>> +\n>> +REGISTER_IPA_ALGORITHM(Adjust, \"Adjust\")\n>> +\n>> +} /* namespace ipa::soft::algorithms */\n>> +\n>> +} /* namespace libcamera */\n>> diff --git a/src/ipa/simple/algorithms/adjust.h b/src/ipa/simple/algorithms/adjust.h\n>> new file mode 100644\n>> index 000000000..c4baa2503\n>> --- /dev/null\n>> +++ b/src/ipa/simple/algorithms/adjust.h\n>> @@ -0,0 +1,52 @@\n>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n>> +/*\n>> + * Copyright (C) 2024-2025, Red Hat Inc.\n>> + *\n>> + * Color correction matrix\n>> + */\n>> +\n>> +#pragma once\n>> +\n>> +#include <optional>\n>> +\n>> +#include \"libcamera/internal/matrix.h\"\n>> +\n>> +#include <libipa/interpolator.h>\n>> +\n>> +#include \"algorithm.h\"\n>> +\n>> +namespace libcamera {\n>> +\n>> +namespace ipa::soft::algorithms {\n>> +\n>> +class Adjust : public Algorithm\n>> +{\n>> +public:\n>> +       Adjust() = default;\n>> +       ~Adjust() = default;\n>> +\n>> +       int init(IPAContext &context, const YamlObject &tuningData) override;\n>> +       int configure(IPAContext &context,\n>> +                     const IPAConfigInfo &configInfo) override;\n>> +       void queueRequest(typename Module::Context &context,\n>> +                         const uint32_t frame,\n>> +                         typename Module::FrameContext &frameContext,\n>> +                         const ControlList &controls) override;\n>> +       void prepare(IPAContext &context,\n>> +                    const uint32_t frame,\n>> +                    IPAFrameContext &frameContext,\n>> +                    DebayerParams *params) override;\n>> +       void process(IPAContext &context, const uint32_t frame,\n>> +                    IPAFrameContext &frameContext,\n>> +                    const SwIspStats *stats,\n>> +                    ControlList &metadata) override;\n>> +\n>> +private:\n>> +       void applySaturation(Matrix<float, 3, 3> &ccm, float saturation);\n>> +\n>> +       std::optional<float> lastSaturation_;\n>> +};\n>> +\n>> +} /* namespace ipa::soft::algorithms */\n>> +\n>> +} /* namespace libcamera */\n>> diff --git a/src/ipa/simple/algorithms/ccm.cpp b/src/ipa/simple/algorithms/ccm.cpp\n>> index 3e9528a91..c541ef466 100644\n>> --- a/src/ipa/simple/algorithms/ccm.cpp\n>> +++ b/src/ipa/simple/algorithms/ccm.cpp\n>> @@ -3,7 +3,7 @@\n>>   * Copyright (C) 2024, Ideas On Board\n>>   * Copyright (C) 2024-2025, Red Hat Inc.\n>>   *\n>> - * Color correction matrix + saturation\n>> + * Color correction matrix\n>\n> I very much like this ;-)\n>\n>\n>>   */\n>>  \n>>  #include \"ccm.h\"\n>> @@ -37,76 +37,27 @@ int Ccm::init([[maybe_unused]] IPAContext &context, const YamlObject &tuningData\n>>         }\n>>  \n>>         context.ccmEnabled = true;\n>> -       context.ctrlMap[&controls::Saturation] = ControlInfo(0.0f, 2.0f, 1.0f);\n>>  \n>>         return 0;\n>>  }\n>>  \n>> -int Ccm::configure(IPAContext &context,\n>> -                  [[maybe_unused]] const IPAConfigInfo &configInfo)\n>> -{\n>> -       context.activeState.knobs.saturation = std::optional<double>();\n>> -\n>> -       return 0;\n>> -}\n>> -\n>> -void Ccm::queueRequest(typename Module::Context &context,\n>> -                      [[maybe_unused]] const uint32_t frame,\n>> -                      [[maybe_unused]] typename Module::FrameContext &frameContext,\n>> -                      const ControlList &controls)\n>> -{\n>> -       const auto &saturation = controls.get(controls::Saturation);\n>> -       if (saturation.has_value()) {\n>> -               context.activeState.knobs.saturation = saturation;\n>> -               LOG(IPASoftCcm, Debug) << \"Setting saturation to \" << saturation.value();\n>> -       }\n>> -}\n>> -\n>> -void Ccm::applySaturation(Matrix<float, 3, 3> &ccm, float saturation)\n>> -{\n>> -       /* https://en.wikipedia.org/wiki/YCbCr#ITU-R_BT.601_conversion */\n>> -       const Matrix<float, 3, 3> rgb2ycbcr{\n>> -               { 0.256788235294, 0.504129411765, 0.0979058823529,\n>> -                 -0.148223529412, -0.290992156863, 0.439215686275,\n>> -                 0.439215686275, -0.367788235294, -0.0714274509804 }\n>> -       };\n>> -       const Matrix<float, 3, 3> ycbcr2rgb{\n>> -               { 1.16438356164, 0, 1.59602678571,\n>> -                 1.16438356164, -0.391762290094, -0.812967647235,\n>> -                 1.16438356164, 2.01723214285, 0 }\n>> -       };\n>> -       const Matrix<float, 3, 3> saturationMatrix{\n>> -               { 1, 0, 0,\n>> -                 0, saturation, 0,\n>> -                 0, 0, saturation }\n>> -       };\n>> -       ccm = ycbcr2rgb * saturationMatrix * rgb2ycbcr * ccm;\n>> -}\n>\n>\n> aha, never mind - we're not duplciating - we're moving. So ignore the\n> colours namespace requirement for now.\n>\n>\n>> -\n>>  void Ccm::prepare(IPAContext &context, const uint32_t frame,\n>>                   IPAFrameContext &frameContext, [[maybe_unused]] DebayerParams *params)\n>>  {\n>> -       auto &saturation = context.activeState.knobs.saturation;\n>> -\n>>         const unsigned int ct = context.activeState.awb.temperatureK;\n>>  \n>> -       /* Change CCM only on saturation or bigger temperature changes. */\n>> +       /* Change CCM only on bigger temperature changes. */\n>>         if (frame > 0 &&\n>> -           utils::abs_diff(ct, lastCt_) < kTemperatureThreshold &&\n>> -           saturation == lastSaturation_) {\n>> +           utils::abs_diff(ct, lastCt_) < kTemperatureThreshold) {\n>>                 frameContext.ccm = context.activeState.ccm;\n>>                 return;\n>>         }\n>>  \n>>         lastCt_ = ct;\n>> -       lastSaturation_ = saturation;\n>>         Matrix<float, 3, 3> ccm = ccm_.getInterpolated(ct);\n>> -       if (saturation)\n>> -               applySaturation(ccm, saturation.value());\n>>  \n>>         context.activeState.correctionMatrix = ccm;\n>>         context.activeState.ccm = ccm;\n>> -       frameContext.saturation = saturation;\n>>         context.activeState.matrixChanged = true;\n>>         frameContext.ccm = ccm;\n>>  }\n>> @@ -118,9 +69,6 @@ void Ccm::process([[maybe_unused]] IPAContext &context,\n>>                   ControlList &metadata)\n>>  {\n>>         metadata.set(controls::ColourCorrectionMatrix, frameContext.ccm.data());\n>> -\n>> -       const auto &saturation = frameContext.saturation;\n>> -       metadata.set(controls::Saturation, saturation.value_or(1.0));\n>>  }\n>>  \n>>  REGISTER_IPA_ALGORITHM(Ccm, \"Ccm\")\n>> diff --git a/src/ipa/simple/algorithms/ccm.h b/src/ipa/simple/algorithms/ccm.h\n>> index 8279a3d59..10c9829f5 100644\n>> --- a/src/ipa/simple/algorithms/ccm.h\n>> +++ b/src/ipa/simple/algorithms/ccm.h\n>> @@ -26,12 +26,6 @@ public:\n>>         ~Ccm() = default;\n>>  \n>>         int init(IPAContext &context, const YamlObject &tuningData) override;\n>> -       int configure(IPAContext &context,\n>> -                     const IPAConfigInfo &configInfo) override;\n>> -       void queueRequest(typename Module::Context &context,\n>> -                         const uint32_t frame,\n>> -                         typename Module::FrameContext &frameContext,\n>> -                         const ControlList &controls) override;\n>>         void prepare(IPAContext &context,\n>>                      const uint32_t frame,\n>>                      IPAFrameContext &frameContext,\n>> @@ -42,11 +36,9 @@ public:\n>>                      ControlList &metadata) override;\n>>  \n>>  private:\n>> -       void applySaturation(Matrix<float, 3, 3> &ccm, float saturation);\n>> -\n>>         unsigned int lastCt_;\n>> -       std::optional<float> lastSaturation_;\n>>         Interpolator<Matrix<float, 3, 3>> ccm_;\n>> +       Matrix<float, 3, 3> lastCcm_;\n>\n> Some point maybe we should do a \"using Ccm = Matrix<float, 3, 3>;\" as a\n> libipa type ;-)\n\nNot exactly `Ccm' I think but yes.  It's also used in rkisp1.  Not for\nthis series but it can be done as a follow-up.\n\n> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\n>\n>>  };\n>>  \n>>  } /* namespace ipa::soft::algorithms */\n>> diff --git a/src/ipa/simple/algorithms/meson.build b/src/ipa/simple/algorithms/meson.build\n>> index 2d0adb059..ebe9f20dd 100644\n>> --- a/src/ipa/simple/algorithms/meson.build\n>> +++ b/src/ipa/simple/algorithms/meson.build\n>> @@ -1,6 +1,7 @@\n>>  # SPDX-License-Identifier: CC0-1.0\n>>  \n>>  soft_simple_ipa_algorithms = files([\n>> +    'adjust.cpp',\n>>      'awb.cpp',\n>>      'agc.cpp',\n>>      'blc.cpp',\n>> diff --git a/src/ipa/simple/data/uncalibrated.yaml b/src/ipa/simple/data/uncalibrated.yaml\n>> index 5508e6686..f0410fe61 100644\n>> --- a/src/ipa/simple/data/uncalibrated.yaml\n>> +++ b/src/ipa/simple/data/uncalibrated.yaml\n>> @@ -14,6 +14,7 @@ algorithms:\n>>    #         ccm: [ 1, 0, 0,\n>>    #                0, 1, 0,\n>>    #                0, 0, 1]\n>> +  - Adjust:\n>>    - Lut:\n>>    - Agc:\n>>  ...\n>> -- \n>> 2.51.1\n>>","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 7BDA4C3263\n\tfor <parsemail@patchwork.libcamera.org>;\n\tThu, 13 Nov 2025 14:24:55 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5686960A80;\n\tThu, 13 Nov 2025 15:24:54 +0100 (CET)","from us-smtp-delivery-124.mimecast.com\n\t(us-smtp-delivery-124.mimecast.com [170.10.129.124])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 185F560805\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tThu, 13 Nov 2025 15:24:52 +0100 (CET)","from mail-wm1-f72.google.com (mail-wm1-f72.google.com\n\t[209.85.128.72]) by relay.mimecast.com with ESMTP with STARTTLS\n\t(version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id\n\tus-mta-411-53f4xVVrOdiUy3g851feWA-1; Thu, 13 Nov 2025 09:24:50 -0500","by mail-wm1-f72.google.com with SMTP id\n\t5b1f17b1804b1-475dabb63f2so4540195e9.3\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tThu, 13 Nov 2025 06:24:50 -0800 (PST)","from mzamazal-thinkpadp1gen7.tpbc.csb\n\t(ip-77-48-47-2.net.vodafone.cz. [77.48.47.2])\n\tby smtp.gmail.com with ESMTPSA id\n\t5b1f17b1804b1-47787e45677sm98592945e9.8.2025.11.13.06.24.47\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tThu, 13 Nov 2025 06:24:47 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=redhat.com header.i=@redhat.com\n\theader.b=\"Sgh1Zg76\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1763043892;\n\th=from:from:reply-to:subject:subject:date:date:message-id:message-id:\n\tto:to:cc:cc:mime-version:mime-version:content-type:content-type:\n\tin-reply-to:in-reply-to:references:references;\n\tbh=Is3RqLhAbAPLJsOk7gWI5sX5pzOkE69vpb1Ire1E9vE=;\n\tb=Sgh1Zg76615g2cyB4HhyG35XZr9ldFHW93YxM5RJS93p7YamJzJglGp5SAwDZCp9oc+vOS\n\tJc3Ozj2CjGQdFhPwEs8LahACtQch3Tx3UKHIf6MrqwIjMdRFreGzkQW21KMbsR0LM+notG\n\tK1E8mKoZLvijMawyqw46t3ETSuN0A44=","X-MC-Unique":"53f4xVVrOdiUy3g851feWA-1","X-Mimecast-MFC-AGG-ID":"53f4xVVrOdiUy3g851feWA_1763043890","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1763043889; x=1763648689;\n\th=mime-version:user-agent:message-id:date:references:in-reply-to\n\t:subject:cc:to:from:x-gm-gg:x-gm-message-state:from:to:cc:subject\n\t:date:message-id:reply-to;\n\tbh=Is3RqLhAbAPLJsOk7gWI5sX5pzOkE69vpb1Ire1E9vE=;\n\tb=CaEZJoHg6icWoTAmiKVUriCyBG3hczHI98EMBpHrsoXECAcY3suByco7V90McVGrPG\n\t7lUW5YDZf43Wo5DUMzM1c18Dy/5IWF0/N/14Gipp+m9b4RqmdGd6CQFfDFmEB/aBnQug\n\tH+2jWN4B60Fl2sX0+g+eN9TWHrVLwn6ic5h6O1bT/wICOeMgi1JCgDlVLvZl77ter7XJ\n\t63quLqMnFNlv8oV7X51D8nUSGC/hSCUoQbTx3Hs/TP71LOlkuJlorPp8yJJIxIE/xg/S\n\tCll8/NJ9bMJmsITs6fkS6JiLB1c35Iqx8IPbdwjd1CZulok3mqrTopNmemdx5ez1vJis\n\tDltQ==","X-Gm-Message-State":"AOJu0YwcsEgzYEdOmWrdEiQMU3MrI6gBlKE2kQCKhUZRvQpHLVl25JQE\n\t/UXXIncD1SyRnE1K8+W4hi7Ws9QlnRv2ySrtSSbzPTd/w4j47+BvrDwY+KGFjoZb3kd6A52KfMy\n\t7kWunqM3mJ0Idr0GN5qKirmqeHVnSd9YAbECGxCmTPfnsH0bUfRVb2inP0Im029twFyANSDNuvC\n\tI4R1ReRyDeLZ0jG7aeF56IZf73AF/yCBp8XgUCbycOYfPm60YZplz6kUGzUWo=","X-Gm-Gg":"ASbGnctegFUdp/MZ/8Ow9AUKsmPn0vsKCs5SiUWBwp5OvBCcFuN1RQEIeL2LDMGr7YV\n\tDgochi/FGIzzjs+AGIIAzQ6m21RwbNyBRBNIhrhmK2cYnds8AsfpipzmfvCJdxAEr3nGYfAkvCn\n\tSNYOoLBhQYqwWaOApeuVMBQsMjtXpVyWdcjYhBG6tt6V/hLSYZ0AhNTlzeWlaz7IGuMZ+mD6J5v\n\ttrR9v4fpOd+dppyTNNrQddnYy0F88ZPohZWlmKnpEfgb7+CXczCwrNe4Ntn9KgBhtvOgmeN7GnM\n\tDIt7E4jyOGVwbDPNdiPBa0AbwlMulRfHeoLolwQXvNPZ0XyFdkt4imeHqB0sCutHnhHoaPCfd1r\n\tzKm0RPbKZe/Y+jiXTSzvG0GvcF9/GnAjpEqN2k79OGUDC9oOeaPD1","X-Received":["by 2002:a05:600c:3b17:b0:471:9da:5232 with SMTP id\n\t5b1f17b1804b1-4778704d781mr65374095e9.15.1763043888853; \n\tThu, 13 Nov 2025 06:24:48 -0800 (PST)","by 2002:a05:600c:3b17:b0:471:9da:5232 with SMTP id\n\t5b1f17b1804b1-4778704d781mr65373595e9.15.1763043888164; \n\tThu, 13 Nov 2025 06:24:48 -0800 (PST)"],"X-Google-Smtp-Source":"AGHT+IE9/eW2kZ275ykYidQEoolLUmGH+Ia0JZkqBXWfxX/7rDbIT5EBNCGKWiBP0rgriEj0G2jIhQ==","From":"Milan Zamazal <mzamazal@redhat.com>","To":"Kieran Bingham <kieran.bingham@ideasonboard.com>","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [RFC PATCH 6/7] libcamera: ipa: simple: Separate saturation\n\tfrom CCM","In-Reply-To":"<176294239515.567526.4505528295042314471@ping.linuxembedded.co.uk>\n\t(Kieran Bingham's message of \"Wed, 12 Nov 2025 10:13:15 +0000\")","References":"<20251112082715.17823-1-mzamazal@redhat.com>\n\t<20251112082715.17823-7-mzamazal@redhat.com>\n\t<176294239515.567526.4505528295042314471@ping.linuxembedded.co.uk>","Date":"Thu, 13 Nov 2025 15:24:46 +0100","Message-ID":"<85seei1041.fsf@mzamazal-thinkpadp1gen7.tpbc.csb>","User-Agent":"Gnus/5.13 (Gnus v5.13)","MIME-Version":"1.0","X-Mimecast-Spam-Score":"0","X-Mimecast-MFC-PROC-ID":"P86qYgHArVz5PPKh_McpINadDAbrD6vYNQB22HNQ7-s_1763043890","X-Mimecast-Originator":"redhat.com","Content-Type":"text/plain","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":36896,"web_url":"https://patchwork.libcamera.org/comment/36896/","msgid":"<20251119042647.GQ10711@pendragon.ideasonboard.com>","date":"2025-11-19T04:26:47","subject":"Re: [RFC PATCH 6/7] libcamera: ipa: simple: Separate saturation from\n\tCCM","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Wed, Nov 12, 2025 at 10:13:15AM +0000, Kieran Bingham wrote:\n> Quoting Milan Zamazal (2025-11-12 08:27:14)\n> > Saturation adjustments are implemented using matrix operations.  They\n> > are currently applied to the colour correction matrix.  Let's move them\n> > to a newly introduced separate \"Adjust\" algorithm.\n> > \n> > This separation has the following advantages:\n> > \n> > - It allows disabling general colour adjustments algorithms without\n> >   disabling the CCM algorithm.\n> > \n> > - It keeps the CCM separated from other corrections.\n> > \n> > - It's generally cleaner.\n> > \n> > Signed-off-by: Milan Zamazal <mzamazal@redhat.com>\n> > ---\n> >  src/ipa/simple/algorithms/adjust.cpp  | 111 ++++++++++++++++++++++++++\n> >  src/ipa/simple/algorithms/adjust.h    |  52 ++++++++++++\n\nI'm not too fond of the name as it's very generic. In rkisp1 we call\nthis cproc, short for colour processing, which is the name of the\nhardware block where those adjustments are applied. Should we use the\nsame name here ? Or is \"adjust\" the right term to cover contrast,\nbrightness, hue and saturation adjustments ?\n\n> >  src/ipa/simple/algorithms/ccm.cpp     |  58 +-------------\n> >  src/ipa/simple/algorithms/ccm.h       |  10 +--\n> >  src/ipa/simple/algorithms/meson.build |   1 +\n> >  src/ipa/simple/data/uncalibrated.yaml |   1 +\n> >  6 files changed, 169 insertions(+), 64 deletions(-)\n> >  create mode 100644 src/ipa/simple/algorithms/adjust.cpp\n> >  create mode 100644 src/ipa/simple/algorithms/adjust.h\n> > \n> > diff --git a/src/ipa/simple/algorithms/adjust.cpp b/src/ipa/simple/algorithms/adjust.cpp\n> > new file mode 100644\n> > index 000000000..282b3ccb0\n> > --- /dev/null\n> > +++ b/src/ipa/simple/algorithms/adjust.cpp\n> > @@ -0,0 +1,111 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2024, Ideas On Board\n> > + * Copyright (C) 2024-2025, Red Hat Inc.\n> > + *\n> > + * Common image adjustments\n> > + */\n> > +\n> > +#include \"adjust.h\"\n> > +\n> > +#include <libcamera/base/log.h>\n> > +#include <libcamera/base/utils.h>\n> > +\n> > +#include <libcamera/control_ids.h>\n> > +\n> > +#include \"libcamera/internal/matrix.h\"\n> > +\n> > +namespace libcamera {\n> > +\n> > +namespace ipa::soft::algorithms {\n> > +\n> > +LOG_DEFINE_CATEGORY(IPASoftAdjust)\n> > +\n> > +int Adjust::init(IPAContext &context, [[maybe_unused]] const YamlObject &tuningData)\n> > +{\n> > +       if (context.ccmEnabled)\n> > +               context.ctrlMap[&controls::Saturation] = ControlInfo(0.0f, 2.0f, 1.0f);\n> \n> Nice, I like the tight coupling of the controls to where they are\n> provided, and only reported if they are possible.\n> \n> > +       return 0;\n> > +}\n> > +\n> > +int Adjust::configure(IPAContext &context,\n> > +                     [[maybe_unused]] const IPAConfigInfo &configInfo)\n> > +{\n> > +       context.activeState.knobs.saturation = std::optional<double>();\n> > +       context.activeState.correctionMatrix =\n> > +               Matrix<float, 3, 3>{ { 1, 0, 0, 0, 1, 0, 0, 0, 1 } };\n> \n> Real nit pick but can we style this in three lines ? Does it help?\n> \n> \t\tMatrix<float, 3, 3>{ { 1, 0, 0,\n> \t\t\t\t       0, 1, 0,\n> \t\t\t\t       0, 0, 1 }\n> \t\t};\n> \n> or \n> \t\tMatrix<float, 3, 3>{\n> \t\t\t{ 1, 0, 0,\n> \t\t\t  0, 1, 0,\n> \t\t\t  0, 0, 1 }\n> \t\t};\n> \n> ?\n\n\t\tMatrix<float, 3, 3>::identity();\n\n> \n> > +\n> > +       return 0;\n> > +}\n> > +\n> > +void Adjust::queueRequest(typename Module::Context &context,\n> > +                         [[maybe_unused]] const uint32_t frame,\n> > +                         [[maybe_unused]] typename Module::FrameContext &frameContext,\n> > +                         const ControlList &controls)\n> > +{\n> > +       const auto &saturation = controls.get(controls::Saturation);\n> > +       if (saturation.has_value()) {\n> > +               context.activeState.knobs.saturation = saturation;\n> > +               LOG(IPASoftAdjust, Debug) << \"Setting saturation to \" << saturation.value();\n> > +       }\n> > +}\n> > +\n> > +void Adjust::applySaturation(Matrix<float, 3, 3> &matrix, float saturation)\n> > +{\n> > +       /* https://en.wikipedia.org/wiki/YCbCr#ITU-R_BT.601_conversion */\n> > +       const Matrix<float, 3, 3> rgb2ycbcr{\n\nstatic const\n\n> > +               { 0.256788235294, 0.504129411765, 0.0979058823529,\n> > +                 -0.148223529412, -0.290992156863, 0.439215686275,\n> > +                 0.439215686275, -0.367788235294, -0.0714274509804 }\n> > +       };\n> > +       const Matrix<float, 3, 3> ycbcr2rgb{\n\nDitto.\n\n> > +               { 1.16438356164, 0, 1.59602678571,\n> > +                 1.16438356164, -0.391762290094, -0.812967647235,\n> > +                 1.16438356164, 2.01723214285, 0 }\n> > +       };\n> \n> I think this is duplicating these const Matrix definitions from the ccm\n> file now.\n> \n> Can you move them to src/ipa/libipa/colours{.cpp,.h} as a pre-cursor\n> please?\n\nWe should then use the values from utils/rkisp1/gen-csc-table.py.\n\n> Perhaps they need to be in a colours::bt601::rgb2ycbcr,ycbcr2rgb\n> namespace if we expect bt709,bt2020 versions or such to be explicit.\n> \n> Edit: Discovered this is a move, not a duplication see below:\n> \n> > +       const Matrix<float, 3, 3> saturationMatrix{\n> > +               { 1, 0, 0,\n> > +                 0, saturation, 0,\n> > +                 0, 0, saturation }\n> > +       };\n> > +       matrix =\n> > +               ycbcr2rgb * saturationMatrix * rgb2ycbcr * matrix;\n> > +}\n> > +\n> > +void Adjust::prepare(IPAContext &context,\n> > +                    const uint32_t frame,\n> > +                    IPAFrameContext &frameContext,\n> > +                    [[maybe_unused]] DebayerParams *params)\n> > +{\n> > +       if (!context.ccmEnabled)\n> > +               return;\n> > +\n> > +       auto &saturation = context.activeState.knobs.saturation;\n> > +\n> > +       if (frame > 0 && saturation == lastSaturation_)\n> > +               return;\n> > +\n> > +       context.activeState.correctionMatrix = context.activeState.ccm;\n> > +       context.activeState.matrixChanged = true;\n> > +       lastSaturation_ = saturation;\n> > +       if (saturation)\n> > +               applySaturation(context.activeState.correctionMatrix, saturation.value());\n> > +\n> > +       frameContext.saturation = saturation;\n> > +}\n> > +\n> > +void Adjust::process([[maybe_unused]] IPAContext &context,\n> > +                    [[maybe_unused]] const uint32_t frame,\n> > +                    IPAFrameContext &frameContext,\n> > +                    [[maybe_unused]] const SwIspStats *stats,\n> > +                    ControlList &metadata)\n> > +{\n> > +       const auto &saturation = frameContext.saturation;\n> > +       metadata.set(controls::Saturation, saturation.value_or(1.0));\n> > +}\n> > +\n> > +REGISTER_IPA_ALGORITHM(Adjust, \"Adjust\")\n> > +\n> > +} /* namespace ipa::soft::algorithms */\n> > +\n> > +} /* namespace libcamera */\n> > diff --git a/src/ipa/simple/algorithms/adjust.h b/src/ipa/simple/algorithms/adjust.h\n> > new file mode 100644\n> > index 000000000..c4baa2503\n> > --- /dev/null\n> > +++ b/src/ipa/simple/algorithms/adjust.h\n> > @@ -0,0 +1,52 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2024-2025, Red Hat Inc.\n> > + *\n> > + * Color correction matrix\n\nYou forgot to rename this.\n\n> > + */\n> > +\n> > +#pragma once\n> > +\n> > +#include <optional>\n> > +\n> > +#include \"libcamera/internal/matrix.h\"\n> > +\n> > +#include <libipa/interpolator.h>\n> > +\n> > +#include \"algorithm.h\"\n> > +\n> > +namespace libcamera {\n> > +\n> > +namespace ipa::soft::algorithms {\n> > +\n> > +class Adjust : public Algorithm\n> > +{\n> > +public:\n> > +       Adjust() = default;\n> > +       ~Adjust() = default;\n> > +\n> > +       int init(IPAContext &context, const YamlObject &tuningData) override;\n> > +       int configure(IPAContext &context,\n> > +                     const IPAConfigInfo &configInfo) override;\n> > +       void queueRequest(typename Module::Context &context,\n> > +                         const uint32_t frame,\n> > +                         typename Module::FrameContext &frameContext,\n> > +                         const ControlList &controls) override;\n> > +       void prepare(IPAContext &context,\n> > +                    const uint32_t frame,\n> > +                    IPAFrameContext &frameContext,\n> > +                    DebayerParams *params) override;\n> > +       void process(IPAContext &context, const uint32_t frame,\n> > +                    IPAFrameContext &frameContext,\n> > +                    const SwIspStats *stats,\n> > +                    ControlList &metadata) override;\n> > +\n> > +private:\n> > +       void applySaturation(Matrix<float, 3, 3> &ccm, float saturation);\n> > +\n> > +       std::optional<float> lastSaturation_;\n> > +};\n> > +\n> > +} /* namespace ipa::soft::algorithms */\n> > +\n> > +} /* namespace libcamera */\n> > diff --git a/src/ipa/simple/algorithms/ccm.cpp b/src/ipa/simple/algorithms/ccm.cpp\n> > index 3e9528a91..c541ef466 100644\n> > --- a/src/ipa/simple/algorithms/ccm.cpp\n> > +++ b/src/ipa/simple/algorithms/ccm.cpp\n> > @@ -3,7 +3,7 @@\n> >   * Copyright (C) 2024, Ideas On Board\n> >   * Copyright (C) 2024-2025, Red Hat Inc.\n> >   *\n> > - * Color correction matrix + saturation\n> > + * Color correction matrix\n> \n> I very much like this ;-)\n> \n> >   */\n> >  \n> >  #include \"ccm.h\"\n> > @@ -37,76 +37,27 @@ int Ccm::init([[maybe_unused]] IPAContext &context, const YamlObject &tuningData\n> >         }\n> >  \n> >         context.ccmEnabled = true;\n> > -       context.ctrlMap[&controls::Saturation] = ControlInfo(0.0f, 2.0f, 1.0f);\n> >  \n> >         return 0;\n> >  }\n> >  \n> > -int Ccm::configure(IPAContext &context,\n> > -                  [[maybe_unused]] const IPAConfigInfo &configInfo)\n> > -{\n> > -       context.activeState.knobs.saturation = std::optional<double>();\n> > -\n> > -       return 0;\n> > -}\n> > -\n> > -void Ccm::queueRequest(typename Module::Context &context,\n> > -                      [[maybe_unused]] const uint32_t frame,\n> > -                      [[maybe_unused]] typename Module::FrameContext &frameContext,\n> > -                      const ControlList &controls)\n> > -{\n> > -       const auto &saturation = controls.get(controls::Saturation);\n> > -       if (saturation.has_value()) {\n> > -               context.activeState.knobs.saturation = saturation;\n> > -               LOG(IPASoftCcm, Debug) << \"Setting saturation to \" << saturation.value();\n> > -       }\n> > -}\n> > -\n> > -void Ccm::applySaturation(Matrix<float, 3, 3> &ccm, float saturation)\n> > -{\n> > -       /* https://en.wikipedia.org/wiki/YCbCr#ITU-R_BT.601_conversion */\n> > -       const Matrix<float, 3, 3> rgb2ycbcr{\n> > -               { 0.256788235294, 0.504129411765, 0.0979058823529,\n> > -                 -0.148223529412, -0.290992156863, 0.439215686275,\n> > -                 0.439215686275, -0.367788235294, -0.0714274509804 }\n> > -       };\n> > -       const Matrix<float, 3, 3> ycbcr2rgb{\n> > -               { 1.16438356164, 0, 1.59602678571,\n> > -                 1.16438356164, -0.391762290094, -0.812967647235,\n> > -                 1.16438356164, 2.01723214285, 0 }\n> > -       };\n> > -       const Matrix<float, 3, 3> saturationMatrix{\n> > -               { 1, 0, 0,\n> > -                 0, saturation, 0,\n> > -                 0, 0, saturation }\n> > -       };\n> > -       ccm = ycbcr2rgb * saturationMatrix * rgb2ycbcr * ccm;\n> > -}\n> \n> aha, never mind - we're not duplciating - we're moving. So ignore the\n> colours namespace requirement for now.\n> \n> > -\n> >  void Ccm::prepare(IPAContext &context, const uint32_t frame,\n> >                   IPAFrameContext &frameContext, [[maybe_unused]] DebayerParams *params)\n> >  {\n> > -       auto &saturation = context.activeState.knobs.saturation;\n> > -\n> >         const unsigned int ct = context.activeState.awb.temperatureK;\n> >  \n> > -       /* Change CCM only on saturation or bigger temperature changes. */\n> > +       /* Change CCM only on bigger temperature changes. */\n> >         if (frame > 0 &&\n> > -           utils::abs_diff(ct, lastCt_) < kTemperatureThreshold &&\n> > -           saturation == lastSaturation_) {\n> > +           utils::abs_diff(ct, lastCt_) < kTemperatureThreshold) {\n> >                 frameContext.ccm = context.activeState.ccm;\n> >                 return;\n> >         }\n> >  \n> >         lastCt_ = ct;\n> > -       lastSaturation_ = saturation;\n> >         Matrix<float, 3, 3> ccm = ccm_.getInterpolated(ct);\n> > -       if (saturation)\n> > -               applySaturation(ccm, saturation.value());\n> >  \n> >         context.activeState.correctionMatrix = ccm;\n> >         context.activeState.ccm = ccm;\n> > -       frameContext.saturation = saturation;\n> >         context.activeState.matrixChanged = true;\n> >         frameContext.ccm = ccm;\n> >  }\n> > @@ -118,9 +69,6 @@ void Ccm::process([[maybe_unused]] IPAContext &context,\n> >                   ControlList &metadata)\n> >  {\n> >         metadata.set(controls::ColourCorrectionMatrix, frameContext.ccm.data());\n> > -\n> > -       const auto &saturation = frameContext.saturation;\n> > -       metadata.set(controls::Saturation, saturation.value_or(1.0));\n> >  }\n> >  \n> >  REGISTER_IPA_ALGORITHM(Ccm, \"Ccm\")\n> > diff --git a/src/ipa/simple/algorithms/ccm.h b/src/ipa/simple/algorithms/ccm.h\n> > index 8279a3d59..10c9829f5 100644\n> > --- a/src/ipa/simple/algorithms/ccm.h\n> > +++ b/src/ipa/simple/algorithms/ccm.h\n> > @@ -26,12 +26,6 @@ public:\n> >         ~Ccm() = default;\n> >  \n> >         int init(IPAContext &context, const YamlObject &tuningData) override;\n> > -       int configure(IPAContext &context,\n> > -                     const IPAConfigInfo &configInfo) override;\n> > -       void queueRequest(typename Module::Context &context,\n> > -                         const uint32_t frame,\n> > -                         typename Module::FrameContext &frameContext,\n> > -                         const ControlList &controls) override;\n> >         void prepare(IPAContext &context,\n> >                      const uint32_t frame,\n> >                      IPAFrameContext &frameContext,\n> > @@ -42,11 +36,9 @@ public:\n> >                      ControlList &metadata) override;\n> >  \n> >  private:\n> > -       void applySaturation(Matrix<float, 3, 3> &ccm, float saturation);\n> > -\n> >         unsigned int lastCt_;\n> > -       std::optional<float> lastSaturation_;\n> >         Interpolator<Matrix<float, 3, 3>> ccm_;\n> > +       Matrix<float, 3, 3> lastCcm_;\n> \n> Some point maybe we should do a \"using Ccm = Matrix<float, 3, 3>;\" as a\n> libipa type ;-)\n\nNot now, there will be too much bikeshedding :-)\n\n> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\n> \n> >  };\n> >  \n> >  } /* namespace ipa::soft::algorithms */\n> > diff --git a/src/ipa/simple/algorithms/meson.build b/src/ipa/simple/algorithms/meson.build\n> > index 2d0adb059..ebe9f20dd 100644\n> > --- a/src/ipa/simple/algorithms/meson.build\n> > +++ b/src/ipa/simple/algorithms/meson.build\n> > @@ -1,6 +1,7 @@\n> >  # SPDX-License-Identifier: CC0-1.0\n> >  \n> >  soft_simple_ipa_algorithms = files([\n> > +    'adjust.cpp',\n> >      'awb.cpp',\n> >      'agc.cpp',\n> >      'blc.cpp',\n> > diff --git a/src/ipa/simple/data/uncalibrated.yaml b/src/ipa/simple/data/uncalibrated.yaml\n> > index 5508e6686..f0410fe61 100644\n> > --- a/src/ipa/simple/data/uncalibrated.yaml\n> > +++ b/src/ipa/simple/data/uncalibrated.yaml\n> > @@ -14,6 +14,7 @@ algorithms:\n> >    #         ccm: [ 1, 0, 0,\n> >    #                0, 1, 0,\n> >    #                0, 0, 1]\n> > +  - Adjust:\n> >    - Lut:\n> >    - Agc:\n> >  ...","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 0AB81C0F1B\n\tfor <parsemail@patchwork.libcamera.org>;\n\tWed, 19 Nov 2025 04:27:28 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A34A660A8B;\n\tWed, 19 Nov 2025 05:27:27 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B1FA560856\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed, 19 Nov 2025 05:27:25 +0100 (CET)","from pendragon.ideasonboard.com (unknown [205.220.129.225])\n\tby perceval.ideasonboard.com (Postfix) with UTF8SMTPSA id B9D24DD9;\n\tWed, 19 Nov 2025 05:25:15 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"gNGLGEl/\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1763526321;\n\tbh=fz0lVK500aLJ27GYyONVAcLokujmvih64OuEu9ZvIdQ=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=gNGLGEl/Wx8/bh707dLlNVVeQYDjxGxHkydyqBWrKLAk+3ttWSjRpyu6NV35371+Z\n\tCG/+z5PtdHvsD/NN5BkP7v75e1jFghGyqpNLW8N9rnOplZoomaB7FHKVhOGI+soggb\n\tQvmZOPqxtzoPtrftnbaaOdGq0RTA1PFasdaC2+aI=","Date":"Wed, 19 Nov 2025 13:26:47 +0900","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Kieran Bingham <kieran.bingham@ideasonboard.com>","Cc":"Milan Zamazal <mzamazal@redhat.com>, libcamera-devel@lists.libcamera.org","Subject":"Re: [RFC PATCH 6/7] libcamera: ipa: simple: Separate saturation from\n\tCCM","Message-ID":"<20251119042647.GQ10711@pendragon.ideasonboard.com>","References":"<20251112082715.17823-1-mzamazal@redhat.com>\n\t<20251112082715.17823-7-mzamazal@redhat.com>\n\t<176294239515.567526.4505528295042314471@ping.linuxembedded.co.uk>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<176294239515.567526.4505528295042314471@ping.linuxembedded.co.uk>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}}]