[PATCH v2] ipa: libipa: histogram: Add transform parameter to constructor
Barnabás Pőcze
pobrn at protonmail.com
Tue Apr 23 16:21:20 CEST 2024
Hi
2024. április 19., péntek 22:44 keltezéssel, Kieran Bingham <kieran.bingham at 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 at 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 at 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
More information about the libcamera-devel
mailing list