[libcamera-devel] [PATCH v2 3/4] ipa: ipu3: Document AGC mean-based algorithm

[Laurent Pinchart]

Hi Jean-Michel,

Thank you for the patch.

On Fri, Aug 27, 2021 at 10:02:26AM +0200, Jean-Michel Hautbois wrote:
> The AGC mean-based algorithm is partially documented. Improve the
> documentation to help understand how it works and mark some \todo for
> future improvements.
> 
> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois at ideasonboard.com>
> ---
>  src/ipa/ipu3/algorithms/agc_mean.cpp | 91 +++++++++++++++++++++++++---
>  src/ipa/ipu3/algorithms/agc_mean.h   |  2 +-
>  2 files changed, 84 insertions(+), 9 deletions(-)
> 
> diff --git a/src/ipa/ipu3/algorithms/agc_mean.cpp b/src/ipa/ipu3/algorithms/agc_mean.cpp
> index 193f6e9a..b535e14b 100644
> --- a/src/ipa/ipu3/algorithms/agc_mean.cpp
> +++ b/src/ipa/ipu3/algorithms/agc_mean.cpp
> @@ -2,7 +2,7 @@
>  /*
>   * Copyright (C) 2021, Ideas On Board
>   *
> - * agc_mean.cpp - AGC/AEC control algorithm
> + * agc_mean.cpp - AGC/AEC mean-based control algorithm

This should go to the patch 2/4.

>   */
>  
>  #include "agc_mean.h"
> @@ -17,12 +17,26 @@
>  
>  #include "libipa/histogram.h"
>  
> +/**
> + * \file agc_mean.h
> + */
> +
>  namespace libcamera {
>  
>  using namespace std::literals::chrono_literals;
>  
>  namespace ipa::ipu3::algorithms {
>  
> +/**
> + * \class AgMean
> + * \brief The class to use the mean-based auto-exposure algorithm
> + *
> + * The mean-based algorithm is calculating an exposure and gain value such as

s/such as/such that/

> + * a given quantity of pixels lie in the top 2% of the histogram. The AWB gains

Is that right ? I could be wrong, but I understand it as setting
exposure and gain such that the mean value of the 2% brightest pixels
(the top 2% of the histogram) equals a fixed target.

> + * are not used here, and all cells in the grid have the same weight, like an
> + * average-metering case.
> + */
> +
>  LOG_DEFINE_CATEGORY(IPU3AgcMean)
>  
>  /* Number of frames to wait before calculating stats on minimum exposure */
> @@ -30,13 +44,20 @@ static constexpr uint32_t kInitialFrameMinAECount = 4;
>  /* Number of frames to wait between new gain/exposure estimations */
>  static constexpr uint32_t kFrameSkipCount = 6;
>  
> -/* Maximum ISO value for analogue gain */
> +/* Minimum ISO value for analogue gain (no digital gain supported) */
>  static constexpr uint32_t kMinISO = 100;
> +/* Maximum ISO value for analogue gain (no digital gain supported) */
>  static constexpr uint32_t kMaxISO = 1500;

Even better, drop those (as they're no ISO values) and set kMinGain and
kMaxGain to 1 and 15 respectively.

>  
> -/* Maximum analogue gain value
> - * \todo grab it from a camera helper */
> +/*
> + * Minimum analogue gain value
> + * \todo grab it from a camera helper
> + */
>  static constexpr uint32_t kMinGain = kMinISO / 100;
> +/*
> + * Maximum analogue gain value
> + * \todo grab it from a camera helper
> + */
>  static constexpr uint32_t kMaxGain = kMaxISO / 100;
>  
>  /* \todo use calculated value based on sensor */
> @@ -45,6 +66,7 @@ static constexpr uint32_t kMaxExposure = 1976;
>  
>  /* 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;
>  
>  /* A cell is 8 bytes and contains averages for RGB values and saturation ratio */
> @@ -57,8 +79,18 @@ AgcMean::AgcMean()
>  {
>  }
>  
> +/**
> + * \brief Configure the AGC given a configInfo
> + * \param[in] context The shared IPA context (\todo not used yet)

Drop the todo, it's clear from the [[maybe_unused]]

> + * \param[in] configInfo The IPA configuration data, received from the pipeline
> + * handler

>From the point of view of the algorithm it's irrelevant where the
configuration data comes from, you can drop the second part of the
sentence.

> + *
> + * \return 0
> + */
>  int AgcMean::configure([[maybe_unused]] IPAContext &context,
> -		        const IPAConfigInfo &configInfo){
> +		        const IPAConfigInfo &configInfo)
> +{
> +	/* \todo use the configInfo fields and IPAContext to store the limits */
>  	lineDuration_ = configInfo.sensorInfo.lineLength * 1.0s
>  		      / configInfo.sensorInfo.pixelRate;
>  	maxExposureTime_ = kMaxExposure * lineDuration_;
> @@ -66,9 +98,22 @@ int AgcMean::configure([[maybe_unused]] IPAContext &context,
>  	return 0;
>  }
>  
> +/**
> + * \brief Estimate the mean quantile of the top 2% of the histogram
> + * \param[in] stats The statistics buffer coming from the pipeline handler

Same here, where it comes from isn't relevant, you can write "The
statistics computed by the ImgU" for instance.

> + * \param[in] grid The grid used to store the statistics in the IPU3
> + */
>  void AgcMean::processBrightness(const ipu3_uapi_stats_3a *stats,
>  			    const ipu3_uapi_grid_config &grid)
>  {
> +	/*
> +	 * Get the applied grid from the statistics buffer. When the kernel
> +	 * receives a grid from the parameters buffer, it will check and align
> +	 * all the values. For instance, it will automatically fill the x_end
> +	 * value based on x_start, grid width and log2 width.
> +	 * \todo Use the grid calculated in configure as there is a bug in IPU3
> +	 * causing the width (maybe height) to be bit-shifted.
> +	 */
>  	const struct ipu3_uapi_grid_config statsAeGrid = stats->stats_4a_config.awb_config.grid;
>  	Rectangle aeRegion = { statsAeGrid.x_start,
>  			       statsAeGrid.y_start,
> @@ -85,6 +130,7 @@ void AgcMean::processBrightness(const ipu3_uapi_stats_3a *stats,
>  	uint32_t i, j;
>  	uint32_t count = 0;
>  
> +	/* Initialise the histogram array */
>  	uint32_t hist[knumHistogramBins] = { 0 };
>  	for (j = topleftY;
>  	     j < topleftY + (aeRegion.size().height >> grid.block_height_log2);
> @@ -92,12 +138,18 @@ void AgcMean::processBrightness(const ipu3_uapi_stats_3a *stats,
>  		for (i = startX + startY; i < endX + startY; i += kCellSize) {
>  			/*
>  			 * The grid width (and maybe height) is not reliable.
> -			 * We observed a bit shift which makes the value 160 to be 32 in the stats grid.
> -			 * Use the one passed at init time.
> +			 * We observed a bit shift which makes the value 160 to
> +			 * be 32 in the stats grid. Use the one from configure.
>  			 */
>  			if (stats->awb_raw_buffer.meta_data[i + 4 + j * grid.width] == 0) {
>  				uint8_t Gr = stats->awb_raw_buffer.meta_data[i + 0 + j * grid.width];
>  				uint8_t Gb = stats->awb_raw_buffer.meta_data[i + 3 + j * grid.width];
> +				/*
> +				 * Store the average green value to estimate the
> +				 * brightness. Even the over exposed pixels are
> +				 * taken into account.
> +				 * \todo remove count which is not used.
> +				 */
>  				hist[(Gr + Gb) / 2]++;
>  				count++;
>  			}
> @@ -108,11 +160,14 @@ void AgcMean::processBrightness(const ipu3_uapi_stats_3a *stats,

While at it, there's a comment here that states

	/* Estimate the quantile mean of the top 2% of the histogram */

I think that's misleading, shouldn't it be

	/* Estimate the mean of the top 2% of the histogram */

?

>  	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 AgcMean::filterExposure()
>  {
>  	double speed = 0.2;
>  	if (prevExposure_ == 0s) {
> -		/* DG stands for digital gain.*/
> +		/* DG stands for digital gain, which is always 1.0 for now. */
>  		prevExposure_ = currentExposure_;
>  		prevExposureNoDg_ = currentExposureNoDg_;
>  	} else {
> @@ -134,6 +189,7 @@ void AgcMean::filterExposure()
>  	 * We can't let the no_dg exposure deviate too far below the
>  	 * total exposure, as there might not be enough digital gain available
>  	 * in the ISP to hide it (which will cause nasty oscillation).
> +	 * \todo add the digital gain usage
>  	 */
>  	double fastReduceThreshold = 0.4;
>  	if (prevExposureNoDg_ <
> @@ -142,6 +198,11 @@ void AgcMean::filterExposure()
>  	LOG(IPU3AgcMean, Debug) << "After filtering, total_exposure " << prevExposure_;
>  }
>  
> +/**
> + * \brief Estimate the new exposure and gain values
> + * \param[in] exposure The exposure value reference as a number of lines
> + * \param[in] gain The gain reference to be updated
> + */
>  void AgcMean::lockExposureGain(uint32_t &exposure, double &gain)
>  {
>  	/* Algorithm initialization should wait for first valid frames */
> @@ -154,15 +215,20 @@ void AgcMean::lockExposureGain(uint32_t &exposure, double &gain)
>  	if (std::abs(iqMean_ - kEvGainTarget * knumHistogramBins) <= 1) {
>  		LOG(IPU3AgcMean, Debug) << "!!! Good exposure with iqMean = " << iqMean_;
>  	} else {
> +		/* Estimate the gain needed to have the proportion wanted */
>  		double newGain = kEvGainTarget * knumHistogramBins / iqMean_;
>  
>  		/* extracted from Rpi::Agc::computeTargetExposure */
> +		/* Calculate the shutter time in seconds */
>  		libcamera::utils::Duration currentShutter = exposure * lineDuration_;
> +		/* Ev = shutter_time * gain */
>  		currentExposureNoDg_ = currentShutter * gain;
>  		LOG(IPU3AgcMean, Debug) << "Actual total exposure " << currentExposureNoDg_
>  				    << " Shutter speed " << currentShutter
>  				    << " Gain " << gain;
> +		/* Apply the gain calculated to the current exposure value */
>  		currentExposure_ = currentExposureNoDg_ * newGain;
> +		/* Clamp the exposure value to the min and max authorized */
>  		libcamera::utils::Duration maxTotalExposure = maxExposureTime_ * kMaxGain;
>  		currentExposure_ = std::min(currentExposure_, maxTotalExposure);
>  		LOG(IPU3AgcMean, Debug) << "Target total exposure " << currentExposure_;
> @@ -170,6 +236,7 @@ void AgcMean::lockExposureGain(uint32_t &exposure, double &gain)
>  		/* \todo: estimate if we need to desaturate */
>  		filterExposure();
>  
> +		/* Divide the exposure value as new exposure and gain values */
>  		libcamera::utils::Duration newExposure = 0.0s;
>  		if (currentShutter < maxExposureTime_) {
>  			exposure = std::clamp(static_cast<uint32_t>(exposure * currentExposure_ / currentExposureNoDg_), kMinExposure, kMaxExposure);
> @@ -185,11 +252,19 @@ void AgcMean::lockExposureGain(uint32_t &exposure, double &gain)
>  	lastFrame_ = frameCount_;
>  }
>  
> +/**
> + * \brief Process IPU3 statistics, and run AGC operations
> + * \param[in] context The shared IPA context
> + * \param[in] stats The IPU3 statistics and ISP results
> + */
>  void AgcMean::process(IPAContext &context, const ipu3_uapi_stats_3a *stats)
>  {
> +	/* Get the latest exposure and gain applied */
>  	uint32_t &exposure = context.frameContext.agc.exposure;
>  	double &gain = context.frameContext.agc.gain;
> +	/* Calculate the current brightness */
>  	processBrightness(stats, context.configuration.grid.bdsGrid);
> +	/* Update the exposure and gain values */
>  	lockExposureGain(exposure, gain);
>  	frameCount_++;
>  }
> diff --git a/src/ipa/ipu3/algorithms/agc_mean.h b/src/ipa/ipu3/algorithms/agc_mean.h
> index 97114121..6232597d 100644
> --- a/src/ipa/ipu3/algorithms/agc_mean.h
> +++ b/src/ipa/ipu3/algorithms/agc_mean.h
> @@ -2,7 +2,7 @@
>  /*
>   * Copyright (C) 2021, Ideas On Board
>   *
> - * agc_mean.h - IPU3 AGC/AEC control algorithm
> + * agc_mean.h - AGC/AEC mean-based control algorithm

This should go to patch 2/4 too.

>   */
>  #ifndef __LIBCAMERA_IPU3_ALGORITHMS_AGC_H__
>  #define __LIBCAMERA_IPU3_ALGORITHMS_AGC_H__

-- 
Regards,

Laurent Pinchart