| Message ID | 20260211131728.96413-3-mzamazal@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
On 11/02/2026 13:17, Milan Zamazal wrote: > In GPU ISP fragment shaders, the black level is simply subtracted from > the pixel value. This means the highest pixel values can never be > reached, possibly resulting in wrong brightness or colour shifts. Fix > this by spreading the resulting value to the whole 0.0..1.0 range. > > The preceding simple pipeline IPA patch ensures `blacklevel' is less > than 1.0, preventing division by zero here. > > Signed-off-by: Milan Zamazal <mzamazal@redhat.com> > --- > src/libcamera/shaders/bayer_1x_packed.frag | 2 +- > src/libcamera/shaders/bayer_unpacked.frag | 2 +- > 2 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/src/libcamera/shaders/bayer_1x_packed.frag b/src/libcamera/shaders/bayer_1x_packed.frag > index 23747f78a..06ddc040b 100644 > --- a/src/libcamera/shaders/bayer_1x_packed.frag > +++ b/src/libcamera/shaders/bayer_1x_packed.frag > @@ -225,7 +225,7 @@ void main(void) > vec3(patterns.y, C, patterns.x) : > vec3(patterns.wz, C)); > > - rgb = rgb - blacklevel; > + rgb = (rgb - blacklevel) / (1.0 - blacklevel); > > /* > * CCM is a 3x3 in the format > diff --git a/src/libcamera/shaders/bayer_unpacked.frag b/src/libcamera/shaders/bayer_unpacked.frag > index 1b85196ae..98dea512c 100644 > --- a/src/libcamera/shaders/bayer_unpacked.frag > +++ b/src/libcamera/shaders/bayer_unpacked.frag > @@ -128,7 +128,7 @@ void main(void) { > vec3(PATTERN.w, C, PATTERN.z) : > vec3(PATTERN.yx, C)); > > - rgb = rgb - blacklevel; > + rgb = (rgb - blacklevel) / (1.0 - blacklevel); > > /* > * CCM is a 3x3 in the format Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
2026. 02. 11. 14:17 keltezéssel, Milan Zamazal írta: > In GPU ISP fragment shaders, the black level is simply subtracted from > the pixel value. This means the highest pixel values can never be > reached, possibly resulting in wrong brightness or colour shifts. Fix > this by spreading the resulting value to the whole 0.0..1.0 range. > > The preceding simple pipeline IPA patch ensures `blacklevel' is less > than 1.0, preventing division by zero here. > > Signed-off-by: Milan Zamazal <mzamazal@redhat.com> > --- > src/libcamera/shaders/bayer_1x_packed.frag | 2 +- > src/libcamera/shaders/bayer_unpacked.frag | 2 +- > 2 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/src/libcamera/shaders/bayer_1x_packed.frag b/src/libcamera/shaders/bayer_1x_packed.frag > index 23747f78a..06ddc040b 100644 > --- a/src/libcamera/shaders/bayer_1x_packed.frag > +++ b/src/libcamera/shaders/bayer_1x_packed.frag > @@ -225,7 +225,7 @@ void main(void) > vec3(patterns.y, C, patterns.x) : > vec3(patterns.wz, C)); > > - rgb = rgb - blacklevel; > + rgb = (rgb - blacklevel) / (1.0 - blacklevel); This may be a dumb question, but how come this is not written as: rgb = clamp(rgb, blackLevel, 1.0) / (1.0 - blackLevel) Can't negative values cause issues in later calculations? > > /* > * CCM is a 3x3 in the format > diff --git a/src/libcamera/shaders/bayer_unpacked.frag b/src/libcamera/shaders/bayer_unpacked.frag > index 1b85196ae..98dea512c 100644 > --- a/src/libcamera/shaders/bayer_unpacked.frag > +++ b/src/libcamera/shaders/bayer_unpacked.frag > @@ -128,7 +128,7 @@ void main(void) { > vec3(PATTERN.w, C, PATTERN.z) : > vec3(PATTERN.yx, C)); > > - rgb = rgb - blacklevel; > + rgb = (rgb - blacklevel) / (1.0 - blacklevel); > > /* > * CCM is a 3x3 in the format
Barnabás Pőcze <barnabas.pocze@ideasonboard.com> writes: > 2026. 02. 11. 14:17 keltezéssel, Milan Zamazal írta: >> In GPU ISP fragment shaders, the black level is simply subtracted from >> the pixel value. This means the highest pixel values can never be >> reached, possibly resulting in wrong brightness or colour shifts. Fix >> this by spreading the resulting value to the whole 0.0..1.0 range. >> The preceding simple pipeline IPA patch ensures `blacklevel' is less >> than 1.0, preventing division by zero here. >> Signed-off-by: Milan Zamazal <mzamazal@redhat.com> >> --- >> src/libcamera/shaders/bayer_1x_packed.frag | 2 +- >> src/libcamera/shaders/bayer_unpacked.frag | 2 +- >> 2 files changed, 2 insertions(+), 2 deletions(-) >> diff --git a/src/libcamera/shaders/bayer_1x_packed.frag b/src/libcamera/shaders/bayer_1x_packed.frag >> index 23747f78a..06ddc040b 100644 >> --- a/src/libcamera/shaders/bayer_1x_packed.frag >> +++ b/src/libcamera/shaders/bayer_1x_packed.frag >> @@ -225,7 +225,7 @@ void main(void) >> vec3(patterns.y, C, patterns.x) : >> vec3(patterns.wz, C)); >> - rgb = rgb - blacklevel; >> + rgb = (rgb - blacklevel) / (1.0 - blacklevel); > > This may be a dumb question, but how come this is not written as: > > rgb = clamp(rgb, blackLevel, 1.0) / (1.0 - blackLevel) > > Can't negative values cause issues in later calculations? Currently not, because CCM may contain negative values and produce negative pixel values anyway. The resulting value is clamped later. So in case rgb < blackLevel (not expected, but still possible), the most likely result (depending on the CCM) is that it ends up being 0 eventually. That said, I think yes, clamping the value here is better, to prevent possible future trouble once more functionality is added to the shaders. >> /* >> * CCM is a 3x3 in the format >> diff --git a/src/libcamera/shaders/bayer_unpacked.frag b/src/libcamera/shaders/bayer_unpacked.frag >> index 1b85196ae..98dea512c 100644 >> --- a/src/libcamera/shaders/bayer_unpacked.frag >> +++ b/src/libcamera/shaders/bayer_unpacked.frag >> @@ -128,7 +128,7 @@ void main(void) { >> vec3(PATTERN.w, C, PATTERN.z) : >> vec3(PATTERN.yx, C)); >> - rgb = rgb - blacklevel; >> + rgb = (rgb - blacklevel) / (1.0 - blacklevel); >> /* >> * CCM is a 3x3 in the format
Milan Zamazal <mzamazal@redhat.com> writes: > Barnabás Pőcze <barnabas.pocze@ideasonboard.com> writes: > >> 2026. 02. 11. 14:17 keltezéssel, Milan Zamazal írta: >>> In GPU ISP fragment shaders, the black level is simply subtracted from >>> the pixel value. This means the highest pixel values can never be >>> reached, possibly resulting in wrong brightness or colour shifts. Fix >>> this by spreading the resulting value to the whole 0.0..1.0 range. >>> The preceding simple pipeline IPA patch ensures `blacklevel' is less >>> than 1.0, preventing division by zero here. >>> Signed-off-by: Milan Zamazal <mzamazal@redhat.com> >>> --- >>> src/libcamera/shaders/bayer_1x_packed.frag | 2 +- >>> src/libcamera/shaders/bayer_unpacked.frag | 2 +- >>> 2 files changed, 2 insertions(+), 2 deletions(-) >>> diff --git a/src/libcamera/shaders/bayer_1x_packed.frag b/src/libcamera/shaders/bayer_1x_packed.frag >>> index 23747f78a..06ddc040b 100644 >>> --- a/src/libcamera/shaders/bayer_1x_packed.frag >>> +++ b/src/libcamera/shaders/bayer_1x_packed.frag >>> @@ -225,7 +225,7 @@ void main(void) >>> vec3(patterns.y, C, patterns.x) : >>> vec3(patterns.wz, C)); >>> - rgb = rgb - blacklevel; >>> + rgb = (rgb - blacklevel) / (1.0 - blacklevel); >> >> This may be a dumb question, but how come this is not written as: >> >> rgb = clamp(rgb, blackLevel, 1.0) / (1.0 - blackLevel) BTW did you actually mean rgb = (clamp(rgb, blacklevel, 1.0) - blacklevel) / (1.0 - blacklevel) or rgb = clamp((rgb - blacklevel) / (1.0 - blacklevel), 0.0, 1.0) ? (I don't think there is a simpler way to express it using common GLSL functions.) >> Can't negative values cause issues in later calculations? > > Currently not, because CCM may contain negative values and produce > negative pixel values anyway. The resulting value is clamped later. So > in case rgb < blackLevel (not expected, but still possible), the most > likely result (depending on the CCM) is that it ends up being 0 > eventually. > > That said, I think yes, clamping the value here is better, to prevent > possible future trouble once more functionality is added to the shaders. > >>> /* >>> * CCM is a 3x3 in the format >>> diff --git a/src/libcamera/shaders/bayer_unpacked.frag b/src/libcamera/shaders/bayer_unpacked.frag >>> index 1b85196ae..98dea512c 100644 >>> --- a/src/libcamera/shaders/bayer_unpacked.frag >>> +++ b/src/libcamera/shaders/bayer_unpacked.frag >>> @@ -128,7 +128,7 @@ void main(void) { >>> vec3(PATTERN.w, C, PATTERN.z) : >>> vec3(PATTERN.yx, C)); >>> - rgb = rgb - blacklevel; >>> + rgb = (rgb - blacklevel) / (1.0 - blacklevel); >>> /* >>> * CCM is a 3x3 in the format
On 11/02/2026 15:47, Milan Zamazal wrote:
> rgb = clamp((rgb - blacklevel) / (1.0 - blacklevel), 0.0, 1.0)
This one, as you can never be outside the 0.0-1.0 window then.
---
bod
2026. 02. 11. 16:47 keltezéssel, Milan Zamazal írta: > Milan Zamazal <mzamazal@redhat.com> writes: > >> Barnabás Pőcze <barnabas.pocze@ideasonboard.com> writes: >> >>> 2026. 02. 11. 14:17 keltezéssel, Milan Zamazal írta: >>>> In GPU ISP fragment shaders, the black level is simply subtracted from >>>> the pixel value. This means the highest pixel values can never be >>>> reached, possibly resulting in wrong brightness or colour shifts. Fix >>>> this by spreading the resulting value to the whole 0.0..1.0 range. >>>> The preceding simple pipeline IPA patch ensures `blacklevel' is less >>>> than 1.0, preventing division by zero here. >>>> Signed-off-by: Milan Zamazal <mzamazal@redhat.com> >>>> --- >>>> src/libcamera/shaders/bayer_1x_packed.frag | 2 +- >>>> src/libcamera/shaders/bayer_unpacked.frag | 2 +- >>>> 2 files changed, 2 insertions(+), 2 deletions(-) >>>> diff --git a/src/libcamera/shaders/bayer_1x_packed.frag b/src/libcamera/shaders/bayer_1x_packed.frag >>>> index 23747f78a..06ddc040b 100644 >>>> --- a/src/libcamera/shaders/bayer_1x_packed.frag >>>> +++ b/src/libcamera/shaders/bayer_1x_packed.frag >>>> @@ -225,7 +225,7 @@ void main(void) >>>> vec3(patterns.y, C, patterns.x) : >>>> vec3(patterns.wz, C)); >>>> - rgb = rgb - blacklevel; >>>> + rgb = (rgb - blacklevel) / (1.0 - blacklevel); >>> >>> This may be a dumb question, but how come this is not written as: >>> >>> rgb = clamp(rgb, blackLevel, 1.0) / (1.0 - blackLevel) > > BTW did you actually mean > > rgb = (clamp(rgb, blacklevel, 1.0) - blacklevel) / (1.0 - blacklevel) > > or > > rgb = clamp((rgb - blacklevel) / (1.0 - blacklevel), 0.0, 1.0) > > ? > > (I don't think there is a simpler way to express it using common GLSL > functions.) Oops, yes, I have forgotten a subtraction. Assuming 0 <= blacklevel < 1, the two seem to be mathematically equivalent. So I think either is fine, but clamping at the end makes the intent clearer in my opinion. > >>> Can't negative values cause issues in later calculations? >> >> Currently not, because CCM may contain negative values and produce >> negative pixel values anyway. The resulting value is clamped later. So >> in case rgb < blackLevel (not expected, but still possible), the most >> likely result (depending on the CCM) is that it ends up being 0 >> eventually. >> >> That said, I think yes, clamping the value here is better, to prevent >> possible future trouble once more functionality is added to the shaders. >> >>>> /* >>>> * CCM is a 3x3 in the format >>>> diff --git a/src/libcamera/shaders/bayer_unpacked.frag b/src/libcamera/shaders/bayer_unpacked.frag >>>> index 1b85196ae..98dea512c 100644 >>>> --- a/src/libcamera/shaders/bayer_unpacked.frag >>>> +++ b/src/libcamera/shaders/bayer_unpacked.frag >>>> @@ -128,7 +128,7 @@ void main(void) { >>>> vec3(PATTERN.w, C, PATTERN.z) : >>>> vec3(PATTERN.yx, C)); >>>> - rgb = rgb - blacklevel; >>>> + rgb = (rgb - blacklevel) / (1.0 - blacklevel); >>>> /* >>>> * CCM is a 3x3 in the format >
Hi Milan, Thank you for the patch. On Wed, Feb 11, 2026 at 02:17:28PM +0100, Milan Zamazal wrote: > In GPU ISP fragment shaders, the black level is simply subtracted from > the pixel value. This means the highest pixel values can never be > reached, possibly resulting in wrong brightness or colour shifts. Fix > this by spreading the resulting value to the whole 0.0..1.0 range. It's expected though. Black level substraction is not the place where you should perform histogram spreading, this is the job of tone mapping, further down the pipeline. > The preceding simple pipeline IPA patch ensures `blacklevel' is less > than 1.0, preventing division by zero here. > > Signed-off-by: Milan Zamazal <mzamazal@redhat.com> > --- > src/libcamera/shaders/bayer_1x_packed.frag | 2 +- > src/libcamera/shaders/bayer_unpacked.frag | 2 +- > 2 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/src/libcamera/shaders/bayer_1x_packed.frag b/src/libcamera/shaders/bayer_1x_packed.frag > index 23747f78a..06ddc040b 100644 > --- a/src/libcamera/shaders/bayer_1x_packed.frag > +++ b/src/libcamera/shaders/bayer_1x_packed.frag > @@ -225,7 +225,7 @@ void main(void) > vec3(patterns.y, C, patterns.x) : > vec3(patterns.wz, C)); > > - rgb = rgb - blacklevel; > + rgb = (rgb - blacklevel) / (1.0 - blacklevel); > > /* > * CCM is a 3x3 in the format > diff --git a/src/libcamera/shaders/bayer_unpacked.frag b/src/libcamera/shaders/bayer_unpacked.frag > index 1b85196ae..98dea512c 100644 > --- a/src/libcamera/shaders/bayer_unpacked.frag > +++ b/src/libcamera/shaders/bayer_unpacked.frag > @@ -128,7 +128,7 @@ void main(void) { > vec3(PATTERN.w, C, PATTERN.z) : > vec3(PATTERN.yx, C)); > > - rgb = rgb - blacklevel; > + rgb = (rgb - blacklevel) / (1.0 - blacklevel); > > /* > * CCM is a 3x3 in the format
Hi, On 11-Feb-26 18:50, Laurent Pinchart wrote: > Hi Milan, > > Thank you for the patch. > > On Wed, Feb 11, 2026 at 02:17:28PM +0100, Milan Zamazal wrote: >> In GPU ISP fragment shaders, the black level is simply subtracted from >> the pixel value. This means the highest pixel values can never be >> reached, possibly resulting in wrong brightness or colour shifts. Fix >> this by spreading the resulting value to the whole 0.0..1.0 range. > > It's expected though. Black level substraction is not the place where > you should perform histogram spreading, this is the job of tone mapping, > further down the pipeline. At the moment we are not correcting for this at all though. So I do think this change makes sense. I do wonder if we cannot fold this into some future multiplication though to reduce the amount of multiplications the GPU needs to do? E.g. multiply all values in the CCM by 1.0 / (1.0 - blacklevel) ? Then we get the same correction factor for free. Regards, Hans > >> The preceding simple pipeline IPA patch ensures `blacklevel' is less >> than 1.0, preventing division by zero here. >> >> Signed-off-by: Milan Zamazal <mzamazal@redhat.com> >> --- >> src/libcamera/shaders/bayer_1x_packed.frag | 2 +- >> src/libcamera/shaders/bayer_unpacked.frag | 2 +- >> 2 files changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/src/libcamera/shaders/bayer_1x_packed.frag b/src/libcamera/shaders/bayer_1x_packed.frag >> index 23747f78a..06ddc040b 100644 >> --- a/src/libcamera/shaders/bayer_1x_packed.frag >> +++ b/src/libcamera/shaders/bayer_1x_packed.frag >> @@ -225,7 +225,7 @@ void main(void) >> vec3(patterns.y, C, patterns.x) : >> vec3(patterns.wz, C)); >> >> - rgb = rgb - blacklevel; >> + rgb = (rgb - blacklevel) / (1.0 - blacklevel); >> >> /* >> * CCM is a 3x3 in the format >> diff --git a/src/libcamera/shaders/bayer_unpacked.frag b/src/libcamera/shaders/bayer_unpacked.frag >> index 1b85196ae..98dea512c 100644 >> --- a/src/libcamera/shaders/bayer_unpacked.frag >> +++ b/src/libcamera/shaders/bayer_unpacked.frag >> @@ -128,7 +128,7 @@ void main(void) { >> vec3(PATTERN.w, C, PATTERN.z) : >> vec3(PATTERN.yx, C)); >> >> - rgb = rgb - blacklevel; >> + rgb = (rgb - blacklevel) / (1.0 - blacklevel); >> >> /* >> * CCM is a 3x3 in the format >
On Wed, Feb 11, 2026 at 07:08:35PM +0100, johannes.goede@oss.qualcomm.com wrote: > On 11-Feb-26 18:50, Laurent Pinchart wrote: > > On Wed, Feb 11, 2026 at 02:17:28PM +0100, Milan Zamazal wrote: > >> In GPU ISP fragment shaders, the black level is simply subtracted from > >> the pixel value. This means the highest pixel values can never be > >> reached, possibly resulting in wrong brightness or colour shifts. Fix > >> this by spreading the resulting value to the whole 0.0..1.0 range. > > > > It's expected though. Black level substraction is not the place where > > you should perform histogram spreading, this is the job of tone mapping, > > further down the pipeline. > > At the moment we are not correcting for this at all though. So I do > think this change makes sense. I'm worried that we are adding quite a few changes that go in the opposite direction of the one we should be taking, and it will then be more difficult to reverse course. > I do wonder if we cannot fold this into some future multiplication > though to reduce the amount of multiplications the GPU needs to do? > > E.g. multiply all values in the CCM by 1.0 / (1.0 - blacklevel) ? > > Then we get the same correction factor for free. > > >> The preceding simple pipeline IPA patch ensures `blacklevel' is less > >> than 1.0, preventing division by zero here. > >> > >> Signed-off-by: Milan Zamazal <mzamazal@redhat.com> > >> --- > >> src/libcamera/shaders/bayer_1x_packed.frag | 2 +- > >> src/libcamera/shaders/bayer_unpacked.frag | 2 +- > >> 2 files changed, 2 insertions(+), 2 deletions(-) > >> > >> diff --git a/src/libcamera/shaders/bayer_1x_packed.frag b/src/libcamera/shaders/bayer_1x_packed.frag > >> index 23747f78a..06ddc040b 100644 > >> --- a/src/libcamera/shaders/bayer_1x_packed.frag > >> +++ b/src/libcamera/shaders/bayer_1x_packed.frag > >> @@ -225,7 +225,7 @@ void main(void) > >> vec3(patterns.y, C, patterns.x) : > >> vec3(patterns.wz, C)); > >> > >> - rgb = rgb - blacklevel; > >> + rgb = (rgb - blacklevel) / (1.0 - blacklevel); > >> > >> /* > >> * CCM is a 3x3 in the format > >> diff --git a/src/libcamera/shaders/bayer_unpacked.frag b/src/libcamera/shaders/bayer_unpacked.frag > >> index 1b85196ae..98dea512c 100644 > >> --- a/src/libcamera/shaders/bayer_unpacked.frag > >> +++ b/src/libcamera/shaders/bayer_unpacked.frag > >> @@ -128,7 +128,7 @@ void main(void) { > >> vec3(PATTERN.w, C, PATTERN.z) : > >> vec3(PATTERN.yx, C)); > >> > >> - rgb = rgb - blacklevel; > >> + rgb = (rgb - blacklevel) / (1.0 - blacklevel); > >> > >> /* > >> * CCM is a 3x3 in the format
Hi Laurent, Laurent Pinchart <laurent.pinchart@ideasonboard.com> writes: > On Wed, Feb 11, 2026 at 07:08:35PM +0100, johannes.goede@oss.qualcomm.com wrote: >> On 11-Feb-26 18:50, Laurent Pinchart wrote: >> > On Wed, Feb 11, 2026 at 02:17:28PM +0100, Milan Zamazal wrote: > >> >> In GPU ISP fragment shaders, the black level is simply subtracted from >> >> the pixel value. This means the highest pixel values can never be >> >> reached, possibly resulting in wrong brightness or colour shifts. Fix >> >> this by spreading the resulting value to the whole 0.0..1.0 range. >> > >> > It's expected though. Black level substraction is not the place where >> > you should perform histogram spreading, this is the job of tone mapping, >> > further down the pipeline. >> >> At the moment we are not correcting for this at all though. So I do >> think this change makes sense. > > I'm worried that we are adding quite a few changes that go in the > opposite direction of the one we should be taking, and it will then be > more difficult to reverse course. What would be the correct solution with the current state? Do you mean something like step 8 in https://karaimer.github.io/camera-pipeline/? Should a histogram spreading be performed between CCM and contrast? Clipping to 0.0 at bottom and determining an approximately maximum value (regardless of colour?) from the image and then dividing by that value, which can be any positive number? Stuff for compute shaders? Or doing something else? In any case, when applying black level, we should still clip values below it since they are invalid, right? >> I do wonder if we cannot fold this into some future multiplication >> though to reduce the amount of multiplications the GPU needs to do? >> >> E.g. multiply all values in the CCM by 1.0 / (1.0 - blacklevel) ? >> >> Then we get the same correction factor for free. >> >> >> The preceding simple pipeline IPA patch ensures `blacklevel' is less >> >> than 1.0, preventing division by zero here. >> >> >> >> Signed-off-by: Milan Zamazal <mzamazal@redhat.com> >> >> --- >> >> src/libcamera/shaders/bayer_1x_packed.frag | 2 +- >> >> src/libcamera/shaders/bayer_unpacked.frag | 2 +- >> >> 2 files changed, 2 insertions(+), 2 deletions(-) >> >> >> >> diff --git a/src/libcamera/shaders/bayer_1x_packed.frag b/src/libcamera/shaders/bayer_1x_packed.frag >> >> index 23747f78a..06ddc040b 100644 >> >> --- a/src/libcamera/shaders/bayer_1x_packed.frag >> >> +++ b/src/libcamera/shaders/bayer_1x_packed.frag >> >> @@ -225,7 +225,7 @@ void main(void) >> >> vec3(patterns.y, C, patterns.x) : >> >> vec3(patterns.wz, C)); >> >> >> >> - rgb = rgb - blacklevel; >> >> + rgb = (rgb - blacklevel) / (1.0 - blacklevel); >> >> >> >> /* >> >> * CCM is a 3x3 in the format >> >> diff --git a/src/libcamera/shaders/bayer_unpacked.frag b/src/libcamera/shaders/bayer_unpacked.frag >> >> index 1b85196ae..98dea512c 100644 >> >> --- a/src/libcamera/shaders/bayer_unpacked.frag >> >> +++ b/src/libcamera/shaders/bayer_unpacked.frag >> >> @@ -128,7 +128,7 @@ void main(void) { >> >> vec3(PATTERN.w, C, PATTERN.z) : >> >> vec3(PATTERN.yx, C)); >> >> >> >> - rgb = rgb - blacklevel; >> >> + rgb = (rgb - blacklevel) / (1.0 - blacklevel); >> >> >> >> /* >> >> * CCM is a 3x3 in the format
On 11/02/2026 21:48, Milan Zamazal wrote: > In any case, when applying black level, we should still clip values > below it since they are invalid, right? So. Here is a simple fix which I have tested that applies the BLC before demosiac - it also includes the clamping for the range. The one thing I've not done here is differentiate between blacklevel.r blacklevel.g and blacklevel.b - Right now we use the same value for all channels - Complexity I can't say I can really "see" with my eyes a difference but, it is logically closer to what Laurent is talking about. --- bod
On 14/02/2026 14:29, Bryan O'Donoghue wrote:
> - Right now we use the same value for all channels
Right now SoftISP supplies the same value for r, g and b so it doesn't
seem really worthwhile adding the additional code to account for
separate values until we actually start to supply that data.
---
bod
Bryan O'Donoghue <bryan.odonoghue@linaro.org> writes: > On 11/02/2026 21:48, Milan Zamazal wrote: >> In any case, when applying black level, we should still clip values >> below it since they are invalid, right? > > So. > > Here is a simple fix which I have tested that applies the BLC before demosiac - it also includes the > clamping for the range. Looking at performance impact in my environment with the unpacked shader, which uses more fetch's per pixel (12 unpacked x 8 packed): - ~2% for the black level subtraction - additional ~7% for adding low bound clamping: max(pixel, 0.0) Once we have multipass, I'm curious whether compute shaders might do this faster, by avoiding applying the same operation 12-times per pixel. > The one thing I've not done here is differentiate between blacklevel.r blacklevel.g and blacklevel.b > > - Right now we use the same value for all channels > - Complexity If we differentiated between black level colour channels now, we would likely have a choice between complicated implementation or poor performance. Again, if we could do this in a preparatory pass, it would make things easier and without that much additional impact. BTW I wonder why we use different debayering algorithms for packed and unpacked data. > I can't say I can really "see" with my eyes a difference but, it is logically closer to what Laurent is > talking about. > > --- > bod
On 16/02/2026 09:51, Milan Zamazal wrote: > Bryan O'Donoghue <bryan.odonoghue@linaro.org> writes: > >> On 11/02/2026 21:48, Milan Zamazal wrote: >>> In any case, when applying black level, we should still clip values >>> below it since they are invalid, right? >> >> So. >> >> Here is a simple fix which I have tested that applies the BLC before demosiac - it also includes the >> clamping for the range. > > Looking at performance impact in my environment with the unpacked > shader, which uses more fetch's per pixel (12 unpacked x 8 packed): > > - ~2% for the black level subtraction > > - additional ~7% for adding low bound clamping: max(pixel, 0.0) > > Once we have multipass, I'm curious whether compute shaders might do > this faster, by avoiding applying the same operation 12-times per pixel. Yeah for multipass what we would do is 1. Run a BLC shader. The job of this shader is to read the raw and write the BLC corrected back per-pixel 2. Render to texture 3. Feed that texture into our existing bayer shader Render-to-texture and passing textures from one shader to the next is how we don't have to care about having individual buffers per algorithm. We just need the input/output buffers we have now, render to texture in the intermediate stages and feed one texture into the next shader. So then yeah @ the bayer sample stage we would be reading BLC corrected data which has only run once. > >> The one thing I've not done here is differentiate between blacklevel.r blacklevel.g and blacklevel.b >> >> - Right now we use the same value for all channels >> - Complexity > > If we differentiated between black level colour channels now, we would > likely have a choice between complicated implementation or poor > performance. Again, if we could do this in a preparatory pass, it would > make things easier and without that much additional impact. > > BTW I wonder why we use different debayering algorithms for packed and > unpacked data. > >> I can't say I can really "see" with my eyes a difference but, it is logically closer to what Laurent is >> talking about. >> >> --- >> bod >
diff --git a/src/libcamera/shaders/bayer_1x_packed.frag b/src/libcamera/shaders/bayer_1x_packed.frag index 23747f78a..06ddc040b 100644 --- a/src/libcamera/shaders/bayer_1x_packed.frag +++ b/src/libcamera/shaders/bayer_1x_packed.frag @@ -225,7 +225,7 @@ void main(void) vec3(patterns.y, C, patterns.x) : vec3(patterns.wz, C)); - rgb = rgb - blacklevel; + rgb = (rgb - blacklevel) / (1.0 - blacklevel); /* * CCM is a 3x3 in the format diff --git a/src/libcamera/shaders/bayer_unpacked.frag b/src/libcamera/shaders/bayer_unpacked.frag index 1b85196ae..98dea512c 100644 --- a/src/libcamera/shaders/bayer_unpacked.frag +++ b/src/libcamera/shaders/bayer_unpacked.frag @@ -128,7 +128,7 @@ void main(void) { vec3(PATTERN.w, C, PATTERN.z) : vec3(PATTERN.yx, C)); - rgb = rgb - blacklevel; + rgb = (rgb - blacklevel) / (1.0 - blacklevel); /* * CCM is a 3x3 in the format
In GPU ISP fragment shaders, the black level is simply subtracted from the pixel value. This means the highest pixel values can never be reached, possibly resulting in wrong brightness or colour shifts. Fix this by spreading the resulting value to the whole 0.0..1.0 range. The preceding simple pipeline IPA patch ensures `blacklevel' is less than 1.0, preventing division by zero here. Signed-off-by: Milan Zamazal <mzamazal@redhat.com> --- src/libcamera/shaders/bayer_1x_packed.frag | 2 +- src/libcamera/shaders/bayer_unpacked.frag | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)