| Message ID | 20260108-sklug-lsc-resampling-v2-dev-v2-9-e682ec4b9893@ideasonboard.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
Quoting Stefan Klug (2026-01-08 16:05:52) > The quantization functionality in the Interpolator hinders in writing > nice code. Don't use it and implement the functionality directly in the > algorithm. This patch doesn't introduce any functional changes. > > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com> > > --- > > Changes in v2: > - Added this patch > --- > src/ipa/rkisp1/algorithms/lsc.cpp | 19 +++++++++++++------ > 1 file changed, 13 insertions(+), 6 deletions(-) > > diff --git a/src/ipa/rkisp1/algorithms/lsc.cpp b/src/ipa/rkisp1/algorithms/lsc.cpp > index c42f74557c7364935254d75ee1fb923d176e0dbd..d5e44021906bda22f16a2c0f2a344e4c40d46774 100644 > --- a/src/ipa/rkisp1/algorithms/lsc.cpp > +++ b/src/ipa/rkisp1/algorithms/lsc.cpp > @@ -308,12 +308,16 @@ std::vector<double> parseSizes(const YamlObject &tuningData, > return sizes; > } > > +static unsigned int quantize(unsigned int value, unsigned int step) > +{ > + return std::lround(value / static_cast<double>(step)) * step; > +} > + > } /* namespace */ > > LensShadingCorrection::LensShadingCorrection() > : lastAppliedCt_(0), lastAppliedQuantizedCt_(0) > { > - sets_.setQuantization(kColourTemperatureChangeThreshhold); > } > > /** > @@ -426,17 +430,20 @@ void LensShadingCorrection::prepare([[maybe_unused]] IPAContext &context, > RkISP1Params *params) > { > uint32_t ct = frameContext.awb.temperatureK; > - if (std::abs(static_cast<int>(ct) - static_cast<int>(lastAppliedCt_)) < > - kColourTemperatureChangeThreshhold) > + unsigned int quantizedCt = quantize(ct, kColourTemperatureChangeThreshhold); > + int ctDiff = static_cast<int>(ct) - static_cast<int>(lastAppliedCt_); > + > + if (quantizedCt == lastAppliedQuantizedCt_) > return; Is this one redundant? I think it would also return on the next ctDiff check too? But maybe there's an edge case I'm not thinking about. Anyway, I don't think it's 'wrong'. If the quantizedCt is the same, then we'll generate the same table so no operations required! Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > - unsigned int quantizedCt; > - const Components &set = sets_.getInterpolated(ct, &quantizedCt); > - if (lastAppliedQuantizedCt_ == quantizedCt) > + > + if (std::abs(ctDiff) < kColourTemperatureChangeThreshhold) > return; > > auto config = params->block<BlockType::Lsc>(); > config.setEnabled(true); > setParameters(*config); > + > + const Components &set = sets_.getInterpolated(quantizedCt); > copyTable(*config, set); > > lastAppliedCt_ = ct; > > -- > 2.51.0 >
2026. 01. 08. 21:23 keltezéssel, Kieran Bingham írta: > Quoting Stefan Klug (2026-01-08 16:05:52) >> The quantization functionality in the Interpolator hinders in writing >> nice code. Don't use it and implement the functionality directly in the >> algorithm. This patch doesn't introduce any functional changes. >> >> Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com> >> >> --- >> >> Changes in v2: >> - Added this patch >> --- >> src/ipa/rkisp1/algorithms/lsc.cpp | 19 +++++++++++++------ >> 1 file changed, 13 insertions(+), 6 deletions(-) >> >> diff --git a/src/ipa/rkisp1/algorithms/lsc.cpp b/src/ipa/rkisp1/algorithms/lsc.cpp >> index c42f74557c7364935254d75ee1fb923d176e0dbd..d5e44021906bda22f16a2c0f2a344e4c40d46774 100644 >> --- a/src/ipa/rkisp1/algorithms/lsc.cpp >> +++ b/src/ipa/rkisp1/algorithms/lsc.cpp >> @@ -308,12 +308,16 @@ std::vector<double> parseSizes(const YamlObject &tuningData, >> return sizes; >> } >> >> +static unsigned int quantize(unsigned int value, unsigned int step) No `static` needed. >> +{ >> + return std::lround(value / static_cast<double>(step)) * step; >> +} >> + >> } /* namespace */ >> >> LensShadingCorrection::LensShadingCorrection() >> : lastAppliedCt_(0), lastAppliedQuantizedCt_(0) >> { >> - sets_.setQuantization(kColourTemperatureChangeThreshhold); >> } >> >> /** >> @@ -426,17 +430,20 @@ void LensShadingCorrection::prepare([[maybe_unused]] IPAContext &context, >> RkISP1Params *params) >> { >> uint32_t ct = frameContext.awb.temperatureK; >> - if (std::abs(static_cast<int>(ct) - static_cast<int>(lastAppliedCt_)) < >> - kColourTemperatureChangeThreshhold) >> + unsigned int quantizedCt = quantize(ct, kColourTemperatureChangeThreshhold); >> + int ctDiff = static_cast<int>(ct) - static_cast<int>(lastAppliedCt_); >> + >> + if (quantizedCt == lastAppliedQuantizedCt_) >> return; > > Is this one redundant? I think it would also return on the next ctDiff > check too? But maybe there's an edge case I'm not thinking about. If I haven't made a mistake, then my calculations also suggest that it is redundant. That is `quantizedCt == lastAppliedQuantizedCt_` implies that `std::abs(ctDiff) < kColourTemperatureChangeThreshhold`, so the earlier condition may be dropped; especially since there are no "expensive" calculations between the two. (That is, of course, if my calculations are correct.) Regards, Barnabás Pőcze > > Anyway, I don't think it's 'wrong'. If the quantizedCt is the same, then > we'll generate the same table so no operations required! > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > >> - unsigned int quantizedCt; >> - const Components &set = sets_.getInterpolated(ct, &quantizedCt); >> - if (lastAppliedQuantizedCt_ == quantizedCt) >> + >> + if (std::abs(ctDiff) < kColourTemperatureChangeThreshhold) >> return; >> >> auto config = params->block<BlockType::Lsc>(); >> config.setEnabled(true); >> setParameters(*config); >> + >> + const Components &set = sets_.getInterpolated(quantizedCt); >> copyTable(*config, set); >> >> lastAppliedCt_ = ct; >> >> -- >> 2.51.0 >>
diff --git a/src/ipa/rkisp1/algorithms/lsc.cpp b/src/ipa/rkisp1/algorithms/lsc.cpp index c42f74557c7364935254d75ee1fb923d176e0dbd..d5e44021906bda22f16a2c0f2a344e4c40d46774 100644 --- a/src/ipa/rkisp1/algorithms/lsc.cpp +++ b/src/ipa/rkisp1/algorithms/lsc.cpp @@ -308,12 +308,16 @@ std::vector<double> parseSizes(const YamlObject &tuningData, return sizes; } +static unsigned int quantize(unsigned int value, unsigned int step) +{ + return std::lround(value / static_cast<double>(step)) * step; +} + } /* namespace */ LensShadingCorrection::LensShadingCorrection() : lastAppliedCt_(0), lastAppliedQuantizedCt_(0) { - sets_.setQuantization(kColourTemperatureChangeThreshhold); } /** @@ -426,17 +430,20 @@ void LensShadingCorrection::prepare([[maybe_unused]] IPAContext &context, RkISP1Params *params) { uint32_t ct = frameContext.awb.temperatureK; - if (std::abs(static_cast<int>(ct) - static_cast<int>(lastAppliedCt_)) < - kColourTemperatureChangeThreshhold) + unsigned int quantizedCt = quantize(ct, kColourTemperatureChangeThreshhold); + int ctDiff = static_cast<int>(ct) - static_cast<int>(lastAppliedCt_); + + if (quantizedCt == lastAppliedQuantizedCt_) return; - unsigned int quantizedCt; - const Components &set = sets_.getInterpolated(ct, &quantizedCt); - if (lastAppliedQuantizedCt_ == quantizedCt) + + if (std::abs(ctDiff) < kColourTemperatureChangeThreshhold) return; auto config = params->block<BlockType::Lsc>(); config.setEnabled(true); setParameters(*config); + + const Components &set = sets_.getInterpolated(quantizedCt); copyTable(*config, set); lastAppliedCt_ = ct;
The quantization functionality in the Interpolator hinders in writing nice code. Don't use it and implement the functionality directly in the algorithm. This patch doesn't introduce any functional changes. Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com> --- Changes in v2: - Added this patch --- src/ipa/rkisp1/algorithms/lsc.cpp | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-)