[v6,14/18] libcamera: software_isp: Move color handling to an algorithm module

21187 diff mbox series

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@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

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@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@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
>

Hi Kieran,

thank you for review.

Kieran Bingham <kieran.bingham@ideasonboard.com> writes:

> 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@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).

Yes, changing from 0.5 to 1/2 in a refactoring patch would add noise.
Whether to stick with 0.5 or change it 1/2 later, I don't care.  Maybe
if we have a reason to use a non-fixed gamma one day, it'll get changed
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?

blc.level is the black level.  gamma.blackLevel tracks the last black
level used when the gamma lookup table was computed;
if blc.level != gamma.blackLevel then it means the gamma table must be
recomputed.  Not particularly pretty but the other options that came to
my mind were even uglier.

> 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@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
>>