| Message ID | 20240508103546.693219-1-paul.elder@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
On Wed, May 08, 2024 at 07:35:46PM +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> Oops, I forgot to pull in Kieran's and Dan's rb tags. Paul > > --- > 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 | 8 +------- > src/ipa/libipa/histogram.h | 14 ++++++++++++-- > 2 files changed, 13 insertions(+), 9 deletions(-) > > diff --git a/src/ipa/libipa/histogram.cpp b/src/ipa/libipa/histogram.cpp > index c1aac59b..628e3755 100644 > --- a/src/ipa/libipa/histogram.cpp > +++ b/src/ipa/libipa/histogram.cpp > @@ -40,14 +40,8 @@ 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) > -{ > - cumulative_.reserve(data.size()); > - cumulative_.push_back(0); > - for (const uint32_t &value : data) > - cumulative_.push_back(cumulative_.back() + value); > -} > > /** > * \fn Histogram::bins() > diff --git a/src/ipa/libipa/histogram.h b/src/ipa/libipa/histogram.h > index 54bb2a19..26326125 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 { > > @@ -23,7 +23,17 @@ class Histogram > { > public: > Histogram() { cumulative_.push_back(0); } > - Histogram(Span<const uint32_t> data); > + > + template<typename Transform> > + Histogram(Span<const uint32_t> data, > + Transform transform = [](uint32_t x) { return x; }) > + { > + 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; > -- > 2.39.2 >
Hi 2024. május 8., szerda 12:35 keltezéssel, Paul Elder <paul.elder@ideasonboard.com> írta: > 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> > > [...] > diff --git a/src/ipa/libipa/histogram.h b/src/ipa/libipa/histogram.h > index 54bb2a19..26326125 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 { > > @@ -23,7 +23,17 @@ class Histogram > { > public: > Histogram() { cumulative_.push_back(0); } > - Histogram(Span<const uint32_t> data); > + > + template<typename Transform> > + Histogram(Span<const uint32_t> data, > + Transform transform = [](uint32_t x) { return x; }) > + { > + cumulative_.resize(data.size() + 1); > + cumulative_[0] = 0; > + for (const auto &[i, value] : utils::enumerate(data)) > + cumulative_[i + 1] = cumulative_[i] + transform(value); > + } As far as I am aware this won't compile if called with just a single argument. You can solve that by adding another constructor, like this: Histogram(Span<const uint32_t> data) : Histogram(data, [](uint32_t x) { return x; }) { } and removing the default argument from the other. > [...] Regards, Barnabás Pőcze
On Wed, May 08, 2024 at 01:08:31PM +0000, Barnabás Pőcze wrote: > Hi > > > 2024. május 8., szerda 12:35 keltezéssel, Paul Elder <paul.elder@ideasonboard.com> írta: > > > 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> > > > > [...] > > diff --git a/src/ipa/libipa/histogram.h b/src/ipa/libipa/histogram.h > > index 54bb2a19..26326125 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 { > > > > @@ -23,7 +23,17 @@ class Histogram > > { > > public: > > Histogram() { cumulative_.push_back(0); } > > - Histogram(Span<const uint32_t> data); > > + > > + template<typename Transform> > > + Histogram(Span<const uint32_t> data, > > + Transform transform = [](uint32_t x) { return x; }) > > + { > > + cumulative_.resize(data.size() + 1); > > + cumulative_[0] = 0; > > + for (const auto &[i, value] : utils::enumerate(data)) > > + cumulative_[i + 1] = cumulative_[i] + transform(value); > > + } > > As far as I am aware this won't compile if called with just a single argument. > > You can solve that by adding another constructor, like this: > > Histogram(Span<const uint32_t> data) > : Histogram(data, [](uint32_t x) { return x; }) > { } Yeeep I realized that soon after... New version incoming! Paul > > and removing the default argument from the other. > > > > [...] >
diff --git a/src/ipa/libipa/histogram.cpp b/src/ipa/libipa/histogram.cpp index c1aac59b..628e3755 100644 --- a/src/ipa/libipa/histogram.cpp +++ b/src/ipa/libipa/histogram.cpp @@ -40,14 +40,8 @@ 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) -{ - cumulative_.reserve(data.size()); - cumulative_.push_back(0); - for (const uint32_t &value : data) - cumulative_.push_back(cumulative_.back() + value); -} /** * \fn Histogram::bins() diff --git a/src/ipa/libipa/histogram.h b/src/ipa/libipa/histogram.h index 54bb2a19..26326125 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 { @@ -23,7 +23,17 @@ class Histogram { public: Histogram() { cumulative_.push_back(0); } - Histogram(Span<const uint32_t> data); + + template<typename Transform> + Histogram(Span<const uint32_t> data, + Transform transform = [](uint32_t x) { return x; }) + { + 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;
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> --- 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 | 8 +------- src/ipa/libipa/histogram.h | 14 ++++++++++++-- 2 files changed, 13 insertions(+), 9 deletions(-)