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

[Barnabás Pőcze]

Hi


2024. március 27., szerda 12:46 keltezéssel, Kieran Bingham <kieran.bingham at ideasonboard.com> írta:

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

I know libcamera does not utilize C++20, but I just wanted to mention that the
ranges library would provide a nice solution to this, I think.
E.g. you could do something like

  // e.g. std::span<const std::uint32_t> values
  histogram { values | std::views::transform([](auto x) { return x >> 4; }) }

( https://gcc.godbolt.org/z/f88TGqGW6 )

And the values would be shifted when creating inserting into the vector,
without the need to allocate a separate vector, like in the code above.


Regards,
Barnabás Pőcze