Filmic auto picker #1818

moy · 2018-11-10T20:24:00Z

This turns the "autotune" button into a picker. While doing this I noticed that pickers weren't activated immediately when active, hence this PR changes that too. Also, color pickers are disabled when one of the 3 black/white/grey slider is moved to avoid bad interaction between the picker automatically moving the sliders and manual usage of the sliders. See commit messages for details.

This needs more testing than I did. If this is accepted, the same should be done for "unbreak input profile" and "color balance".

moy · 2018-11-11T08:39:39Z

What code did you put there ? process() functions should not contain any GUI stuff nor parameters settings. process can be called from darktable-cli with no graphic environnement, and it's not good to set parameters in there. If you need to compute intermediate parameters, that should be done in commit_params().

I'm not 100% satisfied with these calls in process() functions, but the part accessing the GUI is protected by if (g && ...) so it's tolerant to having no GUI.

"Just before starting the processing" is really when you want the picker-related code to be executed, otherwise you need to run the pixelpipe twice (once to get the color, and once to apply the autotune). I was hoping for a picker-specific callback right before process() (which would be the right solution IMHO), but didn't find one.

TurboGit · 2018-11-11T09:03:53Z

I think this is the first time I see GUI code in process(). This must be avoided to me it is wrong to do that.

TurboGit · 2018-11-11T09:05:06Z

In exposure() what is done is use a callback on the "draw" signal for the widget to get the picked color if I'm reading the code correctly. Can't this be done here?

TurboGit · 2018-11-11T10:43:46Z

@moy : see #1819 I still have an icon status issue, minor but annoying, but at least nothing done in process(). I'm not sure to be able to fix that today. If you have an idea?

moy · 2018-11-11T12:24:01Z

#1819 is essentially my first commit here. It doesn't introduce the calls in process(), but it doesn't solve the problem my second commit is trying to solve:

The first click on the picker does not apply the optimization. You need to either re-click on the picker or start drawing. This is especially problematic for the "auto-tune" picker where the goal is really to save click and behave correctly at the first click.
While dragging the mouse to select an area, the optimization is done only while releasing the mouse. I can leave with that, but it's inconsistent with exposure.

I'll try catching the "draw" signal. Unless I missed something this is at least conceptually wrong (although exposure is a proof that it can work): it's called when the widget is redrawn, we want to call it when the pixelpipe reaches the module. But at least, being consistent with exposure means we can refactor all modules the same way if we ever find a better way.

moy · 2018-11-11T12:40:17Z

After experimenting a bit, it seems that:

The draw signal is received right before the module is applied in the pipeline. This is good for us.
It's also received in many other places. It shouldn't be much of an issue for us, since the autotuning is relatively fast. At worse, we can protect it with "if nothing changed, don't re-optimize" or so.
It seems to be called both for the preview pipeline and for the full pipeline. Shouldn't be much of an issue either, but there might be cases where we run either for the preview or the full pipeline depending on which one runs fastest.

moy · 2018-11-11T14:08:54Z

I just force-pushed a version that should be OK:

No more calls in process() functions.
Picker applied ASAP, i.e. when pushing the button and as the picking area changes
Code simpler than before (57 insertions(+), 82 deletions(-), and I added several comments :-) ).

I'm still not happy with putting the code in the draw signal, it's called far too often and doesn't match what we want conceptually, but it's only called too often, and my code protects against useless calls (setting picked_color_max to -INFINITY), so it's OK.

Details in the commit messages.

TurboGit · 2018-11-11T15:54:57Z

There is a GUI issue on my side. Click on one picker, click again on the same picker. It says with an active state.

TurboGit · 2018-11-11T15:55:33Z

src/iop/filmic.c

@@ -1244,11 +1212,18 @@ void gui_init(dt_iop_module_t *self)
  gtk_widget_set_tooltip_text(g->security_factor, _("enlarge or shrink the computed dynamic range"));
  g_signal_connect(G_OBJECT(g->security_factor), "value-changed", G_CALLBACK(security_threshold_callback), self);

