[v6,17/18] libcamera: software_isp: Move exposure+gain to an algorithm module

21190 diff mbox series

This is the last step to fully convert software ISP to Algorithm-based
processing.

The newly introduced frameContext.sensor properties are set, and the
updated code moved, before calling Algorithm::process() to have the
values up-to-date in stats processing.

Resolves software ISP TODO #10.

Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
---
 src/ipa/simple/algorithms/agc.cpp     | 139 +++++++++++++++++++++++
 src/ipa/simple/algorithms/agc.h       |  33 ++++++
 src/ipa/simple/algorithms/meson.build |   1 +
 src/ipa/simple/data/uncalibrated.yaml |   1 +
 src/ipa/simple/ipa_context.cpp        |  11 ++
 src/ipa/simple/ipa_context.h          |  18 +++
 src/ipa/simple/soft_simple.cpp        | 157 +++++---------------------
 src/libcamera/software_isp/TODO       |  10 --
 8 files changed, 233 insertions(+), 137 deletions(-)
 create mode 100644 src/ipa/simple/algorithms/agc.cpp
 create mode 100644 src/ipa/simple/algorithms/agc.h

Quoting Milan Zamazal (2024-09-06 13:09:26)
> This is the last step to fully convert software ISP to Algorithm-based
> processing.
> 
> The newly introduced frameContext.sensor properties are set, and the
> updated code moved, before calling Algorithm::process() to have the
> values up-to-date in stats processing.
> 
> Resolves software ISP TODO #10.

I'm not going to dwell on the details in here either. Modularising this
is a big benefit.

I haven't yet checked if this is comparable to the existing AGC
implementation in libipa, but now it's modular - it would be interesting
to 'try' using the existing libipa algorithm on soft ISP - or try 'this'
implementation on other libipa platforms.

Perhaps if they're equivalent they could be merged, or if they are
uniquely separate they could be named as such in libipa so they could be
selected.

But either way, I think this makes it all more manageable in the long
run.


