[PATCH v3 20/23] libcamera: software_isp: Use floating point for color parameters
Milan Zamazal
mzamazal at redhat.com
Thu Aug 15 10:41:41 CEST 2024
Milan Zamazal <mzamazal at redhat.com> writes:
> Hi Dan,
>
> thank you for review.
>
> Dan Scally <dan.scally at ideasonboard.com> writes:
>
>> Hi Milan
>>
>> On 17/07/2024 09:54, Milan Zamazal wrote:
>>> 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.
>>>
>>> Signed-off-by: Milan Zamazal <mzamazal at redhat.com>
>>> ---
>>> src/ipa/simple/algorithms/blc.cpp | 8 ++++----
>>> src/ipa/simple/algorithms/colors.cpp | 26 ++++++++++++++------------
>>> src/ipa/simple/ipa_context.h | 11 +++++------
>>> 3 files changed, 23 insertions(+), 22 deletions(-)
>>>
>>> diff --git a/src/ipa/simple/algorithms/blc.cpp b/src/ipa/simple/algorithms/blc.cpp
>>> index 3b73d830..ab7e7dd9 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.black.level = 255;
>>> + context.activeState.black.level = 1.0;
>>
>>
>> I'm not sure I agree with that one actually...I expect it to be represented as an absolute pixel intensity value. Is there any functional
>> benefit to making it a floating point?
>
> There is no functional benefit. It's just to make it bit-width
> independent but maybe it's better to avoid useless conversions. Is this
> what you mean or do you have other reasons to prefer the pixel value?
And there is another option, to use the range that
CameraSensorHelper::blackLevel() uses, see "Get black level from the
camera helper" patch.
I don't have a strong preference for any of the options and I'm open to
arguments why some of them is better than the others.
>> Side note: I think I'd also expect the starting point to be 0 rather
>> than 255 (or 1.0), though I don't suppose it matters too much.
>
> It's the maximum so that the algorithm below automatically updates it;
> note that it is also assumed and has been observed that the right (low)
> black level may not be reached on the first image run. I'll add this
> explanation to the commit message.
>
>>> 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.black.level / histogramRatio;
>>> + context.activeState.black.level * SwIspStats::kYHistogramSize;
>>> for (unsigned int i = 0, seen = 0;
>>> i < currentBlackIdx && i < SwIspStats::kYHistogramSize;
>>> i++) {
>>> seen += histogram[i];
>>> if (seen >= pixelThreshold) {
>>> - context.activeState.black.level = i * histogramRatio;
>>> + context.activeState.black.level =
>>> + static_cast<double>(i) / SwIspStats::kYHistogramSize;
>>> LOG(IPASoftBL, Debug)
>>> << "Auto-set black level: "
>>> << i << "/" << SwIspStats::kYHistogramSize
>>> diff --git a/src/ipa/simple/algorithms/colors.cpp b/src/ipa/simple/algorithms/colors.cpp
>>> index 60fdca15..77492d9e 100644
>>> --- a/src/ipa/simple/algorithms/colors.cpp
>>> +++ b/src/ipa/simple/algorithms/colors.cpp
>>> @@ -36,7 +36,7 @@ int Colors::init(IPAContext &context,
>>> updateGammaTable(context);
>>> auto &gains = context.activeState.gains;
>>> - gains.red = gains.green = gains.blue = 256;
>>> + gains.red = gains.green = gains.blue = 1.0;
>> Gains being floats I agree with.
>>> return 0;
>>> }
>>> @@ -47,7 +47,7 @@ void Colors::updateGammaTable(IPAContext &context)
>>> auto &blackLevel = context.activeState.gamma.blackLevel;
>>> blackLevel = context.activeState.black.level;
>>> const unsigned int blackIndex =
>>> - blackLevel * IPAActiveState::kGammaLookupSize / 256;
>>> + context.activeState.black.level * IPAActiveState::kGammaLookupSize;
>>> std::fill(gammaTable.begin(), gammaTable.begin() + blackIndex, 0);
>>> const float divisor = kGammaLookupSize - blackIndex - 1.0;
>>> for (unsigned int i = blackIndex; i < kGammaLookupSize; i++)
>>> @@ -67,15 +67,18 @@ void Colors::prepare(IPAContext &context,
>>> auto &gains = context.activeState.gains;
>>> auto &gammaTable = context.activeState.gamma.gammaTable;
>>> for (unsigned int i = 0; i < DebayerParams::kRGBLookupSize; i++) {
>>> - constexpr unsigned int div =
>>> - static_cast<double>(DebayerParams::kRGBLookupSize) * 256 / kGammaLookupSize;
>>> + constexpr double div =
>>> + static_cast<double>(DebayerParams::kRGBLookupSize) / kGammaLookupSize;
>>> /* Apply gamma after gain! */
>>> unsigned int idx;
>>> - idx = std::min({ i * gains.red / div, kGammaLookupSize - 1 });
>>> + idx = std::min({ static_cast<unsigned int>(i * gains.red / div),
>>> + kGammaLookupSize - 1 });
>>> params->red[i] = gammaTable[idx];
>>> - idx = std::min({ i * gains.green / div, kGammaLookupSize - 1 });
>>> + idx = std::min({ static_cast<unsigned int>(i * gains.green / div),
>>> + kGammaLookupSize - 1 });
>>> params->green[i] = gammaTable[idx];
>>> - idx = std::min({ i * gains.blue / div, kGammaLookupSize - 1 });
>>> + idx = std::min({ static_cast<unsigned int>(i * gains.blue / div),
>>> + kGammaLookupSize - 1 });
>>> params->blue[i] = gammaTable[idx];
>>> }
>>> }
>>> @@ -87,7 +90,7 @@ void Colors::process(IPAContext &context,
>>> [[maybe_unused]] ControlList &metadata)
>>> {
>>> const SwIspStats::Histogram &histogram = stats->yHistogram;
>>> - const uint8_t blackLevel = context.activeState.black.level;
>>> + const double blackLevel = context.activeState.black.level;
>>> /*
>>> * Black level must be subtracted to get the correct AWB ratios, they
>>> @@ -104,12 +107,11 @@ void Colors::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(IPASoftColors, Debug) << "gain R/B " << gains.red << "/" << gains.blue;
>>> }
>>> diff --git a/src/ipa/simple/ipa_context.h b/src/ipa/simple/ipa_context.h
>>> index 979ddb8f..552d6cde 100644
>>> --- a/src/ipa/simple/ipa_context.h
>>> +++ b/src/ipa/simple/ipa_context.h
>>> @@ -9,7 +9,6 @@
>>> #pragma once
>>> #include <array>
>>> -#include <stdint.h>
>>> #include <libipa/fc_queue.h>
>>> @@ -23,17 +22,17 @@ struct IPASessionConfiguration {
>>> struct IPAActiveState {
>>> struct {
>>> - uint8_t level;
>>> + double level;
>>> } black;
>>> 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;
>>> };
>>>
More information about the libcamera-devel
mailing list