| Message ID | 20240319105403.4045592-3-paul.elder@ideasonboard.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
Quoting Paul Elder (2024-03-19 10:54:03) > The histogram reported by the rkisp1 hardware is 20 bits, where the > upper 16 bits are meaningful integer data and the lower 4 bits are > fractional and meant to be discarded. Remove these 4 bits when > constructing the histogram. > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> > --- > src/ipa/rkisp1/algorithms/agc.cpp | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp > index 47a6f7b2..278903a5 100644 > --- a/src/ipa/rkisp1/algorithms/agc.cpp > +++ b/src/ipa/rkisp1/algorithms/agc.cpp > @@ -352,7 +352,7 @@ double Agc::estimateLuminance(Span<const uint8_t> expMeans, double gain) > */ > double Agc::measureBrightness(Span<const uint32_t> hist) const > { > - Histogram histogram{ hist }; > + Histogram histogram{ hist, 4 }; On it's own this doesn't look very clear. So I think we need to add a comment here above to say (in the code) why we're doing the shift. Also, and this is only an idea really - the passing of a parameter here feels odd. I wonder if instead we should implement operator>>= and operator<<= on the Histogram class which will iterate over each entry and do the shift accordingly. Otherwise, it's hard to convey 'which way' the shift of 4 is. And what if someone wants to shift the otherway to make a histogram fit to 20 bits? Something like Histogram histogram{ hist }; /* * The RKISP1 hardware histogram is 20 bits, but represent 16 * bits integer and 4 bits are fractional. Shift to discard the * fractional components. */ histogram >>= 4; This might be seen as less efficient, but I think the intent and operations are then clearer. > /* Estimate the quantile mean of the top 2% of the histogram. */ > return histogram.interQuantileMean(0.98, 1.0); > } > -- > 2.39.2 >
On Tue, Mar 19, 2024 at 12:39:27PM +0000, Kieran Bingham wrote: > Quoting Paul Elder (2024-03-19 10:54:03) > > The histogram reported by the rkisp1 hardware is 20 bits, where the > > upper 16 bits are meaningful integer data and the lower 4 bits are > > fractional and meant to be discarded. Remove these 4 bits when > > constructing the histogram. Are they completely unusable, or are they fractional bits that could be helpful ? I wonder if we shouldn't drop them in the kernel if there's no way whatsoever to use them. > > > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> > > --- > > src/ipa/rkisp1/algorithms/agc.cpp | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp > > index 47a6f7b2..278903a5 100644 > > --- a/src/ipa/rkisp1/algorithms/agc.cpp > > +++ b/src/ipa/rkisp1/algorithms/agc.cpp > > @@ -352,7 +352,7 @@ double Agc::estimateLuminance(Span<const uint8_t> expMeans, double gain) > > */ > > double Agc::measureBrightness(Span<const uint32_t> hist) const > > { > > - Histogram histogram{ hist }; > > + Histogram histogram{ hist, 4 }; > > On it's own this doesn't look very clear. So I think we need to add a > comment here above to say (in the code) why we're doing the shift. > > Also, and this is only an idea really - the passing of a parameter here > feels odd. I wonder if instead we should implement operator>>= and > operator<<= on the Histogram class which will iterate over each entry and > do the shift accordingly. Otherwise, it's hard to convey 'which way' the > shift of 4 is. And what if someone wants to shift the otherway to make a > histogram fit to 20 bits? > > Something like > > Histogram histogram{ hist }; > > /* > * The RKISP1 hardware histogram is 20 bits, but represent 16 > * bits integer and 4 bits are fractional. Shift to discard the > * fractional components. > */ > histogram >>= 4; What if we need other types of conversions in the future ? This doesn't seem very generic. Passing a lambda function that transforms the value would seem better, and would also be more readable. You don't want that function to be called for every value without allowing for compiler optimization though, so the constructor would likely need to be inlined. > This might be seen as less efficient, but I think the intent and > operations are then clearer. Efficiency-wise, that's not great indeed :-( > > /* Estimate the quantile mean of the top 2% of the histogram. */ > > return histogram.interQuantileMean(0.98, 1.0); > > }
On Tue, Mar 19, 2024 at 02:48:49PM +0200, Laurent Pinchart wrote: > On Tue, Mar 19, 2024 at 12:39:27PM +0000, Kieran Bingham wrote: > > Quoting Paul Elder (2024-03-19 10:54:03) > > > The histogram reported by the rkisp1 hardware is 20 bits, where the > > > upper 16 bits are meaningful integer data and the lower 4 bits are > > > fractional and meant to be discarded. Remove these 4 bits when > > > constructing the histogram. > > Are they completely unusable, or are they fractional bits that could be > helpful ? I wonder if we shouldn't drop them in the kernel if there's no > way whatsoever to use them. The programming guide says to not use them. They're generated because the hardware does weighting and it doesn't actually count every single pixel, apparently. > > > > > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> > > > --- > > > src/ipa/rkisp1/algorithms/agc.cpp | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp > > > index 47a6f7b2..278903a5 100644 > > > --- a/src/ipa/rkisp1/algorithms/agc.cpp > > > +++ b/src/ipa/rkisp1/algorithms/agc.cpp > > > @@ -352,7 +352,7 @@ double Agc::estimateLuminance(Span<const uint8_t> expMeans, double gain) > > > */ > > > double Agc::measureBrightness(Span<const uint32_t> hist) const > > > { > > > - Histogram histogram{ hist }; > > > + Histogram histogram{ hist, 4 }; > > > > On it's own this doesn't look very clear. So I think we need to add a > > comment here above to say (in the code) why we're doing the shift. > > > > Also, and this is only an idea really - the passing of a parameter here > > feels odd. I wonder if instead we should implement operator>>= and > > operator<<= on the Histogram class which will iterate over each entry and > > do the shift accordingly. Otherwise, it's hard to convey 'which way' the > > shift of 4 is. And what if someone wants to shift the otherway to make a > > histogram fit to 20 bits? > > > > Something like > > > > Histogram histogram{ hist }; > > > > /* > > * The RKISP1 hardware histogram is 20 bits, but represent 16 > > * bits integer and 4 bits are fractional. Shift to discard the > > * fractional components. > > */ > > histogram >>= 4; > > What if we need other types of conversions in the future ? This doesn't > seem very generic. Passing a lambda function that transforms the value > would seem better, and would also be more readable. You don't want that > function to be called for every value without allowing for compiler > optimization though, so the constructor would likely need to be inlined. idk I can only imagine needing arithmetic operations... which we could add as we need them...? > > This might be seen as less efficient, but I think the intent and > > operations are then clearer. Yeah I think you're right. Paul > Efficiency-wise, that's not great indeed :-( > > > > /* Estimate the quantile mean of the top 2% of the histogram. */ > > > return histogram.interQuantileMean(0.98, 1.0); > > > }
Quoting Paul Elder (2024-03-27 09:35:43) > On Tue, Mar 19, 2024 at 02:48:49PM +0200, Laurent Pinchart wrote: > > On Tue, Mar 19, 2024 at 12:39:27PM +0000, Kieran Bingham wrote: > > > Quoting Paul Elder (2024-03-19 10:54:03) > > > > The histogram reported by the rkisp1 hardware is 20 bits, where the > > > > upper 16 bits are meaningful integer data and the lower 4 bits are > > > > fractional and meant to be discarded. Remove these 4 bits when > > > > constructing the histogram. > > > > Are they completely unusable, or are they fractional bits that could be > > helpful ? I wonder if we shouldn't drop them in the kernel if there's no > > way whatsoever to use them. > > The programming guide says to not use them. They're generated because > the hardware does weighting and it doesn't actually count every single > pixel, apparently. > > > > > > > > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> > > > > --- > > > > src/ipa/rkisp1/algorithms/agc.cpp | 2 +- > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp > > > > index 47a6f7b2..278903a5 100644 > > > > --- a/src/ipa/rkisp1/algorithms/agc.cpp > > > > +++ b/src/ipa/rkisp1/algorithms/agc.cpp > > > > @@ -352,7 +352,7 @@ double Agc::estimateLuminance(Span<const uint8_t> expMeans, double gain) > > > > */ > > > > double Agc::measureBrightness(Span<const uint32_t> hist) const > > > > { > > > > - Histogram histogram{ hist }; > > > > + Histogram histogram{ hist, 4 }; > > > > > > On it's own this doesn't look very clear. So I think we need to add a > > > comment here above to say (in the code) why we're doing the shift. > > > > > > Also, and this is only an idea really - the passing of a parameter here > > > feels odd. I wonder if instead we should implement operator>>= and > > > operator<<= on the Histogram class which will iterate over each entry and > > > do the shift accordingly. Otherwise, it's hard to convey 'which way' the > > > shift of 4 is. And what if someone wants to shift the otherway to make a > > > histogram fit to 20 bits? > > > > > > Something like > > > > > > Histogram histogram{ hist }; > > > > > > /* > > > * The RKISP1 hardware histogram is 20 bits, but represent 16 > > > * bits integer and 4 bits are fractional. Shift to discard the > > > * fractional components. > > > */ > > > histogram >>= 4; > > > > What if we need other types of conversions in the future ? This doesn't > > seem very generic. Passing a lambda function that transforms the value > > would seem better, and would also be more readable. You don't want that > > function to be called for every value without allowing for compiler > > optimization though, so the constructor would likely need to be inlined. > > idk I can only imagine needing arithmetic operations... which we could > add as we need them...? > > > > This might be seen as less efficient, but I think the intent and > > > operations are then clearer. > > Yeah I think you're right. > > > Paul > > > Efficiency-wise, that's not great indeed :-( Sure, but what's our cost here. Is this a hot path? Or are we talking about iterating a table of 5-10 values twice once to add them and once to shift them right? How big is the histogram expected to be? If we're under a cacheline or two in size ~64 bytes? then is it better to prefer readability against over-optimisations? But I'm probably fine with a lambda here too. How would that look ? I can't quite figure out how to do this inline without a copy (which would probably make any performance optimisation redundant anyway?) Is this what you had in mind? double measureBrightness(std::span<const uint32_t> hist) const { std::vector<uint32_t> shiftedHist; shiftedHist.reserve(hist.size()); /* * The RKISP1 hardware histogram is 20 bits, but represents 16 * bits integer and 4 bits are fractional. Shift to discard the * fractional components. */ std::transform(hist.begin(), hist.end(), std::back_inserter(shiftedHist), [](uint32_t value) { return value >> 4; }); Histogram histogram{ shiftedHist }; For full context, here's my sketch I tried to do it in: https://godbolt.org/z/e9sTbYncb I'm equally fine with something like the above though. And it puts the expressiveness of what the adjustment is and why it's required in the platform specific components. -- Kieran
Hi 2024. március 27., szerda 12:46 keltezéssel, Kieran Bingham <kieran.bingham@ideasonboard.com> írta: > Quoting Paul Elder (2024-03-27 09:35:43) > > On Tue, Mar 19, 2024 at 02:48:49PM +0200, Laurent Pinchart wrote: > > > On Tue, Mar 19, 2024 at 12:39:27PM +0000, Kieran Bingham wrote: > > > > Quoting Paul Elder (2024-03-19 10:54:03) > > > > > The histogram reported by the rkisp1 hardware is 20 bits, where the > > > > > upper 16 bits are meaningful integer data and the lower 4 bits are > > > > > fractional and meant to be discarded. Remove these 4 bits when > > > > > constructing the histogram. > > > > > > Are they completely unusable, or are they fractional bits that could be > > > helpful ? I wonder if we shouldn't drop them in the kernel if there's no > > > way whatsoever to use them. > > > > The programming guide says to not use them. They're generated because > > the hardware does weighting and it doesn't actually count every single > > pixel, apparently. > > > > > > > > > > > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> > > > > > --- > > > > > src/ipa/rkisp1/algorithms/agc.cpp | 2 +- > > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > > > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp > > > > > index 47a6f7b2..278903a5 100644 > > > > > --- a/src/ipa/rkisp1/algorithms/agc.cpp > > > > > +++ b/src/ipa/rkisp1/algorithms/agc.cpp > > > > > @@ -352,7 +352,7 @@ double Agc::estimateLuminance(Span<const uint8_t> expMeans, double gain) > > > > > */ > > > > > double Agc::measureBrightness(Span<const uint32_t> hist) const > > > > > { > > > > > - Histogram histogram{ hist }; > > > > > + Histogram histogram{ hist, 4 }; > > > > > > > > On it's own this doesn't look very clear. So I think we need to add a > > > > comment here above to say (in the code) why we're doing the shift. > > > > > > > > Also, and this is only an idea really - the passing of a parameter here > > > > feels odd. I wonder if instead we should implement operator>>= and > > > > operator<<= on the Histogram class which will iterate over each entry and > > > > do the shift accordingly. Otherwise, it's hard to convey 'which way' the > > > > shift of 4 is. And what if someone wants to shift the otherway to make a > > > > histogram fit to 20 bits? > > > > > > > > Something like > > > > > > > > Histogram histogram{ hist }; > > > > > > > > /* > > > > * The RKISP1 hardware histogram is 20 bits, but represent 16 > > > > * bits integer and 4 bits are fractional. Shift to discard the > > > > * fractional components. > > > > */ > > > > histogram >>= 4; > > > > > > What if we need other types of conversions in the future ? This doesn't > > > seem very generic. Passing a lambda function that transforms the value > > > would seem better, and would also be more readable. You don't want that > > > function to be called for every value without allowing for compiler > > > optimization though, so the constructor would likely need to be inlined. > > > > idk I can only imagine needing arithmetic operations... which we could > > add as we need them...? > > > > > > This might be seen as less efficient, but I think the intent and > > > > operations are then clearer. > > > > Yeah I think you're right. > > > > > > Paul > > > > > Efficiency-wise, that's not great indeed :-( > > Sure, but what's our cost here. Is this a hot path? Or are we talking > about iterating a table of 5-10 values twice once to add them and once to > shift them right? How big is the histogram expected to be? > > If we're under a cacheline or two in size ~64 bytes? then is it better > to prefer readability against over-optimisations? > > But I'm probably fine with a lambda here too. > > How would that look ? I can't quite figure out how to do this inline > without a copy (which would probably make any performance optimisation redundant > anyway?) > > > Is this what you had in mind? > > double measureBrightness(std::span<const uint32_t> hist) const { > std::vector<uint32_t> shiftedHist; > shiftedHist.reserve(hist.size()); > > /* > * The RKISP1 hardware histogram is 20 bits, but represents 16 > * bits integer and 4 bits are fractional. Shift to discard the > * fractional components. > */ > std::transform(hist.begin(), hist.end(), > std::back_inserter(shiftedHist), > [](uint32_t value) { return value >> 4; }); > > Histogram histogram{ shiftedHist }; > > For full context, here's my sketch I tried to do it in: > https://godbolt.org/z/e9sTbYncb > > I'm equally fine with something like the above though. And it puts the > expressiveness of what the adjustment is and why it's required in the > platform specific components. > [...] I know libcamera does not utilize C++20, but I just wanted to mention that the ranges library would provide a nice solution to this, I think. E.g. you could do something like // e.g. std::span<const std::uint32_t> values histogram { values | std::views::transform([](auto x) { return x >> 4; }) } ( https://gcc.godbolt.org/z/f88TGqGW6 ) And the values would be shifted when creating inserting into the vector, without the need to allocate a separate vector, like in the code above. Regards, Barnabás Pőcze
Quoting Barnabás Pőcze (2024-03-27 13:44:23) > Hi > > > How would that look ? I can't quite figure out how to do this inline > > without a copy (which would probably make any performance optimisation redundant > > anyway?) > > > > > > > > Is this what you had in mind? > > > > > double measureBrightness(std::span<const uint32_t> hist) const { > > std::vector<uint32_t> shiftedHist; > > shiftedHist.reserve(hist.size()); > > > > > /* > > * The RKISP1 hardware histogram is 20 bits, but represents 16 > > * bits integer and 4 bits are fractional. Shift to discard the > > * fractional components. > > */ > > std::transform(hist.begin(), hist.end(), > > std::back_inserter(shiftedHist), > > [](uint32_t value) { return value >> 4; }); > > > > > Histogram histogram{ shiftedHist }; > > > > > For full context, here's my sketch I tried to do it in: > > https://godbolt.org/z/e9sTbYncb > > > > > I'm equally fine with something like the above though. And it puts the > > expressiveness of what the adjustment is and why it's required in the > > platform specific components. > > [...] > > I know libcamera does not utilize C++20, but I just wanted to mention that the > ranges library would provide a nice solution to this, I think. > E.g. you could do something like > > // e.g. std::span<const std::uint32_t> values > histogram { values | std::views::transform([](auto x) { return x >> 4; }) } > > ( https://gcc.godbolt.org/z/f88TGqGW6 ) > > And the values would be shifted when creating inserting into the vector, > without the need to allocate a separate vector, like in the code above. Aha, nice - that's what I thought I could try to do with a lambda, but failed without the copy of course - hence the above. But indeed, alas, we may be some time away from C++20. -- Kieran > > > Regards, > Barnabás Pőcze
Hi 2024. március 27., szerda 15:42 keltezéssel, Kieran Bingham <kieran.bingham@ideasonboard.com> írta: > Quoting Barnabás Pőcze (2024-03-27 13:44:23) > > Hi > > > > > How would that look ? I can't quite figure out how to do this inline > > > without a copy (which would probably make any performance optimisation redundant > > > anyway?) > > > > > > > > > > > > > Is this what you had in mind? > > > > > > > > double measureBrightness(std::span<const uint32_t> hist) const { > > > std::vector<uint32_t> shiftedHist; > > > shiftedHist.reserve(hist.size()); > > > > > > > > /* > > > * The RKISP1 hardware histogram is 20 bits, but represents 16 > > > * bits integer and 4 bits are fractional. Shift to discard the > > > * fractional components. > > > */ > > > std::transform(hist.begin(), hist.end(), > > > std::back_inserter(shiftedHist), > > > [](uint32_t value) { return value >> 4; }); > > > > > > > > Histogram histogram{ shiftedHist }; > > > > > > > > For full context, here's my sketch I tried to do it in: > > > https://godbolt.org/z/e9sTbYncb > > > > > > > > I'm equally fine with something like the above though. And it puts the > > > expressiveness of what the adjustment is and why it's required in the > > > platform specific components. > > > [...] > > > > I know libcamera does not utilize C++20, but I just wanted to mention that the > > ranges library would provide a nice solution to this, I think. > > E.g. you could do something like > > > > // e.g. std::span<const std::uint32_t> values > > histogram { values | std::views::transform([](auto x) { return x >> 4; }) } > > > > ( https://gcc.godbolt.org/z/f88TGqGW6 ) > > > > And the values would be shifted when creating inserting into the vector, > > without the need to allocate a separate vector, like in the code above. > > Aha, nice - that's what I thought I could try to do with a lambda, > but failed without the copy of course - hence the above. > [...] The constructor could take a lambda like this, which should be equally performant, even if a bit more restricted than what is possible with ranges: template<typename Func, std::enable_if_t<std::is_same_v<std::invoke_result_t<Func, std::uint32_t>, std::uint32_t>> * = nullptr> histogram(std::span<const std::uint32_t> values, Func transform) { for (auto x : values) this->values.push_back(transform(x)); ... } ( https://gcc.godbolt.org/z/P35GrYKzn ) > > But indeed, alas, we may be some time away from C++20. Are there any plans by the way? What are the criteria? Regards, Barnabás Pőcze
diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp index 47a6f7b2..278903a5 100644 --- a/src/ipa/rkisp1/algorithms/agc.cpp +++ b/src/ipa/rkisp1/algorithms/agc.cpp @@ -352,7 +352,7 @@ double Agc::estimateLuminance(Span<const uint8_t> expMeans, double gain) */ double Agc::measureBrightness(Span<const uint32_t> hist) const { - Histogram histogram{ hist }; + Histogram histogram{ hist, 4 }; /* Estimate the quantile mean of the top 2% of the histogram. */ return histogram.interQuantileMean(0.98, 1.0); }
The histogram reported by the rkisp1 hardware is 20 bits, where the upper 16 bits are meaningful integer data and the lower 4 bits are fractional and meant to be discarded. Remove these 4 bits when constructing the histogram. Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> --- src/ipa/rkisp1/algorithms/agc.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)