[PATCH v6 15/18] libcamera: software_isp: Use floating point for color parameters
Kieran Bingham
kieran.bingham at ideasonboard.com
Mon Sep 9 23:01:38 CEST 2024
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)
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