| Message ID | 20250812133856.3981-1-david.plowman@raspberrypi.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi David, Thank you for the patch. On Tue, 12 Aug 2025 at 14:39, David Plowman <david.plowman@raspberrypi.com> wrote: > > The CDN (colour denoise) deviation gets gradually reduced frame by > frame as TDN (temporal denoise) comes in and has more effect. CDN is > more harmful to image detail than TDN, so ramping it down in favour of > TDN is beneficial. > > The tuning file parameters are chosen so that existing tuning files > don't have to be updated and will carry on working "mostly like they > did" (apart from the new back-off). > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com> > --- > src/ipa/rpi/controller/rpi/denoise.cpp | 22 +++++++++++++++++++--- > src/ipa/rpi/controller/rpi/denoise.h | 4 +++- > 2 files changed, 22 insertions(+), 4 deletions(-) > > diff --git a/src/ipa/rpi/controller/rpi/denoise.cpp b/src/ipa/rpi/controller/rpi/denoise.cpp > index ba851658..d9bbeddd 100644 > --- a/src/ipa/rpi/controller/rpi/denoise.cpp > +++ b/src/ipa/rpi/controller/rpi/denoise.cpp > @@ -37,7 +37,17 @@ int DenoiseConfig::read(const libcamera::YamlObject ¶ms) > cdnEnable = params.contains("cdn"); > if (cdnEnable) { > auto &cdnParams = params["cdn"]; > - cdnDeviation = cdnParams["deviation"].get<double>(120); > + cdnDeviationNoTdn = cdnParams["deviation_no_tdn"].get<double>(150); > + /* > + * For backwards compatibility with existing tuning files, interpret "deviation" > + * as giving the "no TDN" deviation, where present. > + */ > + cdnDeviationNoTdn = cdnParams["deviation"].get<double>(cdnDeviationNoTdn); Do we want an if-else clause so that if both "deviation_no_tdn" and "deviation" are set in the file, we stick to the former? With or without that change: Reviewed-by: Naushir Patuck <naush@raspberrypi.com> > + /* > + * A third the value of the no TDN deviation is about right for with-TDN, if > + * the user hasn't specified otherwise. > + */ > + cdnDeviationWithTdn = cdnParams["deviation_with_tdn"].get<double>(cdnDeviationNoTdn / 3); > cdnStrength = cdnParams["strength"].get<double>(0.2); > } > > @@ -48,13 +58,14 @@ int DenoiseConfig::read(const libcamera::YamlObject ¶ms) > tdnThreshold = tdnParams["threshold"].get<double>(0.75); > } else if (sdnEnable) { > /* > - * If SDN is enabled but TDN isn't, overwrite all the SDN settings > + * If SDN is enabled but TDN isn't, overwrite all the SDN/CDN settings > * with the "no TDN" versions. This makes it easier to enable or > * disable TDN in the tuning file without editing all the other > * parameters. > */ > sdnDeviation = sdnDeviation2 = sdnDeviationNoTdn; > sdnStrength = sdnStrengthNoTdn; > + cdnDeviationWithTdn = cdnDeviationNoTdn; > } > > return 0; > @@ -107,6 +118,7 @@ void Denoise::switchMode([[maybe_unused]] CameraMode const &cameraMode, > currentSdnDeviation_ = currentConfig_->sdnDeviationNoTdn; > currentSdnStrength_ = currentConfig_->sdnStrengthNoTdn; > currentSdnDeviation2_ = currentConfig_->sdnDeviationNoTdn; > + currentCdnDeviation_ = currentConfig_->cdnDeviationNoTdn; > } > > void Denoise::prepare(Metadata *imageMetadata) > @@ -159,8 +171,12 @@ void Denoise::prepare(Metadata *imageMetadata) > > if (currentConfig_->cdnEnable && mode_ != DenoiseMode::ColourOff) { > struct CdnStatus cdn; > - cdn.threshold = currentConfig_->cdnDeviation * noiseStatus.noiseSlope + noiseStatus.noiseConstant; > + cdn.threshold = currentCdnDeviation_ * noiseStatus.noiseSlope + noiseStatus.noiseConstant; > cdn.strength = currentConfig_->cdnStrength; > + /* For the next frame, we back off the CDN deviation as TDN ramps up. */ > + double f = currentConfig_->sdnTdnBackoff; > + currentCdnDeviation_ = f * currentCdnDeviation_ + (1 - f) * currentConfig_->cdnDeviationWithTdn; > + > imageMetadata->set("cdn.status", cdn); > LOG(RPiDenoise, Debug) > << "programmed cdn threshold " << cdn.threshold > diff --git a/src/ipa/rpi/controller/rpi/denoise.h b/src/ipa/rpi/controller/rpi/denoise.h > index 79946c97..e23a2e8f 100644 > --- a/src/ipa/rpi/controller/rpi/denoise.h > +++ b/src/ipa/rpi/controller/rpi/denoise.h > @@ -23,7 +23,8 @@ struct DenoiseConfig { > double sdnDeviationNoTdn; > double sdnStrengthNoTdn; > double sdnTdnBackoff; > - double cdnDeviation; > + double cdnDeviationNoTdn; > + double cdnDeviationWithTdn; > double cdnStrength; > double tdnDeviation; > double tdnThreshold; > @@ -54,6 +55,7 @@ private: > double currentSdnDeviation_; > double currentSdnStrength_; > double currentSdnDeviation2_; > + double currentCdnDeviation_; > }; > > } // namespace RPiController > -- > 2.39.5 >
Hi Naush Thanks for the review. On Thu, 14 Aug 2025 at 08:44, Naushir Patuck <naush@raspberrypi.com> wrote: > > Hi David, > > Thank you for the patch. > > On Tue, 12 Aug 2025 at 14:39, David Plowman > <david.plowman@raspberrypi.com> wrote: > > > > The CDN (colour denoise) deviation gets gradually reduced frame by > > frame as TDN (temporal denoise) comes in and has more effect. CDN is > > more harmful to image detail than TDN, so ramping it down in favour of > > TDN is beneficial. > > > > The tuning file parameters are chosen so that existing tuning files > > don't have to be updated and will carry on working "mostly like they > > did" (apart from the new back-off). > > > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com> > > --- > > src/ipa/rpi/controller/rpi/denoise.cpp | 22 +++++++++++++++++++--- > > src/ipa/rpi/controller/rpi/denoise.h | 4 +++- > > 2 files changed, 22 insertions(+), 4 deletions(-) > > > > diff --git a/src/ipa/rpi/controller/rpi/denoise.cpp b/src/ipa/rpi/controller/rpi/denoise.cpp > > index ba851658..d9bbeddd 100644 > > --- a/src/ipa/rpi/controller/rpi/denoise.cpp > > +++ b/src/ipa/rpi/controller/rpi/denoise.cpp > > @@ -37,7 +37,17 @@ int DenoiseConfig::read(const libcamera::YamlObject ¶ms) > > cdnEnable = params.contains("cdn"); > > if (cdnEnable) { > > auto &cdnParams = params["cdn"]; > > - cdnDeviation = cdnParams["deviation"].get<double>(120); > > + cdnDeviationNoTdn = cdnParams["deviation_no_tdn"].get<double>(150); > > + /* > > + * For backwards compatibility with existing tuning files, interpret "deviation" > > + * as giving the "no TDN" deviation, where present. > > + */ > > + cdnDeviationNoTdn = cdnParams["deviation"].get<double>(cdnDeviationNoTdn); > > Do we want an if-else clause so that if both "deviation_no_tdn" and > "deviation" are set in the file, we stick to the former? Not sure, really - don't feel strongly. Or I suppose swapping the order like this would do it too: + /* + * For backwards compatibility with existing tuning files, interpret "deviation" + * as giving the "no TDN" deviation, where present. + */ + cdnDeviationNoTdn = cdnParams["deviation"].get<double>(150); + cdnDeviationNoTdn = cdnParams["deviation_no_tdn"].get<double>(cdnDeviationNoTdn); David > > With or without that change: > > Reviewed-by: Naushir Patuck <naush@raspberrypi.com> > > > > + /* > > + * A third the value of the no TDN deviation is about right for with-TDN, if > > + * the user hasn't specified otherwise. > > + */ > > + cdnDeviationWithTdn = cdnParams["deviation_with_tdn"].get<double>(cdnDeviationNoTdn / 3); > > cdnStrength = cdnParams["strength"].get<double>(0.2); > > } > > > > @@ -48,13 +58,14 @@ int DenoiseConfig::read(const libcamera::YamlObject ¶ms) > > tdnThreshold = tdnParams["threshold"].get<double>(0.75); > > } else if (sdnEnable) { > > /* > > - * If SDN is enabled but TDN isn't, overwrite all the SDN settings > > + * If SDN is enabled but TDN isn't, overwrite all the SDN/CDN settings > > * with the "no TDN" versions. This makes it easier to enable or > > * disable TDN in the tuning file without editing all the other > > * parameters. > > */ > > sdnDeviation = sdnDeviation2 = sdnDeviationNoTdn; > > sdnStrength = sdnStrengthNoTdn; > > + cdnDeviationWithTdn = cdnDeviationNoTdn; > > } > > > > return 0; > > @@ -107,6 +118,7 @@ void Denoise::switchMode([[maybe_unused]] CameraMode const &cameraMode, > > currentSdnDeviation_ = currentConfig_->sdnDeviationNoTdn; > > currentSdnStrength_ = currentConfig_->sdnStrengthNoTdn; > > currentSdnDeviation2_ = currentConfig_->sdnDeviationNoTdn; > > + currentCdnDeviation_ = currentConfig_->cdnDeviationNoTdn; > > } > > > > void Denoise::prepare(Metadata *imageMetadata) > > @@ -159,8 +171,12 @@ void Denoise::prepare(Metadata *imageMetadata) > > > > if (currentConfig_->cdnEnable && mode_ != DenoiseMode::ColourOff) { > > struct CdnStatus cdn; > > - cdn.threshold = currentConfig_->cdnDeviation * noiseStatus.noiseSlope + noiseStatus.noiseConstant; > > + cdn.threshold = currentCdnDeviation_ * noiseStatus.noiseSlope + noiseStatus.noiseConstant; > > cdn.strength = currentConfig_->cdnStrength; > > + /* For the next frame, we back off the CDN deviation as TDN ramps up. */ > > + double f = currentConfig_->sdnTdnBackoff; > > + currentCdnDeviation_ = f * currentCdnDeviation_ + (1 - f) * currentConfig_->cdnDeviationWithTdn; > > + > > imageMetadata->set("cdn.status", cdn); > > LOG(RPiDenoise, Debug) > > << "programmed cdn threshold " << cdn.threshold > > diff --git a/src/ipa/rpi/controller/rpi/denoise.h b/src/ipa/rpi/controller/rpi/denoise.h > > index 79946c97..e23a2e8f 100644 > > --- a/src/ipa/rpi/controller/rpi/denoise.h > > +++ b/src/ipa/rpi/controller/rpi/denoise.h > > @@ -23,7 +23,8 @@ struct DenoiseConfig { > > double sdnDeviationNoTdn; > > double sdnStrengthNoTdn; > > double sdnTdnBackoff; > > - double cdnDeviation; > > + double cdnDeviationNoTdn; > > + double cdnDeviationWithTdn; > > double cdnStrength; > > double tdnDeviation; > > double tdnThreshold; > > @@ -54,6 +55,7 @@ private: > > double currentSdnDeviation_; > > double currentSdnStrength_; > > double currentSdnDeviation2_; > > + double currentCdnDeviation_; > > }; > > > > } // namespace RPiController > > -- > > 2.39.5 > >
On Fri, 15 Aug 2025 at 08:00, David Plowman <david.plowman@raspberrypi.com> wrote: > > Hi Naush > > Thanks for the review. > > On Thu, 14 Aug 2025 at 08:44, Naushir Patuck <naush@raspberrypi.com> wrote: > > > > Hi David, > > > > Thank you for the patch. > > > > On Tue, 12 Aug 2025 at 14:39, David Plowman > > <david.plowman@raspberrypi.com> wrote: > > > > > > The CDN (colour denoise) deviation gets gradually reduced frame by > > > frame as TDN (temporal denoise) comes in and has more effect. CDN is > > > more harmful to image detail than TDN, so ramping it down in favour of > > > TDN is beneficial. > > > > > > The tuning file parameters are chosen so that existing tuning files > > > don't have to be updated and will carry on working "mostly like they > > > did" (apart from the new back-off). > > > > > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com> > > > --- > > > src/ipa/rpi/controller/rpi/denoise.cpp | 22 +++++++++++++++++++--- > > > src/ipa/rpi/controller/rpi/denoise.h | 4 +++- > > > 2 files changed, 22 insertions(+), 4 deletions(-) > > > > > > diff --git a/src/ipa/rpi/controller/rpi/denoise.cpp b/src/ipa/rpi/controller/rpi/denoise.cpp > > > index ba851658..d9bbeddd 100644 > > > --- a/src/ipa/rpi/controller/rpi/denoise.cpp > > > +++ b/src/ipa/rpi/controller/rpi/denoise.cpp > > > @@ -37,7 +37,17 @@ int DenoiseConfig::read(const libcamera::YamlObject ¶ms) > > > cdnEnable = params.contains("cdn"); > > > if (cdnEnable) { > > > auto &cdnParams = params["cdn"]; > > > - cdnDeviation = cdnParams["deviation"].get<double>(120); > > > + cdnDeviationNoTdn = cdnParams["deviation_no_tdn"].get<double>(150); > > > + /* > > > + * For backwards compatibility with existing tuning files, interpret "deviation" > > > + * as giving the "no TDN" deviation, where present. > > > + */ > > > + cdnDeviationNoTdn = cdnParams["deviation"].get<double>(cdnDeviationNoTdn); > > > > Do we want an if-else clause so that if both "deviation_no_tdn" and > > "deviation" are set in the file, we stick to the former? > > Not sure, really - don't feel strongly. Or I suppose swapping the > order like this would do it too: > > + /* > + * For backwards compatibility with existing tuning > files, interpret "deviation" > + * as giving the "no TDN" deviation, where present. > + */ > + cdnDeviationNoTdn = cdnParams["deviation"].get<double>(150); > + cdnDeviationNoTdn = > cdnParams["deviation_no_tdn"].get<double>(cdnDeviationNoTdn); Yup that would do! Reviewed-by: Naushir Patuck <naush@raspberrypi.com> > > David > > > > > With or without that change: > > > > Reviewed-by: Naushir Patuck <naush@raspberrypi.com> > > > > > > > + /* > > > + * A third the value of the no TDN deviation is about right for with-TDN, if > > > + * the user hasn't specified otherwise. > > > + */ > > > + cdnDeviationWithTdn = cdnParams["deviation_with_tdn"].get<double>(cdnDeviationNoTdn / 3); > > > cdnStrength = cdnParams["strength"].get<double>(0.2); > > > } > > > > > > @@ -48,13 +58,14 @@ int DenoiseConfig::read(const libcamera::YamlObject ¶ms) > > > tdnThreshold = tdnParams["threshold"].get<double>(0.75); > > > } else if (sdnEnable) { > > > /* > > > - * If SDN is enabled but TDN isn't, overwrite all the SDN settings > > > + * If SDN is enabled but TDN isn't, overwrite all the SDN/CDN settings > > > * with the "no TDN" versions. This makes it easier to enable or > > > * disable TDN in the tuning file without editing all the other > > > * parameters. > > > */ > > > sdnDeviation = sdnDeviation2 = sdnDeviationNoTdn; > > > sdnStrength = sdnStrengthNoTdn; > > > + cdnDeviationWithTdn = cdnDeviationNoTdn; > > > } > > > > > > return 0; > > > @@ -107,6 +118,7 @@ void Denoise::switchMode([[maybe_unused]] CameraMode const &cameraMode, > > > currentSdnDeviation_ = currentConfig_->sdnDeviationNoTdn; > > > currentSdnStrength_ = currentConfig_->sdnStrengthNoTdn; > > > currentSdnDeviation2_ = currentConfig_->sdnDeviationNoTdn; > > > + currentCdnDeviation_ = currentConfig_->cdnDeviationNoTdn; > > > } > > > > > > void Denoise::prepare(Metadata *imageMetadata) > > > @@ -159,8 +171,12 @@ void Denoise::prepare(Metadata *imageMetadata) > > > > > > if (currentConfig_->cdnEnable && mode_ != DenoiseMode::ColourOff) { > > > struct CdnStatus cdn; > > > - cdn.threshold = currentConfig_->cdnDeviation * noiseStatus.noiseSlope + noiseStatus.noiseConstant; > > > + cdn.threshold = currentCdnDeviation_ * noiseStatus.noiseSlope + noiseStatus.noiseConstant; > > > cdn.strength = currentConfig_->cdnStrength; > > > + /* For the next frame, we back off the CDN deviation as TDN ramps up. */ > > > + double f = currentConfig_->sdnTdnBackoff; > > > + currentCdnDeviation_ = f * currentCdnDeviation_ + (1 - f) * currentConfig_->cdnDeviationWithTdn; > > > + > > > imageMetadata->set("cdn.status", cdn); > > > LOG(RPiDenoise, Debug) > > > << "programmed cdn threshold " << cdn.threshold > > > diff --git a/src/ipa/rpi/controller/rpi/denoise.h b/src/ipa/rpi/controller/rpi/denoise.h > > > index 79946c97..e23a2e8f 100644 > > > --- a/src/ipa/rpi/controller/rpi/denoise.h > > > +++ b/src/ipa/rpi/controller/rpi/denoise.h > > > @@ -23,7 +23,8 @@ struct DenoiseConfig { > > > double sdnDeviationNoTdn; > > > double sdnStrengthNoTdn; > > > double sdnTdnBackoff; > > > - double cdnDeviation; > > > + double cdnDeviationNoTdn; > > > + double cdnDeviationWithTdn; > > > double cdnStrength; > > > double tdnDeviation; > > > double tdnThreshold; > > > @@ -54,6 +55,7 @@ private: > > > double currentSdnDeviation_; > > > double currentSdnStrength_; > > > double currentSdnDeviation2_; > > > + double currentCdnDeviation_; > > > }; > > > > > > } // namespace RPiController > > > -- > > > 2.39.5 > > >
diff --git a/src/ipa/rpi/controller/rpi/denoise.cpp b/src/ipa/rpi/controller/rpi/denoise.cpp index ba851658..d9bbeddd 100644 --- a/src/ipa/rpi/controller/rpi/denoise.cpp +++ b/src/ipa/rpi/controller/rpi/denoise.cpp @@ -37,7 +37,17 @@ int DenoiseConfig::read(const libcamera::YamlObject ¶ms) cdnEnable = params.contains("cdn"); if (cdnEnable) { auto &cdnParams = params["cdn"]; - cdnDeviation = cdnParams["deviation"].get<double>(120); + cdnDeviationNoTdn = cdnParams["deviation_no_tdn"].get<double>(150); + /* + * For backwards compatibility with existing tuning files, interpret "deviation" + * as giving the "no TDN" deviation, where present. + */ + cdnDeviationNoTdn = cdnParams["deviation"].get<double>(cdnDeviationNoTdn); + /* + * A third the value of the no TDN deviation is about right for with-TDN, if + * the user hasn't specified otherwise. + */ + cdnDeviationWithTdn = cdnParams["deviation_with_tdn"].get<double>(cdnDeviationNoTdn / 3); cdnStrength = cdnParams["strength"].get<double>(0.2); } @@ -48,13 +58,14 @@ int DenoiseConfig::read(const libcamera::YamlObject ¶ms) tdnThreshold = tdnParams["threshold"].get<double>(0.75); } else if (sdnEnable) { /* - * If SDN is enabled but TDN isn't, overwrite all the SDN settings + * If SDN is enabled but TDN isn't, overwrite all the SDN/CDN settings * with the "no TDN" versions. This makes it easier to enable or * disable TDN in the tuning file without editing all the other * parameters. */ sdnDeviation = sdnDeviation2 = sdnDeviationNoTdn; sdnStrength = sdnStrengthNoTdn; + cdnDeviationWithTdn = cdnDeviationNoTdn; } return 0; @@ -107,6 +118,7 @@ void Denoise::switchMode([[maybe_unused]] CameraMode const &cameraMode, currentSdnDeviation_ = currentConfig_->sdnDeviationNoTdn; currentSdnStrength_ = currentConfig_->sdnStrengthNoTdn; currentSdnDeviation2_ = currentConfig_->sdnDeviationNoTdn; + currentCdnDeviation_ = currentConfig_->cdnDeviationNoTdn; } void Denoise::prepare(Metadata *imageMetadata) @@ -159,8 +171,12 @@ void Denoise::prepare(Metadata *imageMetadata) if (currentConfig_->cdnEnable && mode_ != DenoiseMode::ColourOff) { struct CdnStatus cdn; - cdn.threshold = currentConfig_->cdnDeviation * noiseStatus.noiseSlope + noiseStatus.noiseConstant; + cdn.threshold = currentCdnDeviation_ * noiseStatus.noiseSlope + noiseStatus.noiseConstant; cdn.strength = currentConfig_->cdnStrength; + /* For the next frame, we back off the CDN deviation as TDN ramps up. */ + double f = currentConfig_->sdnTdnBackoff; + currentCdnDeviation_ = f * currentCdnDeviation_ + (1 - f) * currentConfig_->cdnDeviationWithTdn; + imageMetadata->set("cdn.status", cdn); LOG(RPiDenoise, Debug) << "programmed cdn threshold " << cdn.threshold diff --git a/src/ipa/rpi/controller/rpi/denoise.h b/src/ipa/rpi/controller/rpi/denoise.h index 79946c97..e23a2e8f 100644 --- a/src/ipa/rpi/controller/rpi/denoise.h +++ b/src/ipa/rpi/controller/rpi/denoise.h @@ -23,7 +23,8 @@ struct DenoiseConfig { double sdnDeviationNoTdn; double sdnStrengthNoTdn; double sdnTdnBackoff; - double cdnDeviation; + double cdnDeviationNoTdn; + double cdnDeviationWithTdn; double cdnStrength; double tdnDeviation; double tdnThreshold; @@ -54,6 +55,7 @@ private: double currentSdnDeviation_; double currentSdnStrength_; double currentSdnDeviation2_; + double currentCdnDeviation_; }; } // namespace RPiController
The CDN (colour denoise) deviation gets gradually reduced frame by frame as TDN (temporal denoise) comes in and has more effect. CDN is more harmful to image detail than TDN, so ramping it down in favour of TDN is beneficial. The tuning file parameters are chosen so that existing tuning files don't have to be updated and will carry on working "mostly like they did" (apart from the new back-off). Signed-off-by: David Plowman <david.plowman@raspberrypi.com> --- src/ipa/rpi/controller/rpi/denoise.cpp | 22 +++++++++++++++++++--- src/ipa/rpi/controller/rpi/denoise.h | 4 +++- 2 files changed, 22 insertions(+), 4 deletions(-)