[v5,2/3] libcamera: software_isp: Normalize statistics sums to 8-bit
diff mbox series

Message ID 20260506221944.066FD1EA006B@mailuser.phl.internal
State Accepted
Headers show
Series
  • ipa: simple: Proportional AGC and statistics normalization
Related show

Commit Message

Javier Tia May 6, 2026, 9:46 p.m. UTC 
The SWSTATS_ACCUMULATE_LINE_STATS() macro divides the luminance value
for the histogram to normalize it to 8-bit range, but does not apply
the same normalization to the RGB sums. For 10-bit and 12-bit unpacked
Bayer formats this means the sums are accumulated at native bit depth
(0-1023 or 0-4095 per pixel) while the AWB algorithm subtracts an
8-bit black level from them, under-correcting by 4x or 16x
respectively.

This mismatch between the AWB's gain calculation (using incorrectly
BLC-subtracted sums) and the debayer's correct normalized BLC
subtraction produces a visible color cast. For example, with the
OV2740 sensor (10-bit, BLC=16), the under-subtraction skews R/G gain
by ~9%.

Fix this by right-shifting the RGB sums in finishFrame() to normalize
them to 8-bit scale, matching the histogram and the 8-bit black level
used by AWB. A per-format sumShift_ value is set in configure():
0 for 8-bit and CSI-2 packed formats (already 8-bit), 2 for 10-bit,
and 4 for 12-bit unpacked formats.

Signed-off-by: Javier Tia <floss@jetm.me>
Reviewed-by: Milan Zamazal <mzamazal@redhat.com>
Tested-by: Milan Zamazal <mzamazal@redhat.com>
Reviewed-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
Tested-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
---
 include/libcamera/internal/software_isp/swstats_cpu.h | 1 +
 src/libcamera/software_isp/swstats_cpu.cpp            | 9 +++++++++
 2 files changed, 10 insertions(+)

Comments

Hans de Goede May 7, 2026, 1:16 p.m. UTC | #1 
Hi,

On 6-May-26 23:46, Javier Tia wrote:
> The SWSTATS_ACCUMULATE_LINE_STATS() macro divides the luminance value
> for the histogram to normalize it to 8-bit range, but does not apply
> the same normalization to the RGB sums. For 10-bit and 12-bit unpacked
> Bayer formats this means the sums are accumulated at native bit depth
> (0-1023 or 0-4095 per pixel) while the AWB algorithm subtracts an
> 8-bit black level from them, under-correcting by 4x or 16x
> respectively.
> 
> This mismatch between the AWB's gain calculation (using incorrectly
> BLC-subtracted sums) and the debayer's correct normalized BLC
> subtraction produces a visible color cast. For example, with the
> OV2740 sensor (10-bit, BLC=16), the under-subtraction skews R/G gain
> by ~9%.
> 
> Fix this by right-shifting the RGB sums in finishFrame() to normalize
> them to 8-bit scale, matching the histogram and the 8-bit black level
> used by AWB. A per-format sumShift_ value is set in configure():
> 0 for 8-bit and CSI-2 packed formats (already 8-bit), 2 for 10-bit,
> and 4 for 12-bit unpacked formats.
> 
> Signed-off-by: Javier Tia <floss@jetm.me>
> Reviewed-by: Milan Zamazal <mzamazal@redhat.com>
> Tested-by: Milan Zamazal <mzamazal@redhat.com>
> Reviewed-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
> Tested-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>

Thanks, patch looks good to me:

Reviewed-by: Hans de Goede <johannes.goede@oss.qualcomm.com>

Regards,

Hans




