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

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

Commit Message

Paul Elder April 19, 2024, 5:53 a.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>

---
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(-)

Comments

Kieran Bingham April 19, 2024, 8:44 p.m. UTC | #1
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@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@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
>
Dan Scally April 22, 2024, 10:38 p.m. UTC | #2
On 19/04/2024 21:44, Kieran Bingham wrote:
> 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@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.


I had the same thought...on the grounds that "The" implies it's mandatory whereas "A" does not. But 
I  think I'm just being picky:


Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com>

>
>>    */
>> -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@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
>>
Barnabás Pőcze April 23, 2024, 2:21 p.m. UTC | #3
Hi


2024. április 19., péntek 22:44 keltezéssel, Kieran Bingham <kieran.bingham@ideasonboard.com> írta:

> 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@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!

Unfortunately, there will be no inlining without LTO here. And std::function has
not insignificant overhead even if inlined. For inlining to happen reliably and
without much overhead, the constructor would need to be templated and inline.


> 
> Thanks for reworking.
> 
> Reviewed-by: Kieran Bingham <kieran.bingham@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
> >
> 


Regards,
Barnabás Pőcze
Laurent Pinchart May 3, 2024, 1:20 a.m. UTC | #4
On Tue, Apr 23, 2024 at 02:21:20PM +0000, Barnabás Pőcze wrote:
> Hi
> 
> 
> 2024. április 19., péntek 22:44 keltezéssel, Kieran Bingham <kieran.bingham@ideasonboard.com> írta:
> 
> > 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@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!
> 
> Unfortunately, there will be no inlining without LTO here. And std::function has
> not insignificant overhead even if inlined. For inlining to happen reliably and
> without much overhead, the constructor would need to be templated and inline.

Do you mean passing the transform as a template parameter ? That could
be a good idea, I don't expect a big difference in code size.

Paul, could you give it a try, and check how big the constructor gets ?

> > Thanks for reworking.
> > 
> > Reviewed-by: Kieran Bingham <kieran.bingham@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;
Barnabás Pőcze May 3, 2024, 1:43 a.m. UTC | #5
Hi


2024. május 3., péntek 3:20 keltezéssel, Laurent Pinchart <laurent.pinchart@ideasonboard.com> írta:

> On Tue, Apr 23, 2024 at 02:21:20PM +0000, Barnabás Pőcze wrote:
> > Hi
> >
> >
> > 2024. április 19., péntek 22:44 keltezéssel, Kieran Bingham <kieran.bingham@ideasonboard.com> írta:
> >
> > > 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@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 {
> > > >  [...]
> > > > -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
> > > > [...]
> > > > @@ -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!
> >
> > Unfortunately, there will be no inlining without LTO here. And std::function has
> > not insignificant overhead even if inlined. For inlining to happen reliably and
> > without much overhead, the constructor would need to be templated and inline.
> 
> Do you mean passing the transform as a template parameter ? That could
> be a good idea, I don't expect a big difference in code size.

Yes.


> 
> Paul, could you give it a try, and check how big the constructor gets ?

Avoiding the push_back()s would also curb the code size somewhat.
E.g.

  template<typename Transform>
  Histogram(Span<const uint32_t> data, Transform transform)
  {
    cumulative_.resize(data.size() + 1);
    cumulative_[0] = 0;
    for (const auto &[i, val] : enumerate(data))
      cumulative[i + 1] = cumulative_[i] + transform(val);
  }

> [...]


Regards,
Barnabás Pőcze

Patch
diff mbox series

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
  */
-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; });
 	size_t bins() const { return cumulative_.size() - 1; }
 	uint64_t total() const { return cumulative_[cumulative_.size() - 1]; }
 	uint64_t cumulativeFrequency(double bin) const;