[PATCH v2] ipa: libipa: histogram: Add transform parameter to constructor

[Kieran Bingham]

Quoting Paul Elder (2024-04-19 06:53:36)
> Add a parameter to the histogram constructor that takes a transformation
> function to apply to all the bins upon construction.
> 
> This is necessary notably for the rkisp1, as the values reported from
> the hardware are 20 bits where the upper 16-bits are meaningful integer
> values and the lower 4 bits are fractional and meant to be discarded. As
> adding a right-shift parameter is probably too specialized, a generic
> function is added as a parameter instead.
> 
> Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
> 
> ---
> This used to be "ipa: libipa: histogram: Add rshift parameter to
> constructor"
> 
> Changes in v2:
> - change rshift parameter to a function parameter
> ---
>  src/ipa/libipa/histogram.cpp | 6 ++++--
>  src/ipa/libipa/histogram.h   | 6 ++++--
>  2 files changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/src/ipa/libipa/histogram.cpp b/src/ipa/libipa/histogram.cpp
> index c1aac59b..2a4095f4 100644
> --- a/src/ipa/libipa/histogram.cpp
> +++ b/src/ipa/libipa/histogram.cpp
> @@ -40,13 +40,15 @@ namespace ipa {
>  /**
>   * \brief Create a cumulative histogram
>   * \param[in] data A pre-sorted histogram to be passed
> + * \param[in] transform The transformation function to apply to every bin

I would have likely said "A transformation function to apply to every
bin" but ... it doesn't really impact here.

>   */
> -Histogram::Histogram(Span<const uint32_t> data)
> +Histogram::Histogram(Span<const uint32_t> data,
> +                    std::function<uint32_t(const uint32_t)> transform)
>  {
>         cumulative_.reserve(data.size());
>         cumulative_.push_back(0);
>         for (const uint32_t &value : data)
> -               cumulative_.push_back(cumulative_.back() + value);
> +               cumulative_.push_back(cumulative_.back() + transform(value));
>  }
>  
>  /**
> diff --git a/src/ipa/libipa/histogram.h b/src/ipa/libipa/histogram.h
> index 54bb2a19..89a6b550 100644
> --- a/src/ipa/libipa/histogram.h
> +++ b/src/ipa/libipa/histogram.h
> @@ -8,9 +8,9 @@
>  #pragma once
>  
>  #include <assert.h>
> +#include <functional>
>  #include <limits.h>
>  #include <stdint.h>
> -
>  #include <vector>
>  
>  #include <libcamera/base/span.h>
> @@ -23,7 +23,9 @@ class Histogram
>  {
>  public:
>         Histogram() { cumulative_.push_back(0); }
> -       Histogram(Span<const uint32_t> data);
> +       Histogram(Span<const uint32_t> data,
> +                 std::function<uint32_t(const uint32_t)> transform =
> +                       [](uint32_t x) { return x; });

Well, that default transform is likely quite easy for a compiler to
optimise inline so I think this is as generic as we get here!

Thanks for reworking.

Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>

>         size_t bins() const { return cumulative_.size() - 1; }
>         uint64_t total() const { return cumulative_[cumulative_.size() - 1]; }
>         uint64_t cumulativeFrequency(double bin) const;
> -- 
> 2.39.2
>