[v4] ipa: libipa: histogram: Add transform parameter to constructor
diff mbox series

Message ID 20240508145924.1477599-1-paul.elder@ideasonboard.com
State Superseded
Headers show
Series
  • [v4] ipa: libipa: histogram: Add transform parameter to constructor
Related show

Commit Message

Paul Elder May 8, 2024, 2:59 p.m. UTC 
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@ideasonboard.com>
Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Reviewed-by: Daniel Scally <dan.scally@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(-)

Comments

Laurent Pinchart May 8, 2024, 3:23 p.m. UTC | #1 
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@ideasonboard.com>
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> Reviewed-by: Daniel Scally <dan.scally@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@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;
Paul Elder May 9, 2024, 6:17 a.m. UTC | #2 
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@ideasonboard.com>
> > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > Reviewed-by: Daniel Scally <dan.scally@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@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

Patch
diff mbox series 

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;
 }
 
+/**
+ * \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
+ */
+
 /**
  * \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>
-
 #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;