| Message ID | 20260108-sklug-lsc-resampling-v2-dev-v2-9-e682ec4b9893@ideasonboard.com |
|---|---|
| State | Superseded |
| 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 >>
Hi Barnabás, Quoting Barnabás Pőcze (2026-01-09 11:03:50) > 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. Sure. > > > >> +{ > >> + 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.) Not exactly. Assume we have a quantization of 10, and the input value oscillates between 44 and 46, then the logic would constantly toggle between 40 and 50 which could produce visible flickering. So we need a threshold on the input to prevent flickering. With the current threshold being equal to the quantization we could actually drop the second check as the quantization has definitely changed when the threshold was reached. There is one downside though. In the above example if you hit 44, you will not reach a quantization of 50 when the value goes up, as the next point that will be applied is 55 and therefore quantized to 60. So writing this, I think we should reduce the threshold to quantization/2 (and possible increase the quantization) but keep the logic (maybe revert the order of ifs to test for the threshold first and then for the quantization which makes it easier to follow logically. Best regards, Stefan > > > 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 > >> >
2026. 01. 12. 10:15 keltezéssel, Stefan Klug írta: > Hi Barnabás, > > Quoting Barnabás Pőcze (2026-01-09 11:03:50) >> 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. > > Sure. > >> >> >>>> +{ >>>> + 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.) > > Not exactly. Assume we have a quantization of 10, and the input value > oscillates between 44 and 46, then the logic would constantly toggle > between 40 and 50 which could produce visible flickering. So we need a > threshold on the input to prevent flickering. With the current threshold > being equal to the quantization we could actually drop the second check > as the quantization has definitely changed when the threshold was > reached. There is one downside though. In the above example if you hit Maybe I'm misunderstanding, but all I'm saying is that if `quantizedCt == lastAppliedQuantizedCt_` then `std::abs(ctDiff) < kColourTemperatureChangeThreshhold`. Thus the `quantizedCt == lastAppliedQuantizedCt_` check can be dropped. This is of course assuming that the quantization coefficient and the threshold are the same. If the `quantizedCt == lastAppliedQuantizedCt_` check is removed, then oscillation between 44 and 46 will still be "ignored" because `std::abs(ctDiff) < kColourTemperatureChangeThreshhold` holds true. Regards, Barnabás Pőcze > 44, you will not reach a quantization of 50 when the value goes up, as > the next point that will be applied is 55 and therefore quantized to 60. > So writing this, I think we should reduce the threshold to > quantization/2 (and possible increase the quantization) but keep the > logic (maybe revert the order of ifs to test for the threshold first and > then for the quantization which makes it easier to follow logically. > > Best regards, > Stefan > >> >> >> 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(-)