| Message ID | 20250319160146.61751-2-stefan.klug@ideasonboard.com |
|---|---|
| State | Accepted |
| Commit | 0539e88679df7454d761b607ea82b351e4747a8b |
| Headers | show |
| Series |
|
| Related | show |
Hi Stefan On 19/03/2025 16:01, Stefan Klug wrote: > 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@ideasonboard.com> > --- Looks good to me: Reviewed-by: Daniel Scally <dan.scally@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); > + 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_; > };
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@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 ? > + 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 >
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@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@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 > > >
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); + 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_; };
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@ideasonboard.com> --- src/ipa/rkisp1/algorithms/agc.cpp | 13 ++++++++++--- src/ipa/rkisp1/algorithms/agc.h | 1 + 2 files changed, 11 insertions(+), 3 deletions(-)