[libcamera-devel] [RFC PATCH 5/5] ipa: ipu3: Move IPU3 agc into algorithms

Mon Aug 9 13:05:42 CEST 2021

On 09/08/2021 10:20, Jean-Michel Hautbois wrote:
> Use the Context class and algorithm interface to properly call the AGC
> algorithm from IPAIPU3.
> We need to pass shutter speed and analogue gain through Context. Add a
> dedicated AGC structure for that.

Same comments from the previous patch I think ;-)

> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois at ideasonboard.com>
> ---
>  .../ipu3/{ipu3_agc.cpp => algorithms/agc.cpp} | 56 ++++++++++---------
>  src/ipa/ipu3/{ipu3_agc.h => algorithms/agc.h} | 31 +++++-----
>  src/ipa/ipu3/algorithms/meson.build           |  1 +
>  src/ipa/ipu3/ipa_context.h                    | 11 ++++
>  src/ipa/ipu3/ipu3.cpp                         | 25 +++++----
>  src/ipa/ipu3/meson.build                      |  1 -
>  6 files changed, 70 insertions(+), 55 deletions(-)
>  rename src/ipa/ipu3/{ipu3_agc.cpp => algorithms/agc.cpp} (81%)
>  rename src/ipa/ipu3/{ipu3_agc.h => algorithms/agc.h} (61%)
> 
> diff --git a/src/ipa/ipu3/ipu3_agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp
> similarity index 81%
> rename from src/ipa/ipu3/ipu3_agc.cpp
> rename to src/ipa/ipu3/algorithms/agc.cpp
> index 733814dd..1146a34a 100644
> --- a/src/ipa/ipu3/ipu3_agc.cpp
> +++ b/src/ipa/ipu3/algorithms/agc.cpp
> @@ -2,26 +2,25 @@
>  /*
>   * Copyright (C) 2021, Ideas On Board
>   *
> - * ipu3_agc.cpp - AGC/AEC control algorithm
> + * agc.cpp - AGC/AEC control algorithm
>   */
>  
> -#include "ipu3_agc.h"
> +#include "agc.h"
>  
> -#include <algorithm>
>  #include <cmath>
> +> +#include <algorithm>
>  #include <numeric>

What's going on there? Shouldn't cmath algorithm, and numeric all be one
alphabetically sorted block?

>  
>  #include <libcamera/base/log.h>
>  
> -#include <libcamera/ipa/core_ipa_interface.h>
> -

Was this no longer required as part of the conversion, or something that
could have been done on it's own?

