[libcamera-devel,v3,07/19] ipa: ipu3: agc: Document AGC mean-based algorithm

14273 diff mbox series

The AGC class was not documented while developing. Extend that to
reference the origins of the implementation, and improve the
descriptions on how the algorithm operates internally.

While at it, rename the functions which have bad names.

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

---
v3:
- rename processBrightness to measureBrightness
- rename lockExposureGain to computeExposure
- reword some comments
---
 src/ipa/ipu3/algorithms/agc.cpp | 79 ++++++++++++++++++++++++++++++---
 src/ipa/ipu3/algorithms/agc.h   |  6 +--
 2 files changed, 75 insertions(+), 10 deletions(-)

Hi Jean-Michel,

Thank you for the patch.

On Fri, Oct 22, 2021 at 05:12:06PM +0200, Jean-Michel Hautbois wrote:
> The AGC class was not documented while developing. Extend that to
> reference the origins of the implementation, and improve the
> descriptions on how the algorithm operates internally.
> 
> While at it, rename the functions which have bad names.
> 
> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> 
> ---
> v3:
> - rename processBrightness to measureBrightness
> - rename lockExposureGain to computeExposure
> - reword some comments
> ---
>  src/ipa/ipu3/algorithms/agc.cpp | 79 ++++++++++++++++++++++++++++++---
>  src/ipa/ipu3/algorithms/agc.h   |  6 +--
>  2 files changed, 75 insertions(+), 10 deletions(-)
> 
> diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp
> index 6c151232..b145e522 100644
> --- a/src/ipa/ipu3/algorithms/agc.cpp
> +++ b/src/ipa/ipu3/algorithms/agc.cpp
> @@ -2,7 +2,7 @@
>  /*
>   * Copyright (C) 2021, Ideas On Board
>   *
> - * ipu3_agc.cpp - AGC/AEC control algorithm
> + * ipu3_agc.cpp - AGC/AEC mean-based control algorithm
>   */
>  
>  #include "agc.h"
> @@ -17,17 +17,37 @@
>  
>  #include "libipa/histogram.h"
>  
> +/**
> + * \file agc.h
> + */
> +
>  namespace libcamera {
>  
>  using namespace std::literals::chrono_literals;
>  
>  namespace ipa::ipu3::algorithms {
>  
> +/**
> + * \class Agc
> + * \brief A mean-based auto-exposure algorithm
> + *
> + * This algorithm calculates a shutter time and an analogue gain so that the
> + * average value of the green channel of the brightest 2% of pixels approaches
> + * 0.5. The AWB gains are not used here, and all cells in the grid have the same
> + * weight, like an average-metering case. In this metering mode, the camera uses
> + * light information from the entire scene and creates an average for the final
> + * exposure setting, giving no weighting to any particular portion of the
> + * metered area.
> + *
> + * Reference: Battiato, Messina & Castorina. (2008). Exposure
> + * Correction for Imaging Devices: An Overview. 10.1201/9781420054538.ch12.
> + */
> +
>  LOG_DEFINE_CATEGORY(IPU3Agc)
>  
>  /* Number of frames to wait before calculating stats on minimum exposure */
>  static constexpr uint32_t kInitialFrameMinAECount = 4;
> -/* Number of frames to wait between new gain/exposure estimations */
> +/* Number of frames to wait between new gain/shutter time estimations */
>  static constexpr uint32_t kFrameSkipCount = 6;
>  
>  /* Limits for analogue gain values */
> @@ -36,6 +56,7 @@ static constexpr double kMaxAnalogueGain = 8.0;
>  
>  /* Histogram constants */
>  static constexpr uint32_t knumHistogramBins = 256;
> +/* Target value to reach for the top 2% of the histogram */
>  static constexpr double kEvGainTarget = 0.5;
>  
>  Agc::Agc()
> @@ -45,10 +66,18 @@ Agc::Agc()
>  {
>  }
>  
> +/**
> + * \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, const IPAConfigInfo &configInfo)
>  {
>  	stride_ = context.configuration.grid.stride;
>  
> +	/* \todo use the IPAContext to provide the limits */
>  	lineDuration_ = configInfo.sensorInfo.lineLength * 1.0s
>  		      / configInfo.sensorInfo.pixelRate;
>  
> @@ -70,9 +99,15 @@ int Agc::configure(IPAContext &context, const IPAConfigInfo &configInfo)
>  	return 0;
>  }
>  
> -void Agc::processBrightness(const ipu3_uapi_stats_3a *stats,
> +/**
> + * \brief Estimate the mean value of the top 2% of the histogram
> + * \param[in] stats The statistics computed by the ImgU
> + * \param[in] grid The grid used to store the statistics in the IPU3
> + */
> +void Agc::measureBrightness(const ipu3_uapi_stats_3a *stats,
>  			    const ipu3_uapi_grid_config &grid)
>  {
> +	/* Initialise the histogram array */
>  	uint32_t hist[knumHistogramBins] = { 0 };
>  
>  	for (unsigned int cellY = 0; cellY < grid.height; cellY++) {
> @@ -87,6 +122,11 @@ void Agc::processBrightness(const ipu3_uapi_stats_3a *stats,
>  			if (cell->sat_ratio == 0) {
>  				uint8_t gr = cell->Gr_avg;
>  				uint8_t gb = cell->Gb_avg;
> +				/*
> +				 * Store the average green value to estimate the
> +				 * brightness. Even the over exposed pixels are

s/over exposed/overexposed/

> +				 * taken into account.
> +				 */
>  				hist[(gr + gb) / 2]++;
>  			}
>  		}
> @@ -96,6 +136,9 @@ void Agc::processBrightness(const ipu3_uapi_stats_3a *stats,
>  	iqMean_ = Histogram(Span<uint32_t>(hist)).interQuantileMean(0.98, 1.0);
>  }
>  
> +/**
> + * \brief Apply a filter on the exposure value to limit the speed of changes
> + */
>  void Agc::filterExposure()
>  {
>  	double speed = 0.2;
> @@ -106,7 +149,7 @@ void Agc::filterExposure()
>  		/*
>  		 * If we are close to the desired result, go faster to avoid making
>  		 * multiple micro-adjustments.
> -		 * \ todo: Make this customisable?
> +		 * \todo: Make this customisable?

s/://

>  		 */
>  		if (filteredExposure_ < 1.2 * currentExposure_ &&
>  		    filteredExposure_ > 0.8 * currentExposure_)
> @@ -119,7 +162,12 @@ void Agc::filterExposure()
>  	LOG(IPU3Agc, Debug) << "After filtering, total_exposure " << filteredExposure_;
>  }
>  
> -void Agc::lockExposureGain(uint32_t &exposure, double &analogueGain)
> +/**
> + * \brief Estimate the new exposure and gain values
> + * \param[inout] exposure The exposure value reference as a number of lines
> + * \param[inout] gain The gain reference to be updated
> + */
> +void Agc::computeExposure(uint32_t &exposure, double &analogueGain)
>  {
>  	/* Algorithm initialization should wait for first valid frames */
>  	/* \todo - have a number of frames given by DelayedControls ?
> @@ -136,19 +184,26 @@ void Agc::lockExposureGain(uint32_t &exposure, double &analogueGain)
>  		return;
>  	}
>  
> +	/* Estimate the gain needed to have the proportion wanted */
>  	double evGain = kEvGainTarget * knumHistogramBins / iqMean_;
>  
>  	/* extracted from Rpi::Agc::computeTargetExposure */
> +	/* Calculate the shutter time in seconds */
>  	utils::Duration currentShutter = exposure * lineDuration_;
>  	LOG(IPU3Agc, Debug) << "Actual total exposure " << currentShutter * analogueGain
>  			    << " Shutter speed " << currentShutter
>  			    << " Gain " << analogueGain
>  			    << " Needed ev gain " << evGain;
>  
> +	/*
> +	 * Estimate the current exposure value for the scene as the latest
> +	 * exposure value applied multiplied by the new estimated gain

s/gain/gain./

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> +	 */
>  	currentExposure_ = prevExposureValue_ * evGain;
>  	utils::Duration minShutterSpeed = minExposureLines_ * lineDuration_;
>  	utils::Duration maxShutterSpeed = maxExposureLines_ * lineDuration_;
>  
> +	/* Clamp the exposure value to the min and max authorized */
>  	utils::Duration maxTotalExposure = maxShutterSpeed * maxAnalogueGain_;
>  	currentExposure_ = std::min(currentExposure_, maxTotalExposure);
>  	LOG(IPU3Agc, Debug) << "Target total exposure " << currentExposure_
> @@ -157,6 +212,7 @@ void Agc::lockExposureGain(uint32_t &exposure, double &analogueGain)
>  	/* \todo: estimate if we need to desaturate */
>  	filterExposure();
>  
> +	/* Divide the exposure value as new exposure and gain values */
>  	utils::Duration exposureValue = filteredExposure_;
>  	utils::Duration shutterTime = minShutterSpeed;
>  
> @@ -186,12 +242,21 @@ void Agc::lockExposureGain(uint32_t &exposure, double &analogueGain)
>  	prevExposureValue_ = shutterTime * analogueGain;
>  }
>  
> +/**
> + * \brief Process IPU3 statistics, and run AGC operations
> + * \param[in] context The shared IPA context
> + * \param[in] stats The IPU3 statistics and ISP results
> + *
> + * 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 ipu3_uapi_stats_3a *stats)
>  {
> +	/* Get the latest exposure and gain applied */
>  	uint32_t &exposure = context.frameContext.agc.exposure;
>  	double &analogueGain = context.frameContext.agc.gain;
> -	processBrightness(stats, context.configuration.grid.bdsGrid);
> -	lockExposureGain(exposure, analogueGain);
> +	measureBrightness(stats, context.configuration.grid.bdsGrid);
> +	computeExposure(exposure, analogueGain);
>  	frameCount_++;
>  }
>  
> diff --git a/src/ipa/ipu3/algorithms/agc.h b/src/ipa/ipu3/algorithms/agc.h
> index 1840205b..69e0b831 100644
> --- a/src/ipa/ipu3/algorithms/agc.h
> +++ b/src/ipa/ipu3/algorithms/agc.h
> @@ -2,7 +2,7 @@
>  /*
>   * Copyright (C) 2021, Ideas On Board
>   *
> - * agc.h - IPU3 AGC/AEC control algorithm
> + * agc.h - IPU3 AGC/AEC mean-based control algorithm
>   */
>  #ifndef __LIBCAMERA_IPU3_ALGORITHMS_AGC_H__
>  #define __LIBCAMERA_IPU3_ALGORITHMS_AGC_H__
> @@ -31,10 +31,10 @@ public:
>  	void process(IPAContext &context, const ipu3_uapi_stats_3a *stats) override;
>  
>  private:
> -	void processBrightness(const ipu3_uapi_stats_3a *stats,
> +	void measureBrightness(const ipu3_uapi_stats_3a *stats,
>  			       const ipu3_uapi_grid_config &grid);
>  	void filterExposure();
> -	void lockExposureGain(uint32_t &exposure, double &gain);
> +	void computeExposure(uint32_t &exposure, double &gain);
>  
>  	uint64_t frameCount_;
>  	uint64_t lastFrame_;

Quoting Jean-Michel Hautbois (2021-10-22 16:12:06)
> The AGC class was not documented while developing. Extend that to
> reference the origins of the implementation, and improve the
> descriptions on how the algorithm operates internally.
> 
> While at it, rename the functions which have bad names.

Aha, these are just renames. It threw me off thinking there were bigger
changes in here, but a rename is fine.

> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> 
> ---
> v3:
> - rename processBrightness to measureBrightness
> - rename lockExposureGain to computeExposure
> - reword some comments
> ---
>  src/ipa/ipu3/algorithms/agc.cpp | 79 ++++++++++++++++++++++++++++++---
>  src/ipa/ipu3/algorithms/agc.h   |  6 +--
>  2 files changed, 75 insertions(+), 10 deletions(-)
> 
> diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp
> index 6c151232..b145e522 100644
> --- a/src/ipa/ipu3/algorithms/agc.cpp
> +++ b/src/ipa/ipu3/algorithms/agc.cpp
> @@ -2,7 +2,7 @@
>  /*
>   * Copyright (C) 2021, Ideas On Board
>   *
> - * ipu3_agc.cpp - AGC/AEC control algorithm
> + * ipu3_agc.cpp - AGC/AEC mean-based control algorithm
>   */
>  
>  #include "agc.h"
> @@ -17,17 +17,37 @@
>  
>  #include "libipa/histogram.h"
>  
> +/**
> + * \file agc.h
> + */
> +
>  namespace libcamera {
>  
>  using namespace std::literals::chrono_literals;
>  
>  namespace ipa::ipu3::algorithms {
>  
> +/**
> + * \class Agc
> + * \brief A mean-based auto-exposure algorithm
> + *
> + * This algorithm calculates a shutter time and an analogue gain so that the

Is it an analogue gain? or just a gain? Does the algorithm care if it is
analogue?

Perhaps it does, as it's targetted to the Sensor controls.

> + * average value of the green channel of the brightest 2% of pixels approaches
> + * 0.5. The AWB gains are not used here, and all cells in the grid have the same
> + * weight, like an average-metering case. In this metering mode, the camera uses
> + * light information from the entire scene and creates an average for the final
> + * exposure setting, giving no weighting to any particular portion of the
> + * metered area.
> + *
> + * Reference: Battiato, Messina & Castorina. (2008). Exposure
> + * Correction for Imaging Devices: An Overview. 10.1201/9781420054538.ch12.
> + */
> +
>  LOG_DEFINE_CATEGORY(IPU3Agc)
>  
>  /* Number of frames to wait before calculating stats on minimum exposure */
>  static constexpr uint32_t kInitialFrameMinAECount = 4;
> -/* Number of frames to wait between new gain/exposure estimations */
> +/* Number of frames to wait between new gain/shutter time estimations */
>  static constexpr uint32_t kFrameSkipCount = 6;
>  
>  /* Limits for analogue gain values */
> @@ -36,6 +56,7 @@ static constexpr double kMaxAnalogueGain = 8.0;
>  
>  /* Histogram constants */
>  static constexpr uint32_t knumHistogramBins = 256;

I'd have a new line here.

> +/* Target value to reach for the top 2% of the histogram */

Is this something that is expected to be tuned? or is 0.5 used to mean
quite literally 'the middle' between 1 and 0?

>  static constexpr double kEvGainTarget = 0.5;
>  
>  Agc::Agc()
> @@ -45,10 +66,18 @@ Agc::Agc()
>  {
>  }
>  
> +/**
> + * \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, const IPAConfigInfo &configInfo)
>  {
>         stride_ = context.configuration.grid.stride;
>  
> +       /* \todo use the IPAContext to provide the limits */
>         lineDuration_ = configInfo.sensorInfo.lineLength * 1.0s
>                       / configInfo.sensorInfo.pixelRate;
>  
> @@ -70,9 +99,15 @@ int Agc::configure(IPAContext &context, const IPAConfigInfo &configInfo)
>         return 0;
>  }
>  
> -void Agc::processBrightness(const ipu3_uapi_stats_3a *stats,
> +/**
> + * \brief Estimate the mean value of the top 2% of the histogram
> + * \param[in] stats The statistics computed by the ImgU
> + * \param[in] grid The grid used to store the statistics in the IPU3
> + */
> +void Agc::measureBrightness(const ipu3_uapi_stats_3a *stats,
>                             const ipu3_uapi_grid_config &grid)
>  {
> +       /* Initialise the histogram array */
>         uint32_t hist[knumHistogramBins] = { 0 };
>  
>         for (unsigned int cellY = 0; cellY < grid.height; cellY++) {
> @@ -87,6 +122,11 @@ void Agc::processBrightness(const ipu3_uapi_stats_3a *stats,
>                         if (cell->sat_ratio == 0) {
>                                 uint8_t gr = cell->Gr_avg;
>                                 uint8_t gb = cell->Gb_avg;
> +                               /*
> +                                * Store the average green value to estimate the
> +                                * brightness. Even the over exposed pixels are
> +                                * taken into account.
> +                                */
>                                 hist[(gr + gb) / 2]++;
>                         }
>                 }
> @@ -96,6 +136,9 @@ void Agc::processBrightness(const ipu3_uapi_stats_3a *stats,
>         iqMean_ = Histogram(Span<uint32_t>(hist)).interQuantileMean(0.98, 1.0);
>  }
>  
> +/**
> + * \brief Apply a filter on the exposure value to limit the speed of changes
> + */
>  void Agc::filterExposure()
>  {
>         double speed = 0.2;
> @@ -106,7 +149,7 @@ void Agc::filterExposure()
>                 /*
>                  * If we are close to the desired result, go faster to avoid making
>                  * multiple micro-adjustments.
> -                * \ todo: Make this customisable?
> +                * \todo: Make this customisable?
>                  */
>                 if (filteredExposure_ < 1.2 * currentExposure_ &&
>                     filteredExposure_ > 0.8 * currentExposure_)
> @@ -119,7 +162,12 @@ void Agc::filterExposure()
>         LOG(IPU3Agc, Debug) << "After filtering, total_exposure " << filteredExposure_;
>  }
>  
> -void Agc::lockExposureGain(uint32_t &exposure, double &analogueGain)
> +/**
> + * \brief Estimate the new exposure and gain values
> + * \param[inout] exposure The exposure value reference as a number of lines
> + * \param[inout] gain The gain reference to be updated
> + */
> +void Agc::computeExposure(uint32_t &exposure, double &analogueGain)
>  {
>         /* Algorithm initialization should wait for first valid frames */
>         /* \todo - have a number of frames given by DelayedControls ?
> @@ -136,19 +184,26 @@ void Agc::lockExposureGain(uint32_t &exposure, double &analogueGain)
>                 return;
>         }
>  
> +       /* Estimate the gain needed to have the proportion wanted */
>         double evGain = kEvGainTarget * knumHistogramBins / iqMean_;
>  
>         /* extracted from Rpi::Agc::computeTargetExposure */
> +       /* Calculate the shutter time in seconds */
>         utils::Duration currentShutter = exposure * lineDuration_;
>         LOG(IPU3Agc, Debug) << "Actual total exposure " << currentShutter * analogueGain
>                             << " Shutter speed " << currentShutter
>                             << " Gain " << analogueGain
>                             << " Needed ev gain " << evGain;
>  
> +       /*
> +        * Estimate the current exposure value for the scene as the latest
> +        * exposure value applied multiplied by the new estimated gain
> +        */

Does this estimate what the sensor is currently configured to expose?
Or does it 'calculate' what is desired for the next control to set?

Estimate here implies that the algorithm is guessing what the sensor
might be configured as (which I would hope we can 'know' or determine?)

Ok, the brief of this function clears up my question, as it says
'Estimate the new'.

Is 'estimate' intentional? I would interpret that we would 'calculate'
the new values. Or in fact, given the function name, we might be
'computing' the new vaules...


>         currentExposure_ = prevExposureValue_ * evGain;
>         utils::Duration minShutterSpeed = minExposureLines_ * lineDuration_;
>         utils::Duration maxShutterSpeed = maxExposureLines_ * lineDuration_;
>  
> +       /* Clamp the exposure value to the min and max authorized */
>         utils::Duration maxTotalExposure = maxShutterSpeed * maxAnalogueGain_;
>         currentExposure_ = std::min(currentExposure_, maxTotalExposure);
>         LOG(IPU3Agc, Debug) << "Target total exposure " << currentExposure_
> @@ -157,6 +212,7 @@ void Agc::lockExposureGain(uint32_t &exposure, double &analogueGain)
>         /* \todo: estimate if we need to desaturate */
>         filterExposure();
>  
> +       /* Divide the exposure value as new exposure and gain values */
>         utils::Duration exposureValue = filteredExposure_;
>         utils::Duration shutterTime = minShutterSpeed;
>  
> @@ -186,12 +242,21 @@ void Agc::lockExposureGain(uint32_t &exposure, double &analogueGain)
>         prevExposureValue_ = shutterTime * analogueGain;
>  }
>  
> +/**
> + * \brief Process IPU3 statistics, and run AGC operations
> + * \param[in] context The shared IPA context
> + * \param[in] stats The IPU3 statistics and ISP results
> + *
> + * Identify the current image brightness, and use that to estimate the optimal
> + * new exposure and gain for the scene.

Estimate or calculate? Estimate might be correct (here and above) if we
don't know in a single iteration what values to choose, as I suspect
it's somewhat of an adaptive algorithm that takes a couple of iterations
to get to a desired goal.

If estimate is correct - then I think that covers my main concerns, or
with an update to 'calculate' if that's more correct:

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

> + */
>  void Agc::process(IPAContext &context, const ipu3_uapi_stats_3a *stats)
>  {
> +       /* Get the latest exposure and gain applied */
>         uint32_t &exposure = context.frameContext.agc.exposure;
>         double &analogueGain = context.frameContext.agc.gain;
> -       processBrightness(stats, context.configuration.grid.bdsGrid);
> -       lockExposureGain(exposure, analogueGain);
> +       measureBrightness(stats, context.configuration.grid.bdsGrid);
> +       computeExposure(exposure, analogueGain);
>         frameCount_++;
>  }
>  
> diff --git a/src/ipa/ipu3/algorithms/agc.h b/src/ipa/ipu3/algorithms/agc.h
> index 1840205b..69e0b831 100644
> --- a/src/ipa/ipu3/algorithms/agc.h
> +++ b/src/ipa/ipu3/algorithms/agc.h
> @@ -2,7 +2,7 @@
>  /*
>   * Copyright (C) 2021, Ideas On Board
>   *
> - * agc.h - IPU3 AGC/AEC control algorithm
> + * agc.h - IPU3 AGC/AEC mean-based control algorithm
>   */
>  #ifndef __LIBCAMERA_IPU3_ALGORITHMS_AGC_H__
>  #define __LIBCAMERA_IPU3_ALGORITHMS_AGC_H__
> @@ -31,10 +31,10 @@ public:
>         void process(IPAContext &context, const ipu3_uapi_stats_3a *stats) override;
>  
>  private:
> -       void processBrightness(const ipu3_uapi_stats_3a *stats,
> +       void measureBrightness(const ipu3_uapi_stats_3a *stats,
>                                const ipu3_uapi_grid_config &grid);
>         void filterExposure();
> -       void lockExposureGain(uint32_t &exposure, double &gain);
> +       void computeExposure(uint32_t &exposure, double &gain);
>  
>         uint64_t frameCount_;
>         uint64_t lastFrame_;
> -- 
> 2.32.0
>

Hi Kieran,

On 25/10/2021 23:26, Kieran Bingham wrote:
> Quoting Jean-Michel Hautbois (2021-10-22 16:12:06)
>> The AGC class was not documented while developing. Extend that to
>> reference the origins of the implementation, and improve the
>> descriptions on how the algorithm operates internally.
>>
>> While at it, rename the functions which have bad names.
> 
> Aha, these are just renames. It threw me off thinking there were bigger
> changes in here, but a rename is fine.
> 
>> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
>> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>>
>> ---
>> v3:
>> - rename processBrightness to measureBrightness
>> - rename lockExposureGain to computeExposure
>> - reword some comments
>> ---
>>   src/ipa/ipu3/algorithms/agc.cpp | 79 ++++++++++++++++++++++++++++++---
>>   src/ipa/ipu3/algorithms/agc.h   |  6 +--
>>   2 files changed, 75 insertions(+), 10 deletions(-)
>>
>> diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp
>> index 6c151232..b145e522 100644
>> --- a/src/ipa/ipu3/algorithms/agc.cpp
>> +++ b/src/ipa/ipu3/algorithms/agc.cpp
>> @@ -2,7 +2,7 @@
>>   /*
>>    * Copyright (C) 2021, Ideas On Board
>>    *
>> - * ipu3_agc.cpp - AGC/AEC control algorithm
>> + * ipu3_agc.cpp - AGC/AEC mean-based control algorithm
>>    */
>>   
>>   #include "agc.h"
>> @@ -17,17 +17,37 @@
>>   
>>   #include "libipa/histogram.h"
>>   
>> +/**
>> + * \file agc.h
>> + */
>> +
>>   namespace libcamera {
>>   
>>   using namespace std::literals::chrono_literals;
>>   
>>   namespace ipa::ipu3::algorithms {
>>   
>> +/**
>> + * \class Agc
>> + * \brief A mean-based auto-exposure algorithm
>> + *
>> + * This algorithm calculates a shutter time and an analogue gain so that the
> 
> Is it an analogue gain? or just a gain? Does the algorithm care if it is
> analogue?
> 
> Perhaps it does, as it's targetted to the Sensor controls.

Quoting you and Laurent in v1:

Kieran:
Surely the algorithm itself doesn't care if this is an analogue gain or 
a digital gain.

Laurent:
Analogue gain is applied in the sensor, before the ADC, while digital
gain can be applied anywhere after the ADC. Most sensors will have a
digital gain, which is usually unused, and ISPs also have digital gains.
It's important to split gain between analogue and digital correctly as
they have different implications (in terms of noise for instance), so
the algorithm needs to care.

Kieran:
The IPU3 will apply it as an analogue gain - but that's independent 
isn't it ?

Laurent:
If you meant the IPU3 IPA, yes, it applies the gain as analogue gain. If
you meant the IPU3 hardware, no, it has no analogue gain.

Kieran:
Presumably if we weren't able to apply it fully as analogue - the 
remainder could be applied digitally to meet the desired gain requested 
  by the algorithm?

Laurent:
That's usually what a basic implementation will do, there could be
reason to do something more complex.

Me:
We apply analogue gains right now and should take care of digital gain 
later :-).

> 
>> + * average value of the green channel of the brightest 2% of pixels approaches
>> + * 0.5. The AWB gains are not used here, and all cells in the grid have the same
>> + * weight, like an average-metering case. In this metering mode, the camera uses
>> + * light information from the entire scene and creates an average for the final
>> + * exposure setting, giving no weighting to any particular portion of the
>> + * metered area.
>> + *
>> + * Reference: Battiato, Messina & Castorina. (2008). Exposure
>> + * Correction for Imaging Devices: An Overview. 10.1201/9781420054538.ch12.
>> + */
>> +
>>   LOG_DEFINE_CATEGORY(IPU3Agc)
>>   
>>   /* Number of frames to wait before calculating stats on minimum exposure */
>>   static constexpr uint32_t kInitialFrameMinAECount = 4;
>> -/* Number of frames to wait between new gain/exposure estimations */
>> +/* Number of frames to wait between new gain/shutter time estimations */
>>   static constexpr uint32_t kFrameSkipCount = 6;
>>   
>>   /* Limits for analogue gain values */
>> @@ -36,6 +56,7 @@ static constexpr double kMaxAnalogueGain = 8.0;
>>   
>>   /* Histogram constants */
>>   static constexpr uint32_t knumHistogramBins = 256;
> 
> I'd have a new line here.
> 
>> +/* Target value to reach for the top 2% of the histogram */
> 
> Is this something that is expected to be tuned? or is 0.5 used to mean
> quite literally 'the middle' between 1 and 0?
> 
>>   static constexpr double kEvGainTarget = 0.5;
>>   
>>   Agc::Agc()
>> @@ -45,10 +66,18 @@ Agc::Agc()
>>   {
>>   }
>>   
>> +/**
>> + * \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, const IPAConfigInfo &configInfo)
>>   {
>>          stride_ = context.configuration.grid.stride;
>>   
>> +       /* \todo use the IPAContext to provide the limits */
>>          lineDuration_ = configInfo.sensorInfo.lineLength * 1.0s
>>                        / configInfo.sensorInfo.pixelRate;
>>   
>> @@ -70,9 +99,15 @@ int Agc::configure(IPAContext &context, const IPAConfigInfo &configInfo)
>>          return 0;
>>   }
>>   
>> -void Agc::processBrightness(const ipu3_uapi_stats_3a *stats,
>> +/**
>> + * \brief Estimate the mean value of the top 2% of the histogram
>> + * \param[in] stats The statistics computed by the ImgU
>> + * \param[in] grid The grid used to store the statistics in the IPU3
>> + */
>> +void Agc::measureBrightness(const ipu3_uapi_stats_3a *stats,
>>                              const ipu3_uapi_grid_config &grid)
>>   {
>> +       /* Initialise the histogram array */
>>          uint32_t hist[knumHistogramBins] = { 0 };
>>   
>>          for (unsigned int cellY = 0; cellY < grid.height; cellY++) {
>> @@ -87,6 +122,11 @@ void Agc::processBrightness(const ipu3_uapi_stats_3a *stats,
>>                          if (cell->sat_ratio == 0) {
>>                                  uint8_t gr = cell->Gr_avg;
>>                                  uint8_t gb = cell->Gb_avg;
>> +                               /*
>> +                                * Store the average green value to estimate the
>> +                                * brightness. Even the over exposed pixels are
>> +                                * taken into account.
>> +                                */
>>                                  hist[(gr + gb) / 2]++;
>>                          }
>>                  }
>> @@ -96,6 +136,9 @@ void Agc::processBrightness(const ipu3_uapi_stats_3a *stats,
>>          iqMean_ = Histogram(Span<uint32_t>(hist)).interQuantileMean(0.98, 1.0);
>>   }
>>   
>> +/**
>> + * \brief Apply a filter on the exposure value to limit the speed of changes
>> + */
>>   void Agc::filterExposure()
>>   {
>>          double speed = 0.2;
>> @@ -106,7 +149,7 @@ void Agc::filterExposure()
>>                  /*
>>                   * If we are close to the desired result, go faster to avoid making
>>                   * multiple micro-adjustments.
>> -                * \ todo: Make this customisable?
>> +                * \todo: Make this customisable?
>>                   */
>>                  if (filteredExposure_ < 1.2 * currentExposure_ &&
>>                      filteredExposure_ > 0.8 * currentExposure_)
>> @@ -119,7 +162,12 @@ void Agc::filterExposure()
>>          LOG(IPU3Agc, Debug) << "After filtering, total_exposure " << filteredExposure_;
>>   }
>>   
>> -void Agc::lockExposureGain(uint32_t &exposure, double &analogueGain)
>> +/**
>> + * \brief Estimate the new exposure and gain values
>> + * \param[inout] exposure The exposure value reference as a number of lines
>> + * \param[inout] gain The gain reference to be updated
>> + */
>> +void Agc::computeExposure(uint32_t &exposure, double &analogueGain)
>>   {
>>          /* Algorithm initialization should wait for first valid frames */
>>          /* \todo - have a number of frames given by DelayedControls ?
>> @@ -136,19 +184,26 @@ void Agc::lockExposureGain(uint32_t &exposure, double &analogueGain)
>>                  return;
>>          }
>>   
>> +       /* Estimate the gain needed to have the proportion wanted */
>>          double evGain = kEvGainTarget * knumHistogramBins / iqMean_;
>>   
>>          /* extracted from Rpi::Agc::computeTargetExposure */
>> +       /* Calculate the shutter time in seconds */
>>          utils::Duration currentShutter = exposure * lineDuration_;
>>          LOG(IPU3Agc, Debug) << "Actual total exposure " << currentShutter * analogueGain
>>                              << " Shutter speed " << currentShutter
>>                              << " Gain " << analogueGain
>>                              << " Needed ev gain " << evGain;
>>   
>> +       /*
>> +        * Estimate the current exposure value for the scene as the latest
>> +        * exposure value applied multiplied by the new estimated gain
>> +        */
> 
> Does this estimate what the sensor is currently configured to expose?
> Or does it 'calculate' what is desired for the next control to set?
> 
> Estimate here implies that the algorithm is guessing what the sensor
> might be configured as (which I would hope we can 'know' or determine?)
> 
> Ok, the brief of this function clears up my question, as it says
> 'Estimate the new'.
> 
> Is 'estimate' intentional? I would interpret that we would 'calculate'
> the new values. Or in fact, given the function name, we might be
> 'computing' the new vaules...
> 
> 
>>          currentExposure_ = prevExposureValue_ * evGain;
>>          utils::Duration minShutterSpeed = minExposureLines_ * lineDuration_;
>>          utils::Duration maxShutterSpeed = maxExposureLines_ * lineDuration_;
>>   
>> +       /* Clamp the exposure value to the min and max authorized */
>>          utils::Duration maxTotalExposure = maxShutterSpeed * maxAnalogueGain_;
>>          currentExposure_ = std::min(currentExposure_, maxTotalExposure);
>>          LOG(IPU3Agc, Debug) << "Target total exposure " << currentExposure_
>> @@ -157,6 +212,7 @@ void Agc::lockExposureGain(uint32_t &exposure, double &analogueGain)
>>          /* \todo: estimate if we need to desaturate */
>>          filterExposure();
>>   
>> +       /* Divide the exposure value as new exposure and gain values */
>>          utils::Duration exposureValue = filteredExposure_;
>>          utils::Duration shutterTime = minShutterSpeed;
>>   
>> @@ -186,12 +242,21 @@ void Agc::lockExposureGain(uint32_t &exposure, double &analogueGain)
>>          prevExposureValue_ = shutterTime * analogueGain;
>>   }
>>   
>> +/**
>> + * \brief Process IPU3 statistics, and run AGC operations
>> + * \param[in] context The shared IPA context
>> + * \param[in] stats The IPU3 statistics and ISP results
>> + *
>> + * Identify the current image brightness, and use that to estimate the optimal
>> + * new exposure and gain for the scene.
> 
> Estimate or calculate? Estimate might be correct (here and above) if we
> don't know in a single iteration what values to choose, as I suspect
> it's somewhat of an adaptive algorithm that takes a couple of iterations
> to get to a desired goal.
> 
> If estimate is correct - then I think that covers my main concerns, or
> with an update to 'calculate' if that's more correct:
> 
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> 
>> + */
>>   void Agc::process(IPAContext &context, const ipu3_uapi_stats_3a *stats)
>>   {
>> +       /* Get the latest exposure and gain applied */
>>          uint32_t &exposure = context.frameContext.agc.exposure;
>>          double &analogueGain = context.frameContext.agc.gain;
>> -       processBrightness(stats, context.configuration.grid.bdsGrid);
>> -       lockExposureGain(exposure, analogueGain);
>> +       measureBrightness(stats, context.configuration.grid.bdsGrid);
>> +       computeExposure(exposure, analogueGain);
>>          frameCount_++;
>>   }
>>   
>> diff --git a/src/ipa/ipu3/algorithms/agc.h b/src/ipa/ipu3/algorithms/agc.h
>> index 1840205b..69e0b831 100644
>> --- a/src/ipa/ipu3/algorithms/agc.h
>> +++ b/src/ipa/ipu3/algorithms/agc.h
>> @@ -2,7 +2,7 @@
>>   /*
>>    * Copyright (C) 2021, Ideas On Board
>>    *
>> - * agc.h - IPU3 AGC/AEC control algorithm
>> + * agc.h - IPU3 AGC/AEC mean-based control algorithm
>>    */
>>   #ifndef __LIBCAMERA_IPU3_ALGORITHMS_AGC_H__
>>   #define __LIBCAMERA_IPU3_ALGORITHMS_AGC_H__
>> @@ -31,10 +31,10 @@ public:
>>          void process(IPAContext &context, const ipu3_uapi_stats_3a *stats) override;
>>   
>>   private:
>> -       void processBrightness(const ipu3_uapi_stats_3a *stats,
>> +       void measureBrightness(const ipu3_uapi_stats_3a *stats,
>>                                 const ipu3_uapi_grid_config &grid);
>>          void filterExposure();
>> -       void lockExposureGain(uint32_t &exposure, double &gain);
>> +       void computeExposure(uint32_t &exposure, double &gain);
>>   
>>          uint64_t frameCount_;
>>          uint64_t lastFrame_;
>> -- 
>> 2.32.0
>>

On 25/10/2021 23:26, Kieran Bingham wrote:
> Quoting Jean-Michel Hautbois (2021-10-22 16:12:06)
>> The AGC class was not documented while developing. Extend that to
>> reference the origins of the implementation, and improve the
>> descriptions on how the algorithm operates internally.
>>
>> While at it, rename the functions which have bad names.
> 
> Aha, these are just renames. It threw me off thinking there were bigger
> changes in here, but a rename is fine.
> 
>> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
>> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>>
>> ---
>> v3:
>> - rename processBrightness to measureBrightness
>> - rename lockExposureGain to computeExposure
>> - reword some comments
>> ---
>>   src/ipa/ipu3/algorithms/agc.cpp | 79 ++++++++++++++++++++++++++++++---
>>   src/ipa/ipu3/algorithms/agc.h   |  6 +--
>>   2 files changed, 75 insertions(+), 10 deletions(-)
>>
>> diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp
>> index 6c151232..b145e522 100644
>> --- a/src/ipa/ipu3/algorithms/agc.cpp
>> +++ b/src/ipa/ipu3/algorithms/agc.cpp
>> @@ -2,7 +2,7 @@
>>   /*
>>    * Copyright (C) 2021, Ideas On Board
>>    *
>> - * ipu3_agc.cpp - AGC/AEC control algorithm
>> + * ipu3_agc.cpp - AGC/AEC mean-based control algorithm
>>    */
>>   
>>   #include "agc.h"
>> @@ -17,17 +17,37 @@
>>   
>>   #include "libipa/histogram.h"
>>   
>> +/**
>> + * \file agc.h
>> + */
>> +
>>   namespace libcamera {
>>   
>>   using namespace std::literals::chrono_literals;
>>   
>>   namespace ipa::ipu3::algorithms {
>>   
>> +/**
>> + * \class Agc
>> + * \brief A mean-based auto-exposure algorithm
>> + *
>> + * This algorithm calculates a shutter time and an analogue gain so that the
> 
> Is it an analogue gain? or just a gain? Does the algorithm care if it is
> analogue?
> 
> Perhaps it does, as it's targetted to the Sensor controls.
> 
>> + * average value of the green channel of the brightest 2% of pixels approaches
>> + * 0.5. The AWB gains are not used here, and all cells in the grid have the same
>> + * weight, like an average-metering case. In this metering mode, the camera uses
>> + * light information from the entire scene and creates an average for the final
>> + * exposure setting, giving no weighting to any particular portion of the
>> + * metered area.
>> + *
>> + * Reference: Battiato, Messina & Castorina. (2008). Exposure
>> + * Correction for Imaging Devices: An Overview. 10.1201/9781420054538.ch12.
>> + */
>> +
>>   LOG_DEFINE_CATEGORY(IPU3Agc)
>>   
>>   /* Number of frames to wait before calculating stats on minimum exposure */
>>   static constexpr uint32_t kInitialFrameMinAECount = 4;
>> -/* Number of frames to wait between new gain/exposure estimations */
>> +/* Number of frames to wait between new gain/shutter time estimations */
>>   static constexpr uint32_t kFrameSkipCount = 6;
>>   
>>   /* Limits for analogue gain values */
>> @@ -36,6 +56,7 @@ static constexpr double kMaxAnalogueGain = 8.0;
>>   
>>   /* Histogram constants */
>>   static constexpr uint32_t knumHistogramBins = 256;
> 
> I'd have a new line here.
> 
>> +/* Target value to reach for the top 2% of the histogram */
> 
> Is this something that is expected to be tuned? or is 0.5 used to mean
> quite literally 'the middle' between 1 and 0?

It can be tuned later if we want to have low and high bounds. As we work 
with cumulative histograms, 0.5 is... half of the counted pixels.

Quoting "Raspberry Pi Camera Algorithm and Tuning Guide"
(https://datasheets.raspberrypi.com/camera/raspberry-pi-camera-guide.pdf):

"I(0.98, 1) = 0.5, lower bound. Here we are saying that the top 2% of 
the histogram must lie at or above 0.5 in the pixel range (metering all 
happens before gamma, so this is a moderately bright value). Or in 
short, we are requiring “some of the pixels to be reasonably bright”. 
This is a good strategy for snowy or beach scenes, also for images of 
documents. It actually raises the exposure of the whole scene; without 
it, the entire image would look dull grey which is undesirable in such 
circumstances. On the other hand when, as is usually the case, there is 
an abundance of bright and dark pixels, this constraint has no effect, 
so it is often reasonable to apply this constraint all the time."

> 
>>   static constexpr double kEvGainTarget = 0.5;
>>   
>>   Agc::Agc()
>> @@ -45,10 +66,18 @@ Agc::Agc()
>>   {
>>   }
>>   
>> +/**
>> + * \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, const IPAConfigInfo &configInfo)
>>   {
>>          stride_ = context.configuration.grid.stride;
>>   
>> +       /* \todo use the IPAContext to provide the limits */
>>          lineDuration_ = configInfo.sensorInfo.lineLength * 1.0s
>>                        / configInfo.sensorInfo.pixelRate;
>>   
>> @@ -70,9 +99,15 @@ int Agc::configure(IPAContext &context, const IPAConfigInfo &configInfo)
>>          return 0;
>>   }
>>   
>> -void Agc::processBrightness(const ipu3_uapi_stats_3a *stats,
>> +/**
>> + * \brief Estimate the mean value of the top 2% of the histogram
>> + * \param[in] stats The statistics computed by the ImgU
>> + * \param[in] grid The grid used to store the statistics in the IPU3
>> + */
>> +void Agc::measureBrightness(const ipu3_uapi_stats_3a *stats,
>>                              const ipu3_uapi_grid_config &grid)
>>   {
>> +       /* Initialise the histogram array */
>>          uint32_t hist[knumHistogramBins] = { 0 };
>>   
>>          for (unsigned int cellY = 0; cellY < grid.height; cellY++) {
>> @@ -87,6 +122,11 @@ void Agc::processBrightness(const ipu3_uapi_stats_3a *stats,
>>                          if (cell->sat_ratio == 0) {
>>                                  uint8_t gr = cell->Gr_avg;
>>                                  uint8_t gb = cell->Gb_avg;
>> +                               /*
>> +                                * Store the average green value to estimate the
>> +                                * brightness. Even the over exposed pixels are
>> +                                * taken into account.
>> +                                */
>>                                  hist[(gr + gb) / 2]++;
>>                          }
>>                  }
>> @@ -96,6 +136,9 @@ void Agc::processBrightness(const ipu3_uapi_stats_3a *stats,
>>          iqMean_ = Histogram(Span<uint32_t>(hist)).interQuantileMean(0.98, 1.0);
>>   }
>>   
>> +/**
>> + * \brief Apply a filter on the exposure value to limit the speed of changes
>> + */
>>   void Agc::filterExposure()
>>   {
>>          double speed = 0.2;
>> @@ -106,7 +149,7 @@ void Agc::filterExposure()
>>                  /*
>>                   * If we are close to the desired result, go faster to avoid making
>>                   * multiple micro-adjustments.
>> -                * \ todo: Make this customisable?
>> +                * \todo: Make this customisable?
>>                   */
>>                  if (filteredExposure_ < 1.2 * currentExposure_ &&
>>                      filteredExposure_ > 0.8 * currentExposure_)
>> @@ -119,7 +162,12 @@ void Agc::filterExposure()
>>          LOG(IPU3Agc, Debug) << "After filtering, total_exposure " << filteredExposure_;
>>   }
>>   
>> -void Agc::lockExposureGain(uint32_t &exposure, double &analogueGain)
>> +/**
>> + * \brief Estimate the new exposure and gain values
>> + * \param[inout] exposure The exposure value reference as a number of lines
>> + * \param[inout] gain The gain reference to be updated
>> + */
>> +void Agc::computeExposure(uint32_t &exposure, double &analogueGain)
>>   {
>>          /* Algorithm initialization should wait for first valid frames */
>>          /* \todo - have a number of frames given by DelayedControls ?
>> @@ -136,19 +184,26 @@ void Agc::lockExposureGain(uint32_t &exposure, double &analogueGain)
>>                  return;
>>          }
>>   
>> +       /* Estimate the gain needed to have the proportion wanted */
>>          double evGain = kEvGainTarget * knumHistogramBins / iqMean_;
>>   
>>          /* extracted from Rpi::Agc::computeTargetExposure */
>> +       /* Calculate the shutter time in seconds */
>>          utils::Duration currentShutter = exposure * lineDuration_;
>>          LOG(IPU3Agc, Debug) << "Actual total exposure " << currentShutter * analogueGain
>>                              << " Shutter speed " << currentShutter
>>                              << " Gain " << analogueGain
>>                              << " Needed ev gain " << evGain;
>>   
>> +       /*
>> +        * Estimate the current exposure value for the scene as the latest
>> +        * exposure value applied multiplied by the new estimated gain
>> +        */
> 
> Does this estimate what the sensor is currently configured to expose?
> Or does it 'calculate' what is desired for the next control to set?
> 
> Estimate here implies that the algorithm is guessing what the sensor
> might be configured as (which I would hope we can 'know' or determine?)
> 
> Ok, the brief of this function clears up my question, as it says
> 'Estimate the new'.
> 
> Is 'estimate' intentional? I would interpret that we would 'calculate'
> the new values. Or in fact, given the function name, we might be
> 'computing' the new vaules...

It might be calculate or compute then ;-).

> 
> 
>>          currentExposure_ = prevExposureValue_ * evGain;
>>          utils::Duration minShutterSpeed = minExposureLines_ * lineDuration_;
>>          utils::Duration maxShutterSpeed = maxExposureLines_ * lineDuration_;
>>   
>> +       /* Clamp the exposure value to the min and max authorized */
>>          utils::Duration maxTotalExposure = maxShutterSpeed * maxAnalogueGain_;
>>          currentExposure_ = std::min(currentExposure_, maxTotalExposure);
>>          LOG(IPU3Agc, Debug) << "Target total exposure " << currentExposure_
>> @@ -157,6 +212,7 @@ void Agc::lockExposureGain(uint32_t &exposure, double &analogueGain)
>>          /* \todo: estimate if we need to desaturate */
>>          filterExposure();
>>   
>> +       /* Divide the exposure value as new exposure and gain values */
>>          utils::Duration exposureValue = filteredExposure_;
>>          utils::Duration shutterTime = minShutterSpeed;
>>   
>> @@ -186,12 +242,21 @@ void Agc::lockExposureGain(uint32_t &exposure, double &analogueGain)
>>          prevExposureValue_ = shutterTime * analogueGain;
>>   }
>>   
>> +/**
>> + * \brief Process IPU3 statistics, and run AGC operations
>> + * \param[in] context The shared IPA context
>> + * \param[in] stats The IPU3 statistics and ISP results
>> + *
>> + * Identify the current image brightness, and use that to estimate the optimal
>> + * new exposure and gain for the scene.
> 
> Estimate or calculate? Estimate might be correct (here and above) if we
> don't know in a single iteration what values to choose, as I suspect
> it's somewhat of an adaptive algorithm that takes a couple of iterations
> to get to a desired goal.

In this case, it is more an estimation, I think, as it will apply new 
values, which will take some frames to have an effect, and will 
re-estimate the exposure/gain...

When we will have the IPAFrameContext implemented with a queue and the 
"real" exposure and gain applied on the frame we are looking at, then it 
will be more a calculation, I guess :-).

> 
> If estimate is correct - then I think that covers my main concerns, or
> with an update to 'calculate' if that's more correct:
> 
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> 
>> + */
>>   void Agc::process(IPAContext &context, const ipu3_uapi_stats_3a *stats)
>>   {
>> +       /* Get the latest exposure and gain applied */
>>          uint32_t &exposure = context.frameContext.agc.exposure;
>>          double &analogueGain = context.frameContext.agc.gain;
>> -       processBrightness(stats, context.configuration.grid.bdsGrid);
>> -       lockExposureGain(exposure, analogueGain);
>> +       measureBrightness(stats, context.configuration.grid.bdsGrid);
>> +       computeExposure(exposure, analogueGain);
>>          frameCount_++;
>>   }
>>   
>> diff --git a/src/ipa/ipu3/algorithms/agc.h b/src/ipa/ipu3/algorithms/agc.h
>> index 1840205b..69e0b831 100644
>> --- a/src/ipa/ipu3/algorithms/agc.h
>> +++ b/src/ipa/ipu3/algorithms/agc.h
>> @@ -2,7 +2,7 @@
>>   /*
>>    * Copyright (C) 2021, Ideas On Board
>>    *
>> - * agc.h - IPU3 AGC/AEC control algorithm
>> + * agc.h - IPU3 AGC/AEC mean-based control algorithm
>>    */
>>   #ifndef __LIBCAMERA_IPU3_ALGORITHMS_AGC_H__
>>   #define __LIBCAMERA_IPU3_ALGORITHMS_AGC_H__
>> @@ -31,10 +31,10 @@ public:
>>          void process(IPAContext &context, const ipu3_uapi_stats_3a *stats) override;
>>   
>>   private:
>> -       void processBrightness(const ipu3_uapi_stats_3a *stats,
>> +       void measureBrightness(const ipu3_uapi_stats_3a *stats,
>>                                 const ipu3_uapi_grid_config &grid);
>>          void filterExposure();
>> -       void lockExposureGain(uint32_t &exposure, double &gain);
>> +       void computeExposure(uint32_t &exposure, double &gain);
>>   
>>          uint64_t frameCount_;
>>          uint64_t lastFrame_;
>> -- 
>> 2.32.0
>>

Quoting Jean-Michel Hautbois (2021-10-26 07:23:45)
> 
> 
> On 25/10/2021 23:26, Kieran Bingham wrote:
> > Quoting Jean-Michel Hautbois (2021-10-22 16:12:06)
> >> The AGC class was not documented while developing. Extend that to
> >> reference the origins of the implementation, and improve the
> >> descriptions on how the algorithm operates internally.
> >>
> >> While at it, rename the functions which have bad names.
> > 
> > Aha, these are just renames. It threw me off thinking there were bigger
> > changes in here, but a rename is fine.
> > 
> >> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
> >> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> >>
> >> ---
> >> v3:
> >> - rename processBrightness to measureBrightness
> >> - rename lockExposureGain to computeExposure
> >> - reword some comments
> >> ---
> >>   src/ipa/ipu3/algorithms/agc.cpp | 79 ++++++++++++++++++++++++++++++---
> >>   src/ipa/ipu3/algorithms/agc.h   |  6 +--
> >>   2 files changed, 75 insertions(+), 10 deletions(-)
> >>
> >> diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp
> >> index 6c151232..b145e522 100644
> >> --- a/src/ipa/ipu3/algorithms/agc.cpp
> >> +++ b/src/ipa/ipu3/algorithms/agc.cpp
> >> @@ -2,7 +2,7 @@
> >>   /*
> >>    * Copyright (C) 2021, Ideas On Board
> >>    *
> >> - * ipu3_agc.cpp - AGC/AEC control algorithm
> >> + * ipu3_agc.cpp - AGC/AEC mean-based control algorithm
> >>    */
> >>   
> >>   #include "agc.h"
> >> @@ -17,17 +17,37 @@
> >>   
> >>   #include "libipa/histogram.h"
> >>   
> >> +/**
> >> + * \file agc.h
> >> + */
> >> +
> >>   namespace libcamera {
> >>   
> >>   using namespace std::literals::chrono_literals;
> >>   
> >>   namespace ipa::ipu3::algorithms {
> >>   
> >> +/**
> >> + * \class Agc
> >> + * \brief A mean-based auto-exposure algorithm
> >> + *
> >> + * This algorithm calculates a shutter time and an analogue gain so that the
> > 
> > Is it an analogue gain? or just a gain? Does the algorithm care if it is
> > analogue?
> > 
> > Perhaps it does, as it's targetted to the Sensor controls.
> > 
> >> + * average value of the green channel of the brightest 2% of pixels approaches
> >> + * 0.5. The AWB gains are not used here, and all cells in the grid have the same
> >> + * weight, like an average-metering case. In this metering mode, the camera uses
> >> + * light information from the entire scene and creates an average for the final
> >> + * exposure setting, giving no weighting to any particular portion of the
> >> + * metered area.
> >> + *
> >> + * Reference: Battiato, Messina & Castorina. (2008). Exposure
> >> + * Correction for Imaging Devices: An Overview. 10.1201/9781420054538.ch12.
> >> + */
> >> +
> >>   LOG_DEFINE_CATEGORY(IPU3Agc)
> >>   
> >>   /* Number of frames to wait before calculating stats on minimum exposure */
> >>   static constexpr uint32_t kInitialFrameMinAECount = 4;
> >> -/* Number of frames to wait between new gain/exposure estimations */
> >> +/* Number of frames to wait between new gain/shutter time estimations */
> >>   static constexpr uint32_t kFrameSkipCount = 6;
> >>   
> >>   /* Limits for analogue gain values */
> >> @@ -36,6 +56,7 @@ static constexpr double kMaxAnalogueGain = 8.0;
> >>   
> >>   /* Histogram constants */
> >>   static constexpr uint32_t knumHistogramBins = 256;
> > 
> > I'd have a new line here.
> > 
> >> +/* Target value to reach for the top 2% of the histogram */
> > 
> > Is this something that is expected to be tuned? or is 0.5 used to mean
> > quite literally 'the middle' between 1 and 0?
> 
> It can be tuned later if we want to have low and high bounds. As we work 
> with cumulative histograms, 0.5 is... half of the counted pixels.
> 
> Quoting "Raspberry Pi Camera Algorithm and Tuning Guide"
> (https://datasheets.raspberrypi.com/camera/raspberry-pi-camera-guide.pdf):
> 
> "I(0.98, 1) = 0.5, lower bound. Here we are saying that the top 2% of 
> the histogram must lie at or above 0.5 in the pixel range (metering all 
> happens before gamma, so this is a moderately bright value). Or in 
> short, we are requiring “some of the pixels to be reasonably bright”. 
> This is a good strategy for snowy or beach scenes, also for images of 
> documents. It actually raises the exposure of the whole scene; without 
> it, the entire image would look dull grey which is undesirable in such 
> circumstances. On the other hand when, as is usually the case, there is 
> an abundance of bright and dark pixels, this constraint has no effect, 
> so it is often reasonable to apply this constraint all the time."

That sounds like important information to convey along with this
variable, but should probably be sumarised a bit.

I don't mind if this is handled later though on top of this series as
it's already quite large, and nearing integration.

Could you make a note to revisit this later please?


> >>   static constexpr double kEvGainTarget = 0.5;
> >>   
> >>   Agc::Agc()
> >> @@ -45,10 +66,18 @@ Agc::Agc()
> >>   {
> >>   }
> >>   
> >> +/**
> >> + * \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, const IPAConfigInfo &configInfo)
> >>   {
> >>          stride_ = context.configuration.grid.stride;
> >>   
> >> +       /* \todo use the IPAContext to provide the limits */
> >>          lineDuration_ = configInfo.sensorInfo.lineLength * 1.0s
> >>                        / configInfo.sensorInfo.pixelRate;
> >>   
> >> @@ -70,9 +99,15 @@ int Agc::configure(IPAContext &context, const IPAConfigInfo &configInfo)
> >>          return 0;
> >>   }
> >>   
> >> -void Agc::processBrightness(const ipu3_uapi_stats_3a *stats,
> >> +/**
> >> + * \brief Estimate the mean value of the top 2% of the histogram
> >> + * \param[in] stats The statistics computed by the ImgU
> >> + * \param[in] grid The grid used to store the statistics in the IPU3
> >> + */
> >> +void Agc::measureBrightness(const ipu3_uapi_stats_3a *stats,
> >>                              const ipu3_uapi_grid_config &grid)
> >>   {
> >> +       /* Initialise the histogram array */
> >>          uint32_t hist[knumHistogramBins] = { 0 };
> >>   
> >>          for (unsigned int cellY = 0; cellY < grid.height; cellY++) {
> >> @@ -87,6 +122,11 @@ void Agc::processBrightness(const ipu3_uapi_stats_3a *stats,
> >>                          if (cell->sat_ratio == 0) {
> >>                                  uint8_t gr = cell->Gr_avg;
> >>                                  uint8_t gb = cell->Gb_avg;
> >> +                               /*
> >> +                                * Store the average green value to estimate the
> >> +                                * brightness. Even the over exposed pixels are
> >> +                                * taken into account.
> >> +                                */
> >>                                  hist[(gr + gb) / 2]++;
> >>                          }
> >>                  }
> >> @@ -96,6 +136,9 @@ void Agc::processBrightness(const ipu3_uapi_stats_3a *stats,
> >>          iqMean_ = Histogram(Span<uint32_t>(hist)).interQuantileMean(0.98, 1.0);
> >>   }
> >>   
> >> +/**
> >> + * \brief Apply a filter on the exposure value to limit the speed of changes
> >> + */
> >>   void Agc::filterExposure()
> >>   {
> >>          double speed = 0.2;
> >> @@ -106,7 +149,7 @@ void Agc::filterExposure()
> >>                  /*
> >>                   * If we are close to the desired result, go faster to avoid making
> >>                   * multiple micro-adjustments.
> >> -                * \ todo: Make this customisable?
> >> +                * \todo: Make this customisable?
> >>                   */
> >>                  if (filteredExposure_ < 1.2 * currentExposure_ &&
> >>                      filteredExposure_ > 0.8 * currentExposure_)
> >> @@ -119,7 +162,12 @@ void Agc::filterExposure()
> >>          LOG(IPU3Agc, Debug) << "After filtering, total_exposure " << filteredExposure_;
> >>   }
> >>   
> >> -void Agc::lockExposureGain(uint32_t &exposure, double &analogueGain)
> >> +/**
> >> + * \brief Estimate the new exposure and gain values
> >> + * \param[inout] exposure The exposure value reference as a number of lines
> >> + * \param[inout] gain The gain reference to be updated
> >> + */
> >> +void Agc::computeExposure(uint32_t &exposure, double &analogueGain)
> >>   {
> >>          /* Algorithm initialization should wait for first valid frames */
> >>          /* \todo - have a number of frames given by DelayedControls ?
> >> @@ -136,19 +184,26 @@ void Agc::lockExposureGain(uint32_t &exposure, double &analogueGain)
> >>                  return;
> >>          }
> >>   
> >> +       /* Estimate the gain needed to have the proportion wanted */
> >>          double evGain = kEvGainTarget * knumHistogramBins / iqMean_;
> >>   
> >>          /* extracted from Rpi::Agc::computeTargetExposure */
> >> +       /* Calculate the shutter time in seconds */
> >>          utils::Duration currentShutter = exposure * lineDuration_;
> >>          LOG(IPU3Agc, Debug) << "Actual total exposure " << currentShutter * analogueGain
> >>                              << " Shutter speed " << currentShutter
> >>                              << " Gain " << analogueGain
> >>                              << " Needed ev gain " << evGain;
> >>   
> >> +       /*
> >> +        * Estimate the current exposure value for the scene as the latest
> >> +        * exposure value applied multiplied by the new estimated gain
> >> +        */
> > 
> > Does this estimate what the sensor is currently configured to expose?
> > Or does it 'calculate' what is desired for the next control to set?
> > 
> > Estimate here implies that the algorithm is guessing what the sensor
> > might be configured as (which I would hope we can 'know' or determine?)
> > 
> > Ok, the brief of this function clears up my question, as it says
> > 'Estimate the new'.
> > 
> > Is 'estimate' intentional? I would interpret that we would 'calculate'
> > the new values. Or in fact, given the function name, we might be
> > 'computing' the new vaules...
> 
> It might be calculate or compute then ;-).
> 
> > 
> > 
> >>          currentExposure_ = prevExposureValue_ * evGain;
> >>          utils::Duration minShutterSpeed = minExposureLines_ * lineDuration_;
> >>          utils::Duration maxShutterSpeed = maxExposureLines_ * lineDuration_;
> >>   
> >> +       /* Clamp the exposure value to the min and max authorized */
> >>          utils::Duration maxTotalExposure = maxShutterSpeed * maxAnalogueGain_;
> >>          currentExposure_ = std::min(currentExposure_, maxTotalExposure);
> >>          LOG(IPU3Agc, Debug) << "Target total exposure " << currentExposure_
> >> @@ -157,6 +212,7 @@ void Agc::lockExposureGain(uint32_t &exposure, double &analogueGain)
> >>          /* \todo: estimate if we need to desaturate */
> >>          filterExposure();
> >>   
> >> +       /* Divide the exposure value as new exposure and gain values */
> >>          utils::Duration exposureValue = filteredExposure_;
> >>          utils::Duration shutterTime = minShutterSpeed;
> >>   
> >> @@ -186,12 +242,21 @@ void Agc::lockExposureGain(uint32_t &exposure, double &analogueGain)
> >>          prevExposureValue_ = shutterTime * analogueGain;
> >>   }
> >>   
> >> +/**
> >> + * \brief Process IPU3 statistics, and run AGC operations
> >> + * \param[in] context The shared IPA context
> >> + * \param[in] stats The IPU3 statistics and ISP results
> >> + *
> >> + * Identify the current image brightness, and use that to estimate the optimal
> >> + * new exposure and gain for the scene.
> > 
> > Estimate or calculate? Estimate might be correct (here and above) if we
> > don't know in a single iteration what values to choose, as I suspect
> > it's somewhat of an adaptive algorithm that takes a couple of iterations
> > to get to a desired goal.
> 
> In this case, it is more an estimation, I think, as it will apply new 
> values, which will take some frames to have an effect, and will 
> re-estimate the exposure/gain...
> 
> When we will have the IPAFrameContext implemented with a queue and the 
> "real" exposure and gain applied on the frame we are looking at, then it 
> will be more a calculation, I guess :-).

Ok - I think it sounds like we're shooting ourselves in the foot by not
keeping a queue of FrameContexts that we've sent out. That should not be
difficult to implement, so lets tackle that after integrating this
series.

> > If estimate is correct - then I think that covers my main concerns, or
> > with an update to 'calculate' if that's more correct:
> > 
> > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > 
> >> + */
> >>   void Agc::process(IPAContext &context, const ipu3_uapi_stats_3a *stats)
> >>   {
> >> +       /* Get the latest exposure and gain applied */
> >>          uint32_t &exposure = context.frameContext.agc.exposure;
> >>          double &analogueGain = context.frameContext.agc.gain;
> >> -       processBrightness(stats, context.configuration.grid.bdsGrid);
> >> -       lockExposureGain(exposure, analogueGain);
> >> +       measureBrightness(stats, context.configuration.grid.bdsGrid);
> >> +       computeExposure(exposure, analogueGain);
> >>          frameCount_++;
> >>   }
> >>   
> >> diff --git a/src/ipa/ipu3/algorithms/agc.h b/src/ipa/ipu3/algorithms/agc.h
> >> index 1840205b..69e0b831 100644
> >> --- a/src/ipa/ipu3/algorithms/agc.h
> >> +++ b/src/ipa/ipu3/algorithms/agc.h
> >> @@ -2,7 +2,7 @@
> >>   /*
> >>    * Copyright (C) 2021, Ideas On Board
> >>    *
> >> - * agc.h - IPU3 AGC/AEC control algorithm
> >> + * agc.h - IPU3 AGC/AEC mean-based control algorithm
> >>    */
> >>   #ifndef __LIBCAMERA_IPU3_ALGORITHMS_AGC_H__
> >>   #define __LIBCAMERA_IPU3_ALGORITHMS_AGC_H__
> >> @@ -31,10 +31,10 @@ public:
> >>          void process(IPAContext &context, const ipu3_uapi_stats_3a *stats) override;
> >>   
> >>   private:
> >> -       void processBrightness(const ipu3_uapi_stats_3a *stats,
> >> +       void measureBrightness(const ipu3_uapi_stats_3a *stats,
> >>                                 const ipu3_uapi_grid_config &grid);
> >>          void filterExposure();
> >> -       void lockExposureGain(uint32_t &exposure, double &gain);
> >> +       void computeExposure(uint32_t &exposure, double &gain);
> >>   
> >>          uint64_t frameCount_;
> >>          uint64_t lastFrame_;
> >> -- 
> >> 2.32.0
> >>

Quoting Jean-Michel Hautbois (2021-10-26 07:13:17)
> Hi Kieran,
> 
> On 25/10/2021 23:26, Kieran Bingham wrote:
> > Quoting Jean-Michel Hautbois (2021-10-22 16:12:06)
> >> The AGC class was not documented while developing. Extend that to
> >> reference the origins of the implementation, and improve the
> >> descriptions on how the algorithm operates internally.
> >>
> >> While at it, rename the functions which have bad names.
> > 
> > Aha, these are just renames. It threw me off thinking there were bigger
> > changes in here, but a rename is fine.
> > 
> >> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
> >> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> >>
> >> ---
> >> v3:
> >> - rename processBrightness to measureBrightness
> >> - rename lockExposureGain to computeExposure
> >> - reword some comments
> >> ---
> >>   src/ipa/ipu3/algorithms/agc.cpp | 79 ++++++++++++++++++++++++++++++---
> >>   src/ipa/ipu3/algorithms/agc.h   |  6 +--
> >>   2 files changed, 75 insertions(+), 10 deletions(-)
> >>
> >> diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp
> >> index 6c151232..b145e522 100644
> >> --- a/src/ipa/ipu3/algorithms/agc.cpp
> >> +++ b/src/ipa/ipu3/algorithms/agc.cpp
> >> @@ -2,7 +2,7 @@
> >>   /*
> >>    * Copyright (C) 2021, Ideas On Board
> >>    *
> >> - * ipu3_agc.cpp - AGC/AEC control algorithm
> >> + * ipu3_agc.cpp - AGC/AEC mean-based control algorithm
> >>    */
> >>   
> >>   #include "agc.h"
> >> @@ -17,17 +17,37 @@
> >>   
> >>   #include "libipa/histogram.h"
> >>   
> >> +/**
> >> + * \file agc.h
> >> + */
> >> +
> >>   namespace libcamera {
> >>   
> >>   using namespace std::literals::chrono_literals;
> >>   
> >>   namespace ipa::ipu3::algorithms {
> >>   
> >> +/**
> >> + * \class Agc
> >> + * \brief A mean-based auto-exposure algorithm
> >> + *
> >> + * This algorithm calculates a shutter time and an analogue gain so that the
> > 
> > Is it an analogue gain? or just a gain? Does the algorithm care if it is
> > analogue?
> > 
> > Perhaps it does, as it's targetted to the Sensor controls.
> 
> Quoting you and Laurent in v1:
> 
> Kieran:
> Surely the algorithm itself doesn't care if this is an analogue gain or 
> a digital gain.
> 
> Laurent:
> Analogue gain is applied in the sensor, before the ADC, while digital
> gain can be applied anywhere after the ADC. Most sensors will have a
> digital gain, which is usually unused, and ISPs also have digital gains.
> It's important to split gain between analogue and digital correctly as
> they have different implications (in terms of noise for instance), so
> the algorithm needs to care.
> 
> Kieran:
> The IPU3 will apply it as an analogue gain - but that's independent 
> isn't it ?
> 
> Laurent:
> If you meant the IPU3 IPA, yes, it applies the gain as analogue gain. If
> you meant the IPU3 hardware, no, it has no analogue gain.
> 
> Kieran:
> Presumably if we weren't able to apply it fully as analogue - the 
> remainder could be applied digitally to meet the desired gain requested 
>   by the algorithm?
> 
> Laurent:
> That's usually what a basic implementation will do, there could be
> reason to do something more complex.
> 
> Me:
> We apply analogue gains right now and should take care of digital gain 
> later :-).

Ok, lets leave this all as is then. Thanks for the refresh.

(perhaps in the future we'll have a way to maintain discussion history
alongside progressive patch versions...)

--
Kieran