> ---
>  include/libcamera/internal/software_isp/swstats_cpu.h | 1 +
>  src/libcamera/software_isp/swstats_cpu.cpp            | 9 +++++++++
>  2 files changed, 10 insertions(+)
> 
> diff --git a/include/libcamera/internal/software_isp/swstats_cpu.h b/include/libcamera/internal/software_isp/swstats_cpu.h
> index 802370bd..2dac6945 100644
> --- a/include/libcamera/internal/software_isp/swstats_cpu.h
> +++ b/include/libcamera/internal/software_isp/swstats_cpu.h
> @@ -116,6 +116,7 @@ private:
>  
>  	unsigned int xShift_;
>  	unsigned int stride_;
> +	unsigned int sumShift_;
>  
>  	std::vector<SwIspStats> stats_;
>  	SharedMemObject<SwIspStats> sharedStats_;
> diff --git a/src/libcamera/software_isp/swstats_cpu.cpp b/src/libcamera/software_isp/swstats_cpu.cpp
> index 5366e019..b40d3334 100644
> --- a/src/libcamera/software_isp/swstats_cpu.cpp
> +++ b/src/libcamera/software_isp/swstats_cpu.cpp
> @@ -362,6 +362,11 @@ void SwStatsCpu::finishFrame(uint32_t frame, uint32_t bufferId)
>  			for (unsigned int j = 0; j < SwIspStats::kYHistogramSize; j++)
>  				sharedStats_->yHistogram[j] += s.yHistogram[j];
>  		}
> +		if (sumShift_) {
> +			sharedStats_->sum_.r() >>= sumShift_;
> +			sharedStats_->sum_.g() >>= sumShift_;
> +			sharedStats_->sum_.b() >>= sumShift_;
> +		}
>  	}
>  
>  	sharedStats_->valid = valid;
> @@ -425,12 +430,15 @@ int SwStatsCpu::configure(const StreamConfiguration &inputCfg, unsigned int stat
>  		switch (bayerFormat.bitDepth) {
>  		case 8:
>  			stats0_ = &SwStatsCpu::statsBGGR8Line0;
> +			sumShift_ = 0;
>  			return 0;
>  		case 10:
>  			stats0_ = &SwStatsCpu::statsBGGR10Line0;
> +			sumShift_ = 2;
>  			return 0;
>  		case 12:
>  			stats0_ = &SwStatsCpu::statsBGGR12Line0;
> +			sumShift_ = 4;
>  			return 0;
>  		}
>  	}
> @@ -442,6 +450,7 @@ int SwStatsCpu::configure(const StreamConfiguration &inputCfg, unsigned int stat
>  		/* Skip every 3th and 4th line, sample every other 2x2 block */
>  		ySkipMask_ = 0x02;
>  		xShift_ = 0;
> +		sumShift_ = 0;
>  		processFrame_ = &SwStatsCpu::processBayerFrame2;
>  
>  		switch (bayerFormat.order) {
Laurent Pinchart May 7, 2026, 2:21 p.m. UTC | #2 
On Wed, May 06, 2026 at 03:46:28PM -0600, Javier Tia wrote:
> The SWSTATS_ACCUMULATE_LINE_STATS() macro divides the luminance value
> for the histogram to normalize it to 8-bit range, but does not apply
> the same normalization to the RGB sums. For 10-bit and 12-bit unpacked
> Bayer formats this means the sums are accumulated at native bit depth
> (0-1023 or 0-4095 per pixel) while the AWB algorithm subtracts an
> 8-bit black level from them, under-correcting by 4x or 16x
> respectively.
> 
> This mismatch between the AWB's gain calculation (using incorrectly
> BLC-subtracted sums) and the debayer's correct normalized BLC
> subtraction produces a visible color cast. For example, with the
> OV2740 sensor (10-bit, BLC=16), the under-subtraction skews R/G gain
> by ~9%.
> 
> Fix this by right-shifting the RGB sums in finishFrame() to normalize
> them to 8-bit scale, matching the histogram and the 8-bit black level
> used by AWB. A per-format sumShift_ value is set in configure():
> 0 for 8-bit and CSI-2 packed formats (already 8-bit), 2 for 10-bit,
> and 4 for 12-bit unpacked formats.
> 
> Signed-off-by: Javier Tia <floss@jetm.me>
> Reviewed-by: Milan Zamazal <mzamazal@redhat.com>
> Tested-by: Milan Zamazal <mzamazal@redhat.com>
> Reviewed-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
> Tested-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
> ---
>  include/libcamera/internal/software_isp/swstats_cpu.h | 1 +
>  src/libcamera/software_isp/swstats_cpu.cpp            | 9 +++++++++
>  2 files changed, 10 insertions(+)
> 
> diff --git a/include/libcamera/internal/software_isp/swstats_cpu.h b/include/libcamera/internal/software_isp/swstats_cpu.h
> index 802370bd..2dac6945 100644
> --- a/include/libcamera/internal/software_isp/swstats_cpu.h
> +++ b/include/libcamera/internal/software_isp/swstats_cpu.h
> @@ -116,6 +116,7 @@ private:
>  
>  	unsigned int xShift_;
>  	unsigned int stride_;
> +	unsigned int sumShift_;
>  
>  	std::vector<SwIspStats> stats_;
>  	SharedMemObject<SwIspStats> sharedStats_;
> diff --git a/src/libcamera/software_isp/swstats_cpu.cpp b/src/libcamera/software_isp/swstats_cpu.cpp
> index 5366e019..b40d3334 100644
> --- a/src/libcamera/software_isp/swstats_cpu.cpp
> +++ b/src/libcamera/software_isp/swstats_cpu.cpp
> @@ -362,6 +362,11 @@ void SwStatsCpu::finishFrame(uint32_t frame, uint32_t bufferId)
>  			for (unsigned int j = 0; j < SwIspStats::kYHistogramSize; j++)
>  				sharedStats_->yHistogram[j] += s.yHistogram[j];
>  		}
> +		if (sumShift_) {
> +			sharedStats_->sum_.r() >>= sumShift_;
> +			sharedStats_->sum_.g() >>= sumShift_;
> +			sharedStats_->sum_.b() >>= sumShift_;
> +		}