-  g->auto_button = gtk_button_new_with_label(_("auto tune source"));
+  g->auto_button = dt_bauhaus_combobox_new(self);
+  dt_bauhaus_widget_set_label(g->auto_button, NULL, _("auto tune globally"));


Please keep old string "auto tune source", sounds better.

To me, it doesn't sound better. I don't understand what "auto tune source" was supposed to mean actually. I guess "source" means the source image, and the source image isn't autotuned. The parameters are.

Perhaps "auto tune grey/black/white", to make it explicit that the 3 parameters are considered together?

Yes it means "tune source image". That's the way I understand it and the way Aurélien wrote it. To me it is clear. Maybe we can come up with something better: "auto tune levels" ?

I like auto tune levels indeed. Shorter than my "auto tune grey/black/white" but means essentially the same thing.

TurboGit · 2018-11-11T15:57:33Z

BTW, isn't the callback just before process() the routine commit_params()? Would it work better to use this instead of draw()?

moy · 2018-11-11T16:27:40Z

There is a GUI issue on my side. Click on one picker, click again on the same picker. It says with an active state.

I can't reproduce. I tried:

"double click", i.e. re-click before the effect is applied.
click, wait, click, i.e. re-click after the image is fully recomputed.

Both work for me. Can you give more details on what you did so that I can see what's going on? When you say "active", do you mean the picker is active (drag on image draws the rectangle) or just the icon is still white?

moy · 2018-11-11T16:28:56Z

BTW, isn't the callback just before process() the routine commit_params()? Would it work better to use this instead of draw()?

Is there a documentation about all this somewhere?

TurboGit · 2018-11-11T16:37:33Z

Is there a documentation about all this somewhere?

Not that I know.

moy · 2018-11-11T16:39:20Z

Experimentally, it seems commit_params is called right before the module, but doesn't have the picked_color_*. I guess the colorpicker comes right after that.

TurboGit · 2018-11-11T16:40:33Z

Both work for me. Can you give more details on what you did so that I can see what's going on? When you say "active", do you mean the picker is active (drag on image draws the rectangle) or just the icon is still white?

Yes, that the picker stays white.

I can reproduce on all pickers:

click a picker
the area appear on the picture
click again on the same picker
the area disappear from the picture
this picker icon is still white as if selected
click on the image and the picker is not white anymore, meaning only the icon was not refreshed

Before this, the callbacks to take the picked color into account were only called after releasing the mouse or on deactivation of the picker. This has several drawbacks: * This is inconsistant with the exposure module. * Nothing happens on first click. Sometimes, especially for the auto-tune, picking the whole image is sufficient, hence one may want to get the effect applied when clicking the picker, without even selecting an image area. Fix this by calling apply_picked_color within the pipeline when the picker is active. Once this "continous picking" is inplace, there is no need to apply anything anymore at the time the picker is disabled. This considerably simplifies the logic behind color_picker_callback. Also, there is now no need to call call_apply_picked_color() on mouse release (i.e. when the selection is done). The boolean apply_picked_color is removed, we use the special value picked_color_max = -INFINITY instead when the color picker should be ignored. The value is set to a positive value by the colorpicker library when we get a new value to process.

There would be a bad interaction between moving the slider and the active color picker: the user was able to move the slider but it was comming back immediately after when the picked color was taken into account.

moy · 2018-11-11T17:03:44Z

I sill can't reproduce, but I guess I found the issue (a call to dt_control_queue_redraw before set_colorpick_state). Just force-pushed a fix. Can you test again?

TurboGit · 2018-11-11T17:27:11Z

This is very strange, I still have the same issue.

moy · 2018-11-11T17:34:37Z

Hmm, I guess I can't do more without being able to reproduce locally :-(.

