| Message ID | 20260304232533.56D641EA006B@mailuser.phl.internal |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Javier, thank you for the patch. Javier Tia <floss@jetm.me> writes: > 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. Good finding! I've been playing with an independent implementation of software ISP stats+processing and when comparing its output with libcamera output, I could also see a slight green cast in my libcamera images. With your patch, the green cast is gone and the outputs are basically the same. Reviewed-by: Milan Zamazal <mzamazal@redhat.com> Tested-by: Milan Zamazal <mzamazal@redhat.com> > 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> > --- > 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 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 5c3011a7..bf5e1960 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); > } > @@ -405,12 +410,15 @@ int SwStatsCpu::configure(const StreamConfiguration &inputCfg) > 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; > } > } > @@ -422,6 +430,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) {
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 5c3011a7..bf5e1960 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); } @@ -405,12 +410,15 @@ int SwStatsCpu::configure(const StreamConfiguration &inputCfg) 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; } } @@ -422,6 +430,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) {
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> --- include/libcamera/internal/software_isp/swstats_cpu.h | 1 + src/libcamera/software_isp/swstats_cpu.cpp | 9 +++++++++ 2 files changed, 10 insertions(+)