| Message ID | 20260621-kbingham-awb-saturation-v1-0-b91ea59c6cfb@ideasonboard.com |
|---|---|
| Headers | show |
| Series |
|
| Related | show |
Thanks a lot for the series! I just tested it on multiple devices, most
importantly on a FairPhone 5 with the following tuning file from
Kieran used for the imx858 as well as the s5kjn1:
# SPDX-License-Identifier: CC0-1.0
%YAML 1.1
---
version: 1
algorithms:
- BlackLevel:
blackLevel: 4096
- Awb:
# Color correction matrices can be defined here. The CCM algorithm
# has a significant performance impact, and should only be enabled
# if tuned.
- Ccm:
ccms:
- ct: 2676
ccm: [ 2.02152, -0.41859, -0.60292,
-0.59481, 2.21509, -0.62029,
-0.44029, -1.16865, 2.60894 ]
- ct: 6890
ccm: [ 1.93867, -0.67486, -0.26381,
-0.43049, 2.00702, -0.57652,
-0.15696, -0.51345, 1.67041 ]
- Adjust:
- Agc:
...
and can confirm the previously observed saturation bugs are gone -
applying the CCM now clearly improves the overall result and is IMO
ready for shipping in pmOS!
No other regressions spotted, thus the whole series is:
Tested-by: Robert Mader <robert.mader@collabora.com>
On 21.06.26 01:00, Kieran Bingham wrote:
> These patches have floated around in various forms and versions, but
> this series aims to collate the main parts of fixing black level and
> saturation bugs in the SoftISP for both GPU ISP and CPU ISP.
>
> This is predominantly fixed by splitting the AWB gains out of the CCM
> which has previously been 'combined'. Combining means we do not
> correctly saturate at the sensor levels when applying gains. So now we
> cna do that and clamp accordingly.
>
> To get there, I've added a new clamp operation to the Vector class (and
> fixed up one usage of that in the rkisp1 ... yeah - I'm trying not to go
> down rabbit holes, and focus on saturation but adding a clamp was a
> previous review request ...)
>
> I've made use of Milan's existing Black Level Correction fixes, and
> picked up and applied all previous review comments and refactoring to
> get here.
>
> So far - this is tested on my x13s laptop (with an arbitrary tuning file
> to make sure the CCM is tested) for both GPU and CPU ISP....
>
> Please test if you can and I'm really keen to merge these so that
> both Jacopo's and Bryan's work can be rebased on top.
>
> Signed-off-by: Kieran Bingham<kieran.bingham@ideasonboard.com>
> ---
> Kieran Bingham (4):
> libcamera: vector: Add clamp operation
> ipa: rkisp1: awb: Utilise clamp function
> softisp: Split AWB from Combined Matrix
> shaders: bayer: Use native matrix multiplication
>
> Milan Zamazal (3):
> ipa: simple: Limit the black level value
> libcamera: software_isp: Fix black level application in GPU ISP
> libcamera: software_isp: Fix black level handling in CPU ISP
>
> include/libcamera/internal/vector.h | 8 ++++
> src/ipa/rkisp1/algorithms/awb.cpp | 2 +-
> src/ipa/simple/algorithms/awb.cpp | 16 ++------
> src/ipa/simple/algorithms/blc.cpp | 7 ++--
> src/ipa/simple/ipa_context.h | 5 +--
> src/libcamera/shaders/bayer_1x_packed.frag | 51 +++++-------------------
> src/libcamera/shaders/bayer_unpacked.frag | 51 +++++-------------------
> src/libcamera/software_isp/debayer_cpu.cpp | 64 ++++++++++++++----------------
> src/libcamera/software_isp/debayer_egl.cpp | 18 +++------
> src/libcamera/software_isp/debayer_egl.h | 3 ++
> src/libcamera/vector.cpp | 8 ++++
> test/vector.cpp | 1 +
> 12 files changed, 83 insertions(+), 151 deletions(-)
> ---
> base-commit: 84e1336956a78f7b416f8bd2abf747d93ad64260
> change-id: 20260620-kbingham-awb-saturation-39e26bd1b580
>
> Best regards,
These patches have floated around in various forms and versions, but this series aims to collate the main parts of fixing black level and saturation bugs in the SoftISP for both GPU ISP and CPU ISP. This is predominantly fixed by splitting the AWB gains out of the CCM which has previously been 'combined'. Combining means we do not correctly saturate at the sensor levels when applying gains. So now we cna do that and clamp accordingly. To get there, I've added a new clamp operation to the Vector class (and fixed up one usage of that in the rkisp1 ... yeah - I'm trying not to go down rabbit holes, and focus on saturation but adding a clamp was a previous review request ...) I've made use of Milan's existing Black Level Correction fixes, and picked up and applied all previous review comments and refactoring to get here. So far - this is tested on my x13s laptop (with an arbitrary tuning file to make sure the CCM is tested) for both GPU and CPU ISP.... Please test if you can and I'm really keen to merge these so that both Jacopo's and Bryan's work can be rebased on top. Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> --- Kieran Bingham (4): libcamera: vector: Add clamp operation ipa: rkisp1: awb: Utilise clamp function softisp: Split AWB from Combined Matrix shaders: bayer: Use native matrix multiplication Milan Zamazal (3): ipa: simple: Limit the black level value libcamera: software_isp: Fix black level application in GPU ISP libcamera: software_isp: Fix black level handling in CPU ISP include/libcamera/internal/vector.h | 8 ++++ src/ipa/rkisp1/algorithms/awb.cpp | 2 +- src/ipa/simple/algorithms/awb.cpp | 16 ++------ src/ipa/simple/algorithms/blc.cpp | 7 ++-- src/ipa/simple/ipa_context.h | 5 +-- src/libcamera/shaders/bayer_1x_packed.frag | 51 +++++------------------- src/libcamera/shaders/bayer_unpacked.frag | 51 +++++------------------- src/libcamera/software_isp/debayer_cpu.cpp | 64 ++++++++++++++---------------- src/libcamera/software_isp/debayer_egl.cpp | 18 +++------ src/libcamera/software_isp/debayer_egl.h | 3 ++ src/libcamera/vector.cpp | 8 ++++ test/vector.cpp | 1 + 12 files changed, 83 insertions(+), 151 deletions(-) --- base-commit: 84e1336956a78f7b416f8bd2abf747d93ad64260 change-id: 20260620-kbingham-awb-saturation-39e26bd1b580 Best regards,