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

Message ID 20260306-agc-proportional-v4-2-e87c7e0d837a@jetm.me
State New
Headers show
Series
  • Simple pipeline: proportional AGC, AWB stats fix, OV2740 black level
Related show

Commit Message

Javier Tia March 6, 2026, 6: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            | 7 +++++++
 2 files changed, 8 insertions(+)

Comments

Kieran Bingham April 29, 2026, 3:26 p.m. UTC | #1 
Hi Javier,


Quoting Javier Tia (2026-03-06 18:46:41)
> 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            | 7 +++++++
>  2 files changed, 8 insertions(+)
> 
> diff --git a/include/libcamera/internal/software_isp/swstats_cpu.h b/include/libcamera/internal/software_isp/swstats_cpu.h
> index 64b3e23f..37bfde53 100644
> --- a/include/libcamera/internal/software_isp/swstats_cpu.h
> +++ b/include/libcamera/internal/software_isp/swstats_cpu.h
> @@ -115,6 +115,7 @@ private:
>  
>         unsigned int xShift_;
>         unsigned int stride_;
> +       unsigned int sumShift_;
>  
>         SharedMemObject<SwIspStats> sharedStats_;
>         SwIspStats stats_;
> diff --git a/src/libcamera/software_isp/swstats_cpu.cpp b/src/libcamera/software_isp/swstats_cpu.cpp
> index 1cedcfbc..744a7560 100644
> --- a/src/libcamera/software_isp/swstats_cpu.cpp
> +++ b/src/libcamera/software_isp/swstats_cpu.cpp
> @@ -346,6 +346,11 @@ void SwStatsCpu::startFrame(uint32_t frame)
>  void SwStatsCpu::finishFrame(uint32_t frame, uint32_t bufferId)
>  {
>         stats_.valid = frame % kStatPerNumFrames == 0;
> +       if (stats_.valid && sumShift_) {
> +               stats_.sum_.r() >>= sumShift_;
> +               stats_.sum_.g() >>= sumShift_;
> +               stats_.sum_.b() >>= sumShift_;
> +       }

I was trying to get this merged, but unfortunately something changed
here.

Could you take a look and rebase on v0.7.1 please to submit a v5, then I
think we should get this series merged.

Although I think we could do somethign else for 3/3 - that could be done
later and on top.

I'd be interested to know if this is the cure for the flicker - or if
the 3/3 patch is actually needed in fact.

--
Kieran

>         *sharedStats_ = stats_;
>         statsReady.emit(frame, bufferId);
>  }
> @@ -402,6 +407,7 @@ int SwStatsCpu::configure(const StreamConfiguration &inputCfg)
>         if (bayerFormat.packing == BayerFormat::Packing::None &&
>             setupStandardBayerOrder(bayerFormat.order) == 0) {
>                 processFrame_ = &SwStatsCpu::processBayerFrame2;
> +               sumShift_ = bayerFormat.bitDepth - 8;
>                 switch (bayerFormat.bitDepth) {
>                 case 8:
>                         stats0_ = &SwStatsCpu::statsBGGR8Line0;
> @@ -422,6 +428,7 @@ int SwStatsCpu::configure(const StreamConfiguration &inputCfg)
>                 /* Skip every 3th and 4th line, sample every other 2x2 block */
>                 ySkipMask_ = 0x02;
>                 xShift_ = 0;
> +               sumShift_ = 0;
>                 processFrame_ = &SwStatsCpu::processBayerFrame2;
>  
>                 switch (bayerFormat.order) {
> 
> -- 
> 2.53.0
>
Javier Tia May 6, 2026, 10:28 p.m. UTC | #2 
Hi Kieran,

Thanks for the review. Rebased on current master for v5 - the diff
context in finishFrame() changed because of the multi-threading
refactor (6e53e72e), which restructured the function to aggregate
from a stats_ vector before applying the shift. No functional
changes to the patch itself.

On 3/3: I tested this on OV2740 (ThinkPad X1 Carbon Gen 10). Patch
1/3 alone cures the flicker. A 40-frame capture with patches 1/3
and 2/3 applied shows smooth monotonic brightness convergence from
frame 4 onwards with no oscillation. Patch 3/3 (OV2740 BLC=16)
improves AWB color accuracy by providing the correct black level
baseline for channel gain calculation, but it is independent of the
flicker fix.

Including all three in v5 since 3/3 is a genuine calibration
improvement regardless.

v5 has been sent:
https://lists.libcamera.org/pipermail/libcamera-devel/2026-May/058560.html

Best,
Javier

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 64b3e23f..37bfde53 100644
--- a/include/libcamera/internal/software_isp/swstats_cpu.h
+++ b/include/libcamera/internal/software_isp/swstats_cpu.h
@@ -115,6 +115,7 @@  private:
 
 	unsigned int xShift_;
 	unsigned int stride_;
+	unsigned int sumShift_;
 
 	SharedMemObject<SwIspStats> sharedStats_;
 	SwIspStats stats_;
diff --git a/src/libcamera/software_isp/swstats_cpu.cpp b/src/libcamera/software_isp/swstats_cpu.cpp
index 1cedcfbc..744a7560 100644
--- a/src/libcamera/software_isp/swstats_cpu.cpp
+++ b/src/libcamera/software_isp/swstats_cpu.cpp
@@ -346,6 +346,11 @@  void SwStatsCpu::startFrame(uint32_t frame)
 void SwStatsCpu::finishFrame(uint32_t frame, uint32_t bufferId)
 {
 	stats_.valid = frame % kStatPerNumFrames == 0;
+	if (stats_.valid && sumShift_) {
+		stats_.sum_.r() >>= sumShift_;
+		stats_.sum_.g() >>= sumShift_;
+		stats_.sum_.b() >>= sumShift_;
+	}
 	*sharedStats_ = stats_;
 	statsReady.emit(frame, bufferId);
 }
@@ -402,6 +407,7 @@  int SwStatsCpu::configure(const StreamConfiguration &inputCfg)
 	if (bayerFormat.packing == BayerFormat::Packing::None &&
 	    setupStandardBayerOrder(bayerFormat.order) == 0) {
 		processFrame_ = &SwStatsCpu::processBayerFrame2;
+		sumShift_ = bayerFormat.bitDepth - 8;
 		switch (bayerFormat.bitDepth) {
 		case 8:
 			stats0_ = &SwStatsCpu::statsBGGR8Line0;
@@ -422,6 +428,7 @@  int SwStatsCpu::configure(const StreamConfiguration &inputCfg)
 		/* Skip every 3th and 4th line, sample every other 2x2 block */
 		ySkipMask_ = 0x02;
 		xShift_ = 0;
+		sumShift_ = 0;
 		processFrame_ = &SwStatsCpu::processBayerFrame2;
 
 		switch (bayerFormat.order) {