[libcamera-devel] [PATCH v4 4/4] ipa: ipu3: Add support for IPU3 AEC/AGC algorithm
Kieran Bingham
kieran.bingham at ideasonboard.com
Mon Apr 12 18:56:15 CEST 2021
Hi JM,
On 30/03/2021 22:12, Jean-Michel Hautbois wrote:
> Inherit from the Algorithm class to implement basic auto-exposure and
> auto-gain functions.
I don't think we need to explain that we inherit from the Algorithm class.
Just that we implement a basic AE/AGC ;-)
> Extract computeTargetExposure() and computeGain() and adapt those to the
> IPU3 structure.
> Filtering was added too as it avoids big steps when exposure changes a
> lot.
This sounds like a changelog not a commit message
"""
Implement a basic auto-exposure and auto-gain algorithm for the IPU3.
The functions computeTargetExposure() and computeGain() are adapted from
the Raspberry Pi AGC implementation to suit the IPU3 structures, and
filtering is added to reduce visible stepsize when there are large
exposure changes.
"""
But adapt as required of course ;-)
>
> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois at ideasonboard.com>
> ---
> src/ipa/ipu3/ipu3.cpp | 12 +-
> src/ipa/ipu3/ipu3_agc.cpp | 228 ++++++++++++++++++++++++++++++++++++++
> src/ipa/ipu3/ipu3_agc.h | 67 +++++++++++
> src/ipa/ipu3/meson.build | 1 +
> 4 files changed, 307 insertions(+), 1 deletion(-)
> create mode 100644 src/ipa/ipu3/ipu3_agc.cpp
> create mode 100644 src/ipa/ipu3/ipu3_agc.h
>
> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
> index 1cce11c9..848437b5 100644
> --- a/src/ipa/ipu3/ipu3.cpp
> +++ b/src/ipa/ipu3/ipu3.cpp
> @@ -21,6 +21,7 @@
> #include "libcamera/internal/buffer.h"
> #include "libcamera/internal/log.h"
>
> +#include "ipu3_agc.h"
> #include "ipu3_awb.h"
>
> static constexpr uint32_t kMaxCellWidthPerSet = 160;
> @@ -70,6 +71,8 @@ private:
>
> /* Interface to the AWB algorithm */
> std::unique_ptr<ipa::IPU3Awb> awbAlgo_;
> + /* Interface to the AEC/AGC algorithm */
> + std::unique_ptr<ipa::IPU3Agc> agcAlgo_;
I'm curious that these are pointers and allocated.
Do we need to store them as pointers and call make_unique?
Or can we just store an instance of each in the class...
Do we destroy and recreate them anytime?
> /* Local parameter storage */
> struct ipu3_uapi_params params_;
> @@ -169,6 +172,9 @@ void IPAIPU3::configure(const std::map<uint32_t, ControlInfoMap> &entityControls
>
> awbAlgo_ = std::make_unique<ipa::IPU3Awb>();
> awbAlgo_->initialise(params_, bdsOutputSize, bdsGrid_);
> +
> + agcAlgo_ = std::make_unique<ipa::IPU3Agc>();
> + agcAlgo_->initialise(bdsGrid_);
> }
>
> void IPAIPU3::mapBuffers(const std::vector<IPABuffer> &buffers)
> @@ -240,7 +246,8 @@ void IPAIPU3::processControls([[maybe_unused]] unsigned int frame,
>
> void IPAIPU3::fillParams(unsigned int frame, ipu3_uapi_params *params)
> {
> - awbAlgo_->updateWbParameters(params_, 1.0);
> + if (agcAlgo_->updateControls())
> + awbAlgo_->updateWbParameters(params_, agcAlgo_->gamma());
>
> *params = params_;
>
> @@ -255,7 +262,10 @@ void IPAIPU3::parseStatistics(unsigned int frame,
> {
> ControlList ctrls(controls::controls);
>
> + agcAlgo_->process(stats, exposure_, gain_);
> awbAlgo_->calculateWBGains(stats);
New line here ...
> + if (agcAlgo_->updateControls())
> + setControls(frame);
>
> ipa::ipu3::IPU3Action op;
> op.op = ipa::ipu3::ActionMetadataReady;
> diff --git a/src/ipa/ipu3/ipu3_agc.cpp b/src/ipa/ipu3/ipu3_agc.cpp
> new file mode 100644
> index 00000000..6cb657b3
> --- /dev/null
> +++ b/src/ipa/ipu3/ipu3_agc.cpp
> @@ -0,0 +1,228 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2021, Ideas On Board
> + *
> + * ipu3_agc.cpp - AGC/AEC control algorithm
> + */
> +
> +#include "ipu3_agc.h"
> +
> +#include <algorithm>
> +#include <cmath>
> +#include <numeric>
> +
> +#include "libcamera/internal/log.h"
> +
> +#include "libipa/histogram.h"
> +
> +namespace libcamera {
> +
> +namespace ipa {
> +
> +LOG_DEFINE_CATEGORY(IPU3Agc)
> +
> +/* Number of frames to wait before calculating stats on minimum exposure */
> +static const uint32_t kInitialFrameMinAECount = 4;
> +/* Number of frames to wait between new gain/exposure estimations */
> +static const uint32_t kFrameSkipCount = 6;
> +
> +/* Maximum ISO value for analogue gain */
> +static const uint32_t kMinISO = 100;
> +static const uint32_t kMaxISO = 1500;
> +/* Maximum analogue gain value
> + * \todo grab it from a camera helper */
> +static const uint32_t kMinGain = kMinISO / 100;
> +static const uint32_t kMaxGain = kMaxISO / 100;
> +/* \todo use calculated value based on sensor */
> +static const uint32_t kMinExposure = 1;
> +static const uint32_t kMaxExposure = 1976;
Is 1976 arbitrary?
Indeed these should come from the sensor info. Is that available to us
here yet? Or do we need to do something to get these in where you need them?
> +/* \todo those should be get from pipeline handler ! */
> +/* line duration in microseconds */
> +static const double kLineDuration = 16.8;
> +static const double kMaxExposureTime = kMaxExposure * kLineDuration;
I know some people disagree, but a new line between these distinct
constant blocks makes it easier for me to parse.
> +/* Histogram constants */
> +static const uint32_t knumHistogramBins = 256;
> +static const double kEvGainTarget = 0.5;
And I know you've told me not to say s/const/constexpr/ already ;)
> +
> +IPU3Agc::IPU3Agc()
> + : frameCount_(0), lastFrame_(0),
> + converged_(false), updateControls_(false)
> +{
> + iqMean_ = 0.0;
> + gamma_ = 1.0;
> + histLow_ = 0;
> + histHigh_ = 255;
> + prevTotalExposure_ = 0.0;
> + prevTotalExposureNoDg_ = 0.0;
> + currentTotalExposure_ = 0.0;
> + currentTotalExposureNoDg_ = 0.0;
Should these all go in the constructor initialiser list?
Could the Prev/current total types be wrapped to simplify them?
(If it doesn't make sense to do that then its' a no)
> +}
> +
> +IPU3Agc::~IPU3Agc()
> +{
> +}
Does this need to be defined if it doesn't do anything?
Or is it a placeholder?
> +
> +void IPU3Agc::initialise(struct ipu3_uapi_grid_config &bdsGrid)
> +{
> + aeGrid_ = bdsGrid;
> +}
Newline missing here.
> +void IPU3Agc::processBrightness(const ipu3_uapi_stats_3a *stats)
> +{
> + const struct ipu3_uapi_grid_config statsAeGrid = stats->stats_4a_config.awb_config.grid;
> + Rectangle aeRegion = { statsAeGrid.x_start,
> + statsAeGrid.y_start,
> + static_cast<unsigned int>(statsAeGrid.x_end - statsAeGrid.x_start) + 1,
> + static_cast<unsigned int>(statsAeGrid.y_end - statsAeGrid.y_start) + 1 };
> + Point topleft = aeRegion.topLeft();
> + uint32_t startY = (topleft.y >> aeGrid_.block_height_log2) * aeGrid_.width << aeGrid_.block_width_log2;
> + uint32_t startX = (topleft.x >> aeGrid_.block_width_log2) << aeGrid_.block_width_log2;
X before Y ?
> + uint32_t endX = (startX + (aeRegion.size().width >> aeGrid_.block_width_log2)) << aeGrid_.block_width_log2;
We don't need endY I guess?
> + uint32_t i, j;
> + uint32_t count = 0;
> +
> + cellsBrightness_.clear();
> +
> + for (j = (topleft.y >> aeGrid_.block_height_log2);
> + j < (topleft.y >> aeGrid_.block_height_log2) + (aeRegion.size().height >> aeGrid_.block_height_log2);
topleft.{x,y} >> block_{width,height}... is used a lot here.
Would it help readability to cache it in a descriptive local variable?
> + j++) {
> + for (i = startX + startY; i < endX + startY; i += 8) {
is += 8 a hardcode of the block/grid size? If so - should it be stored
in the class as a constant so it will be updated when it is variable?
> + /* 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. */
/*
* Multiline comment style has the start and end on their own lines
* like this.
*/
> + if (stats->awb_raw_buffer.meta_data[i + 4 + j * aeGrid_.width] == 0) {
> + uint8_t Gr = stats->awb_raw_buffer.meta_data[i + j * aeGrid_.width];
I'd be tempted to put a + 0 + j in here to keep alignment with the
others below...
> + uint8_t R = stats->awb_raw_buffer.meta_data[i + 1 + j * aeGrid_.width];
> + uint8_t B = stats->awb_raw_buffer.meta_data[i + 2 + j * aeGrid_.width];
> + uint8_t Gb = stats->awb_raw_buffer.meta_data[i + 3 + j * aeGrid_.width];
> +
> + cellsBrightness_.push_back(static_cast<uint32_t>(0.2125 * R + 0.7154 * (Gr + Gb) / 2 + 0.0722 * B));
Where is this calculation from?
is it a specific color space etc again?
> + count++;
> + }
> + }
> + }
> + std::vector<uint32_t>::iterator maxIntensity = std::max_element(cellsBrightness_.begin(), cellsBrightness_.end());
> + LOG(IPU3Agc, Debug) << "Most frequent intensity is " << *maxIntensity << " at " << std::distance(cellsBrightness_.begin(), maxIntensity);
> +
> + /* \todo create a class to generate histograms ! */
I presume this comment is distinct from the Histogram class you add in
this series?
Would some of these operations below go into the Histogram class?
> + uint32_t hist[knumHistogramBins] = { 0 };
> + for (uint32_t const &val : cellsBrightness_)
> + hist[val]++;
> +
> + double mean = 0.0;
> + for (i = 0; i < knumHistogramBins; i++) {
> + mean += hist[i] * i;
> + }
> + mean /= count;
> +
> + double variance = 0.0;
> + for (i = 0; i < knumHistogramBins; i++) {
> + variance += ((i - mean) * (i - mean)) * hist[i];
> + }
single line statement with for doesn't need { }
> + variance /= count;
> + variance = std::sqrt(variance);
> +
> + LOG(IPU3Agc, Debug) << "mean value is: " << mean << " and variance is " << variance;
New line here ...
> + /* Limit the gamma effect for now */
> + gamma_ = 1.1;
> +
> + const auto [minBrightness, maxBrightness] = std::minmax_element(cellsBrightness_.begin(), cellsBrightness_.end());
> + histLow_ = *minBrightness;
> + histHigh_ = *maxBrightness;
> +
> + iqMean_ = Histogram(Span<uint32_t>(hist)).interQuantileMean(0.98, 1.0);
Is there a definition for using 0.98 ... 1.0 ?
> +}
> +
> +void IPU3Agc::filterExposure(bool desaturate)
> +{
> + double speed = 0.2;
> + if (prevTotalExposure_ == 0.0) {
> + prevTotalExposure_ = currentTotalExposure_;
> + prevTotalExposureNoDg_ = currentTotalExposureNoDg_;
what is NoDg here?
> + } else {
> + /* If close to the result go faster, to save making so many
> + * micro-adjustments on the way.
> + * \ todo: Make this customisable? */
Comment style should be fixed.
Also the comment doesn't make much sense at the minute I'm afraid.
Perhaps:
"""
If we are close to the desired result, go faster to avoid making
multiple micro-adjustments.
"""
> + if (prevTotalExposure_ < 1.2 * currentTotalExposure_ &&
> + prevTotalExposure_ > 0.8 * currentTotalExposure_)
> + speed = sqrt(speed);
> + prevTotalExposure_ = speed * currentTotalExposure_ +
> + prevTotalExposure_ * (1.0 - speed);
> + /* When desaturing, take a big jump down in exposure_no_dg,
> + * which we'll hide with digital gain. */
> + if (desaturate)
> + prevTotalExposureNoDg_ =
> + currentTotalExposureNoDg_;
> + else
> + prevTotalExposureNoDg_ =
> + speed * currentTotalExposureNoDg_ +
> + prevTotalExposureNoDg_ * (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). */
> + double fastReduceThreshold = 0.4;
> + if (prevTotalExposureNoDg_ <
> + prevTotalExposure_ * fastReduceThreshold)
> + prevTotalExposureNoDg_ = prevTotalExposure_ * fastReduceThreshold;
> + LOG(IPU3Agc, Debug) << "After filtering, total_exposure " << prevTotalExposure_;
> +}
> +
> +void IPU3Agc::lockExposureGain(uint32_t &exposure, uint32_t &gain)> +{
> + updateControls_ = false;
> +
> + /* Algorithm initialization wait for first valid frames */
Does this comment mean "should wait for the first valid frames" or
something else?
> + /* \todo - have a number of frames given by DelayedControls ?
> + * - implement a function for IIR */
Yes, I suspect a Filter class would be helpful/useful in other places.
But perhaps it might be on top/later unless it's easy to abstract.
> + if ((frameCount_ == kInitialFrameMinAECount) || (frameCount_ - lastFrame_ >= kFrameSkipCount)) {
If you invert this condition(s), and return early, the large code block
below can be pulled in one indentation level...
> + /* Are we correctly exposed ? */
> + double newGain = kEvGainTarget * knumHistogramBins / iqMean_;
newGain only seems to be used in the else case below, should it be
determined down there?
> +
> + if (std::abs(iqMean_ - kEvGainTarget * knumHistogramBins) <= 1) {
> + LOG(IPU3Agc, Debug) << "!!! Good exposure with iqMean = " << iqMean_;
> + converged_ = true;
> + } else {
> + /* extracted from Rpi::Agc::computeTargetExposure */
> + double currentShutter = exposure * kLineDuration;
> + currentTotalExposureNoDg_ = currentShutter * gain;
> + LOG(IPU3Agc, Debug) << "Actual total exposure " << currentTotalExposureNoDg_
> + << " Shutter speed " << currentShutter
> + << " Gain " << gain;
> + currentTotalExposure_ = currentTotalExposureNoDg_ * newGain;
> + double maxTotalExposure = kMaxExposureTime * kMaxGain;
> + currentTotalExposure_ = std::min(currentTotalExposure_, maxTotalExposure);
> + LOG(IPU3Agc, Debug) << "Target total exposure " << currentTotalExposure_;
> +
> + /* \todo: estimate if we need to desaturate */
> + filterExposure(false);
> +
> + double newExposure = 0.0;
> + if (currentShutter < kMaxExposureTime) {
> + exposure = std::clamp(static_cast<uint32_t>(exposure * currentTotalExposure_ / currentTotalExposureNoDg_), kMinExposure, kMaxExposure);
> + newExposure = currentTotalExposure_ / exposure;
> + gain = std::clamp(static_cast<uint32_t>(gain * currentTotalExposure_ / newExposure), kMinGain, kMaxGain);
> + updateControls_ = true;
> + } else if (currentShutter >= kMaxExposureTime) {
> + gain = std::clamp(static_cast<uint32_t>(gain * currentTotalExposure_ / currentTotalExposureNoDg_), kMinGain, kMaxGain);
> + newExposure = currentTotalExposure_ / gain;
> + exposure = std::clamp(static_cast<uint32_t>(exposure * currentTotalExposure_ / newExposure), kMinExposure, kMaxExposure);
> + updateControls_ = true;
> + }
> + LOG(IPU3Agc, Debug) << "Adjust exposure " << exposure * kLineDuration << " and gain " << gain;
> + }
> + lastFrame_ = frameCount_;
> + } else {
> + updateControls_ = false;
This was already set at the entry to the function.
> + }
> +}
> +
> +void IPU3Agc::process(const ipu3_uapi_stats_3a *stats, uint32_t &exposure, uint32_t &gain)
> +{
> + processBrightness(stats);
> + lockExposureGain(exposure, gain);
> + frameCount_++;
do we need to keep a frameCount?
I thought the IPA's were given the frame number ...
> +}
> +
> +} /* namespace ipa */
> +
> +} /* namespace libcamera */
> diff --git a/src/ipa/ipu3/ipu3_agc.h b/src/ipa/ipu3/ipu3_agc.h
> new file mode 100644
> index 00000000..d4657a81
> --- /dev/null
> +++ b/src/ipa/ipu3/ipu3_agc.h
> @@ -0,0 +1,67 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2021, Ideas On Board
> + *
> + * ipu3_agc.h - IPU3 AGC/AEC control algorithm
> + */
> +#ifndef __LIBCAMERA_IPU3_AGC_H__
> +#define __LIBCAMERA_IPU3_AGC_H__
> +
> +#include <unordered_map>
> +#include <vector>
> +
> +#include <linux/intel-ipu3.h>
> +
> +#include <libcamera/geometry.h>
> +
> +#include "libipa/algorithm.h"
> +
> +namespace libcamera {
> +
> +namespace ipa {
> +
> +class IPU3Agc : public Algorithm
> +{
> +public:
> + IPU3Agc();
> + ~IPU3Agc();
> +
> + void initialise(struct ipu3_uapi_grid_config &bdsGrid);
> + void process(const ipu3_uapi_stats_3a *stats, uint32_t &exposure, uint32_t &gain);
> + bool converged() { return converged_; }
> + bool updateControls() { return updateControls_; }
> + /* \todo Use a metadata exchange between IPAs */
> + double gamma() { return gamma_; }
Have you looked at how RPi exchange metadata between algorithms along
the pipeline?
(src/ipa/raspberrypi/controller/metadata.hpp)
> +
> +private:
> + void processBrightness(const ipu3_uapi_stats_3a *stats);
> + void filterExposure(bool desaturate);
> + void lockExposureGain(uint32_t &exposure, uint32_t &gain);
> +
> + struct ipu3_uapi_grid_config aeGrid_;
> +
> + uint64_t frameCount_;
> + uint64_t lastFrame_;
> +
> + /* Vector of calculated brightness for each cell */
> + std::vector<uint32_t> cellsBrightness_;
> +
> + bool converged_;
> + bool updateControls_;
> +
> + double iqMean_;
> + double gamma_;
> + uint32_t histLow_;
> + uint32_t histHigh_;
> +
> + double prevTotalExposure_;
> + double prevTotalExposureNoDg_;
> + double currentTotalExposure_;
> + double currentTotalExposureNoDg_;
> +};
> +
> +} /* namespace ipa */
> +
> +} /* namespace libcamera */
> +
> +#endif /* __LIBCAMERA_IPU3_AGC_H__ */
> diff --git a/src/ipa/ipu3/meson.build b/src/ipa/ipu3/meson.build
> index 1040698e..adeae28b 100644
> --- a/src/ipa/ipu3/meson.build
> +++ b/src/ipa/ipu3/meson.build
> @@ -5,6 +5,7 @@ ipa_name = 'ipa_ipu3'
> ipu3_ipa_sources = files([
> 'ipu3.cpp',
> 'ipu3_awb.cpp',
> + 'ipu3_agc.cpp',
> ])
>
> mod = shared_module(ipa_name,
>
--
Regards
--
Kieran
More information about the libcamera-devel
mailing list