channelmixerrgb: display an error message when temperature.c does not…

[aurelienpierre]

… use camera reference white balance

[elstoc]

If we're going to rely on a warning message on the CAT tab to prevent user error, I suppose we might be better off reverting #6793.

[TurboGit]

I suppose we might be better off reverting #6793.

Or set CAT tab active when an error is reported. Maybe a big annoying, but the user has to take some action anyway.

[elstoc]

It might be worth displaying a similar warning on the WB module where D65 isn't selected but CAT tab is enabled (or modern workflow is enabled). On old edits WB remains fully editable with all of its controls, regardless of the workflow setting.

... and if we could change the module header text to red or something for both modules that would be great.

[aurelienpierre]

The latest commit introduces a global method for modules to show a warning sign in header if they require attention:

[aurelienpierre]

I wouldn't go as far as nagging users in white balance module too. There are legitimate reasons to not use the default coeffs in WB, for example because the Nikon input matrices tend to overdo the red, so the best option is to save a preset with a slightly reduced R coeff, compared to default D65. In that case, the alert would be annoying and a false positive.

[elstoc]

I wouldn't go as far as nagging users in white balance module too

I was talking about old edits, where the module still displays all of its controls because the user hasn't yet reset it.

[aurelienpierre]

Old edits should not use color calibration, there is no need to reset anything. If users choose to enable color calibration, they will see the message and take appropriate measures. Old edits should be status quo.

[elstoc]

Ok just trying to cover all bases here. Should the message in color calibration recommend resetting parameters in white balance instead of choosing camera reference, since this actually removes most of the controls from the module.

Note there is an issue for old edits whereby the white balance module shows all of its controls but editing those controls has no effect. The user physically cannot select camera reference. Or perhaps I need to rebuild? (edit: no - still there)

[aurelienpierre]

The 2 latest commits simplify things a lot.

1. temperature.c will write a boolean in dev->proxy.wb_use_D65, the pipeline structure, if it uses the camera reference coefficients
2. channelmixerrgb.c will write its first instance (in the pipeline order) that does chromatic adaptation in dev->proxy.chroma_adaptation, in the pipeline structure.

Then, warning are displayed in white balance and color calibration modules, depending on the pipeline and inner states.

1. if white balance sees a color calibration module has captured the CAT on the pipe, and its coeffs don't match D65, it will show a warning
2. if color calibration does CAT and sees wb_use_D65 == FALSE, it will show a warning about white balance,
3. if color calibration does CAT and sees that dev->proxy.chroma_adaptation is captured by another instance than itself, it will show a warning about another color calibration module.

In case several color calibration instances are competing to capture dev->proxy.chroma_adaptation, the one that has the lowest pipeline order wins. This should be updated as soon as the instances are inverted.

Therefore, these checks are independent of workflows and states that are to be assumed externally, leading to brittle sets of tests and forgotten corner cases.

Notice there are some glitches regarding GUI updates, you need to interact with the modules for the labels to be updated. They are supposed to be updated as soon as the pipeline recomputes too, but it doesn't work.

[matt-maguire]

So, had the CAT in legacy mode for a new image, temperature is doing the WB. Enable channelmixer-rgb, CAT is bypassed as expected. set CAT to something like Daylight, then get warning about conflict with temperature module. Go down to temperature, disable it, and compress history stack. Now channelmixer-rgb complains about double CAT. There's only one instance, but the shift in position seems to have confused things. It's a bit hard to recover from this, maybe need to go back to lighttable, discard history, exit and re-enter darktable.

[edit: oops, may have jumped the gun, let me pull those latest commits and retest]

[aurelienpierre]

Another issue with this is enabling/disabling the local contrast module switches the method (bilateral vs. local laplacian) too.

[elstoc]

If you remove the conditionality here it resolves (mostly) the error where white balance module header isn't being updated when moving around the history stack. There still seems to potentially be an issue where you change the history while there is a pipe computation in progress so it's not 100% reliable but it's much better without this if statement.

[elstoc]

There's a similar error when you discard history with modern workflow, whereby the color calibration header isn't updated until you click on it, but I've not been able to track it down yet. When discarding history the gui_changed function is called once and for some reason it thinks that the current module is not the CAT module: CAT_instance != current_instance is true. Presumably the gui needs to be updated after the pipe has completed but I don't know how to do this.

[aurelienpierre]

Updating GUI after pipe finished is supposed to be done in _develop_ui_pipe_finished_callback() in channelmixerrgb.c, connected to DT_DEBUG_CONTROL_SIGNAL_CONNECT(darktable.signals, DT_SIGNAL_DEVELOP_UI_PIPE_FINISHED, G_CALLBACK(_develop_ui_pipe_finished_callback), self);.
For some reason, that call is useless.

[elstoc]

I've made some changes that I think solve most of the gui update issues I've found. Hopefully they're not to contentious. elstoc@edb65e5

[elstoc]

Another issue with this is enabling/disabling the local contrast module switches the method (bilateral vs. local laplacian) too

What on Earth is doing that? Never mind - try this commit: elstoc@a35e2c9.

I think that fixes all the bugs now.

[elstoc]

Ok really I'm gonna stop testing now. One final attempt: elstoc@39afe42.

As an aside: is this the best way to suggest changes to PRs?

[aurelienpierre]

Thanks for patches, I will test soon.

[aurelienpierre]

The local contrast bug is still present, and now color calibration gets auto-applied with wrong settings on some older pictures, leading to full black images.

[aurelienpierre]

Seems to work as expected now. @TurboGit waiting for your review.

[elstoc]

Phew that took some work. Definitely pretty well user-proof now so I think it was worth it.

[TurboGit]

I'll review tomorrow.

[TurboGit]

Works for me!