| Message ID | 20251015012251.17508-33-bryan.odonoghue@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Bryan, Bryan O'Donoghue <bryan.odonoghue@linaro.org> writes: > Add the ability to apply a vec3 AWB array against the demosaiced data prior > to application of colour correction data. I may be missing something but I think white balance is already applied in lut.cpp to both the CCM and the R/G/B lookup tables. > Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> > --- > include/libcamera/internal/shaders/bayer_1x_packed.frag | 6 ++++++ > include/libcamera/internal/shaders/bayer_unpacked.frag | 6 ++++++ > 2 files changed, 12 insertions(+) > > diff --git a/include/libcamera/internal/shaders/bayer_1x_packed.frag b/include/libcamera/internal/shaders/bayer_1x_packed.frag > index c0632eb1..8b35dd63 100644 > --- a/include/libcamera/internal/shaders/bayer_1x_packed.frag > +++ b/include/libcamera/internal/shaders/bayer_1x_packed.frag > @@ -69,6 +69,7 @@ uniform sampler2D red_param; > uniform sampler2D green_param; > uniform sampler2D blue_param; > uniform mat3 ccm; > +uniform vec3 awb; > > void main(void) > { > @@ -216,6 +217,11 @@ void main(void) > vec3(patterns.y, C, patterns.x) : > vec3(patterns.wz, C)); > > + /* Apply white balance before colour correction matrix */ > + rgb.r = rgb.r * awb.r; > + rgb.g = rgb.g * awb.g; > + rgb.b = rgb.b * awb.b; > + > #if defined(APPLY_CCM_PARAMETERS) > /* > * CCM is a 3x3 in the format > diff --git a/include/libcamera/internal/shaders/bayer_unpacked.frag b/include/libcamera/internal/shaders/bayer_unpacked.frag > index 74ce1509..37bd4812 100644 > --- a/include/libcamera/internal/shaders/bayer_unpacked.frag > +++ b/include/libcamera/internal/shaders/bayer_unpacked.frag > @@ -28,6 +28,7 @@ varying vec4 center; > varying vec4 yCoord; > varying vec4 xCoord; > uniform mat3 ccm; > +uniform vec3 awb; > > void main(void) { > vec3 rgb; > @@ -111,6 +112,11 @@ void main(void) { > vec3(PATTERN.w, C, PATTERN.z) : > vec3(PATTERN.yx, C)); > > + /* Apply white balance before colour correction matrix */ > + rgb.r = rgb.r * awb.r; > + rgb.g = rgb.g * awb.g; > + rgb.b = rgb.b * awb.b; > + > #if defined(APPLY_CCM_PARAMETERS) > /* > * CCM is a 3x3 in the format
Milan Zamazal <mzamazal@redhat.com> writes: > Hi Bryan, > > Bryan O'Donoghue <bryan.odonoghue@linaro.org> writes: > >> Add the ability to apply a vec3 AWB array against the demosaiced data prior >> to application of colour correction data. > > I may be missing something but I think white balance is already applied > in lut.cpp to both the CCM and the R/G/B lookup tables. I must be something missing, I get a colour cast without this, at least when CCM is not used. >> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> >> --- >> include/libcamera/internal/shaders/bayer_1x_packed.frag | 6 ++++++ >> include/libcamera/internal/shaders/bayer_unpacked.frag | 6 ++++++ >> 2 files changed, 12 insertions(+) >> >> diff --git a/include/libcamera/internal/shaders/bayer_1x_packed.frag b/include/libcamera/internal/shaders/bayer_1x_packed.frag >> index c0632eb1..8b35dd63 100644 >> --- a/include/libcamera/internal/shaders/bayer_1x_packed.frag >> +++ b/include/libcamera/internal/shaders/bayer_1x_packed.frag >> @@ -69,6 +69,7 @@ uniform sampler2D red_param; >> uniform sampler2D green_param; >> uniform sampler2D blue_param; >> uniform mat3 ccm; >> +uniform vec3 awb; >> >> void main(void) >> { >> @@ -216,6 +217,11 @@ void main(void) >> vec3(patterns.y, C, patterns.x) : >> vec3(patterns.wz, C)); >> >> + /* Apply white balance before colour correction matrix */ >> + rgb.r = rgb.r * awb.r; >> + rgb.g = rgb.g * awb.g; >> + rgb.b = rgb.b * awb.b; >> + >> #if defined(APPLY_CCM_PARAMETERS) >> /* >> * CCM is a 3x3 in the format >> diff --git a/include/libcamera/internal/shaders/bayer_unpacked.frag b/include/libcamera/internal/shaders/bayer_unpacked.frag >> index 74ce1509..37bd4812 100644 >> --- a/include/libcamera/internal/shaders/bayer_unpacked.frag >> +++ b/include/libcamera/internal/shaders/bayer_unpacked.frag >> @@ -28,6 +28,7 @@ varying vec4 center; >> varying vec4 yCoord; >> varying vec4 xCoord; >> uniform mat3 ccm; >> +uniform vec3 awb; >> >> void main(void) { >> vec3 rgb; >> @@ -111,6 +112,11 @@ void main(void) { >> vec3(PATTERN.w, C, PATTERN.z) : >> vec3(PATTERN.yx, C)); >> >> + /* Apply white balance before colour correction matrix */ >> + rgb.r = rgb.r * awb.r; >> + rgb.g = rgb.g * awb.g; >> + rgb.b = rgb.b * awb.b; >> + >> #if defined(APPLY_CCM_PARAMETERS) >> /* >> * CCM is a 3x3 in the format
Bryan O'Donoghue <bryan.odonoghue@linaro.org> writes: > Add the ability to apply a vec3 AWB array against the demosaiced data prior > to application of colour correction data. > > Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> > --- > include/libcamera/internal/shaders/bayer_1x_packed.frag | 6 ++++++ > include/libcamera/internal/shaders/bayer_unpacked.frag | 6 ++++++ > 2 files changed, 12 insertions(+) > > diff --git a/include/libcamera/internal/shaders/bayer_1x_packed.frag b/include/libcamera/internal/shaders/bayer_1x_packed.frag > index c0632eb1..8b35dd63 100644 > --- a/include/libcamera/internal/shaders/bayer_1x_packed.frag > +++ b/include/libcamera/internal/shaders/bayer_1x_packed.frag > @@ -69,6 +69,7 @@ uniform sampler2D red_param; > uniform sampler2D green_param; > uniform sampler2D blue_param; > uniform mat3 ccm; > +uniform vec3 awb; > > void main(void) > { > @@ -216,6 +217,11 @@ void main(void) > vec3(patterns.y, C, patterns.x) : > vec3(patterns.wz, C)); > > + /* Apply white balance before colour correction matrix */ > + rgb.r = rgb.r * awb.r; > + rgb.g = rgb.g * awb.g; > + rgb.b = rgb.b * awb.b; This block is misaligned, should be indented by 4 spaces rather than a tab. And I think it can be simplified as: rgb = rgb * awb; And it should be applied only if APPLY_RGB_PARAMETERS. For CCM, it's applied to the CCM in lut.cpp: Matrix<float, 3, 3> gainCcm = { { gains.r(), 0, 0, 0, gains.g(), 0, 0, 0, gains.b() } }; auto ccm = context.activeState.ccm.ccm * gainCcm; The same for the other shader. > + > #if defined(APPLY_CCM_PARAMETERS) > /* > * CCM is a 3x3 in the format > diff --git a/include/libcamera/internal/shaders/bayer_unpacked.frag b/include/libcamera/internal/shaders/bayer_unpacked.frag > index 74ce1509..37bd4812 100644 > --- a/include/libcamera/internal/shaders/bayer_unpacked.frag > +++ b/include/libcamera/internal/shaders/bayer_unpacked.frag > @@ -28,6 +28,7 @@ varying vec4 center; > varying vec4 yCoord; > varying vec4 xCoord; > uniform mat3 ccm; > +uniform vec3 awb; > > void main(void) { > vec3 rgb; > @@ -111,6 +112,11 @@ void main(void) { > vec3(PATTERN.w, C, PATTERN.z) : > vec3(PATTERN.yx, C)); > > + /* Apply white balance before colour correction matrix */ > + rgb.r = rgb.r * awb.r; > + rgb.g = rgb.g * awb.g; > + rgb.b = rgb.b * awb.b; > + > #if defined(APPLY_CCM_PARAMETERS) > /* > * CCM is a 3x3 in the format
Milan Zamazal <mzamazal@redhat.com> writes: > Milan Zamazal <mzamazal@redhat.com> writes: > >> Hi Bryan, >> >> Bryan O'Donoghue <bryan.odonoghue@linaro.org> writes: >> >>> Add the ability to apply a vec3 AWB array against the demosaiced data prior >>> to application of colour correction data. >> >> I may be missing something but I think white balance is already applied >> in lut.cpp to both the CCM and the R/G/B lookup tables. > > I must be something missing, I get a colour cast without this, at least > when CCM is not used. The problem was that the shader looks up all the RGB channels in red_param by mistake, as reported on the corresponding patch. AWB shouldn't be handled in the shaders at all, it's already incorporated in the lookup tables or CCM. With the fixes I reported yesterday and today, I get the same output with and without CCM. \o/ >>> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> >>> --- >>> include/libcamera/internal/shaders/bayer_1x_packed.frag | 6 ++++++ >>> include/libcamera/internal/shaders/bayer_unpacked.frag | 6 ++++++ >>> 2 files changed, 12 insertions(+) >>> >>> diff --git a/include/libcamera/internal/shaders/bayer_1x_packed.frag b/include/libcamera/internal/shaders/bayer_1x_packed.frag >>> index c0632eb1..8b35dd63 100644 >>> --- a/include/libcamera/internal/shaders/bayer_1x_packed.frag >>> +++ b/include/libcamera/internal/shaders/bayer_1x_packed.frag >>> @@ -69,6 +69,7 @@ uniform sampler2D red_param; >>> uniform sampler2D green_param; >>> uniform sampler2D blue_param; >>> uniform mat3 ccm; >>> +uniform vec3 awb; >>> >>> void main(void) >>> { >>> @@ -216,6 +217,11 @@ void main(void) >>> vec3(patterns.y, C, patterns.x) : >>> vec3(patterns.wz, C)); >>> >>> + /* Apply white balance before colour correction matrix */ >>> + rgb.r = rgb.r * awb.r; >>> + rgb.g = rgb.g * awb.g; >>> + rgb.b = rgb.b * awb.b; >>> + >>> #if defined(APPLY_CCM_PARAMETERS) >>> /* >>> * CCM is a 3x3 in the format >>> diff --git a/include/libcamera/internal/shaders/bayer_unpacked.frag b/include/libcamera/internal/shaders/bayer_unpacked.frag >>> index 74ce1509..37bd4812 100644 >>> --- a/include/libcamera/internal/shaders/bayer_unpacked.frag >>> +++ b/include/libcamera/internal/shaders/bayer_unpacked.frag >>> @@ -28,6 +28,7 @@ varying vec4 center; >>> varying vec4 yCoord; >>> varying vec4 xCoord; >>> uniform mat3 ccm; >>> +uniform vec3 awb; >>> >>> void main(void) { >>> vec3 rgb; >>> @@ -111,6 +112,11 @@ void main(void) { >>> vec3(PATTERN.w, C, PATTERN.z) : >>> vec3(PATTERN.yx, C)); >>> >>> + /* Apply white balance before colour correction matrix */ >>> + rgb.r = rgb.r * awb.r; >>> + rgb.g = rgb.g * awb.g; >>> + rgb.b = rgb.b * awb.b; >>> + >>> #if defined(APPLY_CCM_PARAMETERS) >>> /* >>> * CCM is a 3x3 in the format
Milan Zamazal <mzamazal@redhat.com> writes: > Bryan O'Donoghue <bryan.odonoghue@linaro.org> writes: > >> Add the ability to apply a vec3 AWB array against the demosaiced data prior >> to application of colour correction data. >> >> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> >> --- >> include/libcamera/internal/shaders/bayer_1x_packed.frag | 6 ++++++ >> include/libcamera/internal/shaders/bayer_unpacked.frag | 6 ++++++ >> 2 files changed, 12 insertions(+) >> >> diff --git a/include/libcamera/internal/shaders/bayer_1x_packed.frag b/include/libcamera/internal/shaders/bayer_1x_packed.frag >> index c0632eb1..8b35dd63 100644 >> --- a/include/libcamera/internal/shaders/bayer_1x_packed.frag >> +++ b/include/libcamera/internal/shaders/bayer_1x_packed.frag >> @@ -69,6 +69,7 @@ uniform sampler2D red_param; >> uniform sampler2D green_param; >> uniform sampler2D blue_param; >> uniform mat3 ccm; >> +uniform vec3 awb; >> >> void main(void) >> { >> @@ -216,6 +217,11 @@ void main(void) >> vec3(patterns.y, C, patterns.x) : >> vec3(patterns.wz, C)); >> >> + /* Apply white balance before colour correction matrix */ >> + rgb.r = rgb.r * awb.r; >> + rgb.g = rgb.g * awb.g; >> + rgb.b = rgb.b * awb.b; > > This block is misaligned, should be indented by 4 spaces rather than a > tab. And I think it can be simplified as: > > rgb = rgb * awb; > > And it should be applied only if APPLY_RGB_PARAMETERS. It shouldn't be applied anywhere. AWB is already part of the lookup tables created in lut.cpp: auto &gains = context.activeState.awb.gains; ... if (!context.ccmEnabled) { for (unsigned int i = 0; i < DebayerParams::kRGBLookupSize; i++) { /* Apply gamma after gain! */ const RGB<float> lutGains = (gains * i / div).min(gammaTableSize - 1); params->red[i] = gammaTable[static_cast<unsigned int>(lutGains.r())]; params->green[i] = gammaTable[static_cast<unsigned int>(lutGains.g())]; params->blue[i] = gammaTable[static_cast<unsigned int>(lutGains.b())]; An unrelated bug in the shader confused me when testing this. Sorry for the incremental reviews. I think I'm done with v3 review and testing now. > For CCM, it's applied to the CCM in lut.cpp: > > Matrix<float, 3, 3> gainCcm = { { gains.r(), 0, 0, > 0, gains.g(), 0, > 0, 0, gains.b() } }; > auto ccm = context.activeState.ccm.ccm * gainCcm; > > The same for the other shader. > >> + >> #if defined(APPLY_CCM_PARAMETERS) >> /* >> * CCM is a 3x3 in the format >> diff --git a/include/libcamera/internal/shaders/bayer_unpacked.frag b/include/libcamera/internal/shaders/bayer_unpacked.frag >> index 74ce1509..37bd4812 100644 >> --- a/include/libcamera/internal/shaders/bayer_unpacked.frag >> +++ b/include/libcamera/internal/shaders/bayer_unpacked.frag >> @@ -28,6 +28,7 @@ varying vec4 center; >> varying vec4 yCoord; >> varying vec4 xCoord; >> uniform mat3 ccm; >> +uniform vec3 awb; >> >> void main(void) { >> vec3 rgb; >> @@ -111,6 +112,11 @@ void main(void) { >> vec3(PATTERN.w, C, PATTERN.z) : >> vec3(PATTERN.yx, C)); >> >> + /* Apply white balance before colour correction matrix */ >> + rgb.r = rgb.r * awb.r; >> + rgb.g = rgb.g * awb.g; >> + rgb.b = rgb.b * awb.b; >> + >> #if defined(APPLY_CCM_PARAMETERS) >> /* >> * CCM is a 3x3 in the format
Hi! > >> Hi Bryan, > >> > >> Bryan O'Donoghue <bryan.odonoghue at linaro.org> writes: > >> > >>> Add the ability to apply a vec3 AWB array against the demosaiced data prior > >>> to application of colour correction data. > >> > >> I may be missing something but I think white balance is already applied > >> in lut.cpp to both the CCM and the R/G/B lookup tables. > > > > I must be something missing, I get a colour cast without this, at least > > when CCM is not used. > > The problem was that the shader looks up all the RGB channels in > red_param by mistake, as reported on the corresponding patch. > > AWB shouldn't be handled in the shaders at all, it's already > incorporated in the lookup tables or CCM. > > With the fixes I reported yesterday and today, I get the same output > with and without CCM. \o/ I'm still going through the code but... You really, really, really want to handle this in shaders on arm64. Touching the framebuffer data from CPU on the arm64 is very expensive due to cache issues. But you can do everything on the GPU, and then you don't have to pay the cost. v4l (4MP)-> cpu -> gpu (1MP)-> cpu === slow v4l (4MP)-> gpu (1MP)-> cpu === better (Second case is even better if you can scale down while debayering and to YUY2. Then you can actually record 0.8MPix, 30fps videos on slow hardware such as Librem 5). Best regards, Pavel
On Fri 2025-11-14 13:17:54, Pavel Machek wrote: > Hi! > > > >> Hi Bryan, > > >> > > >> Bryan O'Donoghue <bryan.odonoghue at linaro.org> writes: > > >> > > >>> Add the ability to apply a vec3 AWB array against the demosaiced data prior > > >>> to application of colour correction data. > > >> > > >> I may be missing something but I think white balance is already applied > > >> in lut.cpp to both the CCM and the R/G/B lookup tables. > > > > > > I must be something missing, I get a colour cast without this, at least > > > when CCM is not used. > > > > The problem was that the shader looks up all the RGB channels in > > red_param by mistake, as reported on the corresponding patch. > > > > AWB shouldn't be handled in the shaders at all, it's already > > incorporated in the lookup tables or CCM. > > > > With the fixes I reported yesterday and today, I get the same output > > with and without CCM. \o/ > > I'm still going through the code but... > > You really, really, really want to handle this in shaders on arm64. > > Touching the framebuffer data from CPU on the arm64 is very expensive > due to cache issues. But you can do everything on the GPU, and then > you don't have to pay the cost. I misunderstood how this works, I did not expect code to use lookup tables on GPU. > v4l (4MP)-> cpu -> gpu (1MP)-> cpu === slow > v4l (4MP)-> gpu (1MP)-> cpu === better > > (Second case is even better if you can scale down while debayering and > to YUY2. Then you can actually record 0.8MPix, 30fps videos on slow > hardware such as Librem 5). And no, I don't believe current code can do debayer+scale or YUY2. I guess I should take a look after this is merged... or maybe I should look at AF, first. Best regards, Pavel
Hi Pavel! On 14.11.25 13:38, Pavel Machek wrote: > On Fri 2025-11-14 13:17:54, Pavel Machek wrote: >> Hi! >> >>>>> Hi Bryan, >>>>> >>>>> Bryan O'Donoghue <bryan.odonoghue at linaro.org> writes: >>>>> >>>>>> Add the ability to apply a vec3 AWB array against the demosaiced data prior >>>>>> to application of colour correction data. >>>>> I may be missing something but I think white balance is already applied >>>>> in lut.cpp to both the CCM and the R/G/B lookup tables. >>>> I must be something missing, I get a colour cast without this, at least >>>> when CCM is not used. >>> The problem was that the shader looks up all the RGB channels in >>> red_param by mistake, as reported on the corresponding patch. >>> >>> AWB shouldn't be handled in the shaders at all, it's already >>> incorporated in the lookup tables or CCM. >>> >>> With the fixes I reported yesterday and today, I get the same output >>> with and without CCM. \o/ >> I'm still going through the code but... >> >> You really, really, really want to handle this in shaders on arm64. >> >> Touching the framebuffer data from CPU on the arm64 is very expensive >> due to cache issues. But you can do everything on the GPU, and then >> you don't have to pay the cost. > I misunderstood how this works, I did not expect code to use lookup > tables on GPU. > >> v4l (4MP)-> cpu -> gpu (1MP)-> cpu === slow >> v4l (4MP)-> gpu (1MP)-> cpu === better >> >> (Second case is even better if you can scale down while debayering and >> to YUY2. Then you can actually record 0.8MPix, 30fps videos on slow >> hardware such as Librem 5). > And no, I don't believe current code can do debayer+scale or YUY2. I > guess I should take a look after this is merged... or maybe I should > look at AF, first. The current series does implement debayering on the GPU (see debayer_egl), however it takes a round-trip to CPU before uploading, i.e. v4l2 -> CPU -> GPU (debayer / conversion to RGB) -> client (which may use it with GPU or CPU) There is a WIP patch to implement direct GPU import when possible (not planned to get picked before the merge / planned as follow-up) https://gitlab.freedesktop.org/rmader/libcamera/-/commit/1f5c2764c4d42c916e52f9c06c44f4ae52519a78 as well as a patch to implement scaling on the GPU (planned to get picked for v4 IIUC) https://gitlab.freedesktop.org/rmader/libcamera/-/commit/048cc02e295fd82f6ecfb1d5c6280a21709cd0d5 > > Best regards, > > Pavel Best regards
diff --git a/include/libcamera/internal/shaders/bayer_1x_packed.frag b/include/libcamera/internal/shaders/bayer_1x_packed.frag index c0632eb1..8b35dd63 100644 --- a/include/libcamera/internal/shaders/bayer_1x_packed.frag +++ b/include/libcamera/internal/shaders/bayer_1x_packed.frag @@ -69,6 +69,7 @@ uniform sampler2D red_param; uniform sampler2D green_param; uniform sampler2D blue_param; uniform mat3 ccm; +uniform vec3 awb; void main(void) { @@ -216,6 +217,11 @@ void main(void) vec3(patterns.y, C, patterns.x) : vec3(patterns.wz, C)); + /* Apply white balance before colour correction matrix */ + rgb.r = rgb.r * awb.r; + rgb.g = rgb.g * awb.g; + rgb.b = rgb.b * awb.b; + #if defined(APPLY_CCM_PARAMETERS) /* * CCM is a 3x3 in the format diff --git a/include/libcamera/internal/shaders/bayer_unpacked.frag b/include/libcamera/internal/shaders/bayer_unpacked.frag index 74ce1509..37bd4812 100644 --- a/include/libcamera/internal/shaders/bayer_unpacked.frag +++ b/include/libcamera/internal/shaders/bayer_unpacked.frag @@ -28,6 +28,7 @@ varying vec4 center; varying vec4 yCoord; varying vec4 xCoord; uniform mat3 ccm; +uniform vec3 awb; void main(void) { vec3 rgb; @@ -111,6 +112,11 @@ void main(void) { vec3(PATTERN.w, C, PATTERN.z) : vec3(PATTERN.yx, C)); + /* Apply white balance before colour correction matrix */ + rgb.r = rgb.r * awb.r; + rgb.g = rgb.g * awb.g; + rgb.b = rgb.b * awb.b; + #if defined(APPLY_CCM_PARAMETERS) /* * CCM is a 3x3 in the format
Add the ability to apply a vec3 AWB array against the demosaiced data prior to application of colour correction data. Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> --- include/libcamera/internal/shaders/bayer_1x_packed.frag | 6 ++++++ include/libcamera/internal/shaders/bayer_unpacked.frag | 6 ++++++ 2 files changed, 12 insertions(+)