Should we do this unconditionally ? Shifting right by 0 is a no-op.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

>  	}
>  
>  	sharedStats_->valid = valid;
> @@ -425,12 +430,15 @@ int SwStatsCpu::configure(const StreamConfiguration &inputCfg, unsigned int stat
>  		switch (bayerFormat.bitDepth) {
>  		case 8:
>  			stats0_ = &SwStatsCpu::statsBGGR8Line0;
> +			sumShift_ = 0;
>  			return 0;
>  		case 10:
>  			stats0_ = &SwStatsCpu::statsBGGR10Line0;
> +			sumShift_ = 2;
>  			return 0;
>  		case 12:
>  			stats0_ = &SwStatsCpu::statsBGGR12Line0;
> +			sumShift_ = 4;
>  			return 0;
>  		}
>  	}
> @@ -442,6 +450,7 @@ int SwStatsCpu::configure(const StreamConfiguration &inputCfg, unsigned int stat
>  		/* Skip every 3th and 4th line, sample every other 2x2 block */
>  		ySkipMask_ = 0x02;
>  		xShift_ = 0;
> +		sumShift_ = 0;
>  		processFrame_ = &SwStatsCpu::processBayerFrame2;
>  
>  		switch (bayerFormat.order) {
Barnabás Pőcze May 7, 2026, 2:23 p.m. UTC | #3 
2026. 05. 07. 16:21 keltezéssel, Laurent Pinchart írta:
> On Wed, May 06, 2026 at 03:46:28PM -0600, Javier Tia wrote:
>> The SWSTATS_ACCUMULATE_LINE_STATS() macro divides the luminance value
>> for the histogram to normalize it to 8-bit range, but does not apply
>> the same normalization to the RGB sums. For 10-bit and 12-bit unpacked
>> Bayer formats this means the sums are accumulated at native bit depth
>> (0-1023 or 0-4095 per pixel) while the AWB algorithm subtracts an
>> 8-bit black level from them, under-correcting by 4x or 16x
>> respectively.
>>
>> This mismatch between the AWB's gain calculation (using incorrectly
>> BLC-subtracted sums) and the debayer's correct normalized BLC
>> subtraction produces a visible color cast. For example, with the
>> OV2740 sensor (10-bit, BLC=16), the under-subtraction skews R/G gain
>> by ~9%.
>>
>> Fix this by right-shifting the RGB sums in finishFrame() to normalize
>> them to 8-bit scale, matching the histogram and the 8-bit black level
>> used by AWB. A per-format sumShift_ value is set in configure():
>> 0 for 8-bit and CSI-2 packed formats (already 8-bit), 2 for 10-bit,
>> and 4 for 12-bit unpacked formats.
>>
>> Signed-off-by: Javier Tia <floss@jetm.me>
>> Reviewed-by: Milan Zamazal <mzamazal@redhat.com>
>> Tested-by: Milan Zamazal <mzamazal@redhat.com>
>> Reviewed-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
>> Tested-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
>> ---
>>   include/libcamera/internal/software_isp/swstats_cpu.h | 1 +
>>   src/libcamera/software_isp/swstats_cpu.cpp            | 9 +++++++++
>>   2 files changed, 10 insertions(+)
>>
>> diff --git a/include/libcamera/internal/software_isp/swstats_cpu.h b/include/libcamera/internal/software_isp/swstats_cpu.h
>> index 802370bd..2dac6945 100644
>> --- a/include/libcamera/internal/software_isp/swstats_cpu.h
>> +++ b/include/libcamera/internal/software_isp/swstats_cpu.h
>> @@ -116,6 +116,7 @@ private:
>>   
>>   	unsigned int xShift_;
>>   	unsigned int stride_;
>> +	unsigned int sumShift_;
>>   
>>   	std::vector<SwIspStats> stats_;
>>   	SharedMemObject<SwIspStats> sharedStats_;
>> diff --git a/src/libcamera/software_isp/swstats_cpu.cpp b/src/libcamera/software_isp/swstats_cpu.cpp
>> index 5366e019..b40d3334 100644
>> --- a/src/libcamera/software_isp/swstats_cpu.cpp
>> +++ b/src/libcamera/software_isp/swstats_cpu.cpp
>> @@ -362,6 +362,11 @@ void SwStatsCpu::finishFrame(uint32_t frame, uint32_t bufferId)
>>   			for (unsigned int j = 0; j < SwIspStats::kYHistogramSize; j++)
>>   				sharedStats_->yHistogram[j] += s.yHistogram[j];
>>   		}
>> +		if (sumShift_) {
>> +			sharedStats_->sum_.r() >>= sumShift_;
>> +			sharedStats_->sum_.g() >>= sumShift_;
>> +			sharedStats_->sum_.b() >>= sumShift_;
>> +		}
> 
> Should we do this unconditionally ? Shifting right by 0 is a no-op.

