[PATCH v7 14/18] libcamera: software_isp: Move color handling to an algorithm module
Milan Zamazal
mzamazal at redhat.com
Fri Sep 27 15:21:09 CEST 2024
Hi Umang,
thank you for reviews.
Umang Jain <umang.jain at ideasonboard.com> writes:
> Hello Milan,
>
> Thank you for the patch.
>
> On 21/09/24 12:01 am, 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 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.
>>
>> Signed-off-by: Milan Zamazal <mzamazal at redhat.com>
>> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>> ---
>> src/ipa/simple/algorithms/awb.cpp | 70 ++++++++++++++++++++++++
>> src/ipa/simple/algorithms/awb.h | 32 +++++++++++
>> src/ipa/simple/algorithms/lut.cpp | 79 +++++++++++++++++++++++++++
>> src/ipa/simple/algorithms/lut.h | 34 ++++++++++++
>> 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..3d76cc2f
>> --- /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::configure(IPAContext &context,
>> + [[maybe_unused]] const IPAConfigInfo &configInfo)
>> +{
>> + 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 */
>> +
>> + 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..db1496cd
>> --- /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 configure(IPAContext &context, const IPAConfigInfo &configInfo) 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..44e13864
>> --- /dev/null
>> +++ b/src/ipa/simple/algorithms/lut.cpp
>> @@ -0,0 +1,79 @@
>> +/* 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::configure(IPAContext &context,
>> + [[maybe_unused]] const IPAConfigInfo &configInfo)
>> +{
>> + /* Gamma value is fixed */
>> + context.configuration.gamma = 0.5;
>> + 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;
>> +}
>
> Please introduce newline(s), for appropriate segmentation. It will help with readability of the code.
I often have trouble to know where to put empty lines but I'll add a
couple of them here and below, HTH.
>> +
>> +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];
>> + }
>> +}
>
> Ditto.
>
> Otherwise, looks good to me on cursory glance,
>
> Acked-by: Umang Jain <umang.jain at ideasonboard.com>
>> +
>> +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..b635987d
>> --- /dev/null
>> +++ b/src/ipa/simple/algorithms/lut.h
>> @@ -0,0 +1,34 @@
>> +/* 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 configure(IPAContext &context, const IPAConfigInfo &configInfo) 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;
>> + } 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);
>> }
>>
More information about the libcamera-devel
mailing list