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

[Laurent Pinchart]

Hi Paul,

Thank you for the patch.

On Wed, May 08, 2024 at 11:59:24PM +0900, Paul Elder wrote:
> 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>
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> Reviewed-by: Daniel Scally <dan.scally at ideasonboard.com>
> 
> ---
> Changes in v4:
> - fix compilation when called with no transform function
> 
> Changes in v3:
> - make the transform function a template parameter, and optimize the
>   constructor
> 
> 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 | 14 ++++++++++----
>  src/ipa/libipa/histogram.h   | 13 ++++++++++++-
>  2 files changed, 22 insertions(+), 5 deletions(-)
> 
> diff --git a/src/ipa/libipa/histogram.cpp b/src/ipa/libipa/histogram.cpp
> index c1aac59b..212acfdf 100644
> --- a/src/ipa/libipa/histogram.cpp
> +++ b/src/ipa/libipa/histogram.cpp
> @@ -43,12 +43,18 @@ namespace ipa {
>   */
>  Histogram::Histogram(Span<const uint32_t> data)
>  {
> -	cumulative_.reserve(data.size());
> -	cumulative_.push_back(0);
> -	for (const uint32_t &value : data)
> -		cumulative_.push_back(cumulative_.back() + value);
> +	cumulative_.resize(data.size() + 1);
> +	cumulative_[0] = 0;
> +	for (const auto &[i, value] : utils::enumerate(data))
> +		cumulative_[i + 1] = cumulative_[i] + value;

This is a nice optimization. I would have split it to another patch
though, or at least mentioned it in the commit message.

>  }
>  
> +/**
> + * \brief Create a cumulative histogram
> + * \param[in] data A pre-sorted histogram to be passed

Looks like there's room for improvement in the existing documentation of
the constructor. "pre-sorted" doesn't make too much sense here, and
neither does "to be passed".

> + * \param[in] transform The transformation function to apply to every bin
> + */
> +
>  /**
>   * \fn Histogram::bins()
>   * \brief Retrieve the number of bins currently used by the Histogram
> diff --git a/src/ipa/libipa/histogram.h b/src/ipa/libipa/histogram.h
> index 54bb2a19..0e9bef2a 100644
> --- a/src/ipa/libipa/histogram.h
> +++ b/src/ipa/libipa/histogram.h
> @@ -10,10 +10,10 @@
>  #include <assert.h>
>  #include <limits.h>
>  #include <stdint.h>

You need to include <type_traits> for std::enable_if_t and
std::is_invocable_v.

With these small issues fixed,

Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

> -
>  #include <vector>
>  
>  #include <libcamera/base/span.h>
> +#include <libcamera/base/utils.h>
>  
>  namespace libcamera {
>  
> @@ -24,6 +24,17 @@ class Histogram
>  public:
>  	Histogram() { cumulative_.push_back(0); }
>  	Histogram(Span<const uint32_t> data);
> +
> +	template<typename Transform,
> +		 std::enable_if_t<std::is_invocable_v<Transform, uint32_t>> * = nullptr>
> +	Histogram(Span<const uint32_t> data, Transform transform)
> +	{
> +		cumulative_.resize(data.size() + 1);
> +		cumulative_[0] = 0;
> +		for (const auto &[i, value] : utils::enumerate(data))
> +			cumulative_[i + 1] = cumulative_[i] + transform(value);
> +	}
> +
>  	size_t bins() const { return cumulative_.size() - 1; }
>  	uint64_t total() const { return cumulative_[cumulative_.size() - 1]; }
>  	uint64_t cumulativeFrequency(double bin) const;

-- 
Regards,

Laurent Pinchart