Tone equalizer 2025-04-06 preview version #18656
Conversation
|
The detailed explanation is here now: https://discuss.pixls.us/t/tone-equalizer-proposal/49314 |
|
macos build fails |
|
Does this preserve existing edits? Just imported some existing tone equalizer presets. Applying them has no effect, therefore I would believe this version isn't backward compatible and will break existing edits. |
src/iop/toneequal.c
Outdated
| @@ -126,35 +128,60 @@ | |||
|
|
|||
| DT_MODULE_INTROSPECTION(2, dt_iop_toneequalizer_params_t) | |||
There was a problem hiding this comment.
You need to update the version here to support old edits - legacy_params() only gets called if the stored version is less than the version given here (and new edits would get confused with old ones without the version bump).
Looks like a missing version bump - the code is present to convert old edits, but darktable doesn't call it since it thinks they're still the current version. |
src/iop/toneequal.c
Outdated
| @@ -73,7 +73,7 @@ | |||
| * | |||
| * Users should be aware that not all the available octaves will be | |||
| * useful on every pictures. Some autom 8000 atic options will help them to | |||
| * optimize the luminance mask, performing histogram analyse, mapping | |||
| * optimize the luminance mask, performing histogram analys, mapping | |||
src/iop/toneequal.c
Outdated
| n->quantization = 0.0f; | ||
| n->smoothing = sqrtf(2.0f); | ||
|
|
||
| // V3 params |
There was a problem hiding this comment.
Please keep this as new_version = 2. The legacy_params() will be called multiple time until we reach the last version. The rule here is that we never have to touch old migration code, we just add a new chunk to go 1 step to final version.
src/iop/toneequal.c
Outdated
|
|
||
| const dt_iop_toneequalizer_params_v2_t *o = old_params; | ||
| dt_iop_toneequalizer_params_v3_t *n = malloc(sizeof(dt_iop_toneequalizer_params_v3_t)); | ||
|
|
There was a problem hiding this comment.
Since all your new fields are at the end of the struct, just do:
memcpy(n, o, sizeof(t_iop_toneequalizer_params_v2_t));
| n->quantization = o->quantization; | ||
| n->smoothing = o->smoothing; | ||
|
|
||
| // V3 params |
There was a problem hiding this comment.
And keep only this section below:
|
Thanks for your feedback. I will provide a new version in a few days which includes the changes that were suggested here. Some advise on my two major roadblocks would be helpful:
|
|
@TurboGit please note there has been a lot of discussion in the thread on pixls. Please take that into consideration. |
|
I am late to the party and I will also keep silent again for private reasons IRL. From my perspective, I do not have many issues with ToneEQ. There is only one thing bothered me from day-one of this module:
I pretty much dislike and had long argues with the author Aurtélien Pierre by its time. He kept trying to convince me that it is not doable differently for this and that reason (as he usually behaves). That bar-indicator on masking tab did not guide me as precise as he always pretended it would be. Nowadays I have mapped those two sliders to two rotaries on my midi-board (X-touch mini) and I can attenuate while looking at the advanced tab. The luminance estimator / preserve details is another thing slightly above my head but from time to time I use it. All the rest, I barely touch After setting the mask, I hover the mouse on my image and scroll the wheel (for this I sometimes need to off/on the module as the rotaries seem to mess with the module-focus). For me the “simple” tab can just be hidden totally. Besides, please do not clutter the advanced tab. I hope my workflow (above) will not be destroyed and nor the old edits. In other words: Never change a running system |
|
@paperdigits : Yes I've seen and followed a bit the discussion, but it became so heated that it has almost no value to me at this point so I don't follow it on pixls anymore. We'll see if part of it is moved here in a PR. |
|
Existing user defined presets no longer work. As for the included presets, whenever I apply one the histogram contracts to take half of the horizontal space and moves to the left edge. If I move the histogram back where I want it and apply a preset again the same problem occurs. |
|
Showing the mask with preserve details shows no difference between Masks are vastly different between tone equalizer on current master and this PR. New masks...
versus current master
|
Ad 1) Nope, you must not use the parameters for keeping data while processing the module. We have the global module data, you might use that to keep data shared by all instances of a module. But you'd have to implement some locking and "per instance" on your own. Unfortunately we currentlx don't have "global_data_per_instance" available. Ad 2) Yes, exports and the cli interface. |
|
@wpferguson Are you sure that the settings were the same? The version in this PR had the bug with the version number introspection, so it did not convert existing settings correctly. It is probably better to wait for a new version to test this. |
So it probably does not make sense at all to depend on values that come from the GUI during export. The core problem is the scale-dependence of the guided filter. An alternative approach for exporting would be to create a downscaled copy of the image (sized like the GUI preview), apply the GF, get the values I need and then apply them to the the full image - essentially simulating the preview calculation. Not the most efficient approach, but it would only be needed during export when auto_align is used. |
|
Not sure I understood the last post correctly, but if I did, in my opinion there would be a problem. Firstly, for the CLI case, where there is no GUI, how can the GF computation be done on a downscaled image sized like the GUI preview? But also if there is a GUI, I think the export result must not depend on the arbitrary size of the darktable window (and thus the preview size) at the time the export is done. (Later exports of the image with unchanged history stack and same export settings must always yield identical results.) |
Definitely not, it won't generally work.
I didn't check /review your code in it's current state but are you sure you did setup correctly? There are some issues but we use feathering guide all over using masks and results are pretty stable to me. About keeping data/results in per-module-instance, this has daunting me too on some other project. Will propose a solution pretty soon that might help you ... |
As far as I understand it, the preview thread calculates the navigation image shown in the top left of the GUI. However the actual image that the thread sees is much bigger (something like 1000px wide), so the navigation image must be a downscaled version. I hope (but have not yet checked) that the size of this internal image is constant. |
The code in the PR is outdated and has known problems, not much use looking at it now. I will update it as soon as I have a somewhat consistent state.
Of course it is possible that I might have broken something, but as far as I am aware, I did not change anything about the guided filter calculation but only modified what happens with the results.
This sounds nice, but my next attempt will be trying to avoid this. |
|
Note that your code can figure out how much the image it has been given has been downscaled in order to determine scaled radii and the like to simulate appropriate results. A bunch of modules with scale-dependent algorithms do this. It isn't perfect, but does yield pretty stable and predictable results. Look for |
|
I have been playing around with the tone equalizer some more and ran into a bit of a roadblock. To recap, what I do is: Image => Grayscale + Guided filter => Mask => Mask Histogram => Percentile values => Auto Scale/Shift values The buffers in the different pipelines (PREVIEW with GUI and the export) have different sizes and the guided filter is scale-dependent, so when I make statistics over the mask, it is expected that there is a systematic error. The final calculated values deviate so much that there is a visible difference between the image that is shown in DT and the export. My idea to overcome this was as to downscale the image during export to the same size as the preview and use the downscaled version to calculate the statistics (essentially simulating the preview pipe during export). However the results were still different even though the calculation was done with images of the same size and with the exact same parameters. Then I added debugging code to write the internal buffers into files and I discovered the reason:
However the left image (PREVIEW) is still a lot blurrier than my downscaled version. I would have expected them to be mostly the same. Using a blurry version of the image is not ideal for my purposes. However the bigger problem is that I need to know, what happened to the image in the preview pipe, if I want to replicate the same steps in the export pipe. I tried to find relevant parts in the DT code, but had no success so far. I am also interested in the calculation of the size of the PREVIEW pipe image, it seems like the height fluctuates the least and is between 880 and 900 pixels. |
|
The demosaic module downscales if the requested output dimensions to the following module are small enough. FULL is almost certainly getting more data out of demosaic than PREVIEW, which reflects in the sharper image after downscaling. Run with |
7c707c2 to
64fc0de
Compare
|
I finally have a version that I consider good enough to show it publicly. It would be nice if someone would take the time to look at my code, i.e. there are a few "TODO MF" markers with open questions. The module should be (if there are no bugs) compatible with old edits. I have checked that with parameters "align: custom, scale: 0, shift: 0" the results are the same as in 5.0.1. Most of my changes were not that much effort. Scaling the curve after the guided filter, coloring the curve, changing the UI (even though it still has a few issues) was not that difficult. The one thing that was hard and got me stuck for weeks is the auto alignment feature:
The data that is available to the pixelpipes during GUI operations is different from the export. The FULL pipe may not see the whole image, so it is not suitable to calculate mask exposure/contrast. The PREVIEW pipe sees the image completely, but it is scaled-down and pretty blurry. The guided filter is sensitive to both of these effects, so statistics on the PREVIEW luminance mask deviate significantly from statistics of the whole image. The (unfortunately not so nice) solution this problem is to request the full image in PIXELPIPE_FULL when necessary. Of course this has an impact on performance. However in practice I found it acceptable and it only occurs when the user explicitly requests auto-alignment of the mask - so users who use the module like before should not experience performance degradation (unless I accidentally broke something, i.e. OpenMP). Known issues:
Other notes: I have seen #18722 and to be honest I am not sure what it does exactly, but I wondered if it would be possible to give modules like tone equalizer access to an "early stable" version of the image - a version from after demosaic, but before any modules that users typically use to change the image. If this was possible, I would switch the calculating the luminance mask to this input, if the user requests auto-alignment, so re-calculating the mask would not be necessary that often. Providing modules with access to an early version of the image would also have other use-cases, like stable parametric masks that don't depend on the input of their respective module. |
And only once when the auto-align is changed, right?
Indeed, you can even resize to negative size of the graph :)
Indeed, we need back the hierarchical presets naming. Should be easy.
A question for @jenshannoschwalm I suppose. During my testing I found the auto-align a very big improvement indeed. I would probably have used the fully-fit as default or maybe the "auto-align at mid-tone". BTW, since the combo label is "auto align mask exposure" I would use simplified naming for the entries:
|
There was a problem hiding this comment.
Some changes proposed while doing a first review.
To me the new ToneEQ UI is nice and better than what we have in current master.
I would suggest to address the remaining minor issues (crash when resizing the graph) + the proposed changes here and do a clean-up of the code to remove dead-code and/or commented out code. Probably also removing the debug code and I'll do a second review.
From there we should also have some other devs testing it, such drastic changes may raise some issues with others. Again to me that's a change in the right direction.
src/iop/toneequal.c
Outdated
| const char *name() | ||
| { | ||
| return _("tone equalizer"); | ||
| } | ||
|
|
||
|
|
There was a problem hiding this comment.
we do not require 2 empty lines between routines, not a convention we have.
src/iop/toneequal.c
Outdated
| int iterations; // $MIN: 1 $MAX: 20 $DEFAULT: 1 $DESCRIPTION: "filter diffusion" | ||
| float post_scale; // $MIN: -3.0 $MAX: 3.0 $DEFAULT: 0.0 $DESCRIPTION: "mask contrast / scale histogram" | ||
| float post_shift; // $MIN: -4.0 $MAX: 4.0 $DEFAULT: 0.0 $DESCRIPTION: "mask brightness / shift histogram" | ||
| dt_iop_toneequalizer_post_auto_align_t post_auto_align; // $DEFAULT: DT_TONEEQ_ALIGN_CUSTOM $DESCRIPTION: "auto align mask exposure" |
There was a problem hiding this comment.
As proposed I would default here to DT_TONEEQ_ALIGN_FIT.
src/iop/toneequal.c
Outdated
| typedef enum dt_iop_toneequalizer_post_auto_align_t | ||
| { | ||
| DT_TONEEQ_ALIGN_CUSTOM = 0, // $DESCRIPTION: "custom" | ||
| DT_TONEEQ_ALIGN_LEFT, // $DESCRIPTION: "auto-align at shadows" |
There was a problem hiding this comment.
As proposed, remove "auto-align " prefix.
| n->quantization = 0.0f; | ||
| n->smoothing = sqrtf(2.0f); | ||
|
|
||
| *new_params = n; | ||
| *new_params_size = sizeof(dt_iop_toneequalizer_params_v2_t); | ||
| *new_version = 2; | ||
| *new_params_size = sizeof(dt_iop_toneequalizer_params_v3_t); |
There was a problem hiding this comment.
Looks like this is wrong, we do a one step update from v1 to v2. As said previously we don't want to change older migration path.
src/iop/toneequal.c
Outdated
| @@ -495,14 +690,14 @@ void init_presets(dt_iop_module_so_t *self) | |||
| p.exposure_boost = 0.0f; | |||
| p.contrast_boost = 0.0f; | |||
| dt_gui_presets_add_generic | |||
| (_("mask blending | all purposes"), self->op, | |||
| (_("mask blending: all purposes"), self->op, | |||
There was a problem hiding this comment.
To keep the hierarchical preset name you need to remove all the change from dt_gui_presets_add_generic 1st parameter. Here and below (multiple occurrences).
src/iop/toneequal.c
Outdated
|
|
||
| // Luminance mask, but no image histogram | ||
| compute_luminance_mask(in_cropped, luminance, roi_out->width, roi_out->height, d, | ||
| FALSE, NULL, NULL, NULL, |
src/iop/toneequal.c
Outdated
| for(size_t k = 0; k < HIRES_HISTO_SAMPLES; ++k) | ||
| { | ||
| // from [0...HIRES_HISTO_SAMPLES] to [-16...8EV] | ||
| float EV = temp_ev_range * (float)k / (float)(HIRES_HISTO_SAMPLES - 1) + HIRES_HISTO_MIN_EV; |
src/iop/toneequal.c
Outdated
| dt_bauhaus_slider_set(g->whites, p->whites); | ||
| dt_bauhaus_slider_set(g->speculars, p->speculars); | ||
| --darktable.gui->reset; | ||
| switch(p->post_auto_align) { |
src/iop/toneequal.c
Outdated
|
|
||
| // TODO MF: Remove this again? Two histograms are only useful for debugging. | ||
| static void show_two_histograms_callback(GtkWidget *togglebutton, | ||
| GdkEventButton *event, |
src/iop/toneequal.c
Outdated
| { | ||
| // draw the inset histogram | ||
| float histo_height; |
There was a problem hiding this comment.
const float histo_height = g->graph_height * (g->two_histograms_display ? 0.5 : 1.0);
This is indeed all very tricky pipeline related stuff. Some comments We always have to remember the restricted fullpipe roi available data.
BTW - nice to see you "keep going" on this :-) |
|
Thanks for the nice feedback, I will have a look at everything. Since I have limited time for coding, this may again take a little time. |
|
the resizing of the graph doesn't work properly on my macos build. Hovering the mouse over the graph causes arbitrary resizing until it's height is 1 (according to darktablerc) sometimes resulting in a crash ... |
|
toggle for image historgram doesn't match to the graph display: |
…d curve coloring.
…sistency changes.
- Removed unused code - Changed descriptions for align left/right/center - Made align "fully fit" the default - Added underscores to the names of static functions - Changed newlines after functions - Removed some debug code and wrapped the rest in ifdefs - Added hierarchical presets - Added extra presets that use "fully fit" and a more linear curve (Compress Shadows/Highlights => v3 EIGF) Graph: - Graph resizing is now limited and no longer crashes Darktable :-) - The handle displays correctly - TODO: Dragging the graph still shows strange "scrolling up" behavior. - TODO: There must still be some other scrolling-related bug. I once had the points in the graph move in an unexpected way when dragging inside the graph, but was not able to reproduce this. Suggestion (not changed in code): - Adding parentheses to expression in gtk.c line 4017 would make the code easier to read. The line breaks suggest that the ternary operator only refers to _resize_wrap_dragging. Special cases to be tested: - When exporting small/scaled-down versions of the image with auto shift/scale, is the result still sufficiently similar to the GUI image? In my test, the shift/scale values in the export pipeline were the same to a scale factor of 50% and deviated for smaller images.
291eb27 to
dfc3df6
Compare
|
I have pushed an update, even tough not all known problems are resolved. Things take time on my end. @MStraeten: Graph resizing is a know issue. I have improvements in the new version, but it is not completely fixed. I have never worked with GTK and the Darktable UI is still harder than pure GTK, so I have a hard time figuring out the reasons for strange behaviours. Regarding the image histogram: This is a debug option, which I plan to remove before release. The differences you point at are basically the reason why I made it in the first place - to see how the image histogram differs from the mask histogram. Commit messageChanged the points suggested by @TurboGit including:
Graph:
Suggestion (not changed in code):
Special cases to be tested:
|
This is a preview of my proposed changes to the tone equalizer module. There is a lot of debug code still in it and there are known problems, see below.
This message only contains code aspects. I will write a detailed post from a user perspective for pixls.us.
Changes
post_scale,post_shiftandpost_auto_alignsettings, which allow adjusting mask exposure and contrast AFTER the guided filter calculation.post_scaleandpost_shiftare derived from this histogram.post_auto_align=custom,post_scale=0,post_shift=0, the results are the same as with the old tone equalizer (I was not able to get a byte-identical export, but in GIMP the difference between my version and 5.0 was fully black in my tests).dt_dev_pixelpipe_piece_hashtodt_dev_hash_plusafter I noticed that the module constantly re-computed the guided filter, even though this was not necessary.compute_lut_correctiontocompute_gui_curve) and sorted the functions. The consequence is that I have touched almost all lines of code, so diffs will not be helpful in tracking my changes.Known issues
Tbh., I had more problems with the anatomy of a darktable module (threads, params, data, gui-data, all the GTK stuff) than with the actual task at hand.
Known problems are:
post_auto_alignis used to set the mask exposure, the values forpost_scaleandpost_shiftare calculated in PREVIEW and used in FULL. However, other pixel pipes (especially export) calculate the mask exposure on their own and may get a different result that leads to a different output.Things I noticed about the module (a.k.a issues already present in 5.0)
Related Discussion
Issue #17287