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

[Paul Elder]

On Wed, May 08, 2024 at 06:23:18PM +0300, Laurent Pinchart wrote:
> 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.
> 

Oh yeah true.

> >  }
> >  
> > +/**
> > + * \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".

"But it does have to be pre-sorted... oh wait, we're passing in a
non-cumulative histogram... so it's already sorted by definition... ok
I see"

> 
> > + * \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.

ack

> 
> With these small issues fixed,
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

Thanks,

Paul

> 
> > -
> >  #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