| Message ID | 20240419055336.1070164-1-paul.elder@ideasonboard.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
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 >
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 >>
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
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;
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
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;
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(-)