[libcamera-devel,RFC,v2,3/3] WIP: ipa: ipu3: Add support for IPU3 AEC/AGC algorithm

11369 diff mbox series

Inherit from the Algorithm class to implement basic auto-exposure and
auto-gain functions.

While exposure is not locked, AWB is not calculated and corrected.
Implement a basic "skewness-based" for demonstration purpose.

Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
---
 src/ipa/ipu3/ipu3.cpp     |  10 ++-
 src/ipa/ipu3/ipu3_agc.cpp | 171 ++++++++++++++++++++++++++++++++++++++
 src/ipa/ipu3/ipu3_agc.h   |  62 ++++++++++++++
 src/ipa/ipu3/meson.build  |   1 +
 4 files changed, 243 insertions(+), 1 deletion(-)
 create mode 100644 src/ipa/ipu3/ipu3_agc.cpp
 create mode 100644 src/ipa/ipu3/ipu3_agc.h

Hi Jean-Michel,

Thank you for the patch.

On Tue, Feb 23, 2021 at 05:40:41PM +0100, Jean-Michel Hautbois wrote:
> Inherit from the Algorithm class to implement basic auto-exposure and
> auto-gain functions.
> 
> While exposure is not locked, AWB is not calculated and corrected.
> Implement a basic "skewness-based" for demonstration purpose.
> 
> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
> ---
>  src/ipa/ipu3/ipu3.cpp     |  10 ++-
>  src/ipa/ipu3/ipu3_agc.cpp | 171 ++++++++++++++++++++++++++++++++++++++
>  src/ipa/ipu3/ipu3_agc.h   |  62 ++++++++++++++
>  src/ipa/ipu3/meson.build  |   1 +
>  4 files changed, 243 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 6fae5160..cabd0c71 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"
>  
>  namespace libcamera {
> @@ -65,6 +66,8 @@ private:
>  
>  	/* Interface to the AWB algorithm */
>  	ipa::IPU3Awb *awbAlgo_;
> +	/* Interface to the AEC/AGC algorithm */
> +	ipa::IPU3Agc *agcAlgo_;

I think those two comments could be dropped. Comments for the parts of
the code that are not trivial to understand would be more useful ;-)

A blank line would be nice here.

