[2/2] ipa: rkisp1: agc: Fix histogram construction
diff mbox series

Message ID 20240319105403.4045592-3-paul.elder@ideasonboard.com
State Accepted
Headers show
Series
  • ipa: rkisp1: Fix histogram
Related show

Commit Message

Paul Elder March 19, 2024, 10:54 a.m. UTC
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.

Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
---
 src/ipa/rkisp1/algorithms/agc.cpp | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Kieran Bingham March 19, 2024, 12:39 p.m. UTC | #1
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.
> 
> Signed-off-by: Paul Elder <paul.elder@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;

This might be seen as less efficient, but I think the intent and
operations are then clearer.


>         /* Estimate the quantile mean of the top 2% of the histogram. */
>         return histogram.interQuantileMean(0.98, 1.0);
>  }
> -- 
> 2.39.2
>
Laurent Pinchart March 19, 2024, 12:48 p.m. UTC | #2
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.

> > 
> > Signed-off-by: Paul Elder <paul.elder@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.

> This might be seen as less efficient, but I think the intent and
> operations are then clearer.

Efficiency-wise, that's not great indeed :-(

> >         /* Estimate the quantile mean of the top 2% of the histogram. */
> >         return histogram.interQuantileMean(0.98, 1.0);
> >  }
Paul Elder March 27, 2024, 9:35 a.m. UTC | #3
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@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 :-(
> 
> > >         /* Estimate the quantile mean of the top 2% of the histogram. */
> > >         return histogram.interQuantileMean(0.98, 1.0);
> > >  }
Kieran Bingham March 27, 2024, 11:46 a.m. UTC | #4
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@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
Barnabás Pőcze March 27, 2024, 1:44 p.m. UTC | #5
Hi


2024. március 27., szerda 12:46 keltezéssel, Kieran Bingham <kieran.bingham@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@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
Kieran Bingham March 27, 2024, 2:42 p.m. UTC | #6
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
Barnabás Pőcze March 27, 2024, 3:21 p.m. UTC | #7
Hi


2024. március 27., szerda 15:42 keltezéssel, Kieran Bingham <kieran.bingham@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
Laurent Pinchart April 30, 2024, 9:01 a.m. UTC | #8
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.

Patch
diff mbox series

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 };
 	/* Estimate the quantile mean of the top 2% of the histogram. */
 	return histogram.interQuantileMean(0.98, 1.0);
 }