[PATCH v2] ipa: libipa: histogram: Add transform parameter to constructor
Dan Scally
dan.scally at ideasonboard.com
Tue Apr 23 00:38:14 CEST 2024
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 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.
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 at 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 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
>>
More information about the libcamera-devel
mailing list