[libcamera-devel,v5.1,10/11] ipa: rkisp1: Introduce AGC

14812 diff mbox series

Now that we have IPAContext and Algorithm, we can implement a simple AGC
based on the IPU3 one. It is very similar, except that there is no
histogram used for an inter quantile mean. The RkISP1 is returning a 5x5
array (for V10) of luminance means. Estimating the relative luminance is
thus a simple mean of all the blocks already calculated by the ISP.

Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>

---
v6:
	- Move lineDuration in a configuration.sensor structure in
	  IPAContext
	- Add a documentation for filterExposure()
	- Move shutterTime initialisation
v5:
	- use private filteredExposure_ and pass currentExposure as a
	  member variable
	- Drop num and replace it with numCells_
	- Make exposure and gain local variables in IPARkISP1
	- Shorter the lines in processEvent()
v4:
	- use #pragma once
	- Return filtered value from the function
	- Store line duration in IPASessionConfiguration
	- Use the hw revision to configure the number of AE cells
---
 src/ipa/rkisp1/algorithms/agc.cpp     | 280 ++++++++++++++++++++++++++
 src/ipa/rkisp1/algorithms/agc.h       |  46 +++++
 src/ipa/rkisp1/algorithms/meson.build |   1 +
 src/ipa/rkisp1/ipa_context.cpp        |  50 +++++
 src/ipa/rkisp1/ipa_context.h          |  22 ++
 src/ipa/rkisp1/rkisp1.cpp             |  83 ++++----
 6 files changed, 438 insertions(+), 44 deletions(-)
 create mode 100644 src/ipa/rkisp1/algorithms/agc.cpp
 create mode 100644 src/ipa/rkisp1/algorithms/agc.h

Hi Jean-Michel,

Thank you for the patch.

