| Message ID | 20260211131728.96413-1-mzamazal@redhat.com |
|---|---|
| Headers | show |
| Series |
|
| Related | show |
Hi, I wonder whether this possibly fixes the pink cast Kieran gets in bright areas. If not, there may be other issues, not necessarily libcamera bugs (I find this video quite explanatory regarding colours in bright areas: https://www.youtube.com/watch?v=ZFGxdb2pH8g). As for the place where black level is applied in GPU ISP, I think we are OK in the packed shader as it just computes averages. The order doesn't matter in such a case: (A * (x - black) + B * (y - black)) / (A + B) = (A * x + B * y - (A + B) * black) / (A + B) = (A * x + B * y) / (A + B) - (A + B) * black / (A + B) = (A * x + B * y) / (A + B) - black But note that if the divisor were C rather than A + B, the equality would no longer hold. I'm afraid the order may matter in the unpacked shader as it probably mixes different colour channels. I must look into it deeper, perhaps with the help of the corresponding paper. The proposed lens shading correction patches perform their computations after black level subtraction. Milan Zamazal <mzamazal@redhat.com> writes: > There is an omission when applying black level in GPU ISP: the black > level is subtracted but the resulting pixel values are not spread back > to the whole 0..1 range. This prevents reaching maximum brightness with > non-zero black level and can also lead to pinkish bright areas or other > colour shifts. > > Milan Zamazal (2): > ipa: simple: Limit the black level value > libcamera: software_isp: Fix black level application in GPU ISP > > src/ipa/simple/algorithms/blc.cpp | 7 ++++--- > src/libcamera/shaders/bayer_1x_packed.frag | 2 +- > src/libcamera/shaders/bayer_unpacked.frag | 2 +- > 3 files changed, 6 insertions(+), 5 deletions(-)
On środa, 11 lutego 2026 14:55:17 czas środkowoeuropejski standardowy Milan Zamazal wrote: > Hi, > > I wonder whether this possibly fixes the pink cast Kieran gets in bright > areas. If not, there may be other issues, not necessarily libcamera > bugs (I find this video quite explanatory regarding colours in bright > areas: https://www.youtube.com/watch?v=ZFGxdb2pH8g). I don't see any code for handling clipped highlights in GPU ISP's shaders, so having pink highlights would be an expected result of applying white balance, wouldn't it? (or is it hiding somewhere else?) Any pixels with clipped values have to be recolored heuristically to get rid of magenta casts. In darktable, that would match to its "highlight reconstruction" module; in dcraw, that's the -H option. As far as I can see, this video talks about much more subtle issues that are being solved with various tone mapping curves (where libcamera applies a basic S-curve right now) that will only start to matter once the basics are done right. When I played with writing an ISP for video recording (https:// source.puri.sm/-/snippets/1223) I used a very basic and not entirely correct yet effective way to deal with highlights: vec3 clip_highlights(vec3 color) { vec3 ratio = color / whitepoint; return whitepoint * mix(ratio, vec3(1.0), smoothstep(0.96, 1.0, ratio)); } where "whitepoint" is vec3(1.0) multiplied by the inverse of the white balanced color correction matrix (this happens after subtracting black level, but before lens shading correction and CCM application). But that's mostly because I haven't researched how proper highlight reconstruction methods actually work:) > As for the place where black level is applied in GPU ISP, I think we are > OK in the packed shader as it just computes averages. The order doesn't > matter in such a case: > > (A * (x - black) + B * (y - black)) / (A + B) = > (A * x + B * y - (A + B) * black) / (A + B) = > (A * x + B * y) / (A + B) - (A + B) * black / (A + B) = > (A * x + B * y) / (A + B) - black > > But note that if the divisor were C rather than A + B, the equality > would no longer hold. I'm afraid the order may matter in the unpacked > shader as it probably mixes different colour channels. I must look into > it deeper, perhaps with the help of the corresponding paper. > > The proposed lens shading correction patches perform their computations > after black level subtraction. > > Milan Zamazal <mzamazal@redhat.com> writes: > > There is an omission when applying black level in GPU ISP: the black > > level is subtracted but the resulting pixel values are not spread back > > to the whole 0..1 range. This prevents reaching maximum brightness with > > non-zero black level and can also lead to pinkish bright areas or other > > colour shifts. > > > > Milan Zamazal (2): > > ipa: simple: Limit the black level value > > libcamera: software_isp: Fix black level application in GPU ISP > > > > src/ipa/simple/algorithms/blc.cpp | 7 ++++--- > > src/libcamera/shaders/bayer_1x_packed.frag | 2 +- > > src/libcamera/shaders/bayer_unpacked.frag | 2 +- > > 3 files changed, 6 insertions(+), 5 deletions(-)
Hi Sebastian, Sebastian Krzyszkowiak <sebastian.krzyszkowiak@puri.sm> writes: > On środa, 11 lutego 2026 14:55:17 czas środkowoeuropejski standardowy Milan > Zamazal wrote: >> Hi, >> > >> I wonder whether this possibly fixes the pink cast Kieran gets in bright >> areas. If not, there may be other issues, not necessarily libcamera >> bugs (I find this video quite explanatory regarding colours in bright >> areas: https://www.youtube.com/watch?v=ZFGxdb2pH8g). > > I don't see any code for handling clipped highlights in GPU ISP's > shaders, Right. > so having pink highlights would be an expected result of applying > white balance, wouldn't it? (or is it hiding somewhere else?) Any > pixels with clipped values have to be recolored heuristically to get > rid of magenta casts. I guess so. But while the casts may possibly occur in many cases, I don't get them too often with, well, my uncalibrated sensor; others may have a similar experience. At this stage when other problems can be present, we must be somewhat careful not to blame everything on clipping. > In darktable, that would match to its "highlight reconstruction" module; in > dcraw, that's the -H option. As far as I can see, this video talks about much > more subtle issues that are being solved with various tone mapping curves > (where libcamera applies a basic S-curve right now) that will only start to > matter once the basics are done right. > > When I played with writing an ISP for video recording (https:// > source.puri.sm/-/snippets/1223) I used a very basic and not entirely correct > yet effective way to deal with highlights: > > vec3 clip_highlights(vec3 color) { > vec3 ratio = color / whitepoint; > return whitepoint * mix(ratio, vec3(1.0), smoothstep(0.96, 1.0, ratio)); > } > > where "whitepoint" is vec3(1.0) multiplied by the inverse of the white > balanced color correction matrix (this happens after subtracting black level, > but before lens shading correction and CCM application). But that's mostly > because I haven't researched how proper highlight reconstruction methods > actually work:) With GPU ISP merged, there are certainly opportunities to submit patches for improvements. :-) >> As for the place where black level is applied in GPU ISP, I think we are >> OK in the packed shader as it just computes averages. The order doesn't >> matter in such a case: >> >> (A * (x - black) + B * (y - black)) / (A + B) = >> (A * x + B * y - (A + B) * black) / (A + B) = >> (A * x + B * y) / (A + B) - (A + B) * black / (A + B) = >> (A * x + B * y) / (A + B) - black >> >> But note that if the divisor were C rather than A + B, the equality >> would no longer hold. I'm afraid the order may matter in the unpacked >> shader as it probably mixes different colour channels. I must look into >> it deeper, perhaps with the help of the corresponding paper. >> >> The proposed lens shading correction patches perform their computations >> after black level subtraction. >> >> Milan Zamazal <mzamazal@redhat.com> writes: >> > There is an omission when applying black level in GPU ISP: the black >> > level is subtracted but the resulting pixel values are not spread back >> > to the whole 0..1 range. This prevents reaching maximum brightness with >> > non-zero black level and can also lead to pinkish bright areas or other >> > colour shifts. >> > >> > Milan Zamazal (2): >> > ipa: simple: Limit the black level value >> > libcamera: software_isp: Fix black level application in GPU ISP >> > >> > src/ipa/simple/algorithms/blc.cpp | 7 ++++--- >> > src/libcamera/shaders/bayer_1x_packed.frag | 2 +- >> > src/libcamera/shaders/bayer_unpacked.frag | 2 +- >> > 3 files changed, 6 insertions(+), 5 deletions(-)
On Wed, Feb 11, 2026 at 05:45:15PM +0100, Milan Zamazal wrote: > Sebastian Krzyszkowiak writes: > > On środa, 11 lutego 2026 14:55:17 czas środkowoeuropejski standardowy Milan Zamazal wrote: > >> Hi, > >> > >> I wonder whether this possibly fixes the pink cast Kieran gets in bright > >> areas. If not, there may be other issues, not necessarily libcamera > >> bugs (I find this video quite explanatory regarding colours in bright > >> areas: https://www.youtube.com/watch?v=ZFGxdb2pH8g). > > > > I don't see any code for handling clipped highlights in GPU ISP's > > shaders, > > Right. > > > so having pink highlights would be an expected result of applying > > white balance, wouldn't it? (or is it hiding somewhere else?) Any > > pixels with clipped values have to be recolored heuristically to get > > rid of magenta casts. > > I guess so. But while the casts may possibly occur in many cases, I > don't get them too often with, well, my uncalibrated sensor; others may > have a similar experience. That's normal, as the colour casts are caused by applying colour gains on saturated pixel values. > At this stage when other problems can be > present, we must be somewhat careful not to blame everything on > clipping. When pixels are saturated, information about their colour is lost. There's no way around that. As Sebastian mentioned, the best we can do is to try to apply heuristics to guess what the colour was, and hope the result will be visually pleasing. Performing histogram equialization at the BLC stage may produce more pleasing results in some cases, but it's a hack and not a proper solution. I'd rather not go in that direction, and instead work on handling the issue correctly. > > In darktable, that would match to its "highlight reconstruction" module; in > > dcraw, that's the -H option. As far as I can see, this video talks about much > > more subtle issues that are being solved with various tone mapping curves > > (where libcamera applies a basic S-curve right now) that will only start to > > matter once the basics are done right. > > > > When I played with writing an ISP for video recording (https:// > > source.puri.sm/-/snippets/1223) I used a very basic and not entirely correct > > yet effective way to deal with highlights: > > > > vec3 clip_highlights(vec3 color) { > > vec3 ratio = color / whitepoint; > > return whitepoint * mix(ratio, vec3(1.0), smoothstep(0.96, 1.0, ratio)); > > } > > > > where "whitepoint" is vec3(1.0) multiplied by the inverse of the white > > balanced color correction matrix (this happens after subtracting black level, > > but before lens shading correction and CCM application). But that's mostly > > because I haven't researched how proper highlight reconstruction methods > > actually work:) > > With GPU ISP merged, there are certainly opportunities to submit patches > for improvements. :-) > > >> As for the place where black level is applied in GPU ISP, I think we are > >> OK in the packed shader as it just computes averages. The order doesn't > >> matter in such a case: > >> > >> (A * (x - black) + B * (y - black)) / (A + B) = > >> (A * x + B * y - (A + B) * black) / (A + B) = > >> (A * x + B * y) / (A + B) - (A + B) * black / (A + B) = > >> (A * x + B * y) / (A + B) - black > >> > >> But note that if the divisor were C rather than A + B, the equality > >> would no longer hold. I'm afraid the order may matter in the unpacked > >> shader as it probably mixes different colour channels. I must look into > >> it deeper, perhaps with the help of the corresponding paper. > >> > >> The proposed lens shading correction patches perform their computations > >> after black level subtraction. > >> > >> Milan Zamazal <mzamazal@redhat.com> writes: > >> > There is an omission when applying black level in GPU ISP: the black > >> > level is subtracted but the resulting pixel values are not spread back > >> > to the whole 0..1 range. This prevents reaching maximum brightness with > >> > non-zero black level and can also lead to pinkish bright areas or other > >> > colour shifts. > >> > > >> > Milan Zamazal (2): > >> > ipa: simple: Limit the black level value > >> > libcamera: software_isp: Fix black level application in GPU ISP > >> > > >> > src/ipa/simple/algorithms/blc.cpp | 7 ++++--- > >> > src/libcamera/shaders/bayer_1x_packed.frag | 2 +- > >> > src/libcamera/shaders/bayer_unpacked.frag | 2 +- > >> > 3 files changed, 6 insertions(+), 5 deletions(-)