[PATCH 2/2] ipa: rkisp1: agc: Fix histogram construction

[Kieran Bingham]

Quoting Paul Elder (2024-03-19 10:54:03)
> The histogram reported by the rkisp1 hardware is 20 bits, where the
> upper 16 bits are meaningful integer data and the lower 4 bits are
> fractional and meant to be discarded. Remove these 4 bits when
> constructing the histogram.
> 
> Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
> ---
>  src/ipa/rkisp1/algorithms/agc.cpp | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp
> index 47a6f7b2..278903a5 100644
> --- a/src/ipa/rkisp1/algorithms/agc.cpp
> +++ b/src/ipa/rkisp1/algorithms/agc.cpp
> @@ -352,7 +352,7 @@ double Agc::estimateLuminance(Span<const uint8_t> expMeans, double gain)
>   */
>  double Agc::measureBrightness(Span<const uint32_t> hist) const
>  {
> -       Histogram histogram{ hist };
> +       Histogram histogram{ hist, 4 };

On it's own this doesn't look very clear. So I think we need to add a
comment here above to say (in the code) why we're doing the shift.

Also, and this is only an idea really - the passing of a parameter here
feels odd. I wonder if instead we should implement operator>>= and
operator<<= on the Histogram class which will iterate over each entry and
do the shift accordingly. Otherwise, it's hard to convey 'which way' the
shift of 4 is. And what if someone wants to shift the otherway to make a
histogram fit to 20 bits?

Something like

	Histogram histogram{ hist };

	/*
	 * The RKISP1 hardware histogram is 20 bits, but represent 16
	 * bits integer and 4 bits are fractional. Shift to discard the
	 * fractional components.
	 */
	histogram >>= 4;

This might be seen as less efficient, but I think the intent and
operations are then clearer.


>         /* Estimate the quantile mean of the top 2% of the histogram. */
>         return histogram.interQuantileMean(0.98, 1.0);
>  }
> -- 
> 2.39.2
>