[libcamera-devel] [PATCH 09/11] ipa: ipu3: agc: Document AGC mean-based algorithm
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Tue Sep 14 07:21:44 CEST 2021
Hi Jean-Michel,
Thank you for the patch.
On Mon, Sep 13, 2021 at 04:58:08PM +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.
>
> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois at ideasonboard.com>
> Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> ---
> src/ipa/ipu3/algorithms/agc.cpp | 116 +++++++++++++++++++++++++++-----
> src/ipa/ipu3/algorithms/agc.h | 2 +-
> 2 files changed, 101 insertions(+), 17 deletions(-)
>
> diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp
> index 2ef998b7..9a55bb72 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,27 +17,49 @@
>
> #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 a gain so that the average value
s/a gain/an analogue gain/
> + * 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;
>
> -/* Maximum ISO value for analogue gain */
> -static constexpr uint32_t kMinISO = 100;
> -static constexpr uint32_t kMaxISO = 1500;
> -
> -/* Maximum analogue gain value
> - * \todo grab it from a camera helper */
> -static constexpr uint32_t kMinGain = kMinISO / 100;
> -static constexpr uint32_t kMaxGain = kMaxISO / 100;
> +/*
> + * Minimum analogue gain value
> + * \todo grab it from a camera helper
> + */
> +static constexpr uint32_t kMinGain = 1;
> +/*
> + * Maximum analogue gain value
> + * \todo grab it from a camera helper
> + */
> +static constexpr uint32_t kMaxGain = 15;
I was about to just comment that these two values should be
floating-point numbers, but then wondered how and why they were used as
integers in the code. I recommend trying the same exercise, it's
"interesting" (and a bug fix is clearly needed).
> /* \todo use calculated value based on sensor */
> static constexpr uint32_t kMinExposure = 1;
> @@ -45,6 +67,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,9 +80,17 @@ 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([[maybe_unused]] IPAContext &context,
> - const IPAConfigInfo &configInfo)
> + const IPAConfigInfo &configInfo)
> {
> + /* \todo use the configInfo fields and IPAContext to store the limits */
Did you mean s/store/provide/ ?
> lineDuration_ = configInfo.sensorInfo.lineLength * 1.0s
> / configInfo.sensorInfo.pixelRate;
> maxExposureTime_ = kMaxExposure * lineDuration_;
> @@ -67,9 +98,22 @@ int Agc::configure([[maybe_unused]] IPAContext &context,
> return 0;
> }
>
> +/**
> + * \brief Estimate the mean quantile of the top 2% of the histogram
s/mean quantile/inter-quantile mean/
It's the mean value of samples between two given quantiles. But you
could equally say "Estimate the mean value of the top 2% of the
histogram", which would be easier to understand.
> + * \param[in] stats The statistics computed by the ImgU
> + * \param[in] grid The grid used to store the statistics in the IPU3
> + */
> void Agc::processBrightness(const ipu3_uapi_stats_3a *stats,
> const ipu3_uapi_grid_config &grid)
Maybe the function should also be renamed accordingly ?
calculateBrightness() would already be a bit more descriptive, or
perhaps measureBrightness(). calculateTop2PercentBrightness() seems a
bit overkill.
> {
> + /*
> + * 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.
Don't we do so already ?
> + */
> const struct ipu3_uapi_grid_config statsAeGrid = stats->stats_4a_config.awb_config.grid;
> Rectangle aeRegion = { statsAeGrid.x_start,
> statsAeGrid.y_start,
> @@ -85,6 +129,7 @@ void Agc::processBrightness(const ipu3_uapi_stats_3a *stats,
> uint32_t endX = (startX + (aeRegion.size().width >> grid.block_width_log2)) << grid.block_width_log2;
> uint32_t i, j;
>
> + /* Initialise the histogram array */
> uint32_t hist[knumHistogramBins] = { 0 };
> for (j = topleftY;
> j < topleftY + (aeRegion.size().height >> grid.block_height_log2);
> @@ -92,13 +137,18 @@ void Agc::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.
> */
> const ipu3_uapi_awb_set_item *currentCell = &stats->awb_raw_buffer.meta_data[i + j * grid.width];
> if (currentCell->sat_ratio == 0) {
> uint8_t Gr = currentCell->Gr_avg;
> uint8_t Gb = currentCell->Gb_avg;
> + /*
> + * Store the average green value to estimate the
> + * brightness. Even the over exposed pixels are
> + * taken into account.
> + */
> hist[(Gr + Gb) / 2]++;
> }
> }
> @@ -108,18 +158,24 @@ 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;
> if (prevExposure_ == 0s) {
> - /* DG stands for digital gain.*/
> + /*
> + * DG stands for digital gain, which is always 1.0 for now as it
> + * is not implemented right now.
> + */
> prevExposure_ = currentExposure_;
> prevExposureNoDg_ = currentExposureNoDg_;
> } else {
> /*
> * 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 (prevExposure_ < 1.2 * currentExposure_ &&
> prevExposure_ > 0.8 * currentExposure_)
> @@ -130,10 +186,12 @@ void Agc::filterExposure()
> prevExposureNoDg_ = speed * currentExposureNoDg_ +
> prevExposureNoDg_ * (1.0 - speed);
> }
> +
> /*
> * 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 implement digital gain setting
s/implement/Implement/
> */
> double fastReduceThreshold = 0.4;
> if (prevExposureNoDg_ <
> @@ -142,6 +200,11 @@ void Agc::filterExposure()
> LOG(IPU3Agc, Debug) << "After filtering, total_exposure " << prevExposure_;
> }
>
> +/**
> + * \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::lockExposureGain(uint32_t &exposure, double &gain)
Here too a better name for the function would be nice.
> {
> /* Algorithm initialization should wait for first valid frames */
> @@ -154,22 +217,33 @@ void Agc::lockExposureGain(uint32_t &exposure, double &gain)
> if (std::abs(iqMean_ - kEvGainTarget * knumHistogramBins) <= 1) {
> LOG(IPU3Agc, 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_;
> +
> + /*
> + * Estimate the current exposure value for the scene as shutter
> + * time multiplicated by the analogue gain.
s/multiplicated/multiplied/
> + */
> currentExposureNoDg_ = currentShutter * gain;
> LOG(IPU3Agc, 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(IPU3Agc, Debug) << "Target total exposure " << currentExposure_;
>
> - /* \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,10 +259,20 @@ void Agc::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
> + *
> + * 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 &gain = context.frameContext.agc.gain;
> +
> processBrightness(stats, context.configuration.grid.bdsGrid);
> lockExposureGain(exposure, gain);
While at it I would have written
lockExposureGain(context.frameContext.agc.exposure,
context.frameContext.agc.gain);
I'm not a big fan of passing those by reference, maybe passing a pointer
to the context could be better, but that's for a separate patch.
> frameCount_++;
> diff --git a/src/ipa/ipu3/algorithms/agc.h b/src/ipa/ipu3/algorithms/agc.h
> index e36797d3..9449ba48 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__
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list