[{"id":37311,"web_url":"https://patchwork.libcamera.org/comment/37311/","msgid":"","date":"2025-12-11T15:25:11","subject":"Re: [PATCH v4 2/7] ipa: rkisp1: algorithms: dpf: Implement noise\n\treduction mode switching","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Rui\n\nyou can shorten the subject here (and in the rest of the series) as\nwell\n\nOn Sun, Dec 07, 2025 at 07:48:03PM -0500, Rui Wang wrote:\n> -Add support for switching between different noise reduction modes.\n> -Introduce `noiseReductionModes_` to store mode-specific configs.\n> -loadReductionConfig() select specific config from configs\n\nMissing '.'\n\nPlease try to be consistent\n\n>\n> Introduce `noiseReductionModes_` to store current configs.\n\nNot sure why some lines are - and this is not.\n\nA simple\n\nAdd support for switching between different noise reduction modes.\n\nIntroduce `noiseReductionModes_` to store mode-specific configs and\nimplement loadReductionConfig() to select a mode configuration using\nthe application provided control value.\n\n>\n> Signed-off-by: Rui Wang \n> ---\n>\n> changelog:\n> - Add blank line\n> - Move V3 first patch's loadReductionConfig and reduction mode helper\n> into this patch\n>\n> src/ipa/rkisp1/algorithms/dpf.cpp | 81 ++++++++++++++++++++++++++++++-\n> src/ipa/rkisp1/algorithms/dpf.h | 11 +++++\n> 2 files changed, 90 insertions(+), 2 deletions(-)\n>\n> diff --git a/src/ipa/rkisp1/algorithms/dpf.cpp b/src/ipa/rkisp1/algorithms/dpf.cpp\n> index cd0a7d9d..18f2a158 100644\n> --- a/src/ipa/rkisp1/algorithms/dpf.cpp\n> +++ b/src/ipa/rkisp1/algorithms/dpf.cpp\n> @@ -37,7 +37,9 @@ namespace ipa::rkisp1::algorithms {\n> LOG_DEFINE_CATEGORY(RkISP1Dpf)\n>\n> Dpf::Dpf()\n> -\t: config_({}), strengthConfig_({})\n> +\t: config_({}), strengthConfig_({}),\n> +\t noiseReductionModes_({}),\n\nthis fits on the previous line\n\n> +\t runningMode_(controls::draft::NoiseReductionModeOff)\n> {\n> }\n>\n> @@ -61,6 +63,53 @@ bool Dpf::parseConfig(const YamlObject &tuningData)\n> \tif (!parseSingleConfig(tuningData, config_, strengthConfig_)) {\n> \t\treturn false;\n> \t}\n> +\n> +\t/* Parse modes. */\n> +\tif (!parseModes(tuningData)) {\n> +\t\treturn false;\n> +\t}\n\nNo curly braces etc etc\n\n\n> +\n> +\treturn true;\n> +}\n> +\n> +bool Dpf::parseModes(const YamlObject &tuningData)\n> +{\n> +\t/* Parse noise reduction modes. */\n> +\tif (!tuningData.contains(\"modes\")) {\n> +\t\treturn true;\n> +\t}\n> +\n> +\tnoiseReductionModes_.clear();\n> +\tfor (const auto &entry : tuningData[\"modes\"].asList()) {\n> +\t\tstd::optional typeOpt =\n> +\t\t\tentry[\"type\"].get();\n> +\t\tif (!typeOpt) {\n> +\t\t\tLOG(RkISP1Dpf, Error) << \"Modes entry missing type\";\n> +\t\t\treturn false;\n> +\t\t}\n\nAre modes mandatory in your opinion ? Don't we have a general\nconfiguration (parsed by parseSingleConfig()) ? Can't a single\nconfiguration be used ? I guess we can't as the controls we have do\nnot allow a \"turn noise reduction on\" but only allow to specify \"use\nthis noise reduction mode\". Given this context, I might have missed\nwhat the outer configuration is used for, if only the ones part of a\nmode can be enabled.\n\n> +\n> +\t\tint32_t modeValue = controls::draft::NoiseReductionModeOff;\n> +\t\tif (*typeOpt == \"minimal\") {\n> +\t\t\tmodeValue = controls::draft::NoiseReductionModeMinimal;\n> +\t\t} else if (*typeOpt == \"fast\") {\n> +\t\t\tmodeValue = controls::draft::NoiseReductionModeFast;\n> +\t\t} else if (*typeOpt == \"highquality\") {\n> +\t\t\tmodeValue = controls::draft::NoiseReductionModeHighQuality;\n> +\t\t} else if (*typeOpt == \"zsl\") {\n> +\t\t\tmodeValue = controls::draft::NoiseReductionModeZSL;\n> +\t\t} else {\n> +\t\t\tLOG(RkISP1Dpf, Error) << \"Unknown mode type: \" << *typeOpt;\n> +\t\t\treturn false;\n> +\t\t}\n> +\n> +\t\tModeConfig mode{};\n\nYou could declare this before the long if {} else if {} section\n\n> +\t\tmode.modeValue = modeValue;\n\nAnd assign mode.modeValue directly instead of going through a\ntemporary\n\n> +\t\tif (!parseSingleConfig(entry, mode.dpf, mode.strength)) {\n> +\t\t\treturn false;\n> +\t\t}\n\nNo curly etc etc\n\n> +\t\tnoiseReductionModes_.push_back(mode);\n> +\t}\n> +\n> \treturn true;\n> }\n>\n> @@ -192,6 +241,29 @@ bool Dpf::parseSingleConfig(const YamlObject &tuningData,\n> \treturn true;\n> }\n>\n> +bool Dpf::loadReductionConfig(int32_t mode)\n> +{\n> +\tauto it = std::find_if(noiseReductionModes_.begin(), noiseReductionModes_.end(),\n> +\t\t\t [mode](const ModeConfig &m) {\n> +\t\t\t\t return m.modeValue == mode;\n> +\t\t\t });\n> +\tif (it == noiseReductionModes_.end()) {\n> +\t\tLOG(RkISP1Dpf, Warning)\n\n\nRight now the main IPA module in rkisp1.cpp registers\n\n\t{ &controls::draft::NoiseReductionMode, ControlInfo(controls::draft::NoiseReductionModeValues) },\n\nmeaning all the defined modes are supported.\n\nI guess we need to do what, in example, Agc::parseMeteringModes()\ndoes: parse the tuning file and populate the supported control values\nwith the entries enabled in the config file.\n\nDo you plan to do so on top of this series ?\n\n\n\n> +\t\t\t<< \"No DPF config for reduction mode \"\n> +\t\t\t<< static_cast(mode);\n\nI don't think you need to cast (below too)\n\n> +\t\treturn false;\n> +\t}\n> +\n> +\tconfig_ = it->dpf;\n> +\tstrengthConfig_ = it->strength;\n> +\n> +\tLOG(RkISP1Dpf, Info)\n\nMaybe just a Debug, this is a regular operation, isn't it ?\n\n> +\t\t<< \"DPF mode=Reduction (config loaded)\"\n> +\t\t<< \" mode=\" << static_cast(mode);\n\n << \"Selected DPF mode \" << mode;\n\nIt would be nicer if we keep a map somewhere to associate the mode id\nwith a human readable description.\n\n> +\n> +\treturn true;\n> +}\n> +\n> /**\n> * \\copydoc libcamera::ipa::Algorithm::queueRequest\n> */\n> @@ -207,6 +279,7 @@ void Dpf::queueRequest(IPAContext &context,\n> \tif (denoise) {\n> \t\tLOG(RkISP1Dpf, Debug) << \"Set denoise to \" << *denoise;\n>\n> +\t\trunningMode_ = *denoise;\n> \t\tswitch (*denoise) {\n> \t\tcase controls::draft::NoiseReductionModeOff:\n> \t\t\tif (dpf.denoise) {\n> @@ -217,8 +290,12 @@ void Dpf::queueRequest(IPAContext &context,\n> \t\tcase controls::draft::NoiseReductionModeMinimal:\n> \t\tcase controls::draft::NoiseReductionModeHighQuality:\n> \t\tcase controls::draft::NoiseReductionModeFast:\n> -\t\t\tif (!dpf.denoise) {\n> +\t\tcase controls::draft::NoiseReductionModeZSL:\n\nWhy wasn't this handle before this patch ?\n\n> +\t\t\tif (loadReductionConfig(runningMode_)) {\n> +\t\t\t\tupdate = true;\n> \t\t\t\tdpf.denoise = true;\n> +\t\t\t} else {\n> +\t\t\t\tdpf.denoise = false;\n> \t\t\t\tupdate = true;\n> \t\t\t}\n\nThis might get tricky.\n\nWe have 4 cases here\n\n1) DPF was off\n 1.a loadReductionConfig() success\n 1.b loadReductionConfig() fail\n2) DPF was on\n 2.a loadReductionConfig() success\n 2.b loadReductionConfig() fail\n\n1.a: update = true, dpf.denoise = true\n1.b: update = false, dpf.denoise = false;\n2.a: update = true, dpf.denoise = true\n2.b: update = false, denoise = false\n\n2.a can be further optimized to only update the denoise configuration\nif the mode has changed compared to the current one\n\nI would rework this function as (not tested)\n\n\tconst auto &denoise = controls.get(controls::draft::NoiseReductionMode);\n\tbool update = false;\n\n\tif (denoise && *denoise != runningMode_) {\n\t\tswitch (*denoise) {\n\t\tcase controls::draft::NoiseReductionModeOff:\n\t\t\tif (dpf.denoise) {\n\t\t\t\tdpf.denoise = false;\n\t\t\t\tupdate = true;\n\t\t\t}\n\t\t\tbreak;\n\t\tcase controls::draft::NoiseReductionModeMinimal:\n\t\tcase controls::draft::NoiseReductionModeHighQuality:\n\t\tcase controls::draft::NoiseReductionModeFast:\n\t\tcase controls::draft::NoiseReductionModeZSL:\n\t\t\tif (loadReductionConfig(*denoise)) {\n\t\t\t\tupdate = true;\n\t\t\t\tdpf.denoise = true;\n\t\t\t}\n\t\t\tbreak;\n\t\tdefault:\n\t\t\tLOG(RkISP1Dpf, Error)\n\t\t\t\t<< \"Unsupported denoise value \"\n\t\t\t\t<< *denoise;\n\t\t\tbreak;\n\t\t}\n\n if (update) {\n runningMode_ = *denoise;\n LOG(RkISP1Dpf, Debug) << \"Set denoise mode to \"\n << *denoise;\n }\n }\n\n\tframeContext.dpf.denoise = dpf.denoise;\n\tframeContext.dpf.update = update;\n\nWhat do you think ?\n\nThanks\n j\n\n> \t\t\tbreak;\n> diff --git a/src/ipa/rkisp1/algorithms/dpf.h b/src/ipa/rkisp1/algorithms/dpf.h\n> index bee6fc9b..30cbaa57 100644\n> --- a/src/ipa/rkisp1/algorithms/dpf.h\n> +++ b/src/ipa/rkisp1/algorithms/dpf.h\n> @@ -30,13 +30,24 @@ public:\n> \t\t RkISP1Params *params) override;\n>\n> private:\n> +\tstruct ModeConfig {\n> +\t\tint32_t modeValue;\n> +\t\trkisp1_cif_isp_dpf_config dpf;\n> +\t\trkisp1_cif_isp_dpf_strength_config strength;\n> +\t};\n> +\n> \tbool parseConfig(const YamlObject &tuningData);\n> +\tbool parseModes(const YamlObject &tuningData);\n> \tbool parseSingleConfig(const YamlObject &tuningData,\n> \t\t\t rkisp1_cif_isp_dpf_config &config,\n> \t\t\t rkisp1_cif_isp_dpf_strength_config &strengthConfig);\n>\n> +\tbool loadReductionConfig(int32_t mode);\n> +\n> \tstruct rkisp1_cif_isp_dpf_config config_;\n> \tstruct rkisp1_cif_isp_dpf_strength_config strengthConfig_;\n> +\tstd::vector noiseReductionModes_;\n> +\tint32_t runningMode_;\n> };\n>\n> } /* namespace ipa::rkisp1::algorithms */\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 5D396C3257\n\tfor ;\n\tThu, 11 Dec 2025 15:25:17 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 38FE261608;\n\tThu, 11 Dec 2025 16:25:16 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C255361603\n\tfor ;\n\tThu, 11 Dec 2025 16:25:14 +0100 (CET)","from ideasonboard.com (93-46-82-201.ip106.fastwebnet.it\n\t[93.46.82.201])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 492381352;\n\tThu, 11 Dec 2025 16:25:12 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"Wk01Ckba\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1765466712;\n\tbh=HC2TJSwyBy1YgLphnEPa+Sn1k4HcmHIKSMkPwJO1f+8=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Wk01CkbaB1UaMGdppIri70s9ln0hmQ4/Rgss9RXhfmqam06GdJJgoJira2LKp3hAW\n\tq9CYDKEdqxjAyQ3Fiqmk4vlopiMPYT8BkXIBWH12SPO4Ao9yYLR933uP/kIzXyMAWE\n\tyXGn3pVR/9Zv1DxMZhtcJSVMCr/m/fMcgDyfr9TU=","Date":"Thu, 11 Dec 2025 16:25:11 +0100","From":"Jacopo Mondi ","To":"Rui Wang ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v4 2/7] ipa: rkisp1: algorithms: dpf: Implement noise\n\treduction mode switching","Message-ID":"","References":"<20251208004808.1274417-1-rui.wang@ideasonboard.com>\n\t<20251208004808.1274417-3-rui.wang@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20251208004808.1274417-3-rui.wang@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":37361,"web_url":"https://patchwork.libcamera.org/comment/37361/","msgid":"<476a6ea1a15debe5233ee0355e22ab08.rui.wang@ideasonboard.com>","date":"2025-12-14T01:20:11","subject":"Re: [PATCH v4 2/7] ipa: rkisp1: algorithms: dpf: Implement noise\n\treduction mode switching","submitter":{"id":241,"url":"https://patchwork.libcamera.org/api/people/241/","name":"Rui Wang","email":"rui.wang@ideasonboard.com"},"content":"Jacopo Mondi wrote:\n> Hi Rui\n> \n> you can shorten the subject here (and in the rest of the series) as\n> well\n> \n> On Sun, Dec 07, 2025 at 07:48:03PM -0500, Rui Wang wrote:\n> > -Add support for switching between different noise reduction modes.\n> > -Introduce `noiseReductionModes_` to store mode-specific configs.\n> > -loadReductionConfig() select specific config from configs\n> \n> Missing '.'\n> \n> Please try to be consistent\n> \n> >\n> > Introduce `noiseReductionModes_` to store current configs.\n> \n> Not sure why some lines are - and this is not.\n> \n> A simple\n> \n> Add support for switching between different noise reduction modes.\n> \n> Introduce `noiseReductionModes_` to store mode-specific configs and\n> implement loadReductionConfig() to select a mode configuration using\n> the application provided control value.\n> \n> >\n> > Signed-off-by: Rui Wang \n> > ---\n> >\n> > changelog:\n> > - Add blank line\n> > - Move V3 first patch's loadReductionConfig and reduction mode helper\n> > into this patch\n> >\n> > src/ipa/rkisp1/algorithms/dpf.cpp | 81 ++++++++++++++++++++++++++++++-\n> > src/ipa/rkisp1/algorithms/dpf.h | 11 +++++\n> > 2 files changed, 90 insertions(+), 2 deletions(-)\n> >\n> > diff --git a/src/ipa/rkisp1/algorithms/dpf.cpp b/src/ipa/rkisp1/algorithms/dpf.cpp\n> > index cd0a7d9d..18f2a158 100644\n> > --- a/src/ipa/rkisp1/algorithms/dpf.cpp\n> > +++ b/src/ipa/rkisp1/algorithms/dpf.cpp\n> > @@ -37,7 +37,9 @@ namespace ipa::rkisp1::algorithms {\n> > LOG_DEFINE_CATEGORY(RkISP1Dpf)\n> >\n> > Dpf::Dpf()\n> > -\t: config_({}), strengthConfig_({})\n> > +\t: config_({}), strengthConfig_({}),\n> > +\t noiseReductionModes_({}),\n> \n> this fits on the previous line\n> \n> > +\t runningMode_(controls::draft::NoiseReductionModeOff)\n> > {\n> > }\n> >\n> > @@ -61,6 +63,53 @@ bool Dpf::parseConfig(const YamlObject &tuningData)\n> > \tif (!parseSingleConfig(tuningData, config_, strengthConfig_)) {\n> > \t\treturn false;\n> > \t}\n> > +\n> > +\t/* Parse modes. */\n> > +\tif (!parseModes(tuningData)) {\n> > +\t\treturn false;\n> > +\t}\n> \n> No curly braces etc etc\n> \n> \n> > +\n> > +\treturn true;\n> > +}\n> > +\n> > +bool Dpf::parseModes(const YamlObject &tuningData)\n> > +{\n> > +\t/* Parse noise reduction modes. */\n> > +\tif (!tuningData.contains(\"modes\")) {\n> > +\t\treturn true;\n> > +\t}\n> > +\n> > +\tnoiseReductionModes_.clear();\n> > +\tfor (const auto &entry : tuningData[\"modes\"].asList()) {\n> > +\t\tstd::optional typeOpt =\n> > +\t\t\tentry[\"type\"].get();\n> > +\t\tif (!typeOpt) {\n> > +\t\t\tLOG(RkISP1Dpf, Error) << \"Modes entry missing type\";\n> > +\t\t\treturn false;\n> > +\t\t}\n> \n> Are modes mandatory in your opinion ? Don't we have a general\n> configuration (parsed by parseSingleConfig()) ? Can't a single\n> configuration be used ? I guess we can't as the controls we have do\n> not allow a \"turn noise reduction on\" but only allow to specify \"use\n> this noise reduction mode\". Given this context, I might have missed\n> what the outer configuration is used for, if only the ones part of a\n> mode can be enabled.\n> \nIn tuning file like:imx219.yaml ,\n the mode config is optional like : highquality/zsl/minimal.\n From control , if it choose a specific without any tuning config , it\n will print some error message and keep previous status\n\n> > +\n> > +\t\tint32_t modeValue = controls::draft::NoiseReductionModeOff;\n> > +\t\tif (*typeOpt == \"minimal\") {\n> > +\t\t\tmodeValue = controls::draft::NoiseReductionModeMinimal;\n> > +\t\t} else if (*typeOpt == \"fast\") {\n> > +\t\t\tmodeValue = controls::draft::NoiseReductionModeFast;\n> > +\t\t} else if (*typeOpt == \"highquality\") {\n> > +\t\t\tmodeValue = controls::draft::NoiseReductionModeHighQuality;\n> > +\t\t} else if (*typeOpt == \"zsl\") {\n> > +\t\t\tmodeValue = controls::draft::NoiseReductionModeZSL;\n> > +\t\t} else {\n> > +\t\t\tLOG(RkISP1Dpf, Error) << \"Unknown mode type: \" << *typeOpt;\n> > +\t\t\treturn false;\n> > +\t\t}\n> > +\n> > +\t\tModeConfig mode{};\n> \n> You could declare this before the long if {} else if {} section\n> \nYes will update in next series\n> > +\t\tmode.modeValue = modeValue;\n> \n> And assign mode.modeValue directly instead of going through a\n> temporary\n> \n> > +\t\tif (!parseSingleConfig(entry, mode.dpf, mode.strength)) {\n> > +\t\t\treturn false;\n> > +\t\t}\n> \n> No curly etc etc\n> \n\nWill update in next series\n\n> > +\t\tnoiseReductionModes_.push_back(mode);\n> > +\t}\n> > +\n> > \treturn true;\n> > }\n> >\n> > @@ -192,6 +241,29 @@ bool Dpf::parseSingleConfig(const YamlObject &tuningData,\n> > \treturn true;\n> > }\n> >\n> > +bool Dpf::loadReductionConfig(int32_t mode)\n> > +{\n> > +\tauto it = std::find_if(noiseReductionModes_.begin(), noiseReductionModes_.end(),\n> > +\t\t\t [mode](const ModeConfig &m) {\n> > +\t\t\t\t return m.modeValue == mode;\n> > +\t\t\t });\n> > +\tif (it == noiseReductionModes_.end()) {\n> > +\t\tLOG(RkISP1Dpf, Warning)\n> \n> \n> Right now the main IPA module in rkisp1.cpp registers\n> \n> \t{ &controls::draft::NoiseReductionMode, ControlInfo(controls::draft::NoiseReductionModeValues) },\n> \n> meaning all the defined modes are supported.\n> \n> I guess we need to do what, in example, Agc::parseMeteringModes()\n> does: parse the tuning file and populate the supported control values\n> with the entries enabled in the config file.\n> \n> Do you plan to do so on top of this series ?\n>\nYes , for each mode has its own specific tuning config for dpf and filter denoise\nand in future I would add Auto Mode into this control as well .\n> \n> \n> > +\t\t\t<< \"No DPF config for reduction mode \"\n> > +\t\t\t<< static_cast(mode);\n> \n> I don't think you need to cast (below too)\nyes it is already int32_t\n> \n> > +\t\treturn false;\n> > +\t}\n> > +\n> > +\tconfig_ = it->dpf;\n> > +\tstrengthConfig_ = it->strength;\n> > +\n> > +\tLOG(RkISP1Dpf, Info)\n> \n> Maybe just a Debug, this is a regular operation, isn't it ?\n>\nI found when select the control ,libcamera it will print log control value\nas info leve . So I print as same level. it prints the logs only when control value changed\n\n\n> > +\t\t<< \"DPF mode=Reduction (config loaded)\"\n> > +\t\t<< \" mode=\" << static_cast(mode);\n> \n> << \"Selected DPF mode \" << mode;\n> \n> It would be nicer if we keep a map somewhere to associate the mode id\n> with a human readable description.\n\nYes, good idea , add map can use into other function for logging and debugging\n> \n> > +\n> > +\treturn true;\n> > +}\n> > +\n> > /**\n> > * \\copydoc libcamera::ipa::Algorithm::queueRequest\n> > */\n> > @@ -207,6 +279,7 @@ void Dpf::queueRequest(IPAContext &context,\n> > \tif (denoise) {\n> > \t\tLOG(RkISP1Dpf, Debug) << \"Set denoise to \" << *denoise;\n> >\n> > +\t\trunningMode_ = *denoise;\n> > \t\tswitch (*denoise) {\n> > \t\tcase controls::draft::NoiseReductionModeOff:\n> > \t\t\tif (dpf.denoise) {\n> > @@ -217,8 +290,12 @@ void Dpf::queueRequest(IPAContext &context,\n> > \t\tcase controls::draft::NoiseReductionModeMinimal:\n> > \t\tcase controls::draft::NoiseReductionModeHighQuality:\n> > \t\tcase controls::draft::NoiseReductionModeFast:\n> > -\t\t\tif (!dpf.denoise) {\n> > +\t\tcase controls::draft::NoiseReductionModeZSL:\n> \n> Why wasn't this handle before this patch ?\n\n on master branch , ZSL is excluded in this switch case. it is not handled by enabling denoise\n> \n> > +\t\t\tif (loadReductionConfig(runningMode_)) {\n> > +\t\t\t\tupdate = true;\n> > \t\t\t\tdpf.denoise = true;\n> > +\t\t\t} else {\n> > +\t\t\t\tdpf.denoise = false;\n> > \t\t\t\tupdate = true;\n> > \t\t\t}\n> \n> This might get tricky.\n> \n> We have 4 cases here\n> \n> 1) DPF was off\n> 1.a loadReductionConfig() success\n> 1.b loadReductionConfig() fail\n> 2) DPF was on\n> 2.a loadReductionConfig() success\n> 2.b loadReductionConfig() fail\n> \n> 1.a: update = true, dpf.denoise = true\n> 1.b: update = false, dpf.denoise = false;\n> 2.a: update = true, dpf.denoise = true\n> 2.b: update = false, denoise = false\n> \n> 2.a can be further optimized to only update the denoise configuration\n> if the mode has changed compared to the current one\n> \n> I would rework this function as (not tested)\n> \n> \tconst auto &denoise = controls.get(controls::draft::NoiseReductionMode);\n> \tbool update = false;\n> \n> \tif (denoise && *denoise != runningMode_) {\n> \t\tswitch (*denoise) {\n> \t\tcase controls::draft::NoiseReductionModeOff:\n> \t\t\tif (dpf.denoise) {\n> \t\t\t\tdpf.denoise = false;\n> \t\t\t\tupdate = true;\n> \t\t\t}\n> \t\t\tbreak;\n> \t\tcase controls::draft::NoiseReductionModeMinimal:\n> \t\tcase controls::draft::NoiseReductionModeHighQuality:\n> \t\tcase controls::draft::NoiseReductionModeFast:\n> \t\tcase controls::draft::NoiseReductionModeZSL:\n> \t\t\tif (loadReductionConfig(*denoise)) {\n> \t\t\t\tupdate = true;\n> \t\t\t\tdpf.denoise = true;\n> \t\t\t}\n> \t\t\tbreak;\n> \t\tdefault:\n> \t\t\tLOG(RkISP1Dpf, Error)\n> \t\t\t\t<< \"Unsupported denoise value \"\n> \t\t\t\t<< *denoise;\n> \t\t\tbreak;\n> \t\t}\n> \n> if (update) {\n> runningMode_ = *denoise;\n> LOG(RkISP1Dpf, Debug) << \"Set denoise mode to \"\n> << *denoise;\n> }\n> }\n> \n> \tframeContext.dpf.denoise = dpf.denoise;\n> \tframeContext.dpf.update = update;\n> \n> What do you think ?\n> \n> Thanks\n> j\n\n Perfectly .thanks for your input , will update the logic in next series\n> \n> > \t\t\tbreak;\n> > diff --git a/src/ipa/rkisp1/algorithms/dpf.h b/src/ipa/rkisp1/algorithms/dpf.h\n> > index bee6fc9b..30cbaa57 100644\n> > --- a/src/ipa/rkisp1/algorithms/dpf.h\n> > +++ b/src/ipa/rkisp1/algorithms/dpf.h\n> > @@ -30,13 +30,24 @@ public:\n> > \t\t RkISP1Params *params) override;\n> >\n> > private:\n> > +\tstruct ModeConfig {\n> > +\t\tint32_t modeValue;\n> > +\t\trkisp1_cif_isp_dpf_config dpf;\n> > +\t\trkisp1_cif_isp_dpf_strength_config strength;\n> > +\t};\n> > +\n> > \tbool parseConfig(const YamlObject &tuningData);\n> > +\tbool parseModes(const YamlObject &tuningData);\n> > \tbool parseSingleConfig(const YamlObject &tuningData,\n> > \t\t\t rkisp1_cif_isp_dpf_config &config,\n> > \t\t\t rkisp1_cif_isp_dpf_strength_config &strengthConfig);\n> >\n> > +\tbool loadReductionConfig(int32_t mode);\n> > +\n> > \tstruct rkisp1_cif_isp_dpf_config config_;\n> > \tstruct rkisp1_cif_isp_dpf_strength_config strengthConfig_;\n> > +\tstd::vector noiseReductionModes_;\n> > +\tint32_t runningMode_;\n> > };\n> >\n> > } /* namespace ipa::rkisp1::algorithms */\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 B5D55C3257\n\tfor ;\n\tSun, 14 Dec 2025 01:20:27 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D45A561924;\n\tSun, 14 Dec 2025 02:20:26 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 047CD6142F\n\tfor ;\n\tSun, 14 Dec 2025 02:20:25 +0100 (CET)","from pyrite.rasen.tech (unknown [209.216.103.65])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 95AF26A8;\n\tSun, 14 Dec 2025 02:20:20 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"UQ3OU0aR\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1765675220;\n\tbh=vqXJuwlTd4mQcp+go9G8kpDyph7olhxYRcKoFKTaFVI=;\n\th=Date:From:To:Cc:In-Reply-To:References:Subject:From;\n\tb=UQ3OU0aRma1/k2mb+4x3RIDPRR+P1D9+yCmp/WQCx2tEQS12p7ska/nf/OVRg6MRI\n\tw5e5q/ik0rp82e0sch8EhfOB+VpXt0xJ/dgVnLogSqX4IdTECkv8lnHXONg1aQjsKP\n\tMZofuAXZJ3oJgMB6gGBgNvXy7UrZhj8UhtVmX33Y=","Date":"Sat, 13 Dec 2025 20:20:11 -0500","Message-ID":"<476a6ea1a15debe5233ee0355e22ab08.rui.wang@ideasonboard.com>","From":"Rui Wang ","To":"Jacopo Mondi ,\n\tRui Wang ","Cc":"libcamera-devel@lists.libcamera.org","In-Reply-To":"","References":"<20251208004808.1274417-1-rui.wang@ideasonboard.com>\n\t<20251208004808.1274417-3-rui.wang@ideasonboard.com>\n\t","Subject":"Re: [PATCH v4 2/7] ipa: rkisp1: algorithms: dpf: Implement noise\n\treduction mode switching","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\" "}}]