I second that!


> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
>>   	}
>>   
>>   	sharedStats_->valid = valid;
>> @@ -425,12 +430,15 @@ int SwStatsCpu::configure(const StreamConfiguration &inputCfg, unsigned int stat
>>   		switch (bayerFormat.bitDepth) {
>>   		case 8:
>>   			stats0_ = &SwStatsCpu::statsBGGR8Line0;
>> +			sumShift_ = 0;
>>   			return 0;
>>   		case 10:
>>   			stats0_ = &SwStatsCpu::statsBGGR10Line0;
>> +			sumShift_ = 2;
>>   			return 0;
>>   		case 12:
>>   			stats0_ = &SwStatsCpu::statsBGGR12Line0;
>> +			sumShift_ = 4;
>>   			return 0;
>>   		}
>>   	}
>> @@ -442,6 +450,7 @@ int SwStatsCpu::configure(const StreamConfiguration &inputCfg, unsigned int stat
>>   		/* Skip every 3th and 4th line, sample every other 2x2 block */
>>   		ySkipMask_ = 0x02;
>>   		xShift_ = 0;
>> +		sumShift_ = 0;
>>   		processFrame_ = &SwStatsCpu::processBayerFrame2;
>>   
>>   		switch (bayerFormat.order) {
>

Patch
diff mbox series 

diff --git a/include/libcamera/internal/software_isp/swstats_cpu.h b/include/libcamera/internal/software_isp/swstats_cpu.h
index 802370bd..2dac6945 100644
--- a/include/libcamera/internal/software_isp/swstats_cpu.h
+++ b/include/libcamera/internal/software_isp/swstats_cpu.h
@@ -116,6 +116,7 @@  private:
 
 	unsigned int xShift_;
 	unsigned int stride_;
+	unsigned int sumShift_;
 
 	std::vector<SwIspStats> stats_;
 	SharedMemObject<SwIspStats> sharedStats_;
diff --git a/src/libcamera/software_isp/swstats_cpu.cpp b/src/libcamera/software_isp/swstats_cpu.cpp
index 5366e019..b40d3334 100644
--- a/src/libcamera/software_isp/swstats_cpu.cpp
+++ b/src/libcamera/software_isp/swstats_cpu.cpp
@@ -362,6 +362,11 @@  void SwStatsCpu::finishFrame(uint32_t frame, uint32_t bufferId)
 			for (unsigned int j = 0; j < SwIspStats::kYHistogramSize; j++)
 				sharedStats_->yHistogram[j] += s.yHistogram[j];
 		}
+		if (sumShift_) {
+			sharedStats_->sum_.r() >>= sumShift_;
+			sharedStats_->sum_.g() >>= sumShift_;
+			sharedStats_->sum_.b() >>= sumShift_;
+		}
 	}
 
 	sharedStats_->valid = valid;
@@ -425,12 +430,15 @@  int SwStatsCpu::configure(const StreamConfiguration &inputCfg, unsigned int stat
 		switch (bayerFormat.bitDepth) {
 		case 8:
 			stats0_ = &SwStatsCpu::statsBGGR8Line0;
+			sumShift_ = 0;
 			return 0;
 		case 10:
 			stats0_ = &SwStatsCpu::statsBGGR10Line0;
+			sumShift_ = 2;
 			return 0;
 		case 12:
 			stats0_ = &SwStatsCpu::statsBGGR12Line0;
+			sumShift_ = 4;
 			return 0;
 		}
 	}
@@ -442,6 +450,7 @@  int SwStatsCpu::configure(const StreamConfiguration &inputCfg, unsigned int stat
 		/* Skip every 3th and 4th line, sample every other 2x2 block */
 		ySkipMask_ = 0x02;
 		xShift_ = 0;
+		sumShift_ = 0;
 		processFrame_ = &SwStatsCpu::processBayerFrame2;
 
 		switch (bayerFormat.order) {