[libcamera-devel] [PATCH 09/11] ipa: ipu3: agc: Document AGC mean-based algorithm
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Tue Sep 14 18:18:47 CEST 2021
Hi Kieran,
On Tue, Sep 14, 2021 at 12:20:14PM +0100, Kieran Bingham wrote:
> On 14/09/2021 06:21, Laurent Pinchart wrote:
> > 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/
>
> Surely the algorithm itself doesn't care if this is an analogue gain or
> a digital gain.
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.
> The IPU3 will apply it as an analogue gain - but that's independent
> isn't it ?
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.
> 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?
That's usually what a basic implementation will do, there could be
reason to do something more complex.
> >> + * 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 ?
>
> Is this relating to the line directly below which references the grid
> from the stats->stats_4a_config.awb_config.grid?
>
> Can we simply/directly reference our grid from our IPAContext?
>
> And if so - it might be better to do that immediately, rather than add
> the \todo.
>
> And if we do that we should mention that it is referenced from our grid,
> rather than the one provided in the statistics due to a kernel bug..
I meant that we use grid.width, with the following comment:
* We observed a bit shift which makes the value 160 to
* be 32 in the stats grid. Use the one from configure.
Isn't this what the \todo is about ? Or did I misundertand either the
code of the \todo ?
> >> + */
> >> 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