>  #include "libipa/histogram.h"
>  
>  namespace libcamera {
>  
>  using namespace std::literals::chrono_literals;
>  
> -namespace ipa::ipu3 {
> +namespace ipa::ipu3::algorithms {
>  
>  LOG_DEFINE_CATEGORY(IPU3Agc)
>  
> @@ -36,8 +35,8 @@ 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;
> +static constexpr double kMinGain = kMinISO / 100;
> +static constexpr double kMaxGain = kMaxISO / 100;

Why do these types change as part of moving the algorithm and restructuring?

It doesn't sound like something related to moving code - so it should be
in it's own patch, either before if it's a bug fix, or after if it
doesn't matter when the types change.

>  /* \todo use calculated value based on sensor */
>  static constexpr uint32_t kMinExposure = 1;
> @@ -50,24 +49,24 @@ static constexpr double kEvGainTarget = 0.5;
>  /* A cell is 8 bytes and contains averages for RGB values and saturation ratio */
>  static constexpr uint8_t kCellSize = 8;
>  
> -IPU3Agc::IPU3Agc()
> +Agc::Agc()
>  	: frameCount_(0), lastFrame_(0), converged_(false),
> -	  updateControls_(false), iqMean_(0.0),
> -	  lineDuration_(0s), maxExposureTime_(0s),
> +	  updateControls_(false), iqMean_(0.0), maxExposureTime_(0s),
>  	  prevExposure_(0s), prevExposureNoDg_(0s),
>  	  currentExposure_(0s), currentExposureNoDg_(0s)
>  {
>  }
>  
> -void IPU3Agc::initialise(struct ipu3_uapi_grid_config &bdsGrid, const IPACameraSensorInfo &sensorInfo)
> +int Agc::configure(IPAContext &context)
>  {
> -	aeGrid_ = bdsGrid;
> +	/* The AGC algorithm uses the AWB statistics */
> +	aeGrid_ = context.awb.grid.bdsGrid;
> +	maxExposureTime_ = kMaxExposure * context.agc.lineDuration;
>  
> -	lineDuration_ = sensorInfo.lineLength * 1.0s / sensorInfo.pixelRate;
> -	maxExposureTime_ = kMaxExposure * lineDuration_;
> +	return 0;
>  }
>  
> -void IPU3Agc::processBrightness(const ipu3_uapi_stats_3a *stats)
> +void Agc::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,
> @@ -108,7 +107,7 @@ void IPU3Agc::processBrightness(const ipu3_uapi_stats_3a *stats)
>  	iqMean_ = Histogram(Span<uint32_t>(hist)).interQuantileMean(0.98, 1.0);
>  }
>  
> -void IPU3Agc::filterExposure()
> +void Agc::filterExposure()
>  {
>  	double speed = 0.2;
>  	if (prevExposure_ == 0s) {
> @@ -128,7 +127,7 @@ void IPU3Agc::filterExposure()
>  		prevExposure_ = speed * currentExposure_ +
>  				prevExposure_ * (1.0 - speed);
>  		prevExposureNoDg_ = speed * currentExposureNoDg_ +
> -				prevExposureNoDg_ * (1.0 - speed);
> +				    prevExposureNoDg_ * (1.0 - speed);

This is a minor whitespace fix, so I guess it showed up in checkstyle as
part of the code move. Could either be it's own patch - or needs to be
metnioned in the commit message as something like:

"Fixed up white-space issues only were fixed as part of the move."

>  	}
>  	/*
>  	 * We can't let the no_dg exposure deviate too far below the
> @@ -142,7 +141,7 @@ void IPU3Agc::filterExposure()
>  	LOG(IPU3Agc, Debug) << "After filtering, total_exposure " << prevExposure_;
>  }
>  
> -void IPU3Agc::lockExposureGain(uint32_t &exposure, double &gain)
> +void Agc::lockExposureGain(IPAContext &context)

Private algorithm functions don't have to take the IPAContext, these
could have been passed in from ::process() still...

>  {
>  	updateControls_ = false;
>  
> @@ -160,7 +159,9 @@ void IPU3Agc::lockExposureGain(uint32_t &exposure, double &gain)
>  		double newGain = kEvGainTarget * knumHistogramBins / iqMean_;
>  
>  		/* extracted from Rpi::Agc::computeTargetExposure */
> -		libcamera::utils::Duration currentShutter = exposure * lineDuration_;
> +		libcamera::utils::Duration currentShutter = context.agc.shutterTime;
> +		uint32_t exposure = currentShutter / context.agc.lineDuration;
> +		double &gain = context.agc.analogueGain;

I'm not sure I like this.

If gain is something that is calculated by this function, it should be
setting the end result after it is calculated.

I guess this is a shortcut that the answer is calculated in place... but
it feels like a hack - as reading the code it's not clear where the
results go.

I.e. usually, you'd expect calculations and then results to be set at
the end or return by the function?

To keep the churn minimal in this patch which just moves to the new
interface, I'd stick with the existing function prototype and pass in
the references from the IPAContext to the private function.

>  		currentExposureNoDg_ = currentShutter * gain;
>  		LOG(IPU3Agc, Debug) << "Actual total exposure " << currentExposureNoDg_
>  				    << " Shutter speed " << currentShutter
> @@ -177,26 +178,27 @@ void IPU3Agc::lockExposureGain(uint32_t &exposure, double &gain)
>  		if (currentShutter < maxExposureTime_) {
>  			exposure = std::clamp(static_cast<uint32_t>(exposure * currentExposure_ / currentExposureNoDg_), kMinExposure, kMaxExposure);
>  			newExposure = currentExposure_ / exposure;
> -			gain = std::clamp(static_cast<uint32_t>(gain * currentExposure_ / newExposure), kMinGain, kMaxGain);
> +			gain = std::clamp(static_cast<double>(gain * currentExposure_ / newExposure), kMinGain, kMaxGain);
>  			updateControls_ = true;
>  		} else if (currentShutter >= maxExposureTime_) {
> -			gain = std::clamp(static_cast<uint32_t>(gain * currentExposure_ / currentExposureNoDg_), kMinGain, kMaxGain);
> +			gain = std::clamp(static_cast<double>(gain * currentExposure_ / currentExposureNoDg_), kMinGain, kMaxGain);
>  			newExposure = currentExposure_ / gain;
>  			exposure = std::clamp(static_cast<uint32_t>(exposure * currentExposure_ / newExposure), kMinExposure, kMaxExposure);
>  			updateControls_ = true;
>  		}
> -		LOG(IPU3Agc, Debug) << "Adjust exposure " << exposure * lineDuration_ << " and gain " << gain;
> +		context.agc.shutterTime = exposure * context.agc.lineDuration;

Perhaps that shutterTime line is something that should happen in
Agc::process() if the context isn't available in the function.

> +		LOG(IPU3Agc, Debug) << "Adjust exposure " << exposure * context.agc.lineDuration << " and gain " << gain;
>  	}
>  	lastFrame_ = frameCount_;
>  }
>  
> -void IPU3Agc::process(const ipu3_uapi_stats_3a *stats, uint32_t &exposure, double &gain)
> +void Agc::process(IPAContext &context)
>  {
> -	processBrightness(stats);
> -	lockExposureGain(exposure, gain);
> +	processBrightness(context.stats);
> +	lockExposureGain(context);
>  	frameCount_++;
>  }
>  
> -} /* namespace ipa::ipu3 */
> +} /* namespace ipa::ipu3::algorithms */
>  
>  } /* namespace libcamera */
> diff --git a/src/ipa/ipu3/ipu3_agc.h b/src/ipa/ipu3/algorithms/agc.h
> similarity index 61%
> rename from src/ipa/ipu3/ipu3_agc.h
> rename to src/ipa/ipu3/algorithms/agc.h
> index 3b7f781e..e87a06f3 100644
> --- a/src/ipa/ipu3/ipu3_agc.h
> +++ b/src/ipa/ipu3/algorithms/agc.h
> @@ -4,44 +4,44 @@
>   *
>   * ipu3_agc.h - IPU3 AGC/AEC control algorithm
>   */
> -#ifndef __LIBCAMERA_IPU3_AGC_H__
> -#define __LIBCAMERA_IPU3_AGC_H__
> +#ifndef __LIBCAMERA_IPU3_ALGORITHMS_AGC_H__
> +#define __LIBCAMERA_IPU3_ALGORITHMS_AGC_H__
> +
> +#include <linux/intel-ipu3.h>
>  
>  #include <array>
>  #include <unordered_map>
>  
> -#include <linux/intel-ipu3.h>
> -

