[{"id":37315,"web_url":"https://patchwork.libcamera.org/comment/37315/","msgid":"","date":"2025-12-11T15:56:18","subject":"Re: [PATCH v4 5/7] ipa: rkisp1: algorithms: dpf: Use\n\tYamlObject::Getter for parsing","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Rui\n\nOn Sun, Dec 07, 2025 at 07:48:06PM -0500, Rui Wang wrote:\n> Refactor DPF configuration parsing to use YamlObject::Getter\n> specializations, similar to the approach used in pwl.cpp. This\n> simplifies parseSingleConfig() by delegating the parsing logic\n> to dedicated getter specializations.\n>\n> Implement two YamlObject::Getter specializations:\n> - rkisp1_cif_isp_dpf_config: Parses filter and nll parameters\n> - rkisp1_cif_isp_dpf_strength_config: Parses strength parameters\n>\n> The specializations are defined at the top of the file in the\n> libcamera namespace.\n>\n> Remove LOG statements from the getter specializations to avoid\n> namespace visibility issues, as the LOG macro requires access\n> to the log category which is defined in a nested namespace.\n>\n> Signed-off-by: Rui Wang \n> ---\n> src/ipa/rkisp1/algorithms/dpf.cpp | 240 ++++++++++++++++--------------\n> 1 file changed, 125 insertions(+), 115 deletions(-)\n>\n> diff --git a/src/ipa/rkisp1/algorithms/dpf.cpp b/src/ipa/rkisp1/algorithms/dpf.cpp\n> index dea04cc3..68a94afe 100644\n> --- a/src/ipa/rkisp1/algorithms/dpf.cpp\n> +++ b/src/ipa/rkisp1/algorithms/dpf.cpp\n> @@ -23,107 +23,31 @@\n>\n> namespace libcamera {\n>\n> -namespace ipa::rkisp1::algorithms {\n> -\n> -/**\n> - * \\class Dpf\n> - * \\brief RkISP1 Denoise Pre-Filter control\n> - *\n> - * The denoise pre-filter algorithm is a bilateral filter which combines a\n> - * range filter and a domain filter. The denoise pre-filter is applied before\n> - * demosaicing.\n> - */\n> -\n> -LOG_DEFINE_CATEGORY(RkISP1Dpf)\n> -\n> -Dpf::Dpf()\n> -\t: config_({}), strengthConfig_({}),\n> -\t noiseReductionModes_({}),\n> -\t runningMode_(controls::draft::NoiseReductionModeOff)\n> -{\n> -}\n> -\n> -/**\n> - * \\copydoc libcamera::ipa::Algorithm::init\n> - */\n> -int Dpf::init([[maybe_unused]] IPAContext &context,\n> -\t const YamlObject &tuningData)\n> -{\n> -\t/* Parse tuning block. */\n> -\tif (!parseConfig(tuningData)) {\n> -\t\treturn -EINVAL;\n> -\t}\n> -\n> -\treturn 0;\n> -}\n> -\n> -bool Dpf::parseConfig(const YamlObject &tuningData)\n> +template<>\n> +std::optional\n> +YamlObject::Getter::get(const YamlObject &obj) const\n> {\n> -\t/* Parse base config. */\n> -\tif (!parseSingleConfig(tuningData, config_, strengthConfig_)) {\n> -\t\treturn false;\n> -\t}\n> +\trkisp1_cif_isp_dpf_strength_config config = {};\n>\n> -\t/* Parse modes. */\n> -\tif (!parseModes(tuningData)) {\n> -\t\treturn false;\n> -\t}\n> +\tconfig.r = obj[\"r\"].get().value_or(64);\n> +\tconfig.g = obj[\"g\"].get().value_or(64);\n> +\tconfig.b = obj[\"b\"].get().value_or(64);\n>\n> -\treturn true;\n> +\treturn config;\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> -\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> -\t\tmode.modeValue = modeValue;\n> -\t\tif (!parseSingleConfig(entry, mode.dpf, mode.strength)) {\n> -\t\t\treturn false;\n> -\t\t}\n> -\t\tnoiseReductionModes_.push_back(mode);\n> -\t}\n> -\n> -\treturn true;\n> -}\n> -\n> -bool Dpf::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> +template<>\n> +std::optional\n> +YamlObject::Getter::get(const YamlObject &obj) const\n> {\n> +\trkisp1_cif_isp_dpf_config config = {};\n> \tstd::vector values;\n>\n> \t/*\n> \t * The domain kernel is configured with a 9x9 kernel for the green\n> \t * pixels, and a 13x9 or 9x9 kernel for red and blue pixels.\n> \t */\n> -\tconst YamlObject &dFObject = tuningData[\"filter\"];\n> +\tconst YamlObject &dFObject = obj[\"filter\"];\n>\n> \t/*\n> \t * For the green component, we have the 9x9 kernel specified\n> @@ -144,11 +68,7 @@ bool Dpf::parseSingleConfig(const YamlObject &tuningData,\n> \t */\n> \tvalues = dFObject[\"g\"].getList().value_or(std::vector{});\n> \tif (values.size() != RKISP1_CIF_ISP_DPF_MAX_SPATIAL_COEFFS) {\n> -\t\tLOG(RkISP1Dpf, Error)\n> -\t\t\t<< \"Invalid 'filter:g': expected \"\n> -\t\t\t<< RKISP1_CIF_ISP_DPF_MAX_SPATIAL_COEFFS\n> -\t\t\t<< \" elements, got \" << values.size();\n> -\t\treturn false;\n> +\t\treturn std::nullopt;\n> \t}\n>\n> \tstd::copy_n(values.begin(), values.size(),\n> @@ -181,12 +101,7 @@ bool Dpf::parseSingleConfig(const YamlObject &tuningData,\n> \tvalues = dFObject[\"rb\"].getList().value_or(std::vector{});\n> \tif (values.size() != RKISP1_CIF_ISP_DPF_MAX_SPATIAL_COEFFS &&\n> \t values.size() != RKISP1_CIF_ISP_DPF_MAX_SPATIAL_COEFFS - 1) {\n> -\t\tLOG(RkISP1Dpf, Error)\n> -\t\t\t<< \"Invalid 'filter:rb': expected \"\n> -\t\t\t<< RKISP1_CIF_ISP_DPF_MAX_SPATIAL_COEFFS - 1\n> -\t\t\t<< \" or \" << RKISP1_CIF_ISP_DPF_MAX_SPATIAL_COEFFS\n> -\t\t\t<< \" elements, got \" << values.size();\n> -\t\treturn false;\n> +\t\treturn std::nullopt;\n> \t}\n>\n> \tconfig.rb_flt.fltsize = values.size() == RKISP1_CIF_ISP_DPF_MAX_SPATIAL_COEFFS\n> @@ -204,39 +119,134 @@ bool Dpf::parseSingleConfig(const YamlObject &tuningData,\n> \t * which stores a piecewise linear function that characterizes the\n> \t * sensor noise profile as a noise level function curve (NLF).\n> \t */\n> -\tconst YamlObject &rFObject = tuningData[\"nll\"];\n> +\tconst YamlObject &rFObject = obj[\"nll\"];\n>\n> \tstd::vector nllValues;\n> \tnllValues = rFObject[\"coeff\"].getList().value_or(std::vector{});\n> \tif (nllValues.size() != RKISP1_CIF_ISP_DPF_MAX_NLF_COEFFS) {\n> -\t\tLOG(RkISP1Dpf, Error)\n> -\t\t\t<< \"Invalid 'nll:coeff': expected \"\n> -\t\t\t<< RKISP1_CIF_ISP_DPF_MAX_NLF_COEFFS\n> -\t\t\t<< \" elements, got \" << nllValues.size();\n> -\t\treturn false;\n> +\t\treturn std::nullopt;\n> \t}\n>\n> \tstd::copy_n(nllValues.begin(), nllValues.size(),\n> \t\t std::begin(config.nll.coeff));\n>\n> -\tstd::string scaleMode = rFObject[\"scale-mode\"].get(\"\");\n> +\tstd::string scaleMode = rFObject[\"scale-mode\"].get().value_or(\"\");\n> \tif (scaleMode == \"linear\") {\n> \t\tconfig.nll.scale_mode = RKISP1_CIF_ISP_NLL_SCALE_LINEAR;\n> \t} else if (scaleMode == \"logarithmic\") {\n> \t\tconfig.nll.scale_mode = RKISP1_CIF_ISP_NLL_SCALE_LOGARITHMIC;\n> \t} else {\n> -\t\tLOG(RkISP1Dpf, Error)\n> -\t\t\t<< \"Invalid 'nll:scale-mode': expected \"\n> -\t\t\t<< \"'linear' or 'logarithmic' value, got \"\n> -\t\t\t<< scaleMode;\n> +\t\treturn std::nullopt;\n> +\t}\n> +\n> +\treturn config;\n> +}\n> +\n> +namespace ipa::rkisp1::algorithms {\n> +\n> +/**\n> + * \\class Dpf\n> + * \\brief RkISP1 Denoise Pre-Filter control\n> + *\n> + * The denoise pre-filter algorithm is a bilateral filter which combines a\n> + * range filter and a domain filter. The denoise pre-filter is applied before\n> + * demosaicing.\n> + */\n> +\n> +LOG_DEFINE_CATEGORY(RkISP1Dpf)\n> +\n> +Dpf::Dpf()\n> +\t: config_({}), strengthConfig_({}),\n> +\t noiseReductionModes_({}),\n> +\t runningMode_(controls::draft::NoiseReductionModeOff)\n> +{\n> +}\n> +\n> +/**\n> + * \\copydoc libcamera::ipa::Algorithm::init\n> + */\n> +int Dpf::init([[maybe_unused]] IPAContext &context,\n> +\t const YamlObject &tuningData)\n> +{\n> +\t/* Parse tuning block. */\n> +\tif (!parseConfig(tuningData)) {\n> +\t\treturn -EINVAL;\n> +\t}\n> +\n> +\treturn 0;\n> +}\n> +\n> +bool Dpf::parseConfig(const YamlObject &tuningData)\n> +{\n> +\t/* Parse base config. */\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> -\tconst YamlObject &fSObject = tuningData[\"strength\"];\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> +\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> +\t\tmode.modeValue = modeValue;\n> +\t\tif (!parseSingleConfig(entry, mode.dpf, mode.strength)) {\n> +\t\t\treturn false;\n> +\t\t}\n> +\t\tnoiseReductionModes_.push_back(mode);\n> +\t}\n> +\n> +\treturn true;\n> +}\n> +\n> +bool Dpf::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> +\tauto dpfConfig = tuningData.get();\n> +\tif (!dpfConfig)\n> +\t\treturn false;\n> +\n> +\tconfig = *dpfConfig;\n> +\n> +\tauto strength = tuningData[\"strength\"].get();\n> +\tif (!strength)\n> +\t\treturn false;\n>\n> -\tstrengthConfig.r = fSObject[\"r\"].get().value_or(64);\n> -\tstrengthConfig.g = fSObject[\"g\"].get().value_or(64);\n> -\tstrengthConfig.b = fSObject[\"b\"].get().value_or(64);\n> +\tstrengthConfig = *strength;\n\nI guess we can live with the additional copies here, for such a much\nnicer code!\n\nReviewed-by: Jacopo Mondi \n\nThanks\n j\n>\n> \treturn true;\n> }\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 64064C3257\n\tfor ;\n\tThu, 11 Dec 2025 15:56:24 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 840F161612;\n\tThu, 11 Dec 2025 16:56:23 +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 ED7B161603\n\tfor ;\n\tThu, 11 Dec 2025 16:56:21 +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 5E800667;\n\tThu, 11 Dec 2025 16:56:19 +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=\"DL1DzfFR\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1765468579;\n\tbh=0HzREuRXLFRXLW4gPCfM4adHzONpOocZ2E7rR207YSk=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=DL1DzfFR9LlbWlChnpeVvUR1KzRghXt8LjyUYM5DwLfwxjhsJkScab5wX9QVNcS6z\n\tE3JPs5+kEozEGUHw/CeQfY8GoonzIB5ilwyglako3VlXGhN75dqDOjXzEvy86r5vlM\n\tOnvn8C6vrrvJKvN8mKQ9qRoG7oFQdQgJvEcff0wM=","Date":"Thu, 11 Dec 2025 16:56:18 +0100","From":"Jacopo Mondi ","To":"Rui Wang ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v4 5/7] ipa: rkisp1: algorithms: dpf: Use\n\tYamlObject::Getter for parsing","Message-ID":"","References":"<20251208004808.1274417-1-rui.wang@ideasonboard.com>\n\t<20251208004808.1274417-6-rui.wang@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20251208004808.1274417-6-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\" "}}]