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

[Kieran Bingham]

Quoting Paul Elder (2024-03-27 09:35:43)
> 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 :-(

Sure, but what's our cost here. Is this a hot path? Or are we talking
about iterating a table of 5-10 values twice once to add them and once to
shift them right? How big is the histogram expected to be?

If we're under a cacheline or two in size ~64 bytes? then is it better
to prefer readability against over-optimisations?

But I'm probably fine with a lambda here too.

How would that look ? I can't quite figure out how to do this inline
without a copy (which would probably make any performance optimisation redundant
anyway?)


Is this what you had in mind?

    double measureBrightness(std::span<const uint32_t> hist) const {
        std::vector<uint32_t> shiftedHist;
        shiftedHist.reserve(hist.size());

        /*
         * The RKISP1 hardware histogram is 20 bits, but represents 16
         * bits integer and 4 bits are fractional. Shift to discard the
         * fractional components.
         */
        std::transform(hist.begin(), hist.end(),
                       std::back_inserter(shiftedHist),
                        [](uint32_t value) { return value >> 4; });

        Histogram histogram{ shiftedHist };

For full context, here's my sketch I tried to do it in:
  https://godbolt.org/z/e9sTbYncb

I'm equally fine with something like the above though. And it puts the
expressiveness of what the adjustment is and why it's required in the
platform specific components.

--
Kieran