> 
> Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
> ---
>  src/ipa/simple/algorithms/agc.cpp     | 139 +++++++++++++++++++++++
>  src/ipa/simple/algorithms/agc.h       |  33 ++++++
>  src/ipa/simple/algorithms/meson.build |   1 +
>  src/ipa/simple/data/uncalibrated.yaml |   1 +
>  src/ipa/simple/ipa_context.cpp        |  11 ++
>  src/ipa/simple/ipa_context.h          |  18 +++
>  src/ipa/simple/soft_simple.cpp        | 157 +++++---------------------
>  src/libcamera/software_isp/TODO       |  10 --
>  8 files changed, 233 insertions(+), 137 deletions(-)
>  create mode 100644 src/ipa/simple/algorithms/agc.cpp
>  create mode 100644 src/ipa/simple/algorithms/agc.h
> 
> diff --git a/src/ipa/simple/algorithms/agc.cpp b/src/ipa/simple/algorithms/agc.cpp
> new file mode 100644
> index 00000000..becfb99f
> --- /dev/null
> +++ b/src/ipa/simple/algorithms/agc.cpp
> @@ -0,0 +1,139 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2024, Red Hat Inc.
> + *
> + * Exposure and gain
> + */
> +
> +#include "agc.h"
> +
> +#include <stdint.h>
> +
> +#include <libcamera/base/log.h>
> +
> +namespace libcamera {
> +
> +LOG_DEFINE_CATEGORY(IPASoftExposure)
> +
> +namespace ipa::soft::algorithms {
> +
> +/*
> + * The number of bins to use for the optimal exposure calculations.
> + */
> +static constexpr unsigned int kExposureBinsCount = 5;
> +
> +/*
> + * The exposure is optimal when the mean sample value of the histogram is
> + * in the middle of the range.
> + */
> +static constexpr float kExposureOptimal = kExposureBinsCount / 2.0;
> +
> +/*
> + * The below value implements the hysteresis for the exposure adjustment.
> + * It is small enough to have the exposure close to the optimal, and is big
> + * enough to prevent the exposure from wobbling around the optimal value.
> + */
> +static constexpr float kExposureSatisfactory = 0.2;
> +
> +Agc::Agc()
> +{
> +}
> +
> +void Agc::updateExposure(IPAContext &context, double exposureMSV)
> +{
> +       /*
> +        * kExpDenominator of 10 gives ~10% increment/decrement;
> +        * kExpDenominator of 5 - about ~20%
> +        */
> +       static constexpr uint8_t kExpDenominator = 10;
> +       static constexpr uint8_t kExpNumeratorUp = kExpDenominator + 1;
> +       static constexpr uint8_t kExpNumeratorDown = kExpDenominator - 1;
> +
> +       double next;
> +       int32_t &exposure = context.activeState.agc.exposure;
> +       double &again = context.activeState.agc.again;
> +
> +       if (exposureMSV < kExposureOptimal - kExposureSatisfactory) {
> +               next = exposure * kExpNumeratorUp / kExpDenominator;
> +               if (next - exposure < 1)
> +                       exposure += 1;
> +               else
> +                       exposure = next;
> +               if (exposure >= context.configuration.agc.exposureMax) {
> +                       next = again * kExpNumeratorUp / kExpDenominator;
> +                       if (next - again < context.configuration.agc.againMinStep)
> +                               again += context.configuration.agc.againMinStep;
> +                       else
> +                               again = next;
> +               }
> +       }
> +
> +       if (exposureMSV > kExposureOptimal + kExposureSatisfactory) {
> +               if (exposure == context.configuration.agc.exposureMax &&
> +                   again > context.configuration.agc.againMin) {
> +                       next = again * kExpNumeratorDown / kExpDenominator;
> +                       if (again - next < context.configuration.agc.againMinStep)
> +                               again -= context.configuration.agc.againMinStep;
> +                       else
> +                               again = next;
> +               } else {
> +                       next = exposure * kExpNumeratorDown / kExpDenominator;
> +                       if (exposure - next < 1)
> +                               exposure -= 1;
> +                       else
> +                               exposure = next;
> +               }
> +       }
> +
> +       exposure = std::clamp(exposure, context.configuration.agc.exposureMin,
> +                             context.configuration.agc.exposureMax);
> +       again = std::clamp(again, context.configuration.agc.againMin,
> +                          context.configuration.agc.againMax);
> +
> +       LOG(IPASoftExposure, Debug)
> +               << "exposureMSV " << exposureMSV
> +               << " exp " << exposure << " again " << again;
> +}
> +
> +void Agc::process(IPAContext &context,
> +                 [[maybe_unused]] const uint32_t frame,
> +                 [[maybe_unused]] IPAFrameContext &frameContext,
> +                 const SwIspStats *stats,
> +                 [[maybe_unused]] ControlList &metadata)
> +{
> +       /*
> +        * Calculate Mean Sample Value (MSV) according to formula from:
> +        * https://www.araa.asn.au/acra/acra2007/papers/paper84final.pdf
> +        */
> +       const auto &histogram = stats->yHistogram;
> +       const unsigned int blackLevelHistIdx =
> +               context.activeState.blc.level * SwIspStats::kYHistogramSize;
> +       const unsigned int histogramSize =
> +               SwIspStats::kYHistogramSize - blackLevelHistIdx;
> +       const unsigned int yHistValsPerBin = histogramSize / kExposureBinsCount;
> +       const unsigned int yHistValsPerBinMod =
> +               histogramSize / (histogramSize % kExposureBinsCount + 1);
> +       int exposureBins[kExposureBinsCount] = {};
> +       unsigned int denom = 0;
> +       unsigned int num = 0;
> +
> +       for (unsigned int i = 0; i < histogramSize; i++) {
> +               unsigned int idx = (i - (i / yHistValsPerBinMod)) / yHistValsPerBin;
> +               exposureBins[idx] += histogram[blackLevelHistIdx + i];
> +       }
> +
> +       for (unsigned int i = 0; i < kExposureBinsCount; i++) {
> +               LOG(IPASoftExposure, Debug) << i << ": " << exposureBins[i];
> +               denom += exposureBins[i];
> +               num += exposureBins[i] * (i + 1);
> +       }
> +
> +       float exposureMSV = (denom == 0 ? 0 : static_cast<float>(num) / denom);
> +       updateExposure(context, exposureMSV);
> +}
> +
> +REGISTER_IPA_ALGORITHM(Agc, "Agc")
> +
> +} /* namespace ipa::soft::algorithms */
> +
> +} /* namespace libcamera */
> diff --git a/src/ipa/simple/algorithms/agc.h b/src/ipa/simple/algorithms/agc.h
> new file mode 100644
> index 00000000..ad5fca9f
> --- /dev/null
> +++ b/src/ipa/simple/algorithms/agc.h
> @@ -0,0 +1,33 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2024, Red Hat Inc.
> + *
> + * Exposure and gain
> + */
> +
> +#pragma once
> +
> +#include "algorithm.h"
> +
> +namespace libcamera {
> +
> +namespace ipa::soft::algorithms {
> +
> +class Agc : public Algorithm
> +{
> +public:
> +       Agc();
> +       ~Agc() = default;
> +
> +       void process(IPAContext &context, const uint32_t frame,
> +                    IPAFrameContext &frameContext,
> +                    const SwIspStats *stats,
> +                    ControlList &metadata) override;
> +
> +private:
> +       void updateExposure(IPAContext &context, double exposureMSV);
> +};
> +
> +} /* namespace ipa::soft::algorithms */
> +
> +} /* namespace libcamera */
> diff --git a/src/ipa/simple/algorithms/meson.build b/src/ipa/simple/algorithms/meson.build
> index f575611e..37a2eb53 100644
> --- a/src/ipa/simple/algorithms/meson.build
> +++ b/src/ipa/simple/algorithms/meson.build
> @@ -2,6 +2,7 @@
>  
>  soft_simple_ipa_algorithms = files([
>      'awb.cpp',
> +    'agc.cpp',
>      'blc.cpp',
>      'lut.cpp',
>  ])
> diff --git a/src/ipa/simple/data/uncalibrated.yaml b/src/ipa/simple/data/uncalibrated.yaml
> index 893a0b92..3f147112 100644
> --- a/src/ipa/simple/data/uncalibrated.yaml
> +++ b/src/ipa/simple/data/uncalibrated.yaml
> @@ -6,4 +6,5 @@ algorithms:
>    - BlackLevel:
>    - Awb:
>    - Lut:
> +  - Agc:
>  ...
> diff --git a/src/ipa/simple/ipa_context.cpp b/src/ipa/simple/ipa_context.cpp
> index 5fa492cb..3f94bbeb 100644
> --- a/src/ipa/simple/ipa_context.cpp
> +++ b/src/ipa/simple/ipa_context.cpp
> @@ -77,6 +77,17 @@ namespace libcamera::ipa::soft {
>   * \brief Gain of blue color
>   */
>  
> +/**
> + * \var IPAActiveState::agc
> + * \brief Context for the AGC algorithm
> + *
> + * \var IPAActiveState::agc.exposure
> + * \brief Current exposure value
> + *
> + * \var IPAActiveState::agc.again
> + * \brief Current analog gain value
> + */
> +
>  /**
>   * \var IPAActiveState::gamma
>   * \brief Context for gamma in the Colors algorithm
> diff --git a/src/ipa/simple/ipa_context.h b/src/ipa/simple/ipa_context.h
> index d09de9a9..ad869f64 100644
> --- a/src/ipa/simple/ipa_context.h
> +++ b/src/ipa/simple/ipa_context.h
> @@ -9,6 +9,8 @@
>  
>  #include <array>
>  
> +#include <libcamera/controls.h>
> +
>  #include <libipa/fc_queue.h>
>  
>  namespace libcamera {
> @@ -17,6 +19,13 @@ namespace ipa::soft {
>  
>  struct IPASessionConfiguration {
>         float gamma;
> +       struct {
> +               int32_t exposureMin, exposureMax;
> +               double againMin, againMax, againMinStep;
> +       } agc;
> +       struct {
> +               double level;
> +       } black;
>  };
>  
>  struct IPAActiveState {
> @@ -30,6 +39,11 @@ struct IPAActiveState {
>                 double blue;
>         } gains;
>  
> +       struct {
> +               int32_t exposure;
> +               double again;
> +       } agc;
> +
>         static constexpr unsigned int kGammaLookupSize = 1024;
>         struct {
>                 std::array<double, kGammaLookupSize> gammaTable;
> @@ -38,6 +52,10 @@ struct IPAActiveState {
>  };
>  
>  struct IPAFrameContext : public FrameContext {
> +       struct {
> +               uint32_t exposure;
> +               double gain;
> +       } sensor;
>  };
>  
>  struct IPAContext {
> diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp
> index 60310a8e..b28c7039 100644
> --- a/src/ipa/simple/soft_simple.cpp
> +++ b/src/ipa/simple/soft_simple.cpp
> @@ -34,23 +34,6 @@ LOG_DEFINE_CATEGORY(IPASoft)
>  
>  namespace ipa::soft {
>  
> -/*
> - * The number of bins to use for the optimal exposure calculations.
> - */
> -static constexpr unsigned int kExposureBinsCount = 5;
> -
> -/*
> - * The exposure is optimal when the mean sample value of the histogram is
> - * in the middle of the range.
> - */
> -static constexpr float kExposureOptimal = kExposureBinsCount / 2.0;
> -
> -/*
> - * The below value implements the hysteresis for the exposure adjustment.
> - * It is small enough to have the exposure close to the optimal, and is big
> - * enough to prevent the exposure from wobbling around the optimal value.
> - */
> -static constexpr float kExposureSatisfactory = 0.2;
>  /* Maximum number of frame contexts to be held */
>  static constexpr uint32_t kMaxFrameContexts = 16;
>  
> @@ -58,8 +41,7 @@ class IPASoftSimple : public ipa::soft::IPASoftInterface, public Module
>  {
>  public:
>         IPASoftSimple()
> -               : params_(nullptr), stats_(nullptr),
> -                 context_({ {}, {}, { kMaxFrameContexts } })
> +               : context_({ {}, {}, { kMaxFrameContexts } })
>         {
>         }
>  
> @@ -92,11 +74,6 @@ private:
>  
>         /* Local parameter storage */
>         struct IPAContext context_;
> -
> -       int32_t exposureMin_, exposureMax_;
> -       int32_t exposure_;
> -       double againMin_, againMax_, againMinStep_;
> -       double again_;
>  };
>  
>  IPASoftSimple::~IPASoftSimple()
> @@ -207,20 +184,23 @@ int IPASoftSimple::configure(const IPAConfigInfo &configInfo)
>         const ControlInfo &exposureInfo = sensorInfoMap_.find(V4L2_CID_EXPOSURE)->second;
>         const ControlInfo &gainInfo = sensorInfoMap_.find(V4L2_CID_ANALOGUE_GAIN)->second;
>  
> -       exposureMin_ = exposureInfo.min().get<int32_t>();
> -       exposureMax_ = exposureInfo.max().get<int32_t>();
> -       if (!exposureMin_) {
> +       context_.configuration.agc.exposureMin = exposureInfo.min().get<int32_t>();
> +       context_.configuration.agc.exposureMax = exposureInfo.max().get<int32_t>();
> +       if (!context_.configuration.agc.exposureMin) {
>                 LOG(IPASoft, Warning) << "Minimum exposure is zero, that can't be linear";
> -               exposureMin_ = 1;
> +               context_.configuration.agc.exposureMin = 1;
>         }
>  
>         int32_t againMin = gainInfo.min().get<int32_t>();
>         int32_t againMax = gainInfo.max().get<int32_t>();
>  
>         if (camHelper_) {
> -               againMin_ = camHelper_->gain(againMin);
> -               againMax_ = camHelper_->gain(againMax);
> -               againMinStep_ = (againMax_ - againMin_) / 100.0;
> +               context_.configuration.agc.againMin = camHelper_->gain(againMin);
> +               context_.configuration.agc.againMax = camHelper_->gain(againMax);
> +               context_.configuration.agc.againMinStep =
> +                       (context_.configuration.agc.againMax -
> +                        context_.configuration.agc.againMin) /
> +                       100.0;
>         } else {
>                 /*
>                  * The camera sensor gain (g) is usually not equal to the value written
> @@ -232,13 +212,14 @@ int IPASoftSimple::configure(const IPAConfigInfo &configInfo)
>                  * the AGC algorithm (abrupt near one edge, and very small near the
>                  * other) we limit the range of the gain values used.
>                  */
> -               againMax_ = againMax;
> +               context_.configuration.agc.againMax = againMax;
>                 if (!againMin) {
>                         LOG(IPASoft, Warning)
>                                 << "Minimum gain is zero, that can't be linear";
> -                       againMin_ = std::min(100, againMin / 2 + againMax / 2);
> +                       context_.configuration.agc.againMin =
> +                               std::min(100, againMin / 2 + againMax / 2);
>                 }
> -               againMinStep_ = 1.0;
> +               context_.configuration.agc.againMinStep = 1.0;
>         }
>  
>         for (auto const &algo : algorithms()) {
> @@ -247,9 +228,12 @@ int IPASoftSimple::configure(const IPAConfigInfo &configInfo)
>                         return ret;
>         }
>  
> -       LOG(IPASoft, Info) << "Exposure " << exposureMin_ << "-" << exposureMax_
> -                          << ", gain " << againMin_ << "-" << againMax_
> -                          << " (" << againMinStep_ << ")";
> +       LOG(IPASoft, Info)
> +               << "Exposure " << context_.configuration.agc.exposureMin << "-"
> +               << context_.configuration.agc.exposureMax
> +               << ", gain " << context_.configuration.agc.againMin << "-"
> +               << context_.configuration.agc.againMax
> +               << " (" << context_.configuration.agc.againMinStep << ")";
>  
>         return 0;
>  }
> @@ -284,6 +268,12 @@ void IPASoftSimple::processStats(const uint32_t frame,
>                                  const ControlList &sensorControls)
>  {
>         IPAFrameContext &frameContext = context_.frameContexts.get(frame);
> +
> +       frameContext.sensor.exposure =
> +               sensorControls.get(V4L2_CID_EXPOSURE).get<int32_t>();
> +       int32_t again = sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get<int32_t>();
> +       frameContext.sensor.gain = camHelper_ ? camHelper_->gain(again) : again;
> +
>         /*
>          * Software ISP currently does not produce any metadata. Use an empty
>          * ControlList for now.
> @@ -294,37 +284,6 @@ void IPASoftSimple::processStats(const uint32_t frame,
>         for (auto const &algo : algorithms())
>                 algo->process(context_, frame, frameContext, stats_, metadata);
>  
> -       /* \todo Switch to the libipa/algorithm.h API someday. */
> -
> -       /*
> -        * 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 =
> -               SwIspStats::kYHistogramSize - blackLevelHistIdx;
> -       const unsigned int yHistValsPerBin = histogramSize / kExposureBinsCount;
> -       const unsigned int yHistValsPerBinMod =
> -               histogramSize / (histogramSize % kExposureBinsCount + 1);
> -       int exposureBins[kExposureBinsCount] = {};
> -       unsigned int denom = 0;
> -       unsigned int num = 0;
> -
> -       for (unsigned int i = 0; i < histogramSize; i++) {
> -               unsigned int idx = (i - (i / yHistValsPerBinMod)) / yHistValsPerBin;
> -               exposureBins[idx] += stats_->yHistogram[blackLevelHistIdx + i];
> -       }
> -
> -       for (unsigned int i = 0; i < kExposureBinsCount; i++) {
> -               LOG(IPASoft, Debug) << i << ": " << exposureBins[i];
> -               denom += exposureBins[i];
> -               num += exposureBins[i] * (i + 1);
> -       }
> -
> -       float exposureMSV = static_cast<float>(num) / denom;
> -
>         /* Sanity check */
>         if (!sensorControls.contains(V4L2_CID_EXPOSURE) ||
>             !sensorControls.contains(V4L2_CID_ANALOGUE_GAIN)) {
> @@ -332,70 +291,14 @@ void IPASoftSimple::processStats(const uint32_t frame,
>                 return;
>         }
>  
> -       exposure_ = sensorControls.get(V4L2_CID_EXPOSURE).get<int32_t>();
> -       int32_t again = sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get<int32_t>();
> -       again_ = camHelper_ ? camHelper_->gain(again) : again;
> -
> -       updateExposure(exposureMSV);
> -
>         ControlList ctrls(sensorInfoMap_);
>  
> -       ctrls.set(V4L2_CID_EXPOSURE, exposure_);
> +       auto &againNew = context_.activeState.agc.again;
> +       ctrls.set(V4L2_CID_EXPOSURE, context_.activeState.agc.exposure);
>         ctrls.set(V4L2_CID_ANALOGUE_GAIN,
> -                 static_cast<int32_t>(camHelper_ ? camHelper_->gainCode(again_) : again_));
> +                 static_cast<int32_t>(camHelper_ ? camHelper_->gainCode(againNew) : againNew));
>  
>         setSensorControls.emit(ctrls);

I'm not 100% certain on the right place to handle updates to the sensor
controls yet - but I think that will get cleared up once more controls
get added in here.

So for now, I believe this is good progress:

Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

> -
> -       LOG(IPASoft, Debug) << "exposureMSV " << exposureMSV
> -                           << " exp " << exposure_ << " again " << again_
> -                           << " black level " << static_cast<unsigned int>(blackLevel);
> -}
> -
> -void IPASoftSimple::updateExposure(double exposureMSV)
> -{
> -       /*
> -        * kExpDenominator of 10 gives ~10% increment/decrement;
> -        * kExpDenominator of 5 - about ~20%
> -        */
> -       static constexpr uint8_t kExpDenominator = 10;
> -       static constexpr uint8_t kExpNumeratorUp = kExpDenominator + 1;
> -       static constexpr uint8_t kExpNumeratorDown = kExpDenominator - 1;
> -
> -       double next;
> -
> -       if (exposureMSV < kExposureOptimal - kExposureSatisfactory) {
> -               next = exposure_ * kExpNumeratorUp / kExpDenominator;
> -               if (next - exposure_ < 1)
> -                       exposure_ += 1;
> -               else
> -                       exposure_ = next;
> -               if (exposure_ >= exposureMax_) {
> -                       next = again_ * kExpNumeratorUp / kExpDenominator;
> -                       if (next - again_ < againMinStep_)
> -                               again_ += againMinStep_;
> -                       else
> -                               again_ = next;
> -               }
> -       }
> -
> -       if (exposureMSV > kExposureOptimal + kExposureSatisfactory) {
> -               if (exposure_ == exposureMax_ && again_ > againMin_) {
> -                       next = again_ * kExpNumeratorDown / kExpDenominator;
> -                       if (again_ - next < againMinStep_)
> -                               again_ -= againMinStep_;
> -                       else
> -                               again_ = next;
> -               } else {
> -                       next = exposure_ * kExpNumeratorDown / kExpDenominator;
> -                       if (exposure_ - next < 1)
> -                               exposure_ -= 1;
> -                       else
> -                               exposure_ = next;
> -               }
> -       }
> -
> -       exposure_ = std::clamp(exposure_, exposureMin_, exposureMax_);
> -       again_ = std::clamp(again_, againMin_, againMax_);
>  }
>  
>  std::string IPASoftSimple::logPrefix() const
> diff --git a/src/libcamera/software_isp/TODO b/src/libcamera/software_isp/TODO
> index 8eeede46..a50db668 100644
> --- a/src/libcamera/software_isp/TODO
> +++ b/src/libcamera/software_isp/TODO
> @@ -199,16 +199,6 @@ Yes, because, well... all the other IPAs were doing that...
>  
>  ---
>  
> -10. Switch to libipa/algorithm.h API in processStats
> -
> ->> void IPASoftSimple::processStats(const ControlList &sensorControls)
> ->>
> -> Do you envision switching to the libipa/algorithm.h API at some point ?
> -
> -At some point, yes.
> -
> ----
> -
>  13. Improve black level and colour gains application
>  
>  I think the black level should eventually be moved before debayering, and
> -- 
> 2.44.1
>

Hi Kieran,

thank you for review.

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

> Quoting Milan Zamazal (2024-09-06 13:09:26)
>> This is the last step to fully convert software ISP to Algorithm-based
>> processing.
>
>> 
>> The newly introduced frameContext.sensor properties are set, and the
>> updated code moved, before calling Algorithm::process() to have the
>> values up-to-date in stats processing.
>> 
>> Resolves software ISP TODO #10.
>
> I'm not going to dwell on the details in here either. Modularising this
> is a big benefit.
>
> I haven't yet checked if this is comparable to the existing AGC
> implementation in libipa, but now it's modular - it would be interesting
> to 'try' using the existing libipa algorithm on soft ISP - or try 'this'
> implementation on other libipa platforms.
>
> Perhaps if they're equivalent they could be merged, or if they are
> uniquely separate they could be named as such in libipa so they could be
> selected.

I'm not currently aware about anything worth merging and trying to
explore it would be over my distraction absorption capacity now.  But
yes, the idea of making common things common regarding the algorithms
etc. has already been raised and it's certainly something we should look
at sooner or later.  Most likely once we start working on improving
software ISP image quality seriously.

> But either way, I think this makes it all more manageable in the long
> run.
>
>
>> 
>> Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
>> ---
>>  src/ipa/simple/algorithms/agc.cpp     | 139 +++++++++++++++++++++++
>>  src/ipa/simple/algorithms/agc.h       |  33 ++++++
>>  src/ipa/simple/algorithms/meson.build |   1 +
>>  src/ipa/simple/data/uncalibrated.yaml |   1 +
>>  src/ipa/simple/ipa_context.cpp        |  11 ++
>>  src/ipa/simple/ipa_context.h          |  18 +++
>>  src/ipa/simple/soft_simple.cpp        | 157 +++++---------------------
>>  src/libcamera/software_isp/TODO       |  10 --
>>  8 files changed, 233 insertions(+), 137 deletions(-)
>>  create mode 100644 src/ipa/simple/algorithms/agc.cpp
>>  create mode 100644 src/ipa/simple/algorithms/agc.h
>> 
>> diff --git a/src/ipa/simple/algorithms/agc.cpp b/src/ipa/simple/algorithms/agc.cpp
>> new file mode 100644
>> index 00000000..becfb99f
>> --- /dev/null
>> +++ b/src/ipa/simple/algorithms/agc.cpp
>> @@ -0,0 +1,139 @@
>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
>> +/*
>> + * Copyright (C) 2024, Red Hat Inc.
>> + *
>> + * Exposure and gain
>> + */
>> +
>> +#include "agc.h"
>> +
>> +#include <stdint.h>
>> +
>> +#include <libcamera/base/log.h>
>> +
>> +namespace libcamera {
>> +
>> +LOG_DEFINE_CATEGORY(IPASoftExposure)
>> +
>> +namespace ipa::soft::algorithms {
>> +
>> +/*
>> + * The number of bins to use for the optimal exposure calculations.
>> + */
>> +static constexpr unsigned int kExposureBinsCount = 5;
>> +
>> +/*
>> + * The exposure is optimal when the mean sample value of the histogram is
>> + * in the middle of the range.
>> + */
>> +static constexpr float kExposureOptimal = kExposureBinsCount / 2.0;
>> +
>> +/*
>> + * The below value implements the hysteresis for the exposure adjustment.
>> + * It is small enough to have the exposure close to the optimal, and is big
>> + * enough to prevent the exposure from wobbling around the optimal value.
>> + */
>> +static constexpr float kExposureSatisfactory = 0.2;
>> +
>> +Agc::Agc()
>> +{
>> +}
>> +
>> +void Agc::updateExposure(IPAContext &context, double exposureMSV)
>> +{
>> +       /*
>> +        * kExpDenominator of 10 gives ~10% increment/decrement;
>> +        * kExpDenominator of 5 - about ~20%
>> +        */
>> +       static constexpr uint8_t kExpDenominator = 10;
>> +       static constexpr uint8_t kExpNumeratorUp = kExpDenominator + 1;
>> +       static constexpr uint8_t kExpNumeratorDown = kExpDenominator - 1;
>> +
>> +       double next;
>> +       int32_t &exposure = context.activeState.agc.exposure;
>> +       double &again = context.activeState.agc.again;
>> +
>> +       if (exposureMSV < kExposureOptimal - kExposureSatisfactory) {
>> +               next = exposure * kExpNumeratorUp / kExpDenominator;
>> +               if (next - exposure < 1)
>> +                       exposure += 1;
>> +               else
>> +                       exposure = next;
>> +               if (exposure >= context.configuration.agc.exposureMax) {
>> +                       next = again * kExpNumeratorUp / kExpDenominator;
>> +                       if (next - again < context.configuration.agc.againMinStep)
>> +                               again += context.configuration.agc.againMinStep;
>> +                       else
>> +                               again = next;
>> +               }
>> +       }
>> +
>> +       if (exposureMSV > kExposureOptimal + kExposureSatisfactory) {
>> +               if (exposure == context.configuration.agc.exposureMax &&
>> +                   again > context.configuration.agc.againMin) {
>> +                       next = again * kExpNumeratorDown / kExpDenominator;
>> +                       if (again - next < context.configuration.agc.againMinStep)
>> +                               again -= context.configuration.agc.againMinStep;
>> +                       else
>> +                               again = next;
>> +               } else {
>> +                       next = exposure * kExpNumeratorDown / kExpDenominator;
>> +                       if (exposure - next < 1)
>> +                               exposure -= 1;
>> +                       else
>> +                               exposure = next;
>> +               }
>> +       }
>> +
>> +       exposure = std::clamp(exposure, context.configuration.agc.exposureMin,
>> +                             context.configuration.agc.exposureMax);
>> +       again = std::clamp(again, context.configuration.agc.againMin,
>> +                          context.configuration.agc.againMax);
>> +
>> +       LOG(IPASoftExposure, Debug)
>> +               << "exposureMSV " << exposureMSV
>> +               << " exp " << exposure << " again " << again;
>> +}
>> +
>> +void Agc::process(IPAContext &context,
>> +                 [[maybe_unused]] const uint32_t frame,
>> +                 [[maybe_unused]] IPAFrameContext &frameContext,
>> +                 const SwIspStats *stats,
>> +                 [[maybe_unused]] ControlList &metadata)
>> +{
>> +       /*
>> +        * Calculate Mean Sample Value (MSV) according to formula from:
>> +        * https://www.araa.asn.au/acra/acra2007/papers/paper84final.pdf
>> +        */
>> +       const auto &histogram = stats->yHistogram;
>> +       const unsigned int blackLevelHistIdx =
>> +               context.activeState.blc.level * SwIspStats::kYHistogramSize;
>> +       const unsigned int histogramSize =
>> +               SwIspStats::kYHistogramSize - blackLevelHistIdx;
>> +       const unsigned int yHistValsPerBin = histogramSize / kExposureBinsCount;
>> +       const unsigned int yHistValsPerBinMod =
>> +               histogramSize / (histogramSize % kExposureBinsCount + 1);
>> +       int exposureBins[kExposureBinsCount] = {};
>> +       unsigned int denom = 0;
>> +       unsigned int num = 0;
>> +
>> +       for (unsigned int i = 0; i < histogramSize; i++) {
>> +               unsigned int idx = (i - (i / yHistValsPerBinMod)) / yHistValsPerBin;
>> +               exposureBins[idx] += histogram[blackLevelHistIdx + i];
>> +       }
>> +
>> +       for (unsigned int i = 0; i < kExposureBinsCount; i++) {
>> +               LOG(IPASoftExposure, Debug) << i << ": " << exposureBins[i];
>> +               denom += exposureBins[i];
>> +               num += exposureBins[i] * (i + 1);
>> +       }
>> +
>> +       float exposureMSV = (denom == 0 ? 0 : static_cast<float>(num) / denom);
>> +       updateExposure(context, exposureMSV);
>> +}
>> +
>> +REGISTER_IPA_ALGORITHM(Agc, "Agc")
>> +
>> +} /* namespace ipa::soft::algorithms */
>> +
>> +} /* namespace libcamera */
>> diff --git a/src/ipa/simple/algorithms/agc.h b/src/ipa/simple/algorithms/agc.h
>> new file mode 100644
>> index 00000000..ad5fca9f
>> --- /dev/null
>> +++ b/src/ipa/simple/algorithms/agc.h
>> @@ -0,0 +1,33 @@
>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
>> +/*
>> + * Copyright (C) 2024, Red Hat Inc.
>> + *
>> + * Exposure and gain
>> + */
>> +
>> +#pragma once
>> +
>> +#include "algorithm.h"
>> +
>> +namespace libcamera {
>> +
>> +namespace ipa::soft::algorithms {
>> +
>> +class Agc : public Algorithm
>> +{
>> +public:
>> +       Agc();
>> +       ~Agc() = default;
>> +
>> +       void process(IPAContext &context, const uint32_t frame,
>> +                    IPAFrameContext &frameContext,
>> +                    const SwIspStats *stats,
>> +                    ControlList &metadata) override;
>> +
>> +private:
>> +       void updateExposure(IPAContext &context, double exposureMSV);
>> +};
>> +
>> +} /* namespace ipa::soft::algorithms */
>> +
>> +} /* namespace libcamera */
>> diff --git a/src/ipa/simple/algorithms/meson.build b/src/ipa/simple/algorithms/meson.build
>> index f575611e..37a2eb53 100644
>> --- a/src/ipa/simple/algorithms/meson.build
>> +++ b/src/ipa/simple/algorithms/meson.build
>> @@ -2,6 +2,7 @@
>>  
>>  soft_simple_ipa_algorithms = files([
>>      'awb.cpp',
>> +    'agc.cpp',
>>      'blc.cpp',
>>      'lut.cpp',
>>  ])
>> diff --git a/src/ipa/simple/data/uncalibrated.yaml b/src/ipa/simple/data/uncalibrated.yaml
>> index 893a0b92..3f147112 100644
>> --- a/src/ipa/simple/data/uncalibrated.yaml
>> +++ b/src/ipa/simple/data/uncalibrated.yaml
>> @@ -6,4 +6,5 @@ algorithms:
>>    - BlackLevel:
>>    - Awb:
>>    - Lut:
>> +  - Agc:
>>  ...
>> diff --git a/src/ipa/simple/ipa_context.cpp b/src/ipa/simple/ipa_context.cpp
>> index 5fa492cb..3f94bbeb 100644
>> --- a/src/ipa/simple/ipa_context.cpp
>> +++ b/src/ipa/simple/ipa_context.cpp
>> @@ -77,6 +77,17 @@ namespace libcamera::ipa::soft {
>>   * \brief Gain of blue color
>>   */
>>  
>> +/**
>> + * \var IPAActiveState::agc
>> + * \brief Context for the AGC algorithm
>> + *
>> + * \var IPAActiveState::agc.exposure
>> + * \brief Current exposure value
>> + *
>> + * \var IPAActiveState::agc.again
>> + * \brief Current analog gain value
>> + */
>> +
>>  /**
>>   * \var IPAActiveState::gamma
>>   * \brief Context for gamma in the Colors algorithm
>> diff --git a/src/ipa/simple/ipa_context.h b/src/ipa/simple/ipa_context.h
>> index d09de9a9..ad869f64 100644
>> --- a/src/ipa/simple/ipa_context.h
>> +++ b/src/ipa/simple/ipa_context.h
>> @@ -9,6 +9,8 @@
>>  
>>  #include <array>
>>  
>> +#include <libcamera/controls.h>
>> +
>>  #include <libipa/fc_queue.h>
>>  
>>  namespace libcamera {
>> @@ -17,6 +19,13 @@ namespace ipa::soft {
>>  
>>  struct IPASessionConfiguration {
>>         float gamma;
>> +       struct {
>> +               int32_t exposureMin, exposureMax;
>> +               double againMin, againMax, againMinStep;
>> +       } agc;
>> +       struct {
>> +               double level;
>> +       } black;
>>  };
>>  
>>  struct IPAActiveState {
>> @@ -30,6 +39,11 @@ struct IPAActiveState {
>>                 double blue;
>>         } gains;
>>  
>> +       struct {
>> +               int32_t exposure;
>> +               double again;
>> +       } agc;
>> +
>>         static constexpr unsigned int kGammaLookupSize = 1024;
>>         struct {
>>                 std::array<double, kGammaLookupSize> gammaTable;
>> @@ -38,6 +52,10 @@ struct IPAActiveState {
>>  };
>>  
>>  struct IPAFrameContext : public FrameContext {
>> +       struct {
>> +               uint32_t exposure;
>> +               double gain;
>> +       } sensor;
>>  };
>>  
>>  struct IPAContext {
>> diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp
>> index 60310a8e..b28c7039 100644
>> --- a/src/ipa/simple/soft_simple.cpp
>> +++ b/src/ipa/simple/soft_simple.cpp
>> @@ -34,23 +34,6 @@ LOG_DEFINE_CATEGORY(IPASoft)
>>  
>>  namespace ipa::soft {
>>  
>> -/*
>> - * The number of bins to use for the optimal exposure calculations.
>> - */
>> -static constexpr unsigned int kExposureBinsCount = 5;
>> -
>> -/*
>> - * The exposure is optimal when the mean sample value of the histogram is
>> - * in the middle of the range.
>> - */
>> -static constexpr float kExposureOptimal = kExposureBinsCount / 2.0;
>> -
>> -/*
>> - * The below value implements the hysteresis for the exposure adjustment.
>> - * It is small enough to have the exposure close to the optimal, and is big
>> - * enough to prevent the exposure from wobbling around the optimal value.
>> - */
>> -static constexpr float kExposureSatisfactory = 0.2;
>>  /* Maximum number of frame contexts to be held */
>>  static constexpr uint32_t kMaxFrameContexts = 16;
>>  
>> @@ -58,8 +41,7 @@ class IPASoftSimple : public ipa::soft::IPASoftInterface, public Module
>>  {
>>  public:
>>         IPASoftSimple()
>> -               : params_(nullptr), stats_(nullptr),
>> -                 context_({ {}, {}, { kMaxFrameContexts } })
>> +               : context_({ {}, {}, { kMaxFrameContexts } })
>>         {
>>         }
>>  
>> @@ -92,11 +74,6 @@ private:
>>  
>>         /* Local parameter storage */
>>         struct IPAContext context_;
>> -
>> -       int32_t exposureMin_, exposureMax_;
>> -       int32_t exposure_;
>> -       double againMin_, againMax_, againMinStep_;
>> -       double again_;
>>  };
>>  
>>  IPASoftSimple::~IPASoftSimple()
>> @@ -207,20 +184,23 @@ int IPASoftSimple::configure(const IPAConfigInfo &configInfo)
>>         const ControlInfo &exposureInfo = sensorInfoMap_.find(V4L2_CID_EXPOSURE)->second;
>>         const ControlInfo &gainInfo = sensorInfoMap_.find(V4L2_CID_ANALOGUE_GAIN)->second;
>>  
>> -       exposureMin_ = exposureInfo.min().get<int32_t>();
>> -       exposureMax_ = exposureInfo.max().get<int32_t>();
>> -       if (!exposureMin_) {
>> +       context_.configuration.agc.exposureMin = exposureInfo.min().get<int32_t>();
>> +       context_.configuration.agc.exposureMax = exposureInfo.max().get<int32_t>();
>> +       if (!context_.configuration.agc.exposureMin) {
>>                 LOG(IPASoft, Warning) << "Minimum exposure is zero, that can't be linear";
>> -               exposureMin_ = 1;
>> +               context_.configuration.agc.exposureMin = 1;
>>         }
>>  
>>         int32_t againMin = gainInfo.min().get<int32_t>();
>>         int32_t againMax = gainInfo.max().get<int32_t>();
>>  
>>         if (camHelper_) {
>> -               againMin_ = camHelper_->gain(againMin);
>> -               againMax_ = camHelper_->gain(againMax);
>> -               againMinStep_ = (againMax_ - againMin_) / 100.0;
>> +               context_.configuration.agc.againMin = camHelper_->gain(againMin);
>> +               context_.configuration.agc.againMax = camHelper_->gain(againMax);
>> +               context_.configuration.agc.againMinStep =
>> +                       (context_.configuration.agc.againMax -
>> +                        context_.configuration.agc.againMin) /
>> +                       100.0;
>>         } else {
>>                 /*
>>                  * The camera sensor gain (g) is usually not equal to the value written
>> @@ -232,13 +212,14 @@ int IPASoftSimple::configure(const IPAConfigInfo &configInfo)
>>                  * the AGC algorithm (abrupt near one edge, and very small near the
>>                  * other) we limit the range of the gain values used.
>>                  */
>> -               againMax_ = againMax;
>> +               context_.configuration.agc.againMax = againMax;
>>                 if (!againMin) {
>>                         LOG(IPASoft, Warning)
>>                                 << "Minimum gain is zero, that can't be linear";
>> -                       againMin_ = std::min(100, againMin / 2 + againMax / 2);
>> +                       context_.configuration.agc.againMin =
>> +                               std::min(100, againMin / 2 + againMax / 2);
>>                 }
>> -               againMinStep_ = 1.0;
>> +               context_.configuration.agc.againMinStep = 1.0;
>>         }
>>  
>>         for (auto const &algo : algorithms()) {
>> @@ -247,9 +228,12 @@ int IPASoftSimple::configure(const IPAConfigInfo &configInfo)
>>                         return ret;
>>         }
>>  
>> -       LOG(IPASoft, Info) << "Exposure " << exposureMin_ << "-" << exposureMax_
>> -                          << ", gain " << againMin_ << "-" << againMax_
>> -                          << " (" << againMinStep_ << ")";
>> +       LOG(IPASoft, Info)
>> +               << "Exposure " << context_.configuration.agc.exposureMin << "-"
>> +               << context_.configuration.agc.exposureMax
>> +               << ", gain " << context_.configuration.agc.againMin << "-"
>> +               << context_.configuration.agc.againMax
>> +               << " (" << context_.configuration.agc.againMinStep << ")";
>>  
>>         return 0;
>>  }
>> @@ -284,6 +268,12 @@ void IPASoftSimple::processStats(const uint32_t frame,
>>                                  const ControlList &sensorControls)
>>  {
>>         IPAFrameContext &frameContext = context_.frameContexts.get(frame);
>> +
>> +       frameContext.sensor.exposure =
>> +               sensorControls.get(V4L2_CID_EXPOSURE).get<int32_t>();
>> +       int32_t again = sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get<int32_t>();
>> +       frameContext.sensor.gain = camHelper_ ? camHelper_->gain(again) : again;
>> +
>>         /*
>>          * Software ISP currently does not produce any metadata. Use an empty
>>          * ControlList for now.
>> @@ -294,37 +284,6 @@ void IPASoftSimple::processStats(const uint32_t frame,
>>         for (auto const &algo : algorithms())
>>                 algo->process(context_, frame, frameContext, stats_, metadata);
>>  
>> -       /* \todo Switch to the libipa/algorithm.h API someday. */
>> -
>> -       /*
>> -        * 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 =
>> -               SwIspStats::kYHistogramSize - blackLevelHistIdx;
>> -       const unsigned int yHistValsPerBin = histogramSize / kExposureBinsCount;
>> -       const unsigned int yHistValsPerBinMod =
>> -               histogramSize / (histogramSize % kExposureBinsCount + 1);
>> -       int exposureBins[kExposureBinsCount] = {};
>> -       unsigned int denom = 0;
>> -       unsigned int num = 0;
>> -
>> -       for (unsigned int i = 0; i < histogramSize; i++) {
>> -               unsigned int idx = (i - (i / yHistValsPerBinMod)) / yHistValsPerBin;
>> -               exposureBins[idx] += stats_->yHistogram[blackLevelHistIdx + i];
>> -       }
>> -
>> -       for (unsigned int i = 0; i < kExposureBinsCount; i++) {
>> -               LOG(IPASoft, Debug) << i << ": " << exposureBins[i];
>> -               denom += exposureBins[i];
>> -               num += exposureBins[i] * (i + 1);
>> -       }
>> -
>> -       float exposureMSV = static_cast<float>(num) / denom;
>> -
>>         /* Sanity check */
>>         if (!sensorControls.contains(V4L2_CID_EXPOSURE) ||
>>             !sensorControls.contains(V4L2_CID_ANALOGUE_GAIN)) {
>> @@ -332,70 +291,14 @@ void IPASoftSimple::processStats(const uint32_t frame,
>>                 return;
>>         }
>>  
>> -       exposure_ = sensorControls.get(V4L2_CID_EXPOSURE).get<int32_t>();
>> -       int32_t again = sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get<int32_t>();
>> -       again_ = camHelper_ ? camHelper_->gain(again) : again;
>> -
>> -       updateExposure(exposureMSV);
>> -
>>         ControlList ctrls(sensorInfoMap_);
>>  
>> -       ctrls.set(V4L2_CID_EXPOSURE, exposure_);
>> +       auto &againNew = context_.activeState.agc.again;
>> +       ctrls.set(V4L2_CID_EXPOSURE, context_.activeState.agc.exposure);
>>         ctrls.set(V4L2_CID_ANALOGUE_GAIN,
>> -                 static_cast<int32_t>(camHelper_ ? camHelper_->gainCode(again_) : again_));
>> +                 static_cast<int32_t>(camHelper_ ? camHelper_->gainCode(againNew) : againNew));
>>  
>>         setSensorControls.emit(ctrls);
>
> I'm not 100% certain on the right place to handle updates to the sensor
> controls yet - but I think that will get cleared up once more controls
> get added in here.

OK, let's see.

> So for now, I believe this is good progress:
>
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>
>> -
>> -       LOG(IPASoft, Debug) << "exposureMSV " << exposureMSV
>> -                           << " exp " << exposure_ << " again " << again_
>> -                           << " black level " << static_cast<unsigned int>(blackLevel);
>> -}
>> -
>> -void IPASoftSimple::updateExposure(double exposureMSV)
>> -{
>> -       /*
>> -        * kExpDenominator of 10 gives ~10% increment/decrement;
>> -        * kExpDenominator of 5 - about ~20%
>> -        */
>> -       static constexpr uint8_t kExpDenominator = 10;
>> -       static constexpr uint8_t kExpNumeratorUp = kExpDenominator + 1;
>> -       static constexpr uint8_t kExpNumeratorDown = kExpDenominator - 1;
>> -
>> -       double next;
>> -
>> -       if (exposureMSV < kExposureOptimal - kExposureSatisfactory) {
>> -               next = exposure_ * kExpNumeratorUp / kExpDenominator;
>> -               if (next - exposure_ < 1)
>> -                       exposure_ += 1;
>> -               else
>> -                       exposure_ = next;
>> -               if (exposure_ >= exposureMax_) {
>> -                       next = again_ * kExpNumeratorUp / kExpDenominator;
>> -                       if (next - again_ < againMinStep_)
>> -                               again_ += againMinStep_;
>> -                       else
>> -                               again_ = next;
>> -               }
>> -       }
>> -
>> -       if (exposureMSV > kExposureOptimal + kExposureSatisfactory) {
>> -               if (exposure_ == exposureMax_ && again_ > againMin_) {
>> -                       next = again_ * kExpNumeratorDown / kExpDenominator;
>> -                       if (again_ - next < againMinStep_)
>> -                               again_ -= againMinStep_;
>> -                       else
>> -                               again_ = next;
>> -               } else {
>> -                       next = exposure_ * kExpNumeratorDown / kExpDenominator;
>> -                       if (exposure_ - next < 1)
>> -                               exposure_ -= 1;
>> -                       else
>> -                               exposure_ = next;
>> -               }
>> -       }
>> -
>> -       exposure_ = std::clamp(exposure_, exposureMin_, exposureMax_);
>> -       again_ = std::clamp(again_, againMin_, againMax_);
>>  }
>>  
>>  std::string IPASoftSimple::logPrefix() const
>> diff --git a/src/libcamera/software_isp/TODO b/src/libcamera/software_isp/TODO
>> index 8eeede46..a50db668 100644
>> --- a/src/libcamera/software_isp/TODO
>> +++ b/src/libcamera/software_isp/TODO
>> @@ -199,16 +199,6 @@ Yes, because, well... all the other IPAs were doing that...
>>  
>>  ---
>>  
>> -10. Switch to libipa/algorithm.h API in processStats
>> -
>> ->> void IPASoftSimple::processStats(const ControlList &sensorControls)
>> ->>
>> -> Do you envision switching to the libipa/algorithm.h API at some point ?
>> -
>> -At some point, yes.
>> -
>> ----
>> -
>>  13. Improve black level and colour gains application
>>  
>>  I think the black level should eventually be moved before debayering, and
>> -- 
>> 2.44.1
>>