[PATCH v2 5/9] ipa: rkisp1: Use interpolator in lsc
Kieran Bingham
kieran.bingham at ideasonboard.com
Fri Sep 13 11:56:39 CEST 2024
Quoting Kieran Bingham (2024-09-13 10:48:21)
> Quoting Stefan Klug (2024-09-13 08:57:23)
> > Now, that the generic interpolator is available, use it to do the
> > interpolation of the lens shading tables. This makes the algorithm
> > easier to read and remove some duplicate code.
> >
> > Signed-off-by: Stefan Klug <stefan.klug at ideasonboard.com>
> > ---
> > src/ipa/rkisp1/algorithms/lsc.cpp | 166 +++++++++---------------------
> > src/ipa/rkisp1/algorithms/lsc.h | 13 +--
> > 2 files changed, 57 insertions(+), 122 deletions(-)
> >
> > diff --git a/src/ipa/rkisp1/algorithms/lsc.cpp b/src/ipa/rkisp1/algorithms/lsc.cpp
> > index 5f3a0388075b..87a04ec048f8 100644
> > --- a/src/ipa/rkisp1/algorithms/lsc.cpp
> > +++ b/src/ipa/rkisp1/algorithms/lsc.cpp
> > @@ -24,6 +24,36 @@
> >
> > namespace libcamera {
> >
> > +namespace ipa {
> > +
> > +constexpr int kColourTemperatureChangeThreshhold = 10;
> > +
> > +template<typename T>
> > +void interpolateVector(const std::vector<T> &a, const std::vector<T> &b,
> > + std::vector<T> &dest, double lambda)
> > +{
> > + assert(a.size() == b.size());
>
> I wonder if we should do
> if (a.size() != b.size()
> LOG(RkISP1Lsc, Fatal) << "Interpolation must be identical sizes";
>
> I assume this should never be possible to happen even if people have
> 'faulty' tuning files?
>
> But if that's the case, I guess it would have to be handled at a higher
> layer than the core interpolate implementation.
I think I've convinced myself that this is covered by the YAML loader -
so a plain assert is fine to enforce the contract.
>
>
> > + dest.resize(a.size());
> > + for (size_t i = 0; i < a.size(); i++) {
> > + dest[i] = a[i] * (1.0 - lambda) + b[i] * lambda;
> > + }
> > +}
> > +
> > +template<>
> > +void Interpolator<rkisp1::algorithms::LensShadingCorrection::Components>::
> > + interpolate(const rkisp1::algorithms::LensShadingCorrection::Components &a,
> > + const rkisp1::algorithms::LensShadingCorrection::Components &b,
> > + rkisp1::algorithms::LensShadingCorrection::Components &dest,
> > + double lambda)
> > +{
> > + interpolateVector(a.r, b.r, dest.r, lambda);
> > + interpolateVector(a.gr, b.gr, dest.gr, lambda);
> > + interpolateVector(a.gb, b.gb, dest.gb, lambda);
> > + interpolateVector(a.b, b.b, dest.b, lambda);
> > +}
>
> I like how easy it is to make custom interpolators now.
>
> > +
> > +} // namespace ipa
> > +
> > namespace ipa::rkisp1::algorithms {
> >
> > /**
> > @@ -90,8 +120,9 @@ static std::vector<uint16_t> parseTable(const YamlObject &tuningData,
> > }
> >
> > LensShadingCorrection::LensShadingCorrection()
> > - : lastCt_({ 0, 0 })
> > + : lastAppliedCt_(0), lastAppliedQuantizedCt_(0)
> > {
> > + sets_.setQuantization(kColourTemperatureChangeThreshhold);
> > }
> >
> > /**
> > @@ -115,17 +146,18 @@ int LensShadingCorrection::init([[maybe_unused]] IPAContext &context,
> > }
> >
> > const auto &sets = yamlSets.asList();
> > + std::map<unsigned int, Components> lscData;
> > for (const auto &yamlSet : sets) {
> > uint32_t ct = yamlSet["ct"].get<uint32_t>(0);
> >
> > - if (sets_.count(ct)) {
> > + if (lscData.count(ct)) {
> > LOG(RkISP1Lsc, Error)
> > << "Multiple sets found for color temperature "
> > << ct;
> > return -EINVAL;
> > }
> >
> > - Components &set = sets_[ct];
> > + Components &set = lscData[ct];
> >
>
> Is this where we can do some validation on the input data to
> ensure/guarantee we don't assert later on ? Or reject the file and
> report to the user?
>
> Worrying about input validation is the only issue blocking me here.
>
>
And I think I didn't see it in this patch as the yaml loader isn't
affected, but I have convinced myself that this is safe because it's
handled there.
Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>
> > set.ct = ct;
> > set.r = parseTable(yamlSet, "r");
> > @@ -142,11 +174,13 @@ int LensShadingCorrection::init([[maybe_unused]] IPAContext &context,
> > }
> > }
> >
> > - if (sets_.empty()) {
> > + if (lscData.empty()) {
> > LOG(RkISP1Lsc, Error) << "Failed to load any sets";
> > return -EINVAL;
> > }
> >
> > + sets_.setData(std::move(lscData));
> > +
> > return 0;
> > }
> >
> > @@ -202,134 +236,34 @@ void LensShadingCorrection::copyTable(rkisp1_cif_isp_lsc_config &config,
> > std::copy(set.b.begin(), set.b.end(), &config.b_data_tbl[0][0]);
> > }
> >
> > -/*
> > - * Interpolate LSC parameters based on color temperature value.
> > - */
> > -void LensShadingCorrection::interpolateTable(rkisp1_cif_isp_lsc_config &config,
> > - const Components &set0,
> > - const Components &set1,
> > - const uint32_t ct)
> > -{
> > - double coeff0 = (set1.ct - ct) / static_cast<double>(set1.ct - set0.ct);
> > - double coeff1 = (ct - set0.ct) / static_cast<double>(set1.ct - set0.ct);
> > -
> > - for (unsigned int i = 0; i < RKISP1_CIF_ISP_LSC_SAMPLES_MAX; ++i) {
> > - for (unsigned int j = 0; j < RKISP1_CIF_ISP_LSC_SAMPLES_MAX; ++j) {
> > - unsigned int sample = i * RKISP1_CIF_ISP_LSC_SAMPLES_MAX + j;
> > -
> > - config.r_data_tbl[i][j] =
> > - set0.r[sample] * coeff0 +
> > - set1.r[sample] * coeff1;
> > -
> > - config.gr_data_tbl[i][j] =
> > - set0.gr[sample] * coeff0 +
> > - set1.gr[sample] * coeff1;
> > -
> > - config.gb_data_tbl[i][j] =
> > - set0.gb[sample] * coeff0 +
> > - set1.gb[sample] * coeff1;
> > -
> > - config.b_data_tbl[i][j] =
> > - set0.b[sample] * coeff0 +
> > - set1.b[sample] * coeff1;
> > - }
> > - }
> > -}
> > -
> > /**
> > * \copydoc libcamera::ipa::Algorithm::prepare
> > */
> > void LensShadingCorrection::prepare(IPAContext &context,
> > - const uint32_t frame,
> > + [[maybe_unused]] const uint32_t frame,
> > [[maybe_unused]] IPAFrameContext &frameContext,
> > RkISP1Params *params)
> > {
> > - /*
> > - * If there is only one set, the configuration has already been done
> > - * for first frame.
> > - */
> > - if (sets_.size() == 1 && frame > 0)
> > - return;
> > -
> > - /*
> > - * If there is only one set, pick it. We can ignore lastCt_, as it will
> > - * never be relevant.
> > - */
> > - if (sets_.size() == 1) {
> > - auto config = params->block<BlockType::Lsc>();
> > - config.setEnabled(true);
> > -
> > - setParameters(*config);
> > - copyTable(*config, sets_.cbegin()->second);
> > - return;
> > - }
> > -
> > uint32_t ct = context.activeState.awb.temperatureK;
> > - ct = std::clamp(ct, sets_.cbegin()->first, sets_.crbegin()->first);
> > -
> > - /*
> > - * If the original is the same, then it means the same adjustment would
> > - * be made. If the adjusted is the same, then it means that it's the
> > - * same as what was actually applied. Thus in these cases we can skip
> > - * reprogramming the LSC.
> > - *
> > - * original == adjusted can only happen if an interpolation
> > - * happened, or if original has an exact entry in sets_. This means
> > - * that if original != adjusted, then original was adjusted to
> > - * the nearest available entry in sets_, resulting in adjusted.
> > - * Clearly, any ct value that is in between original and adjusted
> > - * will be adjusted to the same adjusted value, so we can skip
> > - * reprogramming the LSC table.
> > - *
> > - * We also skip updating the original value, as the last one had a
> > - * larger bound and thus a larger range of ct values that will be
> > - * adjusted to the same adjusted.
> > - */
> > - if ((lastCt_.original <= ct && ct <= lastCt_.adjusted) ||
> > - (lastCt_.adjusted <= ct && ct <= lastCt_.original))
> > + if (std::abs(static_cast<int>(ct) - static_cast<int>(lastAppliedCt_)) <
> > + kColourTemperatureChangeThreshhold)
> > + return;
> > + unsigned int quantizedCt;
> > + const Components &set = sets_.getInterpolated(ct, &quantizedCt);
> > + if (lastAppliedQuantizedCt_ == quantizedCt)
> > return;
> >
> > auto config = params->block<BlockType::Lsc>();
> > config.setEnabled(true);
> > setParameters(*config);
> > + copyTable(*config, set);
> >
> > - /*
> > - * The color temperature matches exactly one of the available LSC tables.
> > - */
> > - if (sets_.count(ct)) {
> > - copyTable(*config, sets_[ct]);
> > - lastCt_ = { ct, ct };
> > - return;
> > - }
> > + lastAppliedCt_ = ct;
> > + lastAppliedQuantizedCt_ = quantizedCt;
> >
> > - /* No shortcuts left; we need to round or interpolate */
> > - auto iter = sets_.upper_bound(ct);
> > - const Components &set1 = iter->second;
> > - const Components &set0 = (--iter)->second;
> > - uint32_t ct0 = set0.ct;
> > - uint32_t ct1 = set1.ct;
> > - uint32_t diff0 = ct - ct0;
> > - uint32_t diff1 = ct1 - ct;
> > - static constexpr double kThreshold = 0.1;
> > - float threshold = kThreshold * (ct1 - ct0);
> > -
> > - if (diff0 < threshold || diff1 < threshold) {
> > - const Components &set = diff0 < diff1 ? set0 : set1;
> > - LOG(RkISP1Lsc, Debug) << "using LSC table for " << set.ct;
> > - copyTable(*config, set);
> > - lastCt_ = { ct, set.ct };
> > - return;
> > - }
> > -
> > - /*
> > - * ct is not within 10% of the difference between the neighbouring
> > - * color temperatures, so we need to interpolate.
> > - */
> > LOG(RkISP1Lsc, Debug)
> > - << "ct is " << ct << ", interpolating between "
> > - << ct0 << " and " << ct1;
> > - interpolateTable(*config, set0, set1, ct);
> > - lastCt_ = { ct, ct };
> > + << "ct is " << ct << ", quantized to "
> > + << quantizedCt;
> > }
> >
> > REGISTER_IPA_ALGORITHM(LensShadingCorrection, "LensShadingCorrection")
> > diff --git a/src/ipa/rkisp1/algorithms/lsc.h b/src/ipa/rkisp1/algorithms/lsc.h
> > index a9c7a230e0fc..5a0824e36dd5 100644
> > --- a/src/ipa/rkisp1/algorithms/lsc.h
> > +++ b/src/ipa/rkisp1/algorithms/lsc.h
> > @@ -9,6 +9,8 @@
> >
> > #include <map>
> >
> > +#include "libipa/interpolator.h"
> > +
> > #include "algorithm.h"
> >
> > namespace libcamera {
> > @@ -27,7 +29,6 @@ public:
> > IPAFrameContext &frameContext,
> > RkISP1Params *params) override;
> >
> > -private:
> > struct Components {
> > uint32_t ct;
> > std::vector<uint16_t> r;
> > @@ -36,23 +37,23 @@ private:
> > std::vector<uint16_t> b;
> > };
> >
> > +private:
> > void setParameters(rkisp1_cif_isp_lsc_config &config);
> > void copyTable(rkisp1_cif_isp_lsc_config &config, const Components &set0);
> > void interpolateTable(rkisp1_cif_isp_lsc_config &config,
> > const Components &set0, const Components &set1,
> > const uint32_t ct);
> >
> > - std::map<uint32_t, Components> sets_;
> > + ipa::Interpolator<Components> sets_;
> > std::vector<double> xSize_;
> > std::vector<double> ySize_;
> > uint16_t xGrad_[RKISP1_CIF_ISP_LSC_SECTORS_TBL_SIZE];
> > uint16_t yGrad_[RKISP1_CIF_ISP_LSC_SECTORS_TBL_SIZE];
> > uint16_t xSizes_[RKISP1_CIF_ISP_LSC_SECTORS_TBL_SIZE];
> > uint16_t ySizes_[RKISP1_CIF_ISP_LSC_SECTORS_TBL_SIZE];
> > - struct {
> > - uint32_t original;
> > - uint32_t adjusted;
> > - } lastCt_;
> > +
> > + unsigned int lastAppliedCt_;
> > + unsigned int lastAppliedQuantizedCt_;
> > };
> >
> > } /* namespace ipa::rkisp1::algorithms */
> > --
> > 2.43.0
> >
More information about the libcamera-devel
mailing list