In principle, the second click should execute

    disable_colorpick(self);
  }
  set_colorpick_state(g, g->which_colorpicker);
  dt_control_queue_redraw();

(at the end of color_picker_callback)

Can you check that the code actually goes this way (GDB or printf)?

I really don't see what could go wrong in this piece of code ...

TurboGit · 2018-11-11T17:56:56Z

Yes, this is called, it seems that there is no redraw of the widget. As I said as soon as I click on the image I do have a draw signal (tested by adding a printf() in the draw() callback) and the icon are properly refreshed. So all this is just a refresh issue!

TurboGit · 2018-11-11T18:14:54Z

I have a solution. No time for now. I'll propose a patch over this PR.

moy · 2018-11-11T18:15:32Z

Cool, thanks.

TurboGit · 2018-11-11T21:53:03Z

Merged with my fix. It was a nice case :) Yet I think we should have a better support for colorpicker and code sharing for all modules. It is quite painful to have to deal with all those details in all modules :(

moy · 2018-11-12T07:12:05Z

Yet I think we should have a better support for colorpicker and code sharing for all modules.

I fully agree. It's less painful now that there's less code to deal with it, but it should still be better factored.

I won't have much time for this in the next few days, but we should:

Before the release, port this PR to profile_gamma and colorbalance
Factor the common parts in libs/colorpicker

TurboGit · 2018-11-12T16:54:22Z

@moy : I'm on that. Should have something by tonight. The factoring of code is done for filmic.

TurboGit · 2018-11-12T20:04:47Z

@moy : done, if you have a chance to do some testing do not hesitate.

moy · 2018-11-12T20:41:30Z

The "auto tune" button in profile_gamma.c, and "optimize luma" / "neutralize colors" in color balance are still broken the same way as the old "auto tune" in filmic. Using the color picker API there like filmic should solve this.

Changing mode in color balance should probably deactivate the picker. Currently, you can activate the fulcrum picker, change mode to lift/gamma/gain where the picker disappears, and still have the picker activated (i.e. dragging within the picture grabs a color, and it seems to do weird things then). Same for the "color control slider" dropdown.

Other pickers (exposure, levels, perhaps clut and white balance) could benefit from this, but this may wait until after the release (no need to risk breaking them now if they are not broken ...).

THANKS!

@aurelienpierre : just a ping in case you didn't see this discussion (a bit hidden in a closed PR ...).

TurboGit · 2018-11-12T20:45:37Z

The "auto tune" button in profile_gamma.c, and "optimize luma" / "neutralize colors" in color balance are still broken the same way as the old "auto tune" in filmic. Using the color picker API there like filmic should solve this.

I don't follow, profile_gamma and colorbalance are now using the color picker API.

TurboGit · 2018-11-12T20:52:37Z

Changing mode in color balance should probably deactivate the picker.

Done. Also resetting picker status when changing controls.

moy · 2018-11-12T20:52:51Z

Hmm, I was using the darktable binary within the build tree (now that it's possible), but apparently I was getting half of the new version (eg. the Git commit id in the version string was the new one), but half of the old version (i.e. not all your changes).

TurboGit · 2018-11-12T20:53:31Z

@moy : yes looks like running from build-tree is not working

moy · 2018-11-12T20:53:54Z

auto-tune levels appears in "unbreak input profile" when the mode is "gamma". I think it should appear only in log mode.

TurboGit · 2018-11-12T20:54:57Z

Right! Good catch, will fix.

moy · 2018-11-12T20:57:49Z

clicking on the "reset" button in "color balance" doesn't disable the color picker icon if the selected icon is one of the "autotune" pickers. Weird.

TurboGit · 2018-11-12T20:59:47Z

I saw that. Just don't know why yet.

moy · 2018-11-12T21:03:10Z

You changed the text from "neutralize colors" to "optimize color". I think the old text was better, it's not really optimizing, but neutralizing.

Actually, in color balance, I think the optimizers are a bit more complex: they were using previously selected areas using the 3 pickers. So it was probably to keep them as normal buttons. No time to investigate more, sorry.

aurelienpierre · 2018-11-12T21:05:17Z

Actually, in color balance, I think the optimizers are a bit more complex: they were using previously selected areas using the 3 pickers. So it was probably to keep them as normal buttons. No time to investigate more, sorry.

Yes indeed. What have you done to my baby ? 🤣

moy · 2018-11-12T21:06:16Z

clicking on the "reset" button in "color balance" doesn't disable the color picker icon if the selected icon is one of the "autotune" pickers. Weird.

Same in profile_gamma, but not in filmic. I don't understand why.

TurboGit · 2018-11-12T21:10:02Z

I think we are facing a Gtk bug. This is just a redraw of the icon issue.

TurboGit · 2018-11-12T21:13:45Z

I have restored the label, sorry a cut&paste error :(

For the colorpicker in color balance I think I have kept the previous behavior (using the selected region if any).

moy · 2018-11-13T06:44:53Z

For the colorpicker in color balance I think I have kept the previous behavior (using the selected region if any).

OK, the behavior is actually more complex than I thought. If 3 patches have been selected, it takes them, but otherwise it guesses from the whole image. Not sure it's really a good idea from the UI point of view, it's really hard to understand how the buttons work without looking at the code :-(.

This can be improved in several ways IMHO. Not all ideas are good, just thinking aloud:

If only 1 or 2 of the 3 patches have been selected, then the callback could use the ones selected and do the whole-image guessing only for the missing one.
If the 3 patches have been selected, then no need to activate the picker: the optimizer could run right away when clicking the button.
Currently, the first call to the callback disables the picker callback but keeps the picker active. I think the line self->request_color_pick = DT_REQUEST_COLORPICK_OFF; in apply_autoluma should either be removed or replaced with dt_iop_color_picker_reset.
Having the button be shaped like pickers when the user is more or less supposed to have picked the colors elsewhere before is confusing. I'd suggest using the picker API but not the picker button. If apply_autoluma disables the picker, it'll be transparent for the user that it's a picker (except for the fact that it'll draw the picker's rectangle temporarily).

moy · 2018-11-13T06:49:34Z

Actually, on overall it seems the pickers in colorbalance are broken now. I already faced this before the last commits, but very often I use the pickers and get a full white image.

aurelienpierre · 2018-11-13T07:07:46Z

it seems to work on my side.

Not sure it's really a good idea from the UI point of view, it's really hard to understand how the buttons work without looking at the code :-

that's because there is still no doc on that. The way I designed it was: have a quick fallback for 80 % of the easy cases, then unclutch the auto mode when it fails. So, the button triggers a full-picture guess when user lazy, and gets whatever the user feeds it otherwise.

moy · 2018-11-13T07:20:20Z

So, the button triggers a full-picture guess when user lazy, and gets whatever the user feeds it otherwise.

The problem is that there's no feedback on what is done: there's no indication about which patch have already been selected, and no indication whether it's a full-picture guess or a 3-patches optimization.

Perhaps just a dt_control_log(_("no patch selected, doing a full-picture guess.")); (or "not enough patches selected") in the case of auto-guessing would help already. Ideally there would also be a visual indication on which patch have already been selected, but it'd be a much more involved change.

TurboGit · 2018-11-13T13:32:37Z

Maybe we should change the label of the color picker when the 3 luma patches (or color patches) have been selected from :

"optimize luma" to "optimize luma from patches"
"neutralize colors" to "neutralize colors from patches"

What do you think?

moy · 2018-11-13T13:42:21Z

Good idea.

TurboGit · 2018-11-13T14:18:50Z

Do you want to work on that? Or should I go with this...

moy · 2018-11-13T14:20:14Z

If you have time, go with it. I'm spending too much time on dt and not enough on other urgent stuff these days :-(.

TurboGit · 2018-11-13T19:55:55Z

Done. Also, I think I have fixed the redraw issue. Was due to some missing redraw in the bauhaus widgets.

moy · 2018-11-13T20:24:38Z

It's getting better and better. I think it'd be better if the optimize/neutralize buttons were not color pickers (i.e. the new behavior with the old buttons), but I can leave with both.

filmic: turn auto button into a picker

0c96ca1

moy mentioned this pull request Nov 11, 2018

filmic: disable color picker icon when 'auto optimized' is clicked #1814

Merged

moy force-pushed the filmic-auto-picker branch from 5dcd4e2 to 25876d1 Compare November 11, 2018 13:57

moy force-pushed the filmic-auto-picker branch from 25876d1 to fa99edc Compare November 11, 2018 14:12

TurboGit reviewed Nov 11, 2018

View reviewed changes

moy added 2 commits November 11, 2018 18:02

filmic: disable color picker when moving black/white/middle sliders

aa8c438

There would be a bad interaction between moving the slider and the active color picker: the user was able to move the slider but it was comming back immediately after when the picked color was taken into account.

moy force-pushed the filmic-auto-picker branch from fa99edc to aa8c438 Compare November 11, 2018 17:03

TurboGit closed this Nov 11, 2018

LebedevRI added this to the 2.6 milestone Nov 29, 2018

Filmic auto picker #1818

Filmic auto picker #1818

Conversation

moy commented Nov 10, 2018 • edited Loading

moy commented Nov 11, 2018

TurboGit commented Nov 11, 2018

TurboGit commented Nov 11, 2018

TurboGit commented Nov 11, 2018

moy commented Nov 11, 2018

moy commented Nov 11, 2018

moy commented Nov 11, 2018 • edited Loading

TurboGit commented Nov 11, 2018

TurboGit Nov 11, 2018

Choose a reason for hiding this comment

moy Nov 11, 2018

Choose a reason for hiding this comment

TurboGit Nov 11, 2018

Choose a reason for hiding this comment

moy Nov 11, 2018

Choose a reason for hiding this comment

TurboGit commented Nov 11, 2018

moy commented Nov 11, 2018

moy commented Nov 11, 2018

TurboGit commented Nov 11, 2018

moy commented Nov 11, 2018

TurboGit commented Nov 11, 2018

moy commented Nov 11, 2018

TurboGit commented Nov 11, 2018

moy commented Nov 11, 2018

TurboGit commented Nov 11, 2018

TurboGit commented Nov 11, 2018

moy commented Nov 11, 2018

TurboGit commented Nov 11, 2018

moy commented Nov 12, 2018

TurboGit commented Nov 12, 2018

TurboGit commented Nov 12, 2018

moy commented Nov 12, 2018

TurboGit commented Nov 12, 2018

TurboGit commented Nov 12, 2018 • edited Loading

moy commented Nov 12, 2018

TurboGit commented Nov 12, 2018

moy commented Nov 12, 2018

TurboGit commented Nov 12, 2018

moy commented Nov 12, 2018

TurboGit commented Nov 12, 2018

moy commented Nov 12, 2018

aurelienpierre commented Nov 12, 2018

moy commented Nov 12, 2018

TurboGit commented Nov 12, 2018

TurboGit commented Nov 12, 2018

moy commented Nov 13, 2018

moy commented Nov 13, 2018

aurelienpierre commented Nov 13, 2018

moy commented Nov 13, 2018

TurboGit commented Nov 13, 2018

moy commented Nov 13, 2018

TurboGit commented Nov 13, 2018

moy commented Nov 13, 2018

TurboGit commented Nov 13, 2018

moy commented Nov 13, 2018

moy commented Nov 10, 2018 •

edited

Loading

moy commented Nov 11, 2018 •

edited

Loading

TurboGit commented Nov 12, 2018 •

edited

Loading