[{"id":37685,"web_url":"https://patchwork.libcamera.org/comment/37685/","msgid":"","date":"2026-01-16T08:56:09","subject":"Re: [PATCH v8 1/7] ipa: rkisp1: algorithms: dpf: refactor DPF\n\tparsing and initialization","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Rui\n\nOn Thu, Jan 15, 2026 at 11:33:12AM -0500, Rui Wang wrote:\n> Split DPF configuration parsing and initialization into clearer,\n> self-contained helpers and modernized initialization patterns.\n>\n> Introduce parseSingleConfig() as DPF tuning file parser helper\n> make future extensions and maintenance easier.\n>\n> Change strengthconfig.r/g/b parser from uint16 to uint8\n> to match rkisp1_cif_isp_dpf_strength_config definition.\n>\n> Signed-off-by: Rui Wang \n> Reviewed-by: Jacopo Mondi \n>\n> ---\n>\n> Changes since v5:\n> - Update typo in commit message\n> No change since v6:\n>\n> Reviewed-by tags from v5 are carried over (no code changes).\n> ---\n> src/ipa/rkisp1/algorithms/dpf.cpp | 52 ++++++++++++++++++++++---------\n> src/ipa/rkisp1/algorithms/dpf.h | 5 +++\n> 2 files changed, 42 insertions(+), 15 deletions(-)\n>\n> diff --git a/src/ipa/rkisp1/algorithms/dpf.cpp b/src/ipa/rkisp1/algorithms/dpf.cpp\n> index 39f3e461..dd3effa1 100644\n> --- a/src/ipa/rkisp1/algorithms/dpf.cpp\n> +++ b/src/ipa/rkisp1/algorithms/dpf.cpp\n> @@ -46,6 +46,28 @@ Dpf::Dpf()\n> */\n> int Dpf::init([[maybe_unused]] IPAContext &context,\n> \t const YamlObject &tuningData)\n> +{\n> +\t/* Parse tuning block. */\n> +\tint ret = parseConfig(tuningData);\n> +\tif (ret)\n> +\t\treturn ret;\n> +\n> +\treturn 0;\n> +}\n> +\n> +int Dpf::parseConfig(const YamlObject &tuningData)\n\nThis function doesn't serve any purpose, it only calls\nparseSingleConfig() and in the next patch it will only call\nparseModes().\n\nCan't you call these functions in init() without this additional\nindirection ?\n\nOr better, can you skip this patch completely ? If (and only if) I\nunderstand the process correctly, this patch still uses the \"old\"\ntuning format, while the next one moves to the \"new\" format where only\n'modes' are specified.\n\nCan we skip this interim patch and move to the new format directly ?\nWe don't have any obbligation towards legacy tuning file and they\nwon't work anyway after this series is applied.\n\nSo, make it clear in the commit message of the next patch that the\ntuning file layout has changed and 'modes' are now mandatory and there\nis no \"default\"/\"generic\" configuration of dpf.\n\nWhat do you think ?\n\n\n> +{\n> +\t/* Parse base config. */\n> +\tint ret = parseSingleConfig(tuningData, config_, strengthConfig_);\n> +\tif (ret)\n> +\t\treturn ret;\n> +\n> +\treturn 0;\n> +}\n> +\n> +int 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> \tstd::vector values;\n>\n> @@ -82,10 +104,10 @@ int Dpf::init([[maybe_unused]] IPAContext &context,\n> \t}\n>\n> \tstd::copy_n(values.begin(), values.size(),\n> -\t\t std::begin(config_.g_flt.spatial_coeff));\n> +\t\t std::begin(config.g_flt.spatial_coeff));\n>\n> -\tconfig_.g_flt.gr_enable = true;\n> -\tconfig_.g_flt.gb_enable = true;\n> +\tconfig.g_flt.gr_enable = true;\n> +\tconfig.g_flt.gb_enable = true;\n>\n> \t/*\n> \t * For the red and blue components, we have the 13x9 kernel specified\n> @@ -119,15 +141,15 @@ int Dpf::init([[maybe_unused]] IPAContext &context,\n> \t\treturn -EINVAL;\n> \t}\n>\n> -\tconfig_.rb_flt.fltsize = values.size() == RKISP1_CIF_ISP_DPF_MAX_SPATIAL_COEFFS\n> -\t\t\t ? RKISP1_CIF_ISP_DPF_RB_FILTERSIZE_13x9\n> -\t\t\t : RKISP1_CIF_ISP_DPF_RB_FILTERSIZE_9x9;\n> +\tconfig.rb_flt.fltsize = values.size() == RKISP1_CIF_ISP_DPF_MAX_SPATIAL_COEFFS\n> +\t\t\t\t\t? RKISP1_CIF_ISP_DPF_RB_FILTERSIZE_13x9\n> +\t\t\t\t\t: RKISP1_CIF_ISP_DPF_RB_FILTERSIZE_9x9;\n>\n> \tstd::copy_n(values.begin(), values.size(),\n> -\t\t std::begin(config_.rb_flt.spatial_coeff));\n> +\t\t std::begin(config.rb_flt.spatial_coeff));\n>\n> -\tconfig_.rb_flt.r_enable = true;\n> -\tconfig_.rb_flt.b_enable = true;\n> +\tconfig.rb_flt.r_enable = true;\n> +\tconfig.rb_flt.b_enable = true;\n>\n> \t/*\n> \t * The range kernel is configured with a noise level lookup table (NLL)\n> @@ -147,13 +169,13 @@ int Dpf::init([[maybe_unused]] IPAContext &context,\n> \t}\n>\n> \tstd::copy_n(nllValues.begin(), nllValues.size(),\n> -\t\t std::begin(config_.nll.coeff));\n> +\t\t std::begin(config.nll.coeff));\n>\n> \tstd::string scaleMode = rFObject[\"scale-mode\"].get(\"\");\n> \tif (scaleMode == \"linear\") {\n> -\t\tconfig_.nll.scale_mode = RKISP1_CIF_ISP_NLL_SCALE_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\tconfig.nll.scale_mode = RKISP1_CIF_ISP_NLL_SCALE_LOGARITHMIC;\n> \t} else {\n> \t\tLOG(RkISP1Dpf, Error)\n> \t\t\t<< \"Invalid 'RangeFilter:scale-mode': expected \"\n> @@ -164,9 +186,9 @@ int Dpf::init([[maybe_unused]] IPAContext &context,\n>\n> \tconst YamlObject &fSObject = tuningData[\"FilterStrength\"];\n>\n> -\tstrengthConfig_.r = fSObject[\"r\"].get(64);\n> -\tstrengthConfig_.g = fSObject[\"g\"].get(64);\n> -\tstrengthConfig_.b = fSObject[\"b\"].get(64);\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>\n> \treturn 0;\n> }\n> diff --git a/src/ipa/rkisp1/algorithms/dpf.h b/src/ipa/rkisp1/algorithms/dpf.h\n> index 2dd8cd36..39186c55 100644\n> --- a/src/ipa/rkisp1/algorithms/dpf.h\n> +++ b/src/ipa/rkisp1/algorithms/dpf.h\n> @@ -30,6 +30,11 @@ public:\n> \t\t RkISP1Params *params) override;\n>\n> private:\n> +\tint parseConfig(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> \tstruct rkisp1_cif_isp_dpf_config config_;\n> \tstruct rkisp1_cif_isp_dpf_strength_config strengthConfig_;\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 DC787BDCBF\n\tfor ;\n\tFri, 16 Jan 2026 08:56:15 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0FE2761FA3;\n\tFri, 16 Jan 2026 09:56:15 +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 572C661A35\n\tfor ;\n\tFri, 16 Jan 2026 09:56:13 +0100 (CET)","from ideasonboard.com (unknown\n\t[IPv6:2001:b07:6462:5de2:520d:d7a3:63ca:99e8])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id D3D913D7;\n\tFri, 16 Jan 2026 09:55:44 +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=\"baP32Nnl\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1768553745;\n\tbh=SZEekmhaaVKHVcRUTm1S8Iyl+j/VV+6zpnL5tW0Li/g=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=baP32NnlSTgLjSenWgJjBQqLzs2KReC/NkNavTQ/oc/FBcH3+3+3pqHglv1L5FvX8\n\trLthzF6BS/w4zTzof1NSoDXqHj0Qz04EARHOnSWE2/zpr+SUNgGaLQQrHI1sS3SbMv\n\ttzVEivo99bZik8kEd3VTi5Y7KeouzkxQsK2+eaOQ=","Date":"Fri, 16 Jan 2026 09:56:09 +0100","From":"Jacopo Mondi ","To":"Rui Wang ","Cc":"libcamera-devel@lists.libcamera.org, \n\tJacopo Mondi ","Subject":"Re: [PATCH v8 1/7] ipa: rkisp1: algorithms: dpf: refactor DPF\n\tparsing and initialization","Message-ID":"","References":"<20260115163318.1339354-1-rui.wang@ideasonboard.com>\n\t<20260115163318.1339354-2-rui.wang@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20260115163318.1339354-2-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":37703,"web_url":"https://patchwork.libcamera.org/comment/37703/","msgid":"<176858580477.1426120.12058434362814772305@rui-Precision-7560.local>","date":"2026-01-16T17:50:04","subject":"Re: [PATCH v8 1/7] ipa: rkisp1: algorithms: dpf: refactor DPF\n\tparsing and initialization","submitter":{"id":241,"url":"https://patchwork.libcamera.org/api/people/241/","name":"Rui Wang","email":"rui.wang@ideasonboard.com"},"content":"Quoting Jacopo Mondi (2026-01-16 03:56:09)\n> Hi Rui\n> \n> On Thu, Jan 15, 2026 at 11:33:12AM -0500, Rui Wang wrote:\n> > Split DPF configuration parsing and initialization into clearer,\n> > self-contained helpers and modernized initialization patterns.\n> >\n> > Introduce parseSingleConfig() as DPF tuning file parser helper\n> > make future extensions and maintenance easier.\n> >\n> > Change strengthconfig.r/g/b parser from uint16 to uint8\n> > to match rkisp1_cif_isp_dpf_strength_config definition.\n> >\n> > Signed-off-by: Rui Wang \n> > Reviewed-by: Jacopo Mondi \n> >\n> > ---\n> >\n> > Changes since v5:\n> > - Update typo in commit message\n> > No change since v6:\n> >\n> > Reviewed-by tags from v5 are carried over (no code changes).\n> > ---\n> > src/ipa/rkisp1/algorithms/dpf.cpp | 52 ++++++++++++++++++++++---------\n> > src/ipa/rkisp1/algorithms/dpf.h | 5 +++\n> > 2 files changed, 42 insertions(+), 15 deletions(-)\n> >\n> > diff --git a/src/ipa/rkisp1/algorithms/dpf.cpp b/src/ipa/rkisp1/algorithms/dpf.cpp\n> > index 39f3e461..dd3effa1 100644\n> > --- a/src/ipa/rkisp1/algorithms/dpf.cpp\n> > +++ b/src/ipa/rkisp1/algorithms/dpf.cpp\n> > @@ -46,6 +46,28 @@ Dpf::Dpf()\n> > */\n> > int Dpf::init([[maybe_unused]] IPAContext &context,\n> > const YamlObject &tuningData)\n> > +{\n> > + /* Parse tuning block. */\n> > + int ret = parseConfig(tuningData);\n> > + if (ret)\n> > + return ret;\n> > +\n> > + return 0;\n> > +}\n> > +\n> > +int Dpf::parseConfig(const YamlObject &tuningData)\n> \n> This function doesn't serve any purpose, it only calls\n> parseSingleConfig() and in the next patch it will only call\n> parseModes().\n> \n> Can't you call these functions in init() without this additional\n> indirection ?\n\nThanks Jacopo,\n Yes , init->parseConfig->parseMode->parseSingleConfig ,it can be improved\n to reduce parseConfig. will update it in v9 patch series\n\n> \n> Or better, can you skip this patch completely ? If (and only if) I\n> understand the process correctly, this patch still uses the \"old\"\n> tuning format, while the next one moves to the \"new\" format where only\n> 'modes' are specified.\n> \n> Can we skip this interim patch and move to the new format directly ?\n> We don't have any obbligation towards legacy tuning file and they\n> won't work anyway after this series is applied.\n> \n> So, make it clear in the commit message of the next patch that the\n> tuning file layout has changed and 'modes' are now mandatory and there\n> is no \"default\"/\"generic\" configuration of dpf.\n> \n> What do you think ?\n\n\"default\" mode looks more reasonable as current design.\nbut my thought is for introducing \"auto\" mode as default in next PR.\nif this \"default\" add into current modes , it will be removed in next PR,\nand currently master by default setting is \"off\". it can be edit from \"control\".\nand highquality/minimal/zsl.. can also be selected as default mode once enable dpf\nTo highlight enable/disabel epf as default will introduce \n- enableMode : 'minimal'\nin v9 patch series\n\n> \n> \n> > +{\n> > + /* Parse base config. */\n> > + int ret = parseSingleConfig(tuningData, config_, strengthConfig_);\n> > + if (ret)\n> > + return ret;\n> > +\n> > + return 0;\n> > +}\n> > +\n> > +int Dpf::parseSingleConfig(const YamlObject &tuningData,\n> > + rkisp1_cif_isp_dpf_config &config,\n> > + rkisp1_cif_isp_dpf_strength_config &strengthConfig)\n> > {\n> > std::vector values;\n> >\n> > @@ -82,10 +104,10 @@ int Dpf::init([[maybe_unused]] IPAContext &context,\n> > }\n> >\n> > std::copy_n(values.begin(), values.size(),\n> > - std::begin(config_.g_flt.spatial_coeff));\n> > + std::begin(config.g_flt.spatial_coeff));\n> >\n> > - config_.g_flt.gr_enable = true;\n> > - config_.g_flt.gb_enable = true;\n> > + config.g_flt.gr_enable = true;\n> > + config.g_flt.gb_enable = true;\n> >\n> > /*\n> > * For the red and blue components, we have the 13x9 kernel specified\n> > @@ -119,15 +141,15 @@ int Dpf::init([[maybe_unused]] IPAContext &context,\n> > return -EINVAL;\n> > }\n> >\n> > - config_.rb_flt.fltsize = values.size() == RKISP1_CIF_ISP_DPF_MAX_SPATIAL_COEFFS\n> > - ? RKISP1_CIF_ISP_DPF_RB_FILTERSIZE_13x9\n> > - : RKISP1_CIF_ISP_DPF_RB_FILTERSIZE_9x9;\n> > + config.rb_flt.fltsize = values.size() == RKISP1_CIF_ISP_DPF_MAX_SPATIAL_COEFFS\n> > + ? RKISP1_CIF_ISP_DPF_RB_FILTERSIZE_13x9\n> > + : RKISP1_CIF_ISP_DPF_RB_FILTERSIZE_9x9;\n> >\n> > std::copy_n(values.begin(), values.size(),\n> > - std::begin(config_.rb_flt.spatial_coeff));\n> > + std::begin(config.rb_flt.spatial_coeff));\n> >\n> > - config_.rb_flt.r_enable = true;\n> > - config_.rb_flt.b_enable = true;\n> > + config.rb_flt.r_enable = true;\n> > + config.rb_flt.b_enable = true;\n> >\n> > /*\n> > * The range kernel is configured with a noise level lookup table (NLL)\n> > @@ -147,13 +169,13 @@ int Dpf::init([[maybe_unused]] IPAContext &context,\n> > }\n> >\n> > std::copy_n(nllValues.begin(), nllValues.size(),\n> > - std::begin(config_.nll.coeff));\n> > + std::begin(config.nll.coeff));\n> >\n> > std::string scaleMode = rFObject[\"scale-mode\"].get(\"\");\n> > if (scaleMode == \"linear\") {\n> > - config_.nll.scale_mode = RKISP1_CIF_ISP_NLL_SCALE_LINEAR;\n> > + config.nll.scale_mode = RKISP1_CIF_ISP_NLL_SCALE_LINEAR;\n> > } else if (scaleMode == \"logarithmic\") {\n> > - config_.nll.scale_mode = RKISP1_CIF_ISP_NLL_SCALE_LOGARITHMIC;\n> > + config.nll.scale_mode = RKISP1_CIF_ISP_NLL_SCALE_LOGARITHMIC;\n> > } else {\n> > LOG(RkISP1Dpf, Error)\n> > << \"Invalid 'RangeFilter:scale-mode': expected \"\n> > @@ -164,9 +186,9 @@ int Dpf::init([[maybe_unused]] IPAContext &context,\n> >\n> > const YamlObject &fSObject = tuningData[\"FilterStrength\"];\n> >\n> > - strengthConfig_.r = fSObject[\"r\"].get(64);\n> > - strengthConfig_.g = fSObject[\"g\"].get(64);\n> > - strengthConfig_.b = fSObject[\"b\"].get(64);\n> > + strengthConfig.r = fSObject[\"r\"].get().value_or(64);\n> > + strengthConfig.g = fSObject[\"g\"].get().value_or(64);\n> > + strengthConfig.b = fSObject[\"b\"].get().value_or(64);\n> >\n> > return 0;\n> > }\n> > diff --git a/src/ipa/rkisp1/algorithms/dpf.h b/src/ipa/rkisp1/algorithms/dpf.h\n> > index 2dd8cd36..39186c55 100644\n> > --- a/src/ipa/rkisp1/algorithms/dpf.h\n> > +++ b/src/ipa/rkisp1/algorithms/dpf.h\n> > @@ -30,6 +30,11 @@ public:\n> > RkISP1Params *params) override;\n> >\n> > private:\n> > + int parseConfig(const YamlObject &tuningData);\n> > + int parseSingleConfig(const YamlObject &tuningData,\n> > + rkisp1_cif_isp_dpf_config &config,\n> > + rkisp1_cif_isp_dpf_strength_config &strengthConfig);\n> > +\n> > struct rkisp1_cif_isp_dpf_config config_;\n> > struct rkisp1_cif_isp_dpf_strength_config strengthConfig_;\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 6E381BDCBF\n\tfor ;\n\tFri, 16 Jan 2026 17:50:20 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8A03D61FBC;\n\tFri, 16 Jan 2026 18:50:19 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 9A0AD615B2\n\tfor ;\n\tFri, 16 Jan 2026 18:50:18 +0100 (CET)","from pyrite.rasen.tech (unknown [209.216.103.65])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id DA8E7352;\n\tFri, 16 Jan 2026 18:49:49 +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=\"NN6L6fHx\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1768585790;\n\tbh=fPNkTD6XYVjYHxiYYE2xdK0mlmQCOGzgK8YzyG/e9r0=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=NN6L6fHxBgT6y1J9IxWJ6KHtF0d+Qxfa89pSdQx1z8PyFZDpxbkYHx8AYeiNu7EsZ\n\tNmtbnLR7sBdcDID04aVEtXYn+BEXulimGbJEfsJRrCCUdIYxuxNbC/TawL6YEhLfN8\n\tkew0/k3QjsGonAZt195shDNmQU/v4VIoWEqjJvzA=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"","References":"<20260115163318.1339354-1-rui.wang@ideasonboard.com>\n\t<20260115163318.1339354-2-rui.wang@ideasonboard.com>\n\t","Subject":"Re: [PATCH v8 1/7] ipa: rkisp1: algorithms: dpf: refactor DPF\n\tparsing and initialization","From":"Rui Wang ","Cc":"libcamera-devel@lists.libcamera.org,\n\tJacopo Mondi ","To":"Jacopo Mondi ","Date":"Fri, 16 Jan 2026 12:50:04 -0500","Message-ID":"<176858580477.1426120.12058434362814772305@rui-Precision-7560.local>","User-Agent":"alot/0.12","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\" "}}]