[PATCH v7 17/18] libcamera: software_isp: Move exposure+gain to an algorithm module
Umang Jain
umang.jain at ideasonboard.com
Fri Sep 27 09:07:55 CEST 2024
Hi Milan,
Thank you for the patch.
On 21/09/24 12:01 am, Milan Zamazal wrote:
> This is the last step to fully convert software ISP to Algorithm-based
> processing.
>
> The newly introduced frameContext.sensor properties are set, and the
s/sensor properties/sensor parameters
> 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 at redhat.com>
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.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..783dfb8b
> --- /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.
s/The below value/This/
> + * 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 / (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] += 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 cf1ef3fd..eb095c85 100644
> --- a/src/ipa/simple/ipa_context.h
> +++ b/src/ipa/simple/ipa_context.h
> @@ -10,6 +10,8 @@
> #include <array>
> #include <stdint.h>
>
> +#include <libcamera/controls.h>
> +
Is there required here?
I see additions pertaining to ints and doubles .. so not sure if it's
required ? Or does this belong to previous patch(es) ?
Reviewed-by: Umang Jain <umang.jain at ideasonboard.com>
> #include <libipa/fc_queue.h>
>
> namespace libcamera {
> @@ -18,6 +20,13 @@ namespace ipa::soft {
>
> struct IPASessionConfiguration {
> float gamma;
> + struct {
> + int32_t exposureMin, exposureMax;
> + double againMin, againMax, againMinStep;
> + } agc;
> + struct {
> + double level;
> + } black;
> };
>
> struct IPAActiveState {
> @@ -31,6 +40,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;
> @@ -39,6 +53,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);
> -
> - 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
More information about the libcamera-devel
mailing list