>  	/* Local parameter storage */
>  	ipu3_uapi_params params_;
>  };
> @@ -101,6 +104,8 @@ void IPAIPU3::configure(const std::map<uint32_t, ControlInfoMap> &entityControls
>  	awbAlgo_ = new ipa::IPU3Awb();
>  	awbAlgo_->initialise(params_);
>  
> +	agcAlgo_ = new ipa::IPU3Agc();

Never deleted. You can use a std::unique_ptr<>. Same for AWB.

> +
>  	setControls(0);
>  }
>  
> @@ -187,7 +192,10 @@ void IPAIPU3::parseStatistics(unsigned int frame,
>  {
>  	ControlList ctrls(controls::controls);
>  
> -	awbAlgo_->calculateWBGains(Rectangle(250, 160, 800, 400), stats);
> +	agcAlgo_->process(stats, exposure_, gain_);
> +	if (agcAlgo_->converged())
> +		awbAlgo_->calculateWBGains(Rectangle(250, 160, 800, 400), stats);
> +
>  	setControls(frame);
>  
>  	ipa::ipu3::IPU3Action op;
> diff --git a/src/ipa/ipu3/ipu3_agc.cpp b/src/ipa/ipu3/ipu3_agc.cpp
> new file mode 100644
> index 00000000..2636e2ea
> --- /dev/null
> +++ b/src/ipa/ipu3/ipu3_agc.cpp
> @@ -0,0 +1,171 @@
> +/* SPDX-License-Identifier: BSD-2-Clause */
> +/*
> + * Copyright (C) 2019, Raspberry Pi (Trading) Limited

Is there any code here coming from the RPi IPA ?

> + *
> + * ipu3_agc.cpp - AGC/AEC control algorithm
> + */
> +
> +#include "ipu3_agc.h"
> +
> +#include <numeric>
> +
> +#include "libcamera/internal/log.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;

s/const/constexpr/

Same below.

> +/* Number of frames to wait before calculating stats on maximum exposure */
> +static const uint32_t kInitialFrameMaxAECount = 8;
> +/* Number of frames to wait before calculating stats and estimate gain/exposure */
> +static const uint32_t kInitialFrameSkipCount = 12;
> +
> +/* Number of frames to wait between new gain/exposure estimations */
> +static const uint32_t kFrameSkipCount = 4;
> +
> +IPU3Agc::IPU3Agc()
> +	: frameCount_(0), converged_(false)
> +{
> +}
> +
> +IPU3Agc::~IPU3Agc()
> +{
> +}
> +
> +void IPU3Agc::initialise()
> +{
> +}
> +
> +void IPU3Agc::process()
> +{
> +}
> +
> +/*
> + * \todo This function is taken from numerical recipes and calculates all
> + * moments. It needs to be rewritten properly and maybe in a "math" class ?
> + */
> +void IPU3Agc::moments(std::unordered_map<uint32_t, uint32_t> &data, int n)

data is never modified, you can make it const.

> +{
> +	int j;

unsigned int ?

> +	double ep = 0.0, s, p;
> +	double ave, adev, sdev;
> +	double var, skew, curt;
> +
> +	s = 0.0;
> +	for (j = 1; j <= n; j++)
> +		s += data[j];
> +
> +	ave = s / n;
> +	adev = var = skew = curt = 0.0;
> +
> +	for (j = 1; j <= n; j++) {
> +		adev += s = data[j] - (ave);
> +		ep += s;
> +		var += (p = s * s);
> +		skew += (p *= s);
> +		curt += (p *= s);
> +	}
> +
> +	adev /= n;
> +	var = (var - ep * ep / n) / (n - 1);
> +	sdev = std::sqrt(var);
> +
> +	if (var) {
> +		skew /= n * var * sdev;
> +		curt = curt / (n * var * var) - 3.0;
> +	}
> +	skew_ = skew;
> +}
> +
> +void IPU3Agc::processBrightness(const ipu3_uapi_stats_3a *stats)
> +{
> +	cellsBrightness_.clear();

Only used in this function, you can make it a local variable.

> +
> +	/*\todo Replace constant values with real BDS configuration */
> +	for (uint32_t j = 0; j < 45; j++) {

You still repeat the exact same inner loop 45 times...

> +		for (uint32_t i = 0; i < 160 * 45 * 8; i += 8) {
> +			uint8_t Gr = stats->awb_raw_buffer.meta_data[i];
> +			uint8_t R = stats->awb_raw_buffer.meta_data[i + 1];
> +			uint8_t B = stats->awb_raw_buffer.meta_data[i + 2];
> +			uint8_t Gb = stats->awb_raw_buffer.meta_data[i + 3];
> +			cellsBrightness_.push_back(static_cast<uint32_t>(0.299 * R + 0.587 * (Gr + Gb) / 2 + 0.114 * B));
> +		}
> +	}
> +	std::sort(cellsBrightness_.begin(), cellsBrightness_.end());
> +
> +	/* \todo create a class to generate histograms ! */
> +	std::unordered_map<uint32_t, uint32_t> hist;

Is it me, or are you using a map to store a vector ?

	std::vector<uint32_t> histogram{ 256 };

Or possibly

	std::array<uint32_t, 256> histogram{};

> +	for (uint32_t const &val : cellsBrightness_)
> +		hist[val]++;
> +	moments(hist, 256);

And you can drop the second argument, as it will be conveyed either
implicitly by the vector, or explicitly through the array type (I'd use
Span in the second case though).

> +}
> +
> +/* \todo make this function a math one ? */
> +uint32_t IPU3Agc::rootApproximation()
> +{
> +	return (currentExposure_ * prevSkew_ + prevExposure_ * currentSkew_) / (prevSkew_ + currentSkew_);
> +}
> +
> +void IPU3Agc::lockExposure(uint32_t &exposure, uint32_t &gain)
> +{
> +	/* Algorithm initialization wait for first valid frames */
> +	/* \todo - have a number of frames given by DelayedControls ?
> +	 * - implement a function for IIR */

	/*
	 * ...
	 * ...
	 */

> +	if (frameCount_ == kInitialFrameMinAECount) {
> +		prevExposure_ = exposure;
> +
> +		prevSkew_ = skew_;
> +		/* \todo use configured values */
> +		exposure = 800;
> +		gain = 8;
> +		currentExposure_ = exposure;
> +	} else if (frameCount_ == kInitialFrameMaxAECount) {
> +		currentSkew_ = skew_;
> +		exposure = rootApproximation();
> +		nextExposure_ = exposure;
> +		lastFrame_ = frameCount_;
> +	} else if ((frameCount_ >= kInitialFrameSkipCount) && (frameCount_ - lastFrame_ >= kFrameSkipCount)) {
> +		currentSkew_ = skew_;
> +		/* \todo properly calculate a gain */
> +		if (frameCount_ == kInitialFrameSkipCount)
> +			gain = ((8 * prevSkew_) + (1 * currentSkew_)) / (prevSkew_ + currentSkew_);
> +
> +		if (currentSkew_ - prevSkew_ > 1) {
> +			/* under exposed */
> +			prevExposure_ = nextExposure_;
> +			exposure = rootApproximation();
> +			nextExposure_ = exposure;
> +		} else if (currentSkew_ - prevSkew_ < -1) {
> +			/* over exposed */
> +			currentExposure_ = nextExposure_;
> +			exposure = rootApproximation();
> +			nextExposure_ = exposure;
> +		} else {
> +			/* we have converged */
> +			converged_ = true;
> +		}
> +		lastFrame_ = frameCount_;
> +		prevSkew_ = currentSkew_;
> +	}

As commented in 2/3, a comment block to explain what this does would be
useful.

> +}
> +
> +void IPU3Agc::process(const ipu3_uapi_stats_3a *stats, uint32_t &exposure, uint32_t &gain)
> +{
> +	processBrightness(stats);
> +	if (!converged_)
> +		lockExposure(exposure, gain);
> +	else {
> +		/* Are we still well exposed ? */
> +		if ((skew_ < 2) || (skew_ > 4))

No need for inner parentheses.

> +			converged_ = false;
> +	}
> +	frameCount_++;
> +}
> +
> +} /* 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..b14a2a2f
> --- /dev/null
> +++ b/src/ipa/ipu3/ipu3_agc.h
> @@ -0,0 +1,62 @@
> +/* SPDX-License-Identifier: BSD-2-Clause */
> +/*
> + * Copyright (C) 2019, Raspberry Pi (Trading) Limited
> + *
> + * 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>

This doesn't seem to be needed here.

> +
> +#include "libipa/algorithm.h"
> +
> +namespace libcamera {
> +
> +namespace ipa {
> +
> +class IPU3Agc : public Algorithm
> +{
> +public:
> +	IPU3Agc();
> +	~IPU3Agc();
> +
> +	void initialise() override;
> +	void process() override;
> +
> +	void process(const ipu3_uapi_stats_3a *stats, uint32_t &exposure, uint32_t &gain);
> +	bool converged() { return converged_; }
> +
> +private:
> +	void moments(std::unordered_map<uint32_t, uint32_t> &data, int n);
> +	void processBrightness(const ipu3_uapi_stats_3a *stats);
> +	uint32_t rootApproximation();
> +	void lockExposure(uint32_t &exposure, uint32_t &gain);
> +
> +	uint64_t frameCount_;
> +	uint64_t lastFrame_;
> +
> +	/* Vector of calculated brightness for each cell */
> +	std::vector<uint32_t> cellsBrightness_;
> +
> +	/* Values for filtering */
> +	uint32_t prevExposure_;
> +	uint32_t currentExposure_;
> +	uint32_t nextExposure_;

Here we have prev, current, next

> +
> +	double skew_;
> +	double prevSkew_;
> +	double currentSkew_;

And here we have "", prev and current. A common naming scheme would make
it easier to read the code.

> +	bool converged_;
> +};
> +
> +} /* namespace ipa */
> +
> +} /* namespace libcamera */
> +
> +#endif /* __LIBCAMERA_IPU3_AGC_H__ */
> diff --git a/src/ipa/ipu3/meson.build b/src/ipa/ipu3/meson.build
> index 07a864c8..43ad0e0d 100644
> --- a/src/ipa/ipu3/meson.build
> +++ b/src/ipa/ipu3/meson.build
> @@ -4,6 +4,7 @@ ipa_name = 'ipa_ipu3'
>  
>  ipu3_ipa_sources = files([
>    'ipu3.cpp',
> +  'ipu3_agc.cpp',
>    'ipu3_awb.cpp',
>  ])
>