On Fri, Nov 26, 2021 at 05:43:01PM +0100, Jean-Michel Hautbois wrote:
> Now that we have IPAContext and Algorithm, we can implement a simple AGC
> based on the IPU3 one. It is very similar, except that there is no
> histogram used for an inter quantile mean. The RkISP1 is returning a 5x5
> array (for V10) of luminance means. Estimating the relative luminance is
> thus a simple mean of all the blocks already calculated by the ISP.
> 
> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
> 
> ---
> v6:
> 	- Move lineDuration in a configuration.sensor structure in
> 	  IPAContext
> 	- Add a documentation for filterExposure()
> 	- Move shutterTime initialisation
> v5:
> 	- use private filteredExposure_ and pass currentExposure as a
> 	  member variable
> 	- Drop num and replace it with numCells_
> 	- Make exposure and gain local variables in IPARkISP1
> 	- Shorter the lines in processEvent()
> v4:
> 	- use #pragma once
> 	- Return filtered value from the function
> 	- Store line duration in IPASessionConfiguration
> 	- Use the hw revision to configure the number of AE cells
> ---
>  src/ipa/rkisp1/algorithms/agc.cpp     | 280 ++++++++++++++++++++++++++
>  src/ipa/rkisp1/algorithms/agc.h       |  46 +++++
>  src/ipa/rkisp1/algorithms/meson.build |   1 +
>  src/ipa/rkisp1/ipa_context.cpp        |  50 +++++
>  src/ipa/rkisp1/ipa_context.h          |  22 ++
>  src/ipa/rkisp1/rkisp1.cpp             |  83 ++++----
>  6 files changed, 438 insertions(+), 44 deletions(-)
>  create mode 100644 src/ipa/rkisp1/algorithms/agc.cpp
>  create mode 100644 src/ipa/rkisp1/algorithms/agc.h
> 
> diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp
> new file mode 100644
> index 00000000..55d7405a
> --- /dev/null
> +++ b/src/ipa/rkisp1/algorithms/agc.cpp
> @@ -0,0 +1,280 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2021, Ideas On Board
> + *
> + * agc.cpp - AGC/AEC mean-based control algorithm
> + */
> +
> +#include "agc.h"
> +
> +#include <algorithm>
> +#include <chrono>
> +#include <cmath>
> +
> +#include <libcamera/base/log.h>
> +
> +#include <libcamera/ipa/core_ipa_interface.h>
> +
> +/**
> + * \file agc.h
> + */
> +
> +namespace libcamera {
> +
> +using namespace std::literals::chrono_literals;
> +
> +namespace ipa::rkisp1::algorithms {
> +
> +/**
> + * \class Agc
> + * \brief A mean-based auto-exposure algorithm
> + */
> +
> +LOG_DEFINE_CATEGORY(RkISP1Agc)
> +
> +/* Limits for analogue gain values */
> +static constexpr double kMinAnalogueGain = 1.0;
> +static constexpr double kMaxAnalogueGain = 8.0;
> +
> +/* \todo Honour the FrameDurationLimits control instead of hardcoding a limit */
> +static constexpr utils::Duration kMaxShutterSpeed = 60ms;
> +
> +/* Number of frames to wait before calculating stats on minimum exposure */
> +static constexpr uint32_t kNumStartupFrames = 10;
> +
> +/*
> + * Relative luminance target.
> + *
> + * It's a number that's chosen so that, when the camera points at a grey
> + * target, the resulting image brightness is considered right.
> + */
> +static constexpr double kRelativeLuminanceTarget = 0.4;

This is much higher than for the IPU3, why is that ?

> +
> +Agc::Agc()
> +	: frameCount_(0), filteredExposure_(0s)
> +{
> +}
> +
> +/**
> + * \brief Configure the AGC given a configInfo
> + * \param[in] context The shared IPA context
> + * \param[in] configInfo The IPA configuration data
> + *
> + * \return 0
> + */
> +int Agc::configure(IPAContext &context,
> +		   [[maybe_unused]] const IPACameraSensorInfo &configInfo)
> +{
> +	/* Configure the default exposure and gain. */
> +	context.frameContext.agc.gain = std::max(context.configuration.agc.minAnalogueGain, kMinAnalogueGain);
> +	context.frameContext.agc.exposure = 10ms / context.configuration.sensor.lineDuration;
> +
> +	/*
> +	 * According to the RkISP1 documentation:
> +	 * - versions <= V11 have RKISP1_CIF_ISP_AE_MEAN_MAX_V10 entries,

s/<= V11/< V12/

> +	 * - versions >= V12 have RKISP1_CIF_ISP_AE_MEAN_MAX_V12 entries.
> +	 */
> +	if (context.configuration.hw.revision < RKISP1_V12)
> +		numCells_ = RKISP1_CIF_ISP_AE_MEAN_MAX_V10;
> +	else
> +		numCells_ = RKISP1_CIF_ISP_AE_MEAN_MAX_V12;
> +
> +	return 0;
> +}
> +
> +/**
> + * \brief Apply a filter on the exposure value to limit the speed of changes
> + * \param[in] currentExposure The target exposure from the AGC algorithm
> + * \return The filtered exposure

\return should go to the end.

> + *
> + * The speed of the filter is adaptive, and will produce the target quicker
> + * during startup, or when the target exposure is within 20% of the most recent
> + * filter output.
> + */
> +utils::Duration Agc::filterExposure(utils::Duration currentExposure)

I would s/currentExposure/exposure/ as it's not "current".

> +{
> +	double speed = 0.2;
> +
> +	/* Adapt instantly if we are in startup phase */

s/phase/phase./ (and similarly through the rest of the file)

> +	if (frameCount_ < kNumStartupFrames)
> +		speed = 1.0;
> +
> +	if (filteredExposure_ == 0s) {
> +		filteredExposure_ = currentExposure;

Do we actually need this ? speed is set to 1.0 for the first frames,
which I expect is the only time when filteredExposure_ can be equal to
0. This is something you can fix (if my analysis is correct) for both
IPU3 and RkISP1 on top of this series.

> +	} else {
> +		/*
> +		 * If we are close to the desired result, go faster to avoid making
> +		 * multiple micro-adjustments.
> +		 * \todo Make this customisable?
> +		 */
> +		if (filteredExposure_ < 1.2 * currentExposure &&
> +		    filteredExposure_ > 0.8 * currentExposure)
> +			speed = sqrt(speed);
> +
> +		filteredExposure_ = speed * currentExposure +
> +				    filteredExposure_ * (1.0 - speed);
> +	}
> +
> +	LOG(RkISP1Agc, Debug) << "After filtering, total_exposure " << filteredExposure_;

s/total_exposure/exposure/ ?

> +
> +	return filteredExposure_;
> +}
> +
> +/**
> + * \brief Estimate the new exposure and gain values
> + * \param[inout] frameContext The shared IPA frame Context
> + * \param[in] yGain The gain calculated on the current brightness level
> + */
> +void Agc::computeExposure(IPAContext &context, double yGain)
> +{
> +	IPASessionConfiguration &configuration = context.configuration;
> +	IPAFrameContext &frameContext = context.frameContext;
> +
> +	/* Get the effective exposure and gain applied on the sensor. */
> +	uint32_t exposure = frameContext.sensor.exposure;
> +	double analogueGain = frameContext.sensor.gain;
> +
> +	utils::Duration minShutterSpeed = configuration.agc.minShutterSpeed;
> +	utils::Duration maxShutterSpeed = std::min(configuration.agc.maxShutterSpeed,
> +						   kMaxShutterSpeed);
> +
> +	double minAnalogueGain = std::max(configuration.agc.minAnalogueGain,
> +					  kMinAnalogueGain);
> +	double maxAnalogueGain = std::min(configuration.agc.maxAnalogueGain,
> +					  kMaxAnalogueGain);
> +
> +	/* Consider within 1% of the target as correctly exposed */
> +	if (std::abs(yGain - 1.0) < 0.01)
> +		LOG(RkISP1Agc, Debug) << "We are well exposed (iqMean = "
> +				      << yGain << ")";

This sounds like it will flood the logs without bringing much value, as
the yGain is printed below too. We used to return early in this case,
I'm not sure we we don't anymore. Also something we should address (if
desired) for IPU3 and RkISP1 on top.

> +
> +	/* extracted from Rpi::Agc::computeTargetExposure */
> +
> +	/* Calculate the shutter time in seconds */
> +	utils::Duration currentShutter = exposure * configuration.sensor.lineDuration;
> +
> +	/*
> +	 * Update the exposure value for the next computation using the values
> +	 * of exposure and gain really used by the sensor.
> +	 */
> +	utils::Duration effectiveExposureValue = currentShutter * analogueGain;
> +
> +	LOG(RkISP1Agc, Debug) << "Actual total exposure " << currentShutter * analogueGain
> +			      << " Shutter speed " << currentShutter
> +			      << " Gain " << analogueGain
> +			      << " Needed ev gain " << yGain;
> +
> +	/*
> +	 * Calculate the current exposure value for the scene as the latest
> +	 * exposure value applied multiplied by the new estimated gain.
> +	 */
> +	utils::Duration currentExposure = effectiveExposureValue * yGain;

s/currentExposure/exposureValue/ here and below.

> +
> +	/* Clamp the exposure value to the min and max authorized */
> +	utils::Duration maxTotalExposure = maxShutterSpeed * maxAnalogueGain;
> +	currentExposure = std::min(currentExposure, maxTotalExposure);
> +	LOG(RkISP1Agc, Debug) << "Target total exposure " << currentExposure
> +			      << ", maximum is " << maxTotalExposure;
> +
> +	/*
> +	 * Divide the exposure value as new exposure and gain values
> +	 * \todo: estimate if we need to desaturate

s/todo:/todo/

> +	 */
> +	utils::Duration exposureValue = filterExposure(currentExposure);
> +
> +	/*
> +	* Push the shutter time up to the maximum first, and only then
> +	* increase the gain.
> +	*/

Incorrect indentation.

> +	utils::Duration shutterTime = std::clamp<utils::Duration>(exposureValue / minAnalogueGain,
> +								  minShutterSpeed, maxShutterSpeed);
> +	double stepGain = std::clamp(exposureValue / shutterTime,
> +				     minAnalogueGain, maxAnalogueGain);
> +	LOG(RkISP1Agc, Debug) << "Divided up shutter and gain are "
> +			      << shutterTime << " and "
> +			      << stepGain;
> +
> +	/* Update the estimated exposure and gain. */
> +	frameContext.agc.exposure = shutterTime / configuration.sensor.lineDuration;
> +	frameContext.agc.gain = stepGain;
> +}
> +
> +/**
> + * \brief Estimate the relative luminance of the frame with a given gain
> + * \param[in] ae The RkISP1 statistics and ISP results
> + * \param[in] gain The gain calculated on the current brightness level

 * \param[in] gain The gain to apply to the frame

> + * \return The relative luminance
> + *
> + * This function estimates the average relative luminance of the frame that
> + * would be output by the sensor if an additional \a gain was applied.
> + *
> + * The estimation is based on the AE statistics for the current frame. Y
> + * averages for all cells are first multiplied by the gain, and then saturated
> + * to approximate the sensor behaviour at high brightness values. The
> + * approximation is quite rough, as it doesn't take into account non-linearities
> + * when approaching saturation.

It's actually worse than that in this case, as saturating after the
conversion to YUV also doesn't take into account the fact that the R, G
and B components contribute differently to the relative luminance. A
different algorithm may be better (not as part of this patch of course).

> + *
> + * The values are normalized to the [0.0, 1.0] range, where 1.0 corresponds to a
> + * theoretical perfect reflector of 100% reference white.
> + *
> + * More detailed information can be found in:
> + * https://en.wikipedia.org/wiki/Relative_luminance
> + */
> +double Agc::estimateLuminance(const rkisp1_cif_isp_ae_stat *ae,
> +			      double gain)
> +{
> +	double ySum = 0.0;
> +
> +	/* Sum the averages, saturated to 255. */
> +	for (unsigned int aeCell = 0; aeCell < numCells_; aeCell++)
> +		ySum += std::min(ae->exp_mean[aeCell] * gain, 255.0);
> +
> +	/* \todo Weight with the AWB gains */
> +
> +	return ySum / numCells_ / 255;
> +}
> +
> +/**
> + * \brief Process RkISP1 statistics, and run AGC operations
> + * \param[in] context The shared IPA context
> + * \param[in] stats The RKIsp1 statistics and ISP results

s/RKIsp1/RkISP1/

> + *
> + * Identify the current image brightness, and use that to estimate the optimal
> + * new exposure and gain for the scene.
> + */
> +void Agc::process(IPAContext &context, const rkisp1_stat_buffer *stats)
> +{
> +	const rkisp1_cif_isp_stat *params = &stats->params;
> +	ASSERT(stats->meas_type & RKISP1_CIF_ISP_STAT_AUTOEXP);
> +
> +	const rkisp1_cif_isp_ae_stat *ae = &params->ae;
> +
> +	/*
> +	 * Estimate the gain needed to achieve a relative luminance target. To
> +	 * account for non-linearity caused by saturation, the value needs to be
> +	 * estimated in an iterative process, as multiplying by a gain will not
> +	 * increase the relative luminance by the same factor if some image
> +	 * regions are saturated.
> +	 */
> +	double yGain = 1.0;
> +	double yTarget = kRelativeLuminanceTarget;
> +
> +	for (unsigned int i = 0; i < 8; i++) {
> +		double yValue = estimateLuminance(ae, yGain);
> +		double extra_gain = std::min(10.0, yTarget / (yValue + .001));
> +
> +		yGain *= extra_gain;
> +		LOG(RkISP1Agc, Debug) << "Y value: " << yValue
> +				      << ", Y target: " << yTarget
> +				      << ", gives gain " << yGain;
> +		if (extra_gain < 1.01)
> +			break;
> +	}
> +
> +	computeExposure(context, yGain);
> +	frameCount_++;

frameCount_ is never reset to 0, is that expected ?

> +}
> +
> +} /* namespace ipa::rkisp1::algorithms */
> +
> +} /* namespace libcamera */
> diff --git a/src/ipa/rkisp1/algorithms/agc.h b/src/ipa/rkisp1/algorithms/agc.h
> new file mode 100644
> index 00000000..8ec172d7
> --- /dev/null
> +++ b/src/ipa/rkisp1/algorithms/agc.h
> @@ -0,0 +1,46 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2021, Ideas On Board
> + *
> + * agc.h - RkISP1 AGC/AEC mean-based control algorithm
> + */
> +
> +#pragma once
> +
> +#include <linux/rkisp1-config.h>
> +
> +#include <libcamera/base/utils.h>
> +
> +#include <libcamera/geometry.h>
> +
> +#include "algorithm.h"
> +
> +namespace libcamera {
> +
> +struct IPACameraSensorInfo;
> +
> +namespace ipa::rkisp1::algorithms {
> +
> +class Agc : public Algorithm
> +{
> +public:
> +	Agc();
> +	~Agc() = default;
> +
> +	int configure(IPAContext &context, const IPACameraSensorInfo &configInfo) override;
> +	void process(IPAContext &context, const rkisp1_stat_buffer *stats) override;
> +
> +private:
> +	void computeExposure(IPAContext &Context, double yGain);
> +	utils::Duration filterExposure(utils::Duration currentExposure);

s/currentExposure/exposure/ here too.

> +	double estimateLuminance(const rkisp1_cif_isp_ae_stat *ae, double gain);
> +
> +	uint64_t frameCount_;
> +
> +	uint32_t numCells_;
> +
> +	utils::Duration filteredExposure_;
> +};
> +
> +} /* namespace ipa::rkisp1::algorithms */
> +} /* namespace libcamera */
> diff --git a/src/ipa/rkisp1/algorithms/meson.build b/src/ipa/rkisp1/algorithms/meson.build
> index 1c6c59cf..a19c1a4f 100644
> --- a/src/ipa/rkisp1/algorithms/meson.build
> +++ b/src/ipa/rkisp1/algorithms/meson.build
> @@ -1,4 +1,5 @@
>  # SPDX-License-Identifier: CC0-1.0
>  
>  rkisp1_ipa_algorithms = files([
> +    'agc.cpp',
>  ])
> diff --git a/src/ipa/rkisp1/ipa_context.cpp b/src/ipa/rkisp1/ipa_context.cpp
> index 6b53dfdf..9cb2a9fd 100644
> --- a/src/ipa/rkisp1/ipa_context.cpp
> +++ b/src/ipa/rkisp1/ipa_context.cpp
> @@ -56,6 +56,21 @@ namespace libcamera::ipa::rkisp1 {
>   */
>  
>  /**
> + * \var IPASessionConfiguration::agc
> + * \brief AGC parameters configuration of the IPA
> + *
> + * \var IPASessionConfiguration::agc.minShutterSpeed
> + * \brief Minimum shutter speed supported with the configured sensor
> + *
> + * \var IPASessionConfiguration::agc.maxShutterSpeed
> + * \brief Maximum shutter speed supported with the configured sensor
> + *
> + * \var IPASessionConfiguration::agc.minAnalogueGain
> + * \brief Minimum analogue gain supported with the configured sensor
> + *
> + * \var IPASessionConfiguration::agc.maxAnalogueGain
> + * \brief Maximum analogue gain supported with the configured sensor
> + *
>   * \var IPASessionConfiguration::hw
>   * \brief RkISP1-specific hardware information
>   *
> @@ -63,4 +78,39 @@ namespace libcamera::ipa::rkisp1 {
>   * \brief Hardware revision of the ISP
>   */
>  
> +/**
> + * \var IPASessionConfiguration::sensor
> + * \brief Sensor-specific configuration of the IPA
> + *
> + * \var IPASessionConfiguration::sensor.lineDuration
> + * \brief Line duration in microseconds
> + */
> +
> +/**
> + * \var IPAFrameContext::agc
> + * \brief Context for the Automatic Gain Control algorithm
> + *
> + * The exposure and gain determined are expected to be applied to the sensor
> + * at the earliest opportunity.
> + *
> + * \var IPAFrameContext::agc.exposure
> + * \brief Exposure time expressed as a number of lines
> + *
> + * \var IPAFrameContext::agc.gain
> + * \brief Analogue gain multiplier
> + *
> + * The gain should be adapted to the sensor specific gain code before applying.
> + */
> +
> +/**
> + * \var IPAFrameContext::sensor
> + * \brief Effective sensor values
> + *
> + * \var IPAFrameContext::sensor.exposure
> + * \brief Exposure time expressed as a number of lines
> + *
> + * \var IPAFrameContext::sensor.gain
> + * \brief Analogue gain multiplier
> + */
> +
>  } /* namespace libcamera::ipa::rkisp1 */
> diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h
> index 9342025b..b94ade0c 100644
> --- a/src/ipa/rkisp1/ipa_context.h
> +++ b/src/ipa/rkisp1/ipa_context.h
> @@ -10,17 +10,39 @@
>  
>  #include <linux/rkisp1-config.h>
>  
> +#include <libcamera/base/utils.h>
> +
>  namespace libcamera {
>  
>  namespace ipa::rkisp1 {
>  
>  struct IPASessionConfiguration {
> +	struct {
> +		utils::Duration minShutterSpeed;
> +		utils::Duration maxShutterSpeed;
> +		double minAnalogueGain;
> +		double maxAnalogueGain;
> +	} agc;
> +
> +	struct {
> +		utils::Duration lineDuration;
> +	} sensor;
> +
>  	struct {
>  		rkisp1_cif_isp_version revision;
>  	} hw;
>  };
>  
>  struct IPAFrameContext {
> +	struct {
> +		uint32_t exposure;
> +		double gain;
> +	} agc;
> +
> +	struct {
> +		uint32_t exposure;
> +		double gain;
> +	} sensor;
>  };
>  
>  struct IPAContext {
> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> index 59676a70..38917fb7 100644
> --- a/src/ipa/rkisp1/rkisp1.cpp
> +++ b/src/ipa/rkisp1/rkisp1.cpp
> @@ -25,6 +25,7 @@
>  
>  #include <libcamera/internal/mapped_framebuffer.h>
>  
> +#include "algorithms/agc.h"
>  #include "algorithms/algorithm.h"
>  #include "libipa/camera_sensor_helper.h"
>  
> @@ -34,6 +35,8 @@ namespace libcamera {
>  
>  LOG_DEFINE_CATEGORY(IPARkISP1)
>  
> +using namespace std::literals::chrono_literals;
> +
>  namespace ipa::rkisp1 {
>  
>  class IPARkISP1 : public IPARkISP1Interface
> @@ -66,16 +69,13 @@ private:
>  
>  	/* Camera sensor controls. */
>  	bool autoExposure_;
> -	uint32_t exposure_;
>  	uint32_t minExposure_;
>  	uint32_t maxExposure_;
> -	uint32_t gain_;
>  	uint32_t minGain_;
>  	uint32_t maxGain_;
>  
>  	/* revision-specific data */
>  	rkisp1_cif_isp_version hwRevision_;
> -	unsigned int hwAeMeanMax_;
>  	unsigned int hwHistBinNMax_;
>  	unsigned int hwGammaOutMaxSamples_;
>  	unsigned int hwHistogramWeightGridsSize_;
> @@ -95,13 +95,11 @@ int IPARkISP1::init(const IPASettings &settings, unsigned int hwRevision)
>  	/* \todo Add support for other revisions */
>  	switch (hwRevision) {
>  	case RKISP1_V10:
> -		hwAeMeanMax_ = RKISP1_CIF_ISP_AE_MEAN_MAX_V10;
>  		hwHistBinNMax_ = RKISP1_CIF_ISP_HIST_BIN_N_MAX_V10;
>  		hwGammaOutMaxSamples_ = RKISP1_CIF_ISP_GAMMA_OUT_MAX_SAMPLES_V10;
>  		hwHistogramWeightGridsSize_ = RKISP1_CIF_ISP_HISTOGRAM_WEIGHT_GRIDS_SIZE_V10;
>  		break;
>  	case RKISP1_V12:
> -		hwAeMeanMax_ = RKISP1_CIF_ISP_AE_MEAN_MAX_V12;
>  		hwHistBinNMax_ = RKISP1_CIF_ISP_HIST_BIN_N_MAX_V12;
>  		hwGammaOutMaxSamples_ = RKISP1_CIF_ISP_GAMMA_OUT_MAX_SAMPLES_V12;
>  		hwHistogramWeightGridsSize_ = RKISP1_CIF_ISP_HISTOGRAM_WEIGHT_GRIDS_SIZE_V12;
> @@ -126,6 +124,9 @@ int IPARkISP1::init(const IPASettings &settings, unsigned int hwRevision)
>  		return -ENODEV;
>  	}
>  
> +	/* Construct our Algorithms */
> +	algorithms_.push_back(std::make_unique<algorithms::Agc>());
> +
>  	return 0;
>  }
>  
> @@ -167,11 +168,9 @@ int IPARkISP1::configure([[maybe_unused]] const IPACameraSensorInfo &info,
>  
>  	minExposure_ = itExp->second.min().get<int32_t>();
>  	maxExposure_ = itExp->second.max().get<int32_t>();
> -	exposure_ = minExposure_;
>  
>  	minGain_ = itGain->second.min().get<int32_t>();
>  	maxGain_ = itGain->second.max().get<int32_t>();
> -	gain_ = minGain_;
>  
>  	LOG(IPARkISP1, Info)
>  		<< "Exposure: " << minExposure_ << "-" << maxExposure_
> @@ -183,6 +182,26 @@ int IPARkISP1::configure([[maybe_unused]] const IPACameraSensorInfo &info,
>  	/* Set the hardware revision for the algorithms. */
>  	context_.configuration.hw.revision = hwRevision_;
>  
> +	context_.configuration.sensor.lineDuration = info.lineLength * 1.0s / info.pixelRate;
> +
> +	/*
> +	 * When the AGC computes the new exposure values for a frame, it needs
> +	 * to know the limits for shutter speed and analogue gain.
> +	 * As it depends on the sensor, update it with the controls.
> +	 *
> +	 * \todo take VBLANK into account for maximum shutter speed
> +	 */
> +	context_.configuration.agc.minShutterSpeed = minExposure_ * context_.configuration.sensor.lineDuration;
> +	context_.configuration.agc.maxShutterSpeed = maxExposure_ * context_.configuration.sensor.lineDuration;
> +	context_.configuration.agc.minAnalogueGain = camHelper_->gain(minGain_);
> +	context_.configuration.agc.maxAnalogueGain = camHelper_->gain(maxGain_);
> +
> +	for (auto const &algo : algorithms_) {
> +		int ret = algo->configure(context_, info);
> +		if (ret)
> +			return ret;
> +	}
> +
>  	return 0;
>  }
>  
> @@ -227,6 +246,11 @@ void IPARkISP1::processEvent(const RkISP1Event &event)
>  			reinterpret_cast<rkisp1_stat_buffer *>(
>  				mappedBuffers_.at(bufferId).planes()[0].data());
>  
> +		context_.frameContext.sensor.exposure =
> +			event.sensorControls.get(V4L2_CID_EXPOSURE).get<int32_t>();
> +		context_.frameContext.sensor.gain =
> +			camHelper_->gain(event.sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get<int32_t>());
> +
>  		updateStatistics(frame, stats);
>  		break;
>  	}
> @@ -271,44 +295,12 @@ void IPARkISP1::queueRequest(unsigned int frame, rkisp1_params_cfg *params,
>  void IPARkISP1::updateStatistics(unsigned int frame,
>  				 const rkisp1_stat_buffer *stats)
>  {
> -	const rkisp1_cif_isp_stat *params = &stats->params;
>  	unsigned int aeState = 0;
>  
> -	if (stats->meas_type & RKISP1_CIF_ISP_STAT_AUTOEXP) {
> -		const rkisp1_cif_isp_ae_stat *ae = &params->ae;
> -
> -		const unsigned int target = 60;
> -
> -		unsigned int value = 0;
> -		unsigned int num = 0;
> -		for (unsigned int i = 0; i < hwAeMeanMax_; i++) {
> -			if (ae->exp_mean[i] <= 15)
> -				continue;
> -
> -			value += ae->exp_mean[i];
> -			num++;
> -		}
> -		value /= num;
> +	for (auto const &algo : algorithms_)
> +		algo->process(context_, stats);
>  
> -		double factor = (double)target / value;
> -
> -		if (frame % 3 == 0) {
> -			double exposure;
> -
> -			exposure = factor * exposure_ * gain_ / minGain_;
> -			exposure_ = std::clamp<uint64_t>((uint64_t)exposure,
> -							 minExposure_,
> -							 maxExposure_);
> -
> -			exposure = exposure / exposure_ * minGain_;
> -			gain_ = std::clamp<uint64_t>((uint64_t)exposure,
> -						     minGain_, maxGain_);
> -
> -			setControls(frame + 1);
> -		}
> -
> -		aeState = fabs(factor - 1.0f) < 0.05f ? 2 : 1;
> -	}
> +	setControls(frame);
>  
>  	metadataReady(frame, aeState);
>  }
> @@ -318,9 +310,12 @@ void IPARkISP1::setControls(unsigned int frame)
>  	RkISP1Action op;
>  	op.op = ActionV4L2Set;
>  
> +	uint32_t exposure = context_.frameContext.agc.exposure;
> +	uint32_t gain = camHelper_->gainCode(context_.frameContext.agc.gain);
> +
>  	ControlList ctrls(ctrls_);
> -	ctrls.set(V4L2_CID_EXPOSURE, static_cast<int32_t>(exposure_));
> -	ctrls.set(V4L2_CID_ANALOGUE_GAIN, static_cast<int32_t>(gain_));
> +	ctrls.set(V4L2_CID_EXPOSURE, static_cast<int32_t>(exposure));
> +	ctrls.set(V4L2_CID_ANALOGUE_GAIN, static_cast<int32_t>(gain));
>  	op.sensorControls = ctrls;
>  
>  	queueFrameAction.emit(frame, op);

Hi Laurent,

On 26/11/2021 23:56, Laurent Pinchart wrote:
> Hi Jean-Michel,
> 
> Thank you for the patch.
> 
> On Fri, Nov 26, 2021 at 05:43:01PM +0100, Jean-Michel Hautbois wrote:
>> Now that we have IPAContext and Algorithm, we can implement a simple AGC
>> based on the IPU3 one. It is very similar, except that there is no
>> histogram used for an inter quantile mean. The RkISP1 is returning a 5x5
>> array (for V10) of luminance means. Estimating the relative luminance is
>> thus a simple mean of all the blocks already calculated by the ISP.
>>
>> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
>>
>> ---
>> v6:
>> 	- Move lineDuration in a configuration.sensor structure in
>> 	  IPAContext
>> 	- Add a documentation for filterExposure()
>> 	- Move shutterTime initialisation
>> v5:
>> 	- use private filteredExposure_ and pass currentExposure as a
>> 	  member variable
>> 	- Drop num and replace it with numCells_
>> 	- Make exposure and gain local variables in IPARkISP1
>> 	- Shorter the lines in processEvent()
>> v4:
>> 	- use #pragma once
>> 	- Return filtered value from the function
>> 	- Store line duration in IPASessionConfiguration
>> 	- Use the hw revision to configure the number of AE cells
>> ---
>>   src/ipa/rkisp1/algorithms/agc.cpp     | 280 ++++++++++++++++++++++++++
>>   src/ipa/rkisp1/algorithms/agc.h       |  46 +++++
>>   src/ipa/rkisp1/algorithms/meson.build |   1 +
>>   src/ipa/rkisp1/ipa_context.cpp        |  50 +++++
>>   src/ipa/rkisp1/ipa_context.h          |  22 ++
>>   src/ipa/rkisp1/rkisp1.cpp             |  83 ++++----
>>   6 files changed, 438 insertions(+), 44 deletions(-)
>>   create mode 100644 src/ipa/rkisp1/algorithms/agc.cpp
>>   create mode 100644 src/ipa/rkisp1/algorithms/agc.h
>>
>> diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp
>> new file mode 100644
>> index 00000000..55d7405a
>> --- /dev/null
>> +++ b/src/ipa/rkisp1/algorithms/agc.cpp
>> @@ -0,0 +1,280 @@
>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
>> +/*
>> + * Copyright (C) 2021, Ideas On Board
>> + *
>> + * agc.cpp - AGC/AEC mean-based control algorithm
>> + */
>> +
>> +#include "agc.h"
>> +
>> +#include <algorithm>
>> +#include <chrono>
>> +#include <cmath>
>> +
>> +#include <libcamera/base/log.h>
>> +
>> +#include <libcamera/ipa/core_ipa_interface.h>
>> +
>> +/**
>> + * \file agc.h
>> + */
>> +
>> +namespace libcamera {
>> +
>> +using namespace std::literals::chrono_literals;
>> +
>> +namespace ipa::rkisp1::algorithms {
>> +
>> +/**
>> + * \class Agc
>> + * \brief A mean-based auto-exposure algorithm
>> + */
>> +
>> +LOG_DEFINE_CATEGORY(RkISP1Agc)
>> +
>> +/* Limits for analogue gain values */
>> +static constexpr double kMinAnalogueGain = 1.0;
>> +static constexpr double kMaxAnalogueGain = 8.0;
>> +
>> +/* \todo Honour the FrameDurationLimits control instead of hardcoding a limit */
>> +static constexpr utils::Duration kMaxShutterSpeed = 60ms;
>> +
>> +/* Number of frames to wait before calculating stats on minimum exposure */
>> +static constexpr uint32_t kNumStartupFrames = 10;
>> +
>> +/*
>> + * Relative luminance target.
>> + *
>> + * It's a number that's chosen so that, when the camera points at a grey
>> + * target, the resulting image brightness is considered right.
>> + */
>> +static constexpr double kRelativeLuminanceTarget = 0.4;
> 
> This is much higher than for the IPU3, why is that ?

I am not yet sure why, but the measures I did on a grey target lead to 
this value for the brightness to be correct. It is on my todo list to 
understand why it is not the same for IPU3 and RkISP1 (is it related to 
the sensor ? the ISP ? something else ? not sure yet)

> 
>> +
>> +Agc::Agc()
>> +	: frameCount_(0), filteredExposure_(0s)
>> +{
>> +}
>> +
>> +/**
>> + * \brief Configure the AGC given a configInfo
>> + * \param[in] context The shared IPA context
>> + * \param[in] configInfo The IPA configuration data
>> + *
>> + * \return 0
>> + */
>> +int Agc::configure(IPAContext &context,
>> +		   [[maybe_unused]] const IPACameraSensorInfo &configInfo)
>> +{
>> +	/* Configure the default exposure and gain. */
>> +	context.frameContext.agc.gain = std::max(context.configuration.agc.minAnalogueGain, kMinAnalogueGain);
>> +	context.frameContext.agc.exposure = 10ms / context.configuration.sensor.lineDuration;
>> +
>> +	/*
>> +	 * According to the RkISP1 documentation:
>> +	 * - versions <= V11 have RKISP1_CIF_ISP_AE_MEAN_MAX_V10 entries,
> 
> s/<= V11/< V12/

OK, but this is a strict copy/paste from the RkISP1 include file ;-).

> 
>> +	 * - versions >= V12 have RKISP1_CIF_ISP_AE_MEAN_MAX_V12 entries.
>> +	 */
>> +	if (context.configuration.hw.revision < RKISP1_V12)
>> +		numCells_ = RKISP1_CIF_ISP_AE_MEAN_MAX_V10;
>> +	else
>> +		numCells_ = RKISP1_CIF_ISP_AE_MEAN_MAX_V12;
>> +
>> +	return 0;
>> +}
>> +
>> +/**
>> + * \brief Apply a filter on the exposure value to limit the speed of changes
>> + * \param[in] currentExposure The target exposure from the AGC algorithm
>> + * \return The filtered exposure
> 
> \return should go to the end.
> 
>> + *
>> + * The speed of the filter is adaptive, and will produce the target quicker
>> + * during startup, or when the target exposure is within 20% of the most recent
>> + * filter output.
>> + */
>> +utils::Duration Agc::filterExposure(utils::Duration currentExposure)
> 
> I would s/currentExposure/exposure/ as it's not "current".

What about expectedExposure ?

> 
>> +{
>> +	double speed = 0.2;
>> +
>> +	/* Adapt instantly if we are in startup phase */
> 
> s/phase/phase./ (and similarly through the rest of the file)
> 
>> +	if (frameCount_ < kNumStartupFrames)
>> +		speed = 1.0;
>> +
>> +	if (filteredExposure_ == 0s) {
>> +		filteredExposure_ = currentExposure;
> 
> Do we actually need this ? speed is set to 1.0 for the first frames,
> which I expect is the only time when filteredExposure_ can be equal to
> 0. This is something you can fix (if my analysis is correct) for both
> IPU3 and RkISP1 on top of this series.

Indeed, thanks.

> 
>> +	} else {
>> +		/*
>> +		 * If we are close to the desired result, go faster to avoid making
>> +		 * multiple micro-adjustments.
>> +		 * \todo Make this customisable?
>> +		 */
>> +		if (filteredExposure_ < 1.2 * currentExposure &&
>> +		    filteredExposure_ > 0.8 * currentExposure)
>> +			speed = sqrt(speed);
>> +
>> +		filteredExposure_ = speed * currentExposure +
>> +				    filteredExposure_ * (1.0 - speed);
>> +	}
>> +
>> +	LOG(RkISP1Agc, Debug) << "After filtering, total_exposure " << filteredExposure_;
> 
> s/total_exposure/exposure/ ?
> 
>> +
>> +	return filteredExposure_;
>> +}
>> +
>> +/**
>> + * \brief Estimate the new exposure and gain values
>> + * \param[inout] frameContext The shared IPA frame Context
>> + * \param[in] yGain The gain calculated on the current brightness level
>> + */
>> +void Agc::computeExposure(IPAContext &context, double yGain)
>> +{
>> +	IPASessionConfiguration &configuration = context.configuration;
>> +	IPAFrameContext &frameContext = context.frameContext;
>> +
>> +	/* Get the effective exposure and gain applied on the sensor. */
>> +	uint32_t exposure = frameContext.sensor.exposure;
>> +	double analogueGain = frameContext.sensor.gain;
>> +
>> +	utils::Duration minShutterSpeed = configuration.agc.minShutterSpeed;
>> +	utils::Duration maxShutterSpeed = std::min(configuration.agc.maxShutterSpeed,
>> +						   kMaxShutterSpeed);
>> +
>> +	double minAnalogueGain = std::max(configuration.agc.minAnalogueGain,
>> +					  kMinAnalogueGain);
>> +	double maxAnalogueGain = std::min(configuration.agc.maxAnalogueGain,
>> +					  kMaxAnalogueGain);
>> +
>> +	/* Consider within 1% of the target as correctly exposed */
>> +	if (std::abs(yGain - 1.0) < 0.01)
>> +		LOG(RkISP1Agc, Debug) << "We are well exposed (iqMean = "
>> +				      << yGain << ")";
> 
> This sounds like it will flood the logs without bringing much value, as
> the yGain is printed below too. We used to return early in this case,
> I'm not sure we we don't anymore. Also something we should address (if
> desired) for IPU3 and RkISP1 on top.

You are right.

> 
>> +
>> +	/* extracted from Rpi::Agc::computeTargetExposure */
>> +
>> +	/* Calculate the shutter time in seconds */
>> +	utils::Duration currentShutter = exposure * configuration.sensor.lineDuration;
>> +
>> +	/*
>> +	 * Update the exposure value for the next computation using the values
>> +	 * of exposure and gain really used by the sensor.
>> +	 */
>> +	utils::Duration effectiveExposureValue = currentShutter * analogueGain;
>> +
>> +	LOG(RkISP1Agc, Debug) << "Actual total exposure " << currentShutter * analogueGain
>> +			      << " Shutter speed " << currentShutter
>> +			      << " Gain " << analogueGain
>> +			      << " Needed ev gain " << yGain;
>> +
>> +	/*
>> +	 * Calculate the current exposure value for the scene as the latest
>> +	 * exposure value applied multiplied by the new estimated gain.
>> +	 */
>> +	utils::Duration currentExposure = effectiveExposureValue * yGain;
> 
> s/currentExposure/exposureValue/ here and below.
> 
>> +
>> +	/* Clamp the exposure value to the min and max authorized */
>> +	utils::Duration maxTotalExposure = maxShutterSpeed * maxAnalogueGain;
>> +	currentExposure = std::min(currentExposure, maxTotalExposure);
>> +	LOG(RkISP1Agc, Debug) << "Target total exposure " << currentExposure
>> +			      << ", maximum is " << maxTotalExposure;
>> +
>> +	/*
>> +	 * Divide the exposure value as new exposure and gain values
>> +	 * \todo: estimate if we need to desaturate
> 
> s/todo:/todo/
> 
>> +	 */
>> +	utils::Duration exposureValue = filterExposure(currentExposure);
>> +
>> +	/*
>> +	* Push the shutter time up to the maximum first, and only then
>> +	* increase the gain.
>> +	*/
> 
> Incorrect indentation.
> 
>> +	utils::Duration shutterTime = std::clamp<utils::Duration>(exposureValue / minAnalogueGain,
>> +								  minShutterSpeed, maxShutterSpeed);
>> +	double stepGain = std::clamp(exposureValue / shutterTime,
>> +				     minAnalogueGain, maxAnalogueGain);
>> +	LOG(RkISP1Agc, Debug) << "Divided up shutter and gain are "
>> +			      << shutterTime << " and "
>> +			      << stepGain;
>> +
>> +	/* Update the estimated exposure and gain. */
>> +	frameContext.agc.exposure = shutterTime / configuration.sensor.lineDuration;
>> +	frameContext.agc.gain = stepGain;
>> +}
>> +
>> +/**
>> + * \brief Estimate the relative luminance of the frame with a given gain
>> + * \param[in] ae The RkISP1 statistics and ISP results
>> + * \param[in] gain The gain calculated on the current brightness level
> 
>   * \param[in] gain The gain to apply to the frame
> 
>> + * \return The relative luminance
>> + *
>> + * This function estimates the average relative luminance of the frame that
>> + * would be output by the sensor if an additional \a gain was applied.
>> + *
>> + * The estimation is based on the AE statistics for the current frame. Y
>> + * averages for all cells are first multiplied by the gain, and then saturated
>> + * to approximate the sensor behaviour at high brightness values. The
>> + * approximation is quite rough, as it doesn't take into account non-linearities
>> + * when approaching saturation.
> 
> It's actually worse than that in this case, as saturating after the
> conversion to YUV also doesn't take into account the fact that the R, G
> and B components contribute differently to the relative luminance. A
> different algorithm may be better (not as part of this patch of course).
> 
>> + *
>> + * The values are normalized to the [0.0, 1.0] range, where 1.0 corresponds to a
>> + * theoretical perfect reflector of 100% reference white.
>> + *
>> + * More detailed information can be found in:
>> + * https://en.wikipedia.org/wiki/Relative_luminance
>> + */
>> +double Agc::estimateLuminance(const rkisp1_cif_isp_ae_stat *ae,
>> +			      double gain)
>> +{
>> +	double ySum = 0.0;
>> +
>> +	/* Sum the averages, saturated to 255. */
>> +	for (unsigned int aeCell = 0; aeCell < numCells_; aeCell++)
>> +		ySum += std::min(ae->exp_mean[aeCell] * gain, 255.0);
>> +
>> +	/* \todo Weight with the AWB gains */

This could be why we don't have the same kRelativeLuminanceTarget BTW ;-).

>> +
>> +	return ySum / numCells_ / 255;
>> +}
>> +
>> +/**
>> + * \brief Process RkISP1 statistics, and run AGC operations
>> + * \param[in] context The shared IPA context
>> + * \param[in] stats The RKIsp1 statistics and ISP results
> 
> s/RKIsp1/RkISP1/
> 
>> + *
>> + * Identify the current image brightness, and use that to estimate the optimal
>> + * new exposure and gain for the scene.
>> + */
>> +void Agc::process(IPAContext &context, const rkisp1_stat_buffer *stats)
>> +{
>> +	const rkisp1_cif_isp_stat *params = &stats->params;
>> +	ASSERT(stats->meas_type & RKISP1_CIF_ISP_STAT_AUTOEXP);
>> +
>> +	const rkisp1_cif_isp_ae_stat *ae = &params->ae;
>> +
>> +	/*
>> +	 * Estimate the gain needed to achieve a relative luminance target. To
>> +	 * account for non-linearity caused by saturation, the value needs to be
>> +	 * estimated in an iterative process, as multiplying by a gain will not
>> +	 * increase the relative luminance by the same factor if some image
>> +	 * regions are saturated.
>> +	 */
>> +	double yGain = 1.0;
>> +	double yTarget = kRelativeLuminanceTarget;
>> +
>> +	for (unsigned int i = 0; i < 8; i++) {
>> +		double yValue = estimateLuminance(ae, yGain);
>> +		double extra_gain = std::min(10.0, yTarget / (yValue + .001));
>> +
>> +		yGain *= extra_gain;
>> +		LOG(RkISP1Agc, Debug) << "Y value: " << yValue
>> +				      << ", Y target: " << yTarget
>> +				      << ", gives gain " << yGain;
>> +		if (extra_gain < 1.01)
>> +			break;
>> +	}
>> +
>> +	computeExposure(context, yGain);
>> +	frameCount_++;
> 
> frameCount_ is never reset to 0, is that expected ?

No :-(

> 
>> +}
>> +
>> +} /* namespace ipa::rkisp1::algorithms */
>> +
>> +} /* namespace libcamera */
>> diff --git a/src/ipa/rkisp1/algorithms/agc.h b/src/ipa/rkisp1/algorithms/agc.h
>> new file mode 100644
>> index 00000000..8ec172d7
>> --- /dev/null
>> +++ b/src/ipa/rkisp1/algorithms/agc.h
>> @@ -0,0 +1,46 @@
>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
>> +/*
>> + * Copyright (C) 2021, Ideas On Board
>> + *
>> + * agc.h - RkISP1 AGC/AEC mean-based control algorithm
>> + */
>> +
>> +#pragma once
>> +
>> +#include <linux/rkisp1-config.h>
>> +
>> +#include <libcamera/base/utils.h>
>> +
>> +#include <libcamera/geometry.h>
>> +
>> +#include "algorithm.h"
>> +
>> +namespace libcamera {
>> +
>> +struct IPACameraSensorInfo;
>> +
>> +namespace ipa::rkisp1::algorithms {
>> +
>> +class Agc : public Algorithm
>> +{
>> +public:
>> +	Agc();
>> +	~Agc() = default;
>> +
>> +	int configure(IPAContext &context, const IPACameraSensorInfo &configInfo) override;
>> +	void process(IPAContext &context, const rkisp1_stat_buffer *stats) override;
>> +
>> +private:
>> +	void computeExposure(IPAContext &Context, double yGain);
>> +	utils::Duration filterExposure(utils::Duration currentExposure);
> 
> s/currentExposure/exposure/ here too.
> 
>> +	double estimateLuminance(const rkisp1_cif_isp_ae_stat *ae, double gain);
>> +
>> +	uint64_t frameCount_;
>> +
>> +	uint32_t numCells_;
>> +
>> +	utils::Duration filteredExposure_;
>> +};
>> +
>> +} /* namespace ipa::rkisp1::algorithms */
>> +} /* namespace libcamera */
>> diff --git a/src/ipa/rkisp1/algorithms/meson.build b/src/ipa/rkisp1/algorithms/meson.build
>> index 1c6c59cf..a19c1a4f 100644
>> --- a/src/ipa/rkisp1/algorithms/meson.build
>> +++ b/src/ipa/rkisp1/algorithms/meson.build
>> @@ -1,4 +1,5 @@
>>   # SPDX-License-Identifier: CC0-1.0
>>   
>>   rkisp1_ipa_algorithms = files([
>> +    'agc.cpp',
>>   ])
>> diff --git a/src/ipa/rkisp1/ipa_context.cpp b/src/ipa/rkisp1/ipa_context.cpp
>> index 6b53dfdf..9cb2a9fd 100644
>> --- a/src/ipa/rkisp1/ipa_context.cpp
>> +++ b/src/ipa/rkisp1/ipa_context.cpp
>> @@ -56,6 +56,21 @@ namespace libcamera::ipa::rkisp1 {
>>    */
>>   
>>   /**
>> + * \var IPASessionConfiguration::agc
>> + * \brief AGC parameters configuration of the IPA
>> + *
>> + * \var IPASessionConfiguration::agc.minShutterSpeed
>> + * \brief Minimum shutter speed supported with the configured sensor
>> + *
>> + * \var IPASessionConfiguration::agc.maxShutterSpeed
>> + * \brief Maximum shutter speed supported with the configured sensor
>> + *
>> + * \var IPASessionConfiguration::agc.minAnalogueGain
>> + * \brief Minimum analogue gain supported with the configured sensor
>> + *
>> + * \var IPASessionConfiguration::agc.maxAnalogueGain
>> + * \brief Maximum analogue gain supported with the configured sensor
>> + *
>>    * \var IPASessionConfiguration::hw
>>    * \brief RkISP1-specific hardware information
>>    *
>> @@ -63,4 +78,39 @@ namespace libcamera::ipa::rkisp1 {
>>    * \brief Hardware revision of the ISP
>>    */
>>   
>> +/**
>> + * \var IPASessionConfiguration::sensor
>> + * \brief Sensor-specific configuration of the IPA
>> + *
>> + * \var IPASessionConfiguration::sensor.lineDuration
>> + * \brief Line duration in microseconds
>> + */
>> +
>> +/**
>> + * \var IPAFrameContext::agc
>> + * \brief Context for the Automatic Gain Control algorithm
>> + *
>> + * The exposure and gain determined are expected to be applied to the sensor
>> + * at the earliest opportunity.
>> + *
>> + * \var IPAFrameContext::agc.exposure
>> + * \brief Exposure time expressed as a number of lines
>> + *
>> + * \var IPAFrameContext::agc.gain
>> + * \brief Analogue gain multiplier
>> + *
>> + * The gain should be adapted to the sensor specific gain code before applying.
>> + */
>> +
>> +/**
>> + * \var IPAFrameContext::sensor
>> + * \brief Effective sensor values
>> + *
>> + * \var IPAFrameContext::sensor.exposure
>> + * \brief Exposure time expressed as a number of lines
>> + *
>> + * \var IPAFrameContext::sensor.gain
>> + * \brief Analogue gain multiplier
>> + */
>> +
>>   } /* namespace libcamera::ipa::rkisp1 */
>> diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h
>> index 9342025b..b94ade0c 100644
>> --- a/src/ipa/rkisp1/ipa_context.h
>> +++ b/src/ipa/rkisp1/ipa_context.h
>> @@ -10,17 +10,39 @@
>>   
>>   #include <linux/rkisp1-config.h>
>>   
>> +#include <libcamera/base/utils.h>
>> +
>>   namespace libcamera {
>>   
>>   namespace ipa::rkisp1 {
>>   
>>   struct IPASessionConfiguration {
>> +	struct {
>> +		utils::Duration minShutterSpeed;
>> +		utils::Duration maxShutterSpeed;
>> +		double minAnalogueGain;
>> +		double maxAnalogueGain;
>> +	} agc;
>> +
>> +	struct {
>> +		utils::Duration lineDuration;
>> +	} sensor;
>> +
>>   	struct {
>>   		rkisp1_cif_isp_version revision;
>>   	} hw;
>>   };
>>   
>>   struct IPAFrameContext {
>> +	struct {
>> +		uint32_t exposure;
>> +		double gain;
>> +	} agc;
>> +
>> +	struct {
>> +		uint32_t exposure;
>> +		double gain;
>> +	} sensor;
>>   };
>>   
>>   struct IPAContext {
>> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
>> index 59676a70..38917fb7 100644
>> --- a/src/ipa/rkisp1/rkisp1.cpp
>> +++ b/src/ipa/rkisp1/rkisp1.cpp
>> @@ -25,6 +25,7 @@
>>   
>>   #include <libcamera/internal/mapped_framebuffer.h>
>>   
>> +#include "algorithms/agc.h"
>>   #include "algorithms/algorithm.h"
>>   #include "libipa/camera_sensor_helper.h"
>>   
>> @@ -34,6 +35,8 @@ namespace libcamera {
>>   
>>   LOG_DEFINE_CATEGORY(IPARkISP1)
>>   
>> +using namespace std::literals::chrono_literals;
>> +
>>   namespace ipa::rkisp1 {
>>   
>>   class IPARkISP1 : public IPARkISP1Interface
>> @@ -66,16 +69,13 @@ private:
>>   
>>   	/* Camera sensor controls. */
>>   	bool autoExposure_;
>> -	uint32_t exposure_;
>>   	uint32_t minExposure_;
>>   	uint32_t maxExposure_;
>> -	uint32_t gain_;
>>   	uint32_t minGain_;
>>   	uint32_t maxGain_;
>>   
>>   	/* revision-specific data */
>>   	rkisp1_cif_isp_version hwRevision_;
>> -	unsigned int hwAeMeanMax_;
>>   	unsigned int hwHistBinNMax_;
>>   	unsigned int hwGammaOutMaxSamples_;
>>   	unsigned int hwHistogramWeightGridsSize_;
>> @@ -95,13 +95,11 @@ int IPARkISP1::init(const IPASettings &settings, unsigned int hwRevision)
>>   	/* \todo Add support for other revisions */
>>   	switch (hwRevision) {
>>   	case RKISP1_V10:
>> -		hwAeMeanMax_ = RKISP1_CIF_ISP_AE_MEAN_MAX_V10;
>>   		hwHistBinNMax_ = RKISP1_CIF_ISP_HIST_BIN_N_MAX_V10;
>>   		hwGammaOutMaxSamples_ = RKISP1_CIF_ISP_GAMMA_OUT_MAX_SAMPLES_V10;
>>   		hwHistogramWeightGridsSize_ = RKISP1_CIF_ISP_HISTOGRAM_WEIGHT_GRIDS_SIZE_V10;
>>   		break;
>>   	case RKISP1_V12:
>> -		hwAeMeanMax_ = RKISP1_CIF_ISP_AE_MEAN_MAX_V12;
>>   		hwHistBinNMax_ = RKISP1_CIF_ISP_HIST_BIN_N_MAX_V12;
>>   		hwGammaOutMaxSamples_ = RKISP1_CIF_ISP_GAMMA_OUT_MAX_SAMPLES_V12;
>>   		hwHistogramWeightGridsSize_ = RKISP1_CIF_ISP_HISTOGRAM_WEIGHT_GRIDS_SIZE_V12;
>> @@ -126,6 +124,9 @@ int IPARkISP1::init(const IPASettings &settings, unsigned int hwRevision)
>>   		return -ENODEV;
>>   	}
>>   
>> +	/* Construct our Algorithms */
>> +	algorithms_.push_back(std::make_unique<algorithms::Agc>());
>> +
>>   	return 0;
>>   }
>>   
>> @@ -167,11 +168,9 @@ int IPARkISP1::configure([[maybe_unused]] const IPACameraSensorInfo &info,
>>   
>>   	minExposure_ = itExp->second.min().get<int32_t>();
>>   	maxExposure_ = itExp->second.max().get<int32_t>();
>> -	exposure_ = minExposure_;
>>   
>>   	minGain_ = itGain->second.min().get<int32_t>();
>>   	maxGain_ = itGain->second.max().get<int32_t>();
>> -	gain_ = minGain_;
>>   
>>   	LOG(IPARkISP1, Info)
>>   		<< "Exposure: " << minExposure_ << "-" << maxExposure_
>> @@ -183,6 +182,26 @@ int IPARkISP1::configure([[maybe_unused]] const IPACameraSensorInfo &info,
>>   	/* Set the hardware revision for the algorithms. */
>>   	context_.configuration.hw.revision = hwRevision_;
>>   
>> +	context_.configuration.sensor.lineDuration = info.lineLength * 1.0s / info.pixelRate;
>> +
>> +	/*
>> +	 * When the AGC computes the new exposure values for a frame, it needs
>> +	 * to know the limits for shutter speed and analogue gain.
>> +	 * As it depends on the sensor, update it with the controls.
>> +	 *
>> +	 * \todo take VBLANK into account for maximum shutter speed
>> +	 */
>> +	context_.configuration.agc.minShutterSpeed = minExposure_ * context_.configuration.sensor.lineDuration;
>> +	context_.configuration.agc.maxShutterSpeed = maxExposure_ * context_.configuration.sensor.lineDuration;
>> +	context_.configuration.agc.minAnalogueGain = camHelper_->gain(minGain_);
>> +	context_.configuration.agc.maxAnalogueGain = camHelper_->gain(maxGain_);
>> +
>> +	for (auto const &algo : algorithms_) {
>> +		int ret = algo->configure(context_, info);
>> +		if (ret)
>> +			return ret;
>> +	}
>> +
>>   	return 0;
>>   }
>>   
>> @@ -227,6 +246,11 @@ void IPARkISP1::processEvent(const RkISP1Event &event)
>>   			reinterpret_cast<rkisp1_stat_buffer *>(
>>   				mappedBuffers_.at(bufferId).planes()[0].data());
>>   
>> +		context_.frameContext.sensor.exposure =
>> +			event.sensorControls.get(V4L2_CID_EXPOSURE).get<int32_t>();
>> +		context_.frameContext.sensor.gain =
>> +			camHelper_->gain(event.sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get<int32_t>());
>> +
>>   		updateStatistics(frame, stats);
>>   		break;
>>   	}
>> @@ -271,44 +295,12 @@ void IPARkISP1::queueRequest(unsigned int frame, rkisp1_params_cfg *params,
>>   void IPARkISP1::updateStatistics(unsigned int frame,
>>   				 const rkisp1_stat_buffer *stats)
>>   {
>> -	const rkisp1_cif_isp_stat *params = &stats->params;
>>   	unsigned int aeState = 0;
>>   
>> -	if (stats->meas_type & RKISP1_CIF_ISP_STAT_AUTOEXP) {
>> -		const rkisp1_cif_isp_ae_stat *ae = &params->ae;
>> -
>> -		const unsigned int target = 60;
>> -
>> -		unsigned int value = 0;
>> -		unsigned int num = 0;
>> -		for (unsigned int i = 0; i < hwAeMeanMax_; i++) {
>> -			if (ae->exp_mean[i] <= 15)
>> -				continue;
>> -
>> -			value += ae->exp_mean[i];
>> -			num++;
>> -		}
>> -		value /= num;
>> +	for (auto const &algo : algorithms_)
>> +		algo->process(context_, stats);
>>   
>> -		double factor = (double)target / value;
>> -
>> -		if (frame % 3 == 0) {
>> -			double exposure;
>> -
>> -			exposure = factor * exposure_ * gain_ / minGain_;
>> -			exposure_ = std::clamp<uint64_t>((uint64_t)exposure,
>> -							 minExposure_,
>> -							 maxExposure_);
>> -
>> -			exposure = exposure / exposure_ * minGain_;
>> -			gain_ = std::clamp<uint64_t>((uint64_t)exposure,
>> -						     minGain_, maxGain_);
>> -
>> -			setControls(frame + 1);
>> -		}
>> -
>> -		aeState = fabs(factor - 1.0f) < 0.05f ? 2 : 1;
>> -	}
>> +	setControls(frame);
>>   
>>   	metadataReady(frame, aeState);
>>   }
>> @@ -318,9 +310,12 @@ void IPARkISP1::setControls(unsigned int frame)
>>   	RkISP1Action op;
>>   	op.op = ActionV4L2Set;
>>   
>> +	uint32_t exposure = context_.frameContext.agc.exposure;
>> +	uint32_t gain = camHelper_->gainCode(context_.frameContext.agc.gain);
>> +
>>   	ControlList ctrls(ctrls_);
>> -	ctrls.set(V4L2_CID_EXPOSURE, static_cast<int32_t>(exposure_));
>> -	ctrls.set(V4L2_CID_ANALOGUE_GAIN, static_cast<int32_t>(gain_));
>> +	ctrls.set(V4L2_CID_EXPOSURE, static_cast<int32_t>(exposure));
>> +	ctrls.set(V4L2_CID_ANALOGUE_GAIN, static_cast<int32_t>(gain));
>>   	op.sensorControls = ctrls;
>>   
>>   	queueFrameAction.emit(frame, op);
>

Hi Jean-Michel,

On Mon, Nov 29, 2021 at 02:04:30PM +0100, Jean-Michel Hautbois wrote:
> On 26/11/2021 23:56, Laurent Pinchart wrote:
> > On Fri, Nov 26, 2021 at 05:43:01PM +0100, Jean-Michel Hautbois wrote:
> >> Now that we have IPAContext and Algorithm, we can implement a simple AGC
> >> based on the IPU3 one. It is very similar, except that there is no
> >> histogram used for an inter quantile mean. The RkISP1 is returning a 5x5
> >> array (for V10) of luminance means. Estimating the relative luminance is
> >> thus a simple mean of all the blocks already calculated by the ISP.
> >>
> >> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
> >>
> >> ---
> >> v6:
> >> 	- Move lineDuration in a configuration.sensor structure in
> >> 	  IPAContext
> >> 	- Add a documentation for filterExposure()
> >> 	- Move shutterTime initialisation
> >> v5:
> >> 	- use private filteredExposure_ and pass currentExposure as a
> >> 	  member variable
> >> 	- Drop num and replace it with numCells_
> >> 	- Make exposure and gain local variables in IPARkISP1
> >> 	- Shorter the lines in processEvent()
> >> v4:
> >> 	- use #pragma once
> >> 	- Return filtered value from the function
> >> 	- Store line duration in IPASessionConfiguration
> >> 	- Use the hw revision to configure the number of AE cells
> >> ---
> >>   src/ipa/rkisp1/algorithms/agc.cpp     | 280 ++++++++++++++++++++++++++
> >>   src/ipa/rkisp1/algorithms/agc.h       |  46 +++++
> >>   src/ipa/rkisp1/algorithms/meson.build |   1 +
> >>   src/ipa/rkisp1/ipa_context.cpp        |  50 +++++
> >>   src/ipa/rkisp1/ipa_context.h          |  22 ++
> >>   src/ipa/rkisp1/rkisp1.cpp             |  83 ++++----
> >>   6 files changed, 438 insertions(+), 44 deletions(-)
> >>   create mode 100644 src/ipa/rkisp1/algorithms/agc.cpp
> >>   create mode 100644 src/ipa/rkisp1/algorithms/agc.h
> >>
> >> diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp
> >> new file mode 100644
> >> index 00000000..55d7405a
> >> --- /dev/null
> >> +++ b/src/ipa/rkisp1/algorithms/agc.cpp
> >> @@ -0,0 +1,280 @@
> >> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> >> +/*
> >> + * Copyright (C) 2021, Ideas On Board
> >> + *
> >> + * agc.cpp - AGC/AEC mean-based control algorithm
> >> + */
> >> +
> >> +#include "agc.h"
> >> +
> >> +#include <algorithm>
> >> +#include <chrono>
> >> +#include <cmath>
> >> +
> >> +#include <libcamera/base/log.h>
> >> +
> >> +#include <libcamera/ipa/core_ipa_interface.h>
> >> +
> >> +/**
> >> + * \file agc.h
> >> + */
> >> +
> >> +namespace libcamera {
> >> +
> >> +using namespace std::literals::chrono_literals;
> >> +
> >> +namespace ipa::rkisp1::algorithms {
> >> +
> >> +/**
> >> + * \class Agc
> >> + * \brief A mean-based auto-exposure algorithm
> >> + */
> >> +
> >> +LOG_DEFINE_CATEGORY(RkISP1Agc)
> >> +
> >> +/* Limits for analogue gain values */
> >> +static constexpr double kMinAnalogueGain = 1.0;
> >> +static constexpr double kMaxAnalogueGain = 8.0;
> >> +
> >> +/* \todo Honour the FrameDurationLimits control instead of hardcoding a limit */
> >> +static constexpr utils::Duration kMaxShutterSpeed = 60ms;
> >> +
> >> +/* Number of frames to wait before calculating stats on minimum exposure */
> >> +static constexpr uint32_t kNumStartupFrames = 10;
> >> +
> >> +/*
> >> + * Relative luminance target.
> >> + *
> >> + * It's a number that's chosen so that, when the camera points at a grey
> >> + * target, the resulting image brightness is considered right.
> >> + */
> >> +static constexpr double kRelativeLuminanceTarget = 0.4;
> > 
> > This is much higher than for the IPU3, why is that ?
> 
> I am not yet sure why, but the measures I did on a grey target lead to 
> this value for the brightness to be correct. It is on my todo list to 
> understand why it is not the same for IPU3 and RkISP1 (is it related to 
> the sensor ? the ISP ? something else ? not sure yet)

Can you add a \todo comment ? I think it's also important enough to
record a task in the bug tracker.

> >> +
> >> +Agc::Agc()
> >> +	: frameCount_(0), filteredExposure_(0s)
> >> +{
> >> +}
> >> +
> >> +/**
> >> + * \brief Configure the AGC given a configInfo
> >> + * \param[in] context The shared IPA context
> >> + * \param[in] configInfo The IPA configuration data
> >> + *
> >> + * \return 0
> >> + */
> >> +int Agc::configure(IPAContext &context,
> >> +		   [[maybe_unused]] const IPACameraSensorInfo &configInfo)
> >> +{
> >> +	/* Configure the default exposure and gain. */
> >> +	context.frameContext.agc.gain = std::max(context.configuration.agc.minAnalogueGain, kMinAnalogueGain);
> >> +	context.frameContext.agc.exposure = 10ms / context.configuration.sensor.lineDuration;
> >> +
> >> +	/*
> >> +	 * According to the RkISP1 documentation:
> >> +	 * - versions <= V11 have RKISP1_CIF_ISP_AE_MEAN_MAX_V10 entries,
> > 
> > s/<= V11/< V12/
> 
> OK, but this is a strict copy/paste from the RkISP1 include file ;-).
> 
> >> +	 * - versions >= V12 have RKISP1_CIF_ISP_AE_MEAN_MAX_V12 entries.
> >> +	 */
> >> +	if (context.configuration.hw.revision < RKISP1_V12)
> >> +		numCells_ = RKISP1_CIF_ISP_AE_MEAN_MAX_V10;
> >> +	else
> >> +		numCells_ = RKISP1_CIF_ISP_AE_MEAN_MAX_V12;
> >> +
> >> +	return 0;
> >> +}
> >> +
> >> +/**
> >> + * \brief Apply a filter on the exposure value to limit the speed of changes
> >> + * \param[in] currentExposure The target exposure from the AGC algorithm
> >> + * \return The filtered exposure
> > 
> > \return should go to the end.
> > 
> >> + *
> >> + * The speed of the filter is adaptive, and will produce the target quicker
> >> + * during startup, or when the target exposure is within 20% of the most recent
> >> + * filter output.
> >> + */
> >> +utils::Duration Agc::filterExposure(utils::Duration currentExposure)
> > 
> > I would s/currentExposure/exposure/ as it's not "current".
> 
> What about expectedExposure ?

From the point of view of the filter, it's really just an input value.
You could name it inputExposure, I'm not sure it would make the code
easier to read.

> >> +{
> >> +	double speed = 0.2;
> >> +
> >> +	/* Adapt instantly if we are in startup phase */
> > 
> > s/phase/phase./ (and similarly through the rest of the file)
> > 
> >> +	if (frameCount_ < kNumStartupFrames)
> >> +		speed = 1.0;
> >> +
> >> +	if (filteredExposure_ == 0s) {
> >> +		filteredExposure_ = currentExposure;
> > 
> > Do we actually need this ? speed is set to 1.0 for the first frames,
> > which I expect is the only time when filteredExposure_ can be equal to
> > 0. This is something you can fix (if my analysis is correct) for both
> > IPU3 and RkISP1 on top of this series.
> 
> Indeed, thanks.
> 
> >> +	} else {
> >> +		/*
> >> +		 * If we are close to the desired result, go faster to avoid making
> >> +		 * multiple micro-adjustments.
> >> +		 * \todo Make this customisable?
> >> +		 */
> >> +		if (filteredExposure_ < 1.2 * currentExposure &&
> >> +		    filteredExposure_ > 0.8 * currentExposure)
> >> +			speed = sqrt(speed);
> >> +
> >> +		filteredExposure_ = speed * currentExposure +
> >> +				    filteredExposure_ * (1.0 - speed);
> >> +	}
> >> +
> >> +	LOG(RkISP1Agc, Debug) << "After filtering, total_exposure " << filteredExposure_;
> > 
> > s/total_exposure/exposure/ ?
> > 
> >> +
> >> +	return filteredExposure_;
> >> +}
> >> +
> >> +/**
> >> + * \brief Estimate the new exposure and gain values
> >> + * \param[inout] frameContext The shared IPA frame Context
> >> + * \param[in] yGain The gain calculated on the current brightness level
> >> + */
> >> +void Agc::computeExposure(IPAContext &context, double yGain)
> >> +{
> >> +	IPASessionConfiguration &configuration = context.configuration;
> >> +	IPAFrameContext &frameContext = context.frameContext;
> >> +
> >> +	/* Get the effective exposure and gain applied on the sensor. */
> >> +	uint32_t exposure = frameContext.sensor.exposure;
> >> +	double analogueGain = frameContext.sensor.gain;
> >> +
> >> +	utils::Duration minShutterSpeed = configuration.agc.minShutterSpeed;
> >> +	utils::Duration maxShutterSpeed = std::min(configuration.agc.maxShutterSpeed,
> >> +						   kMaxShutterSpeed);
> >> +
> >> +	double minAnalogueGain = std::max(configuration.agc.minAnalogueGain,
> >> +					  kMinAnalogueGain);
> >> +	double maxAnalogueGain = std::min(configuration.agc.maxAnalogueGain,
> >> +					  kMaxAnalogueGain);
> >> +
> >> +	/* Consider within 1% of the target as correctly exposed */
> >> +	if (std::abs(yGain - 1.0) < 0.01)
> >> +		LOG(RkISP1Agc, Debug) << "We are well exposed (iqMean = "
> >> +				      << yGain << ")";
> > 
> > This sounds like it will flood the logs without bringing much value, as
> > the yGain is printed below too. We used to return early in this case,
> > I'm not sure we we don't anymore. Also something we should address (if
> > desired) for IPU3 and RkISP1 on top.
> 
> You are right.
> 
> >> +
> >> +	/* extracted from Rpi::Agc::computeTargetExposure */
> >> +
> >> +	/* Calculate the shutter time in seconds */
> >> +	utils::Duration currentShutter = exposure * configuration.sensor.lineDuration;
> >> +
> >> +	/*
> >> +	 * Update the exposure value for the next computation using the values
> >> +	 * of exposure and gain really used by the sensor.
> >> +	 */
> >> +	utils::Duration effectiveExposureValue = currentShutter * analogueGain;
> >> +
> >> +	LOG(RkISP1Agc, Debug) << "Actual total exposure " << currentShutter * analogueGain
> >> +			      << " Shutter speed " << currentShutter
> >> +			      << " Gain " << analogueGain
> >> +			      << " Needed ev gain " << yGain;
> >> +
> >> +	/*
> >> +	 * Calculate the current exposure value for the scene as the latest
> >> +	 * exposure value applied multiplied by the new estimated gain.
> >> +	 */
> >> +	utils::Duration currentExposure = effectiveExposureValue * yGain;
> > 
> > s/currentExposure/exposureValue/ here and below.
> > 
> >> +
> >> +	/* Clamp the exposure value to the min and max authorized */
> >> +	utils::Duration maxTotalExposure = maxShutterSpeed * maxAnalogueGain;
> >> +	currentExposure = std::min(currentExposure, maxTotalExposure);
> >> +	LOG(RkISP1Agc, Debug) << "Target total exposure " << currentExposure
> >> +			      << ", maximum is " << maxTotalExposure;
> >> +
> >> +	/*
> >> +	 * Divide the exposure value as new exposure and gain values
> >> +	 * \todo: estimate if we need to desaturate
> > 
> > s/todo:/todo/
> > 
> >> +	 */
> >> +	utils::Duration exposureValue = filterExposure(currentExposure);
> >> +
> >> +	/*
> >> +	* Push the shutter time up to the maximum first, and only then
> >> +	* increase the gain.
> >> +	*/
> > 
> > Incorrect indentation.
> > 
> >> +	utils::Duration shutterTime = std::clamp<utils::Duration>(exposureValue / minAnalogueGain,
> >> +								  minShutterSpeed, maxShutterSpeed);
> >> +	double stepGain = std::clamp(exposureValue / shutterTime,
> >> +				     minAnalogueGain, maxAnalogueGain);
> >> +	LOG(RkISP1Agc, Debug) << "Divided up shutter and gain are "
> >> +			      << shutterTime << " and "
> >> +			      << stepGain;
> >> +
> >> +	/* Update the estimated exposure and gain. */
> >> +	frameContext.agc.exposure = shutterTime / configuration.sensor.lineDuration;
> >> +	frameContext.agc.gain = stepGain;
> >> +}
> >> +
> >> +/**
> >> + * \brief Estimate the relative luminance of the frame with a given gain
> >> + * \param[in] ae The RkISP1 statistics and ISP results
> >> + * \param[in] gain The gain calculated on the current brightness level
> > 
> >   * \param[in] gain The gain to apply to the frame
> > 
> >> + * \return The relative luminance
> >> + *
> >> + * This function estimates the average relative luminance of the frame that
> >> + * would be output by the sensor if an additional \a gain was applied.
> >> + *
> >> + * The estimation is based on the AE statistics for the current frame. Y
> >> + * averages for all cells are first multiplied by the gain, and then saturated
> >> + * to approximate the sensor behaviour at high brightness values. The
> >> + * approximation is quite rough, as it doesn't take into account non-linearities
> >> + * when approaching saturation.
> > 
> > It's actually worse than that in this case, as saturating after the
> > conversion to YUV also doesn't take into account the fact that the R, G
> > and B components contribute differently to the relative luminance. A
> > different algorithm may be better (not as part of this patch of course).

Could you also record this by the way ?

 * when approaching saturation, nor does it take into account the R, G and B
 * components saturating at the same levels but contributing differently to the
 * relative luminance.

> >> + *
> >> + * The values are normalized to the [0.0, 1.0] range, where 1.0 corresponds to a
> >> + * theoretical perfect reflector of 100% reference white.
> >> + *
> >> + * More detailed information can be found in:
> >> + * https://en.wikipedia.org/wiki/Relative_luminance
> >> + */
> >> +double Agc::estimateLuminance(const rkisp1_cif_isp_ae_stat *ae,
> >> +			      double gain)
> >> +{
> >> +	double ySum = 0.0;
> >> +
> >> +	/* Sum the averages, saturated to 255. */
> >> +	for (unsigned int aeCell = 0; aeCell < numCells_; aeCell++)
> >> +		ySum += std::min(ae->exp_mean[aeCell] * gain, 255.0);
> >> +
> >> +	/* \todo Weight with the AWB gains */
> 
> This could be why we don't have the same kRelativeLuminanceTarget BTW ;-).
> 
> >> +
> >> +	return ySum / numCells_ / 255;
> >> +}
> >> +
> >> +/**
> >> + * \brief Process RkISP1 statistics, and run AGC operations
> >> + * \param[in] context The shared IPA context
> >> + * \param[in] stats The RKIsp1 statistics and ISP results
> > 
> > s/RKIsp1/RkISP1/
> > 
> >> + *
> >> + * Identify the current image brightness, and use that to estimate the optimal
> >> + * new exposure and gain for the scene.
> >> + */
> >> +void Agc::process(IPAContext &context, const rkisp1_stat_buffer *stats)
> >> +{
> >> +	const rkisp1_cif_isp_stat *params = &stats->params;
> >> +	ASSERT(stats->meas_type & RKISP1_CIF_ISP_STAT_AUTOEXP);
> >> +
> >> +	const rkisp1_cif_isp_ae_stat *ae = &params->ae;
> >> +
> >> +	/*
> >> +	 * Estimate the gain needed to achieve a relative luminance target. To
> >> +	 * account for non-linearity caused by saturation, the value needs to be
> >> +	 * estimated in an iterative process, as multiplying by a gain will not
> >> +	 * increase the relative luminance by the same factor if some image
> >> +	 * regions are saturated.
> >> +	 */
> >> +	double yGain = 1.0;
> >> +	double yTarget = kRelativeLuminanceTarget;
> >> +
> >> +	for (unsigned int i = 0; i < 8; i++) {
> >> +		double yValue = estimateLuminance(ae, yGain);
> >> +		double extra_gain = std::min(10.0, yTarget / (yValue + .001));
> >> +
> >> +		yGain *= extra_gain;
> >> +		LOG(RkISP1Agc, Debug) << "Y value: " << yValue
> >> +				      << ", Y target: " << yTarget
> >> +				      << ", gives gain " << yGain;
> >> +		if (extra_gain < 1.01)
> >> +			break;
> >> +	}
> >> +
> >> +	computeExposure(context, yGain);
> >> +	frameCount_++;
> > 
> > frameCount_ is never reset to 0, is that expected ?
> 
> No :-(
> 
> >> +}
> >> +
> >> +} /* namespace ipa::rkisp1::algorithms */
> >> +
> >> +} /* namespace libcamera */
> >> diff --git a/src/ipa/rkisp1/algorithms/agc.h b/src/ipa/rkisp1/algorithms/agc.h
> >> new file mode 100644
> >> index 00000000..8ec172d7
> >> --- /dev/null
> >> +++ b/src/ipa/rkisp1/algorithms/agc.h
> >> @@ -0,0 +1,46 @@
> >> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> >> +/*
> >> + * Copyright (C) 2021, Ideas On Board
> >> + *
> >> + * agc.h - RkISP1 AGC/AEC mean-based control algorithm
> >> + */
> >> +
> >> +#pragma once
> >> +
> >> +#include <linux/rkisp1-config.h>
> >> +
> >> +#include <libcamera/base/utils.h>
> >> +
> >> +#include <libcamera/geometry.h>
> >> +
> >> +#include "algorithm.h"
> >> +
> >> +namespace libcamera {
> >> +
> >> +struct IPACameraSensorInfo;
> >> +
> >> +namespace ipa::rkisp1::algorithms {
> >> +
> >> +class Agc : public Algorithm
> >> +{
> >> +public:
> >> +	Agc();
> >> +	~Agc() = default;
> >> +
> >> +	int configure(IPAContext &context, const IPACameraSensorInfo &configInfo) override;
> >> +	void process(IPAContext &context, const rkisp1_stat_buffer *stats) override;
> >> +
> >> +private:
> >> +	void computeExposure(IPAContext &Context, double yGain);
> >> +	utils::Duration filterExposure(utils::Duration currentExposure);
> > 
> > s/currentExposure/exposure/ here too.
> > 
> >> +	double estimateLuminance(const rkisp1_cif_isp_ae_stat *ae, double gain);
> >> +
> >> +	uint64_t frameCount_;
> >> +
> >> +	uint32_t numCells_;
> >> +
> >> +	utils::Duration filteredExposure_;
> >> +};
> >> +
> >> +} /* namespace ipa::rkisp1::algorithms */
> >> +} /* namespace libcamera */
> >> diff --git a/src/ipa/rkisp1/algorithms/meson.build b/src/ipa/rkisp1/algorithms/meson.build
> >> index 1c6c59cf..a19c1a4f 100644
> >> --- a/src/ipa/rkisp1/algorithms/meson.build
> >> +++ b/src/ipa/rkisp1/algorithms/meson.build
> >> @@ -1,4 +1,5 @@
> >>   # SPDX-License-Identifier: CC0-1.0
> >>   
> >>   rkisp1_ipa_algorithms = files([
> >> +    'agc.cpp',
> >>   ])
> >> diff --git a/src/ipa/rkisp1/ipa_context.cpp b/src/ipa/rkisp1/ipa_context.cpp
> >> index 6b53dfdf..9cb2a9fd 100644
> >> --- a/src/ipa/rkisp1/ipa_context.cpp
> >> +++ b/src/ipa/rkisp1/ipa_context.cpp
> >> @@ -56,6 +56,21 @@ namespace libcamera::ipa::rkisp1 {
> >>    */
> >>   
> >>   /**
> >> + * \var IPASessionConfiguration::agc
> >> + * \brief AGC parameters configuration of the IPA
> >> + *
> >> + * \var IPASessionConfiguration::agc.minShutterSpeed
> >> + * \brief Minimum shutter speed supported with the configured sensor
> >> + *
> >> + * \var IPASessionConfiguration::agc.maxShutterSpeed
> >> + * \brief Maximum shutter speed supported with the configured sensor
> >> + *
> >> + * \var IPASessionConfiguration::agc.minAnalogueGain
> >> + * \brief Minimum analogue gain supported with the configured sensor
> >> + *
> >> + * \var IPASessionConfiguration::agc.maxAnalogueGain
> >> + * \brief Maximum analogue gain supported with the configured sensor
> >> + *
> >>    * \var IPASessionConfiguration::hw
> >>    * \brief RkISP1-specific hardware information
> >>    *
> >> @@ -63,4 +78,39 @@ namespace libcamera::ipa::rkisp1 {
> >>    * \brief Hardware revision of the ISP
> >>    */
> >>   
> >> +/**
> >> + * \var IPASessionConfiguration::sensor
> >> + * \brief Sensor-specific configuration of the IPA
> >> + *
> >> + * \var IPASessionConfiguration::sensor.lineDuration
> >> + * \brief Line duration in microseconds
> >> + */
> >> +
> >> +/**
> >> + * \var IPAFrameContext::agc
> >> + * \brief Context for the Automatic Gain Control algorithm
> >> + *
> >> + * The exposure and gain determined are expected to be applied to the sensor
> >> + * at the earliest opportunity.
> >> + *
> >> + * \var IPAFrameContext::agc.exposure
> >> + * \brief Exposure time expressed as a number of lines
> >> + *
> >> + * \var IPAFrameContext::agc.gain
> >> + * \brief Analogue gain multiplier
> >> + *
> >> + * The gain should be adapted to the sensor specific gain code before applying.
> >> + */
> >> +
> >> +/**
> >> + * \var IPAFrameContext::sensor
> >> + * \brief Effective sensor values
> >> + *
> >> + * \var IPAFrameContext::sensor.exposure
> >> + * \brief Exposure time expressed as a number of lines
> >> + *
> >> + * \var IPAFrameContext::sensor.gain
> >> + * \brief Analogue gain multiplier
> >> + */
> >> +
> >>   } /* namespace libcamera::ipa::rkisp1 */
> >> diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h
> >> index 9342025b..b94ade0c 100644
> >> --- a/src/ipa/rkisp1/ipa_context.h
> >> +++ b/src/ipa/rkisp1/ipa_context.h
> >> @@ -10,17 +10,39 @@
> >>   
> >>   #include <linux/rkisp1-config.h>
> >>   
> >> +#include <libcamera/base/utils.h>
> >> +
> >>   namespace libcamera {
> >>   
> >>   namespace ipa::rkisp1 {
> >>   
> >>   struct IPASessionConfiguration {
> >> +	struct {
> >> +		utils::Duration minShutterSpeed;
> >> +		utils::Duration maxShutterSpeed;
> >> +		double minAnalogueGain;
> >> +		double maxAnalogueGain;
> >> +	} agc;
> >> +
> >> +	struct {
> >> +		utils::Duration lineDuration;
> >> +	} sensor;
> >> +
> >>   	struct {
> >>   		rkisp1_cif_isp_version revision;
> >>   	} hw;
> >>   };
> >>   
> >>   struct IPAFrameContext {
> >> +	struct {
> >> +		uint32_t exposure;
> >> +		double gain;
> >> +	} agc;
> >> +
> >> +	struct {
> >> +		uint32_t exposure;
> >> +		double gain;
> >> +	} sensor;
> >>   };
> >>   
> >>   struct IPAContext {
> >> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> >> index 59676a70..38917fb7 100644
> >> --- a/src/ipa/rkisp1/rkisp1.cpp
> >> +++ b/src/ipa/rkisp1/rkisp1.cpp
> >> @@ -25,6 +25,7 @@
> >>   
> >>   #include <libcamera/internal/mapped_framebuffer.h>
> >>   
> >> +#include "algorithms/agc.h"
> >>   #include "algorithms/algorithm.h"
> >>   #include "libipa/camera_sensor_helper.h"
> >>   
> >> @@ -34,6 +35,8 @@ namespace libcamera {
> >>   
> >>   LOG_DEFINE_CATEGORY(IPARkISP1)
> >>   
> >> +using namespace std::literals::chrono_literals;
> >> +
> >>   namespace ipa::rkisp1 {
> >>   
> >>   class IPARkISP1 : public IPARkISP1Interface
> >> @@ -66,16 +69,13 @@ private:
> >>   
> >>   	/* Camera sensor controls. */
> >>   	bool autoExposure_;
> >> -	uint32_t exposure_;
> >>   	uint32_t minExposure_;
> >>   	uint32_t maxExposure_;
> >> -	uint32_t gain_;
> >>   	uint32_t minGain_;
> >>   	uint32_t maxGain_;
> >>   
> >>   	/* revision-specific data */
> >>   	rkisp1_cif_isp_version hwRevision_;
> >> -	unsigned int hwAeMeanMax_;
> >>   	unsigned int hwHistBinNMax_;
> >>   	unsigned int hwGammaOutMaxSamples_;
> >>   	unsigned int hwHistogramWeightGridsSize_;
> >> @@ -95,13 +95,11 @@ int IPARkISP1::init(const IPASettings &settings, unsigned int hwRevision)
> >>   	/* \todo Add support for other revisions */
> >>   	switch (hwRevision) {
> >>   	case RKISP1_V10:
> >> -		hwAeMeanMax_ = RKISP1_CIF_ISP_AE_MEAN_MAX_V10;
> >>   		hwHistBinNMax_ = RKISP1_CIF_ISP_HIST_BIN_N_MAX_V10;
> >>   		hwGammaOutMaxSamples_ = RKISP1_CIF_ISP_GAMMA_OUT_MAX_SAMPLES_V10;
> >>   		hwHistogramWeightGridsSize_ = RKISP1_CIF_ISP_HISTOGRAM_WEIGHT_GRIDS_SIZE_V10;
> >>   		break;
> >>   	case RKISP1_V12:
> >> -		hwAeMeanMax_ = RKISP1_CIF_ISP_AE_MEAN_MAX_V12;
> >>   		hwHistBinNMax_ = RKISP1_CIF_ISP_HIST_BIN_N_MAX_V12;
> >>   		hwGammaOutMaxSamples_ = RKISP1_CIF_ISP_GAMMA_OUT_MAX_SAMPLES_V12;
> >>   		hwHistogramWeightGridsSize_ = RKISP1_CIF_ISP_HISTOGRAM_WEIGHT_GRIDS_SIZE_V12;
> >> @@ -126,6 +124,9 @@ int IPARkISP1::init(const IPASettings &settings, unsigned int hwRevision)
> >>   		return -ENODEV;
> >>   	}
> >>   
> >> +	/* Construct our Algorithms */
> >> +	algorithms_.push_back(std::make_unique<algorithms::Agc>());
> >> +
> >>   	return 0;
> >>   }
> >>   
> >> @@ -167,11 +168,9 @@ int IPARkISP1::configure([[maybe_unused]] const IPACameraSensorInfo &info,
> >>   
> >>   	minExposure_ = itExp->second.min().get<int32_t>();
> >>   	maxExposure_ = itExp->second.max().get<int32_t>();
> >> -	exposure_ = minExposure_;
> >>   
> >>   	minGain_ = itGain->second.min().get<int32_t>();
> >>   	maxGain_ = itGain->second.max().get<int32_t>();
> >> -	gain_ = minGain_;
> >>   
> >>   	LOG(IPARkISP1, Info)
> >>   		<< "Exposure: " << minExposure_ << "-" << maxExposure_
> >> @@ -183,6 +182,26 @@ int IPARkISP1::configure([[maybe_unused]] const IPACameraSensorInfo &info,
> >>   	/* Set the hardware revision for the algorithms. */
> >>   	context_.configuration.hw.revision = hwRevision_;
> >>   
> >> +	context_.configuration.sensor.lineDuration = info.lineLength * 1.0s / info.pixelRate;
> >> +
> >> +	/*
> >> +	 * When the AGC computes the new exposure values for a frame, it needs
> >> +	 * to know the limits for shutter speed and analogue gain.
> >> +	 * As it depends on the sensor, update it with the controls.
> >> +	 *
> >> +	 * \todo take VBLANK into account for maximum shutter speed
> >> +	 */
> >> +	context_.configuration.agc.minShutterSpeed = minExposure_ * context_.configuration.sensor.lineDuration;
> >> +	context_.configuration.agc.maxShutterSpeed = maxExposure_ * context_.configuration.sensor.lineDuration;
> >> +	context_.configuration.agc.minAnalogueGain = camHelper_->gain(minGain_);
> >> +	context_.configuration.agc.maxAnalogueGain = camHelper_->gain(maxGain_);
> >> +
> >> +	for (auto const &algo : algorithms_) {
> >> +		int ret = algo->configure(context_, info);
> >> +		if (ret)
> >> +			return ret;
> >> +	}
> >> +
> >>   	return 0;
> >>   }
> >>   
> >> @@ -227,6 +246,11 @@ void IPARkISP1::processEvent(const RkISP1Event &event)
> >>   			reinterpret_cast<rkisp1_stat_buffer *>(
> >>   				mappedBuffers_.at(bufferId).planes()[0].data());
> >>   
> >> +		context_.frameContext.sensor.exposure =
> >> +			event.sensorControls.get(V4L2_CID_EXPOSURE).get<int32_t>();
> >> +		context_.frameContext.sensor.gain =
> >> +			camHelper_->gain(event.sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get<int32_t>());
> >> +
> >>   		updateStatistics(frame, stats);
> >>   		break;
> >>   	}
> >> @@ -271,44 +295,12 @@ void IPARkISP1::queueRequest(unsigned int frame, rkisp1_params_cfg *params,
> >>   void IPARkISP1::updateStatistics(unsigned int frame,
> >>   				 const rkisp1_stat_buffer *stats)
> >>   {
> >> -	const rkisp1_cif_isp_stat *params = &stats->params;
> >>   	unsigned int aeState = 0;
> >>   
> >> -	if (stats->meas_type & RKISP1_CIF_ISP_STAT_AUTOEXP) {
> >> -		const rkisp1_cif_isp_ae_stat *ae = &params->ae;
> >> -
> >> -		const unsigned int target = 60;
> >> -
> >> -		unsigned int value = 0;
> >> -		unsigned int num = 0;
> >> -		for (unsigned int i = 0; i < hwAeMeanMax_; i++) {
> >> -			if (ae->exp_mean[i] <= 15)
> >> -				continue;
> >> -
> >> -			value += ae->exp_mean[i];
> >> -			num++;
> >> -		}
> >> -		value /= num;
> >> +	for (auto const &algo : algorithms_)
> >> +		algo->process(context_, stats);
> >>   
> >> -		double factor = (double)target / value;
> >> -
> >> -		if (frame % 3 == 0) {
> >> -			double exposure;
> >> -
> >> -			exposure = factor * exposure_ * gain_ / minGain_;
> >> -			exposure_ = std::clamp<uint64_t>((uint64_t)exposure,
> >> -							 minExposure_,
> >> -							 maxExposure_);
> >> -
> >> -			exposure = exposure / exposure_ * minGain_;
> >> -			gain_ = std::clamp<uint64_t>((uint64_t)exposure,
> >> -						     minGain_, maxGain_);
> >> -
> >> -			setControls(frame + 1);
> >> -		}
> >> -
> >> -		aeState = fabs(factor - 1.0f) < 0.05f ? 2 : 1;
> >> -	}
> >> +	setControls(frame);
> >>   
> >>   	metadataReady(frame, aeState);
> >>   }
> >> @@ -318,9 +310,12 @@ void IPARkISP1::setControls(unsigned int frame)
> >>   	RkISP1Action op;
> >>   	op.op = ActionV4L2Set;
> >>   
> >> +	uint32_t exposure = context_.frameContext.agc.exposure;
> >> +	uint32_t gain = camHelper_->gainCode(context_.frameContext.agc.gain);
> >> +
> >>   	ControlList ctrls(ctrls_);
> >> -	ctrls.set(V4L2_CID_EXPOSURE, static_cast<int32_t>(exposure_));
> >> -	ctrls.set(V4L2_CID_ANALOGUE_GAIN, static_cast<int32_t>(gain_));
> >> +	ctrls.set(V4L2_CID_EXPOSURE, static_cast<int32_t>(exposure));
> >> +	ctrls.set(V4L2_CID_ANALOGUE_GAIN, static_cast<int32_t>(gain));
> >>   	op.sensorControls = ctrls;
> >>   
> >>   	queueFrameAction.emit(frame, op);