[RFC,v1,2/2] ipa: mali-c55: agc: Ignore invalid statistics

28771 diff mbox series

It is possible that, for example, the first frame has no valid statistics
yet. In that case the histograms are empty. This is problematic because
e.g. `interQuantileMean()` may return NaN, which causes issues in the
calculations (this affects both the color temperature calculation and
`AgcTraits::estimanteLuminance()` as well).

Previously this has not manifested itself because implementation details in
`AgcMeanLuminance` prevented the NaNs from propagating, but 25cb6cd274d3
("libipa: agc_mean_luminance: Improve inital gain estimation") has changed that.

So ignore statistics if any of the color histograms are empty.

Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
---

It is possible that maybe something else should be adjusted in the kernel
or libcamera so that even the first image is processed with the right
parameters. Because it appears to me the first parameter buffer only
takes effect after the first frame.

---
 src/ipa/mali-c55/algorithms/agc.cpp | 26 ++++++++++++++++----------
 src/ipa/mali-c55/algorithms/agc.h   |  5 +++++
 2 files changed, 21 insertions(+), 10 deletions(-)

--
2.56.0

Quoting Barnabás Pőcze (2026-11-05 09:01:56)
> It is possible that, for example, the first frame has no valid statistics
> yet. In that case the histograms are empty. This is problematic because
> e.g. `interQuantileMean()` may return NaN, which causes issues in the
> calculations (this affects both the color temperature calculation and
> `AgcTraits::estimanteLuminance()` as well).
> 
> Previously this has not manifested itself because implementation details in
> `AgcMeanLuminance` prevented the NaNs from propagating, but 25cb6cd274d3
> ("libipa: agc_mean_luminance: Improve inital gain estimation") has changed that.
> 
> So ignore statistics if any of the color histograms are empty.
> 
> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
> ---
> 
> It is possible that maybe something else should be adjusted in the kernel
> or libcamera so that even the first image is processed with the right
> parameters. Because it appears to me the first parameter buffer only
> takes effect after the first frame.
> 
> ---
>  src/ipa/mali-c55/algorithms/agc.cpp | 26 ++++++++++++++++----------
>  src/ipa/mali-c55/algorithms/agc.h   |  5 +++++
>  2 files changed, 21 insertions(+), 10 deletions(-)
> 
> diff --git a/src/ipa/mali-c55/algorithms/agc.cpp b/src/ipa/mali-c55/algorithms/agc.cpp
> index 2c747da891..d665ba5244 100644
> --- a/src/ipa/mali-c55/algorithms/agc.cpp
> +++ b/src/ipa/mali-c55/algorithms/agc.cpp
> @@ -257,16 +257,22 @@ void Agc::process(IPAContext &context,
>         }
> 
>         statistics_.parseStatistics(stats);
> -       context.activeState.agc.temperatureK = estimateCCT({ { statistics_.rHist.interQuantileMean(0, 1),
> -                                                              statistics_.gHist.interQuantileMean(0, 1),
> -                                                              statistics_.bHist.interQuantileMean(0, 1) } });
> -
> -       agc_.process(context.configuration.agc, context.activeState.agc, frameContext.agc, {{
> -               .traits = AgcTraits(statistics_),
> -               .yHist = statistics_.yHist,
> -               .exposure = frameContext.sensor.exposure,
> -               .gain = frameContext.sensor.gain,
> -       }}, metadata);
> +       if (statistics_.isValid()) {
> +               context.activeState.agc.temperatureK = estimateCCT(
> +                       { { statistics_.rHist.interQuantileMean(0, 1),
> +                           statistics_.gHist.interQuantileMean(0, 1),
> +                           statistics_.bHist.interQuantileMean(0, 1) } });
> +
> +               agc_.process(context.configuration.agc, context.activeState.agc, frameContext.agc, {{
> +                       .traits = AgcTraits(statistics_),
> +                       .yHist = statistics_.yHist,
> +                       .exposure = frameContext.sensor.exposure,
> +                       .gain = frameContext.sensor.gain,
> +               }}, metadata);
> +       } else {
> +               LOG(MaliC55Agc, Debug) << "ignoring invalid statistics";
> +               agc_.process(context.configuration.agc, context.activeState.agc, frameContext.agc, {}, metadata);
> +       }
> 

I started thinking about the usual, what if we check the error condition
and return early, but then we'd have to duplicate this. So I guess
there's not much in it:


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

>         metadata.set(controls::ColourTemperature, context.activeState.agc.temperatureK);
>  }
> diff --git a/src/ipa/mali-c55/algorithms/agc.h b/src/ipa/mali-c55/algorithms/agc.h
> index 809a57c53a..998ef57210 100644
> --- a/src/ipa/mali-c55/algorithms/agc.h
> +++ b/src/ipa/mali-c55/algorithms/agc.h
> @@ -32,6 +32,11 @@ public:
>         uint32_t decodeBinValue(uint16_t binVal);
>         void parseStatistics(const mali_c55_stats_buffer *stats);
> 
> +       bool isValid() const
> +       {
> +               return !rHist.empty() && !gHist.empty() && !bHist.empty();
> +       }

That's much nicer to make it easier to read.

> +
>         Histogram rHist;
>         Histogram gHist;
>         Histogram bHist;
> --
> 2.56.0