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

filmicrgb: fix norm handling in v6 #11480

Merged
merged 1 commit into from
Apr 9, 2022

Conversation

flannelhead
Copy link
Contributor

We need to clamp the RGB norm before calculating ratios. Otherwise things like this can happen:

  1. Consider the max RGB norm and input pixels (1, 1, 1) and (1, 1, 2). Filmic white is set such that the source white is at (1, 1, 1).
  2. Norms are calculated and they are 1 and 2, respectively.
  3. Ratios are calculated: (1, 1, 1), (0.5, 0.5, 1).
  4. When the filmic tone curve is applied to the norm, it is also clamped to the [0, 1] range at some point. Now both norms have value 1.
  5. Ratios are restored; now the output pixels read (1, 1, 1) and (0.5, 0.5, 1). This is bad as the original blue pixel was brighter than the white!
  6. Gamut mapping will preserve the color of the blue pixel and it results in a nasty-looking dark patch in the middle of a bright surrounding (think of e.g. lights where the center is clipped in raw).

To remedy this, we already clamp the norm before calculating the ratios. This way when the ratios are restored, they will retain their brightness ordering. Gamut mapping takes care of finally clamping the brightest highlights back into the target range. I don't know if dumb clamping like this is the best solution, but the filmic curve is not really well defined outside the designed dynamic range i.e. it doesn't extend above the source white point value. Correct me if I'm wrong, please. From brief testing I find the white point adjustment in filmic behaving now much more like I would expect it to - the brightest parts will eventually turn white when bringing down the white point slider. Without this PR, this is not always the case.

This behaviour exists also with the earlier filmic v5 revision but probably the mandatory desaturation has masked this issue.

@flannelhead
Copy link
Contributor Author

This should fix issues like the one reported in #10976 (comment)

@elstoc elstoc requested a review from aurelienpierre April 8, 2022 20:14
@aurelienpierre
Copy link
Member

Correct me if I'm wrong, please.

You are not.

Good job !

@flannelhead
Copy link
Contributor Author

I think this is correct as-is but something could still be done to make the transitions a bit smoother. I'm thinking about it.

@TurboGit TurboGit added this to the 4.0 milestone Apr 9, 2022
@TurboGit TurboGit added bugfix pull request fixing a bug priority: high core features are broken and not usable at all, software crashes labels Apr 9, 2022
Copy link
Member

@TurboGit TurboGit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thanks!

@TurboGit TurboGit merged commit 2f64ed7 into darktable-org:master Apr 9, 2022
@piratenpanda
Copy link
Contributor

did not change the behaviour as described in #10976 (comment)

@flannelhead
Copy link
Contributor Author

@piratenpanda thanks for the report. Do you happen to have a raw file + XMP you could share where this behaviour is seen?

@piratenpanda
Copy link
Contributor

Ok might as well be user error. If I lower the white value I can recreate the v5 look now. Sorry for the noise. That's most likely expected behaviour that the white points are different for v5 and v6.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix pull request fixing a bug priority: high core features are broken and not usable at all, software crashes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants