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

[Laurent Pinchart]

On Tue, Mar 19, 2024 at 12:39:27PM +0000, Kieran Bingham wrote:
> 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.

Are they completely unusable, or are they fractional bits that could be
helpful ? I wonder if we shouldn't drop them in the kernel if there's no
way whatsoever to use them.

> > 
> > 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;

What if we need other types of conversions in the future ? This doesn't
seem very generic. Passing a lambda function that transforms the value
would seem better, and would also be more readable. You don't want that
function to be called for every value without allowing for compiler
optimization though, so the constructor would likely need to be inlined.

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

Efficiency-wise, that's not great indeed :-(

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

-- 
Regards,

Laurent Pinchart