[{"id":37411,"web_url":"https://patchwork.libcamera.org/comment/37411/","msgid":"","date":"2025-12-16T16:56:29","subject":"Re: [PATCH v5 2/6] ipa: rkisp1: algorithms: dpf: Implement mode\n\tswitching","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"On Sun, Dec 14, 2025 at 01:16:42PM -0500, Rui Wang wrote:\n> Add support for switching between different noise reduction modes.\n>\n> Introduce `noiseReductionModes_` to store mode-specific configs.\n>\n> LoadReductionConfig() select specific config from configs.\n>\n> Signed-off-by: Rui Wang \n> ---\n>\n> changelog:\n> - Format commit message\n> - Refactored parseModes to utilize kModesMap for string-to-mode lookups,\n> replacing the previous if-else chain.\n> - Fixed logic in queueRequest to load configuration using the requested\n> *denoise mode instead of the stale runningMode_.\n>\n> src/ipa/rkisp1/algorithms/dpf.cpp | 91 +++++++++++++++++++++++++++++--\n> src/ipa/rkisp1/algorithms/dpf.h | 11 ++++\n> 2 files changed, 97 insertions(+), 5 deletions(-)\n>\n> diff --git a/src/ipa/rkisp1/algorithms/dpf.cpp b/src/ipa/rkisp1/algorithms/dpf.cpp\n> index dd3effa1..392edda1 100644\n> --- a/src/ipa/rkisp1/algorithms/dpf.cpp\n> +++ b/src/ipa/rkisp1/algorithms/dpf.cpp\n> @@ -8,6 +8,7 @@\n> #include \"dpf.h\"\n>\n> #include \n> +#include \n> #include \n> #include \n>\n> @@ -36,8 +37,20 @@ namespace ipa::rkisp1::algorithms {\n>\n> LOG_DEFINE_CATEGORY(RkISP1Dpf)\n>\n> +namespace {\n> +\n> +const std::map kModesMap = {\n> +\t{ controls::draft::NoiseReductionModeMinimal, \"minimal\" },\n> +\t{ controls::draft::NoiseReductionModeFast, \"fast\" },\n> +\t{ controls::draft::NoiseReductionModeHighQuality, \"highquality\" },\n> +\t{ controls::draft::NoiseReductionModeZSL, \"zsl\" },\n> +};\n> +\n> +} /* namespace */\n> +\n> Dpf::Dpf()\n> -\t: config_({}), strengthConfig_({})\n> +\t: config_({}), strengthConfig_({}), noiseReductionModes_({}),\n> +\t runningMode_(controls::draft::NoiseReductionModeOff)\n> {\n> }\n>\n> @@ -62,6 +75,48 @@ int Dpf::parseConfig(const YamlObject &tuningData)\n> \tif (ret)\n> \t\treturn ret;\n>\n> +\t/* Parse modes. */\n> +\tret = parseModes(tuningData);\n\n return parseModes(tuningData);\n\n> +\tif (ret)\n> +\t\treturn ret;\n> +\n> +\treturn 0;\n> +}\n> +\n> +int Dpf::parseModes(const YamlObject &tuningData)\n> +{\n> +\t/* Parse noise reduction modes. */\n> +\tif (!tuningData.contains(\"modes\"))\n> +\t\treturn -EINVAL;\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 -EINVAL;\n> +\t\t}\n> +\n> +\t\tModeConfig mode;\n> +\t\tauto it = std::find_if(kModesMap.begin(), kModesMap.end(),\n> +\t\t\t\t [&typeOpt](const auto &pair) {\n> +\t\t\t\t\t return pair.second == *typeOpt;\n> +\t\t\t\t });\n> +\n> +\t\tif (it == kModesMap.end()) {\n> +\t\t\tLOG(RkISP1Dpf, Error) << \"Unknown mode type: \" << *typeOpt;\n> +\t\t\treturn -EINVAL;\n> +\t\t}\n> +\n> +\t\tmode.modeValue = it->first;\n> +\t\tauto ret = parseSingleConfig(entry, mode.dpf, mode.strength);\n> +\t\tif (ret != 0)\n if (ret)\n\n> +\t\t\treturn ret;\n> +\n> +\t\tnoiseReductionModes_.push_back(mode);\n> +\t}\n> +\n> \treturn 0;\n> }\n>\n> @@ -193,6 +248,29 @@ int Dpf::parseSingleConfig(const YamlObject &tuningData,\n> \treturn 0;\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> +\t\t\t<< \"No DPF config for reduction mode \"\n> +\t\t\t<< static_cast(mode);\n\nYou don't need a cast I guess\n\n> +\t\treturn false;\n> +\t}\n> +\n> +\tconfig_ = it->dpf;\n> +\tstrengthConfig_ = it->strength;\n> +\n> +\tLOG(RkISP1Dpf, Info)\n\nI still think this is too verbose, this is about loading a\nconfiguration from tuning file for an algorithm, this is a debug\nmessage, nothing here's worth an Info imho\n\n> +\t\t<< \"DPF mode=Reduction (config loaded)\"\n> +\t\t<< \" mode=\" << kModesMap.at(mode);\n> +\n> +\treturn true;\n> +}\n> +\n> /**\n> * \\copydoc libcamera::ipa::Algorithm::queueRequest\n> */\n> @@ -206,8 +284,6 @@ void Dpf::queueRequest(IPAContext &context,\n>\n> \tconst auto &denoise = controls.get(controls::draft::NoiseReductionMode);\n> \tif (denoise) {\n> -\t\tLOG(RkISP1Dpf, Debug) << \"Set denoise to \" << *denoise;\n> -\n> \t\tswitch (*denoise) {\n> \t\tcase controls::draft::NoiseReductionModeOff:\n> \t\t\tif (dpf.denoise) {\n> @@ -218,9 +294,10 @@ 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\t\t\tdpf.denoise = true;\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> @@ -229,6 +306,10 @@ void Dpf::queueRequest(IPAContext &context,\n> \t\t\t\t<< *denoise;\n> \t\t\tbreak;\n> \t\t}\n> +\t\tif (update) {\n> +\t\t\trunningMode_ = *denoise;\n> +\t\t\tLOG(RkISP1Dpf, Debug) << \"Set denoise to \" << kModesMap.at(runningMode_);\n> +\t\t}\n> \t}\n>\n> \tframeContext.dpf.denoise = dpf.denoise;\n> diff --git a/src/ipa/rkisp1/algorithms/dpf.h b/src/ipa/rkisp1/algorithms/dpf.h\n> index 39186c55..2a2c7052 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> \tint parseConfig(const YamlObject &tuningData);\n> +\tint parseModes(const YamlObject &tuningData);\n> \tint 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\nWith the above addressed\n\nReviewed-by: Jacopo Mondi \n\nThanks\n j\n\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 06B32C3257\n\tfor ;\n\tTue, 16 Dec 2025 16:56:34 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 47F4061A27;\n\tTue, 16 Dec 2025 17:56:34 +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 5F7116191D\n\tfor ;\n\tTue, 16 Dec 2025 17:56:32 +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 83E4E6DE;\n\tTue, 16 Dec 2025 17:56:26 +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=\"EWQDv7k+\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1765904186;\n\tbh=H3S4mzI60VKoavq/dchfSluknD35p175hxeScbXMlPo=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=EWQDv7k+PD5O6OCnZJjsWsuCg6d427AWpwIEtv4caQg7tvToQ6a98f4+9a5IJa+sA\n\ti+zOHSwCYbE+zs5o43kt3OUI2HDMnO68rMHZS2RCTG+r4Mk/6d3tuQq/muJ0Lo2ebB\n\tASIJ1GUcBVgQprccvecUxrE73LWfksl04OmnUO9E=","Date":"Tue, 16 Dec 2025 17:56:29 +0100","From":"Jacopo Mondi ","To":"Rui Wang ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v5 2/6] ipa: rkisp1: algorithms: dpf: Implement mode\n\tswitching","Message-ID":"","References":"<20251214181646.573675-1-rui.wang@ideasonboard.com>\n\t<20251214181646.573675-3-rui.wang@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20251214181646.573675-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\" "}}]