Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Disabled clip negative GRG from gamut makes image black #9315

Closed
BigSerpent opened this issue Jun 22, 2021 · 14 comments · Fixed by #9322
Closed

Disabled clip negative GRG from gamut makes image black #9315

BigSerpent opened this issue Jun 22, 2021 · 14 comments · Fixed by #9322
Labels
understood: unclear devs lack most or all important info and can do nothing, the report will be closed after 2 weeks

Comments

@BigSerpent
Copy link

In the color calibration module disabled "clip negative GRG from gamut" option makes an image black.
Git master at the commit 4965e8f

@johnny-bit
Copy link
Member

johnny-bit commented Jun 23, 2021

Cannot reproduce with ANY of my test files, opencl or not on nVidia 1060 on linux.

Can you specify more details according to issue template?

EDIT: Actually my opencl crashed, so i can't check opencl.

@johnny-bit johnny-bit added the understood: unclear devs lack most or all important info and can do nothing, the report will be closed after 2 weeks label Jun 23, 2021
@BigSerpent
Copy link
Author

Sorry for short description. I used Filmic RGB and color calibration modules as usual at importing. The image was OK. When I tried to make one image black and white with color calibration B/W built-in preset, the image preview window in the darkroom became black. So was the thumbnail in the lightroom mode. I haven't tried to export the image.
Some experiments have shown the image becomes black if the clip negative GRG from gamut checkbox is disabled regardless of other settings of the color calibration module.
I've deleted the build directory before compilation. I haven't cleaned program directory manually before the installation. Can try it later today.

@MStraeten
Copy link
Collaborator

i checked an older build (10.6.) there's the issue is not reproducible. A build at 18.6. shows this issue.
so i need to bisect to get the commit that introduced the issue

@aurelienpierre
Copy link
Member

In the color calibration module disabled "clip negative GRG from gamut" option makes an image black.

Yeah. That's why this option exists and is enabled by default. You disable it, you deal with the issues you create. Not a bug.

@MStraeten
Copy link
Collaborator

if it works fine with an older build after feature freeze and not with a later build after feature freeze the something changed in between ...

@johnny-bit
Copy link
Member

the only change to colorbalancergb between featurefreeze and now was #9257 (afaik)

@MStraeten
Copy link
Collaborator

bisect result:

commit ff3eb7e60623935f9ba068a50528607e24cccd37
Author: Aurélien PIERRE <aurelien@aurelienpierre.com>
Date:   Wed Jun 16 13:36:04 2021 +0200

    color calibration : disable Yxy <-> XYZ conversion if no gamut mapping is required

    fix #9076

 data/kernels/channelmixer.cl | 15 ++++++++++-----
 src/iop/channelmixerrgb.c    | 25 ++++++++++++++++---------
 2 files changed, 26 insertions(+), 14 deletions(-)

@BigSerpent
Copy link
Author

I use one of the default presets I used to and get incorrect result. Is it correct behavior?

@MStraeten
Copy link
Collaborator

yes, because the presets don't check "clip negative RGB from gamut"
just check it and it will be ok

@BigSerpent
Copy link
Author

It is a workaround, but not a normal workflow :) . i guess if an option is presented and not disabled for an user, it can be used by one.

@aurelienpierre
Copy link
Member

It is not a workaround. Negative RGB are not supposed to happen, there is no negative light nor negative energy. They will trigger invalid computations later. So we either clip them or let the software handle them as NaN, or possibly let users deal with them the way they want.

By the way, I tested Intel OpenCL and don't reproduce black images. Worse case scenario is invalid pixels.

@BigSerpent
Copy link
Author

OK, let's speak from a user's POV. There are two scenarios:

  1. One uses a build-in BW film preset and gets a black image. Is he wrong in his actions? Should he (a regular photographer, for instance) know there is an illegal option in the preset he should correct manually?
  2. Just for tuning a photo one unchecks the checkbox and gets a black image. What for is the checkbox at all if it cannot be unchecked without getting wrong results? What is the aim of the ability of a user to turn it on and off?

@aurelienpierre
Copy link
Member

  1. The built-in presets have been corrected by @johnny-bit yesterday,
  2. The problem is the presence of negative RGB depends on the original image AND the input profile AND the white balance setting AND the chromatic adaptation settings. They won't happen all the time. When they happen, clipping them is destructive and not the more accurate option (it may change hue), although it is a safe one (since we don't trust OpenCL drivers to do the right thing).

Usually, you rather want to increase the gamut compression and disable the black clipping to remap out-of-gamut colors at constant hue and luminance (non-destructive). But then, you need to understand that as long as you fail to find the chroma compression that brings all the gamut back into the valid range, the image will be entirely garbled at no fault of the software. So disabling the negative clipping should only be used by people who understand what color spaces are and how we deal with them.

But then, again, I have never seen a fully black image here, on CPU or Intel eGPU or Nvidia dGPU, so I bet a faulty driver is to blame for black images.

@johnny-bit
Copy link
Member

  1. Yeah, built-in presets should "just work"
  2. there are usecases when you clearly know what you're doing and likely won't produce black img output. So it's sorta "i know it looks like i'm aming bazooka at my foot, but trust me i'm doing rocket jump actually" :P

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
understood: unclear devs lack most or all important info and can do nothing, the report will be closed after 2 weeks
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants