[{"id":33187,"web_url":"https://patchwork.libcamera.org/comment/33187/","msgid":"<20250126235637.GJ1133@pendragon.ideasonboard.com>","date":"2025-01-26T23:56:37","subject":"Re: [PATCH v4 6/9] libcamera: software_isp: Add CCM algorithm","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Milan,\n\nOn Mon, Jan 13, 2025 at 02:51:03PM +0100, Milan Zamazal wrote:\n> This patch adds color correction matrix (CCM) algorithm to software ISP.\n> It is based on the corresponding algorithm in rkisp1.\n> \n> The primary difference against hardware pipelines is that applying the\n> CCM is optional. Applying CCM causes a significant slowdown, time\n> needed to process a frame raises by 40-90% on tested platforms. If CCM\n> is really needed, it can be applied, if not, it's better to stick\n> without it. This can be configured by presence or omission of Ccm\n> algorithm in the tuning file.\n> \n> CCM is changed only if the determined temperature changes by at least\n> 100 K (an arbitrarily selected value), to avoid recomputing the matrices\n> and lookup tables all the time.\n> \n> The outputs of the algorithm are not used yet, they will be enabled in\n> followup patches.\n> \n> Signed-off-by: Milan Zamazal \n> Reviewed-by: Kieran Bingham \n> ---\n> src/ipa/simple/algorithms/ccm.cpp | 88 +++++++++++++++++++++++++++\n> src/ipa/simple/algorithms/ccm.h | 45 ++++++++++++++\n> src/ipa/simple/algorithms/meson.build | 1 +\n> src/ipa/simple/ipa_context.h | 13 ++++\n> 4 files changed, 147 insertions(+)\n> create mode 100644 src/ipa/simple/algorithms/ccm.cpp\n> create mode 100644 src/ipa/simple/algorithms/ccm.h\n> \n> diff --git a/src/ipa/simple/algorithms/ccm.cpp b/src/ipa/simple/algorithms/ccm.cpp\n> new file mode 100644\n> index 00000000..3c7fca2d\n> --- /dev/null\n> +++ b/src/ipa/simple/algorithms/ccm.cpp\n> @@ -0,0 +1,88 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2024, Ideas On Board\n> + * Copyright (C) 2024, Red Hat Inc.\n> + *\n> + * Color correction matrix\n> + */\n> +\n> +#include \"ccm.h\"\n> +\n> +#include \n\nI think this isn't needed anymore.\n\n> +\n> +#include \n> +#include \n> +\n> +#include \n> +\n> +namespace libcamera {\n> +\n> +namespace ipa::soft::algorithms {\n> +\n> +LOG_DEFINE_CATEGORY(IPASoftCcm)\n> +\n> +unsigned int Ccm::kTemperatureThreshold = 100;\n> +\n> +int Ccm::init([[maybe_unused]] IPAContext &context, const YamlObject &tuningData)\n> +{\n> +\tint ret = ccm_.readYaml(tuningData[\"ccms\"], \"ct\", \"ccm\");\n> +\tif (ret < 0) {\n> +\t\tLOG(IPASoftCcm, Warning)\n> +\t\t\t<< \"Failed to parse 'ccm' \"\n> +\t\t\t<< \"parameter from tuning file; falling back to unit matrix\";\n\ns/unit/identity/\n\nBut is this message correct ? I don't know you fall back to the identify\nmatrix. Maybe \"disabling CCM\" would be better ?\n\n> +\t\tccmEnabled_ = false;\n\nIs there a reason not to return an error here ? What's the use case for\nspecifying a CCM algorithm in the tuning file with valid data ? I\nunderstand you want to support disabling CCM, and that's easily done by\nnot listing the algorithm in the tuning file at all. Unless I'm missing\nsomething, you can return an error here, and drop the ccmEnabled_\nvariable.\n\n> +\t} else {\n> +\t\tccmEnabled_ = true;\n> +\t}\n> +\n> +\treturn 0;\n> +}\n> +\n> +void Ccm::prepare(IPAContext &context, const uint32_t frame,\n> +\t\t IPAFrameContext &frameContext, [[maybe_unused]] DebayerParams *params)\n> +{\n> +\tcontext.activeState.ccm.enabled = ccmEnabled_;\n> +\n> +\tif (!ccmEnabled_)\n> +\t\treturn;\n> +\n> +\tunsigned int ct = context.activeState.awb.temperatureK;\n\nconst\n\n> +\n> +\t/* Change CCM only on bigger temperature changes. */\n> +\tif (frame > 0 &&\n> +\t utils::abs_diff(ct, ct_) < kTemperatureThreshold) {\n> +\t\tframeContext.ccm.ccm = context.activeState.ccm.ccm;\n> +\t\tcontext.activeState.ccm.changed = false;\n> +\t\treturn;\n> +\t}\n> +\n> +\tct_ = ct;\n\nRenaming ct_ to lastCt_ would make the code clearer in my opinion.\n\n> +\tMatrix ccm = ccm_.getInterpolated(ct);\n> +\n> +\tcontext.activeState.ccm.ccm = ccm;\n> +\tframeContext.ccm.ccm = ccm;\n> +\tcontext.activeState.ccm.changed = true;\n> +}\n> +\n> +void Ccm::process([[maybe_unused]] IPAContext &context,\n> +\t\t [[maybe_unused]] const uint32_t frame,\n> +\t\t IPAFrameContext &frameContext,\n> +\t\t [[maybe_unused]] const SwIspStats *stats,\n> +\t\t ControlList &metadata)\n> +{\n> +\tif (!ccmEnabled_)\n> +\t\treturn;\n> +\n> +\tfloat m[9];\n> +\tfor (unsigned int i = 0; i < 3; i++) {\n> +\t\tfor (unsigned int j = 0; j < 3; j++)\n> +\t\t\tm[i * 3 + j] = frameContext.ccm.ccm[i][j];\n> +\t}\n\nI wonder if the Matrix class should expose its data_ member with a\nread-only accessor, to avoid this kind of copies. Not a candidate for\nthis patch.\n\n> +\tmetadata.set(controls::ColourCorrectionMatrix, m);\n> +}\n> +\n> +REGISTER_IPA_ALGORITHM(Ccm, \"Ccm\")\n> +\n> +} /* namespace ipa::soft::algorithms */\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/ipa/simple/algorithms/ccm.h b/src/ipa/simple/algorithms/ccm.h\n> new file mode 100644\n> index 00000000..23481a08\n> --- /dev/null\n> +++ b/src/ipa/simple/algorithms/ccm.h\n> @@ -0,0 +1,45 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2024, Red Hat Inc.\n> + *\n> + * Color correction matrix\n> + */\n> +\n> +#pragma once\n> +\n> +#include \"libcamera/internal/matrix.h\"\n> +\n> +#include \n> +\n> +#include \"algorithm.h\"\n> +\n> +namespace libcamera {\n> +\n> +namespace ipa::soft::algorithms {\n> +\n> +class Ccm : public Algorithm\n> +{\n> +public:\n> +\tCcm() = default;\n> +\t~Ccm() = default;\n> +\n> +\tint init(IPAContext &context, const YamlObject &tuningData) override;\n> +\tvoid prepare(IPAContext &context,\n> +\t\t const uint32_t frame,\n> +\t\t IPAFrameContext &frameContext,\n> +\t\t DebayerParams *params) override;\n> +\tvoid process(IPAContext &context, const uint32_t frame,\n> +\t\t IPAFrameContext &frameContext,\n> +\t\t const SwIspStats *stats,\n> +\t\t ControlList &metadata) override;\n> +\n> +private:\n> +\tstatic unsigned int kTemperatureThreshold;\n\n\tstatic constexpr unsigned int kTemperatureThreshold = 100;\n\nThis could also be a global variable in ccm.cpp.\n\n> +\tunsigned int ct_;\n> +\tbool ccmEnabled_;\n> +\tInterpolator> ccm_;\n> +};\n> +\n> +} /* namespace ipa::soft::algorithms */\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/ipa/simple/algorithms/meson.build b/src/ipa/simple/algorithms/meson.build\n> index 37a2eb53..2d0adb05 100644\n> --- a/src/ipa/simple/algorithms/meson.build\n> +++ b/src/ipa/simple/algorithms/meson.build\n> @@ -4,5 +4,6 @@ soft_simple_ipa_algorithms = files([\n> 'awb.cpp',\n> 'agc.cpp',\n> 'blc.cpp',\n> + 'ccm.cpp',\n> 'lut.cpp',\n> ])\n> diff --git a/src/ipa/simple/ipa_context.h b/src/ipa/simple/ipa_context.h\n> index 607af45a..0def3eef 100644\n> --- a/src/ipa/simple/ipa_context.h\n> +++ b/src/ipa/simple/ipa_context.h\n> @@ -13,6 +13,8 @@\n> \n> #include \n> \n> +#include \"libcamera/internal/matrix.h\"\n> +\n> #include \n> \n> namespace libcamera {\n> @@ -50,6 +52,13 @@ struct IPAActiveState {\n> \t\tuint8_t blackLevel;\n> \t\tdouble contrast;\n> \t} gamma;\n> +\n> +\tstruct {\n> +\t\tMatrix ccm;\n> +\t\tbool enabled;\n> +\t\tbool changed;\n> +\t} ccm;\n> +\n> \tstruct {\n> \t\t/* 0..2 range, 1.0 = normal */\n> \t\tstd::optional contrast;\n> @@ -57,6 +66,10 @@ struct IPAActiveState {\n> };\n> \n> struct IPAFrameContext : public FrameContext {\n> +\tstruct {\n> +\t\tMatrix ccm;\n> +\t} ccm;\n> +\n> \tstruct {\n> \t\tint32_t exposure;\n> \t\tdouble gain;","headers":{"Return-Path":"","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 55FFABEFBE\n\tfor ;\n\tSun, 26 Jan 2025 23:56:53 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5965F6855D;\n\tMon, 27 Jan 2025 00:56:52 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 70F026187B\n\tfor ;\n\tMon, 27 Jan 2025 00:56:50 +0100 (CET)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 02470496;\n\tMon, 27 Jan 2025 00:55:43 +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=\"tg0w00Wu\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1737935744;\n\tbh=mB75P9jiUz8puIio38VtZFC2xuOPxfKdZsjTyuTEqtg=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=tg0w00WuHAvHJwN5cdLM+1ry/9rDbTcZlopAMWdMEBW7ekLvoIeN/sYQ55njylv60\n\txaE3PdjAWmi/NkonknkYLF+4cA9gTbA+BkLQq7x4/9rgJmWMZiN6y7JIJd26WdIs9q\n\tUeOh8BuyGW1ZBqBt/5vfgoccSvZDE8vZjhGEFrkA=","Date":"Mon, 27 Jan 2025 01:56:37 +0200","From":"Laurent Pinchart ","To":"Milan Zamazal ","Cc":"libcamera-devel@lists.libcamera.org,\n\tRobert Mader ,\n\tHans de Goede ,\n\tKieran Bingham ","Subject":"Re: [PATCH v4 6/9] libcamera: software_isp: Add CCM algorithm","Message-ID":"<20250126235637.GJ1133@pendragon.ideasonboard.com>","References":"<20250113135108.13924-1-mzamazal@redhat.com>\n\t<20250113135108.13924-7-mzamazal@redhat.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20250113135108.13924-7-mzamazal@redhat.com>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]