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

Tone Equalizer curve fixes #17883

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jenshannoschwalm
Copy link
Collaborator

Tone equalizer fixes

  • Fix a check in get_luminance_from_buffer()
  • pseudo_solve() problems are tracked while committing the parameters.
    This means we always have valid reports even if parameters didn't change and
    we re-open the image in darkroom.
  • some refactoring
  • some correct use of gboolean instead of int
  • Use gboolean for 'checks' when calling pseudo_solve()

Fixes #17866

@jenshannoschwalm jenshannoschwalm added bugfix pull request fixing a bug priority: medium core features are degraded in a way that is still mostly usable, software stutters scope: UI user interface and interactions scope: codebase making darktable source code easier to manage labels Nov 23, 2024
@TurboGit
Copy link
Member

I still can reproduce the issue with this PR:

image

image

image

image

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.

Still something wrong...

@jenshannoschwalm
Copy link
Collaborator Author

jenshannoschwalm commented Nov 23, 2024

Added additional reports while in interactive mode, squashed and force-pushed.

@TurboGit i checked again and i can't reproduce a non-report any longer. Please note

  • you might check while using -d params and modify the curve-smoothing slider. The warning only appears if the is a non-solved situation so your third curve would be mathematically ok - although looking quite strange - , your forth curve should be reported.
  • runtime checks should report only if tone equalizer module is active.

Let me know if you still have the issue.
Release note might be: Fixed missing control logs about bad advanced tone equalizer curve

@TurboGit
Copy link
Member

Still reproduce, when the curve go crazy (4th picture above) I get:

    38.2297 Cholesky decomposition returned NaNs
    38.2297 Cholesky LU triangular descent returned NaNs
    38.2297 Cholesky LU triangular ascent returned NaNs

On the 3rd example above, I see nothing reported and my picture gets really wrong so the "crazy" curve is applied.

@jenshannoschwalm
Copy link
Collaborator Author

On the 3rd example above, I see nothing reported and my picture gets really wrong so the "crazy" curve is applied.

Right. But how to solve that? I mean, that's how the solver works it's doesn't care about crazy curves right now, just about non solvable, basically it just checks for "avoid div by zero". (See choleski.h _choleski_decompose_safe, _triangular_descent_safe and _triangular_ascent_safe.

We could probably use some eps instead of comparing to zero?
Do that now and here or in another PR possibly even for 5.2?

@jenshannoschwalm
Copy link
Collaborator Author

@TurboGit i added a version with tests against close-to-zero instead of being zero. Looks really good. As the solver is not used elsewhere it might even be possible to use it for 5.0

@TurboGit
Copy link
Member

Hum... I still have the issue on my side with this latest version. I'll say let's this for 5.2, I don't feel like changing this so close from the release.

@TurboGit TurboGit added this to the 5.2 milestone Nov 23, 2024
@jenshannoschwalm
Copy link
Collaborator Author

Not sure if i misunderstood. Do you have a situation where a) there are logs about bad data but b) no control_log message? If so, could you share the xmp?

@TurboGit
Copy link
Member

With this situation:

image

I have nothing in the debug log on console and no control log.

XMP:
6Q1B7618.CR2.xmp.txt

@jenshannoschwalm
Copy link
Collaborator Author

jenshannoschwalm commented Nov 23, 2024

Edited comment: I see and agree.

So for now only the reporting in color equalizer has been fixed and we sill require a better solution for the solver.

@jenshannoschwalm jenshannoschwalm changed the title Tone Equalizer fixes Tone Equalizer curve fixes Nov 24, 2024
@jenshannoschwalm jenshannoschwalm marked this pull request as draft November 24, 2024 09:24
@jenshannoschwalm jenshannoschwalm force-pushed the toneequal_fixes branch 2 times, most recently from 5b0280f to fed8df7 Compare November 26, 2024 05:25
- pseudo_solve() problems are tracked while committing the parameters.
  This means we always have valid runtime reports even if parameters didn't change and
  we re-open the image in darkroom.
- report in interactive mode as there is no pipe running
- some refactoring
- log report for choleski failing
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: medium core features are degraded in a way that is still mostly usable, software stutters scope: codebase making darktable source code easier to manage scope: UI user interface and interactions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Erratic curve behavior in the tone equalizer when curve smoothing is set to high values.
2 participants