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

[Paul Elder]

On Tue, Mar 19, 2024 at 02:48:49PM +0200, Laurent Pinchart wrote:
> 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.

The programming guide says to not use them. They're generated because
the hardware does weighting and it doesn't actually count every single
pixel, apparently.

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

idk I can only imagine needing arithmetic operations... which we could
add as we need them...?

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

Yeah I think you're right.


Paul

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