[{"id":33798,"web_url":"https://patchwork.libcamera.org/comment/33798/","msgid":"<174344278820.3394313.4649916776360845530@ping.linuxembedded.co.uk>","date":"2025-03-31T17:39:48","subject":"Re: [PATCH v2 11/17] ipa: rkisp1: Implement manual\n\tColourCorrectionMatrix control","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Stefan Klug (2025-03-19 16:11:16)\n> Add a manual ColourCorrectionMatrix control. This was already discussed\n> while implementing manual colour temperature but was never implemented.\n> The control allows to manually specify the CCM when AwbEnable is false.\n> \n> Signed-off-by: Stefan Klug \n> \n> ---\n> \n> Changes in v2:\n> - None\n> ---\n> src/ipa/rkisp1/algorithms/ccm.cpp | 59 ++++++++++++++++++++++++++++---\n> src/ipa/rkisp1/algorithms/ccm.h | 6 ++++\n> src/ipa/rkisp1/ipa_context.h | 3 +-\n> 3 files changed, 62 insertions(+), 6 deletions(-)\n> \n> diff --git a/src/ipa/rkisp1/algorithms/ccm.cpp b/src/ipa/rkisp1/algorithms/ccm.cpp\n> index 2e5e91006b55..303ac3dd2fe2 100644\n> --- a/src/ipa/rkisp1/algorithms/ccm.cpp\n> +++ b/src/ipa/rkisp1/algorithms/ccm.cpp\n> @@ -36,17 +36,25 @@ namespace ipa::rkisp1::algorithms {\n> \n> LOG_DEFINE_CATEGORY(RkISP1Ccm)\n> \n> +constexpr Matrix kIdentity3x3 = Matrix::identity();\n> +\n> /**\n> * \\copydoc libcamera::ipa::Algorithm::init\n> */\n> int Ccm::init([[maybe_unused]] IPAContext &context, const YamlObject &tuningData)\n> {\n> + auto &cmap = context.ctrlMap;\n> + cmap[&controls::ColourCorrectionMatrix] = ControlInfo(\n> + ControlValue(-8.0f),\n> + ControlValue(7.993f),\n> + ControlValue(kIdentity3x3.data()));\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_.setData({ { 0, Matrix::identity() } });\n> + ccm_.setData({ { 0, kIdentity3x3 } });\n> }\n> \n> ret = offsets_.readYaml(tuningData[\"ccms\"], \"ct\", \"offsets\");\n> @@ -61,13 +69,48 @@ int Ccm::init([[maybe_unused]] IPAContext &context, const YamlObject &tuningData\n> return 0;\n> }\n> \n> +/**\n> + * \\copydoc libcamera::ipa::Algorithm::configure\n> + */\n> +int Ccm::configure(IPAContext &context,\n> + [[maybe_unused]] const IPACameraSensorInfo &configInfo)\n> +{\n> + auto &as = context.activeState;\n> + as.ccm.manual = kIdentity3x3;\n> + as.ccm.automatic = ccm_.getInterpolated(as.awb.automatic.temperatureK);\n> + LOG(RkISP1Ccm, Debug) << \"init matrix \" << as.ccm.manual;\n\nI'm not sure that debug line adds much value...\n\nBut aside from that, it seems to match the control documentation.\n\n\nReviewed-by: Kieran Bingham \n\n\n> + return 0;\n> +}\n> +\n> +void Ccm::queueRequest(IPAContext &context,\n> + [[maybe_unused]] const uint32_t frame,\n> + IPAFrameContext &frameContext,\n> + const ControlList &controls)\n> +{\n> + /* Nothing to do here, the ccm will be calculated in prepare() */\n> + if (frameContext.awb.autoEnabled)\n> + return;\n> +\n> + auto &ccm = context.activeState.ccm;\n> +\n> + const auto &colourTemperature = controls.get(controls::ColourTemperature);\n> + const auto &ccmMatrix = controls.get(controls::ColourCorrectionMatrix);\n> + if (ccmMatrix)\n> + ccm.manual = Matrix(*ccmMatrix);\n> + else if (colourTemperature)\n> + ccm.manual = ccm_.getInterpolated(*colourTemperature);\n> +\n> + LOG(RkISP1Ccm, Debug) << \"queueRequest matrix \" << ccm.manual;\n> + frameContext.ccm.ccm = ccm.manual;\n> +}\n> +\n> void Ccm::setParameters(struct rkisp1_cif_isp_ctk_config &config,\n> const Matrix &matrix,\n> const Matrix &offsets)\n> {\n> /*\n> * 4 bit integer and 7 bit fractional, ranging from -8 (0x400) to\n> - * +7.992 (0x3ff)\n> + * +7.9921875 (0x3ff)\n> */\n> for (unsigned int i = 0; i < 3; i++) {\n> for (unsigned int j = 0; j < 3; j++)\n> @@ -88,14 +131,20 @@ void Ccm::setParameters(struct rkisp1_cif_isp_ctk_config &config,\n> void Ccm::prepare(IPAContext &context, const uint32_t frame,\n> IPAFrameContext &frameContext, RkISP1Params *params)\n> {\n> - uint32_t ct = frameContext.awb.temperatureK;\n> + if (!frameContext.awb.autoEnabled) {\n> + auto config = params->block();\n> + config.setEnabled(true);\n> + setParameters(*config, frameContext.ccm.ccm, Matrix());\n> + return;\n> + }\n> \n> + uint32_t ct = frameContext.awb.temperatureK;\n> /*\n> * \\todo The colour temperature will likely be noisy, add filtering to\n> * avoid updating the CCM matrix all the time.\n> */\n> if (frame > 0 && ct == ct_) {\n> - frameContext.ccm.ccm = context.activeState.ccm.ccm;\n> + frameContext.ccm.ccm = context.activeState.ccm.automatic;\n> return;\n> }\n> \n> @@ -103,7 +152,7 @@ void Ccm::prepare(IPAContext &context, const uint32_t frame,\n> Matrix ccm = ccm_.getInterpolated(ct);\n> Matrix offsets = offsets_.getInterpolated(ct);\n> \n> - context.activeState.ccm.ccm = ccm;\n> + context.activeState.ccm.automatic = ccm;\n> frameContext.ccm.ccm = ccm;\n> \n> auto config = params->block();\n> diff --git a/src/ipa/rkisp1/algorithms/ccm.h b/src/ipa/rkisp1/algorithms/ccm.h\n> index a5d9a9a45e5d..c301e6e531c8 100644\n> --- a/src/ipa/rkisp1/algorithms/ccm.h\n> +++ b/src/ipa/rkisp1/algorithms/ccm.h\n> @@ -26,6 +26,12 @@ public:\n> ~Ccm() = default;\n> \n> int init(IPAContext &context, const YamlObject &tuningData) override;\n> + int configure(IPAContext &context,\n> + const IPACameraSensorInfo &configInfo) override;\n> + void queueRequest(IPAContext &context,\n> + const uint32_t frame,\n> + IPAFrameContext &frameContext,\n> + const ControlList &controls) override;\n> void prepare(IPAContext &context, const uint32_t frame,\n> IPAFrameContext &frameContext,\n> RkISP1Params *params) override;\n> diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h\n> index 769e9f114e23..f0d504215d34 100644\n> --- a/src/ipa/rkisp1/ipa_context.h\n> +++ b/src/ipa/rkisp1/ipa_context.h\n> @@ -101,7 +101,8 @@ struct IPAActiveState {\n> } awb;\n> \n> struct {\n> - Matrix ccm;\n> + Matrix manual;\n> + Matrix automatic;\n> } ccm;\n> \n> struct {\n> -- \n> 2.43.0\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 442AAC3213\n\tfor ;\n\tMon, 31 Mar 2025 17:39:54 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 69DE668986;\n\tMon, 31 Mar 2025 19:39:53 +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 9C59268967\n\tfor ;\n\tMon, 31 Mar 2025 19:39:51 +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 C19CF703;\n\tMon, 31 Mar 2025 19:37:59 +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=\"Xk7+zC0B\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1743442679;\n\tbh=JgoaSSwy/JLJ2/2YnvZlqYsu//hM5J5V49td0hhV5N8=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=Xk7+zC0BnV6OwyXyUHvvf3gHYYuujqG31p735B5UW2orWjtxoSi3p+KbluC838KuV\n\tjN7jE7T38ypdmJLyMvo7quKy5IvJHk1/jmx4qPZmx4BuNTMrN4f1eoSOcAfug8Torp\n\t2xwTnEvlQblig+FZNE/K1hHe5n082yJECJGRmMNo=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20250319161152.63625-12-stefan.klug@ideasonboard.com>","References":"<20250319161152.63625-1-stefan.klug@ideasonboard.com>\n\t<20250319161152.63625-12-stefan.klug@ideasonboard.com>","Subject":"Re: [PATCH v2 11/17] ipa: rkisp1: Implement manual\n\tColourCorrectionMatrix control","From":"Kieran Bingham ","Cc":"Stefan Klug ","To":"Stefan Klug ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Mon, 31 Mar 2025 18:39:48 +0100","Message-ID":"<174344278820.3394313.4649916776360845530@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":33866,"web_url":"https://patchwork.libcamera.org/comment/33866/","msgid":"<20250401213005.GE3494@pendragon.ideasonboard.com>","date":"2025-04-01T21:30:05","subject":"Re: [PATCH v2 11/17] ipa: rkisp1: Implement manual\n\tColourCorrectionMatrix control","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Mon, Mar 31, 2025 at 06:39:48PM +0100, Kieran Bingham wrote:\n> Quoting Stefan Klug (2025-03-19 16:11:16)\n> > Add a manual ColourCorrectionMatrix control. This was already discussed\n> > while implementing manual colour temperature but was never implemented.\n> > The control allows to manually specify the CCM when AwbEnable is false.\n> > \n> > Signed-off-by: Stefan Klug \n> > \n> > ---\n> > \n> > Changes in v2:\n> > - None\n> > ---\n> > src/ipa/rkisp1/algorithms/ccm.cpp | 59 ++++++++++++++++++++++++++++---\n> > src/ipa/rkisp1/algorithms/ccm.h | 6 ++++\n> > src/ipa/rkisp1/ipa_context.h | 3 +-\n> > 3 files changed, 62 insertions(+), 6 deletions(-)\n> > \n> > diff --git a/src/ipa/rkisp1/algorithms/ccm.cpp b/src/ipa/rkisp1/algorithms/ccm.cpp\n> > index 2e5e91006b55..303ac3dd2fe2 100644\n> > --- a/src/ipa/rkisp1/algorithms/ccm.cpp\n> > +++ b/src/ipa/rkisp1/algorithms/ccm.cpp\n> > @@ -36,17 +36,25 @@ namespace ipa::rkisp1::algorithms {\n> > \n> > LOG_DEFINE_CATEGORY(RkISP1Ccm)\n> > \n> > +constexpr Matrix kIdentity3x3 = Matrix::identity();\n> > +\n> > /**\n> > * \\copydoc libcamera::ipa::Algorithm::init\n> > */\n> > int Ccm::init([[maybe_unused]] IPAContext &context, const YamlObject &tuningData)\n> > {\n> > + auto &cmap = context.ctrlMap;\n> > + cmap[&controls::ColourCorrectionMatrix] = ControlInfo(\n> > + ControlValue(-8.0f),\n> > + ControlValue(7.993f),\n> > + ControlValue(kIdentity3x3.data()));\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_.setData({ { 0, Matrix::identity() } });\n> > + ccm_.setData({ { 0, kIdentity3x3 } });\n> > }\n> > \n> > ret = offsets_.readYaml(tuningData[\"ccms\"], \"ct\", \"offsets\");\n> > @@ -61,13 +69,48 @@ int Ccm::init([[maybe_unused]] IPAContext &context, const YamlObject &tuningData\n> > return 0;\n> > }\n> > \n> > +/**\n> > + * \\copydoc libcamera::ipa::Algorithm::configure\n> > + */\n> > +int Ccm::configure(IPAContext &context,\n> > + [[maybe_unused]] const IPACameraSensorInfo &configInfo)\n> > +{\n> > + auto &as = context.activeState;\n> > + as.ccm.manual = kIdentity3x3;\n> > + as.ccm.automatic = ccm_.getInterpolated(as.awb.automatic.temperatureK);\n> > + LOG(RkISP1Ccm, Debug) << \"init matrix \" << as.ccm.manual;\n> \n> I'm not sure that debug line adds much value...\n> \n> But aside from that, it seems to match the control documentation.\n> \n> \n> Reviewed-by: Kieran Bingham \n> \n> \n> > + return 0;\n> > +}\n> > +\n> > +void Ccm::queueRequest(IPAContext &context,\n> > + [[maybe_unused]] const uint32_t frame,\n> > + IPAFrameContext &frameContext,\n> > + const ControlList &controls)\n> > +{\n> > + /* Nothing to do here, the ccm will be calculated in prepare() */\n> > + if (frameContext.awb.autoEnabled)\n> > + return;\n> > +\n> > + auto &ccm = context.activeState.ccm;\n> > +\n> > + const auto &colourTemperature = controls.get(controls::ColourTemperature);\n> > + const auto &ccmMatrix = controls.get(controls::ColourCorrectionMatrix);\n> > + if (ccmMatrix)\n> > + ccm.manual = Matrix(*ccmMatrix);\n> > + else if (colourTemperature)\n> > + ccm.manual = ccm_.getInterpolated(*colourTemperature);\n> > +\n> > + LOG(RkISP1Ccm, Debug) << \"queueRequest matrix \" << ccm.manual;\n\nTo make the log less noisy, should this be restricted to when a CT or\nCCM matrix control was set ? Or maybe also indicate where the CCM came\nfrom ?\n\n\tif (ccmMatrix) {\n\t\tccm.manual = Matrix(*ccmMatrix);\n \t\tLOG(RkISP1Ccm, Debug)\n\t\t\t<< \"Setting manual CCM from CCM control to \" << ccm.manual;\n\t} else if (colourTemperature) {\n\t\tccm.manual = ccm_.getInterpolated(*colourTemperature);\n \t\tLOG(RkISP1Ccm, Debug)\n\t\t\t<< \"Setting manual CCM from CT control to \" << ccm.manual;\n\t}\n\nor something similar.\n\n> > + frameContext.ccm.ccm = ccm.manual;\n> > +}\n> > +\n> > void Ccm::setParameters(struct rkisp1_cif_isp_ctk_config &config,\n> > const Matrix &matrix,\n> > const Matrix &offsets)\n> > {\n> > /*\n> > * 4 bit integer and 7 bit fractional, ranging from -8 (0x400) to\n> > - * +7.992 (0x3ff)\n> > + * +7.9921875 (0x3ff)\n\nSomeone likes precision :-)\n\n> > */\n> > for (unsigned int i = 0; i < 3; i++) {\n> > for (unsigned int j = 0; j < 3; j++)\n> > @@ -88,14 +131,20 @@ void Ccm::setParameters(struct rkisp1_cif_isp_ctk_config &config,\n> > void Ccm::prepare(IPAContext &context, const uint32_t frame,\n> > IPAFrameContext &frameContext, RkISP1Params *params)\n> > {\n> > - uint32_t ct = frameContext.awb.temperatureK;\n> > + if (!frameContext.awb.autoEnabled) {\n> > + auto config = params->block();\n> > + config.setEnabled(true);\n> > + setParameters(*config, frameContext.ccm.ccm, Matrix());\n> > + return;\n> > + }\n> > \n> > + uint32_t ct = frameContext.awb.temperatureK;\n> > /*\n> > * \\todo The colour temperature will likely be noisy, add filtering to\n> > * avoid updating the CCM matrix all the time.\n> > */\n> > if (frame > 0 && ct == ct_) {\n> > - frameContext.ccm.ccm = context.activeState.ccm.ccm;\n> > + frameContext.ccm.ccm = context.activeState.ccm.automatic;\n> > return;\n> > }\n> > \n> > @@ -103,7 +152,7 @@ void Ccm::prepare(IPAContext &context, const uint32_t frame,\n> > Matrix ccm = ccm_.getInterpolated(ct);\n> > Matrix offsets = offsets_.getInterpolated(ct);\n> > \n> > - context.activeState.ccm.ccm = ccm;\n> > + context.activeState.ccm.automatic = ccm;\n> > frameContext.ccm.ccm = ccm;\n> > \n> > auto config = params->block();\n> > diff --git a/src/ipa/rkisp1/algorithms/ccm.h b/src/ipa/rkisp1/algorithms/ccm.h\n> > index a5d9a9a45e5d..c301e6e531c8 100644\n> > --- a/src/ipa/rkisp1/algorithms/ccm.h\n> > +++ b/src/ipa/rkisp1/algorithms/ccm.h\n> > @@ -26,6 +26,12 @@ public:\n> > ~Ccm() = default;\n> > \n> > int init(IPAContext &context, const YamlObject &tuningData) override;\n> > + int configure(IPAContext &context,\n> > + const IPACameraSensorInfo &configInfo) override;\n> > + void queueRequest(IPAContext &context,\n> > + const uint32_t frame,\n> > + IPAFrameContext &frameContext,\n> > + const ControlList &controls) override;\n> > void prepare(IPAContext &context, const uint32_t frame,\n> > IPAFrameContext &frameContext,\n> > RkISP1Params *params) override;\n> > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h\n> > index 769e9f114e23..f0d504215d34 100644\n> > --- a/src/ipa/rkisp1/ipa_context.h\n> > +++ b/src/ipa/rkisp1/ipa_context.h\n> > @@ -101,7 +101,8 @@ struct IPAActiveState {\n> > } awb;\n> > \n> > struct {\n> > - Matrix ccm;\n> > + Matrix manual;\n> > + Matrix automatic;\n\nMissing documentation updates.\n\n> > } ccm;\n> > \n> > struct {","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 6C6FCC323E\n\tfor ;\n\tTue, 1 Apr 2025 21:30:33 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B31F468981;\n\tTue, 1 Apr 2025 23:30:32 +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 0828968947\n\tfor ;\n\tTue, 1 Apr 2025 23:30:31 +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 439CA741;\n\tTue, 1 Apr 2025 23:28:38 +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=\"SoMvQ7O8\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1743542918;\n\tbh=NzDvfgTeAwzdYKt196LES+Pp0BfQuVxPiW/Iuqn+WRM=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=SoMvQ7O8i+Nvt1vuXfTSOyK1miM3IkZGMKfoy7UL91fzOWCzigRHjq88yWNK3Te6z\n\t8ATpa0j58Mk88RKOnAIIJlQWaX55UP3xufiEdHpVmpGB3LqgsNtndR7iDwby1SfLxf\n\tQuFr9yx51sJPij1NqmEpLc4emkjrqMoWFvoiIulY=","Date":"Wed, 2 Apr 2025 00:30:05 +0300","From":"Laurent Pinchart ","To":"Kieran Bingham ","Cc":"Stefan Klug ,\n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v2 11/17] ipa: rkisp1: Implement manual\n\tColourCorrectionMatrix control","Message-ID":"<20250401213005.GE3494@pendragon.ideasonboard.com>","References":"<20250319161152.63625-1-stefan.klug@ideasonboard.com>\n\t<20250319161152.63625-12-stefan.klug@ideasonboard.com>\n\t<174344278820.3394313.4649916776360845530@ping.linuxembedded.co.uk>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<174344278820.3394313.4649916776360845530@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":33890,"web_url":"https://patchwork.libcamera.org/comment/33890/","msgid":"","date":"2025-04-02T14:45:34","subject":"Re: [PATCH v2 11/17] ipa: rkisp1: Implement manual\n\tColourCorrectionMatrix control","submitter":{"id":184,"url":"https://patchwork.libcamera.org/api/people/184/","name":"Stefan Klug","email":"stefan.klug@ideasonboard.com"},"content":"Hi Laurent,\n\nThank you for the review. \n\nOn Wed, Apr 02, 2025 at 12:30:05AM +0300, Laurent Pinchart wrote:\n> On Mon, Mar 31, 2025 at 06:39:48PM +0100, Kieran Bingham wrote:\n> > Quoting Stefan Klug (2025-03-19 16:11:16)\n> > > Add a manual ColourCorrectionMatrix control. This was already discussed\n> > > while implementing manual colour temperature but was never implemented.\n> > > The control allows to manually specify the CCM when AwbEnable is false.\n> > > \n> > > Signed-off-by: Stefan Klug \n> > > \n> > > ---\n> > > \n> > > Changes in v2:\n> > > - None\n> > > ---\n> > > src/ipa/rkisp1/algorithms/ccm.cpp | 59 ++++++++++++++++++++++++++++---\n> > > src/ipa/rkisp1/algorithms/ccm.h | 6 ++++\n> > > src/ipa/rkisp1/ipa_context.h | 3 +-\n> > > 3 files changed, 62 insertions(+), 6 deletions(-)\n> > > \n> > > diff --git a/src/ipa/rkisp1/algorithms/ccm.cpp b/src/ipa/rkisp1/algorithms/ccm.cpp\n> > > index 2e5e91006b55..303ac3dd2fe2 100644\n> > > --- a/src/ipa/rkisp1/algorithms/ccm.cpp\n> > > +++ b/src/ipa/rkisp1/algorithms/ccm.cpp\n> > > @@ -36,17 +36,25 @@ namespace ipa::rkisp1::algorithms {\n> > > \n> > > LOG_DEFINE_CATEGORY(RkISP1Ccm)\n> > > \n> > > +constexpr Matrix kIdentity3x3 = Matrix::identity();\n> > > +\n> > > /**\n> > > * \\copydoc libcamera::ipa::Algorithm::init\n> > > */\n> > > int Ccm::init([[maybe_unused]] IPAContext &context, const YamlObject &tuningData)\n> > > {\n> > > + auto &cmap = context.ctrlMap;\n> > > + cmap[&controls::ColourCorrectionMatrix] = ControlInfo(\n> > > + ControlValue(-8.0f),\n> > > + ControlValue(7.993f),\n> > > + ControlValue(kIdentity3x3.data()));\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_.setData({ { 0, Matrix::identity() } });\n> > > + ccm_.setData({ { 0, kIdentity3x3 } });\n> > > }\n> > > \n> > > ret = offsets_.readYaml(tuningData[\"ccms\"], \"ct\", \"offsets\");\n> > > @@ -61,13 +69,48 @@ int Ccm::init([[maybe_unused]] IPAContext &context, const YamlObject &tuningData\n> > > return 0;\n> > > }\n> > > \n> > > +/**\n> > > + * \\copydoc libcamera::ipa::Algorithm::configure\n> > > + */\n> > > +int Ccm::configure(IPAContext &context,\n> > > + [[maybe_unused]] const IPACameraSensorInfo &configInfo)\n> > > +{\n> > > + auto &as = context.activeState;\n> > > + as.ccm.manual = kIdentity3x3;\n> > > + as.ccm.automatic = ccm_.getInterpolated(as.awb.automatic.temperatureK);\n> > > + LOG(RkISP1Ccm, Debug) << \"init matrix \" << as.ccm.manual;\n> > \n> > I'm not sure that debug line adds much value...\n> > \n> > But aside from that, it seems to match the control documentation.\n> > \n> > \n> > Reviewed-by: Kieran Bingham \n> > \n> > \n> > > + return 0;\n> > > +}\n> > > +\n> > > +void Ccm::queueRequest(IPAContext &context,\n> > > + [[maybe_unused]] const uint32_t frame,\n> > > + IPAFrameContext &frameContext,\n> > > + const ControlList &controls)\n> > > +{\n> > > + /* Nothing to do here, the ccm will be calculated in prepare() */\n> > > + if (frameContext.awb.autoEnabled)\n> > > + return;\n> > > +\n> > > + auto &ccm = context.activeState.ccm;\n> > > +\n> > > + const auto &colourTemperature = controls.get(controls::ColourTemperature);\n> > > + const auto &ccmMatrix = controls.get(controls::ColourCorrectionMatrix);\n> > > + if (ccmMatrix)\n> > > + ccm.manual = Matrix(*ccmMatrix);\n> > > + else if (colourTemperature)\n> > > + ccm.manual = ccm_.getInterpolated(*colourTemperature);\n> > > +\n> > > + LOG(RkISP1Ccm, Debug) << \"queueRequest matrix \" << ccm.manual;\n> \n> To make the log less noisy, should this be restricted to when a CT or\n> CCM matrix control was set ? Or maybe also indicate where the CCM came\n> from ?\n> \n> \tif (ccmMatrix) {\n> \t\tccm.manual = Matrix(*ccmMatrix);\n> \t\tLOG(RkISP1Ccm, Debug)\n> \t\t\t<< \"Setting manual CCM from CCM control to \" << ccm.manual;\n> \t} else if (colourTemperature) {\n> \t\tccm.manual = ccm_.getInterpolated(*colourTemperature);\n> \t\tLOG(RkISP1Ccm, Debug)\n> \t\t\t<< \"Setting manual CCM from CT control to \" << ccm.manual;\n> \t}\n\nYes, that is better. Integrated.\n\n> \n> or something similar.\n> \n> > > + frameContext.ccm.ccm = ccm.manual;\n> > > +}\n> > > +\n> > > void Ccm::setParameters(struct rkisp1_cif_isp_ctk_config &config,\n> > > const Matrix &matrix,\n> > > const Matrix &offsets)\n> > > {\n> > > /*\n> > > * 4 bit integer and 7 bit fractional, ranging from -8 (0x400) to\n> > > - * +7.992 (0x3ff)\n> > > + * +7.9921875 (0x3ff)\n> \n> Someone likes precision :-)\n\nI wanted a way to explain why I used 7.993 as control limit :-)\n\n> \n> > > */\n> > > for (unsigned int i = 0; i < 3; i++) {\n> > > for (unsigned int j = 0; j < 3; j++)\n> > > @@ -88,14 +131,20 @@ void Ccm::setParameters(struct rkisp1_cif_isp_ctk_config &config,\n> > > void Ccm::prepare(IPAContext &context, const uint32_t frame,\n> > > IPAFrameContext &frameContext, RkISP1Params *params)\n> > > {\n> > > - uint32_t ct = frameContext.awb.temperatureK;\n> > > + if (!frameContext.awb.autoEnabled) {\n> > > + auto config = params->block();\n> > > + config.setEnabled(true);\n> > > + setParameters(*config, frameContext.ccm.ccm, Matrix());\n> > > + return;\n> > > + }\n> > > \n> > > + uint32_t ct = frameContext.awb.temperatureK;\n> > > /*\n> > > * \\todo The colour temperature will likely be noisy, add filtering to\n> > > * avoid updating the CCM matrix all the time.\n> > > */\n> > > if (frame > 0 && ct == ct_) {\n> > > - frameContext.ccm.ccm = context.activeState.ccm.ccm;\n> > > + frameContext.ccm.ccm = context.activeState.ccm.automatic;\n> > > return;\n> > > }\n> > > \n> > > @@ -103,7 +152,7 @@ void Ccm::prepare(IPAContext &context, const uint32_t frame,\n> > > Matrix ccm = ccm_.getInterpolated(ct);\n> > > Matrix offsets = offsets_.getInterpolated(ct);\n> > > \n> > > - context.activeState.ccm.ccm = ccm;\n> > > + context.activeState.ccm.automatic = ccm;\n> > > frameContext.ccm.ccm = ccm;\n> > > \n> > > auto config = params->block();\n> > > diff --git a/src/ipa/rkisp1/algorithms/ccm.h b/src/ipa/rkisp1/algorithms/ccm.h\n> > > index a5d9a9a45e5d..c301e6e531c8 100644\n> > > --- a/src/ipa/rkisp1/algorithms/ccm.h\n> > > +++ b/src/ipa/rkisp1/algorithms/ccm.h\n> > > @@ -26,6 +26,12 @@ public:\n> > > ~Ccm() = default;\n> > > \n> > > int init(IPAContext &context, const YamlObject &tuningData) override;\n> > > + int configure(IPAContext &context,\n> > > + const IPACameraSensorInfo &configInfo) override;\n> > > + void queueRequest(IPAContext &context,\n> > > + const uint32_t frame,\n> > > + IPAFrameContext &frameContext,\n> > > + const ControlList &controls) override;\n> > > void prepare(IPAContext &context, const uint32_t frame,\n> > > IPAFrameContext &frameContext,\n> > > RkISP1Params *params) override;\n> > > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h\n> > > index 769e9f114e23..f0d504215d34 100644\n> > > --- a/src/ipa/rkisp1/ipa_context.h\n> > > +++ b/src/ipa/rkisp1/ipa_context.h\n> > > @@ -101,7 +101,8 @@ struct IPAActiveState {\n> > > } awb;\n> > > \n> > > struct {\n> > > - Matrix ccm;\n> > > + Matrix manual;\n> > > + Matrix automatic;\n> \n> Missing documentation updates.\n\nAhrgh, we should really add rkisp1 to doxygen. But that is a bit of\npain....\n\nRegards,\nStefan\n\n> \n> > > } ccm;\n> > > \n> > > struct {\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 DC862C3213\n\tfor ;\n\tWed, 2 Apr 2025 14:45:39 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id DF4F068995;\n\tWed, 2 Apr 2025 16:45:38 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 446A468979\n\tfor ;\n\tWed, 2 Apr 2025 16:45:37 +0200 (CEST)","from ideasonboard.com (unknown\n\t[IPv6:2a00:6020:448c:6c00:3e5a:57a6:9731:f378])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 356AF6A2;\n\tWed, 2 Apr 2025 16:43:44 +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=\"lrA9GNHV\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1743605024;\n\tbh=piB3AhVOcNEKI9kfpjoIEcMz+0kpyXAOil/lPrj56Ms=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=lrA9GNHV3qiDf0gR/lp2NCJtqSRtdHfZrZcG/L4NjSbJdeaaFRg5vWWPwWAlFBezt\n\tNRtwIoRweVWHs4D59uIRHebQoqH4Y7a9z7EVQ9QMGqD6UW/dI8Lt0M2gyg5dt9BXz1\n\t6zUkXTcplvqah8t1v2y02JykZte8xaVlk16yAr2E=","Date":"Wed, 2 Apr 2025 16:45:34 +0200","From":"Stefan Klug ","To":"Laurent Pinchart ","Cc":"Kieran Bingham , \n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v2 11/17] ipa: rkisp1: Implement manual\n\tColourCorrectionMatrix control","Message-ID":"","References":"<20250319161152.63625-1-stefan.klug@ideasonboard.com>\n\t<20250319161152.63625-12-stefan.klug@ideasonboard.com>\n\t<174344278820.3394313.4649916776360845530@ping.linuxembedded.co.uk>\n\t<20250401213005.GE3494@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20250401213005.GE3494@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\" "}}]