Why did the linux header move? I think it was in the right place here.

https://git.libcamera.org/libcamera/libcamera.git/tree/Documentation/coding-style.rst#n64
states:

1. The header declaring the API being implemented (if any)
2. The C and C++ system and standard library headers
3. Other libraries' headers, with one group per library
4. Other project's headers

>  #include <libcamera/base/utils.h>
>  
>  #include <libcamera/geometry.h>
>  
> -#include "algorithms/algorithm.h"
> -#include "libipa/algorithm.h"
> +#include "algorithm.h"
>  
>  namespace libcamera {
>  
>  struct IPACameraSensorInfo;
>  
> -namespace ipa::ipu3 {
> +namespace ipa::ipu3::algorithms {
>  
>  using utils::Duration;
>  
> -class IPU3Agc : public ipa::Algorithm
> +class Agc : public Algorithm
>  {
>  public:
> -	IPU3Agc();
> -	~IPU3Agc() = default;
> +	Agc();
> +	~Agc() = default;
> +
> +	int configure(IPAContext &context) override;
> +	void process(IPAContext &context) override;
>  
> -	void initialise(struct ipu3_uapi_grid_config &bdsGrid, const IPACameraSensorInfo &sensorInfo);
> -	void process(const ipu3_uapi_stats_3a *stats, uint32_t &exposure, double &gain);
>  	bool converged() { return converged_; }
>  	bool updateControls() { return updateControls_; }
>  
>  private:
>  	void processBrightness(const ipu3_uapi_stats_3a *stats);
>  	void filterExposure();
> -	void lockExposureGain(uint32_t &exposure, double &gain);
> +	void lockExposureGain(IPAContext &context);
>  
>  	struct ipu3_uapi_grid_config aeGrid_;
>  
> @@ -53,7 +53,6 @@ private:
>  
>  	double iqMean_;
>  
> -	Duration lineDuration_;
>  	Duration maxExposureTime_;
>  
>  	Duration prevExposure_;
> @@ -62,8 +61,8 @@ private:
>  	Duration currentExposureNoDg_;
>  };
>  
> -} /* namespace ipa::ipu3 */
> +} /* namespace ipa::ipu3::algorithms */
>  
>  } /* namespace libcamera */
>  
> -#endif /* __LIBCAMERA_IPU3_AGC_H__ */
> +#endif /* __LIBCAMERA_IPU3_ALGORITHMS_AGC_H__ */
> diff --git a/src/ipa/ipu3/algorithms/meson.build b/src/ipa/ipu3/algorithms/meson.build
> index df36d719..3faf913e 100644
> --- a/src/ipa/ipu3/algorithms/meson.build
> +++ b/src/ipa/ipu3/algorithms/meson.build
> @@ -1,6 +1,7 @@
>  # SPDX-License-Identifier: CC0-1.0
>  
>  ipu3_ipa_algorithms = files([
> +    'agc.cpp',
>      'awb.cpp',
>      'contrast.cpp',
>  ])
> diff --git a/src/ipa/ipu3/ipa_context.h b/src/ipa/ipu3/ipa_context.h
> index c43b275b..1d7a1984 100644
> --- a/src/ipa/ipu3/ipa_context.h
> +++ b/src/ipa/ipu3/ipa_context.h
> @@ -11,10 +11,14 @@
>  
>  #include <linux/intel-ipu3.h>
>  
> +#include <libcamera/base/utils.h>
> +
>  #include <libcamera/geometry.h>
>  
>  namespace libcamera {
>  
> +using libcamera::utils::Duration;
> +
>  namespace ipa::ipu3 {
>  
>  struct IPAContext {
> @@ -33,6 +37,13 @@ struct IPAContext {
>  			Size bdsOutputSize;
>  		} grid;
>  	} awb;
> +
> +	/* AGC specific parameters to share */
> +	struct Agc {
> +		Duration lineDuration;
> +		Duration shutterTime;
> +		double analogueGain;
> +	} agc;
>  };
>  
>  } /* namespace ipa::ipu3 */
> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
> index 82506461..07b1d11c 100644
> --- a/src/ipa/ipu3/ipu3.cpp
> +++ b/src/ipa/ipu3/ipu3.cpp
> @@ -22,12 +22,12 @@
>  
>  #include "libcamera/internal/framebuffer.h"
>  
> +#include "algorithms/agc.h"
>  #include "algorithms/algorithm.h"

