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

21322 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>
Reviewed-by: Kieran Bingham <kieran.bingham@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

Milan Zamazal <mzamazal@redhat.com> writes:

> 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>
> Reviewed-by: Kieran Bingham <kieran.bingham@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.
> + * 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>
> +
>  #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;

This struct should be dropped, it's not used anywhere.

>  };
>  
>  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

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@redhat.com>
> Reviewed-by: Kieran Bingham <kieran.bingham@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@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

Hi Umang,

thank you for review.

Umang Jain <umang.jain@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@redhat.com>
>> Reviewed-by: Kieran Bingham <kieran.bingham@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@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