| Message ID | 20260115163318.1339354-2-rui.wang@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Rui On Thu, Jan 15, 2026 at 11:33:12AM -0500, Rui Wang wrote: > Split DPF configuration parsing and initialization into clearer, > self-contained helpers and modernized initialization patterns. > > Introduce parseSingleConfig() as DPF tuning file parser helper > make future extensions and maintenance easier. > > Change strengthconfig.r/g/b parser from uint16 to uint8 > to match rkisp1_cif_isp_dpf_strength_config definition. > > Signed-off-by: Rui Wang <rui.wang@ideasonboard.com> > Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > > --- > > Changes since v5: > - Update typo in commit message > No change since v6: > > Reviewed-by tags from v5 are carried over (no code changes). > --- > src/ipa/rkisp1/algorithms/dpf.cpp | 52 ++++++++++++++++++++++--------- > src/ipa/rkisp1/algorithms/dpf.h | 5 +++ > 2 files changed, 42 insertions(+), 15 deletions(-) > > diff --git a/src/ipa/rkisp1/algorithms/dpf.cpp b/src/ipa/rkisp1/algorithms/dpf.cpp > index 39f3e461..dd3effa1 100644 > --- a/src/ipa/rkisp1/algorithms/dpf.cpp > +++ b/src/ipa/rkisp1/algorithms/dpf.cpp > @@ -46,6 +46,28 @@ Dpf::Dpf() > */ > int Dpf::init([[maybe_unused]] IPAContext &context, > const YamlObject &tuningData) > +{ > + /* Parse tuning block. */ > + int ret = parseConfig(tuningData); > + if (ret) > + return ret; > + > + return 0; > +} > + > +int Dpf::parseConfig(const YamlObject &tuningData) This function doesn't serve any purpose, it only calls parseSingleConfig() and in the next patch it will only call parseModes(). Can't you call these functions in init() without this additional indirection ? Or better, can you skip this patch completely ? If (and only if) I understand the process correctly, this patch still uses the "old" tuning format, while the next one moves to the "new" format where only 'modes' are specified. Can we skip this interim patch and move to the new format directly ? We don't have any obbligation towards legacy tuning file and they won't work anyway after this series is applied. So, make it clear in the commit message of the next patch that the tuning file layout has changed and 'modes' are now mandatory and there is no "default"/"generic" configuration of dpf. What do you think ? > +{ > + /* Parse base config. */ > + int ret = parseSingleConfig(tuningData, config_, strengthConfig_); > + if (ret) > + return ret; > + > + return 0; > +} > + > +int Dpf::parseSingleConfig(const YamlObject &tuningData, > + rkisp1_cif_isp_dpf_config &config, > + rkisp1_cif_isp_dpf_strength_config &strengthConfig) > { > std::vector<uint8_t> values; > > @@ -82,10 +104,10 @@ int Dpf::init([[maybe_unused]] IPAContext &context, > } > > std::copy_n(values.begin(), values.size(), > - std::begin(config_.g_flt.spatial_coeff)); > + std::begin(config.g_flt.spatial_coeff)); > > - config_.g_flt.gr_enable = true; > - config_.g_flt.gb_enable = true; > + config.g_flt.gr_enable = true; > + config.g_flt.gb_enable = true; > > /* > * For the red and blue components, we have the 13x9 kernel specified > @@ -119,15 +141,15 @@ int Dpf::init([[maybe_unused]] IPAContext &context, > return -EINVAL; > } > > - config_.rb_flt.fltsize = values.size() == RKISP1_CIF_ISP_DPF_MAX_SPATIAL_COEFFS > - ? RKISP1_CIF_ISP_DPF_RB_FILTERSIZE_13x9 > - : RKISP1_CIF_ISP_DPF_RB_FILTERSIZE_9x9; > + config.rb_flt.fltsize = values.size() == RKISP1_CIF_ISP_DPF_MAX_SPATIAL_COEFFS > + ? RKISP1_CIF_ISP_DPF_RB_FILTERSIZE_13x9 > + : RKISP1_CIF_ISP_DPF_RB_FILTERSIZE_9x9; > > std::copy_n(values.begin(), values.size(), > - std::begin(config_.rb_flt.spatial_coeff)); > + std::begin(config.rb_flt.spatial_coeff)); > > - config_.rb_flt.r_enable = true; > - config_.rb_flt.b_enable = true; > + config.rb_flt.r_enable = true; > + config.rb_flt.b_enable = true; > > /* > * The range kernel is configured with a noise level lookup table (NLL) > @@ -147,13 +169,13 @@ int Dpf::init([[maybe_unused]] IPAContext &context, > } > > std::copy_n(nllValues.begin(), nllValues.size(), > - std::begin(config_.nll.coeff)); > + std::begin(config.nll.coeff)); > > std::string scaleMode = rFObject["scale-mode"].get<std::string>(""); > if (scaleMode == "linear") { > - config_.nll.scale_mode = RKISP1_CIF_ISP_NLL_SCALE_LINEAR; > + config.nll.scale_mode = RKISP1_CIF_ISP_NLL_SCALE_LINEAR; > } else if (scaleMode == "logarithmic") { > - config_.nll.scale_mode = RKISP1_CIF_ISP_NLL_SCALE_LOGARITHMIC; > + config.nll.scale_mode = RKISP1_CIF_ISP_NLL_SCALE_LOGARITHMIC; > } else { > LOG(RkISP1Dpf, Error) > << "Invalid 'RangeFilter:scale-mode': expected " > @@ -164,9 +186,9 @@ int Dpf::init([[maybe_unused]] IPAContext &context, > > const YamlObject &fSObject = tuningData["FilterStrength"]; > > - strengthConfig_.r = fSObject["r"].get<uint16_t>(64); > - strengthConfig_.g = fSObject["g"].get<uint16_t>(64); > - strengthConfig_.b = fSObject["b"].get<uint16_t>(64); > + strengthConfig.r = fSObject["r"].get<uint8_t>().value_or(64); > + strengthConfig.g = fSObject["g"].get<uint8_t>().value_or(64); > + strengthConfig.b = fSObject["b"].get<uint8_t>().value_or(64); > > return 0; > } > diff --git a/src/ipa/rkisp1/algorithms/dpf.h b/src/ipa/rkisp1/algorithms/dpf.h > index 2dd8cd36..39186c55 100644 > --- a/src/ipa/rkisp1/algorithms/dpf.h > +++ b/src/ipa/rkisp1/algorithms/dpf.h > @@ -30,6 +30,11 @@ public: > RkISP1Params *params) override; > > private: > + int parseConfig(const YamlObject &tuningData); > + int parseSingleConfig(const YamlObject &tuningData, > + rkisp1_cif_isp_dpf_config &config, > + rkisp1_cif_isp_dpf_strength_config &strengthConfig); > + > struct rkisp1_cif_isp_dpf_config config_; > struct rkisp1_cif_isp_dpf_strength_config strengthConfig_; > }; > -- > 2.43.0 >
Quoting Jacopo Mondi (2026-01-16 03:56:09) > Hi Rui > > On Thu, Jan 15, 2026 at 11:33:12AM -0500, Rui Wang wrote: > > Split DPF configuration parsing and initialization into clearer, > > self-contained helpers and modernized initialization patterns. > > > > Introduce parseSingleConfig() as DPF tuning file parser helper > > make future extensions and maintenance easier. > > > > Change strengthconfig.r/g/b parser from uint16 to uint8 > > to match rkisp1_cif_isp_dpf_strength_config definition. > > > > Signed-off-by: Rui Wang <rui.wang@ideasonboard.com> > > Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > > > > --- > > > > Changes since v5: > > - Update typo in commit message > > No change since v6: > > > > Reviewed-by tags from v5 are carried over (no code changes). > > --- > > src/ipa/rkisp1/algorithms/dpf.cpp | 52 ++++++++++++++++++++++--------- > > src/ipa/rkisp1/algorithms/dpf.h | 5 +++ > > 2 files changed, 42 insertions(+), 15 deletions(-) > > > > diff --git a/src/ipa/rkisp1/algorithms/dpf.cpp b/src/ipa/rkisp1/algorithms/dpf.cpp > > index 39f3e461..dd3effa1 100644 > > --- a/src/ipa/rkisp1/algorithms/dpf.cpp > > +++ b/src/ipa/rkisp1/algorithms/dpf.cpp > > @@ -46,6 +46,28 @@ Dpf::Dpf() > > */ > > int Dpf::init([[maybe_unused]] IPAContext &context, > > const YamlObject &tuningData) > > +{ > > + /* Parse tuning block. */ > > + int ret = parseConfig(tuningData); > > + if (ret) > > + return ret; > > + > > + return 0; > > +} > > + > > +int Dpf::parseConfig(const YamlObject &tuningData) > > This function doesn't serve any purpose, it only calls > parseSingleConfig() and in the next patch it will only call > parseModes(). > > Can't you call these functions in init() without this additional > indirection ? Thanks Jacopo, Yes , init->parseConfig->parseMode->parseSingleConfig ,it can be improved to reduce parseConfig. will update it in v9 patch series > > Or better, can you skip this patch completely ? If (and only if) I > understand the process correctly, this patch still uses the "old" > tuning format, while the next one moves to the "new" format where only > 'modes' are specified. > > Can we skip this interim patch and move to the new format directly ? > We don't have any obbligation towards legacy tuning file and they > won't work anyway after this series is applied. > > So, make it clear in the commit message of the next patch that the > tuning file layout has changed and 'modes' are now mandatory and there > is no "default"/"generic" configuration of dpf. > > What do you think ? "default" mode looks more reasonable as current design. but my thought is for introducing "auto" mode as default in next PR. if this "default" add into current modes , it will be removed in next PR, and currently master by default setting is "off". it can be edit from "control". and highquality/minimal/zsl.. can also be selected as default mode once enable dpf To highlight enable/disabel epf as default will introduce - enableMode : 'minimal' in v9 patch series > > > > +{ > > + /* Parse base config. */ > > + int ret = parseSingleConfig(tuningData, config_, strengthConfig_); > > + if (ret) > > + return ret; > > + > > + return 0; > > +} > > + > > +int Dpf::parseSingleConfig(const YamlObject &tuningData, > > + rkisp1_cif_isp_dpf_config &config, > > + rkisp1_cif_isp_dpf_strength_config &strengthConfig) > > { > > std::vector<uint8_t> values; > > > > @@ -82,10 +104,10 @@ int Dpf::init([[maybe_unused]] IPAContext &context, > > } > > > > std::copy_n(values.begin(), values.size(), > > - std::begin(config_.g_flt.spatial_coeff)); > > + std::begin(config.g_flt.spatial_coeff)); > > > > - config_.g_flt.gr_enable = true; > > - config_.g_flt.gb_enable = true; > > + config.g_flt.gr_enable = true; > > + config.g_flt.gb_enable = true; > > > > /* > > * For the red and blue components, we have the 13x9 kernel specified > > @@ -119,15 +141,15 @@ int Dpf::init([[maybe_unused]] IPAContext &context, > > return -EINVAL; > > } > > > > - config_.rb_flt.fltsize = values.size() == RKISP1_CIF_ISP_DPF_MAX_SPATIAL_COEFFS > > - ? RKISP1_CIF_ISP_DPF_RB_FILTERSIZE_13x9 > > - : RKISP1_CIF_ISP_DPF_RB_FILTERSIZE_9x9; > > + config.rb_flt.fltsize = values.size() == RKISP1_CIF_ISP_DPF_MAX_SPATIAL_COEFFS > > + ? RKISP1_CIF_ISP_DPF_RB_FILTERSIZE_13x9 > > + : RKISP1_CIF_ISP_DPF_RB_FILTERSIZE_9x9; > > > > std::copy_n(values.begin(), values.size(), > > - std::begin(config_.rb_flt.spatial_coeff)); > > + std::begin(config.rb_flt.spatial_coeff)); > > > > - config_.rb_flt.r_enable = true; > > - config_.rb_flt.b_enable = true; > > + config.rb_flt.r_enable = true; > > + config.rb_flt.b_enable = true; > > > > /* > > * The range kernel is configured with a noise level lookup table (NLL) > > @@ -147,13 +169,13 @@ int Dpf::init([[maybe_unused]] IPAContext &context, > > } > > > > std::copy_n(nllValues.begin(), nllValues.size(), > > - std::begin(config_.nll.coeff)); > > + std::begin(config.nll.coeff)); > > > > std::string scaleMode = rFObject["scale-mode"].get<std::string>(""); > > if (scaleMode == "linear") { > > - config_.nll.scale_mode = RKISP1_CIF_ISP_NLL_SCALE_LINEAR; > > + config.nll.scale_mode = RKISP1_CIF_ISP_NLL_SCALE_LINEAR; > > } else if (scaleMode == "logarithmic") { > > - config_.nll.scale_mode = RKISP1_CIF_ISP_NLL_SCALE_LOGARITHMIC; > > + config.nll.scale_mode = RKISP1_CIF_ISP_NLL_SCALE_LOGARITHMIC; > > } else { > > LOG(RkISP1Dpf, Error) > > << "Invalid 'RangeFilter:scale-mode': expected " > > @@ -164,9 +186,9 @@ int Dpf::init([[maybe_unused]] IPAContext &context, > > > > const YamlObject &fSObject = tuningData["FilterStrength"]; > > > > - strengthConfig_.r = fSObject["r"].get<uint16_t>(64); > > - strengthConfig_.g = fSObject["g"].get<uint16_t>(64); > > - strengthConfig_.b = fSObject["b"].get<uint16_t>(64); > > + strengthConfig.r = fSObject["r"].get<uint8_t>().value_or(64); > > + strengthConfig.g = fSObject["g"].get<uint8_t>().value_or(64); > > + strengthConfig.b = fSObject["b"].get<uint8_t>().value_or(64); > > > > return 0; > > } > > diff --git a/src/ipa/rkisp1/algorithms/dpf.h b/src/ipa/rkisp1/algorithms/dpf.h > > index 2dd8cd36..39186c55 100644 > > --- a/src/ipa/rkisp1/algorithms/dpf.h > > +++ b/src/ipa/rkisp1/algorithms/dpf.h > > @@ -30,6 +30,11 @@ public: > > RkISP1Params *params) override; > > > > private: > > + int parseConfig(const YamlObject &tuningData); > > + int parseSingleConfig(const YamlObject &tuningData, > > + rkisp1_cif_isp_dpf_config &config, > > + rkisp1_cif_isp_dpf_strength_config &strengthConfig); > > + > > struct rkisp1_cif_isp_dpf_config config_; > > struct rkisp1_cif_isp_dpf_strength_config strengthConfig_; > > }; > > -- > > 2.43.0 > >
diff --git a/src/ipa/rkisp1/algorithms/dpf.cpp b/src/ipa/rkisp1/algorithms/dpf.cpp index 39f3e461..dd3effa1 100644 --- a/src/ipa/rkisp1/algorithms/dpf.cpp +++ b/src/ipa/rkisp1/algorithms/dpf.cpp @@ -46,6 +46,28 @@ Dpf::Dpf() */ int Dpf::init([[maybe_unused]] IPAContext &context, const YamlObject &tuningData) +{ + /* Parse tuning block. */ + int ret = parseConfig(tuningData); + if (ret) + return ret; + + return 0; +} + +int Dpf::parseConfig(const YamlObject &tuningData) +{ + /* Parse base config. */ + int ret = parseSingleConfig(tuningData, config_, strengthConfig_); + if (ret) + return ret; + + return 0; +} + +int Dpf::parseSingleConfig(const YamlObject &tuningData, + rkisp1_cif_isp_dpf_config &config, + rkisp1_cif_isp_dpf_strength_config &strengthConfig) { std::vector<uint8_t> values; @@ -82,10 +104,10 @@ int Dpf::init([[maybe_unused]] IPAContext &context, } std::copy_n(values.begin(), values.size(), - std::begin(config_.g_flt.spatial_coeff)); + std::begin(config.g_flt.spatial_coeff)); - config_.g_flt.gr_enable = true; - config_.g_flt.gb_enable = true; + config.g_flt.gr_enable = true; + config.g_flt.gb_enable = true; /* * For the red and blue components, we have the 13x9 kernel specified @@ -119,15 +141,15 @@ int Dpf::init([[maybe_unused]] IPAContext &context, return -EINVAL; } - config_.rb_flt.fltsize = values.size() == RKISP1_CIF_ISP_DPF_MAX_SPATIAL_COEFFS - ? RKISP1_CIF_ISP_DPF_RB_FILTERSIZE_13x9 - : RKISP1_CIF_ISP_DPF_RB_FILTERSIZE_9x9; + config.rb_flt.fltsize = values.size() == RKISP1_CIF_ISP_DPF_MAX_SPATIAL_COEFFS + ? RKISP1_CIF_ISP_DPF_RB_FILTERSIZE_13x9 + : RKISP1_CIF_ISP_DPF_RB_FILTERSIZE_9x9; std::copy_n(values.begin(), values.size(), - std::begin(config_.rb_flt.spatial_coeff)); + std::begin(config.rb_flt.spatial_coeff)); - config_.rb_flt.r_enable = true; - config_.rb_flt.b_enable = true; + config.rb_flt.r_enable = true; + config.rb_flt.b_enable = true; /* * The range kernel is configured with a noise level lookup table (NLL) @@ -147,13 +169,13 @@ int Dpf::init([[maybe_unused]] IPAContext &context, } std::copy_n(nllValues.begin(), nllValues.size(), - std::begin(config_.nll.coeff)); + std::begin(config.nll.coeff)); std::string scaleMode = rFObject["scale-mode"].get<std::string>(""); if (scaleMode == "linear") { - config_.nll.scale_mode = RKISP1_CIF_ISP_NLL_SCALE_LINEAR; + config.nll.scale_mode = RKISP1_CIF_ISP_NLL_SCALE_LINEAR; } else if (scaleMode == "logarithmic") { - config_.nll.scale_mode = RKISP1_CIF_ISP_NLL_SCALE_LOGARITHMIC; + config.nll.scale_mode = RKISP1_CIF_ISP_NLL_SCALE_LOGARITHMIC; } else { LOG(RkISP1Dpf, Error) << "Invalid 'RangeFilter:scale-mode': expected " @@ -164,9 +186,9 @@ int Dpf::init([[maybe_unused]] IPAContext &context, const YamlObject &fSObject = tuningData["FilterStrength"]; - strengthConfig_.r = fSObject["r"].get<uint16_t>(64); - strengthConfig_.g = fSObject["g"].get<uint16_t>(64); - strengthConfig_.b = fSObject["b"].get<uint16_t>(64); + strengthConfig.r = fSObject["r"].get<uint8_t>().value_or(64); + strengthConfig.g = fSObject["g"].get<uint8_t>().value_or(64); + strengthConfig.b = fSObject["b"].get<uint8_t>().value_or(64); return 0; } diff --git a/src/ipa/rkisp1/algorithms/dpf.h b/src/ipa/rkisp1/algorithms/dpf.h index 2dd8cd36..39186c55 100644 --- a/src/ipa/rkisp1/algorithms/dpf.h +++ b/src/ipa/rkisp1/algorithms/dpf.h @@ -30,6 +30,11 @@ public: RkISP1Params *params) override; private: + int parseConfig(const YamlObject &tuningData); + int parseSingleConfig(const YamlObject &tuningData, + rkisp1_cif_isp_dpf_config &config, + rkisp1_cif_isp_dpf_strength_config &strengthConfig); + struct rkisp1_cif_isp_dpf_config config_; struct rkisp1_cif_isp_dpf_strength_config strengthConfig_; };