[{"id":29960,"web_url":"https://patchwork.libcamera.org/comment/29960/","msgid":"<20240616173516.GB7378@pendragon.ideasonboard.com>","date":"2024-06-16T17:35:16","subject":"Re: [PATCH v9 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 Fri, Jun 14, 2024 at 08:37:57PM +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> Reviewed-by: Kieran Bingham \n> \n> ---\n> Changes in v9:\n> - compilation fixes\n> \n> Changes in v8:\n> - cleanups\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 | 142 ++++++++++++++++++++++++++\n> src/ipa/rkisp1/algorithms/ccm.h | 49 +++++++++\n> src/ipa/rkisp1/algorithms/meson.build | 1 +\n> src/ipa/rkisp1/ipa_context.h | 5 +\n> 4 files changed, 197 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..2690bd699e41\n> --- /dev/null\n> +++ b/src/ipa/rkisp1/algorithms/ccm.cpp\n> @@ -0,0 +1,142 @@\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> +\t}\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, 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 (frame > 0 && ct == ct_)\n> +\t\treturn;\n\nRepeating a comment from v7: I expect temperatureK to be fairly noisy,\nshould we add a threshold to the comparison to avoid updating the CCM\nparameters on every frame in practice ?\n\nIt can be done on top if we add a \\todo comment here:\n\n\tuint32_t ct = context.activeState.awb.temperatureK;\n\n\t/*\n\t * \\todo The colour temperature will likely be noisy, add filtering to\n\t * avoid updating the CCM matrix all the time.\n\t */\n\tif (frame > 0 && ct == ct_)\n\t\treturn;\n\nWith that,\n\nReviewed-by: Laurent Pinchart \n\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 Ccm::process([[maybe_unused]] IPAContext &context,\n> +\t\t [[maybe_unused]] const uint32_t frame,\n> +\t\t IPAFrameContext &frameContext,\n> +\t\t [[maybe_unused]] const rkisp1_stat_buffer *stats,\n> +\t\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> +\t}\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..30cb882180cc\n> --- /dev/null\n> +++ b/src/ipa/rkisp1/algorithms/ccm.h\n> @@ -0,0 +1,49 @@\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(IPAContext &context, const uint32_t frame,\n> +\t\t IPAFrameContext &frameContext,\n> +\t\t 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 6022480d4fd2..4f01a7f8ed33 100644\n> --- a/src/ipa/rkisp1/ipa_context.h\n> +++ b/src/ipa/rkisp1/ipa_context.h\n> @@ -17,6 +17,7 @@\n> #include \n> \n> #include \n> +#include \n> \n> namespace libcamera {\n> \n> @@ -163,6 +164,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 {","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 44E33BD87C\n\tfor ;\n\tSun, 16 Jun 2024 17:35:40 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 740ED65456;\n\tSun, 16 Jun 2024 19:35: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 9967965456\n\tfor ;\n\tSun, 16 Jun 2024 19:35:37 +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 173832D5;\n\tSun, 16 Jun 2024 19:35:21 +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=\"r75IPJsF\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1718559321;\n\tbh=P1K4ZsoVpFbQ/EeTPSR1AA2D88klb5w2x7bKkhr+gjU=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=r75IPJsFwNBDjvHZM8NrY2KJpJj8Y2I1agfUF+qPe5ncoumhl3OR13/Js7IJvjvGp\n\t7YBK2Ur6nOyPzXUFsef+ZeoNViilINiqwKHf13XhLpR62EaPyuYAmKj0WVotVmxFQ1\n\t0u8AWso6iOCyQ2JfSRGRxju/D/F+dSxigGxfIZCU=","Date":"Sun, 16 Jun 2024 20:35:16 +0300","From":"Laurent Pinchart ","To":"Paul Elder ","Cc":"libcamera-devel@lists.libcamera.org,\n\tStefan Klug ,\n\tKieran Bingham ","Subject":"Re: [PATCH v9 3/3] ipa: rkisp1: algorithms: Add crosstalk algorithm","Message-ID":"<20240616173516.GB7378@pendragon.ideasonboard.com>","References":"<20240614113757.109452-1-paul.elder@ideasonboard.com>\n\t<20240614113757.109452-4-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20240614113757.109452-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\" "}}]