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

[Laurent Pinchart]

Hi Barnabás,

On Wed, Mar 27, 2024 at 03:21:17PM +0000, Barnabás Pőcze wrote:
> 2024. március 27., szerda 15:42 keltezéssel, Kieran Bingham í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?

We have plans to move to C++20 only to the extent that we think we'll do
it one day :-) The requirements include

- Having the language well supported by the compilers in LTS releases of
  major distributions.

  We're probably there for the latest LTS releases, but maybe not quite
  for all LTS releases that haven't reached their EOL. I haven't
  checked.

- Having ChromeOS and Android HAL layers remain compatible

  We currently implement the legacy C-based Android HAL interface. The
  new HAL interface's ABI is based binder protocols. We should thus be
  fine on Android from an ABI point of view. I haven't checked if the
  Android toolchain supports C++20 correctly though.

  On Chrome OS the situation is a bit different. The HAL ABI is C-based,
  but we link to Chrome OS C++ libraries. Chrome OS hasn't switched to
  C++20 yet, so we would probably have issues there.

- Minimizing breakage for our users

  Existing applications using libcamera may be compatible with C++20.
  They would need to upgrade, and we need to give them enough time to do
  so. This is the criteria that is the most difficult to quantify. I
  don't have any particular worry in mind personally.

-- 
Regards,

Laurent Pinchart