[{"id":29194,"web_url":"https://patchwork.libcamera.org/comment/29194/","msgid":"<20240410221113.zniovqe46j5dbu3j@macbook-air>","date":"2024-04-10T22:11:13","subject":"Re: [PATCH 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\nthank you for the patch.\n\nOn Fri, Apr 05, 2024 at 05:40:50PM +0900, Paul Elder wrote:\n> Add an algorithm module to the rkisp1 IPA for crosstalk correction.\n> \n> Signed-off-by: Paul Elder \n> ---\n> src/ipa/rkisp1/algorithms/ctk.cpp | 98 +++++++++++++++++++++++++++\n> src/ipa/rkisp1/algorithms/ctk.h | 41 +++++++++++\n> src/ipa/rkisp1/algorithms/meson.build | 1 +\n> 3 files changed, 140 insertions(+)\n> create mode 100644 src/ipa/rkisp1/algorithms/ctk.cpp\n> create mode 100644 src/ipa/rkisp1/algorithms/ctk.h\n> \n> diff --git a/src/ipa/rkisp1/algorithms/ctk.cpp b/src/ipa/rkisp1/algorithms/ctk.cpp\n> new file mode 100644\n> index 00000000..76f5da93\n> --- /dev/null\n> +++ b/src/ipa/rkisp1/algorithms/ctk.cpp\n> @@ -0,0 +1,98 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2024, Ideas On Board\n> + *\n> + * ctk.cpp - RkISP1 Cross Talk Correction control algorithm\n> + */\n> +\n> +#include \"ctk.h\"\n> +\n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +\n> +#include \n> +#include \n> +\n> +#include \n> +\n> +#include \"libcamera/internal/yaml_parser.h\"\n> +\n> +#include \"libipa/matrix_interpolator.h\"\n> +\n> +/**\n> + * \\file ctk.h\n> + */\n> +\n> +namespace libcamera {\n> +\n> +namespace ipa::rkisp1::algorithms {\n> +\n> +/**\n> + * \\class Ctk\n> + * \\brief A cross talk correction algorithm\n> + */\n\nMaybe it's just my bubble, but I wouldn't look for ctk, but for\nColorCorrection or CC in the algorithms. Maybe we should agree on one\nname libcamera wide (In rpi it's called ccm which feels natural to me\nalthough technically one could argue that the algorithm is only cc).\n\n> +\n> +LOG_DEFINE_CATEGORY(RkISP1Ctk)\n> +\n> +int Ctk::parseYaml(const YamlObject &tuningData, const char *prop)\n> +{\n> +\tint ret = 0;\n> +\n> +\tconst YamlObject &yaml = tuningData[prop];\n> +\tret = ccm_.readYaml(yaml);\n> +\tif (ret < 0) {\n> +\t\tLOG(RkISP1Ctk, Error)\n> +\t\t\t<< \"Failed to parse '\" << prop << \"' parameter from tuning file\";\n> +\t}\n\nIn the error case I would expect it to fallback to a unit matrix.\n\n> +\n> +\treturn ret;\n> +}\n> +\n> +/**\n> + * \\copydoc libcamera::ipa::Algorithm::init\n> + */\n> +int Ctk::init([[maybe_unused]] IPAContext &context, const YamlObject &tuningData)\n> +{\n> +\treturn parseYaml(tuningData, \"ctms\");\n\nThat's another name for the same thing. Why not ccms?\n\n> +}\n> +\n> +void Ctk::setParameters(rkisp1_params_cfg *params, const Matrix &matrix)\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] = static_cast(matrix.data[i * 3 + j]);\n\nI think the conversion to fixed point should be done here and the yaml\nshould contain floats, to keep the hardware specifics out of the yaml.\n\n> +\n> +\tLOG(RkISP1Ctk, Debug) << \"Setting matrix \" << matrix.toString();\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 Ctk::prepare(IPAContext &context, [[maybe_unused]] const uint32_t frame,\n> +\t\t [[maybe_unused]] IPAFrameContext &frameContext,\n> +\t\t rkisp1_params_cfg *params)\n> +{\n> +\tuint32_t ct = context.activeState.awb.temperatureK;\n> +\tMatrix ccm = ccm_.get(ct);\n> +\n> +\tsetParameters(params, ccm);\n> +}\n> +\n> +REGISTER_IPA_ALGORITHM(Ctk, \"Ctk\")\n> +\n> +} /* namespace ipa::rkisp1::algorithms */\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/ipa/rkisp1/algorithms/ctk.h b/src/ipa/rkisp1/algorithms/ctk.h\n> new file mode 100644\n> index 00000000..4f429df4\n> --- /dev/null\n> +++ b/src/ipa/rkisp1/algorithms/ctk.h\n> @@ -0,0 +1,41 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2024, Ideas On Board\n> + *\n> + * ctk.h - RkISP1 Cross Talk Correction 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 Ctk : public Algorithm\n> +{\n> +public:\n> +\tCtk(){};\n> +\t~Ctk() = 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> +\n> +private:\n> +\tint parseYaml(const YamlObject &tuningData, const char *prop);\n> +\tvoid setParameters(rkisp1_params_cfg *params, const Matrix &matrix);\n> +\n> +\tMatrixInterpolator ccm_;\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 93a48329..c9891e87 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> + 'ctk.cpp',\n\nThis should be sorted.\n\nBest regards,\nStefan\n\n> 'cproc.cpp',\n> 'dpcc.cpp',\n> 'dpf.cpp',\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 D210DC0DA4\n\tfor ;\n\tWed, 10 Apr 2024 22:11:24 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0A7F163352;\n\tThu, 11 Apr 2024 00:11:24 +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 C29B063339\n\tfor ;\n\tThu, 11 Apr 2024 00:11:21 +0200 (CEST)","from ideasonboard.com\n\t(p200300cd3f2c2e00d7b53aefac1fc2ef.dip0.t-ipconnect.de\n\t[IPv6:2003:cd:3f2c:2e00:d7b5:3aef:ac1f:c2ef])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 1EF5E673;\n\tThu, 11 Apr 2024 00:10:37 +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=\"Sw32+0NX\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1712787039;\n\tbh=RfpOcmMPX7wzh1538canLXL5UDLtAC58dL4cRGE2nD4=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Sw32+0NXmIByj8U0AnBEulh0dZjcGuk4dORaAKzrVytJJwnwtXoaf9OWfyBSr4PUu\n\tU6CinUgFd2xUSPNAuMbQNYh20aSAsyeXUEXAcnFzqrks/QY9/l10emzg2KkUYWWjQ9\n\tx7Nv7Rljyi5KXg+/2gAM4JG3aQi/I3kTnmgpJ/o0=","Date":"Thu, 11 Apr 2024 00:11:13 +0200","From":"Stefan Klug ","To":"Paul Elder ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH 3/3] ipa: rkisp1: algorithms: Add crosstalk algorithm","Message-ID":"<20240410221113.zniovqe46j5dbu3j@macbook-air>","References":"<20240405084050.1919105-1-paul.elder@ideasonboard.com>\n\t<20240405084050.1919105-4-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20240405084050.1919105-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":29195,"web_url":"https://patchwork.libcamera.org/comment/29195/","msgid":"<171278842774.3165863.688526289970483570@ping.linuxembedded.co.uk>","date":"2024-04-10T22:33:47","subject":"Re: [PATCH 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 Stefan Klug (2024-04-10 23:11:13)\n> Hi Paul,\n> \n> thank you for the patch.\n> \n> On Fri, Apr 05, 2024 at 05:40:50PM +0900, Paul Elder wrote:\n> > Add an algorithm module to the rkisp1 IPA for crosstalk correction.\n> > \n> > Signed-off-by: Paul Elder \n> > ---\n> > src/ipa/rkisp1/algorithms/ctk.cpp | 98 +++++++++++++++++++++++++++\n> > src/ipa/rkisp1/algorithms/ctk.h | 41 +++++++++++\n> > src/ipa/rkisp1/algorithms/meson.build | 1 +\n> > 3 files changed, 140 insertions(+)\n> > create mode 100644 src/ipa/rkisp1/algorithms/ctk.cpp\n> > create mode 100644 src/ipa/rkisp1/algorithms/ctk.h\n> > \n> > diff --git a/src/ipa/rkisp1/algorithms/ctk.cpp b/src/ipa/rkisp1/algorithms/ctk.cpp\n> > new file mode 100644\n> > index 00000000..76f5da93\n> > --- /dev/null\n> > +++ b/src/ipa/rkisp1/algorithms/ctk.cpp\n> > @@ -0,0 +1,98 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2024, Ideas On Board\n> > + *\n> > + * ctk.cpp - RkISP1 Cross Talk Correction control algorithm\n> > + */\n> > +\n> > +#include \"ctk.h\"\n> > +\n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +\n> > +#include \n> > +#include \n> > +\n> > +#include \n> > +\n> > +#include \"libcamera/internal/yaml_parser.h\"\n> > +\n> > +#include \"libipa/matrix_interpolator.h\"\n> > +\n> > +/**\n> > + * \\file ctk.h\n> > + */\n> > +\n> > +namespace libcamera {\n> > +\n> > +namespace ipa::rkisp1::algorithms {\n> > +\n> > +/**\n> > + * \\class Ctk\n> > + * \\brief A cross talk correction algorithm\n> > + */\n> \n> Maybe it's just my bubble, but I wouldn't look for ctk, but for\n> ColorCorrection or CC in the algorithms. Maybe we should agree on one\n> name libcamera wide (In rpi it's called ccm which feels natural to me\n> although technically one could argue that the algorithm is only cc).\n> \n> > +\n> > +LOG_DEFINE_CATEGORY(RkISP1Ctk)\n> > +\n> > +int Ctk::parseYaml(const YamlObject &tuningData, const char *prop)\n> > +{\n> > + int ret = 0;\n> > +\n> > + const YamlObject &yaml = tuningData[prop];\n> > + ret = ccm_.readYaml(yaml);\n> > + if (ret < 0) {\n> > + LOG(RkISP1Ctk, Error)\n> > + << \"Failed to parse '\" << prop << \"' parameter from tuning file\";\n> > + }\n> \n> In the error case I would expect it to fallback to a unit matrix.\n\nI think this was probably intended to be called the 'identity' matrix.\nBut I think I agree here, as we should expect without any better\ninformation to just pass through the data in this component.\n\nI don't think we need the braces around the LOG either...\n\n> \n> > +\n> > + return ret;\n> > +}\n> > +\n> > +/**\n> > + * \\copydoc libcamera::ipa::Algorithm::init\n> > + */\n> > +int Ctk::init([[maybe_unused]] IPAContext &context, const YamlObject &tuningData)\n> > +{\n> > + return parseYaml(tuningData, \"ctms\");\n> \n> That's another name for the same thing. Why not ccms?\n\nExcept the isp structure is called rkisp1_cif_isp_ctk_config below which\nwould lead me to believe this should be reading in the 'ctks' ? But\nthey're not crosstalks ... they color correction matrices ... eeek.\n\nTough to name things, do we name it to match the component we're\npopulating (ctk), or the type of data we're filling it with (ccm)\n\n> > +}\n> > +\n> > +void Ctk::setParameters(rkisp1_params_cfg *params, const Matrix &matrix)\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] = static_cast(matrix.data[i * 3 + j]);\n> \n> I think the conversion to fixed point should be done here and the yaml\n> should contain floats, to keep the hardware specifics out of the yaml.\n\nI think we've discussed this too and already agreed on this, so indeed I\nagree here.\n\n> > + LOG(RkISP1Ctk, Debug) << \"Setting matrix \" << matrix.toString();\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 Ctk::prepare(IPAContext &context, [[maybe_unused]] const uint32_t frame,\n> > + [[maybe_unused]] IPAFrameContext &frameContext,\n> > + rkisp1_params_cfg *params)\n> > +{\n> > + uint32_t ct = context.activeState.awb.temperatureK;\n> > + Matrix ccm = ccm_.get(ct);\n\nWow. I thought this class wasn't doing anything, but this is actaully\n/awesome/ and shows how much code is factored into the helpers.\n\nI might bikeshed and wonder if this should be 'ccm_.interpolate(ct);' to\nbe clearer on what will happen, but I don't really care. It should be\nclear that the ccms are stored in an interpolater ;-)\n\nI wonder if we should cache the interpolation though so that we don't\nrecalculate/reinterpolate for every frame, and instead only\nre-interpolate if the ct changes by more than ... X ?\n\n--\nKieran\n\n\n\n> > +\n> > + setParameters(params, ccm);\n> > +}\n> > +\n> > +REGISTER_IPA_ALGORITHM(Ctk, \"Ctk\")\n> > +\n> > +} /* namespace ipa::rkisp1::algorithms */\n> > +\n> > +} /* namespace libcamera */\n> > diff --git a/src/ipa/rkisp1/algorithms/ctk.h b/src/ipa/rkisp1/algorithms/ctk.h\n> > new file mode 100644\n> > index 00000000..4f429df4\n> > --- /dev/null\n> > +++ b/src/ipa/rkisp1/algorithms/ctk.h\n> > @@ -0,0 +1,41 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2024, Ideas On Board\n> > + *\n> > + * ctk.h - RkISP1 Cross Talk Correction 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 Ctk : public Algorithm\n> > +{\n> > +public:\n> > + Ctk(){};\n> > + ~Ctk() = 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> > +\n> > +private:\n> > + int parseYaml(const YamlObject &tuningData, const char *prop);\n> > + void setParameters(rkisp1_params_cfg *params, const Matrix &matrix);\n> > +\n> > + MatrixInterpolator ccm_;\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 93a48329..c9891e87 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> > + 'ctk.cpp',\n> \n> This should be sorted.\n> \n> Best regards,\n> Stefan\n> \n> > 'cproc.cpp',\n> > 'dpcc.cpp',\n> > 'dpf.cpp',\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 65F12BE08B\n\tfor ;\n\tWed, 10 Apr 2024 22:33:53 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 93DC36333B;\n\tThu, 11 Apr 2024 00:33:52 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 31C0563339\n\tfor ;\n\tThu, 11 Apr 2024 00:33:51 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(aztw-30-b2-v4wan-166917-cust845.vm26.cable.virginm.net\n\t[82.37.23.78])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 99910673;\n\tThu, 11 Apr 2024 00:33:08 +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=\"VapX/01C\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1712788388;\n\tbh=nLWoOItZc+uWW+mjk7NIDcBtOluNAMpJuXM1SDrfxDY=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=VapX/01CKkM6UQryvnOoc7NwlATTr+GamERF0o1eubt+W3s5mWD800IYPluFOxqAE\n\tbMUsaNHVCiDsZhAsXdag/L/Z0w+8uQiyZZR28voWA/p9haQe8w/OZJbJEBjPbwSSwB\n\tMhf1Rprse6Q4vk8GMAGrJhSxBsPKyYtsIx8tsuuc=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20240410221113.zniovqe46j5dbu3j@macbook-air>","References":"<20240405084050.1919105-1-paul.elder@ideasonboard.com>\n\t<20240405084050.1919105-4-paul.elder@ideasonboard.com>\n\t<20240410221113.zniovqe46j5dbu3j@macbook-air>","Subject":"Re: [PATCH 3/3] ipa: rkisp1: algorithms: Add crosstalk algorithm","From":"Kieran Bingham ","Cc":"libcamera-devel@lists.libcamera.org","To":"Paul Elder ,\n\tStefan Klug ","Date":"Wed, 10 Apr 2024 23:33:47 +0100","Message-ID":"<171278842774.3165863.688526289970483570@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\" "}}]