[{"id":29833,"web_url":"https://patchwork.libcamera.org/comment/29833/","msgid":"<171811516483.1148289.15469360068204996564@ping.linuxembedded.co.uk>","date":"2024-06-11T14:12:44","subject":"Re: [PATCH v7 3/3] ipa: rkisp1: algorithms: Add crosstalk algorithm","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Paul Elder (2024-06-11 15:02:07)\n> Add an algorithm module to the rkisp1 IPA for crosstalk correction.\n> \n> Signed-off-by: Paul Elder \n> Reviewed-by: Stefan Klug \n> \n> ---\n> Changes in v7:\n> - make offsets_ default to zero-matrices as opposed to identity matrices\n> - checkstyle\n> - populate metadata\n> - add ccm to IPAFrameContext\n> - don't update the ccm if the color temperature didn't change\n> \n> No change in v6\n> \n> Changes in v5:\n> - clean up documentation\n> - coalesce parseYaml into init\n> \n> Changes in v4:\n> - remove stray semicolons\n> - use the new matrix interpolator readYaml\n> - use the new matrix operator[] getter\n> \n> Changes in v3:\n> - read ccm offsets from tuning data, and write these offsets to the\n> parameters buffer\n> - make parseYaml return void, as it should fill in default data if\n> unable to read, thus never failing\n> \n> Changes in v2:\n> - rename ctk to ccm\n> - reset the matrix interpolator to identity matrix if failed to read\n> from tuning file\n> ---\n> src/ipa/rkisp1/algorithms/ccm.cpp | 140 ++++++++++++++++++++++++++\n> src/ipa/rkisp1/algorithms/ccm.h | 50 +++++++++\n> src/ipa/rkisp1/algorithms/meson.build | 1 +\n> src/ipa/rkisp1/ipa_context.h | 5 +\n> 4 files changed, 196 insertions(+)\n> create mode 100644 src/ipa/rkisp1/algorithms/ccm.cpp\n> create mode 100644 src/ipa/rkisp1/algorithms/ccm.h\n> \n> diff --git a/src/ipa/rkisp1/algorithms/ccm.cpp b/src/ipa/rkisp1/algorithms/ccm.cpp\n> new file mode 100644\n> index 000000000000..09fe4b2aa1bc\n> --- /dev/null\n> +++ b/src/ipa/rkisp1/algorithms/ccm.cpp\n> @@ -0,0 +1,140 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2024, Ideas On Board\n> + *\n> + * RkISP1 Color Correction Matrix control algorithm\n> + */\n> +\n> +#include \"ccm.h\"\n> +\n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +\n> +#include \n> +#include \n> +\n> +#include \n> +\n> +#include \n> +\n> +#include \"libcamera/internal/yaml_parser.h\"\n> +\n> +#include \"../utils.h\"\n> +#include \"libipa/matrix_interpolator.h\"\n> +\n> +/**\n> + * \\file ccm.h\n> + */\n> +\n> +namespace libcamera {\n> +\n> +namespace ipa::rkisp1::algorithms {\n> +\n> +/**\n> + * \\class Ccm\n> + * \\brief A color correction matrix algorithm\n> + */\n> +\n> +LOG_DEFINE_CATEGORY(RkISP1Ccm)\n> +\n> +/**\n> + * \\copydoc libcamera::ipa::Algorithm::init\n> + */\n> +int Ccm::init([[maybe_unused]] IPAContext &context, const YamlObject &tuningData)\n> +{\n> + int ret = ccm_.readYaml(tuningData[\"ccms\"], \"ct\", \"ccm\");\n> + if (ret < 0) {\n> + LOG(RkISP1Ccm, Warning)\n> + << \"Failed to parse 'ccm' \"\n> + << \"parameter from tuning file; falling back to unit matrix\";\n> + ccm_.reset();\n> + }\n> +\n> + ret = offsets_.readYaml(tuningData[\"ccms\"], \"ct\", \"offsets\");\n> + if (ret < 0) {\n> + LOG(RkISP1Ccm, Warning)\n> + << \"Failed to parse 'offsets' \"\n> + << \"parameter from tuning file; falling back to zero offsets\";\n> + /*\n> + * MatrixInterpolator::reset() resets to identity matrices\n> + * while here we need zero matrices so we need to construct it\n> + * ourselves.\n> + */\n> + Matrix m({ 0, 0, 0 });\n> + std::map> matrices = { { 0, m } };\n> + offsets_ = MatrixInterpolator(matrices);\n> + }\n> +\n> + return 0;\n> +}\n> +\n> +void Ccm::setParameters(rkisp1_params_cfg *params,\n> + const Matrix &matrix,\n> + const Matrix &offsets)\n> +{\n> + struct rkisp1_cif_isp_ctk_config &config = params->others.ctk_config;\n> +\n> + /*\n> + * 4 bit integer and 7 bit fractional, ranging from -8 (0x400) to\n> + * +7.992 (0x3ff)\n> + */\n> + for (unsigned int i = 0; i < 3; i++)\n> + for (unsigned int j = 0; j < 3; j++)\n> + config.coeff[i][j] =\n> + utils::floatingToFixedPoint<4, 7, uint16_t, double>(matrix[i][j]);\n> +\n> + for (unsigned int i = 0; i < 3; i++)\n> + config.ct_offset[i] = offsets[i][0] & 0xfff;\n> +\n> + LOG(RkISP1Ccm, Debug) << \"Setting matrix \" << matrix;\n> + LOG(RkISP1Ccm, Debug) << \"Setting offsets \" << offsets;\n> +\n> + params->module_en_update |= RKISP1_CIF_ISP_MODULE_CTK;\n> + params->module_ens |= RKISP1_CIF_ISP_MODULE_CTK;\n> + params->module_cfg_update |= RKISP1_CIF_ISP_MODULE_CTK;\n> +}\n> +\n> +/**\n> + * \\copydoc libcamera::ipa::Algorithm::prepare\n> + */\n> +void Ccm::prepare(IPAContext &context, [[maybe_unused]] const uint32_t frame,\n> + IPAFrameContext &frameContext,\n> + rkisp1_params_cfg *params)\n> +{\n> + uint32_t ct = context.activeState.awb.temperatureK;\n> + if (ct == ct_)\n> + return;\n\nInterestingly, I think this is fine (certainly for now) ... but this\nmeans that the output request metadata will only write the CCM if it\nchanges.\n\nI think that's actually 'fine' if we imply that any frame that doesn't\nsupply metadata is the same as the previous state. Though I think RPi\ncurrently reports it for every frame regardless.\n\nI'd be curious to see how Stefan's tooling handles this.\n\nWe can always update on top, and my comments are addressed so:\n\nReviewed-by: Kieran Bingham \n\n\n> +\n> + ct_ = ct;\n> + Matrix ccm = ccm_.get(ct);\n> + Matrix offsets = offsets_.get(ct);\n> +\n> + frameContext.ccm.ccm = ccm;\n> +\n> + setParameters(params, ccm, offsets);\n> +}\n> +\n> +/**\n> + * \\copydoc libcamera::ipa::Algorithm::process\n> + */\n> +void process([[maybe_unused]] IPAContext &context,\n> + [[maybe_unused]] const uint32_t frame,\n> + IPAFrameContext &frameContext,\n> + [[maybe_unused]] const rkisp1_stat_buffer *stats,\n> + ControlList &metadata)\n> +{\n> + float m[9];\n> + for (unsigned int i = 0; i < 3; i++)\n> + for (unsigned int j = 0; j < 3; j++)\n> + m[i] = frameContext.ccm.ccm[i][j];\n> + metadata.set(controls::ColourCorrectionMatrix, m);\n> +}\n> +\n> +REGISTER_IPA_ALGORITHM(Ccm, \"Ccm\")\n> +\n> +} /* namespace ipa::rkisp1::algorithms */\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/ipa/rkisp1/algorithms/ccm.h b/src/ipa/rkisp1/algorithms/ccm.h\n> new file mode 100644\n> index 000000000000..09a6801626b4\n> --- /dev/null\n> +++ b/src/ipa/rkisp1/algorithms/ccm.h\n> @@ -0,0 +1,50 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2024, Ideas On Board\n> + *\n> + * RkISP1 Color Correction Matrix control algorithm\n> + */\n> +\n> +#pragma once\n> +\n> +#include \n> +\n> +#include \"libipa/matrix.h\"\n> +#include \"libipa/matrix_interpolator.h\"\n> +\n> +#include \"algorithm.h\"\n> +\n> +namespace libcamera {\n> +\n> +namespace ipa::rkisp1::algorithms {\n> +\n> +class Ccm : public Algorithm\n> +{\n> +public:\n> + Ccm() {}\n> + ~Ccm() = default;\n> +\n> + int init(IPAContext &context, const YamlObject &tuningData) override;\n> + void prepare(IPAContext &context, const uint32_t frame,\n> + IPAFrameContext &frameContext,\n> + rkisp1_params_cfg *params) override;\n> + void process([[maybe_unused]] IPAContext &context,\n> + [[maybe_unused]] const uint32_t frame,\n> + IPAFrameContext &frameContext,\n> + [[maybe_unused]] const rkisp1_stat_buffer *stats,\n> + ControlList &metadata) override;\n> +\n> +private:\n> + void parseYaml(const YamlObject &tuningData);\n> + void setParameters(rkisp1_params_cfg *params,\n> + const Matrix &matrix,\n> + const Matrix &offsets);\n> +\n> + unsigned int ct_;\n> + MatrixInterpolator ccm_;\n> + MatrixInterpolator offsets_;\n> +};\n> +\n> +} /* namespace ipa::rkisp1::algorithms */\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/ipa/rkisp1/algorithms/meson.build b/src/ipa/rkisp1/algorithms/meson.build\n> index 6ee71a9b5da3..1734a6675f78 100644\n> --- a/src/ipa/rkisp1/algorithms/meson.build\n> +++ b/src/ipa/rkisp1/algorithms/meson.build\n> @@ -4,6 +4,7 @@ rkisp1_ipa_algorithms = files([\n> 'agc.cpp',\n> 'awb.cpp',\n> 'blc.cpp',\n> + 'ccm.cpp',\n> 'cproc.cpp',\n> 'dpcc.cpp',\n> 'dpf.cpp',\n> diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h\n> index 2a994d81ae41..cfb1f9770870 100644\n> --- a/src/ipa/rkisp1/ipa_context.h\n> +++ b/src/ipa/rkisp1/ipa_context.h\n> @@ -16,6 +16,7 @@\n> #include \n> \n> #include \n> +#include \n> \n> namespace libcamera {\n> \n> @@ -155,6 +156,10 @@ struct IPAFrameContext : public FrameContext {\n> uint32_t exposure;\n> double gain;\n> } sensor;\n> +\n> + struct {\n> + Matrix ccm;\n> + } ccm;\n> };\n> \n> struct IPAContext {\n> -- \n> 2.39.2\n>","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 7B934BD87C\n\tfor ;\n\tTue, 11 Jun 2024 14:12:51 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 50DFE65456;\n\tTue, 11 Jun 2024 16:12:50 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 249A961A26\n\tfor ;\n\tTue, 11 Jun 2024 16:12:48 +0200 (CEST)","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 3450129A;\n\tTue, 11 Jun 2024 16:12:35 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"GltDUHAG\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1718115155;\n\tbh=FtVgZB2sNJ5CHT7KO+jELAeqP0m8RXLLeodpO2bq9Vk=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=GltDUHAG5jp7x1rDMFQ5S7q0n3oKs9zXVKAtqfQskZR40Ax69isxeLfxoEe3bDK/g\n\tT2HsJYmuDYcofHSNOrqYEZH1p4eTwZUCzA4jpsQYTGhh3n9uPO8s6H4M8Umm6+PpOd\n\txPTiGm1WEvpenxb+D8PhTL7GhfeBtnoRq3rwdwW4=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20240611140207.520083-4-paul.elder@ideasonboard.com>","References":"<20240611140207.520083-1-paul.elder@ideasonboard.com>\n\t<20240611140207.520083-4-paul.elder@ideasonboard.com>","Subject":"Re: [PATCH v7 3/3] ipa: rkisp1: algorithms: Add crosstalk algorithm","From":"Kieran Bingham ","Cc":"Paul Elder ,\n\tStefan Klug ","To":"Paul Elder ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Tue, 11 Jun 2024 15:12:44 +0100","Message-ID":"<171811516483.1148289.15469360068204996564@ping.linuxembedded.co.uk>","User-Agent":"alot/0.10","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\" "}},{"id":29855,"web_url":"https://patchwork.libcamera.org/comment/29855/","msgid":"<20240611235120.GF15006@pendragon.ideasonboard.com>","date":"2024-06-11T23:51:20","subject":"Re: [PATCH v7 3/3] ipa: rkisp1: algorithms: Add crosstalk algorithm","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Paul,\n\nThank you for the patch.\n\nOn Tue, Jun 11, 2024 at 03:12:44PM +0100, Kieran Bingham wrote:\n> Quoting Paul Elder (2024-06-11 15:02:07)\n> > Add an algorithm module to the rkisp1 IPA for crosstalk correction.\n> > \n> > Signed-off-by: Paul Elder \n> > Reviewed-by: Stefan Klug \n> > \n> > ---\n> > Changes in v7:\n> > - make offsets_ default to zero-matrices as opposed to identity matrices\n> > - checkstyle\n> > - populate metadata\n> > - add ccm to IPAFrameContext\n> > - don't update the ccm if the color temperature didn't change\n> > \n> > No change in v6\n> > \n> > Changes in v5:\n> > - clean up documentation\n> > - coalesce parseYaml into init\n> > \n> > Changes in v4:\n> > - remove stray semicolons\n> > - use the new matrix interpolator readYaml\n> > - use the new matrix operator[] getter\n> > \n> > Changes in v3:\n> > - read ccm offsets from tuning data, and write these offsets to the\n> > parameters buffer\n> > - make parseYaml return void, as it should fill in default data if\n> > unable to read, thus never failing\n> > \n> > Changes in v2:\n> > - rename ctk to ccm\n> > - reset the matrix interpolator to identity matrix if failed to read\n> > from tuning file\n> > ---\n> > src/ipa/rkisp1/algorithms/ccm.cpp | 140 ++++++++++++++++++++++++++\n> > src/ipa/rkisp1/algorithms/ccm.h | 50 +++++++++\n> > src/ipa/rkisp1/algorithms/meson.build | 1 +\n> > src/ipa/rkisp1/ipa_context.h | 5 +\n> > 4 files changed, 196 insertions(+)\n> > create mode 100644 src/ipa/rkisp1/algorithms/ccm.cpp\n> > create mode 100644 src/ipa/rkisp1/algorithms/ccm.h\n> > \n> > diff --git a/src/ipa/rkisp1/algorithms/ccm.cpp b/src/ipa/rkisp1/algorithms/ccm.cpp\n> > new file mode 100644\n> > index 000000000000..09fe4b2aa1bc\n> > --- /dev/null\n> > +++ b/src/ipa/rkisp1/algorithms/ccm.cpp\n> > @@ -0,0 +1,140 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2024, Ideas On Board\n> > + *\n> > + * RkISP1 Color Correction Matrix control algorithm\n> > + */\n> > +\n> > +#include \"ccm.h\"\n> > +\n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +\n> > +#include \n> > +#include \n> > +\n> > +#include \n> > +\n> > +#include \n> > +\n> > +#include \"libcamera/internal/yaml_parser.h\"\n> > +\n> > +#include \"../utils.h\"\n> > +#include \"libipa/matrix_interpolator.h\"\n> > +\n> > +/**\n> > + * \\file ccm.h\n> > + */\n> > +\n> > +namespace libcamera {\n> > +\n> > +namespace ipa::rkisp1::algorithms {\n> > +\n> > +/**\n> > + * \\class Ccm\n> > + * \\brief A color correction matrix algorithm\n> > + */\n> > +\n> > +LOG_DEFINE_CATEGORY(RkISP1Ccm)\n> > +\n> > +/**\n> > + * \\copydoc libcamera::ipa::Algorithm::init\n> > + */\n> > +int Ccm::init([[maybe_unused]] IPAContext &context, const YamlObject &tuningData)\n> > +{\n> > + int ret = ccm_.readYaml(tuningData[\"ccms\"], \"ct\", \"ccm\");\n> > + if (ret < 0) {\n> > + LOG(RkISP1Ccm, Warning)\n> > + << \"Failed to parse 'ccm' \"\n> > + << \"parameter from tuning file; falling back to unit matrix\";\n> > + ccm_.reset();\n> > + }\n> > +\n> > + ret = offsets_.readYaml(tuningData[\"ccms\"], \"ct\", \"offsets\");\n> > + if (ret < 0) {\n> > + LOG(RkISP1Ccm, Warning)\n> > + << \"Failed to parse 'offsets' \"\n> > + << \"parameter from tuning file; falling back to zero offsets\";\n> > + /*\n> > + * MatrixInterpolator::reset() resets to identity matrices\n> > + * while here we need zero matrices so we need to construct it\n> > + * ourselves.\n> > + */\n> > + Matrix m({ 0, 0, 0 });\n> > + std::map> matrices = { { 0, m } };\n> > + offsets_ = MatrixInterpolator(matrices);\n> > + }\n> > +\n> > + return 0;\n> > +}\n> > +\n> > +void Ccm::setParameters(rkisp1_params_cfg *params,\n> > + const Matrix &matrix,\n> > + const Matrix &offsets)\n> > +{\n> > + struct rkisp1_cif_isp_ctk_config &config = params->others.ctk_config;\n> > +\n> > + /*\n> > + * 4 bit integer and 7 bit fractional, ranging from -8 (0x400) to\n> > + * +7.992 (0x3ff)\n> > + */\n> > + for (unsigned int i = 0; i < 3; i++)\n> > + for (unsigned int j = 0; j < 3; j++)\n> > + config.coeff[i][j] =\n> > + utils::floatingToFixedPoint<4, 7, uint16_t, double>(matrix[i][j]);\n\nCurly braces for outer loop.\n\n> > +\n> > + for (unsigned int i = 0; i < 3; i++)\n> > + config.ct_offset[i] = offsets[i][0] & 0xfff;\n> > +\n> > + LOG(RkISP1Ccm, Debug) << \"Setting matrix \" << matrix;\n> > + LOG(RkISP1Ccm, Debug) << \"Setting offsets \" << offsets;\n> > +\n> > + params->module_en_update |= RKISP1_CIF_ISP_MODULE_CTK;\n> > + params->module_ens |= RKISP1_CIF_ISP_MODULE_CTK;\n> > + params->module_cfg_update |= RKISP1_CIF_ISP_MODULE_CTK;\n> > +}\n> > +\n> > +/**\n> > + * \\copydoc libcamera::ipa::Algorithm::prepare\n> > + */\n> > +void Ccm::prepare(IPAContext &context, [[maybe_unused]] const uint32_t frame,\n> > + IPAFrameContext &frameContext,\n> > + rkisp1_params_cfg *params)\n> > +{\n> > + uint32_t ct = context.activeState.awb.temperatureK;\n> > + if (ct == ct_)\n\nct_ should be initialized/reset in configure(), to ensure that we'll\nalways set the CCM matrix on the first frame.\n\nI expect temperatureK to be fairly noisy, should we add a threshold to\nthe comparison to avoid updating the CCM parameters on every frame in\npractice ?\n\n> > + return;\n> \n> Interestingly, I think this is fine (certainly for now) ... but this\n> means that the output request metadata will only write the CCM if it\n> changes.\n> \n> I think that's actually 'fine' if we imply that any frame that doesn't\n> supply metadata is the same as the previous state. Though I think RPi\n> currently reports it for every frame regardless.\n> \n> I'd be curious to see how Stefan's tooling handles this.\n> \n> We can always update on top, and my comments are addressed so:\n> \n> Reviewed-by: Kieran Bingham \n> \n> > +\n> > + ct_ = ct;\n> > + Matrix ccm = ccm_.get(ct);\n> > + Matrix offsets = offsets_.get(ct);\n> > +\n> > + frameContext.ccm.ccm = ccm;\n> > +\n> > + setParameters(params, ccm, offsets);\n> > +}\n> > +\n> > +/**\n> > + * \\copydoc libcamera::ipa::Algorithm::process\n> > + */\n> > +void process([[maybe_unused]] IPAContext &context,\n> > + [[maybe_unused]] const uint32_t frame,\n> > + IPAFrameContext &frameContext,\n> > + [[maybe_unused]] const rkisp1_stat_buffer *stats,\n> > + ControlList &metadata)\n> > +{\n> > + float m[9];\n> > + for (unsigned int i = 0; i < 3; i++)\n> > + for (unsigned int j = 0; j < 3; j++)\n> > + m[i] = frameContext.ccm.ccm[i][j];\n\nCurly braces for outer loop.\n\n> > + metadata.set(controls::ColourCorrectionMatrix, m);\n\nIt's interesting we're reporting the matrix but not the offsets. Not a\nblocker for this series, but should this be addressed ?\n\n> > +}\n> > +\n> > +REGISTER_IPA_ALGORITHM(Ccm, \"Ccm\")\n> > +\n> > +} /* namespace ipa::rkisp1::algorithms */\n> > +\n> > +} /* namespace libcamera */\n> > diff --git a/src/ipa/rkisp1/algorithms/ccm.h b/src/ipa/rkisp1/algorithms/ccm.h\n> > new file mode 100644\n> > index 000000000000..09a6801626b4\n> > --- /dev/null\n> > +++ b/src/ipa/rkisp1/algorithms/ccm.h\n> > @@ -0,0 +1,50 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2024, Ideas On Board\n> > + *\n> > + * RkISP1 Color Correction Matrix control algorithm\n> > + */\n> > +\n> > +#pragma once\n> > +\n> > +#include \n> > +\n> > +#include \"libipa/matrix.h\"\n> > +#include \"libipa/matrix_interpolator.h\"\n> > +\n> > +#include \"algorithm.h\"\n> > +\n> > +namespace libcamera {\n> > +\n> > +namespace ipa::rkisp1::algorithms {\n> > +\n> > +class Ccm : public Algorithm\n> > +{\n> > +public:\n> > + Ccm() {}\n> > + ~Ccm() = default;\n> > +\n> > + int init(IPAContext &context, const YamlObject &tuningData) override;\n> > + void prepare(IPAContext &context, const uint32_t frame,\n> > + IPAFrameContext &frameContext,\n> > + rkisp1_params_cfg *params) override;\n> > + void process([[maybe_unused]] IPAContext &context,\n\n[[maybe_unused]] is not needed in the function declaration.\n\n> > + [[maybe_unused]] const uint32_t frame,\n> > + IPAFrameContext &frameContext,\n> > + [[maybe_unused]] const rkisp1_stat_buffer *stats,\n> > + ControlList &metadata) override;\n> > +\n> > +private:\n> > + void parseYaml(const YamlObject &tuningData);\n> > + void setParameters(rkisp1_params_cfg *params,\n> > + const Matrix &matrix,\n> > + const Matrix &offsets);\n> > +\n> > + unsigned int ct_;\n> > + MatrixInterpolator ccm_;\n\nGiven that the hardware representation is a 4.7 fixed point, would it be\nenough to use floats instead of doubles (here and in the frame context)\n?\n\n> > + MatrixInterpolator offsets_;\n\nWhy is the matrix stored in floating point values and the offsets as\nintegers ?\n\n> > +};\n> > +\n> > +} /* namespace ipa::rkisp1::algorithms */\n> > +\n> > +} /* namespace libcamera */\n> > diff --git a/src/ipa/rkisp1/algorithms/meson.build b/src/ipa/rkisp1/algorithms/meson.build\n> > index 6ee71a9b5da3..1734a6675f78 100644\n> > --- a/src/ipa/rkisp1/algorithms/meson.build\n> > +++ b/src/ipa/rkisp1/algorithms/meson.build\n> > @@ -4,6 +4,7 @@ rkisp1_ipa_algorithms = files([\n> > 'agc.cpp',\n> > 'awb.cpp',\n> > 'blc.cpp',\n> > + 'ccm.cpp',\n> > 'cproc.cpp',\n> > 'dpcc.cpp',\n> > 'dpf.cpp',\n> > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h\n> > index 2a994d81ae41..cfb1f9770870 100644\n> > --- a/src/ipa/rkisp1/ipa_context.h\n> > +++ b/src/ipa/rkisp1/ipa_context.h\n> > @@ -16,6 +16,7 @@\n> > #include \n> > \n> > #include \n> > +#include \n> > \n> > namespace libcamera {\n> > \n> > @@ -155,6 +156,10 @@ struct IPAFrameContext : public FrameContext {\n> > uint32_t exposure;\n> > double gain;\n> > } sensor;\n> > +\n> > + struct {\n> > + Matrix ccm;\n> > + } ccm;\n> > };\n> > \n> > struct IPAContext {","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 355D8C3237\n\tfor ;\n\tTue, 11 Jun 2024 23:51:43 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 33C5A65456;\n\tWed, 12 Jun 2024 01:51:42 +0200 (CEST)","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 881E361A26\n\tfor ;\n\tWed, 12 Jun 2024 01:51:40 +0200 (CEST)","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 64AC3B3;\n\tWed, 12 Jun 2024 01:51:27 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"KnIFQ6dE\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1718149887;\n\tbh=/bcVgGrbKglGeq2Ck0FoB5WmDVP/Xth/VuXg0+2Il8E=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=KnIFQ6dEP0/TzbxhnPVvVOjPdOUKxVS2cpS/B7P8xJUOfTBmYd98vHfWZ/ZO3YoWu\n\tE4thPveqrz6Oypvg61vwx9d/bz613PshtSuRj/CzYIxCCZhN5kx2a5xUVUjuS9gjOJ\n\tWcIFcuJGx247CEVSGJWrhK1YVrgFGc+uRCl09mNw=","Date":"Wed, 12 Jun 2024 02:51:20 +0300","From":"Laurent Pinchart ","To":"Kieran Bingham ","Cc":"Paul Elder ,\n\tlibcamera-devel@lists.libcamera.org,\n\tStefan Klug ","Subject":"Re: [PATCH v7 3/3] ipa: rkisp1: algorithms: Add crosstalk algorithm","Message-ID":"<20240611235120.GF15006@pendragon.ideasonboard.com>","References":"<20240611140207.520083-1-paul.elder@ideasonboard.com>\n\t<20240611140207.520083-4-paul.elder@ideasonboard.com>\n\t<171811516483.1148289.15469360068204996564@ping.linuxembedded.co.uk>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<171811516483.1148289.15469360068204996564@ping.linuxembedded.co.uk>","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\" "}},{"id":29868,"web_url":"https://patchwork.libcamera.org/comment/29868/","msgid":"","date":"2024-06-12T08:04:38","subject":"Re: [PATCH v7 3/3] ipa: rkisp1: algorithms: Add crosstalk algorithm","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"On Wed, Jun 12, 2024 at 02:51:20AM +0300, Laurent Pinchart wrote:\n> Hi Paul,\n> \n> Thank you for the patch.\n> \n> On Tue, Jun 11, 2024 at 03:12:44PM +0100, Kieran Bingham wrote:\n> > Quoting Paul Elder (2024-06-11 15:02:07)\n> > > Add an algorithm module to the rkisp1 IPA for crosstalk correction.\n> > > \n> > > Signed-off-by: Paul Elder \n> > > Reviewed-by: Stefan Klug \n> > > \n> > > ---\n> > > Changes in v7:\n> > > - make offsets_ default to zero-matrices as opposed to identity matrices\n> > > - checkstyle\n> > > - populate metadata\n> > > - add ccm to IPAFrameContext\n> > > - don't update the ccm if the color temperature didn't change\n> > > \n> > > No change in v6\n> > > \n> > > Changes in v5:\n> > > - clean up documentation\n> > > - coalesce parseYaml into init\n> > > \n> > > Changes in v4:\n> > > - remove stray semicolons\n> > > - use the new matrix interpolator readYaml\n> > > - use the new matrix operator[] getter\n> > > \n> > > Changes in v3:\n> > > - read ccm offsets from tuning data, and write these offsets to the\n> > > parameters buffer\n> > > - make parseYaml return void, as it should fill in default data if\n> > > unable to read, thus never failing\n> > > \n> > > Changes in v2:\n> > > - rename ctk to ccm\n> > > - reset the matrix interpolator to identity matrix if failed to read\n> > > from tuning file\n> > > ---\n> > > src/ipa/rkisp1/algorithms/ccm.cpp | 140 ++++++++++++++++++++++++++\n> > > src/ipa/rkisp1/algorithms/ccm.h | 50 +++++++++\n> > > src/ipa/rkisp1/algorithms/meson.build | 1 +\n> > > src/ipa/rkisp1/ipa_context.h | 5 +\n> > > 4 files changed, 196 insertions(+)\n> > > create mode 100644 src/ipa/rkisp1/algorithms/ccm.cpp\n> > > create mode 100644 src/ipa/rkisp1/algorithms/ccm.h\n> > > \n> > > diff --git a/src/ipa/rkisp1/algorithms/ccm.cpp b/src/ipa/rkisp1/algorithms/ccm.cpp\n> > > new file mode 100644\n> > > index 000000000000..09fe4b2aa1bc\n> > > --- /dev/null\n> > > +++ b/src/ipa/rkisp1/algorithms/ccm.cpp\n> > > @@ -0,0 +1,140 @@\n> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > +/*\n> > > + * Copyright (C) 2024, Ideas On Board\n> > > + *\n> > > + * RkISP1 Color Correction Matrix control algorithm\n> > > + */\n> > > +\n> > > +#include \"ccm.h\"\n> > > +\n> > > +#include \n> > > +#include \n> > > +#include \n> > > +#include \n> > > +#include \n> > > +\n> > > +#include \n> > > +#include \n> > > +\n> > > +#include \n> > > +\n> > > +#include \n> > > +\n> > > +#include \"libcamera/internal/yaml_parser.h\"\n> > > +\n> > > +#include \"../utils.h\"\n> > > +#include \"libipa/matrix_interpolator.h\"\n> > > +\n> > > +/**\n> > > + * \\file ccm.h\n> > > + */\n> > > +\n> > > +namespace libcamera {\n> > > +\n> > > +namespace ipa::rkisp1::algorithms {\n> > > +\n> > > +/**\n> > > + * \\class Ccm\n> > > + * \\brief A color correction matrix algorithm\n> > > + */\n> > > +\n> > > +LOG_DEFINE_CATEGORY(RkISP1Ccm)\n> > > +\n> > > +/**\n> > > + * \\copydoc libcamera::ipa::Algorithm::init\n> > > + */\n> > > +int Ccm::init([[maybe_unused]] IPAContext &context, const YamlObject &tuningData)\n> > > +{\n> > > + int ret = ccm_.readYaml(tuningData[\"ccms\"], \"ct\", \"ccm\");\n> > > + if (ret < 0) {\n> > > + LOG(RkISP1Ccm, Warning)\n> > > + << \"Failed to parse 'ccm' \"\n> > > + << \"parameter from tuning file; falling back to unit matrix\";\n> > > + ccm_.reset();\n> > > + }\n> > > +\n> > > + ret = offsets_.readYaml(tuningData[\"ccms\"], \"ct\", \"offsets\");\n> > > + if (ret < 0) {\n> > > + LOG(RkISP1Ccm, Warning)\n> > > + << \"Failed to parse 'offsets' \"\n> > > + << \"parameter from tuning file; falling back to zero offsets\";\n> > > + /*\n> > > + * MatrixInterpolator::reset() resets to identity matrices\n> > > + * while here we need zero matrices so we need to construct it\n> > > + * ourselves.\n> > > + */\n> > > + Matrix m({ 0, 0, 0 });\n> > > + std::map> matrices = { { 0, m } };\n> > > + offsets_ = MatrixInterpolator(matrices);\n> > > + }\n> > > +\n> > > + return 0;\n> > > +}\n> > > +\n> > > +void Ccm::setParameters(rkisp1_params_cfg *params,\n> > > + const Matrix &matrix,\n> > > + const Matrix &offsets)\n> > > +{\n> > > + struct rkisp1_cif_isp_ctk_config &config = params->others.ctk_config;\n> > > +\n> > > + /*\n> > > + * 4 bit integer and 7 bit fractional, ranging from -8 (0x400) to\n> > > + * +7.992 (0x3ff)\n> > > + */\n> > > + for (unsigned int i = 0; i < 3; i++)\n> > > + for (unsigned int j = 0; j < 3; j++)\n> > > + config.coeff[i][j] =\n> > > + utils::floatingToFixedPoint<4, 7, uint16_t, double>(matrix[i][j]);\n> \n> Curly braces for outer loop.\n> \n> > > +\n> > > + for (unsigned int i = 0; i < 3; i++)\n> > > + config.ct_offset[i] = offsets[i][0] & 0xfff;\n> > > +\n> > > + LOG(RkISP1Ccm, Debug) << \"Setting matrix \" << matrix;\n> > > + LOG(RkISP1Ccm, Debug) << \"Setting offsets \" << offsets;\n> > > +\n> > > + params->module_en_update |= RKISP1_CIF_ISP_MODULE_CTK;\n> > > + params->module_ens |= RKISP1_CIF_ISP_MODULE_CTK;\n> > > + params->module_cfg_update |= RKISP1_CIF_ISP_MODULE_CTK;\n> > > +}\n> > > +\n> > > +/**\n> > > + * \\copydoc libcamera::ipa::Algorithm::prepare\n> > > + */\n> > > +void Ccm::prepare(IPAContext &context, [[maybe_unused]] const uint32_t frame,\n> > > + IPAFrameContext &frameContext,\n> > > + rkisp1_params_cfg *params)\n> > > +{\n> > > + uint32_t ct = context.activeState.awb.temperatureK;\n> > > + if (ct == ct_)\n> \n> ct_ should be initialized/reset in configure(), to ensure that we'll\n> always set the CCM matrix on the first frame.\n> \n> I expect temperatureK to be fairly noisy, should we add a threshold to\n> the comparison to avoid updating the CCM parameters on every frame in\n> practice ?\n> \n> > > + return;\n> > \n> > Interestingly, I think this is fine (certainly for now) ... but this\n> > means that the output request metadata will only write the CCM if it\n> > changes.\n\nThere was discussion before about \"do we need to always write *all*\nmetadata\" and iirc the conclusion was no so...\n\n> > \n> > I think that's actually 'fine' if we imply that any frame that doesn't\n> > supply metadata is the same as the previous state. Though I think RPi\n> > currently reports it for every frame regardless.\n> > \n> > I'd be curious to see how Stefan's tooling handles this.\n> > \n> > We can always update on top, and my comments are addressed so:\n> > \n> > Reviewed-by: Kieran Bingham \n> > \n> > > +\n> > > + ct_ = ct;\n> > > + Matrix ccm = ccm_.get(ct);\n> > > + Matrix offsets = offsets_.get(ct);\n> > > +\n> > > + frameContext.ccm.ccm = ccm;\n> > > +\n> > > + setParameters(params, ccm, offsets);\n> > > +}\n> > > +\n> > > +/**\n> > > + * \\copydoc libcamera::ipa::Algorithm::process\n> > > + */\n> > > +void process([[maybe_unused]] IPAContext &context,\n> > > + [[maybe_unused]] const uint32_t frame,\n> > > + IPAFrameContext &frameContext,\n> > > + [[maybe_unused]] const rkisp1_stat_buffer *stats,\n> > > + ControlList &metadata)\n> > > +{\n> > > + float m[9];\n> > > + for (unsigned int i = 0; i < 3; i++)\n> > > + for (unsigned int j = 0; j < 3; j++)\n> > > + m[i] = frameContext.ccm.ccm[i][j];\n> \n> Curly braces for outer loop.\n> \n> > > + metadata.set(controls::ColourCorrectionMatrix, m);\n> \n> It's interesting we're reporting the matrix but not the offsets. Not a\n> blocker for this series, but should this be addressed ?\n> \n\nI wasn't sure if there was demand for reporting the offsets...\n\nAnd actually I remember a link [1] that Stefan sent me a long time ago\nthat said something about them costing more computation and rarely\nimproving results...?\n\n[1] https://www.imatest.com/docs/colormatrix/\n\n> > > +}\n> > > +\n> > > +REGISTER_IPA_ALGORITHM(Ccm, \"Ccm\")\n> > > +\n> > > +} /* namespace ipa::rkisp1::algorithms */\n> > > +\n> > > +} /* namespace libcamera */\n> > > diff --git a/src/ipa/rkisp1/algorithms/ccm.h b/src/ipa/rkisp1/algorithms/ccm.h\n> > > new file mode 100644\n> > > index 000000000000..09a6801626b4\n> > > --- /dev/null\n> > > +++ b/src/ipa/rkisp1/algorithms/ccm.h\n> > > @@ -0,0 +1,50 @@\n> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > +/*\n> > > + * Copyright (C) 2024, Ideas On Board\n> > > + *\n> > > + * RkISP1 Color Correction Matrix control algorithm\n> > > + */\n> > > +\n> > > +#pragma once\n> > > +\n> > > +#include \n> > > +\n> > > +#include \"libipa/matrix.h\"\n> > > +#include \"libipa/matrix_interpolator.h\"\n> > > +\n> > > +#include \"algorithm.h\"\n> > > +\n> > > +namespace libcamera {\n> > > +\n> > > +namespace ipa::rkisp1::algorithms {\n> > > +\n> > > +class Ccm : public Algorithm\n> > > +{\n> > > +public:\n> > > + Ccm() {}\n> > > + ~Ccm() = default;\n> > > +\n> > > + int init(IPAContext &context, const YamlObject &tuningData) override;\n> > > + void prepare(IPAContext &context, const uint32_t frame,\n> > > + IPAFrameContext &frameContext,\n> > > + rkisp1_params_cfg *params) override;\n> > > + void process([[maybe_unused]] IPAContext &context,\n> \n> [[maybe_unused]] is not needed in the function declaration.\n> \n> > > + [[maybe_unused]] const uint32_t frame,\n> > > + IPAFrameContext &frameContext,\n> > > + [[maybe_unused]] const rkisp1_stat_buffer *stats,\n> > > + ControlList &metadata) override;\n> > > +\n> > > +private:\n> > > + void parseYaml(const YamlObject &tuningData);\n> > > + void setParameters(rkisp1_params_cfg *params,\n> > > + const Matrix &matrix,\n> > > + const Matrix &offsets);\n> > > +\n> > > + unsigned int ct_;\n> > > + MatrixInterpolator ccm_;\n> \n> Given that the hardware representation is a 4.7 fixed point, would it be\n> enough to use floats instead of doubles (here and in the frame context)\n> ?\n> \n> > > + MatrixInterpolator offsets_;\n> \n> Why is the matrix stored in floating point values and the offsets as\n> integers ?\n\nBecause the offsets are integers in hardware as well while the ccm is\nfixed-point.\n\n\nPaul\n\n> \n> > > +};\n> > > +\n> > > +} /* namespace ipa::rkisp1::algorithms */\n> > > +\n> > > +} /* namespace libcamera */\n> > > diff --git a/src/ipa/rkisp1/algorithms/meson.build b/src/ipa/rkisp1/algorithms/meson.build\n> > > index 6ee71a9b5da3..1734a6675f78 100644\n> > > --- a/src/ipa/rkisp1/algorithms/meson.build\n> > > +++ b/src/ipa/rkisp1/algorithms/meson.build\n> > > @@ -4,6 +4,7 @@ rkisp1_ipa_algorithms = files([\n> > > 'agc.cpp',\n> > > 'awb.cpp',\n> > > 'blc.cpp',\n> > > + 'ccm.cpp',\n> > > 'cproc.cpp',\n> > > 'dpcc.cpp',\n> > > 'dpf.cpp',\n> > > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h\n> > > index 2a994d81ae41..cfb1f9770870 100644\n> > > --- a/src/ipa/rkisp1/ipa_context.h\n> > > +++ b/src/ipa/rkisp1/ipa_context.h\n> > > @@ -16,6 +16,7 @@\n> > > #include \n> > > \n> > > #include \n> > > +#include \n> > > \n> > > namespace libcamera {\n> > > \n> > > @@ -155,6 +156,10 @@ struct IPAFrameContext : public FrameContext {\n> > > uint32_t exposure;\n> > > double gain;\n> > > } sensor;\n> > > +\n> > > + struct {\n> > > + Matrix ccm;\n> > > + } ccm;\n> > > };\n> > > \n> > > struct IPAContext {\n>","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 2F605BD87C\n\tfor ;\n\tWed, 12 Jun 2024 08:04:48 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4961665461;\n\tWed, 12 Jun 2024 10:04:47 +0200 (CEST)","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 57CD061A2A\n\tfor ;\n\tWed, 12 Jun 2024 10:04:46 +0200 (CEST)","from pyrite.rasen.tech (h175-177-049-156.catv02.itscom.jp\n\t[175.177.49.156])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 7C48763B;\n\tWed, 12 Jun 2024 10:04:31 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"GbRK66BI\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1718179472;\n\tbh=5/JZq403FNY2iDIBzGKnye/Rb1uE1IghILK76on27gQ=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=GbRK66BIsWXFWzS6SfBQOZU0jk1gbB8QzzjTupa5NGyNVjFXCJQLy9ryCdsoHQkgC\n\ttzsYtPKkJpQeWZ0665Rg4l1Oeh2DBl/gZz3+S780rEITTCFoFB+JtJhLCnhhrhtorO\n\tYN4I0Bx/y7BaWfuPj4r88CKsldFee/2UrJSP+cvo=","Date":"Wed, 12 Jun 2024 17:04:38 +0900","From":"Paul Elder ","To":"Laurent Pinchart ","Cc":"Kieran Bingham ,\n\tlibcamera-devel@lists.libcamera.org,\n\tStefan Klug ","Subject":"Re: [PATCH v7 3/3] ipa: rkisp1: algorithms: Add crosstalk algorithm","Message-ID":"","References":"<20240611140207.520083-1-paul.elder@ideasonboard.com>\n\t<20240611140207.520083-4-paul.elder@ideasonboard.com>\n\t<171811516483.1148289.15469360068204996564@ping.linuxembedded.co.uk>\n\t<20240611235120.GF15006@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<20240611235120.GF15006@pendragon.ideasonboard.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\" "}},{"id":29871,"web_url":"https://patchwork.libcamera.org/comment/29871/","msgid":"<20240612093834.GI28989@pendragon.ideasonboard.com>","date":"2024-06-12T09:38:34","subject":"Re: [PATCH v7 3/3] ipa: rkisp1: algorithms: Add crosstalk algorithm","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Wed, Jun 12, 2024 at 05:04:38PM +0900, Paul Elder wrote:\n> On Wed, Jun 12, 2024 at 02:51:20AM +0300, Laurent Pinchart wrote:\n> > On Tue, Jun 11, 2024 at 03:12:44PM +0100, Kieran Bingham wrote:\n> > > Quoting Paul Elder (2024-06-11 15:02:07)\n> > > > Add an algorithm module to the rkisp1 IPA for crosstalk correction.\n> > > > \n> > > > Signed-off-by: Paul Elder \n> > > > Reviewed-by: Stefan Klug \n> > > > \n> > > > ---\n> > > > Changes in v7:\n> > > > - make offsets_ default to zero-matrices as opposed to identity matrices\n> > > > - checkstyle\n> > > > - populate metadata\n> > > > - add ccm to IPAFrameContext\n> > > > - don't update the ccm if the color temperature didn't change\n> > > > \n> > > > No change in v6\n> > > > \n> > > > Changes in v5:\n> > > > - clean up documentation\n> > > > - coalesce parseYaml into init\n> > > > \n> > > > Changes in v4:\n> > > > - remove stray semicolons\n> > > > - use the new matrix interpolator readYaml\n> > > > - use the new matrix operator[] getter\n> > > > \n> > > > Changes in v3:\n> > > > - read ccm offsets from tuning data, and write these offsets to the\n> > > > parameters buffer\n> > > > - make parseYaml return void, as it should fill in default data if\n> > > > unable to read, thus never failing\n> > > > \n> > > > Changes in v2:\n> > > > - rename ctk to ccm\n> > > > - reset the matrix interpolator to identity matrix if failed to read\n> > > > from tuning file\n> > > > ---\n> > > > src/ipa/rkisp1/algorithms/ccm.cpp | 140 ++++++++++++++++++++++++++\n> > > > src/ipa/rkisp1/algorithms/ccm.h | 50 +++++++++\n> > > > src/ipa/rkisp1/algorithms/meson.build | 1 +\n> > > > src/ipa/rkisp1/ipa_context.h | 5 +\n> > > > 4 files changed, 196 insertions(+)\n> > > > create mode 100644 src/ipa/rkisp1/algorithms/ccm.cpp\n> > > > create mode 100644 src/ipa/rkisp1/algorithms/ccm.h\n> > > > \n> > > > diff --git a/src/ipa/rkisp1/algorithms/ccm.cpp b/src/ipa/rkisp1/algorithms/ccm.cpp\n> > > > new file mode 100644\n> > > > index 000000000000..09fe4b2aa1bc\n> > > > --- /dev/null\n> > > > +++ b/src/ipa/rkisp1/algorithms/ccm.cpp\n> > > > @@ -0,0 +1,140 @@\n> > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > > +/*\n> > > > + * Copyright (C) 2024, Ideas On Board\n> > > > + *\n> > > > + * RkISP1 Color Correction Matrix control algorithm\n> > > > + */\n> > > > +\n> > > > +#include \"ccm.h\"\n> > > > +\n> > > > +#include \n> > > > +#include \n> > > > +#include \n> > > > +#include \n> > > > +#include \n> > > > +\n> > > > +#include \n> > > > +#include \n> > > > +\n> > > > +#include \n> > > > +\n> > > > +#include \n> > > > +\n> > > > +#include \"libcamera/internal/yaml_parser.h\"\n> > > > +\n> > > > +#include \"../utils.h\"\n> > > > +#include \"libipa/matrix_interpolator.h\"\n> > > > +\n> > > > +/**\n> > > > + * \\file ccm.h\n> > > > + */\n> > > > +\n> > > > +namespace libcamera {\n> > > > +\n> > > > +namespace ipa::rkisp1::algorithms {\n> > > > +\n> > > > +/**\n> > > > + * \\class Ccm\n> > > > + * \\brief A color correction matrix algorithm\n> > > > + */\n> > > > +\n> > > > +LOG_DEFINE_CATEGORY(RkISP1Ccm)\n> > > > +\n> > > > +/**\n> > > > + * \\copydoc libcamera::ipa::Algorithm::init\n> > > > + */\n> > > > +int Ccm::init([[maybe_unused]] IPAContext &context, const YamlObject &tuningData)\n> > > > +{\n> > > > + int ret = ccm_.readYaml(tuningData[\"ccms\"], \"ct\", \"ccm\");\n> > > > + if (ret < 0) {\n> > > > + LOG(RkISP1Ccm, Warning)\n> > > > + << \"Failed to parse 'ccm' \"\n> > > > + << \"parameter from tuning file; falling back to unit matrix\";\n> > > > + ccm_.reset();\n> > > > + }\n> > > > +\n> > > > + ret = offsets_.readYaml(tuningData[\"ccms\"], \"ct\", \"offsets\");\n> > > > + if (ret < 0) {\n> > > > + LOG(RkISP1Ccm, Warning)\n> > > > + << \"Failed to parse 'offsets' \"\n> > > > + << \"parameter from tuning file; falling back to zero offsets\";\n> > > > + /*\n> > > > + * MatrixInterpolator::reset() resets to identity matrices\n> > > > + * while here we need zero matrices so we need to construct it\n> > > > + * ourselves.\n> > > > + */\n> > > > + Matrix m({ 0, 0, 0 });\n> > > > + std::map> matrices = { { 0, m } };\n> > > > + offsets_ = MatrixInterpolator(matrices);\n> > > > + }\n> > > > +\n> > > > + return 0;\n> > > > +}\n> > > > +\n> > > > +void Ccm::setParameters(rkisp1_params_cfg *params,\n> > > > + const Matrix &matrix,\n> > > > + const Matrix &offsets)\n> > > > +{\n> > > > + struct rkisp1_cif_isp_ctk_config &config = params->others.ctk_config;\n> > > > +\n> > > > + /*\n> > > > + * 4 bit integer and 7 bit fractional, ranging from -8 (0x400) to\n> > > > + * +7.992 (0x3ff)\n> > > > + */\n> > > > + for (unsigned int i = 0; i < 3; i++)\n> > > > + for (unsigned int j = 0; j < 3; j++)\n> > > > + config.coeff[i][j] =\n> > > > + utils::floatingToFixedPoint<4, 7, uint16_t, double>(matrix[i][j]);\n> > \n> > Curly braces for outer loop.\n> > \n> > > > +\n> > > > + for (unsigned int i = 0; i < 3; i++)\n> > > > + config.ct_offset[i] = offsets[i][0] & 0xfff;\n> > > > +\n> > > > + LOG(RkISP1Ccm, Debug) << \"Setting matrix \" << matrix;\n> > > > + LOG(RkISP1Ccm, Debug) << \"Setting offsets \" << offsets;\n> > > > +\n> > > > + params->module_en_update |= RKISP1_CIF_ISP_MODULE_CTK;\n> > > > + params->module_ens |= RKISP1_CIF_ISP_MODULE_CTK;\n> > > > + params->module_cfg_update |= RKISP1_CIF_ISP_MODULE_CTK;\n> > > > +}\n> > > > +\n> > > > +/**\n> > > > + * \\copydoc libcamera::ipa::Algorithm::prepare\n> > > > + */\n> > > > +void Ccm::prepare(IPAContext &context, [[maybe_unused]] const uint32_t frame,\n> > > > + IPAFrameContext &frameContext,\n> > > > + rkisp1_params_cfg *params)\n> > > > +{\n> > > > + uint32_t ct = context.activeState.awb.temperatureK;\n> > > > + if (ct == ct_)\n> > \n> > ct_ should be initialized/reset in configure(), to ensure that we'll\n> > always set the CCM matrix on the first frame.\n> > \n> > I expect temperatureK to be fairly noisy, should we add a threshold to\n> > the comparison to avoid updating the CCM parameters on every frame in\n> > practice ?\n> > \n> > > > + return;\n> > > \n> > > Interestingly, I think this is fine (certainly for now) ... but this\n> > > means that the output request metadata will only write the CCM if it\n> > > changes.\n> \n> There was discussion before about \"do we need to always write *all*\n> metadata\" and iirc the conclusion was no so...\n\nI'm sure we'll revisit the topic at some point.\n\n> > > I think that's actually 'fine' if we imply that any frame that doesn't\n> > > supply metadata is the same as the previous state. Though I think RPi\n> > > currently reports it for every frame regardless.\n> > > \n> > > I'd be curious to see how Stefan's tooling handles this.\n> > > \n> > > We can always update on top, and my comments are addressed so:\n> > > \n> > > Reviewed-by: Kieran Bingham \n> > > \n> > > > +\n> > > > + ct_ = ct;\n> > > > + Matrix ccm = ccm_.get(ct);\n> > > > + Matrix offsets = offsets_.get(ct);\n> > > > +\n> > > > + frameContext.ccm.ccm = ccm;\n> > > > +\n> > > > + setParameters(params, ccm, offsets);\n> > > > +}\n> > > > +\n> > > > +/**\n> > > > + * \\copydoc libcamera::ipa::Algorithm::process\n> > > > + */\n> > > > +void process([[maybe_unused]] IPAContext &context,\n> > > > + [[maybe_unused]] const uint32_t frame,\n> > > > + IPAFrameContext &frameContext,\n> > > > + [[maybe_unused]] const rkisp1_stat_buffer *stats,\n> > > > + ControlList &metadata)\n> > > > +{\n> > > > + float m[9];\n> > > > + for (unsigned int i = 0; i < 3; i++)\n> > > > + for (unsigned int j = 0; j < 3; j++)\n> > > > + m[i] = frameContext.ccm.ccm[i][j];\n> > \n> > Curly braces for outer loop.\n> > \n> > > > + metadata.set(controls::ColourCorrectionMatrix, m);\n> > \n> > It's interesting we're reporting the matrix but not the offsets. Not a\n> > blocker for this series, but should this be addressed ?\n> \n> I wasn't sure if there was demand for reporting the offsets...\n> \n> And actually I remember a link [1] that Stefan sent me a long time ago\n> that said something about them costing more computation and rarely\n> improving results...?\n\nThat's what I was wondering too. I wonder if we should drop the offset.\n\n> [1] https://www.imatest.com/docs/colormatrix/\n> \n> > > > +}\n> > > > +\n> > > > +REGISTER_IPA_ALGORITHM(Ccm, \"Ccm\")\n> > > > +\n> > > > +} /* namespace ipa::rkisp1::algorithms */\n> > > > +\n> > > > +} /* namespace libcamera */\n> > > > diff --git a/src/ipa/rkisp1/algorithms/ccm.h b/src/ipa/rkisp1/algorithms/ccm.h\n> > > > new file mode 100644\n> > > > index 000000000000..09a6801626b4\n> > > > --- /dev/null\n> > > > +++ b/src/ipa/rkisp1/algorithms/ccm.h\n> > > > @@ -0,0 +1,50 @@\n> > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > > +/*\n> > > > + * Copyright (C) 2024, Ideas On Board\n> > > > + *\n> > > > + * RkISP1 Color Correction Matrix control algorithm\n> > > > + */\n> > > > +\n> > > > +#pragma once\n> > > > +\n> > > > +#include \n> > > > +\n> > > > +#include \"libipa/matrix.h\"\n> > > > +#include \"libipa/matrix_interpolator.h\"\n> > > > +\n> > > > +#include \"algorithm.h\"\n> > > > +\n> > > > +namespace libcamera {\n> > > > +\n> > > > +namespace ipa::rkisp1::algorithms {\n> > > > +\n> > > > +class Ccm : public Algorithm\n> > > > +{\n> > > > +public:\n> > > > + Ccm() {}\n> > > > + ~Ccm() = default;\n> > > > +\n> > > > + int init(IPAContext &context, const YamlObject &tuningData) override;\n> > > > + void prepare(IPAContext &context, const uint32_t frame,\n> > > > + IPAFrameContext &frameContext,\n> > > > + rkisp1_params_cfg *params) override;\n> > > > + void process([[maybe_unused]] IPAContext &context,\n> > \n> > [[maybe_unused]] is not needed in the function declaration.\n> > \n> > > > + [[maybe_unused]] const uint32_t frame,\n> > > > + IPAFrameContext &frameContext,\n> > > > + [[maybe_unused]] const rkisp1_stat_buffer *stats,\n> > > > + ControlList &metadata) override;\n> > > > +\n> > > > +private:\n> > > > + void parseYaml(const YamlObject &tuningData);\n> > > > + void setParameters(rkisp1_params_cfg *params,\n> > > > + const Matrix &matrix,\n> > > > + const Matrix &offsets);\n> > > > +\n> > > > + unsigned int ct_;\n> > > > + MatrixInterpolator ccm_;\n> > \n> > Given that the hardware representation is a 4.7 fixed point, would it be\n> > enough to use floats instead of doubles (here and in the frame context)\n> > ?\n> > \n> > > > + MatrixInterpolator offsets_;\n> > \n> > Why is the matrix stored in floating point values and the offsets as\n> > integers ?\n> \n> Because the offsets are integers in hardware as well while the ccm is\n> fixed-point.\n> \n> > > > +};\n> > > > +\n> > > > +} /* namespace ipa::rkisp1::algorithms */\n> > > > +\n> > > > +} /* namespace libcamera */\n> > > > diff --git a/src/ipa/rkisp1/algorithms/meson.build b/src/ipa/rkisp1/algorithms/meson.build\n> > > > index 6ee71a9b5da3..1734a6675f78 100644\n> > > > --- a/src/ipa/rkisp1/algorithms/meson.build\n> > > > +++ b/src/ipa/rkisp1/algorithms/meson.build\n> > > > @@ -4,6 +4,7 @@ rkisp1_ipa_algorithms = files([\n> > > > 'agc.cpp',\n> > > > 'awb.cpp',\n> > > > 'blc.cpp',\n> > > > + 'ccm.cpp',\n> > > > 'cproc.cpp',\n> > > > 'dpcc.cpp',\n> > > > 'dpf.cpp',\n> > > > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h\n> > > > index 2a994d81ae41..cfb1f9770870 100644\n> > > > --- a/src/ipa/rkisp1/ipa_context.h\n> > > > +++ b/src/ipa/rkisp1/ipa_context.h\n> > > > @@ -16,6 +16,7 @@\n> > > > #include \n> > > > \n> > > > #include \n> > > > +#include \n> > > > \n> > > > namespace libcamera {\n> > > > \n> > > > @@ -155,6 +156,10 @@ struct IPAFrameContext : public FrameContext {\n> > > > uint32_t exposure;\n> > > > double gain;\n> > > > } sensor;\n> > > > +\n> > > > + struct {\n> > > > + Matrix ccm;\n> > > > + } ccm;\n> > > > };\n> > > > \n> > > > struct IPAContext {","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 4F058BD87C\n\tfor ;\n\tWed, 12 Jun 2024 09:38:57 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7D0576545A;\n\tWed, 12 Jun 2024 11:38:56 +0200 (CEST)","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 70B4B61A2A\n\tfor ;\n\tWed, 12 Jun 2024 11:38:55 +0200 (CEST)","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 CE112230;\n\tWed, 12 Jun 2024 11:38:41 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"tJqZihD4\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1718185122;\n\tbh=azT0qb9O1VQ57q5gkSh9AWRFtUTB3P5qmOgnl4+Bnt0=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=tJqZihD43ZIK67ChjJQdHXgF3TiPTgI+8vhHn28gRcLWgEpPRQ1cvdA6dWLjRidMn\n\tHYD0rfwsxCmeh4AFOo4apw57hd0Ns9dkrVHcrt384mgcv94mrJySOUXpcdbXkoJrz1\n\tjOV9UN3vBFQfPPIoXS7jnqu4POxUtsLAn7MdNj0Q=","Date":"Wed, 12 Jun 2024 12:38:34 +0300","From":"Laurent Pinchart ","To":"Paul Elder ","Cc":"Kieran Bingham ,\n\tlibcamera-devel@lists.libcamera.org,\n\tStefan Klug ","Subject":"Re: [PATCH v7 3/3] ipa: rkisp1: algorithms: Add crosstalk algorithm","Message-ID":"<20240612093834.GI28989@pendragon.ideasonboard.com>","References":"<20240611140207.520083-1-paul.elder@ideasonboard.com>\n\t<20240611140207.520083-4-paul.elder@ideasonboard.com>\n\t<171811516483.1148289.15469360068204996564@ping.linuxembedded.co.uk>\n\t<20240611235120.GF15006@pendragon.ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","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\" "}},{"id":29893,"web_url":"https://patchwork.libcamera.org/comment/29893/","msgid":"","date":"2024-06-13T08:13:28","subject":"Re: [PATCH v7 3/3] ipa: rkisp1: algorithms: Add crosstalk algorithm","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"On Wed, Jun 12, 2024 at 12:38:34PM +0300, Laurent Pinchart wrote:\n> On Wed, Jun 12, 2024 at 05:04:38PM +0900, Paul Elder wrote:\n> > On Wed, Jun 12, 2024 at 02:51:20AM +0300, Laurent Pinchart wrote:\n> > > On Tue, Jun 11, 2024 at 03:12:44PM +0100, Kieran Bingham wrote:\n> > > > Quoting Paul Elder (2024-06-11 15:02:07)\n> > > > > Add an algorithm module to the rkisp1 IPA for crosstalk correction.\n> > > > > \n> > > > > Signed-off-by: Paul Elder \n> > > > > Reviewed-by: Stefan Klug \n> > > > > \n> > > > > ---\n> > > > > Changes in v7:\n> > > > > - make offsets_ default to zero-matrices as opposed to identity matrices\n> > > > > - checkstyle\n> > > > > - populate metadata\n> > > > > - add ccm to IPAFrameContext\n> > > > > - don't update the ccm if the color temperature didn't change\n> > > > > \n> > > > > No change in v6\n> > > > > \n> > > > > Changes in v5:\n> > > > > - clean up documentation\n> > > > > - coalesce parseYaml into init\n> > > > > \n> > > > > Changes in v4:\n> > > > > - remove stray semicolons\n> > > > > - use the new matrix interpolator readYaml\n> > > > > - use the new matrix operator[] getter\n> > > > > \n> > > > > Changes in v3:\n> > > > > - read ccm offsets from tuning data, and write these offsets to the\n> > > > > parameters buffer\n> > > > > - make parseYaml return void, as it should fill in default data if\n> > > > > unable to read, thus never failing\n> > > > > \n> > > > > Changes in v2:\n> > > > > - rename ctk to ccm\n> > > > > - reset the matrix interpolator to identity matrix if failed to read\n> > > > > from tuning file\n> > > > > ---\n> > > > > src/ipa/rkisp1/algorithms/ccm.cpp | 140 ++++++++++++++++++++++++++\n> > > > > src/ipa/rkisp1/algorithms/ccm.h | 50 +++++++++\n> > > > > src/ipa/rkisp1/algorithms/meson.build | 1 +\n> > > > > src/ipa/rkisp1/ipa_context.h | 5 +\n> > > > > 4 files changed, 196 insertions(+)\n> > > > > create mode 100644 src/ipa/rkisp1/algorithms/ccm.cpp\n> > > > > create mode 100644 src/ipa/rkisp1/algorithms/ccm.h\n> > > > > \n> > > > > diff --git a/src/ipa/rkisp1/algorithms/ccm.cpp b/src/ipa/rkisp1/algorithms/ccm.cpp\n> > > > > new file mode 100644\n> > > > > index 000000000000..09fe4b2aa1bc\n> > > > > --- /dev/null\n> > > > > +++ b/src/ipa/rkisp1/algorithms/ccm.cpp\n> > > > > @@ -0,0 +1,140 @@\n> > > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > > > +/*\n> > > > > + * Copyright (C) 2024, Ideas On Board\n> > > > > + *\n> > > > > + * RkISP1 Color Correction Matrix control algorithm\n> > > > > + */\n> > > > > +\n> > > > > +#include \"ccm.h\"\n> > > > > +\n> > > > > +#include \n> > > > > +#include \n> > > > > +#include \n> > > > > +#include \n> > > > > +#include \n> > > > > +\n> > > > > +#include \n> > > > > +#include \n> > > > > +\n> > > > > +#include \n> > > > > +\n> > > > > +#include \n> > > > > +\n> > > > > +#include \"libcamera/internal/yaml_parser.h\"\n> > > > > +\n> > > > > +#include \"../utils.h\"\n> > > > > +#include \"libipa/matrix_interpolator.h\"\n> > > > > +\n> > > > > +/**\n> > > > > + * \\file ccm.h\n> > > > > + */\n> > > > > +\n> > > > > +namespace libcamera {\n> > > > > +\n> > > > > +namespace ipa::rkisp1::algorithms {\n> > > > > +\n> > > > > +/**\n> > > > > + * \\class Ccm\n> > > > > + * \\brief A color correction matrix algorithm\n> > > > > + */\n> > > > > +\n> > > > > +LOG_DEFINE_CATEGORY(RkISP1Ccm)\n> > > > > +\n> > > > > +/**\n> > > > > + * \\copydoc libcamera::ipa::Algorithm::init\n> > > > > + */\n> > > > > +int Ccm::init([[maybe_unused]] IPAContext &context, const YamlObject &tuningData)\n> > > > > +{\n> > > > > + int ret = ccm_.readYaml(tuningData[\"ccms\"], \"ct\", \"ccm\");\n> > > > > + if (ret < 0) {\n> > > > > + LOG(RkISP1Ccm, Warning)\n> > > > > + << \"Failed to parse 'ccm' \"\n> > > > > + << \"parameter from tuning file; falling back to unit matrix\";\n> > > > > + ccm_.reset();\n> > > > > + }\n> > > > > +\n> > > > > + ret = offsets_.readYaml(tuningData[\"ccms\"], \"ct\", \"offsets\");\n> > > > > + if (ret < 0) {\n> > > > > + LOG(RkISP1Ccm, Warning)\n> > > > > + << \"Failed to parse 'offsets' \"\n> > > > > + << \"parameter from tuning file; falling back to zero offsets\";\n> > > > > + /*\n> > > > > + * MatrixInterpolator::reset() resets to identity matrices\n> > > > > + * while here we need zero matrices so we need to construct it\n> > > > > + * ourselves.\n> > > > > + */\n> > > > > + Matrix m({ 0, 0, 0 });\n> > > > > + std::map> matrices = { { 0, m } };\n> > > > > + offsets_ = MatrixInterpolator(matrices);\n> > > > > + }\n> > > > > +\n> > > > > + return 0;\n> > > > > +}\n> > > > > +\n> > > > > +void Ccm::setParameters(rkisp1_params_cfg *params,\n> > > > > + const Matrix &matrix,\n> > > > > + const Matrix &offsets)\n> > > > > +{\n> > > > > + struct rkisp1_cif_isp_ctk_config &config = params->others.ctk_config;\n> > > > > +\n> > > > > + /*\n> > > > > + * 4 bit integer and 7 bit fractional, ranging from -8 (0x400) to\n> > > > > + * +7.992 (0x3ff)\n> > > > > + */\n> > > > > + for (unsigned int i = 0; i < 3; i++)\n> > > > > + for (unsigned int j = 0; j < 3; j++)\n> > > > > + config.coeff[i][j] =\n> > > > > + utils::floatingToFixedPoint<4, 7, uint16_t, double>(matrix[i][j]);\n> > > \n> > > Curly braces for outer loop.\n> > > \n> > > > > +\n> > > > > + for (unsigned int i = 0; i < 3; i++)\n> > > > > + config.ct_offset[i] = offsets[i][0] & 0xfff;\n> > > > > +\n> > > > > + LOG(RkISP1Ccm, Debug) << \"Setting matrix \" << matrix;\n> > > > > + LOG(RkISP1Ccm, Debug) << \"Setting offsets \" << offsets;\n> > > > > +\n> > > > > + params->module_en_update |= RKISP1_CIF_ISP_MODULE_CTK;\n> > > > > + params->module_ens |= RKISP1_CIF_ISP_MODULE_CTK;\n> > > > > + params->module_cfg_update |= RKISP1_CIF_ISP_MODULE_CTK;\n> > > > > +}\n> > > > > +\n> > > > > +/**\n> > > > > + * \\copydoc libcamera::ipa::Algorithm::prepare\n> > > > > + */\n> > > > > +void Ccm::prepare(IPAContext &context, [[maybe_unused]] const uint32_t frame,\n> > > > > + IPAFrameContext &frameContext,\n> > > > > + rkisp1_params_cfg *params)\n> > > > > +{\n> > > > > + uint32_t ct = context.activeState.awb.temperatureK;\n> > > > > + if (ct == ct_)\n> > > \n> > > ct_ should be initialized/reset in configure(), to ensure that we'll\n> > > always set the CCM matrix on the first frame.\n> > > \n> > > I expect temperatureK to be fairly noisy, should we add a threshold to\n> > > the comparison to avoid updating the CCM parameters on every frame in\n> > > practice ?\n> > > \n> > > > > + return;\n> > > > \n> > > > Interestingly, I think this is fine (certainly for now) ... but this\n> > > > means that the output request metadata will only write the CCM if it\n> > > > changes.\n> > \n> > There was discussion before about \"do we need to always write *all*\n> > metadata\" and iirc the conclusion was no so...\n> \n> I'm sure we'll revisit the topic at some point.\n> \n> > > > I think that's actually 'fine' if we imply that any frame that doesn't\n> > > > supply metadata is the same as the previous state. Though I think RPi\n> > > > currently reports it for every frame regardless.\n> > > > \n> > > > I'd be curious to see how Stefan's tooling handles this.\n> > > > \n> > > > We can always update on top, and my comments are addressed so:\n> > > > \n> > > > Reviewed-by: Kieran Bingham \n> > > > \n> > > > > +\n> > > > > + ct_ = ct;\n> > > > > + Matrix ccm = ccm_.get(ct);\n> > > > > + Matrix offsets = offsets_.get(ct);\n> > > > > +\n> > > > > + frameContext.ccm.ccm = ccm;\n> > > > > +\n> > > > > + setParameters(params, ccm, offsets);\n> > > > > +}\n> > > > > +\n> > > > > +/**\n> > > > > + * \\copydoc libcamera::ipa::Algorithm::process\n> > > > > + */\n> > > > > +void process([[maybe_unused]] IPAContext &context,\n> > > > > + [[maybe_unused]] const uint32_t frame,\n> > > > > + IPAFrameContext &frameContext,\n> > > > > + [[maybe_unused]] const rkisp1_stat_buffer *stats,\n> > > > > + ControlList &metadata)\n> > > > > +{\n> > > > > + float m[9];\n> > > > > + for (unsigned int i = 0; i < 3; i++)\n> > > > > + for (unsigned int j = 0; j < 3; j++)\n> > > > > + m[i] = frameContext.ccm.ccm[i][j];\n> > > \n> > > Curly braces for outer loop.\n> > > \n> > > > > + metadata.set(controls::ColourCorrectionMatrix, m);\n> > > \n> > > It's interesting we're reporting the matrix but not the offsets. Not a\n> > > blocker for this series, but should this be addressed ?\n> > \n> > I wasn't sure if there was demand for reporting the offsets...\n> > \n> > And actually I remember a link [1] that Stefan sent me a long time ago\n> > that said something about them costing more computation and rarely\n> > improving results...?\n> \n> That's what I was wondering too. I wonder if we should drop the offset.\n> \n\nI think they should be there to have the /option/ of being set.\n\n\nPaul\n\n> > [1] https://www.imatest.com/docs/colormatrix/\n> > \n> > > > > +}\n> > > > > +\n> > > > > +REGISTER_IPA_ALGORITHM(Ccm, \"Ccm\")\n> > > > > +\n> > > > > +} /* namespace ipa::rkisp1::algorithms */\n> > > > > +\n> > > > > +} /* namespace libcamera */\n> > > > > diff --git a/src/ipa/rkisp1/algorithms/ccm.h b/src/ipa/rkisp1/algorithms/ccm.h\n> > > > > new file mode 100644\n> > > > > index 000000000000..09a6801626b4\n> > > > > --- /dev/null\n> > > > > +++ b/src/ipa/rkisp1/algorithms/ccm.h\n> > > > > @@ -0,0 +1,50 @@\n> > > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > > > +/*\n> > > > > + * Copyright (C) 2024, Ideas On Board\n> > > > > + *\n> > > > > + * RkISP1 Color Correction Matrix control algorithm\n> > > > > + */\n> > > > > +\n> > > > > +#pragma once\n> > > > > +\n> > > > > +#include \n> > > > > +\n> > > > > +#include \"libipa/matrix.h\"\n> > > > > +#include \"libipa/matrix_interpolator.h\"\n> > > > > +\n> > > > > +#include \"algorithm.h\"\n> > > > > +\n> > > > > +namespace libcamera {\n> > > > > +\n> > > > > +namespace ipa::rkisp1::algorithms {\n> > > > > +\n> > > > > +class Ccm : public Algorithm\n> > > > > +{\n> > > > > +public:\n> > > > > + Ccm() {}\n> > > > > + ~Ccm() = default;\n> > > > > +\n> > > > > + int init(IPAContext &context, const YamlObject &tuningData) override;\n> > > > > + void prepare(IPAContext &context, const uint32_t frame,\n> > > > > + IPAFrameContext &frameContext,\n> > > > > + rkisp1_params_cfg *params) override;\n> > > > > + void process([[maybe_unused]] IPAContext &context,\n> > > \n> > > [[maybe_unused]] is not needed in the function declaration.\n> > > \n> > > > > + [[maybe_unused]] const uint32_t frame,\n> > > > > + IPAFrameContext &frameContext,\n> > > > > + [[maybe_unused]] const rkisp1_stat_buffer *stats,\n> > > > > + ControlList &metadata) override;\n> > > > > +\n> > > > > +private:\n> > > > > + void parseYaml(const YamlObject &tuningData);\n> > > > > + void setParameters(rkisp1_params_cfg *params,\n> > > > > + const Matrix &matrix,\n> > > > > + const Matrix &offsets);\n> > > > > +\n> > > > > + unsigned int ct_;\n> > > > > + MatrixInterpolator ccm_;\n> > > \n> > > Given that the hardware representation is a 4.7 fixed point, would it be\n> > > enough to use floats instead of doubles (here and in the frame context)\n> > > ?\n> > > \n> > > > > + MatrixInterpolator offsets_;\n> > > \n> > > Why is the matrix stored in floating point values and the offsets as\n> > > integers ?\n> > \n> > Because the offsets are integers in hardware as well while the ccm is\n> > fixed-point.\n> > \n> > > > > +};\n> > > > > +\n> > > > > +} /* namespace ipa::rkisp1::algorithms */\n> > > > > +\n> > > > > +} /* namespace libcamera */\n> > > > > diff --git a/src/ipa/rkisp1/algorithms/meson.build b/src/ipa/rkisp1/algorithms/meson.build\n> > > > > index 6ee71a9b5da3..1734a6675f78 100644\n> > > > > --- a/src/ipa/rkisp1/algorithms/meson.build\n> > > > > +++ b/src/ipa/rkisp1/algorithms/meson.build\n> > > > > @@ -4,6 +4,7 @@ rkisp1_ipa_algorithms = files([\n> > > > > 'agc.cpp',\n> > > > > 'awb.cpp',\n> > > > > 'blc.cpp',\n> > > > > + 'ccm.cpp',\n> > > > > 'cproc.cpp',\n> > > > > 'dpcc.cpp',\n> > > > > 'dpf.cpp',\n> > > > > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h\n> > > > > index 2a994d81ae41..cfb1f9770870 100644\n> > > > > --- a/src/ipa/rkisp1/ipa_context.h\n> > > > > +++ b/src/ipa/rkisp1/ipa_context.h\n> > > > > @@ -16,6 +16,7 @@\n> > > > > #include \n> > > > > \n> > > > > #include \n> > > > > +#include \n> > > > > \n> > > > > namespace libcamera {\n> > > > > \n> > > > > @@ -155,6 +156,10 @@ struct IPAFrameContext : public FrameContext {\n> > > > > uint32_t exposure;\n> > > > > double gain;\n> > > > > } sensor;\n> > > > > +\n> > > > > + struct {\n> > > > > + Matrix ccm;\n> > > > > + } ccm;\n> > > > > };\n> > > > > \n> > > > > struct IPAContext {\n>","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 7BDC9C3237\n\tfor ;\n\tThu, 13 Jun 2024 08:13:38 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6E1F465493;\n\tThu, 13 Jun 2024 10:13:37 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 670636548C\n\tfor ;\n\tThu, 13 Jun 2024 10:13:35 +0200 (CEST)","from pyrite.rasen.tech (h175-177-049-156.catv02.itscom.jp\n\t[175.177.49.156])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 0FDA2BEB;\n\tThu, 13 Jun 2024 10:13:19 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"HGdBsYDS\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1718266401;\n\tbh=XGOmUvXIaUty0vASSt+VfPcXTsLa5rabHiiP93GXNwA=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=HGdBsYDSLRU5o42drXblwRWB5Pbr+og7/UXccM1NhYXgfnpgJ6DP4vRO79pf8zVTP\n\tSe6S7+7lSoiXkY7VLZzxTWhekbMJsXbjbUUh9x5Di1qmhWS3NlNAV2JV+3v4/SKJwj\n\teBFzOJ5mSSag84G6AnhCfxChTZh7W/qehZOMuMTE=","Date":"Thu, 13 Jun 2024 17:13:28 +0900","From":"Paul Elder ","To":"Laurent Pinchart ","Cc":"Kieran Bingham ,\n\tlibcamera-devel@lists.libcamera.org,\n\tStefan Klug ","Subject":"Re: [PATCH v7 3/3] ipa: rkisp1: algorithms: Add crosstalk algorithm","Message-ID":"","References":"<20240611140207.520083-1-paul.elder@ideasonboard.com>\n\t<20240611140207.520083-4-paul.elder@ideasonboard.com>\n\t<171811516483.1148289.15469360068204996564@ping.linuxembedded.co.uk>\n\t<20240611235120.GF15006@pendragon.ideasonboard.com>\n\t\n\t<20240612093834.GI28989@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<20240612093834.GI28989@pendragon.ideasonboard.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\" "}},{"id":29922,"web_url":"https://patchwork.libcamera.org/comment/29922/","msgid":"<20240613120453.GL6019@pendragon.ideasonboard.com>","date":"2024-06-13T12:04:53","subject":"Re: [PATCH v7 3/3] ipa: rkisp1: algorithms: Add crosstalk algorithm","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Thu, Jun 13, 2024 at 05:13:28PM +0900, Paul Elder wrote:\n> On Wed, Jun 12, 2024 at 12:38:34PM +0300, Laurent Pinchart wrote:\n> > On Wed, Jun 12, 2024 at 05:04:38PM +0900, Paul Elder wrote:\n> > > On Wed, Jun 12, 2024 at 02:51:20AM +0300, Laurent Pinchart wrote:\n> > > > On Tue, Jun 11, 2024 at 03:12:44PM +0100, Kieran Bingham wrote:\n> > > > > Quoting Paul Elder (2024-06-11 15:02:07)\n> > > > > > Add an algorithm module to the rkisp1 IPA for crosstalk correction.\n> > > > > > \n> > > > > > Signed-off-by: Paul Elder \n> > > > > > Reviewed-by: Stefan Klug \n> > > > > > \n> > > > > > ---\n> > > > > > Changes in v7:\n> > > > > > - make offsets_ default to zero-matrices as opposed to identity matrices\n> > > > > > - checkstyle\n> > > > > > - populate metadata\n> > > > > > - add ccm to IPAFrameContext\n> > > > > > - don't update the ccm if the color temperature didn't change\n> > > > > > \n> > > > > > No change in v6\n> > > > > > \n> > > > > > Changes in v5:\n> > > > > > - clean up documentation\n> > > > > > - coalesce parseYaml into init\n> > > > > > \n> > > > > > Changes in v4:\n> > > > > > - remove stray semicolons\n> > > > > > - use the new matrix interpolator readYaml\n> > > > > > - use the new matrix operator[] getter\n> > > > > > \n> > > > > > Changes in v3:\n> > > > > > - read ccm offsets from tuning data, and write these offsets to the\n> > > > > > parameters buffer\n> > > > > > - make parseYaml return void, as it should fill in default data if\n> > > > > > unable to read, thus never failing\n> > > > > > \n> > > > > > Changes in v2:\n> > > > > > - rename ctk to ccm\n> > > > > > - reset the matrix interpolator to identity matrix if failed to read\n> > > > > > from tuning file\n> > > > > > ---\n> > > > > > src/ipa/rkisp1/algorithms/ccm.cpp | 140 ++++++++++++++++++++++++++\n> > > > > > src/ipa/rkisp1/algorithms/ccm.h | 50 +++++++++\n> > > > > > src/ipa/rkisp1/algorithms/meson.build | 1 +\n> > > > > > src/ipa/rkisp1/ipa_context.h | 5 +\n> > > > > > 4 files changed, 196 insertions(+)\n> > > > > > create mode 100644 src/ipa/rkisp1/algorithms/ccm.cpp\n> > > > > > create mode 100644 src/ipa/rkisp1/algorithms/ccm.h\n> > > > > > \n> > > > > > diff --git a/src/ipa/rkisp1/algorithms/ccm.cpp b/src/ipa/rkisp1/algorithms/ccm.cpp\n> > > > > > new file mode 100644\n> > > > > > index 000000000000..09fe4b2aa1bc\n> > > > > > --- /dev/null\n> > > > > > +++ b/src/ipa/rkisp1/algorithms/ccm.cpp\n> > > > > > @@ -0,0 +1,140 @@\n> > > > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > > > > +/*\n> > > > > > + * Copyright (C) 2024, Ideas On Board\n> > > > > > + *\n> > > > > > + * RkISP1 Color Correction Matrix control algorithm\n> > > > > > + */\n> > > > > > +\n> > > > > > +#include \"ccm.h\"\n> > > > > > +\n> > > > > > +#include \n> > > > > > +#include \n> > > > > > +#include \n> > > > > > +#include \n> > > > > > +#include \n> > > > > > +\n> > > > > > +#include \n> > > > > > +#include \n> > > > > > +\n> > > > > > +#include \n> > > > > > +\n> > > > > > +#include \n> > > > > > +\n> > > > > > +#include \"libcamera/internal/yaml_parser.h\"\n> > > > > > +\n> > > > > > +#include \"../utils.h\"\n> > > > > > +#include \"libipa/matrix_interpolator.h\"\n> > > > > > +\n> > > > > > +/**\n> > > > > > + * \\file ccm.h\n> > > > > > + */\n> > > > > > +\n> > > > > > +namespace libcamera {\n> > > > > > +\n> > > > > > +namespace ipa::rkisp1::algorithms {\n> > > > > > +\n> > > > > > +/**\n> > > > > > + * \\class Ccm\n> > > > > > + * \\brief A color correction matrix algorithm\n> > > > > > + */\n> > > > > > +\n> > > > > > +LOG_DEFINE_CATEGORY(RkISP1Ccm)\n> > > > > > +\n> > > > > > +/**\n> > > > > > + * \\copydoc libcamera::ipa::Algorithm::init\n> > > > > > + */\n> > > > > > +int Ccm::init([[maybe_unused]] IPAContext &context, const YamlObject &tuningData)\n> > > > > > +{\n> > > > > > + int ret = ccm_.readYaml(tuningData[\"ccms\"], \"ct\", \"ccm\");\n> > > > > > + if (ret < 0) {\n> > > > > > + LOG(RkISP1Ccm, Warning)\n> > > > > > + << \"Failed to parse 'ccm' \"\n> > > > > > + << \"parameter from tuning file; falling back to unit matrix\";\n> > > > > > + ccm_.reset();\n> > > > > > + }\n> > > > > > +\n> > > > > > + ret = offsets_.readYaml(tuningData[\"ccms\"], \"ct\", \"offsets\");\n> > > > > > + if (ret < 0) {\n> > > > > > + LOG(RkISP1Ccm, Warning)\n> > > > > > + << \"Failed to parse 'offsets' \"\n> > > > > > + << \"parameter from tuning file; falling back to zero offsets\";\n> > > > > > + /*\n> > > > > > + * MatrixInterpolator::reset() resets to identity matrices\n> > > > > > + * while here we need zero matrices so we need to construct it\n> > > > > > + * ourselves.\n> > > > > > + */\n> > > > > > + Matrix m({ 0, 0, 0 });\n> > > > > > + std::map> matrices = { { 0, m } };\n> > > > > > + offsets_ = MatrixInterpolator(matrices);\n> > > > > > + }\n> > > > > > +\n> > > > > > + return 0;\n> > > > > > +}\n> > > > > > +\n> > > > > > +void Ccm::setParameters(rkisp1_params_cfg *params,\n> > > > > > + const Matrix &matrix,\n> > > > > > + const Matrix &offsets)\n> > > > > > +{\n> > > > > > + struct rkisp1_cif_isp_ctk_config &config = params->others.ctk_config;\n> > > > > > +\n> > > > > > + /*\n> > > > > > + * 4 bit integer and 7 bit fractional, ranging from -8 (0x400) to\n> > > > > > + * +7.992 (0x3ff)\n> > > > > > + */\n> > > > > > + for (unsigned int i = 0; i < 3; i++)\n> > > > > > + for (unsigned int j = 0; j < 3; j++)\n> > > > > > + config.coeff[i][j] =\n> > > > > > + utils::floatingToFixedPoint<4, 7, uint16_t, double>(matrix[i][j]);\n> > > > \n> > > > Curly braces for outer loop.\n> > > > \n> > > > > > +\n> > > > > > + for (unsigned int i = 0; i < 3; i++)\n> > > > > > + config.ct_offset[i] = offsets[i][0] & 0xfff;\n> > > > > > +\n> > > > > > + LOG(RkISP1Ccm, Debug) << \"Setting matrix \" << matrix;\n> > > > > > + LOG(RkISP1Ccm, Debug) << \"Setting offsets \" << offsets;\n> > > > > > +\n> > > > > > + params->module_en_update |= RKISP1_CIF_ISP_MODULE_CTK;\n> > > > > > + params->module_ens |= RKISP1_CIF_ISP_MODULE_CTK;\n> > > > > > + params->module_cfg_update |= RKISP1_CIF_ISP_MODULE_CTK;\n> > > > > > +}\n> > > > > > +\n> > > > > > +/**\n> > > > > > + * \\copydoc libcamera::ipa::Algorithm::prepare\n> > > > > > + */\n> > > > > > +void Ccm::prepare(IPAContext &context, [[maybe_unused]] const uint32_t frame,\n> > > > > > + IPAFrameContext &frameContext,\n> > > > > > + rkisp1_params_cfg *params)\n> > > > > > +{\n> > > > > > + uint32_t ct = context.activeState.awb.temperatureK;\n> > > > > > + if (ct == ct_)\n> > > > \n> > > > ct_ should be initialized/reset in configure(), to ensure that we'll\n> > > > always set the CCM matrix on the first frame.\n> > > > \n> > > > I expect temperatureK to be fairly noisy, should we add a threshold to\n> > > > the comparison to avoid updating the CCM parameters on every frame in\n> > > > practice ?\n> > > > \n> > > > > > + return;\n> > > > > \n> > > > > Interestingly, I think this is fine (certainly for now) ... but this\n> > > > > means that the output request metadata will only write the CCM if it\n> > > > > changes.\n> > > \n> > > There was discussion before about \"do we need to always write *all*\n> > > metadata\" and iirc the conclusion was no so...\n> > \n> > I'm sure we'll revisit the topic at some point.\n> > \n> > > > > I think that's actually 'fine' if we imply that any frame that doesn't\n> > > > > supply metadata is the same as the previous state. Though I think RPi\n> > > > > currently reports it for every frame regardless.\n> > > > > \n> > > > > I'd be curious to see how Stefan's tooling handles this.\n> > > > > \n> > > > > We can always update on top, and my comments are addressed so:\n> > > > > \n> > > > > Reviewed-by: Kieran Bingham \n> > > > > \n> > > > > > +\n> > > > > > + ct_ = ct;\n> > > > > > + Matrix ccm = ccm_.get(ct);\n> > > > > > + Matrix offsets = offsets_.get(ct);\n> > > > > > +\n> > > > > > + frameContext.ccm.ccm = ccm;\n> > > > > > +\n> > > > > > + setParameters(params, ccm, offsets);\n> > > > > > +}\n> > > > > > +\n> > > > > > +/**\n> > > > > > + * \\copydoc libcamera::ipa::Algorithm::process\n> > > > > > + */\n> > > > > > +void process([[maybe_unused]] IPAContext &context,\n> > > > > > + [[maybe_unused]] const uint32_t frame,\n> > > > > > + IPAFrameContext &frameContext,\n> > > > > > + [[maybe_unused]] const rkisp1_stat_buffer *stats,\n> > > > > > + ControlList &metadata)\n> > > > > > +{\n> > > > > > + float m[9];\n> > > > > > + for (unsigned int i = 0; i < 3; i++)\n> > > > > > + for (unsigned int j = 0; j < 3; j++)\n> > > > > > + m[i] = frameContext.ccm.ccm[i][j];\n> > > > \n> > > > Curly braces for outer loop.\n> > > > \n> > > > > > + metadata.set(controls::ColourCorrectionMatrix, m);\n> > > > \n> > > > It's interesting we're reporting the matrix but not the offsets. Not a\n> > > > blocker for this series, but should this be addressed ?\n> > > \n> > > I wasn't sure if there was demand for reporting the offsets...\n> > > \n> > > And actually I remember a link [1] that Stefan sent me a long time ago\n> > > that said something about them costing more computation and rarely\n> > > improving results...?\n> > \n> > That's what I was wondering too. I wonder if we should drop the offset.\n> \n> I think they should be there to have the /option/ of being set.\n\nIt costs a little bit of CPU time. Most likely not the end of the world,\nbut things do add up. I suppose we'll get more visibility once we have a\ntuning tool in place to produce the data. If the tool hardcodes offsets\nto 0, I'd rather drop them from the implementation.\n\n> > > [1] https://www.imatest.com/docs/colormatrix/\n> > > \n> > > > > > +}\n> > > > > > +\n> > > > > > +REGISTER_IPA_ALGORITHM(Ccm, \"Ccm\")\n> > > > > > +\n> > > > > > +} /* namespace ipa::rkisp1::algorithms */\n> > > > > > +\n> > > > > > +} /* namespace libcamera */\n> > > > > > diff --git a/src/ipa/rkisp1/algorithms/ccm.h b/src/ipa/rkisp1/algorithms/ccm.h\n> > > > > > new file mode 100644\n> > > > > > index 000000000000..09a6801626b4\n> > > > > > --- /dev/null\n> > > > > > +++ b/src/ipa/rkisp1/algorithms/ccm.h\n> > > > > > @@ -0,0 +1,50 @@\n> > > > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > > > > +/*\n> > > > > > + * Copyright (C) 2024, Ideas On Board\n> > > > > > + *\n> > > > > > + * RkISP1 Color Correction Matrix control algorithm\n> > > > > > + */\n> > > > > > +\n> > > > > > +#pragma once\n> > > > > > +\n> > > > > > +#include \n> > > > > > +\n> > > > > > +#include \"libipa/matrix.h\"\n> > > > > > +#include \"libipa/matrix_interpolator.h\"\n> > > > > > +\n> > > > > > +#include \"algorithm.h\"\n> > > > > > +\n> > > > > > +namespace libcamera {\n> > > > > > +\n> > > > > > +namespace ipa::rkisp1::algorithms {\n> > > > > > +\n> > > > > > +class Ccm : public Algorithm\n> > > > > > +{\n> > > > > > +public:\n> > > > > > + Ccm() {}\n> > > > > > + ~Ccm() = default;\n> > > > > > +\n> > > > > > + int init(IPAContext &context, const YamlObject &tuningData) override;\n> > > > > > + void prepare(IPAContext &context, const uint32_t frame,\n> > > > > > + IPAFrameContext &frameContext,\n> > > > > > + rkisp1_params_cfg *params) override;\n> > > > > > + void process([[maybe_unused]] IPAContext &context,\n> > > > \n> > > > [[maybe_unused]] is not needed in the function declaration.\n> > > > \n> > > > > > + [[maybe_unused]] const uint32_t frame,\n> > > > > > + IPAFrameContext &frameContext,\n> > > > > > + [[maybe_unused]] const rkisp1_stat_buffer *stats,\n> > > > > > + ControlList &metadata) override;\n> > > > > > +\n> > > > > > +private:\n> > > > > > + void parseYaml(const YamlObject &tuningData);\n> > > > > > + void setParameters(rkisp1_params_cfg *params,\n> > > > > > + const Matrix &matrix,\n> > > > > > + const Matrix &offsets);\n> > > > > > +\n> > > > > > + unsigned int ct_;\n> > > > > > + MatrixInterpolator ccm_;\n> > > > \n> > > > Given that the hardware representation is a 4.7 fixed point, would it be\n> > > > enough to use floats instead of doubles (here and in the frame context)\n> > > > ?\n> > > > \n> > > > > > + MatrixInterpolator offsets_;\n> > > > \n> > > > Why is the matrix stored in floating point values and the offsets as\n> > > > integers ?\n> > > \n> > > Because the offsets are integers in hardware as well while the ccm is\n> > > fixed-point.\n> > > \n> > > > > > +};\n> > > > > > +\n> > > > > > +} /* namespace ipa::rkisp1::algorithms */\n> > > > > > +\n> > > > > > +} /* namespace libcamera */\n> > > > > > diff --git a/src/ipa/rkisp1/algorithms/meson.build b/src/ipa/rkisp1/algorithms/meson.build\n> > > > > > index 6ee71a9b5da3..1734a6675f78 100644\n> > > > > > --- a/src/ipa/rkisp1/algorithms/meson.build\n> > > > > > +++ b/src/ipa/rkisp1/algorithms/meson.build\n> > > > > > @@ -4,6 +4,7 @@ rkisp1_ipa_algorithms = files([\n> > > > > > 'agc.cpp',\n> > > > > > 'awb.cpp',\n> > > > > > 'blc.cpp',\n> > > > > > + 'ccm.cpp',\n> > > > > > 'cproc.cpp',\n> > > > > > 'dpcc.cpp',\n> > > > > > 'dpf.cpp',\n> > > > > > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h\n> > > > > > index 2a994d81ae41..cfb1f9770870 100644\n> > > > > > --- a/src/ipa/rkisp1/ipa_context.h\n> > > > > > +++ b/src/ipa/rkisp1/ipa_context.h\n> > > > > > @@ -16,6 +16,7 @@\n> > > > > > #include \n> > > > > > \n> > > > > > #include \n> > > > > > +#include \n> > > > > > \n> > > > > > namespace libcamera {\n> > > > > > \n> > > > > > @@ -155,6 +156,10 @@ struct IPAFrameContext : public FrameContext {\n> > > > > > uint32_t exposure;\n> > > > > > double gain;\n> > > > > > } sensor;\n> > > > > > +\n> > > > > > + struct {\n> > > > > > + Matrix ccm;\n> > > > > > + } ccm;\n> > > > > > };\n> > > > > > \n> > > > > > struct IPAContext {","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 C6B17C3237\n\tfor ;\n\tThu, 13 Jun 2024 12:05:17 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A5E1B65496;\n\tThu, 13 Jun 2024 14:05:16 +0200 (CEST)","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 1458F6548D\n\tfor ;\n\tThu, 13 Jun 2024 14:05:15 +0200 (CEST)","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 A8135743;\n\tThu, 13 Jun 2024 14:05:00 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"LRZVseuK\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1718280300;\n\tbh=Z3MAEfRCbCbNUtNOS5FSZEeFfccY5MGtGdM95PgYvFk=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=LRZVseuKXWXLEcaECpEN7YbU6fw4qZdnFj4bdZ5RbN0PIVeUTJ23HtiVDPj9Oi0po\n\tq0Nt6zZwDZ6zx3H0N5GlkZMgQfZ8dk4LKRCoumDjn3t6aBuYgwK4eI6qNcsyyygBMi\n\tbjeXDGo3oXQhLRj+G6XmS3TDcRxbQO4R8UejwpJ4=","Date":"Thu, 13 Jun 2024 15:04:53 +0300","From":"Laurent Pinchart ","To":"Paul Elder ","Cc":"Kieran Bingham ,\n\tlibcamera-devel@lists.libcamera.org,\n\tStefan Klug ","Subject":"Re: [PATCH v7 3/3] ipa: rkisp1: algorithms: Add crosstalk algorithm","Message-ID":"<20240613120453.GL6019@pendragon.ideasonboard.com>","References":"<20240611140207.520083-1-paul.elder@ideasonboard.com>\n\t<20240611140207.520083-4-paul.elder@ideasonboard.com>\n\t<171811516483.1148289.15469360068204996564@ping.linuxembedded.co.uk>\n\t<20240611235120.GF15006@pendragon.ideasonboard.com>\n\t\n\t<20240612093834.GI28989@pendragon.ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","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\" "}},{"id":29939,"web_url":"https://patchwork.libcamera.org/comment/29939/","msgid":"<3vahyml5r6kfcap65q67qn5ksd67ppcgdu7s6qa6dnm22xjjxx@roehpjndj2mk>","date":"2024-06-13T20:47:58","subject":"Re: [PATCH v7 3/3] ipa: rkisp1: algorithms: Add crosstalk algorithm","submitter":{"id":184,"url":"https://patchwork.libcamera.org/api/people/184/","name":"Stefan Klug","email":"stefan.klug@ideasonboard.com"},"content":"Hi Paul,\n\nI rebased my tuning work on this new series and got a undefined symbol\nwhen loading the ipa. The definition of process() is missing Ccm::\n\n\nOn Tue, Jun 11, 2024 at 11:02:07PM +0900, Paul Elder wrote:\n> Add an algorithm module to the rkisp1 IPA for crosstalk correction.\n> \n> Signed-off-by: Paul Elder \n> Reviewed-by: Stefan Klug \n> \n> ---\n> Changes in v7:\n> - make offsets_ default to zero-matrices as opposed to identity matrices\n> - checkstyle\n> - populate metadata\n> - add ccm to IPAFrameContext\n> - don't update the ccm if the color temperature didn't change\n> \n> No change in v6\n> \n> Changes in v5:\n> - clean up documentation\n> - coalesce parseYaml into init\n> \n> Changes in v4:\n> - remove stray semicolons\n> - use the new matrix interpolator readYaml\n> - use the new matrix operator[] getter\n> \n> Changes in v3:\n> - read ccm offsets from tuning data, and write these offsets to the\n> parameters buffer\n> - make parseYaml return void, as it should fill in default data if\n> unable to read, thus never failing\n> \n> Changes in v2:\n> - rename ctk to ccm\n> - reset the matrix interpolator to identity matrix if failed to read\n> from tuning file\n> ---\n> src/ipa/rkisp1/algorithms/ccm.cpp | 140 ++++++++++++++++++++++++++\n> src/ipa/rkisp1/algorithms/ccm.h | 50 +++++++++\n> src/ipa/rkisp1/algorithms/meson.build | 1 +\n> src/ipa/rkisp1/ipa_context.h | 5 +\n> 4 files changed, 196 insertions(+)\n> create mode 100644 src/ipa/rkisp1/algorithms/ccm.cpp\n> create mode 100644 src/ipa/rkisp1/algorithms/ccm.h\n> \n> diff --git a/src/ipa/rkisp1/algorithms/ccm.cpp b/src/ipa/rkisp1/algorithms/ccm.cpp\n> new file mode 100644\n> index 000000000000..09fe4b2aa1bc\n> --- /dev/null\n> +++ b/src/ipa/rkisp1/algorithms/ccm.cpp\n> @@ -0,0 +1,140 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2024, Ideas On Board\n> + *\n> + * RkISP1 Color Correction Matrix control algorithm\n> + */\n> +\n> +#include \"ccm.h\"\n> +\n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +\n> +#include \n> +#include \n> +\n> +#include \n> +\n> +#include \n> +\n> +#include \"libcamera/internal/yaml_parser.h\"\n> +\n> +#include \"../utils.h\"\n> +#include \"libipa/matrix_interpolator.h\"\n> +\n> +/**\n> + * \\file ccm.h\n> + */\n> +\n> +namespace libcamera {\n> +\n> +namespace ipa::rkisp1::algorithms {\n> +\n> +/**\n> + * \\class Ccm\n> + * \\brief A color correction matrix algorithm\n> + */\n> +\n> +LOG_DEFINE_CATEGORY(RkISP1Ccm)\n> +\n> +/**\n> + * \\copydoc libcamera::ipa::Algorithm::init\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(RkISP1Ccm, Warning)\n> +\t\t\t<< \"Failed to parse 'ccm' \"\n> +\t\t\t<< \"parameter from tuning file; falling back to unit matrix\";\n> +\t\tccm_.reset();\n> +\t}\n> +\n> +\tret = offsets_.readYaml(tuningData[\"ccms\"], \"ct\", \"offsets\");\n> +\tif (ret < 0) {\n> +\t\tLOG(RkISP1Ccm, Warning)\n> +\t\t\t<< \"Failed to parse 'offsets' \"\n> +\t\t\t<< \"parameter from tuning file; falling back to zero offsets\";\n> +\t\t/*\n> +\t\t * MatrixInterpolator::reset() resets to identity matrices\n> +\t\t * while here we need zero matrices so we need to construct it\n> +\t\t * ourselves.\n> +\t\t */\n> +\t\tMatrix m({ 0, 0, 0 });\n> +\t\tstd::map> matrices = { { 0, m } };\n> +\t\toffsets_ = MatrixInterpolator(matrices);\n> +\t}\n> +\n> +\treturn 0;\n> +}\n> +\n> +void Ccm::setParameters(rkisp1_params_cfg *params,\n> +\t\t\tconst Matrix &matrix,\n> +\t\t\tconst Matrix &offsets)\n> +{\n> +\tstruct rkisp1_cif_isp_ctk_config &config = params->others.ctk_config;\n> +\n> +\t/*\n> +\t * 4 bit integer and 7 bit fractional, ranging from -8 (0x400) to\n> +\t * +7.992 (0x3ff)\n> +\t */\n> +\tfor (unsigned int i = 0; i < 3; i++)\n> +\t\tfor (unsigned int j = 0; j < 3; j++)\n> +\t\t\tconfig.coeff[i][j] =\n> +\t\t\t\tutils::floatingToFixedPoint<4, 7, uint16_t, double>(matrix[i][j]);\n> +\n> +\tfor (unsigned int i = 0; i < 3; i++)\n> +\t\tconfig.ct_offset[i] = offsets[i][0] & 0xfff;\n> +\n> +\tLOG(RkISP1Ccm, Debug) << \"Setting matrix \" << matrix;\n> +\tLOG(RkISP1Ccm, Debug) << \"Setting offsets \" << offsets;\n> +\n> +\tparams->module_en_update |= RKISP1_CIF_ISP_MODULE_CTK;\n> +\tparams->module_ens |= RKISP1_CIF_ISP_MODULE_CTK;\n> +\tparams->module_cfg_update |= RKISP1_CIF_ISP_MODULE_CTK;\n> +}\n> +\n> +/**\n> + * \\copydoc libcamera::ipa::Algorithm::prepare\n> + */\n> +void Ccm::prepare(IPAContext &context, [[maybe_unused]] const uint32_t frame,\n> +\t\t IPAFrameContext &frameContext,\n> +\t\t rkisp1_params_cfg *params)\n> +{\n> +\tuint32_t ct = context.activeState.awb.temperatureK;\n> +\tif (ct == ct_)\n> +\t\treturn;\n> +\n> +\tct_ = ct;\n> +\tMatrix ccm = ccm_.get(ct);\n> +\tMatrix offsets = offsets_.get(ct);\n> +\n> +\tframeContext.ccm.ccm = ccm;\n> +\n> +\tsetParameters(params, ccm, offsets);\n> +}\n> +\n> +/**\n> + * \\copydoc libcamera::ipa::Algorithm::process\n> + */\n> +void process([[maybe_unused]] IPAContext &context,\n\nThis needs to be Ccm::process.\n\nCheers,\nStefan\n\n> +\t [[maybe_unused]] const uint32_t frame,\n> +\t IPAFrameContext &frameContext,\n> +\t [[maybe_unused]] const rkisp1_stat_buffer *stats,\n> +\t ControlList &metadata)\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] = frameContext.ccm.ccm[i][j];\n> +\tmetadata.set(controls::ColourCorrectionMatrix, m);\n> +}\n> +\n> +REGISTER_IPA_ALGORITHM(Ccm, \"Ccm\")\n> +\n> +} /* namespace ipa::rkisp1::algorithms */\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/ipa/rkisp1/algorithms/ccm.h b/src/ipa/rkisp1/algorithms/ccm.h\n> new file mode 100644\n> index 000000000000..09a6801626b4\n> --- /dev/null\n> +++ b/src/ipa/rkisp1/algorithms/ccm.h\n> @@ -0,0 +1,50 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2024, Ideas On Board\n> + *\n> + * RkISP1 Color Correction Matrix control algorithm\n> + */\n> +\n> +#pragma once\n> +\n> +#include \n> +\n> +#include \"libipa/matrix.h\"\n> +#include \"libipa/matrix_interpolator.h\"\n> +\n> +#include \"algorithm.h\"\n> +\n> +namespace libcamera {\n> +\n> +namespace ipa::rkisp1::algorithms {\n> +\n> +class Ccm : public Algorithm\n> +{\n> +public:\n> +\tCcm() {}\n> +\t~Ccm() = default;\n> +\n> +\tint init(IPAContext &context, const YamlObject &tuningData) override;\n> +\tvoid prepare(IPAContext &context, const uint32_t frame,\n> +\t\t IPAFrameContext &frameContext,\n> +\t\t rkisp1_params_cfg *params) override;\n> +\tvoid 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 rkisp1_stat_buffer *stats,\n> +\t\t ControlList &metadata) override;\n> +\n> +private:\n> +\tvoid parseYaml(const YamlObject &tuningData);\n> +\tvoid setParameters(rkisp1_params_cfg *params,\n> +\t\t\t const Matrix &matrix,\n> +\t\t\t const Matrix &offsets);\n> +\n> +\tunsigned int ct_;\n> +\tMatrixInterpolator ccm_;\n> +\tMatrixInterpolator offsets_;\n> +};\n> +\n> +} /* namespace ipa::rkisp1::algorithms */\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/ipa/rkisp1/algorithms/meson.build b/src/ipa/rkisp1/algorithms/meson.build\n> index 6ee71a9b5da3..1734a6675f78 100644\n> --- a/src/ipa/rkisp1/algorithms/meson.build\n> +++ b/src/ipa/rkisp1/algorithms/meson.build\n> @@ -4,6 +4,7 @@ rkisp1_ipa_algorithms = files([\n> 'agc.cpp',\n> 'awb.cpp',\n> 'blc.cpp',\n> + 'ccm.cpp',\n> 'cproc.cpp',\n> 'dpcc.cpp',\n> 'dpf.cpp',\n> diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h\n> index 2a994d81ae41..cfb1f9770870 100644\n> --- a/src/ipa/rkisp1/ipa_context.h\n> +++ b/src/ipa/rkisp1/ipa_context.h\n> @@ -16,6 +16,7 @@\n> #include \n> \n> #include \n> +#include \n> \n> namespace libcamera {\n> \n> @@ -155,6 +156,10 @@ struct IPAFrameContext : public FrameContext {\n> \t\tuint32_t exposure;\n> \t\tdouble gain;\n> \t} sensor;\n> +\n> +\tstruct {\n> +\t\tMatrix ccm;\n> +\t} ccm;\n> };\n> \n> struct IPAContext {\n> -- \n> 2.39.2\n>","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 1EE30C3237\n\tfor ;\n\tThu, 13 Jun 2024 20:48:04 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 176FD6548F;\n\tThu, 13 Jun 2024 22:48:03 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 88C016548B\n\tfor ;\n\tThu, 13 Jun 2024 22:48:01 +0200 (CEST)","from ideasonboard.com (unknown\n\t[IPv6:2a00:6020:448c:6c00:1c65:7dc2:a1b2:956d])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 3B5A54CC;\n\tThu, 13 Jun 2024 22:47:47 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"a3GejNpj\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1718311667;\n\tbh=mTXY4JT7L94OfdnPcFAE3PXV0ZumQF/bMvFhmHtlC+w=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=a3GejNpjYNDznZpasH2t3n3ozb3LGIU+f1cxyueI1tBmGE8WcAy2n0DNv0DWgLKUK\n\tipZAvY47Ng+uFAzH69Dl4Cc94hSK+Ct8AQ4hfAEe6mow4UTrJe7xdt4F8HNq5sAZ51\n\t026Q6TgDFQXywermpHY/cajBiA0SIR/bwY7uZNwo=","Date":"Thu, 13 Jun 2024 22:47:58 +0200","From":"Stefan Klug ","To":"Paul Elder ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v7 3/3] ipa: rkisp1: algorithms: Add crosstalk algorithm","Message-ID":"<3vahyml5r6kfcap65q67qn5ksd67ppcgdu7s6qa6dnm22xjjxx@roehpjndj2mk>","References":"<20240611140207.520083-1-paul.elder@ideasonboard.com>\n\t<20240611140207.520083-4-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20240611140207.520083-4-paul.elder@ideasonboard.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\" "}},{"id":29944,"web_url":"https://patchwork.libcamera.org/comment/29944/","msgid":"<171835323576.2248009.15136623245056611795@ping.linuxembedded.co.uk>","date":"2024-06-14T08:20:35","subject":"Re: [PATCH v7 3/3] ipa: rkisp1: algorithms: Add crosstalk algorithm","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Laurent Pinchart (2024-06-13 13:04:53)\n> On Thu, Jun 13, 2024 at 05:13:28PM +0900, Paul Elder wrote:\n> > On Wed, Jun 12, 2024 at 12:38:34PM +0300, Laurent Pinchart wrote:\n> > > On Wed, Jun 12, 2024 at 05:04:38PM +0900, Paul Elder wrote:\n> > > > On Wed, Jun 12, 2024 at 02:51:20AM +0300, Laurent Pinchart wrote:\n> > > > > On Tue, Jun 11, 2024 at 03:12:44PM +0100, Kieran Bingham wrote:\n> > > > > > Quoting Paul Elder (2024-06-11 15:02:07)\n> > > > > > > Add an algorithm module to the rkisp1 IPA for crosstalk correction.\n> > > > > > > \n> > > > > > > Signed-off-by: Paul Elder \n> > > > > > > Reviewed-by: Stefan Klug \n> > > > > > > \n> > > > > > > ---\n> > > > > > > Changes in v7:\n> > > > > > > - make offsets_ default to zero-matrices as opposed to identity matrices\n> > > > > > > - checkstyle\n> > > > > > > - populate metadata\n> > > > > > > - add ccm to IPAFrameContext\n> > > > > > > - don't update the ccm if the color temperature didn't change\n> > > > > > > \n> > > > > > > No change in v6\n> > > > > > > \n> > > > > > > Changes in v5:\n> > > > > > > - clean up documentation\n> > > > > > > - coalesce parseYaml into init\n> > > > > > > \n> > > > > > > Changes in v4:\n> > > > > > > - remove stray semicolons\n> > > > > > > - use the new matrix interpolator readYaml\n> > > > > > > - use the new matrix operator[] getter\n> > > > > > > \n> > > > > > > Changes in v3:\n> > > > > > > - read ccm offsets from tuning data, and write these offsets to the\n> > > > > > > parameters buffer\n> > > > > > > - make parseYaml return void, as it should fill in default data if\n> > > > > > > unable to read, thus never failing\n> > > > > > > \n> > > > > > > Changes in v2:\n> > > > > > > - rename ctk to ccm\n> > > > > > > - reset the matrix interpolator to identity matrix if failed to read\n> > > > > > > from tuning file\n> > > > > > > ---\n> > > > > > > src/ipa/rkisp1/algorithms/ccm.cpp | 140 ++++++++++++++++++++++++++\n> > > > > > > src/ipa/rkisp1/algorithms/ccm.h | 50 +++++++++\n> > > > > > > src/ipa/rkisp1/algorithms/meson.build | 1 +\n> > > > > > > src/ipa/rkisp1/ipa_context.h | 5 +\n> > > > > > > 4 files changed, 196 insertions(+)\n> > > > > > > create mode 100644 src/ipa/rkisp1/algorithms/ccm.cpp\n> > > > > > > create mode 100644 src/ipa/rkisp1/algorithms/ccm.h\n> > > > > > > \n> > > > > > > diff --git a/src/ipa/rkisp1/algorithms/ccm.cpp b/src/ipa/rkisp1/algorithms/ccm.cpp\n> > > > > > > new file mode 100644\n> > > > > > > index 000000000000..09fe4b2aa1bc\n> > > > > > > --- /dev/null\n> > > > > > > +++ b/src/ipa/rkisp1/algorithms/ccm.cpp\n> > > > > > > @@ -0,0 +1,140 @@\n> > > > > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > > > > > +/*\n> > > > > > > + * Copyright (C) 2024, Ideas On Board\n> > > > > > > + *\n> > > > > > > + * RkISP1 Color Correction Matrix control algorithm\n> > > > > > > + */\n> > > > > > > +\n> > > > > > > +#include \"ccm.h\"\n> > > > > > > +\n> > > > > > > +#include \n> > > > > > > +#include \n> > > > > > > +#include \n> > > > > > > +#include \n> > > > > > > +#include \n> > > > > > > +\n> > > > > > > +#include \n> > > > > > > +#include \n> > > > > > > +\n> > > > > > > +#include \n> > > > > > > +\n> > > > > > > +#include \n> > > > > > > +\n> > > > > > > +#include \"libcamera/internal/yaml_parser.h\"\n> > > > > > > +\n> > > > > > > +#include \"../utils.h\"\n> > > > > > > +#include \"libipa/matrix_interpolator.h\"\n> > > > > > > +\n> > > > > > > +/**\n> > > > > > > + * \\file ccm.h\n> > > > > > > + */\n> > > > > > > +\n> > > > > > > +namespace libcamera {\n> > > > > > > +\n> > > > > > > +namespace ipa::rkisp1::algorithms {\n> > > > > > > +\n> > > > > > > +/**\n> > > > > > > + * \\class Ccm\n> > > > > > > + * \\brief A color correction matrix algorithm\n> > > > > > > + */\n> > > > > > > +\n> > > > > > > +LOG_DEFINE_CATEGORY(RkISP1Ccm)\n> > > > > > > +\n> > > > > > > +/**\n> > > > > > > + * \\copydoc libcamera::ipa::Algorithm::init\n> > > > > > > + */\n> > > > > > > +int Ccm::init([[maybe_unused]] IPAContext &context, const YamlObject &tuningData)\n> > > > > > > +{\n> > > > > > > + int ret = ccm_.readYaml(tuningData[\"ccms\"], \"ct\", \"ccm\");\n> > > > > > > + if (ret < 0) {\n> > > > > > > + LOG(RkISP1Ccm, Warning)\n> > > > > > > + << \"Failed to parse 'ccm' \"\n> > > > > > > + << \"parameter from tuning file; falling back to unit matrix\";\n> > > > > > > + ccm_.reset();\n> > > > > > > + }\n> > > > > > > +\n> > > > > > > + ret = offsets_.readYaml(tuningData[\"ccms\"], \"ct\", \"offsets\");\n> > > > > > > + if (ret < 0) {\n> > > > > > > + LOG(RkISP1Ccm, Warning)\n> > > > > > > + << \"Failed to parse 'offsets' \"\n> > > > > > > + << \"parameter from tuning file; falling back to zero offsets\";\n> > > > > > > + /*\n> > > > > > > + * MatrixInterpolator::reset() resets to identity matrices\n> > > > > > > + * while here we need zero matrices so we need to construct it\n> > > > > > > + * ourselves.\n> > > > > > > + */\n> > > > > > > + Matrix m({ 0, 0, 0 });\n> > > > > > > + std::map> matrices = { { 0, m } };\n> > > > > > > + offsets_ = MatrixInterpolator(matrices);\n> > > > > > > + }\n> > > > > > > +\n> > > > > > > + return 0;\n> > > > > > > +}\n> > > > > > > +\n> > > > > > > +void Ccm::setParameters(rkisp1_params_cfg *params,\n> > > > > > > + const Matrix &matrix,\n> > > > > > > + const Matrix &offsets)\n> > > > > > > +{\n> > > > > > > + struct rkisp1_cif_isp_ctk_config &config = params->others.ctk_config;\n> > > > > > > +\n> > > > > > > + /*\n> > > > > > > + * 4 bit integer and 7 bit fractional, ranging from -8 (0x400) to\n> > > > > > > + * +7.992 (0x3ff)\n> > > > > > > + */\n> > > > > > > + for (unsigned int i = 0; i < 3; i++)\n> > > > > > > + for (unsigned int j = 0; j < 3; j++)\n> > > > > > > + config.coeff[i][j] =\n> > > > > > > + utils::floatingToFixedPoint<4, 7, uint16_t, double>(matrix[i][j]);\n> > > > > \n> > > > > Curly braces for outer loop.\n> > > > > \n> > > > > > > +\n> > > > > > > + for (unsigned int i = 0; i < 3; i++)\n> > > > > > > + config.ct_offset[i] = offsets[i][0] & 0xfff;\n> > > > > > > +\n> > > > > > > + LOG(RkISP1Ccm, Debug) << \"Setting matrix \" << matrix;\n> > > > > > > + LOG(RkISP1Ccm, Debug) << \"Setting offsets \" << offsets;\n> > > > > > > +\n> > > > > > > + params->module_en_update |= RKISP1_CIF_ISP_MODULE_CTK;\n> > > > > > > + params->module_ens |= RKISP1_CIF_ISP_MODULE_CTK;\n> > > > > > > + params->module_cfg_update |= RKISP1_CIF_ISP_MODULE_CTK;\n> > > > > > > +}\n> > > > > > > +\n> > > > > > > +/**\n> > > > > > > + * \\copydoc libcamera::ipa::Algorithm::prepare\n> > > > > > > + */\n> > > > > > > +void Ccm::prepare(IPAContext &context, [[maybe_unused]] const uint32_t frame,\n> > > > > > > + IPAFrameContext &frameContext,\n> > > > > > > + rkisp1_params_cfg *params)\n> > > > > > > +{\n> > > > > > > + uint32_t ct = context.activeState.awb.temperatureK;\n> > > > > > > + if (ct == ct_)\n> > > > > \n> > > > > ct_ should be initialized/reset in configure(), to ensure that we'll\n> > > > > always set the CCM matrix on the first frame.\n> > > > > \n> > > > > I expect temperatureK to be fairly noisy, should we add a threshold to\n> > > > > the comparison to avoid updating the CCM parameters on every frame in\n> > > > > practice ?\n> > > > > \n> > > > > > > + return;\n> > > > > > \n> > > > > > Interestingly, I think this is fine (certainly for now) ... but this\n> > > > > > means that the output request metadata will only write the CCM if it\n> > > > > > changes.\n> > > > \n> > > > There was discussion before about \"do we need to always write *all*\n> > > > metadata\" and iirc the conclusion was no so...\n> > > \n> > > I'm sure we'll revisit the topic at some point.\n> > > \n> > > > > > I think that's actually 'fine' if we imply that any frame that doesn't\n> > > > > > supply metadata is the same as the previous state. Though I think RPi\n> > > > > > currently reports it for every frame regardless.\n> > > > > > \n> > > > > > I'd be curious to see how Stefan's tooling handles this.\n> > > > > > \n> > > > > > We can always update on top, and my comments are addressed so:\n> > > > > > \n> > > > > > Reviewed-by: Kieran Bingham \n> > > > > > \n> > > > > > > +\n> > > > > > > + ct_ = ct;\n> > > > > > > + Matrix ccm = ccm_.get(ct);\n> > > > > > > + Matrix offsets = offsets_.get(ct);\n> > > > > > > +\n> > > > > > > + frameContext.ccm.ccm = ccm;\n> > > > > > > +\n> > > > > > > + setParameters(params, ccm, offsets);\n> > > > > > > +}\n> > > > > > > +\n> > > > > > > +/**\n> > > > > > > + * \\copydoc libcamera::ipa::Algorithm::process\n> > > > > > > + */\n> > > > > > > +void process([[maybe_unused]] IPAContext &context,\n> > > > > > > + [[maybe_unused]] const uint32_t frame,\n> > > > > > > + IPAFrameContext &frameContext,\n> > > > > > > + [[maybe_unused]] const rkisp1_stat_buffer *stats,\n> > > > > > > + ControlList &metadata)\n> > > > > > > +{\n> > > > > > > + float m[9];\n> > > > > > > + for (unsigned int i = 0; i < 3; i++)\n> > > > > > > + for (unsigned int j = 0; j < 3; j++)\n> > > > > > > + m[i] = frameContext.ccm.ccm[i][j];\n> > > > > \n> > > > > Curly braces for outer loop.\n> > > > > \n> > > > > > > + metadata.set(controls::ColourCorrectionMatrix, m);\n> > > > > \n> > > > > It's interesting we're reporting the matrix but not the offsets. Not a\n> > > > > blocker for this series, but should this be addressed ?\n> > > > \n> > > > I wasn't sure if there was demand for reporting the offsets...\n> > > > \n> > > > And actually I remember a link [1] that Stefan sent me a long time ago\n> > > > that said something about them costing more computation and rarely\n> > > > improving results...?\n> > > \n> > > That's what I was wondering too. I wonder if we should drop the offset.\n> > \n> > I think they should be there to have the /option/ of being set.\n> \n> It costs a little bit of CPU time. Most likely not the end of the world,\n> but things do add up. I suppose we'll get more visibility once we have a\n\nYes, but if we don't have the ability to control we won't be able to\nexperiement. I'd rather keep this in. The cost is low. Unless we're\nreally worried about the cost of 3 integers...\n\nReviewed-by: Kieran Bingham \n\n\n> tuning tool in place to produce the data. If the tool hardcodes offsets\n> to 0, I'd rather drop them from the implementation.\n> \n> > > > [1] https://www.imatest.com/docs/colormatrix/\n> > > > \n> > > > > > > +}\n> > > > > > > +\n> > > > > > > +REGISTER_IPA_ALGORITHM(Ccm, \"Ccm\")\n> > > > > > > +\n> > > > > > > +} /* namespace ipa::rkisp1::algorithms */\n> > > > > > > +\n> > > > > > > +} /* namespace libcamera */\n> > > > > > > diff --git a/src/ipa/rkisp1/algorithms/ccm.h b/src/ipa/rkisp1/algorithms/ccm.h\n> > > > > > > new file mode 100644\n> > > > > > > index 000000000000..09a6801626b4\n> > > > > > > --- /dev/null\n> > > > > > > +++ b/src/ipa/rkisp1/algorithms/ccm.h\n> > > > > > > @@ -0,0 +1,50 @@\n> > > > > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > > > > > +/*\n> > > > > > > + * Copyright (C) 2024, Ideas On Board\n> > > > > > > + *\n> > > > > > > + * RkISP1 Color Correction Matrix control algorithm\n> > > > > > > + */\n> > > > > > > +\n> > > > > > > +#pragma once\n> > > > > > > +\n> > > > > > > +#include \n> > > > > > > +\n> > > > > > > +#include \"libipa/matrix.h\"\n> > > > > > > +#include \"libipa/matrix_interpolator.h\"\n> > > > > > > +\n> > > > > > > +#include \"algorithm.h\"\n> > > > > > > +\n> > > > > > > +namespace libcamera {\n> > > > > > > +\n> > > > > > > +namespace ipa::rkisp1::algorithms {\n> > > > > > > +\n> > > > > > > +class Ccm : public Algorithm\n> > > > > > > +{\n> > > > > > > +public:\n> > > > > > > + Ccm() {}\n> > > > > > > + ~Ccm() = default;\n> > > > > > > +\n> > > > > > > + int init(IPAContext &context, const YamlObject &tuningData) override;\n> > > > > > > + void prepare(IPAContext &context, const uint32_t frame,\n> > > > > > > + IPAFrameContext &frameContext,\n> > > > > > > + rkisp1_params_cfg *params) override;\n> > > > > > > + void process([[maybe_unused]] IPAContext &context,\n> > > > > \n> > > > > [[maybe_unused]] is not needed in the function declaration.\n> > > > > \n> > > > > > > + [[maybe_unused]] const uint32_t frame,\n> > > > > > > + IPAFrameContext &frameContext,\n> > > > > > > + [[maybe_unused]] const rkisp1_stat_buffer *stats,\n> > > > > > > + ControlList &metadata) override;\n> > > > > > > +\n> > > > > > > +private:\n> > > > > > > + void parseYaml(const YamlObject &tuningData);\n> > > > > > > + void setParameters(rkisp1_params_cfg *params,\n> > > > > > > + const Matrix &matrix,\n> > > > > > > + const Matrix &offsets);\n> > > > > > > +\n> > > > > > > + unsigned int ct_;\n> > > > > > > + MatrixInterpolator ccm_;\n> > > > > \n> > > > > Given that the hardware representation is a 4.7 fixed point, would it be\n> > > > > enough to use floats instead of doubles (here and in the frame context)\n> > > > > ?\n> > > > > \n> > > > > > > + MatrixInterpolator offsets_;\n> > > > > \n> > > > > Why is the matrix stored in floating point values and the offsets as\n> > > > > integers ?\n> > > > \n> > > > Because the offsets are integers in hardware as well while the ccm is\n> > > > fixed-point.\n> > > > \n> > > > > > > +};\n> > > > > > > +\n> > > > > > > +} /* namespace ipa::rkisp1::algorithms */\n> > > > > > > +\n> > > > > > > +} /* namespace libcamera */\n> > > > > > > diff --git a/src/ipa/rkisp1/algorithms/meson.build b/src/ipa/rkisp1/algorithms/meson.build\n> > > > > > > index 6ee71a9b5da3..1734a6675f78 100644\n> > > > > > > --- a/src/ipa/rkisp1/algorithms/meson.build\n> > > > > > > +++ b/src/ipa/rkisp1/algorithms/meson.build\n> > > > > > > @@ -4,6 +4,7 @@ rkisp1_ipa_algorithms = files([\n> > > > > > > 'agc.cpp',\n> > > > > > > 'awb.cpp',\n> > > > > > > 'blc.cpp',\n> > > > > > > + 'ccm.cpp',\n> > > > > > > 'cproc.cpp',\n> > > > > > > 'dpcc.cpp',\n> > > > > > > 'dpf.cpp',\n> > > > > > > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h\n> > > > > > > index 2a994d81ae41..cfb1f9770870 100644\n> > > > > > > --- a/src/ipa/rkisp1/ipa_context.h\n> > > > > > > +++ b/src/ipa/rkisp1/ipa_context.h\n> > > > > > > @@ -16,6 +16,7 @@\n> > > > > > > #include \n> > > > > > > \n> > > > > > > #include \n> > > > > > > +#include \n> > > > > > > \n> > > > > > > namespace libcamera {\n> > > > > > > \n> > > > > > > @@ -155,6 +156,10 @@ struct IPAFrameContext : public FrameContext {\n> > > > > > > uint32_t exposure;\n> > > > > > > double gain;\n> > > > > > > } sensor;\n> > > > > > > +\n> > > > > > > + struct {\n> > > > > > > + Matrix ccm;\n> > > > > > > + } ccm;\n> > > > > > > };\n> > > > > > > \n> > > > > > > struct IPAContext {\n> \n> -- \n> Regards,\n> \n> Laurent Pinchart","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 C68B8C3237\n\tfor ;\n\tFri, 14 Jun 2024 08:20:40 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B12896548D;\n\tFri, 14 Jun 2024 10:20:39 +0200 (CEST)","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 4983F65456\n\tfor ;\n\tFri, 14 Jun 2024 10:20:38 +0200 (CEST)","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 7554E183;\n\tFri, 14 Jun 2024 10:20:23 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"R9kAGhaF\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1718353223;\n\tbh=U9OH+0sb22fOj9ooaqRDJwJBzWgb82l9SPyJHs+3LO0=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=R9kAGhaFIU0VEJDW78WK/zkuXZSI3TFo28nBLHbvGMYLrFXC+J4yHaxhPUcsw5WeQ\n\tLopyMSyk7TEOBEoCPKhnAc/z2hhewqaEhHk+GgV3YjcRKtsAbtpKxO3BxK0OJkvQdd\n\tq343lGlEWkROVgAdroTPyXoFn/j+bCmO4kisCYPE=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20240613120453.GL6019@pendragon.ideasonboard.com>","References":"<20240611140207.520083-1-paul.elder@ideasonboard.com>\n\t<20240611140207.520083-4-paul.elder@ideasonboard.com>\n\t<171811516483.1148289.15469360068204996564@ping.linuxembedded.co.uk>\n\t<20240611235120.GF15006@pendragon.ideasonboard.com>\n\t\n\t<20240612093834.GI28989@pendragon.ideasonboard.com>\n\t\n\t<20240613120453.GL6019@pendragon.ideasonboard.com>","Subject":"Re: [PATCH v7 3/3] ipa: rkisp1: algorithms: Add crosstalk algorithm","From":"Kieran Bingham ","Cc":"libcamera-devel@lists.libcamera.org,\n\tStefan Klug ","To":"Laurent Pinchart ,\n\tPaul Elder ","Date":"Fri, 14 Jun 2024 09:20:35 +0100","Message-ID":"<171835323576.2248009.15136623245056611795@ping.linuxembedded.co.uk>","User-Agent":"alot/0.10","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\" "}},{"id":29959,"web_url":"https://patchwork.libcamera.org/comment/29959/","msgid":"<20240616171458.GA5954@pendragon.ideasonboard.com>","date":"2024-06-16T17:14:58","subject":"Re: [PATCH v7 3/3] ipa: rkisp1: algorithms: Add crosstalk algorithm","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Fri, Jun 14, 2024 at 09:20:35AM +0100, Kieran Bingham wrote:\n> Quoting Laurent Pinchart (2024-06-13 13:04:53)\n> > On Thu, Jun 13, 2024 at 05:13:28PM +0900, Paul Elder wrote:\n> > > On Wed, Jun 12, 2024 at 12:38:34PM +0300, Laurent Pinchart wrote:\n> > > > On Wed, Jun 12, 2024 at 05:04:38PM +0900, Paul Elder wrote:\n> > > > > On Wed, Jun 12, 2024 at 02:51:20AM +0300, Laurent Pinchart wrote:\n> > > > > > On Tue, Jun 11, 2024 at 03:12:44PM +0100, Kieran Bingham wrote:\n> > > > > > > Quoting Paul Elder (2024-06-11 15:02:07)\n> > > > > > > > Add an algorithm module to the rkisp1 IPA for crosstalk correction.\n> > > > > > > > \n> > > > > > > > Signed-off-by: Paul Elder \n> > > > > > > > Reviewed-by: Stefan Klug \n> > > > > > > > \n> > > > > > > > ---\n> > > > > > > > Changes in v7:\n> > > > > > > > - make offsets_ default to zero-matrices as opposed to identity matrices\n> > > > > > > > - checkstyle\n> > > > > > > > - populate metadata\n> > > > > > > > - add ccm to IPAFrameContext\n> > > > > > > > - don't update the ccm if the color temperature didn't change\n> > > > > > > > \n> > > > > > > > No change in v6\n> > > > > > > > \n> > > > > > > > Changes in v5:\n> > > > > > > > - clean up documentation\n> > > > > > > > - coalesce parseYaml into init\n> > > > > > > > \n> > > > > > > > Changes in v4:\n> > > > > > > > - remove stray semicolons\n> > > > > > > > - use the new matrix interpolator readYaml\n> > > > > > > > - use the new matrix operator[] getter\n> > > > > > > > \n> > > > > > > > Changes in v3:\n> > > > > > > > - read ccm offsets from tuning data, and write these offsets to the\n> > > > > > > > parameters buffer\n> > > > > > > > - make parseYaml return void, as it should fill in default data if\n> > > > > > > > unable to read, thus never failing\n> > > > > > > > \n> > > > > > > > Changes in v2:\n> > > > > > > > - rename ctk to ccm\n> > > > > > > > - reset the matrix interpolator to identity matrix if failed to read\n> > > > > > > > from tuning file\n> > > > > > > > ---\n> > > > > > > > src/ipa/rkisp1/algorithms/ccm.cpp | 140 ++++++++++++++++++++++++++\n> > > > > > > > src/ipa/rkisp1/algorithms/ccm.h | 50 +++++++++\n> > > > > > > > src/ipa/rkisp1/algorithms/meson.build | 1 +\n> > > > > > > > src/ipa/rkisp1/ipa_context.h | 5 +\n> > > > > > > > 4 files changed, 196 insertions(+)\n> > > > > > > > create mode 100644 src/ipa/rkisp1/algorithms/ccm.cpp\n> > > > > > > > create mode 100644 src/ipa/rkisp1/algorithms/ccm.h\n> > > > > > > > \n> > > > > > > > diff --git a/src/ipa/rkisp1/algorithms/ccm.cpp b/src/ipa/rkisp1/algorithms/ccm.cpp\n> > > > > > > > new file mode 100644\n> > > > > > > > index 000000000000..09fe4b2aa1bc\n> > > > > > > > --- /dev/null\n> > > > > > > > +++ b/src/ipa/rkisp1/algorithms/ccm.cpp\n> > > > > > > > @@ -0,0 +1,140 @@\n> > > > > > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > > > > > > +/*\n> > > > > > > > + * Copyright (C) 2024, Ideas On Board\n> > > > > > > > + *\n> > > > > > > > + * RkISP1 Color Correction Matrix control algorithm\n> > > > > > > > + */\n> > > > > > > > +\n> > > > > > > > +#include \"ccm.h\"\n> > > > > > > > +\n> > > > > > > > +#include \n> > > > > > > > +#include \n> > > > > > > > +#include \n> > > > > > > > +#include \n> > > > > > > > +#include \n> > > > > > > > +\n> > > > > > > > +#include \n> > > > > > > > +#include \n> > > > > > > > +\n> > > > > > > > +#include \n> > > > > > > > +\n> > > > > > > > +#include \n> > > > > > > > +\n> > > > > > > > +#include \"libcamera/internal/yaml_parser.h\"\n> > > > > > > > +\n> > > > > > > > +#include \"../utils.h\"\n> > > > > > > > +#include \"libipa/matrix_interpolator.h\"\n> > > > > > > > +\n> > > > > > > > +/**\n> > > > > > > > + * \\file ccm.h\n> > > > > > > > + */\n> > > > > > > > +\n> > > > > > > > +namespace libcamera {\n> > > > > > > > +\n> > > > > > > > +namespace ipa::rkisp1::algorithms {\n> > > > > > > > +\n> > > > > > > > +/**\n> > > > > > > > + * \\class Ccm\n> > > > > > > > + * \\brief A color correction matrix algorithm\n> > > > > > > > + */\n> > > > > > > > +\n> > > > > > > > +LOG_DEFINE_CATEGORY(RkISP1Ccm)\n> > > > > > > > +\n> > > > > > > > +/**\n> > > > > > > > + * \\copydoc libcamera::ipa::Algorithm::init\n> > > > > > > > + */\n> > > > > > > > +int Ccm::init([[maybe_unused]] IPAContext &context, const YamlObject &tuningData)\n> > > > > > > > +{\n> > > > > > > > + int ret = ccm_.readYaml(tuningData[\"ccms\"], \"ct\", \"ccm\");\n> > > > > > > > + if (ret < 0) {\n> > > > > > > > + LOG(RkISP1Ccm, Warning)\n> > > > > > > > + << \"Failed to parse 'ccm' \"\n> > > > > > > > + << \"parameter from tuning file; falling back to unit matrix\";\n> > > > > > > > + ccm_.reset();\n> > > > > > > > + }\n> > > > > > > > +\n> > > > > > > > + ret = offsets_.readYaml(tuningData[\"ccms\"], \"ct\", \"offsets\");\n> > > > > > > > + if (ret < 0) {\n> > > > > > > > + LOG(RkISP1Ccm, Warning)\n> > > > > > > > + << \"Failed to parse 'offsets' \"\n> > > > > > > > + << \"parameter from tuning file; falling back to zero offsets\";\n> > > > > > > > + /*\n> > > > > > > > + * MatrixInterpolator::reset() resets to identity matrices\n> > > > > > > > + * while here we need zero matrices so we need to construct it\n> > > > > > > > + * ourselves.\n> > > > > > > > + */\n> > > > > > > > + Matrix m({ 0, 0, 0 });\n> > > > > > > > + std::map> matrices = { { 0, m } };\n> > > > > > > > + offsets_ = MatrixInterpolator(matrices);\n> > > > > > > > + }\n> > > > > > > > +\n> > > > > > > > + return 0;\n> > > > > > > > +}\n> > > > > > > > +\n> > > > > > > > +void Ccm::setParameters(rkisp1_params_cfg *params,\n> > > > > > > > + const Matrix &matrix,\n> > > > > > > > + const Matrix &offsets)\n> > > > > > > > +{\n> > > > > > > > + struct rkisp1_cif_isp_ctk_config &config = params->others.ctk_config;\n> > > > > > > > +\n> > > > > > > > + /*\n> > > > > > > > + * 4 bit integer and 7 bit fractional, ranging from -8 (0x400) to\n> > > > > > > > + * +7.992 (0x3ff)\n> > > > > > > > + */\n> > > > > > > > + for (unsigned int i = 0; i < 3; i++)\n> > > > > > > > + for (unsigned int j = 0; j < 3; j++)\n> > > > > > > > + config.coeff[i][j] =\n> > > > > > > > + utils::floatingToFixedPoint<4, 7, uint16_t, double>(matrix[i][j]);\n> > > > > > \n> > > > > > Curly braces for outer loop.\n> > > > > > \n> > > > > > > > +\n> > > > > > > > + for (unsigned int i = 0; i < 3; i++)\n> > > > > > > > + config.ct_offset[i] = offsets[i][0] & 0xfff;\n> > > > > > > > +\n> > > > > > > > + LOG(RkISP1Ccm, Debug) << \"Setting matrix \" << matrix;\n> > > > > > > > + LOG(RkISP1Ccm, Debug) << \"Setting offsets \" << offsets;\n> > > > > > > > +\n> > > > > > > > + params->module_en_update |= RKISP1_CIF_ISP_MODULE_CTK;\n> > > > > > > > + params->module_ens |= RKISP1_CIF_ISP_MODULE_CTK;\n> > > > > > > > + params->module_cfg_update |= RKISP1_CIF_ISP_MODULE_CTK;\n> > > > > > > > +}\n> > > > > > > > +\n> > > > > > > > +/**\n> > > > > > > > + * \\copydoc libcamera::ipa::Algorithm::prepare\n> > > > > > > > + */\n> > > > > > > > +void Ccm::prepare(IPAContext &context, [[maybe_unused]] const uint32_t frame,\n> > > > > > > > + IPAFrameContext &frameContext,\n> > > > > > > > + rkisp1_params_cfg *params)\n> > > > > > > > +{\n> > > > > > > > + uint32_t ct = context.activeState.awb.temperatureK;\n> > > > > > > > + if (ct == ct_)\n> > > > > > \n> > > > > > ct_ should be initialized/reset in configure(), to ensure that we'll\n> > > > > > always set the CCM matrix on the first frame.\n> > > > > > \n> > > > > > I expect temperatureK to be fairly noisy, should we add a threshold to\n> > > > > > the comparison to avoid updating the CCM parameters on every frame in\n> > > > > > practice ?\n> > > > > > \n> > > > > > > > + return;\n> > > > > > > \n> > > > > > > Interestingly, I think this is fine (certainly for now) ... but this\n> > > > > > > means that the output request metadata will only write the CCM if it\n> > > > > > > changes.\n> > > > > \n> > > > > There was discussion before about \"do we need to always write *all*\n> > > > > metadata\" and iirc the conclusion was no so...\n> > > > \n> > > > I'm sure we'll revisit the topic at some point.\n> > > > \n> > > > > > > I think that's actually 'fine' if we imply that any frame that doesn't\n> > > > > > > supply metadata is the same as the previous state. Though I think RPi\n> > > > > > > currently reports it for every frame regardless.\n> > > > > > > \n> > > > > > > I'd be curious to see how Stefan's tooling handles this.\n> > > > > > > \n> > > > > > > We can always update on top, and my comments are addressed so:\n> > > > > > > \n> > > > > > > Reviewed-by: Kieran Bingham \n> > > > > > > \n> > > > > > > > +\n> > > > > > > > + ct_ = ct;\n> > > > > > > > + Matrix ccm = ccm_.get(ct);\n> > > > > > > > + Matrix offsets = offsets_.get(ct);\n> > > > > > > > +\n> > > > > > > > + frameContext.ccm.ccm = ccm;\n> > > > > > > > +\n> > > > > > > > + setParameters(params, ccm, offsets);\n> > > > > > > > +}\n> > > > > > > > +\n> > > > > > > > +/**\n> > > > > > > > + * \\copydoc libcamera::ipa::Algorithm::process\n> > > > > > > > + */\n> > > > > > > > +void process([[maybe_unused]] IPAContext &context,\n> > > > > > > > + [[maybe_unused]] const uint32_t frame,\n> > > > > > > > + IPAFrameContext &frameContext,\n> > > > > > > > + [[maybe_unused]] const rkisp1_stat_buffer *stats,\n> > > > > > > > + ControlList &metadata)\n> > > > > > > > +{\n> > > > > > > > + float m[9];\n> > > > > > > > + for (unsigned int i = 0; i < 3; i++)\n> > > > > > > > + for (unsigned int j = 0; j < 3; j++)\n> > > > > > > > + m[i] = frameContext.ccm.ccm[i][j];\n> > > > > > \n> > > > > > Curly braces for outer loop.\n> > > > > > \n> > > > > > > > + metadata.set(controls::ColourCorrectionMatrix, m);\n> > > > > > \n> > > > > > It's interesting we're reporting the matrix but not the offsets. Not a\n> > > > > > blocker for this series, but should this be addressed ?\n> > > > > \n> > > > > I wasn't sure if there was demand for reporting the offsets...\n> > > > > \n> > > > > And actually I remember a link [1] that Stefan sent me a long time ago\n> > > > > that said something about them costing more computation and rarely\n> > > > > improving results...?\n> > > > \n> > > > That's what I was wondering too. I wonder if we should drop the offset.\n> > > \n> > > I think they should be there to have the /option/ of being set.\n> > \n> > It costs a little bit of CPU time. Most likely not the end of the world,\n> > but things do add up. I suppose we'll get more visibility once we have a\n> \n> Yes, but if we don't have the ability to control we won't be able to\n> experiement. I'd rather keep this in. The cost is low. Unless we're\n> really worried about the cost of 3 integers...\n\nIt's more the CPU cost that we should try to save. We can revisit this\nlater after experimenting with the tuning.\n\n> Reviewed-by: Kieran Bingham \n> \n> > tuning tool in place to produce the data. If the tool hardcodes offsets\n> > to 0, I'd rather drop them from the implementation.\n> > \n> > > > > [1] https://www.imatest.com/docs/colormatrix/\n> > > > > \n> > > > > > > > +}\n> > > > > > > > +\n> > > > > > > > +REGISTER_IPA_ALGORITHM(Ccm, \"Ccm\")\n> > > > > > > > +\n> > > > > > > > +} /* namespace ipa::rkisp1::algorithms */\n> > > > > > > > +\n> > > > > > > > +} /* namespace libcamera */\n> > > > > > > > diff --git a/src/ipa/rkisp1/algorithms/ccm.h b/src/ipa/rkisp1/algorithms/ccm.h\n> > > > > > > > new file mode 100644\n> > > > > > > > index 000000000000..09a6801626b4\n> > > > > > > > --- /dev/null\n> > > > > > > > +++ b/src/ipa/rkisp1/algorithms/ccm.h\n> > > > > > > > @@ -0,0 +1,50 @@\n> > > > > > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > > > > > > +/*\n> > > > > > > > + * Copyright (C) 2024, Ideas On Board\n> > > > > > > > + *\n> > > > > > > > + * RkISP1 Color Correction Matrix control algorithm\n> > > > > > > > + */\n> > > > > > > > +\n> > > > > > > > +#pragma once\n> > > > > > > > +\n> > > > > > > > +#include \n> > > > > > > > +\n> > > > > > > > +#include \"libipa/matrix.h\"\n> > > > > > > > +#include \"libipa/matrix_interpolator.h\"\n> > > > > > > > +\n> > > > > > > > +#include \"algorithm.h\"\n> > > > > > > > +\n> > > > > > > > +namespace libcamera {\n> > > > > > > > +\n> > > > > > > > +namespace ipa::rkisp1::algorithms {\n> > > > > > > > +\n> > > > > > > > +class Ccm : public Algorithm\n> > > > > > > > +{\n> > > > > > > > +public:\n> > > > > > > > + Ccm() {}\n> > > > > > > > + ~Ccm() = default;\n> > > > > > > > +\n> > > > > > > > + int init(IPAContext &context, const YamlObject &tuningData) override;\n> > > > > > > > + void prepare(IPAContext &context, const uint32_t frame,\n> > > > > > > > + IPAFrameContext &frameContext,\n> > > > > > > > + rkisp1_params_cfg *params) override;\n> > > > > > > > + void process([[maybe_unused]] IPAContext &context,\n> > > > > > \n> > > > > > [[maybe_unused]] is not needed in the function declaration.\n> > > > > > \n> > > > > > > > + [[maybe_unused]] const uint32_t frame,\n> > > > > > > > + IPAFrameContext &frameContext,\n> > > > > > > > + [[maybe_unused]] const rkisp1_stat_buffer *stats,\n> > > > > > > > + ControlList &metadata) override;\n> > > > > > > > +\n> > > > > > > > +private:\n> > > > > > > > + void parseYaml(const YamlObject &tuningData);\n> > > > > > > > + void setParameters(rkisp1_params_cfg *params,\n> > > > > > > > + const Matrix &matrix,\n> > > > > > > > + const Matrix &offsets);\n> > > > > > > > +\n> > > > > > > > + unsigned int ct_;\n> > > > > > > > + MatrixInterpolator ccm_;\n> > > > > > \n> > > > > > Given that the hardware representation is a 4.7 fixed point, would it be\n> > > > > > enough to use floats instead of doubles (here and in the frame context)\n> > > > > > ?\n> > > > > > \n> > > > > > > > + MatrixInterpolator offsets_;\n> > > > > > \n> > > > > > Why is the matrix stored in floating point values and the offsets as\n> > > > > > integers ?\n> > > > > \n> > > > > Because the offsets are integers in hardware as well while the ccm is\n> > > > > fixed-point.\n> > > > > \n> > > > > > > > +};\n> > > > > > > > +\n> > > > > > > > +} /* namespace ipa::rkisp1::algorithms */\n> > > > > > > > +\n> > > > > > > > +} /* namespace libcamera */\n> > > > > > > > diff --git a/src/ipa/rkisp1/algorithms/meson.build b/src/ipa/rkisp1/algorithms/meson.build\n> > > > > > > > index 6ee71a9b5da3..1734a6675f78 100644\n> > > > > > > > --- a/src/ipa/rkisp1/algorithms/meson.build\n> > > > > > > > +++ b/src/ipa/rkisp1/algorithms/meson.build\n> > > > > > > > @@ -4,6 +4,7 @@ rkisp1_ipa_algorithms = files([\n> > > > > > > > 'agc.cpp',\n> > > > > > > > 'awb.cpp',\n> > > > > > > > 'blc.cpp',\n> > > > > > > > + 'ccm.cpp',\n> > > > > > > > 'cproc.cpp',\n> > > > > > > > 'dpcc.cpp',\n> > > > > > > > 'dpf.cpp',\n> > > > > > > > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h\n> > > > > > > > index 2a994d81ae41..cfb1f9770870 100644\n> > > > > > > > --- a/src/ipa/rkisp1/ipa_context.h\n> > > > > > > > +++ b/src/ipa/rkisp1/ipa_context.h\n> > > > > > > > @@ -16,6 +16,7 @@\n> > > > > > > > #include \n> > > > > > > > \n> > > > > > > > #include \n> > > > > > > > +#include \n> > > > > > > > \n> > > > > > > > namespace libcamera {\n> > > > > > > > \n> > > > > > > > @@ -155,6 +156,10 @@ struct IPAFrameContext : public FrameContext {\n> > > > > > > > uint32_t exposure;\n> > > > > > > > double gain;\n> > > > > > > > } sensor;\n> > > > > > > > +\n> > > > > > > > + struct {\n> > > > > > > > + Matrix ccm;\n> > > > > > > > + } ccm;\n> > > > > > > > };\n> > > > > > > > \n> > > > > > > > struct IPAContext {\n> > \n> > -- \n> > Regards,\n> > \n> > Laurent Pinchart","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 C8AFDC3237\n\tfor ;\n\tSun, 16 Jun 2024 17:15:22 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id BEAEB6548D;\n\tSun, 16 Jun 2024 19:15:21 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D5DBC65456\n\tfor ;\n\tSun, 16 Jun 2024 19:15:19 +0200 (CEST)","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 35EDA8CC;\n\tSun, 16 Jun 2024 19:15:03 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"PwsFOTLM\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1718558103;\n\tbh=wTuiBcai0oddt/b4m83+XAWkiomW7L9XC6GYvXegLMY=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=PwsFOTLMJi23vf+JIwSegIm03dwwLQJf/nChNX6fh/KzSy3goQhySTwI7C4ItVDjm\n\tRumSjNXctx96280RBxAZYqT6xxyBgKQfHslG39wvbrn6/PN/k5CVg/H4Ik0qorppXy\n\tjA+liIqiqe3Sb7wfXc5UwEre3yrTcgv5LPDhnlaw=","Date":"Sun, 16 Jun 2024 20:14:58 +0300","From":"Laurent Pinchart ","To":"Kieran Bingham ","Cc":"Paul Elder ,\n\tlibcamera-devel@lists.libcamera.org,\n\tStefan Klug ","Subject":"Re: [PATCH v7 3/3] ipa: rkisp1: algorithms: Add crosstalk algorithm","Message-ID":"<20240616171458.GA5954@pendragon.ideasonboard.com>","References":"<20240611140207.520083-1-paul.elder@ideasonboard.com>\n\t<20240611140207.520083-4-paul.elder@ideasonboard.com>\n\t<171811516483.1148289.15469360068204996564@ping.linuxembedded.co.uk>\n\t<20240611235120.GF15006@pendragon.ideasonboard.com>\n\t\n\t<20240612093834.GI28989@pendragon.ideasonboard.com>\n\t\n\t<20240613120453.GL6019@pendragon.ideasonboard.com>\n\t<171835323576.2248009.15136623245056611795@ping.linuxembedded.co.uk>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<171835323576.2248009.15136623245056611795@ping.linuxembedded.co.uk>","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\" "}}]