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

[Kieran Bingham]

Quoting Barnabás Pőcze (2024-03-27 13:44:23)
> Hi
> 
> > 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.

Aha, nice - that's what I thought I could try to do with a lambda,
but failed without the copy of course - hence the above.

But indeed, alas, we may be some time away from C++20.
--
Kieran


> 
> 
> Regards,
> Barnabás Pőcze