[PATCH v3 19/23] libcamera: software_isp: Move color handling to an algorithm module
Milan Zamazal
mzamazal at redhat.com
Tue Aug 13 16:10:33 CEST 2024
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:
>> After black level handling has been moved to an algorithm module, white
>> balance and the construction of color tables can be moved to an
>> algorithm module too. This time, the moved code is split between stats
>> processing and parameter construction methods.
>>
>> 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.
>>
>> Signed-off-by: Milan Zamazal <mzamazal at redhat.com>
>> ---
>> src/ipa/simple/algorithms/colors.cpp | 121 ++++++++++++++++++++++++++
>> src/ipa/simple/algorithms/colors.h | 46 ++++++++++
>> src/ipa/simple/algorithms/meson.build | 1 +
>> src/ipa/simple/data/uncalibrated.yaml | 1 +
>> src/ipa/simple/ipa_context.cpp | 30 +++++++
>> src/ipa/simple/ipa_context.h | 12 +++
>> src/ipa/simple/soft_simple.cpp | 66 +-------------
>> 7 files changed, 215 insertions(+), 62 deletions(-)
>> create mode 100644 src/ipa/simple/algorithms/colors.cpp
>> create mode 100644 src/ipa/simple/algorithms/colors.h
>>
>> diff --git a/src/ipa/simple/algorithms/colors.cpp b/src/ipa/simple/algorithms/colors.cpp
>> new file mode 100644
>> index 00000000..60fdca15
>> --- /dev/null
>> +++ b/src/ipa/simple/algorithms/colors.cpp
>> @@ -0,0 +1,121 @@
>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
>> +/*
>> + * Copyright (C) 2024, Red Hat Inc.
>> + *
>> + * Color gains (AWB + gamma)
>> + */
>> +
>> +#include "colors.h"
>> +
>> +#include <algorithm>
>> +#include <cmath>
>> +#include <numeric>
>> +#include <stdint.h>
>> +
>> +#include <libcamera/base/log.h>
>> +
>> +#include "simple/ipa_context.h"
>> +
>> +namespace libcamera {
>> +
>> +LOG_DEFINE_CATEGORY(IPASoftColors)
>> +
>> +namespace ipa::soft::algorithms {
>> +
>> +static constexpr unsigned int kGammaLookupSize = 1024;
>> +
>> +Colors::Colors()
>> +{
>> +}
>> +
>> +int Colors::init(IPAContext &context,
>> + [[maybe_unused]] const YamlObject &tuningData)
>> +{
>> + /* Gamma value is fixed */
>> + context.configuration.gamma = 0.5;
>> + updateGammaTable(context);
>> +
>> + auto &gains = context.activeState.gains;
>> + gains.red = gains.green = gains.blue = 256;
>> +
>> + return 0;
>> +}
>
>
> I think my preference would be to split this into "Gamma" and "Awb" algorithms...it adds boilerplate of course, but having the separation
> makes them much easier to understand in my opinion.
Makes sense, will do.
>> +
>> +void Colors::updateGammaTable(IPAContext &context)
>> +{
>> + auto &gammaTable = context.activeState.gamma.gammaTable;
>> + auto &blackLevel = context.activeState.gamma.blackLevel;
>> + blackLevel = context.activeState.black.level;
> double assignment
Not really because the first variable is a reference. But it is
confusing, I'll change it.
>> + const unsigned int blackIndex =
>> + blackLevel * IPAActiveState::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 * powf((i - blackIndex) / divisor,
>> + context.configuration.gamma);
>> +}
>> +
>> +void Colors::prepare(IPAContext &context,
>> + [[maybe_unused]] const uint32_t frame,
>> + [[maybe_unused]] IPAFrameContext &frameContext,
>> + DebayerParams *params)
>> +{
>> + /* Update the gamma table if needed */
>> + if (context.activeState.gamma.blackLevel != context.activeState.black.level)
> Is it worth adding some tolerance with abs_diff()? In my experience
> values are almost never exactly the same frame-to-frame.
The current black level computation updates the value only if it is
smaller than the previous one so it should stabilize sooner or later.
Maybe worth to add to the comment here.
>> + updateGammaTable(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;
>> + /* Apply gamma after gain! */
>> + unsigned int idx;
>> + idx = std::min({ i * gains.red / div, kGammaLookupSize - 1 });
>> + params->red[i] = gammaTable[idx];
>> + idx = std::min({ i * gains.green / div, kGammaLookupSize - 1 });
>> + params->green[i] = gammaTable[idx];
>> + idx = std::min({ i * gains.blue / div, kGammaLookupSize - 1 });
>> + params->blue[i] = gammaTable[idx];
>> + }
>> +}
>> +
>> +void Colors::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.black.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 */
>> +
>> + LOG(IPASoftColors, Debug) << "gain R/B " << gains.red << "/" << gains.blue;
>> +}
>> +
>> +REGISTER_IPA_ALGORITHM(Colors, "Colors")
>> +
>> +} /* namespace ipa::soft::algorithms */
>> +
>> +} /* namespace libcamera */
>> diff --git a/src/ipa/simple/algorithms/colors.h b/src/ipa/simple/algorithms/colors.h
>> new file mode 100644
>> index 00000000..2c2f7752
>> --- /dev/null
>> +++ b/src/ipa/simple/algorithms/colors.h
>> @@ -0,0 +1,46 @@
>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
>> +/*
>> + * Copyright (C) 2024, Red Hat Inc.
>> + *
>> + * Color gains (AWB + gamma)
>> + */
>> +
>> +#pragma once
>> +
>> +#include <libcamera/controls.h>
>> +
>> +#include "libcamera/internal/software_isp/debayer_params.h"
>> +#include "libcamera/internal/software_isp/swisp_stats.h"
>> +
>> +#include "algorithm.h"
>> +#include "ipa_context.h"
>> +
>> +namespace libcamera {
>> +
>> +namespace ipa::soft::algorithms {
>> +
>> +class Colors : public Algorithm
>> +{
>> +public:
>> + Colors();
>> + ~Colors() = default;
>> +
>> + int init(IPAContext &context, const YamlObject &tuningData)
>> + override;
>> + void prepare(IPAContext &context,
>> + const uint32_t frame,
>> + IPAFrameContext &frameContext,
>> + DebayerParams *params) override;
>> + void process(IPAContext &context,
>> + const uint32_t frame,
>> + IPAFrameContext &frameContext,
>> + const SwIspStats *stats,
>> + ControlList &metadata) 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..324c7b4e 100644
>> --- a/src/ipa/simple/algorithms/meson.build
>> +++ b/src/ipa/simple/algorithms/meson.build
>> @@ -2,4 +2,5 @@
>> soft_simple_ipa_algorithms = files([
>> 'blc.cpp',
>> + 'colors.cpp',
>> ])
>> diff --git a/src/ipa/simple/data/uncalibrated.yaml b/src/ipa/simple/data/uncalibrated.yaml
>> index e0d03d96..7e5149e5 100644
>> --- a/src/ipa/simple/data/uncalibrated.yaml
>> +++ b/src/ipa/simple/data/uncalibrated.yaml
>> @@ -4,4 +4,5 @@
>> version: 1
>> algorithms:
>> - BlackLevel:
>> + - Colors:
>> ...
>> 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 ed07dbb8..979ddb8f 100644
>> --- a/src/ipa/simple/ipa_context.h
>> +++ b/src/ipa/simple/ipa_context.h
>> @@ -8,6 +8,7 @@
>> #pragma once
>> +#include <array>
>> #include <stdint.h>
>> #include <libipa/fc_queue.h>
>> @@ -17,12 +18,23 @@ namespace libcamera {
>> namespace ipa::soft {
>> struct IPASessionConfiguration {
>> + float gamma;
>> };
>> struct IPAActiveState {
>> struct {
>> uint8_t level;
>> } black;
>> + 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;
>> + } gamma;
>> };
>> struct IPAFrameContext : public FrameContext {
>> diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp
>> index 2fb3a473..2658e359 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>
>> @@ -92,9 +90,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_;
>> @@ -282,6 +277,7 @@ void IPASoftSimple::prepare(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(
>> @@ -300,62 +296,6 @@ void IPASoftSimple::processStats(
>> for (auto const &algo : algorithms())
>> algo->process(context_, frame, frameContext, stats_, metadata);
>> - SwIspStats::Histogram histogram = stats_->yHistogram;
>> - const uint8_t blackLevel = context_.activeState.black.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(
>> * 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.black.level;
>> const unsigned int blackLevelHistIdx =
>> blackLevel / (256 / SwIspStats::kYHistogramSize);
>> const unsigned int histogramSize =
>> @@ -421,7 +362,8 @@ void IPASoftSimple::processStats(
>> LOG(IPASoft, Debug) << "exposureMSV " << exposureMSV
>> << " exp " << exposure_ << " again " << again_
>> - << " gain R/B " << gainR << "/" << gainB
>> + << " gain R/B " << context_.activeState.gains.red
>> + << "/" << context_.activeState.gains.blue
>> << " black level " << static_cast<unsigned int>(blackLevel);
>> }
>>
More information about the libcamera-devel
mailing list