[PATCH v7 17/18] libcamera: software_isp: Move exposure+gain to an algorithm module
Milan Zamazal
mzamazal at redhat.com
Fri Sep 27 15:26:16 CEST 2024
Hi Umang,
thank you for review.
Umang Jain <umang.jain at ideasonboard.com> writes:
> 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/
This one is present in the original code and just moved. I don't like
making unnecessary changes in moved code since reviewing code movements
is already difficult enough. So let's fix this in a separate patch
later.
>> + * 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) ?
As far as I can see the include is not needed. I'll remove it.
> 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