[libcamera-devel] [PATCH v1 5/7] ipa: ipu3: Improve AWB behaviour

[Laurent Pinchart]

Hi Jean-Michel,

Thank you for the patch.

On Mon, Jun 28, 2021 at 10:22:53PM +0200, Jean-Michel Hautbois wrote:
> In order to use a better grid, clamp the values calculated from the BDS
> resolution to at least 2 times the minimum grid size.
> The number of zones needed to have sufficiently relevant statistics is
> now based on the region sizes and not on an arbitrary value.
> Last, the default green gains where not properly used, it should be 8192
> and not 4096 to have a multiplier of 1.0 on the R/G/B gains.

Sounds like this should be split in two patches.

> The default CCM is adjusted for Surface Go2 only, and should eventually be
> calculated in the CameraSensorHelper class.

How do you envision the CameraSensorHelper class calculating this ?

> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois at ideasonboard.com>
> ---
>  src/ipa/ipu3/ipu3.cpp     | 12 ++++++++----
>  src/ipa/ipu3/ipu3_awb.cpp | 23 +++++++++++------------
>  src/ipa/ipu3/ipu3_awb.h   |  1 +
>  3 files changed, 20 insertions(+), 16 deletions(-)
> 
> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
> index 4466391a..9a2def64 100644
> --- a/src/ipa/ipu3/ipu3.cpp
> +++ b/src/ipa/ipu3/ipu3.cpp
> @@ -123,12 +123,16 @@ void IPAIPU3::calculateBdsGrid(const Size &bdsOutputSize)
>  	bdsGrid_ = {};
>  
>  	for (uint32_t widthShift = 3; widthShift <= 7; ++widthShift) {
> -		uint32_t width = std::min(kMaxCellWidthPerSet,
> -					  bdsOutputSize.width >> widthShift);
> +		uint32_t width = std::clamp(bdsOutputSize.width >> widthShift,
> +					    2 * kAwbStatsSizeX,
> +					    kMaxCellWidthPerSet);
> +
>  		width = width << widthShift;
>  		for (uint32_t heightShift = 3; heightShift <= 7; ++heightShift) {
> -			int32_t height = std::min(kMaxCellHeightPerSet,
> -						  bdsOutputSize.height >> heightShift);
> +			uint32_t height = std::clamp(bdsOutputSize.height >> heightShift,
> +						     2 * kAwbStatsSizeY,
> +						     kMaxCellHeightPerSet);
> +
>  			height = height << heightShift;
>  			uint32_t error  = std::abs(static_cast<int>(width - bdsOutputSize.width))
>  							+ std::abs(static_cast<int>(height - bdsOutputSize.height));
> diff --git a/src/ipa/ipu3/ipu3_awb.cpp b/src/ipa/ipu3/ipu3_awb.cpp
> index a94935c5..a39536b0 100644
> --- a/src/ipa/ipu3/ipu3_awb.cpp
> +++ b/src/ipa/ipu3/ipu3_awb.cpp
> @@ -18,9 +18,6 @@ namespace ipa::ipu3 {
>  
>  LOG_DEFINE_CATEGORY(IPU3Awb)
>  
> -static constexpr uint32_t kMinZonesCounted = 16;
> -static constexpr uint32_t kMinGreenLevelInZone = 32;
> -
>  /**
>   * \struct IspStatsRegion
>   * \brief RGB statistics for a given region
> @@ -92,7 +89,7 @@ static constexpr uint32_t kMinGreenLevelInZone = 32;
>  
>  /* Default settings for Bayer noise reduction replicated from the Kernel */
>  static const struct ipu3_uapi_bnr_static_config imguCssBnrDefaults = {
> -	.wb_gains = { 16, 16, 16, 16 },
> +	.wb_gains = { 8192, 8192, 8192, 8192 },

Does the kernel report 8192 as a default value ?

>  	.wb_gains_thr = { 255, 255, 255, 255 },
>  	.thr_coeffs = { 1700, 0, 31, 31, 0, 16 },
>  	.thr_ctrl_shd = { 26, 26, 26, 26 },
> @@ -130,7 +127,7 @@ static const struct ipu3_uapi_awb_config_s imguCssAwbDefaults = {
>  /* Default color correction matrix defined as an identity matrix */
>  static const struct ipu3_uapi_ccm_mat_config imguCssCcmDefault = {
>  	8191, 0, 0, 0,
> -	0, 8191, 0, 0,
> +	0, 6000, 0, 0,

that's not an identify matrix anymore.

>  	0, 0, 8191, 0
>  };
>  
> @@ -166,6 +163,7 @@ IPU3Awb::IPU3Awb()
>  	asyncResults_.greenGain = 1.0;
>  	asyncResults_.redGain = 1.0;
>  	asyncResults_.temperatureK = 4500;
> +	minZonesCounted_ = 0;
>  }
>  
>  IPU3Awb::~IPU3Awb()
> @@ -241,7 +239,7 @@ void IPU3Awb::generateZones(std::vector<RGB> &zones)
>  	for (unsigned int i = 0; i < kAwbStatsSizeX * kAwbStatsSizeY; i++) {
>  		RGB zone;
>  		double counted = awbStats_[i].counted;
> -		if (counted >= kMinZonesCounted) {
> +		if (counted >= minZonesCounted_) {
>  			zone.G = awbStats_[i].gSum / counted;
>  			if (zone.G >= kMinGreenLevelInZone) {
>  				zone.R = awbStats_[i].rSum / counted;
> @@ -258,6 +256,7 @@ void IPU3Awb::generateAwbStats(const ipu3_uapi_stats_3a *stats)
>  	uint32_t regionWidth = round(awbGrid_.width / static_cast<double>(kAwbStatsSizeX));
>  	uint32_t regionHeight = round(awbGrid_.height / static_cast<double>(kAwbStatsSizeY));
>  
> +	minZonesCounted_ = ((regionWidth * regionHeight) * 4) / 5;
>  	/*
>  	 * Generate a (kAwbStatsSizeX x kAwbStatsSizeY) array from the IPU3 grid which is
>  	 * (awbGrid_.width x awbGrid_.height).
> @@ -269,7 +268,7 @@ void IPU3Awb::generateAwbStats(const ipu3_uapi_stats_3a *stats)
>  			uint32_t cellY = ((cellPosition / awbGrid_.width) / regionHeight) % kAwbStatsSizeY;
>  
>  			uint32_t awbRegionPosition = cellY * kAwbStatsSizeX + cellX;
> -			cellPosition *= 8;
> +			cellPosition *= sizeof(Ipu3AwbCell);
>  
>  			/* Cast the initial IPU3 structure to simplify the reading */
>  			Ipu3AwbCell *currentCell = reinterpret_cast<Ipu3AwbCell *>(const_cast<uint8_t *>(&stats->awb_raw_buffer.meta_data[cellPosition]));
> @@ -361,12 +360,12 @@ void IPU3Awb::updateWbParameters(ipu3_uapi_params &params, double agcGamma)
>  	/*
>  	 * Green gains should not be touched and considered 1.
>  	 * Default is 16, so do not change it at all.
> -	 * 4096 is the value for a gain of 1.0
> +	 * 8192 is the value for a gain of 1.0
>  	 */
> -	params.acc_param.bnr.wb_gains.gr = 16;
> -	params.acc_param.bnr.wb_gains.r = 4096 * asyncResults_.redGain;
> -	params.acc_param.bnr.wb_gains.b = 4096 * asyncResults_.blueGain;
> -	params.acc_param.bnr.wb_gains.gb = 16;
> +	params.acc_param.bnr.wb_gains.gr = 8192;
> +	params.acc_param.bnr.wb_gains.r = 8192 * asyncResults_.redGain;
> +	params.acc_param.bnr.wb_gains.b = 8192 * asyncResults_.blueGain;
> +	params.acc_param.bnr.wb_gains.gb = 8192;
>  
>  	LOG(IPU3Awb, Debug) << "Color temperature estimated: " << asyncResults_.temperatureK
>  			    << " and gamma calculated: " << agcGamma;
> diff --git a/src/ipa/ipu3/ipu3_awb.h b/src/ipa/ipu3/ipu3_awb.h
> index 795e32e3..23865c21 100644
> --- a/src/ipa/ipu3/ipu3_awb.h
> +++ b/src/ipa/ipu3/ipu3_awb.h
> @@ -49,6 +49,7 @@ private:
>  	std::vector<RGB> zones_;
>  	IspStatsRegion awbStats_[kAwbStatsSizeX * kAwbStatsSizeY];
>  	AwbStatus asyncResults_;
> +	uint32_t minZonesCounted_;
>  };
>  
>  } /* namespace ipa::ipu3 */

-- 
Regards,

Laurent Pinchart