| Message ID | 20251208004808.1274417-3-rui.wang@ideasonboard.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
Hi Rui you can shorten the subject here (and in the rest of the series) as well On Sun, Dec 07, 2025 at 07:48:03PM -0500, Rui Wang wrote: > -Add support for switching between different noise reduction modes. > -Introduce `noiseReductionModes_` to store mode-specific configs. > -loadReductionConfig() select specific config from configs Missing '.' Please try to be consistent > > Introduce `noiseReductionModes_` to store current configs. Not sure why some lines are - and this is not. A simple Add support for switching between different noise reduction modes. Introduce `noiseReductionModes_` to store mode-specific configs and implement loadReductionConfig() to select a mode configuration using the application provided control value. > > Signed-off-by: Rui Wang <rui.wang@ideasonboard.com> > --- > > changelog: > - Add blank line > - Move V3 first patch's loadReductionConfig and reduction mode helper > into this patch > > src/ipa/rkisp1/algorithms/dpf.cpp | 81 ++++++++++++++++++++++++++++++- > src/ipa/rkisp1/algorithms/dpf.h | 11 +++++ > 2 files changed, 90 insertions(+), 2 deletions(-) > > diff --git a/src/ipa/rkisp1/algorithms/dpf.cpp b/src/ipa/rkisp1/algorithms/dpf.cpp > index cd0a7d9d..18f2a158 100644 > --- a/src/ipa/rkisp1/algorithms/dpf.cpp > +++ b/src/ipa/rkisp1/algorithms/dpf.cpp > @@ -37,7 +37,9 @@ namespace ipa::rkisp1::algorithms { > LOG_DEFINE_CATEGORY(RkISP1Dpf) > > Dpf::Dpf() > - : config_({}), strengthConfig_({}) > + : config_({}), strengthConfig_({}), > + noiseReductionModes_({}), this fits on the previous line > + runningMode_(controls::draft::NoiseReductionModeOff) > { > } > > @@ -61,6 +63,53 @@ bool Dpf::parseConfig(const YamlObject &tuningData) > if (!parseSingleConfig(tuningData, config_, strengthConfig_)) { > return false; > } > + > + /* Parse modes. */ > + if (!parseModes(tuningData)) { > + return false; > + } No curly braces etc etc > + > + return true; > +} > + > +bool Dpf::parseModes(const YamlObject &tuningData) > +{ > + /* Parse noise reduction modes. */ > + if (!tuningData.contains("modes")) { > + return true; > + } > + > + noiseReductionModes_.clear(); > + for (const auto &entry : tuningData["modes"].asList()) { > + std::optional<std::string> typeOpt = > + entry["type"].get<std::string>(); > + if (!typeOpt) { > + LOG(RkISP1Dpf, Error) << "Modes entry missing type"; > + return false; > + } Are modes mandatory in your opinion ? Don't we have a general configuration (parsed by parseSingleConfig()) ? Can't a single configuration be used ? I guess we can't as the controls we have do not allow a "turn noise reduction on" but only allow to specify "use this noise reduction mode". Given this context, I might have missed what the outer configuration is used for, if only the ones part of a mode can be enabled. > + > + int32_t modeValue = controls::draft::NoiseReductionModeOff; > + if (*typeOpt == "minimal") { > + modeValue = controls::draft::NoiseReductionModeMinimal; > + } else if (*typeOpt == "fast") { > + modeValue = controls::draft::NoiseReductionModeFast; > + } else if (*typeOpt == "highquality") { > + modeValue = controls::draft::NoiseReductionModeHighQuality; > + } else if (*typeOpt == "zsl") { > + modeValue = controls::draft::NoiseReductionModeZSL; > + } else { > + LOG(RkISP1Dpf, Error) << "Unknown mode type: " << *typeOpt; > + return false; > + } > + > + ModeConfig mode{}; You could declare this before the long if {} else if {} section > + mode.modeValue = modeValue; And assign mode.modeValue directly instead of going through a temporary > + if (!parseSingleConfig(entry, mode.dpf, mode.strength)) { > + return false; > + } No curly etc etc > + noiseReductionModes_.push_back(mode); > + } > + > return true; > } > > @@ -192,6 +241,29 @@ bool Dpf::parseSingleConfig(const YamlObject &tuningData, > return true; > } > > +bool Dpf::loadReductionConfig(int32_t mode) > +{ > + auto it = std::find_if(noiseReductionModes_.begin(), noiseReductionModes_.end(), > + [mode](const ModeConfig &m) { > + return m.modeValue == mode; > + }); > + if (it == noiseReductionModes_.end()) { > + LOG(RkISP1Dpf, Warning) Right now the main IPA module in rkisp1.cpp registers { &controls::draft::NoiseReductionMode, ControlInfo(controls::draft::NoiseReductionModeValues) }, meaning all the defined modes are supported. I guess we need to do what, in example, Agc::parseMeteringModes() does: parse the tuning file and populate the supported control values with the entries enabled in the config file. Do you plan to do so on top of this series ? > + << "No DPF config for reduction mode " > + << static_cast<int>(mode); I don't think you need to cast (below too) > + return false; > + } > + > + config_ = it->dpf; > + strengthConfig_ = it->strength; > + > + LOG(RkISP1Dpf, Info) Maybe just a Debug, this is a regular operation, isn't it ? > + << "DPF mode=Reduction (config loaded)" > + << " mode=" << static_cast<int>(mode); << "Selected DPF mode " << mode; It would be nicer if we keep a map somewhere to associate the mode id with a human readable description. > + > + return true; > +} > + > /** > * \copydoc libcamera::ipa::Algorithm::queueRequest > */ > @@ -207,6 +279,7 @@ void Dpf::queueRequest(IPAContext &context, > if (denoise) { > LOG(RkISP1Dpf, Debug) << "Set denoise to " << *denoise; > > + runningMode_ = *denoise; > switch (*denoise) { > case controls::draft::NoiseReductionModeOff: > if (dpf.denoise) { > @@ -217,8 +290,12 @@ void Dpf::queueRequest(IPAContext &context, > case controls::draft::NoiseReductionModeMinimal: > case controls::draft::NoiseReductionModeHighQuality: > case controls::draft::NoiseReductionModeFast: > - if (!dpf.denoise) { > + case controls::draft::NoiseReductionModeZSL: Why wasn't this handle before this patch ? > + if (loadReductionConfig(runningMode_)) { > + update = true; > dpf.denoise = true; > + } else { > + dpf.denoise = false; > update = true; > } This might get tricky. We have 4 cases here 1) DPF was off 1.a loadReductionConfig() success 1.b loadReductionConfig() fail 2) DPF was on 2.a loadReductionConfig() success 2.b loadReductionConfig() fail 1.a: update = true, dpf.denoise = true 1.b: update = false, dpf.denoise = false; 2.a: update = true, dpf.denoise = true 2.b: update = false, denoise = false 2.a can be further optimized to only update the denoise configuration if the mode has changed compared to the current one I would rework this function as (not tested) const auto &denoise = controls.get(controls::draft::NoiseReductionMode); bool update = false; if (denoise && *denoise != runningMode_) { switch (*denoise) { case controls::draft::NoiseReductionModeOff: if (dpf.denoise) { dpf.denoise = false; update = true; } break; case controls::draft::NoiseReductionModeMinimal: case controls::draft::NoiseReductionModeHighQuality: case controls::draft::NoiseReductionModeFast: case controls::draft::NoiseReductionModeZSL: if (loadReductionConfig(*denoise)) { update = true; dpf.denoise = true; } break; default: LOG(RkISP1Dpf, Error) << "Unsupported denoise value " << *denoise; break; } if (update) { runningMode_ = *denoise; LOG(RkISP1Dpf, Debug) << "Set denoise mode to " << *denoise; } } frameContext.dpf.denoise = dpf.denoise; frameContext.dpf.update = update; What do you think ? Thanks j > break; > diff --git a/src/ipa/rkisp1/algorithms/dpf.h b/src/ipa/rkisp1/algorithms/dpf.h > index bee6fc9b..30cbaa57 100644 > --- a/src/ipa/rkisp1/algorithms/dpf.h > +++ b/src/ipa/rkisp1/algorithms/dpf.h > @@ -30,13 +30,24 @@ public: > RkISP1Params *params) override; > > private: > + struct ModeConfig { > + int32_t modeValue; > + rkisp1_cif_isp_dpf_config dpf; > + rkisp1_cif_isp_dpf_strength_config strength; > + }; > + > bool parseConfig(const YamlObject &tuningData); > + bool parseModes(const YamlObject &tuningData); > bool parseSingleConfig(const YamlObject &tuningData, > rkisp1_cif_isp_dpf_config &config, > rkisp1_cif_isp_dpf_strength_config &strengthConfig); > > + bool loadReductionConfig(int32_t mode); > + > struct rkisp1_cif_isp_dpf_config config_; > struct rkisp1_cif_isp_dpf_strength_config strengthConfig_; > + std::vector<ModeConfig> noiseReductionModes_; > + int32_t runningMode_; > }; > > } /* namespace ipa::rkisp1::algorithms */ > -- > 2.43.0 >
Jacopo Mondi wrote: > Hi Rui > > you can shorten the subject here (and in the rest of the series) as > well > > On Sun, Dec 07, 2025 at 07:48:03PM -0500, Rui Wang wrote: > > -Add support for switching between different noise reduction modes. > > -Introduce `noiseReductionModes_` to store mode-specific configs. > > -loadReductionConfig() select specific config from configs > > Missing '.' > > Please try to be consistent > > > > > Introduce `noiseReductionModes_` to store current configs. > > Not sure why some lines are - and this is not. > > A simple > > Add support for switching between different noise reduction modes. > > Introduce `noiseReductionModes_` to store mode-specific configs and > implement loadReductionConfig() to select a mode configuration using > the application provided control value. > > > > > Signed-off-by: Rui Wang <rui.wang@ideasonboard.com> > > --- > > > > changelog: > > - Add blank line > > - Move V3 first patch's loadReductionConfig and reduction mode helper > > into this patch > > > > src/ipa/rkisp1/algorithms/dpf.cpp | 81 ++++++++++++++++++++++++++++++- > > src/ipa/rkisp1/algorithms/dpf.h | 11 +++++ > > 2 files changed, 90 insertions(+), 2 deletions(-) > > > > diff --git a/src/ipa/rkisp1/algorithms/dpf.cpp b/src/ipa/rkisp1/algorithms/dpf.cpp > > index cd0a7d9d..18f2a158 100644 > > --- a/src/ipa/rkisp1/algorithms/dpf.cpp > > +++ b/src/ipa/rkisp1/algorithms/dpf.cpp > > @@ -37,7 +37,9 @@ namespace ipa::rkisp1::algorithms { > > LOG_DEFINE_CATEGORY(RkISP1Dpf) > > > > Dpf::Dpf() > > - : config_({}), strengthConfig_({}) > > + : config_({}), strengthConfig_({}), > > + noiseReductionModes_({}), > > this fits on the previous line > > > + runningMode_(controls::draft::NoiseReductionModeOff) > > { > > } > > > > @@ -61,6 +63,53 @@ bool Dpf::parseConfig(const YamlObject &tuningData) > > if (!parseSingleConfig(tuningData, config_, strengthConfig_)) { > > return false; > > } > > + > > + /* Parse modes. */ > > + if (!parseModes(tuningData)) { > > + return false; > > + } > > No curly braces etc etc > > > > + > > + return true; > > +} > > + > > +bool Dpf::parseModes(const YamlObject &tuningData) > > +{ > > + /* Parse noise reduction modes. */ > > + if (!tuningData.contains("modes")) { > > + return true; > > + } > > + > > + noiseReductionModes_.clear(); > > + for (const auto &entry : tuningData["modes"].asList()) { > > + std::optional<std::string> typeOpt = > > + entry["type"].get<std::string>(); > > + if (!typeOpt) { > > + LOG(RkISP1Dpf, Error) << "Modes entry missing type"; > > + return false; > > + } > > Are modes mandatory in your opinion ? Don't we have a general > configuration (parsed by parseSingleConfig()) ? Can't a single > configuration be used ? I guess we can't as the controls we have do > not allow a "turn noise reduction on" but only allow to specify "use > this noise reduction mode". Given this context, I might have missed > what the outer configuration is used for, if only the ones part of a > mode can be enabled. > In tuning file like:imx219.yaml , the mode config is optional like : highquality/zsl/minimal. From control , if it choose a specific without any tuning config , it will print some error message and keep previous status > > + > > + int32_t modeValue = controls::draft::NoiseReductionModeOff; > > + if (*typeOpt == "minimal") { > > + modeValue = controls::draft::NoiseReductionModeMinimal; > > + } else if (*typeOpt == "fast") { > > + modeValue = controls::draft::NoiseReductionModeFast; > > + } else if (*typeOpt == "highquality") { > > + modeValue = controls::draft::NoiseReductionModeHighQuality; > > + } else if (*typeOpt == "zsl") { > > + modeValue = controls::draft::NoiseReductionModeZSL; > > + } else { > > + LOG(RkISP1Dpf, Error) << "Unknown mode type: " << *typeOpt; > > + return false; > > + } > > + > > + ModeConfig mode{}; > > You could declare this before the long if {} else if {} section > Yes will update in next series > > + mode.modeValue = modeValue; > > And assign mode.modeValue directly instead of going through a > temporary > > > + if (!parseSingleConfig(entry, mode.dpf, mode.strength)) { > > + return false; > > + } > > No curly etc etc > Will update in next series > > + noiseReductionModes_.push_back(mode); > > + } > > + > > return true; > > } > > > > @@ -192,6 +241,29 @@ bool Dpf::parseSingleConfig(const YamlObject &tuningData, > > return true; > > } > > > > +bool Dpf::loadReductionConfig(int32_t mode) > > +{ > > + auto it = std::find_if(noiseReductionModes_.begin(), noiseReductionModes_.end(), > > + [mode](const ModeConfig &m) { > > + return m.modeValue == mode; > > + }); > > + if (it == noiseReductionModes_.end()) { > > + LOG(RkISP1Dpf, Warning) > > > Right now the main IPA module in rkisp1.cpp registers > > { &controls::draft::NoiseReductionMode, ControlInfo(controls::draft::NoiseReductionModeValues) }, > > meaning all the defined modes are supported. > > I guess we need to do what, in example, Agc::parseMeteringModes() > does: parse the tuning file and populate the supported control values > with the entries enabled in the config file. > > Do you plan to do so on top of this series ? > Yes , for each mode has its own specific tuning config for dpf and filter denoise and in future I would add Auto Mode into this control as well . > > > > + << "No DPF config for reduction mode " > > + << static_cast<int>(mode); > > I don't think you need to cast (below too) yes it is already int32_t > > > + return false; > > + } > > + > > + config_ = it->dpf; > > + strengthConfig_ = it->strength; > > + > > + LOG(RkISP1Dpf, Info) > > Maybe just a Debug, this is a regular operation, isn't it ? > I found when select the control ,libcamera it will print log control value as info leve . So I print as same level. it prints the logs only when control value changed > > + << "DPF mode=Reduction (config loaded)" > > + << " mode=" << static_cast<int>(mode); > > << "Selected DPF mode " << mode; > > It would be nicer if we keep a map somewhere to associate the mode id > with a human readable description. Yes, good idea , add map can use into other function for logging and debugging > > > + > > + return true; > > +} > > + > > /** > > * \copydoc libcamera::ipa::Algorithm::queueRequest > > */ > > @@ -207,6 +279,7 @@ void Dpf::queueRequest(IPAContext &context, > > if (denoise) { > > LOG(RkISP1Dpf, Debug) << "Set denoise to " << *denoise; > > > > + runningMode_ = *denoise; > > switch (*denoise) { > > case controls::draft::NoiseReductionModeOff: > > if (dpf.denoise) { > > @@ -217,8 +290,12 @@ void Dpf::queueRequest(IPAContext &context, > > case controls::draft::NoiseReductionModeMinimal: > > case controls::draft::NoiseReductionModeHighQuality: > > case controls::draft::NoiseReductionModeFast: > > - if (!dpf.denoise) { > > + case controls::draft::NoiseReductionModeZSL: > > Why wasn't this handle before this patch ? on master branch , ZSL is excluded in this switch case. it is not handled by enabling denoise > > > + if (loadReductionConfig(runningMode_)) { > > + update = true; > > dpf.denoise = true; > > + } else { > > + dpf.denoise = false; > > update = true; > > } > > This might get tricky. > > We have 4 cases here > > 1) DPF was off > 1.a loadReductionConfig() success > 1.b loadReductionConfig() fail > 2) DPF was on > 2.a loadReductionConfig() success > 2.b loadReductionConfig() fail > > 1.a: update = true, dpf.denoise = true > 1.b: update = false, dpf.denoise = false; > 2.a: update = true, dpf.denoise = true > 2.b: update = false, denoise = false > > 2.a can be further optimized to only update the denoise configuration > if the mode has changed compared to the current one > > I would rework this function as (not tested) > > const auto &denoise = controls.get(controls::draft::NoiseReductionMode); > bool update = false; > > if (denoise && *denoise != runningMode_) { > switch (*denoise) { > case controls::draft::NoiseReductionModeOff: > if (dpf.denoise) { > dpf.denoise = false; > update = true; > } > break; > case controls::draft::NoiseReductionModeMinimal: > case controls::draft::NoiseReductionModeHighQuality: > case controls::draft::NoiseReductionModeFast: > case controls::draft::NoiseReductionModeZSL: > if (loadReductionConfig(*denoise)) { > update = true; > dpf.denoise = true; > } > break; > default: > LOG(RkISP1Dpf, Error) > << "Unsupported denoise value " > << *denoise; > break; > } > > if (update) { > runningMode_ = *denoise; > LOG(RkISP1Dpf, Debug) << "Set denoise mode to " > << *denoise; > } > } > > frameContext.dpf.denoise = dpf.denoise; > frameContext.dpf.update = update; > > What do you think ? > > Thanks > j Perfectly .thanks for your input , will update the logic in next series > > > break; > > diff --git a/src/ipa/rkisp1/algorithms/dpf.h b/src/ipa/rkisp1/algorithms/dpf.h > > index bee6fc9b..30cbaa57 100644 > > --- a/src/ipa/rkisp1/algorithms/dpf.h > > +++ b/src/ipa/rkisp1/algorithms/dpf.h > > @@ -30,13 +30,24 @@ public: > > RkISP1Params *params) override; > > > > private: > > + struct ModeConfig { > > + int32_t modeValue; > > + rkisp1_cif_isp_dpf_config dpf; > > + rkisp1_cif_isp_dpf_strength_config strength; > > + }; > > + > > bool parseConfig(const YamlObject &tuningData); > > + bool parseModes(const YamlObject &tuningData); > > bool parseSingleConfig(const YamlObject &tuningData, > > rkisp1_cif_isp_dpf_config &config, > > rkisp1_cif_isp_dpf_strength_config &strengthConfig); > > > > + bool loadReductionConfig(int32_t mode); > > + > > struct rkisp1_cif_isp_dpf_config config_; > > struct rkisp1_cif_isp_dpf_strength_config strengthConfig_; > > + std::vector<ModeConfig> noiseReductionModes_; > > + int32_t runningMode_; > > }; > > > > } /* namespace ipa::rkisp1::algorithms */ > > -- > > 2.43.0 > >
diff --git a/src/ipa/rkisp1/algorithms/dpf.cpp b/src/ipa/rkisp1/algorithms/dpf.cpp index cd0a7d9d..18f2a158 100644 --- a/src/ipa/rkisp1/algorithms/dpf.cpp +++ b/src/ipa/rkisp1/algorithms/dpf.cpp @@ -37,7 +37,9 @@ namespace ipa::rkisp1::algorithms { LOG_DEFINE_CATEGORY(RkISP1Dpf) Dpf::Dpf() - : config_({}), strengthConfig_({}) + : config_({}), strengthConfig_({}), + noiseReductionModes_({}), + runningMode_(controls::draft::NoiseReductionModeOff) { } @@ -61,6 +63,53 @@ bool Dpf::parseConfig(const YamlObject &tuningData) if (!parseSingleConfig(tuningData, config_, strengthConfig_)) { return false; } + + /* Parse modes. */ + if (!parseModes(tuningData)) { + return false; + } + + return true; +} + +bool Dpf::parseModes(const YamlObject &tuningData) +{ + /* Parse noise reduction modes. */ + if (!tuningData.contains("modes")) { + return true; + } + + noiseReductionModes_.clear(); + for (const auto &entry : tuningData["modes"].asList()) { + std::optional<std::string> typeOpt = + entry["type"].get<std::string>(); + if (!typeOpt) { + LOG(RkISP1Dpf, Error) << "Modes entry missing type"; + return false; + } + + int32_t modeValue = controls::draft::NoiseReductionModeOff; + if (*typeOpt == "minimal") { + modeValue = controls::draft::NoiseReductionModeMinimal; + } else if (*typeOpt == "fast") { + modeValue = controls::draft::NoiseReductionModeFast; + } else if (*typeOpt == "highquality") { + modeValue = controls::draft::NoiseReductionModeHighQuality; + } else if (*typeOpt == "zsl") { + modeValue = controls::draft::NoiseReductionModeZSL; + } else { + LOG(RkISP1Dpf, Error) << "Unknown mode type: " << *typeOpt; + return false; + } + + ModeConfig mode{}; + mode.modeValue = modeValue; + if (!parseSingleConfig(entry, mode.dpf, mode.strength)) { + return false; + } + noiseReductionModes_.push_back(mode); + } + return true; } @@ -192,6 +241,29 @@ bool Dpf::parseSingleConfig(const YamlObject &tuningData, return true; } +bool Dpf::loadReductionConfig(int32_t mode) +{ + auto it = std::find_if(noiseReductionModes_.begin(), noiseReductionModes_.end(), + [mode](const ModeConfig &m) { + return m.modeValue == mode; + }); + if (it == noiseReductionModes_.end()) { + LOG(RkISP1Dpf, Warning) + << "No DPF config for reduction mode " + << static_cast<int>(mode); + return false; + } + + config_ = it->dpf; + strengthConfig_ = it->strength; + + LOG(RkISP1Dpf, Info) + << "DPF mode=Reduction (config loaded)" + << " mode=" << static_cast<int>(mode); + + return true; +} + /** * \copydoc libcamera::ipa::Algorithm::queueRequest */ @@ -207,6 +279,7 @@ void Dpf::queueRequest(IPAContext &context, if (denoise) { LOG(RkISP1Dpf, Debug) << "Set denoise to " << *denoise; + runningMode_ = *denoise; switch (*denoise) { case controls::draft::NoiseReductionModeOff: if (dpf.denoise) { @@ -217,8 +290,12 @@ void Dpf::queueRequest(IPAContext &context, case controls::draft::NoiseReductionModeMinimal: case controls::draft::NoiseReductionModeHighQuality: case controls::draft::NoiseReductionModeFast: - if (!dpf.denoise) { + case controls::draft::NoiseReductionModeZSL: + if (loadReductionConfig(runningMode_)) { + update = true; dpf.denoise = true; + } else { + dpf.denoise = false; update = true; } break; diff --git a/src/ipa/rkisp1/algorithms/dpf.h b/src/ipa/rkisp1/algorithms/dpf.h index bee6fc9b..30cbaa57 100644 --- a/src/ipa/rkisp1/algorithms/dpf.h +++ b/src/ipa/rkisp1/algorithms/dpf.h @@ -30,13 +30,24 @@ public: RkISP1Params *params) override; private: + struct ModeConfig { + int32_t modeValue; + rkisp1_cif_isp_dpf_config dpf; + rkisp1_cif_isp_dpf_strength_config strength; + }; + bool parseConfig(const YamlObject &tuningData); + bool parseModes(const YamlObject &tuningData); bool parseSingleConfig(const YamlObject &tuningData, rkisp1_cif_isp_dpf_config &config, rkisp1_cif_isp_dpf_strength_config &strengthConfig); + bool loadReductionConfig(int32_t mode); + struct rkisp1_cif_isp_dpf_config config_; struct rkisp1_cif_isp_dpf_strength_config strengthConfig_; + std::vector<ModeConfig> noiseReductionModes_; + int32_t runningMode_; }; } /* namespace ipa::rkisp1::algorithms */
-Add support for switching between different noise reduction modes. -Introduce `noiseReductionModes_` to store mode-specific configs. -loadReductionConfig() select specific config from configs Introduce `noiseReductionModes_` to store current configs. Signed-off-by: Rui Wang <rui.wang@ideasonboard.com> --- changelog: - Add blank line - Move V3 first patch's loadReductionConfig and reduction mode helper into this patch src/ipa/rkisp1/algorithms/dpf.cpp | 81 ++++++++++++++++++++++++++++++- src/ipa/rkisp1/algorithms/dpf.h | 11 +++++ 2 files changed, 90 insertions(+), 2 deletions(-)