[PATCH 1/2] ipa: rkisp1: agc: Fix metering modes

[Kieran Bingham]

Quoting Stefan Klug (2025-03-25 13:46:12)
> Hi Kieran,
> 
> Thank you for the review.
> 
> On Tue, Mar 25, 2025 at 10:18:46AM +0000, Kieran Bingham wrote:
> > Quoting Stefan Klug (2025-03-19 16:01:36)
> > > The weights for a given metering mode are applied to the histogram data
> > > inside the histogram statistics block. The AE statistics do not contain
> > > any weights. Therefore the weights are honored when AgcMeanLuminance
> > > calculates the upper or lower constraints, but ignored in the
> > > calculation of the frame luminance. Fix that by manually applying the
> > > weights in the luminance calculation.
> > > 
> > > Fixes: 4c5152843a2a ("ipa: rkisp1: Derive rkisp1::algorithms::Agc from AgcMeanLuminance")
> > > Signed-off-by: Stefan Klug <stefan.klug at ideasonboard.com>
> > > ---
> > >  src/ipa/rkisp1/algorithms/agc.cpp | 13 ++++++++++---
> > >  src/ipa/rkisp1/algorithms/agc.h   |  1 +
> > >  2 files changed, 11 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp
> > > index 5a3ba0131cf1..101cad5d0893 100644
> > > --- a/src/ipa/rkisp1/algorithms/agc.cpp
> > > +++ b/src/ipa/rkisp1/algorithms/agc.cpp
> > > @@ -439,15 +439,20 @@ void Agc::fillMetadata(IPAContext &context, IPAFrameContext &frameContext,
> > >   */
> > >  double Agc::estimateLuminance(double gain) const
> > >  {
> > > +       ASSERT(expMeans_.size() == weights_.size());
> > >         double ySum = 0.0;
> > > +       double wSum = 0.0;
> > >  
> > >         /* Sum the averages, saturated to 255. */
> > > -       for (uint8_t expMean : expMeans_)
> > > -               ySum += std::min(expMean * gain, 255.0);
> > > +       for (unsigned i = 0; i < expMeans_.size(); i++) {
> > > +               double w = weights_[i];
> > > +               ySum += std::min(expMeans_[i] * gain, 255.0) * w;
> > > +               wSum += w;
> > > +       }
> > >  
> > >         /* \todo Weight with the AWB gains */
> > >  
> > > -       return ySum / expMeans_.size() / 255;
> > > +       return ySum / wSum / 255;
> > >  }
> > >  
> > >  /**
> > > @@ -515,6 +520,8 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,
> > >         Histogram hist({ params->hist.hist_bins, context.hw->numHistogramBins },
> > >                        [](uint32_t x) { return x >> 4; });
> > >         expMeans_ = { params->ae.exp_mean, context.hw->numAeCells };
> > > +       std::vector<uint8_t> &modeWeights = meteringModes_.at(frameContext.agc.meteringMode);
> > 
> > Where do the weights come from? Are they always guaranteed to be
> > available at this point ?
> 
> The weights come from the tuning file. The libtuning agc module contains
> hardcoded values for centre weighted, spot and matrix. If there are no
> metering modes in the tuning file, a matrix mode is generated within
> parseMeteringModes().  So yes, they are always available at this point.

Then I'm happy:


Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>

> 
> Best regards,
> Stefan
> 
> > 
> > > +       weights_ = { modeWeights.data(), modeWeights.size() };
> > >  
> > >         /*
> > >          * Set the AGC limits using the fixed exposure time and/or gain in
> > > diff --git a/src/ipa/rkisp1/algorithms/agc.h b/src/ipa/rkisp1/algorithms/agc.h
> > > index 62bcde999fe3..7867eed9c4e3 100644
> > > --- a/src/ipa/rkisp1/algorithms/agc.h
> > > +++ b/src/ipa/rkisp1/algorithms/agc.h
> > > @@ -55,6 +55,7 @@ private:
> > >                                   utils::Duration frameDuration);
> > >  
> > >         Span<const uint8_t> expMeans_;
> > > +       Span<const uint8_t> weights_;
> > >  
> > >         std::map<int32_t, std::vector<uint8_t>> meteringModes_;
> > >  };
> > > -- 
> > > 2.43.0
> > >