[PATCH v2] ipa: libipa: histogram: Add transform parameter to constructor
Barnabás Pőcze
pobrn at protonmail.com
Fri May 3 03:43:22 CEST 2024
Hi
2024. május 3., péntek 3:20 keltezéssel, Laurent Pinchart <laurent.pinchart at 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 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 {
> > > > [...]
> > > > -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
More information about the libcamera-devel
mailing list