[PATCH v6 15/18] libcamera: software_isp: Use floating point for color parameters
Milan Zamazal
mzamazal at redhat.com
Mon Sep 16 17:46:01 CEST 2024
Kieran Bingham <kieran.bingham at ideasonboard.com> writes:
> Quoting Milan Zamazal (2024-09-06 13:09:24)
>> It's more natural to represent color gains and black level as floating
>> point numbers rather than using a particular pixel-related
>
>> representation.
>>
>> double is used rather than float because it's a more common floating
>> point type in libcamera algorithms. Otherwise there is no obvious
>> reason to select one over the other here.
>>
>> The constructed color tables still use integer representation for
>> efficiency.
>
> Ohh look at this, one comment I was going to try to make on earlier code
> already handled \o/
>
> I can't spot any gotchas here ... (And I've tested this series, so I
> believe it's working well)
Hi Kieran,
thank you for testing and reviews. I'll look at your comments more
closely later this week.
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>
>>
>> Signed-off-by: Milan Zamazal <mzamazal at redhat.com>
>> ---
>> src/ipa/simple/algorithms/awb.cpp | 11 +++++------
>> src/ipa/simple/algorithms/blc.cpp | 8 ++++----
>> src/ipa/simple/algorithms/lut.cpp | 15 +++++++++------
>> src/ipa/simple/ipa_context.h | 11 +++++------
>> 4 files changed, 23 insertions(+), 22 deletions(-)
>>
>> diff --git a/src/ipa/simple/algorithms/awb.cpp b/src/ipa/simple/algorithms/awb.cpp
>> index f89f1d3d..61d9fc73 100644
>> --- a/src/ipa/simple/algorithms/awb.cpp
>> +++ b/src/ipa/simple/algorithms/awb.cpp
>> @@ -24,7 +24,7 @@ int Awb::init(IPAContext &context,
>> [[maybe_unused]] const YamlObject &tuningData)
>> {
>> auto &gains = context.activeState.gains;
>> - gains.red = gains.green = gains.blue = 256;
>> + gains.red = gains.green = gains.blue = 1.0;
>>
>> return 0;
>> }
>> @@ -36,7 +36,7 @@ void Awb::process(IPAContext &context,
>> [[maybe_unused]] ControlList &metadata)
>> {
>> const SwIspStats::Histogram &histogram = stats->yHistogram;
>> - const uint8_t blackLevel = context.activeState.blc.level;
>> + const double blackLevel = context.activeState.blc.level;
>>
>> /*
>> * Black level must be subtracted to get the correct AWB ratios, they
>> @@ -53,12 +53,11 @@ void Awb::process(IPAContext &context,
>> /*
>> * Calculate red and blue gains for AWB.
>> * Clamp max gain at 4.0, this also avoids 0 division.
>> - * Gain: 128 = 0.5, 256 = 1.0, 512 = 2.0, etc.
>> */
>> auto &gains = context.activeState.gains;
>> - gains.red = sumR <= sumG / 4 ? 1024 : 256 * sumG / sumR;
>> - gains.blue = sumB <= sumG / 4 ? 1024 : 256 * sumG / sumB;
>> - /* Green gain is fixed to 256 */
>> + gains.red = sumR <= sumG / 4 ? 4.0 : static_cast<double>(sumG) / sumR;
>> + gains.blue = sumB <= sumG / 4 ? 4.0 : static_cast<double>(sumG) / sumB;
>> + /* Green gain is fixed to 1.0 */
>>
>> LOG(IPASoftAwb, Debug) << "gain R/B " << gains.red << "/" << gains.blue;
>> }
>> diff --git a/src/ipa/simple/algorithms/blc.cpp b/src/ipa/simple/algorithms/blc.cpp
>> index 08f4345e..1ceae85d 100644
>> --- a/src/ipa/simple/algorithms/blc.cpp
>> +++ b/src/ipa/simple/algorithms/blc.cpp
>> @@ -24,7 +24,7 @@ BlackLevel::BlackLevel()
>> int BlackLevel::init(IPAContext &context,
>> [[maybe_unused]] const YamlObject &tuningData)
>> {
>> - context.activeState.blc.level = 255;
>> + context.activeState.blc.level = 1.0;
>> return 0;
>> }
>>
>> @@ -44,16 +44,16 @@ void BlackLevel::process(IPAContext &context,
>> const unsigned int total =
>> std::accumulate(begin(histogram), end(histogram), 0);
>> const unsigned int pixelThreshold = ignoredPercentage * total;
>> - const unsigned int histogramRatio = 256 / SwIspStats::kYHistogramSize;
>> const unsigned int currentBlackIdx =
>> - context.activeState.blc.level / histogramRatio;
>> + context.activeState.blc.level * SwIspStats::kYHistogramSize;
>>
>> for (unsigned int i = 0, seen = 0;
>> i < currentBlackIdx && i < SwIspStats::kYHistogramSize;
>> i++) {
>> seen += histogram[i];
>> if (seen >= pixelThreshold) {
>> - context.activeState.blc.level = i * histogramRatio;
>> + context.activeState.blc.level =
>> + static_cast<double>(i) / SwIspStats::kYHistogramSize;
>> LOG(IPASoftBL, Debug)
>> << "Auto-set black level: "
>> << i << "/" << SwIspStats::kYHistogramSize
>> diff --git a/src/ipa/simple/algorithms/lut.cpp b/src/ipa/simple/algorithms/lut.cpp
>> index 6781f34e..d3b33710 100644
>> --- a/src/ipa/simple/algorithms/lut.cpp
>> +++ b/src/ipa/simple/algorithms/lut.cpp
>> @@ -32,7 +32,7 @@ void Lut::updateGammaTable(IPAContext &context)
>> {
>> auto &gammaTable = context.activeState.gamma.gammaTable;
>> auto blackLevel = context.activeState.blc.level;
>> - const unsigned int blackIndex = blackLevel * gammaTable.size() / 256;
>> + const unsigned int blackIndex = blackLevel * gammaTable.size();
>> std::fill(gammaTable.begin(), gammaTable.begin() + blackIndex, 0);
>> const float divisor = gammaTable.size() - blackIndex - 1.0;
>> for (unsigned int i = blackIndex; i < gammaTable.size(); i++)
>> @@ -58,15 +58,18 @@ void Lut::prepare(IPAContext &context,
>> auto &gammaTable = context.activeState.gamma.gammaTable;
>> const unsigned int gammaTableSize = gammaTable.size();
>> for (unsigned int i = 0; i < DebayerParams::kRGBLookupSize; i++) {
>> - const unsigned int div = static_cast<double>(DebayerParams::kRGBLookupSize) *
>> - 256 / gammaTableSize;
>> + const double div = static_cast<double>(DebayerParams::kRGBLookupSize) /
>> + gammaTableSize;
>> /* Apply gamma after gain! */
>> unsigned int idx;
>> - idx = std::min({ i * gains.red / div, gammaTableSize - 1 });
>> + idx = std::min({ static_cast<unsigned int>(i * gains.red / div),
>> + gammaTableSize - 1 });
>> params->red[i] = gammaTable[idx];
>> - idx = std::min({ i * gains.green / div, gammaTableSize - 1 });
>> + idx = std::min({ static_cast<unsigned int>(i * gains.green / div),
>> + gammaTableSize - 1 });
>> params->green[i] = gammaTable[idx];
>> - idx = std::min({ i * gains.blue / div, gammaTableSize - 1 });
>> + idx = std::min({ static_cast<unsigned int>(i * gains.blue / div),
>> + gammaTableSize - 1 });
>> params->blue[i] = gammaTable[idx];
>> }
>> }
>> diff --git a/src/ipa/simple/ipa_context.h b/src/ipa/simple/ipa_context.h
>> index 737bbbc3..d09de9a9 100644
>> --- a/src/ipa/simple/ipa_context.h
>> +++ b/src/ipa/simple/ipa_context.h
>> @@ -8,7 +8,6 @@
>> #pragma once
>>
>> #include <array>
>> -#include <stdint.h>
>>
>> #include <libipa/fc_queue.h>
>>
>> @@ -22,19 +21,19 @@ struct IPASessionConfiguration {
>>
>> struct IPAActiveState {
>> struct {
>> - uint8_t level;
>> + double level;
>> } blc;
>>
>> struct {
>> - unsigned int red;
>> - unsigned int green;
>> - unsigned int blue;
>> + double red;
>> + double green;
>> + double blue;
>> } gains;
>>
>> static constexpr unsigned int kGammaLookupSize = 1024;
>> struct {
>> std::array<double, kGammaLookupSize> gammaTable;
>> - uint8_t blackLevel;
>> + double blackLevel;
>> } gamma;
>> };
>>
>> --
>> 2.44.1
>>
More information about the libcamera-devel
mailing list