[PATCH v3 20/23] libcamera: software_isp: Use floating point for color parameters
Dan Scally
dan.scally at ideasonboard.com
Tue Aug 13 13:10:55 CEST 2024
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? 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.
> 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