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

12740 diff mbox series

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.

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

Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@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(-)

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@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 */