I don't like including an algorithm, then including the algorithm header.

Either we include the algorithm header first, and ignore alphabetical
ordering, or we don't need to include algorithm ?

I think this is an instance where we should ignore alphabetical - But
maybe Laurent has different ideas.

>  #include "algorithms/awb.h"
>  #include "algorithms/contrast.h"
>  #include "ipa_context.h"
>  
> -#include "ipu3_agc.h"
>  #include "libipa/camera_sensor_helper.h"
>  
>  static constexpr uint32_t kMaxCellWidthPerSet = 160;
> @@ -37,6 +37,8 @@ namespace libcamera {
>  
>  LOG_DEFINE_CATEGORY(IPAIPU3)
>  
> +using namespace std::literals::chrono_literals;
> +

Why has this sprung up in here? Presumably regarding Duration or something?

Should this be specified in the header that brings that in ?

>  namespace ipa::ipu3 {
>  
>  class IPAIPU3 : public IPAIPU3Interface
> @@ -81,8 +83,6 @@ private:
>  	uint32_t minGain_;
>  	uint32_t maxGain_;
>  
> -	/* Interface to the AEC/AGC algorithm */
> -	std::unique_ptr<IPU3Agc> agcAlgo_;
>  	/* Interface to the Camera Helper */
>  	std::unique_ptr<CameraSensorHelper> camHelper_;
>  
> @@ -103,6 +103,7 @@ int IPAIPU3::init(const IPASettings &settings)
>  	}
>  
>  	/* Construct our Algorithms */
> +	algorithms_.emplace_back(new algorithms::Agc());
>  	algorithms_.emplace_back(new algorithms::Awb());
>  	algorithms_.emplace_back(new algorithms::Contrast());
>  
> @@ -214,11 +215,14 @@ int IPAIPU3::configure(const IPAConfigInfo &configInfo)
>  	calculateBdsGrid(configInfo.bdsOutputSize);
>  	context_.awb.grid.bdsOutputSize = configInfo.bdsOutputSize;
>  
> +	/* Prepare AGC parameters */
> +	context_.agc.lineDuration = sensorInfo_.lineLength * 1.0s / sensorInfo_.pixelRate;
> +	context_.agc.shutterTime = exposure_ * context_.agc.lineDuration;
> +	context_.agc.analogueGain = camHelper_->gain(gain_);

How do we avoid algorithm specific code living in this function.
Can this be handled during Agc::configure()?

What information do we need to pass into configure to make that happen?

> +
>  	configureAlgorithms();

it might even be that the implementation of configureAlgorithms() may as
well be directly coded in here....

Same for the others potentially too.

Might be something to revisit later.

>  
>  	bdsGrid_ = context_.awb.grid.bdsGrid;
> -	agcAlgo_ = std::make_unique<IPU3Agc>();
> -	agcAlgo_->initialise(bdsGrid_, sensorInfo_);
>  
>  	return 0;
>  }
> @@ -341,12 +345,7 @@ void IPAIPU3::parseStatistics(unsigned int frame,
>  	/* Run the process for each algorithm on the stats */
>  	processAlgorithms(stats);
>  
> -	double gain = camHelper_->gain(gain_);
> -	agcAlgo_->process(stats, exposure_, gain);
> -	gain_ = camHelper_->gainCode(gain);
> -
> -	if (agcAlgo_->updateControls())
> -		setControls(frame);
> +	setControls(frame);

Will we always have controls to set?

Should we have a control list in the IPAContext that can be updated, or
a flag?

Not necessarily for this patch - but in trying to think about the
overall design goals.

>  
>  	/* \todo Use VBlank value calculated from each frame exposure. */
>  	int64_t frameDuration = sensorInfo_.lineLength * (defVBlank_ + sensorInfo_.outputSize.height) /
> @@ -365,6 +364,10 @@ void IPAIPU3::setControls(unsigned int frame)
>  	IPU3Action op;
>  	op.op = ActionSetSensorControls;
>  
> +	/* Convert gain and exposure */
> +	gain_ = camHelper_->gainCode(context_.agc.analogueGain);
> +	exposure_ = context_.agc.shutterTime / context_.agc.lineDuration;
> +
>  	ControlList ctrls(ctrls_);
>  	ctrls.set(V4L2_CID_EXPOSURE, static_cast<int32_t>(exposure_));
>  	ctrls.set(V4L2_CID_ANALOGUE_GAIN, static_cast<int32_t>(gain_));
> diff --git a/src/ipa/ipu3/meson.build b/src/ipa/ipu3/meson.build
> index d1126947..39320980 100644
> --- a/src/ipa/ipu3/meson.build
> +++ b/src/ipa/ipu3/meson.build
> @@ -6,7 +6,6 @@ ipa_name = 'ipa_ipu3'
>  
>  ipu3_ipa_sources = files([
>      'ipu3.cpp',
> -    'ipu3_agc.cpp',
>  ])
>  
>  ipu3_ipa_sources += ipu3_ipa_algorithms
> 