[PATCH v6 14/18] libcamera: software_isp: Move color handling to an algorithm module
Kieran Bingham
kieran.bingham at ideasonboard.com
Mon Sep 9 22:53:14 CEST 2024
Quoting Milan Zamazal (2024-09-06 13:09:23)
> After black level handling has been moved to an algorithm module, white
> balance and the construction of color tables can be moved to algorithm
> modules too.
>
> This time, the moved code is split between stats processing and
> parameter construction methods. It is also split to two algorithm
> modules:
>
> - White balance computation.
>
> - Gamma table computation and color lookup tables construction. While
> this applies the color gains computed by the white balance algorithm,
> it is not directly related to white balance. And we may want to
> modify the color lookup tables in future according to other parameters
> than just gamma and white balance gains.
>
> Gamma table computation and color lookup tables construction could be
> split to separate algorithms too. But there is no big need for that now
> so they are kept together for simplicity.
>
> This is the only part of the software ISP algorithms that sets the
> parameters so emitting setIspParams can be moved to prepare() method.
>
> A more natural representation of the gains (and black level) would be
> floating point numbers. This is not done here in order to minimize
> changes in code movements. It will be addressed in a followup patch.
Aha, I was just commenting on that, but now I see this. I agree, lets
keep such a change separate - this is just a refactor patch.
>
> Signed-off-by: Milan Zamazal <mzamazal at redhat.com>
> ---
> src/ipa/simple/algorithms/awb.cpp | 70 ++++++++++++++++++++++++
> src/ipa/simple/algorithms/awb.h | 32 +++++++++++
> src/ipa/simple/algorithms/lut.cpp | 78 +++++++++++++++++++++++++++
> src/ipa/simple/algorithms/lut.h | 35 ++++++++++++
> src/ipa/simple/algorithms/meson.build | 2 +
> src/ipa/simple/data/uncalibrated.yaml | 2 +
> src/ipa/simple/ipa_context.cpp | 30 +++++++++++
> src/ipa/simple/ipa_context.h | 14 +++++
> src/ipa/simple/soft_simple.cpp | 64 +---------------------
> 9 files changed, 265 insertions(+), 62 deletions(-)
> create mode 100644 src/ipa/simple/algorithms/awb.cpp
> create mode 100644 src/ipa/simple/algorithms/awb.h
> create mode 100644 src/ipa/simple/algorithms/lut.cpp
> create mode 100644 src/ipa/simple/algorithms/lut.h
>
> diff --git a/src/ipa/simple/algorithms/awb.cpp b/src/ipa/simple/algorithms/awb.cpp
> new file mode 100644
> index 00000000..f89f1d3d
> --- /dev/null
> +++ b/src/ipa/simple/algorithms/awb.cpp
> @@ -0,0 +1,70 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2024, Red Hat Inc.
> + *
> + * Auto white balance
> + */
> +
> +#include "awb.h"
> +
> +#include <numeric>
> +#include <stdint.h>
> +
> +#include <libcamera/base/log.h>
> +
> +#include "simple/ipa_context.h"
> +
> +namespace libcamera {
> +
> +LOG_DEFINE_CATEGORY(IPASoftAwb)
> +
> +namespace ipa::soft::algorithms {
> +
> +int Awb::init(IPAContext &context,
> + [[maybe_unused]] const YamlObject &tuningData)
> +{
> + auto &gains = context.activeState.gains;
> + gains.red = gains.green = gains.blue = 256;
> +
> + return 0;
> +}
> +
> +void Awb::process(IPAContext &context,
> + [[maybe_unused]] const uint32_t frame,
> + [[maybe_unused]] IPAFrameContext &frameContext,
> + const SwIspStats *stats,
> + [[maybe_unused]] ControlList &metadata)
> +{
> + const SwIspStats::Histogram &histogram = stats->yHistogram;
> + const uint8_t blackLevel = context.activeState.blc.level;
> +
> + /*
> + * Black level must be subtracted to get the correct AWB ratios, they
> + * would be off if they were computed from the whole brightness range
> + * rather than from the sensor range.
> + */
> + const uint64_t nPixels = std::accumulate(
> + histogram.begin(), histogram.end(), 0);
> + const uint64_t offset = blackLevel * nPixels;
> + const uint64_t sumR = stats->sumR_ - offset / 4;
> + const uint64_t sumG = stats->sumG_ - offset / 2;
> + const uint64_t sumB = stats->sumB_ - offset / 4;
> +
> + /*
> + * 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 */
Why are we encoding fixed point maths here instead of floats?
Edit: Never mind ;-) It was like this before and will be tackled
separately.
> +
> + LOG(IPASoftAwb, Debug) << "gain R/B " << gains.red << "/" << gains.blue;
> +}
> +
> +REGISTER_IPA_ALGORITHM(Awb, "Awb")
> +
> +} /* namespace ipa::soft::algorithms */
> +
> +} /* namespace libcamera */
> diff --git a/src/ipa/simple/algorithms/awb.h b/src/ipa/simple/algorithms/awb.h
> new file mode 100644
> index 00000000..235249b6
> --- /dev/null
> +++ b/src/ipa/simple/algorithms/awb.h
> @@ -0,0 +1,32 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2024, Red Hat Inc.
> + *
> + * Auto white balance
> + */
> +
> +#pragma once
> +
> +#include "algorithm.h"
> +
> +namespace libcamera {
> +
> +namespace ipa::soft::algorithms {
> +
> +class Awb : public Algorithm
> +{
> +public:
> + Awb() = default;
> + ~Awb() = default;
> +
> + int init(IPAContext &context, const YamlObject &tuningData) override;
> + void process(IPAContext &context,
> + const uint32_t frame,
> + IPAFrameContext &frameContext,
> + const SwIspStats *stats,
> + ControlList &metadata) override;
> +};
> +
> +} /* namespace ipa::soft::algorithms */
> +
> +} /* namespace libcamera */
> diff --git a/src/ipa/simple/algorithms/lut.cpp b/src/ipa/simple/algorithms/lut.cpp
> new file mode 100644
> index 00000000..6781f34e
> --- /dev/null
> +++ b/src/ipa/simple/algorithms/lut.cpp
> @@ -0,0 +1,78 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2024, Red Hat Inc.
> + *
> + * Color lookup tables construction
> + */
> +
> +#include "lut.h"
> +
> +#include <algorithm>
> +#include <cmath>
> +#include <stdint.h>
> +
> +#include <libcamera/base/log.h>
> +
> +#include "simple/ipa_context.h"
> +
> +namespace libcamera {
> +
> +namespace ipa::soft::algorithms {
> +
> +int Lut::init(IPAContext &context, [[maybe_unused]] const YamlObject &tuningData)
> +{
> + /* Gamma value is fixed */
> + context.configuration.gamma = 0.5;
I think we specify the gamma control as '1/gamma' ... so I'm curious if
we should initialise as gamma = 1 / 2; (But I get that's a bit
superfluos right now, and not for this patch which is just refactoring
the structures anyway).
> + updateGammaTable(context);
> +
> + return 0;
> +}
> +
> +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;
> + 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++)
> + gammaTable[i] = UINT8_MAX * std::pow((i - blackIndex) / divisor,
> + context.configuration.gamma);
> + context.activeState.gamma.blackLevel = blackLevel;
> +}
> +
> +void Lut::prepare(IPAContext &context,
> + [[maybe_unused]] const uint32_t frame,
> + [[maybe_unused]] IPAFrameContext &frameContext,
> + [[maybe_unused]] DebayerParams *params)
> +{
> + /*
> + * Update the gamma table if needed. This means if black level changes
> + * and since the black level gets updated only if a lower value is
> + * observed, it's not permanently prone to minor fluctuations or
> + * rounding errors.
> + */
> + if (context.activeState.gamma.blackLevel != context.activeState.blc.level)
> + updateGammaTable(context);
> + auto &gains = context.activeState.gains;
> + 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;
> + /* Apply gamma after gain! */
> + unsigned int idx;
> + idx = std::min({ i * gains.red / div, gammaTableSize - 1 });
> + params->red[i] = gammaTable[idx];
> + idx = std::min({ i * gains.green / div, gammaTableSize - 1 });
> + params->green[i] = gammaTable[idx];
> + idx = std::min({ i * gains.blue / div, gammaTableSize - 1 });
> + params->blue[i] = gammaTable[idx];
> + }
> +}
> +
> +REGISTER_IPA_ALGORITHM(Lut, "Lut")
> +
> +} /* namespace ipa::soft::algorithms */
> +
> +} /* namespace libcamera */
> diff --git a/src/ipa/simple/algorithms/lut.h b/src/ipa/simple/algorithms/lut.h
> new file mode 100644
> index 00000000..2c034e9f
> --- /dev/null
> +++ b/src/ipa/simple/algorithms/lut.h
> @@ -0,0 +1,35 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2024, Red Hat Inc.
> + *
> + * Color lookup tables construction
> + */
> +
> +#pragma once
> +
> +#include "algorithm.h"
> +
> +namespace libcamera {
> +
> +namespace ipa::soft::algorithms {
> +
> +class Lut : public Algorithm
> +{
> +public:
> + Lut() = default;
> + ~Lut() = default;
> +
> + int init(IPAContext &context, const YamlObject &tuningData)
> + override;
> + void prepare(IPAContext &context,
> + const uint32_t frame,
> + IPAFrameContext &frameContext,
> + DebayerParams *params) override;
> +
> +private:
> + void updateGammaTable(IPAContext &context);
> +};
> +
> +} /* namespace ipa::soft::algorithms */
> +
> +} /* namespace libcamera */
> diff --git a/src/ipa/simple/algorithms/meson.build b/src/ipa/simple/algorithms/meson.build
> index 1f63c220..f575611e 100644
> --- a/src/ipa/simple/algorithms/meson.build
> +++ b/src/ipa/simple/algorithms/meson.build
> @@ -1,5 +1,7 @@
> # SPDX-License-Identifier: CC0-1.0
>
> soft_simple_ipa_algorithms = files([
> + 'awb.cpp',
> 'blc.cpp',
> + 'lut.cpp',
> ])
> diff --git a/src/ipa/simple/data/uncalibrated.yaml b/src/ipa/simple/data/uncalibrated.yaml
> index e0d03d96..893a0b92 100644
> --- a/src/ipa/simple/data/uncalibrated.yaml
> +++ b/src/ipa/simple/data/uncalibrated.yaml
> @@ -4,4 +4,6 @@
> version: 1
> algorithms:
> - BlackLevel:
> + - Awb:
> + - Lut:
> ...
> diff --git a/src/ipa/simple/ipa_context.cpp b/src/ipa/simple/ipa_context.cpp
> index 268cff32..5fa492cb 100644
> --- a/src/ipa/simple/ipa_context.cpp
> +++ b/src/ipa/simple/ipa_context.cpp
> @@ -50,6 +50,11 @@ namespace libcamera::ipa::soft {
> * \brief The current state of IPA algorithms
> */
>
> +/**
> + * \var IPASessionConfiguration::gamma
> + * \brief Gamma value to be used in the raw image processing
> + */
> +
> /**
> * \var IPAActiveState::black
> * \brief Context for the Black Level algorithm
> @@ -58,4 +63,29 @@ namespace libcamera::ipa::soft {
> * \brief Current determined black level
> */
>
> +/**
> + * \var IPAActiveState::gains
> + * \brief Context for gains in the Colors algorithm
> + *
> + * \var IPAActiveState::gains.red
> + * \brief Gain of red color
> + *
> + * \var IPAActiveState::gains.green
> + * \brief Gain of green color
> + *
> + * \var IPAActiveState::gains.blue
> + * \brief Gain of blue color
> + */
> +
> +/**
> + * \var IPAActiveState::gamma
> + * \brief Context for gamma in the Colors algorithm
> + *
> + * \var IPAActiveState::gamma.gammaTable
> + * \brief Computed gamma table
> + *
> + * \var IPAActiveState::gamma.blackLevel
> + * \brief Black level used for the gamma table computation
> + */
> +
> } /* namespace libcamera::ipa::soft */
> diff --git a/src/ipa/simple/ipa_context.h b/src/ipa/simple/ipa_context.h
> index ac2a59d7..737bbbc3 100644
> --- a/src/ipa/simple/ipa_context.h
> +++ b/src/ipa/simple/ipa_context.h
> @@ -7,6 +7,7 @@
>
> #pragma once
>
> +#include <array>
> #include <stdint.h>
>
> #include <libipa/fc_queue.h>
> @@ -16,12 +17,25 @@ namespace libcamera {
> namespace ipa::soft {
>
> struct IPASessionConfiguration {
> + float gamma;
> };
>
> struct IPAActiveState {
> struct {
> uint8_t level;
> } blc;
> +
> + struct {
> + unsigned int red;
> + unsigned int green;
> + unsigned int blue;
> + } gains;
> +
> + static constexpr unsigned int kGammaLookupSize = 1024;
> + struct {
> + std::array<double, kGammaLookupSize> gammaTable;
> + uint8_t blackLevel;
What is the distinction between blc.level and gamma.blackLevel?
I think as this is just restructuring to modules - I'd overlook this for
future cleanups if the redundancy could/should be removed anyway so:
Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> + } gamma;
> };
>
> struct IPAFrameContext : public FrameContext {
> diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp
> index 3332e2b1..f8d923c5 100644
> --- a/src/ipa/simple/soft_simple.cpp
> +++ b/src/ipa/simple/soft_simple.cpp
> @@ -5,8 +5,6 @@
> * Simple Software Image Processing Algorithm module
> */
>
> -#include <cmath>
> -#include <numeric>
> #include <stdint.h>
> #include <sys/mman.h>
>
> @@ -93,9 +91,6 @@ private:
> std::unique_ptr<CameraSensorHelper> camHelper_;
> ControlInfoMap sensorInfoMap_;
>
> - static constexpr unsigned int kGammaLookupSize = 1024;
> - std::array<uint8_t, kGammaLookupSize> gammaTable_;
> - int lastBlackLevel_ = -1;
> /* Local parameter storage */
> struct IPAContext context_;
>
> @@ -283,6 +278,7 @@ void IPASoftSimple::fillParamsBuffer(const uint32_t frame)
> IPAFrameContext &frameContext = context_.frameContexts.get(frame);
> for (auto const &algo : algorithms())
> algo->prepare(context_, frame, frameContext, params_);
> + setIspParams.emit();
> }
>
> void IPASoftSimple::processStats(const uint32_t frame,
> @@ -300,62 +296,6 @@ void IPASoftSimple::processStats(const uint32_t frame,
> for (auto const &algo : algorithms())
> algo->process(context_, frame, frameContext, stats_, metadata);
>
> - SwIspStats::Histogram histogram = stats_->yHistogram;
> - const uint8_t blackLevel = context_.activeState.blc.level;
> -
> - /*
> - * Black level must be subtracted to get the correct AWB ratios, they
> - * would be off if they were computed from the whole brightness range
> - * rather than from the sensor range.
> - */
> - const uint64_t nPixels = std::accumulate(
> - histogram.begin(), histogram.end(), 0);
> - const uint64_t offset = blackLevel * nPixels;
> - const uint64_t sumR = stats_->sumR_ - offset / 4;
> - const uint64_t sumG = stats_->sumG_ - offset / 2;
> - const uint64_t sumB = stats_->sumB_ - offset / 4;
> -
> - /*
> - * 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.
> - */
> - const unsigned int gainR = sumR <= sumG / 4 ? 1024 : 256 * sumG / sumR;
> - const unsigned int gainB = sumB <= sumG / 4 ? 1024 : 256 * sumG / sumB;
> - /* Green gain and gamma values are fixed */
> - constexpr unsigned int gainG = 256;
> -
> - /* Update the gamma table if needed */
> - if (blackLevel != lastBlackLevel_) {
> - constexpr float gamma = 0.5;
> - const unsigned int blackIndex = blackLevel * kGammaLookupSize / 256;
> - std::fill(gammaTable_.begin(), gammaTable_.begin() + blackIndex, 0);
> - const float divisor = kGammaLookupSize - blackIndex - 1.0;
> - for (unsigned int i = blackIndex; i < kGammaLookupSize; i++)
> - gammaTable_[i] = UINT8_MAX *
> - std::pow((i - blackIndex) / divisor, gamma);
> -
> - lastBlackLevel_ = blackLevel;
> - }
> -
> - for (unsigned int i = 0; i < DebayerParams::kRGBLookupSize; i++) {
> - constexpr unsigned int div =
> - DebayerParams::kRGBLookupSize * 256 / kGammaLookupSize;
> - unsigned int idx;
> -
> - /* Apply gamma after gain! */
> - idx = std::min({ i * gainR / div, (kGammaLookupSize - 1) });
> - params_->red[i] = gammaTable_[idx];
> -
> - idx = std::min({ i * gainG / div, (kGammaLookupSize - 1) });
> - params_->green[i] = gammaTable_[idx];
> -
> - idx = std::min({ i * gainB / div, (kGammaLookupSize - 1) });
> - params_->blue[i] = gammaTable_[idx];
> - }
> -
> - setIspParams.emit();
> -
> /* \todo Switch to the libipa/algorithm.h API someday. */
>
> /*
> @@ -372,6 +312,7 @@ void IPASoftSimple::processStats(const uint32_t frame,
> * Calculate Mean Sample Value (MSV) according to formula from:
> * https://www.araa.asn.au/acra/acra2007/papers/paper84final.pdf
> */
> + const uint8_t blackLevel = context_.activeState.blc.level;
> const unsigned int blackLevelHistIdx =
> blackLevel / (256 / SwIspStats::kYHistogramSize);
> const unsigned int histogramSize =
> @@ -421,7 +362,6 @@ void IPASoftSimple::processStats(const uint32_t frame,
>
> LOG(IPASoft, Debug) << "exposureMSV " << exposureMSV
> << " exp " << exposure_ << " again " << again_
> - << " gain R/B " << gainR << "/" << gainB
> << " black level " << static_cast<unsigned int>(blackLevel);
> }
>
> --
> 2.44.1
>
More information about the libcamera-devel
mailing list