| Message ID | 20251208004808.1274417-2-rui.wang@ideasonboard.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
Hi Rui subject is a bit long, it could be ipa: rkisp1: dpf: refactor DPF parsing and initialization On Sun, Dec 07, 2025 at 07:48:02PM -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 defination s/defination/definition If you end with a '.' one of the entries, please add it to the other ones as well. Actually I would not make this a list at all but a simpler: Split DPF configuration parsing and initialization into clearer, self-contained helpers and modernized initialization patterns. Introduce parseSingleConfig() as DPF tuning file parser helper in order to make future extensions and maintenance easier While at it, 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> > --- > Changelog in v4: > -Split V3 one patch into 6 patches Thanks, way easier to review > -The first patch only focus on code reorgnization, no new logic added > -Delete Makefile from V3 patch > > src/ipa/rkisp1/algorithms/dpf.cpp | 61 +++++++++++++++++++++---------- > src/ipa/rkisp1/algorithms/dpf.h | 5 +++ > 2 files changed, 46 insertions(+), 20 deletions(-) > > diff --git a/src/ipa/rkisp1/algorithms/dpf.cpp b/src/ipa/rkisp1/algorithms/dpf.cpp > index 39f3e461..cd0a7d9d 100644 > --- a/src/ipa/rkisp1/algorithms/dpf.cpp > +++ b/src/ipa/rkisp1/algorithms/dpf.cpp > @@ -46,6 +46,27 @@ Dpf::Dpf() > */ > int Dpf::init([[maybe_unused]] IPAContext &context, > const YamlObject &tuningData) > +{ > + /* Parse tuning block. */ > + if (!parseConfig(tuningData)) { > + return -EINVAL; > + } Please, this is a tiny thing but it has been repeated multiple times: no curly braces for single line statements > + > + return 0; > +} > + > +bool Dpf::parseConfig(const YamlObject &tuningData) > +{ > + /* Parse base config. */ > + if (!parseSingleConfig(tuningData, config_, strengthConfig_)) { > + return false; > + } ditto this could just be return parseSingleConfig(tuningData, config_, strengthConfig_); > + return true; > +} > + > +bool Dpf::parseSingleConfig(const YamlObject &tuningData, > + rkisp1_cif_isp_dpf_config &config, > + rkisp1_cif_isp_dpf_strength_config &strengthConfig) > { > std::vector<uint8_t> values; > > @@ -78,14 +99,14 @@ int Dpf::init([[maybe_unused]] IPAContext &context, > << "Invalid 'DomainFilter:g': expected " > << RKISP1_CIF_ISP_DPF_MAX_SPATIAL_COEFFS > << " elements, got " << values.size(); > - return -EINVAL; > + return false; I would propagated the errors up and return an int from Dpf::parseSingleConfig() and Dpf::parseConfig(), but since Dpf::init() returns -EINVAL unconditionally, it doesn't matter much. Preferably I would have changed Dpf::init() to return the error code from parseConfig() so that different error codes could be propagated up eventually > } > > 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 > @@ -116,18 +137,18 @@ int Dpf::init([[maybe_unused]] IPAContext &context, > << RKISP1_CIF_ISP_DPF_MAX_SPATIAL_COEFFS - 1 > << " or " << RKISP1_CIF_ISP_DPF_MAX_SPATIAL_COEFFS > << " elements, got " << values.size(); > - return -EINVAL; > + return false; > } > > - 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) > @@ -143,32 +164,32 @@ int Dpf::init([[maybe_unused]] IPAContext &context, > << "Invalid 'RangeFilter:coeff': expected " > << RKISP1_CIF_ISP_DPF_MAX_NLF_COEFFS > << " elements, got " << nllValues.size(); > - return -EINVAL; > + return false; > } > > 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 " > << "'linear' or 'logarithmic' value, got " > << scaleMode; > - return -EINVAL; > + return false; > } > > 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; > + return true; > } > > /** > diff --git a/src/ipa/rkisp1/algorithms/dpf.h b/src/ipa/rkisp1/algorithms/dpf.h > index 2dd8cd36..bee6fc9b 100644 > --- a/src/ipa/rkisp1/algorithms/dpf.h > +++ b/src/ipa/rkisp1/algorithms/dpf.h > @@ -30,6 +30,11 @@ public: > RkISP1Params *params) override; > > private: > + bool parseConfig(const YamlObject &tuningData); > + bool parseSingleConfig(const YamlObject &tuningData, > + rkisp1_cif_isp_dpf_config &config, > + rkisp1_cif_isp_dpf_strength_config &strengthConfig); > + Thanks With the above issues addressed Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> Thanks j > struct rkisp1_cif_isp_dpf_config config_; > struct rkisp1_cif_isp_dpf_strength_config strengthConfig_; > }; > -- > 2.43.0 >
Jacopo Mondi wrote: > Hi Rui > > subject is a bit long, it could be > > ipa: rkisp1: dpf: refactor DPF parsing and initialization > > > On Sun, Dec 07, 2025 at 07:48:02PM -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 defination > > s/defination/definition > > If you end with a '.' one of the entries, please add it to the other > ones as well. Actually I would not make this a list at all but a > simpler: > > Split DPF configuration parsing and initialization into clearer, > self-contained helpers and modernized initialization patterns. > > Introduce parseSingleConfig() as DPF tuning file parser helper > in order to make future extensions and maintenance easier > > While at it, 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> > > --- > > Changelog in v4: > > -Split V3 one patch into 6 patches > > Thanks, way easier to review > > > -The first patch only focus on code reorgnization, no new logic added > > -Delete Makefile from V3 patch > > > > src/ipa/rkisp1/algorithms/dpf.cpp | 61 +++++++++++++++++++++---------- > > src/ipa/rkisp1/algorithms/dpf.h | 5 +++ > > 2 files changed, 46 insertions(+), 20 deletions(-) > > > > diff --git a/src/ipa/rkisp1/algorithms/dpf.cpp b/src/ipa/rkisp1/algorithms/dpf.cpp > > index 39f3e461..cd0a7d9d 100644 > > --- a/src/ipa/rkisp1/algorithms/dpf.cpp > > +++ b/src/ipa/rkisp1/algorithms/dpf.cpp > > @@ -46,6 +46,27 @@ Dpf::Dpf() > > */ > > int Dpf::init([[maybe_unused]] IPAContext &context, > > const YamlObject &tuningData) > > +{ > > + /* Parse tuning block. */ > > + if (!parseConfig(tuningData)) { > > + return -EINVAL; > > + } > > Please, this is a tiny thing but it has been repeated multiple times: > no curly braces for single line statements > yes I will remove all those braces to quick return. > > + > > + return 0; > > +} > > + > > +bool Dpf::parseConfig(const YamlObject &tuningData) > > +{ > > + /* Parse base config. */ > > + if (!parseSingleConfig(tuningData, config_, strengthConfig_)) { > > + return false; > > + } > > ditto > > this could just be > > return parseSingleConfig(tuningData, config_, strengthConfig_); yes , if the return value chagne from boolean to int , it can return directly but I would prefer to use ret = parseSingleConfig, in that case , the following patch can reuse ret for checking . > > > + return true; > > +} > > + > > +bool Dpf::parseSingleConfig(const YamlObject &tuningData, > > + rkisp1_cif_isp_dpf_config &config, > > + rkisp1_cif_isp_dpf_strength_config &strengthConfig) > > { > > std::vector<uint8_t> values; > > > > @@ -78,14 +99,14 @@ int Dpf::init([[maybe_unused]] IPAContext &context, > > << "Invalid 'DomainFilter:g': expected " > > << RKISP1_CIF_ISP_DPF_MAX_SPATIAL_COEFFS > > << " elements, got " << values.size(); > > - return -EINVAL; > > + return false; > > I would propagated the errors up and return an int from > Dpf::parseSingleConfig() and Dpf::parseConfig(), but since > Dpf::init() returns -EINVAL unconditionally, it doesn't matter much. > > Preferably I would have changed Dpf::init() to return the error code > from parseConfig() so that different error codes could be propagated > up eventually > > > } yes , If all parse* function follow same return value, the ERROR code can back tracking to init function will update into next series > > > > 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 > > @@ -116,18 +137,18 @@ int Dpf::init([[maybe_unused]] IPAContext &context, > > << RKISP1_CIF_ISP_DPF_MAX_SPATIAL_COEFFS - 1 > > << " or " << RKISP1_CIF_ISP_DPF_MAX_SPATIAL_COEFFS > > << " elements, got " << values.size(); > > - return -EINVAL; > > + return false; > > } > > > > - 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) > > @@ -143,32 +164,32 @@ int Dpf::init([[maybe_unused]] IPAContext &context, > > << "Invalid 'RangeFilter:coeff': expected " > > << RKISP1_CIF_ISP_DPF_MAX_NLF_COEFFS > > << " elements, got " << nllValues.size(); > > - return -EINVAL; > > + return false; > > } > > > > 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 " > > << "'linear' or 'logarithmic' value, got " > > << scaleMode; > > - return -EINVAL; > > + return false; > > } > > > > 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; > > + return true; > > } > > > > /** > > diff --git a/src/ipa/rkisp1/algorithms/dpf.h b/src/ipa/rkisp1/algorithms/dpf.h > > index 2dd8cd36..bee6fc9b 100644 > > --- a/src/ipa/rkisp1/algorithms/dpf.h > > +++ b/src/ipa/rkisp1/algorithms/dpf.h > > @@ -30,6 +30,11 @@ public: > > RkISP1Params *params) override; > > > > private: > > + bool parseConfig(const YamlObject &tuningData); > > + bool parseSingleConfig(const YamlObject &tuningData, > > + rkisp1_cif_isp_dpf_config &config, > > + rkisp1_cif_isp_dpf_strength_config &strengthConfig); > > + > > Thanks > > With the above issues addressed > Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > > Thanks > j > > > 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..cd0a7d9d 100644 --- a/src/ipa/rkisp1/algorithms/dpf.cpp +++ b/src/ipa/rkisp1/algorithms/dpf.cpp @@ -46,6 +46,27 @@ Dpf::Dpf() */ int Dpf::init([[maybe_unused]] IPAContext &context, const YamlObject &tuningData) +{ + /* Parse tuning block. */ + if (!parseConfig(tuningData)) { + return -EINVAL; + } + + return 0; +} + +bool Dpf::parseConfig(const YamlObject &tuningData) +{ + /* Parse base config. */ + if (!parseSingleConfig(tuningData, config_, strengthConfig_)) { + return false; + } + return true; +} + +bool Dpf::parseSingleConfig(const YamlObject &tuningData, + rkisp1_cif_isp_dpf_config &config, + rkisp1_cif_isp_dpf_strength_config &strengthConfig) { std::vector<uint8_t> values; @@ -78,14 +99,14 @@ int Dpf::init([[maybe_unused]] IPAContext &context, << "Invalid 'DomainFilter:g': expected " << RKISP1_CIF_ISP_DPF_MAX_SPATIAL_COEFFS << " elements, got " << values.size(); - return -EINVAL; + return false; } 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 @@ -116,18 +137,18 @@ int Dpf::init([[maybe_unused]] IPAContext &context, << RKISP1_CIF_ISP_DPF_MAX_SPATIAL_COEFFS - 1 << " or " << RKISP1_CIF_ISP_DPF_MAX_SPATIAL_COEFFS << " elements, got " << values.size(); - return -EINVAL; + return false; } - 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) @@ -143,32 +164,32 @@ int Dpf::init([[maybe_unused]] IPAContext &context, << "Invalid 'RangeFilter:coeff': expected " << RKISP1_CIF_ISP_DPF_MAX_NLF_COEFFS << " elements, got " << nllValues.size(); - return -EINVAL; + return false; } 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 " << "'linear' or 'logarithmic' value, got " << scaleMode; - return -EINVAL; + return false; } 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; + return true; } /** diff --git a/src/ipa/rkisp1/algorithms/dpf.h b/src/ipa/rkisp1/algorithms/dpf.h index 2dd8cd36..bee6fc9b 100644 --- a/src/ipa/rkisp1/algorithms/dpf.h +++ b/src/ipa/rkisp1/algorithms/dpf.h @@ -30,6 +30,11 @@ public: RkISP1Params *params) override; private: + bool parseConfig(const YamlObject &tuningData); + bool 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_; };
-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 defination Signed-off-by: Rui Wang <rui.wang@ideasonboard.com> --- Changelog in v4: -Split V3 one patch into 6 patches -The first patch only focus on code reorgnization, no new logic added -Delete Makefile from V3 patch src/ipa/rkisp1/algorithms/dpf.cpp | 61 +++++++++++++++++++++---------- src/ipa/rkisp1/algorithms/dpf.h | 5 +++ 2 files changed, 46 insertions(+), 20 deletions(-)