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

[Barnabás Pőcze]

Hi


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

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

The constructor could take a lambda like this, which should be equally performant,
even if a bit more restricted than what is possible with ranges:

  template<typename Func, std::enable_if_t<std::is_same_v<std::invoke_result_t<Func, std::uint32_t>, std::uint32_t>> * = nullptr>
  histogram(std::span<const std::uint32_t> values, Func transform)
  {
    for (auto x : values)
      this->values.push_back(transform(x));
    ...
  }

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

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

Are there any plans by the way? What are the criteria?


Regards,
Barnabás Pőcze