[kofa73]

Addresses #19873

What this would buy us:

• technical
  • when the dual white balance + color calibration method was introduced, white balance used the 'camera reference' (D65) multipliers. Colours, including neutral areas, were fixed in color calibration, via proper CAT. The drawback was that some algorithms are hurt by the not-quite-accurate white balance.
    Quoting 1c5df70, which introduced as-shot to reference - the excerpt below is about the use of reference (D65) multipliers:

    There is one drawback with this approach. Some modules in the pipe would like to have "perfect white balance" correction - the rgb channels all have the same value for any greytone.

    Examples:

    1. Some hightlights reconstruction algos take the other channels or surrounding data into account and modify data "towards white".
    2. raw chromatic aberration correction also iterates from channel differences. Good "white is white" coeffs help significantly here.
    3. Some demosaicers also have slightly improved output on pixelpeeping level.

  • we now take this further, allowing the user to choose RGB multipliers via any method (sliders, colour picker, temperature + tint) supported by the white balance module, to provide neutrals that are even better than the camera's estimate
  • finally, we allow color calibration to use those user-provided multipliers ('as set in white balance module', in addition to 'as shot in camera' introduced in 1c5df70).
• usability
  • these changes allow users to work with controls they are familiar with from other software, like 'temperature' and 'tint', and setting white balance in white balance, a more discoverable place for newcomers.

Changes:

• added a new checkbox, prepare data for color calibration to white balance temperature.c to explicitly control the late_correction flag in 'user' / 'manual' modes
  • the checkbox is only visible in such modes (late_correction is always off in 'camera reference' mode, always on in 'as-shot to reference')
  • if switching from 'camera reference' or 'as-shot to reference' to a 'user' mode, the checkbox is automatically ticked if color calibration is enabled
• replaced reference chr->as_shot with chr->wb_coeffs in highlights.c, colorin.c, opposed.c and segbased.c
• illuminants.h:
  • added a new illuminant, DT_ILLUMINANT_FROM_WB, plus handling in conditionals, similar to DT_ILLUMINANT_CAMERA, but relying on dev->chroma->wb_coeffs
  • find_temperature_from_raw_coeffs:
    • renamed to find_temperature_from_as_shot_coeffs
    • extracted the logic to find_temperature_from_wb_coeffs, to allow directly using coefficients from temperature.c, instead of always going through ratios relative to the D65 values
    • added param wb_coeffs to illuminant_to_xy for absolute coefficients from temperature.c; renamed param custom_wb to correction_ratios to express what it represents; passed to find_temperature_from_wb_coeffs in DT_ILLUMINANT_FROM_WB mode
• channelmixerrgb.c:
  • _dev_is_D65_chroma no longer needs as_shot multipliers in late_correction mode
  • renamed _get_white_balance_coeff to _get_d65_correction_ratios
  • wired up the calls to find_temperature_from_wb_coeffs to handle DT_ILLUMINANT_FROM_WB

Draft for now, to allow discussion (and I haven't tested much, but wanted to make the changes visible to others; plus, I need to clean up some temp files)

Open issues

These were found by the bots, I'll go through them, will take time.

Major

• src/iop/temperature.c:1565 — In the default scene-referred workflow, reload_defaults() leaves late_correction = TRUE for images without camera-matrix support (ordinary non-raw and HDR RGB input), so for a non-Lab input profile colorin's D65 correction (D65coeffs = {1,1,1,1}) exactly cancels manually enabled and adjusted white balance; monochrome raws are not affected because hide_enable_button forces piece->enabled = FALSE and commit_params returns before setting chr->late_correction.
• src/iop/temperature.c:1203 — _update_preset() uses chr->adaptation != NULL as a proxy for "color calibration is doing CAT", but channelmixerrgb registers itself unconditionally in reload_defaults, so leaving the D65 preset always switches late_correction on and silently replaces the user's white balance with the camera reference when color calibration is disabled.
• src/iop/colorin.c:1201 — The new arbitrary-WB correction paths read session-global WB state instead of the current pipe's coefficients, so concurrent pipe generations can render and cache colors using mismatched white balance.
• src/iop/hlreconstruct/opposed.c:236 — The shared-state defect recorded above at
  src/iop/colorin.c:1201 also exists in three highlight-reconstruction paths that this branch
  switched from the per-image constant chr->as_shot to the live chr->wb_coeffs; in
  opposed.c the value derived from the racy read is additionally stored in a process-wide
  static chrominance cache under a key built from the pipe's committed parameters, so one
  mixed-generation run keeps being served until those parameters change.
• src/iop/temperature.c:1223 — _update_preset() passes the DtBauhausWidget toggle g->check_late_correction to the GtkToggleButton API, an invalid GObject cast, so every call logs GLib/GTK criticals and the "prepare data for color calibration" checkbox keeps a stale state on preset transitions that change late_correction, until the next dt_iop_gui_update().

Minor

• dev-doc updates needed

To be investigated further

• src/iop/channelmixerrgb.c:593 — This branch redefined _dev_is_D65_chroma() to return TRUE for
  any late_correction state, where the base version additionally required
  wb_coeffs == as_shot. As a result _get_d65_correction_ratios() now yields all-1.0 ratios in
  exactly the state this branch exists to enable — a manual white balance with late_correction
  on — so the "as set in camera" illuminant is derived from the unmodified EXIF as-shot
  coefficients in process(), process_cl(), reload_defaults() and the GUI, and the
  wb_applied_twice warning can no longer fire. Whether 1.0 is the correct new value depends on
  what colorin normalises the data to -> not a bug: with late correction on, "as shot" and a manual WB render the same outside clipped areas (dE2000 mean 0.15); the "white balance applied twice" warning appears only with late correction off and WB not at camera reference

[ralfbrown]

Did you intend to include cmixer_orig.c.txt in the PR?
And are the two .md files in the right place?

[kofa73]

Did you intend to include cmixer_orig.c.txt in the PR? And are the two .md files in the right place?

See the end of the description:

Draft for now, to allow discussion (and I haven't tested much, but wanted to make the changes visible to others; plus, I need to clean up some temp files)

[kofa73]

@ralfbrown : extra files removed, thank for your patience

[jenshannoschwalm]

A first complete code reading and remembering your comments before; generally i think this is a minor but very good improvement and will certainly help with some issues. Also it make the intention more clear and fixes some common misunderstandings.

I spotted some places like this:

dt_aligned_pixel_t correction;
  for_four_channels(k)
  {
    if(late && chr->wb_coeffs[k] > 1e-6f)
      correction[k] = chr->D65coeffs[k] / chr->wb_coeffs[k];
    else
      correction[k] = 1.0f;
  }

Could you explain if/why this is nevessary or is it just code-style?

[kofa73]

The key is the line:

correction[k] = chr->D65coeffs[k] / chr->wb_coeffs[k];

This used to be something like (below is one concrete example):

chr->D65coeffs[0] / chr->as_shot[0]

The new denominator comes from chr->wb_coeffs[k] (value set in temperature.c), which means it may be set using one of the sliders. The sliders may have a value of 0, so we must protect the division:

typedef struct dt_iop_temperature_params_t
{
  float red;     // $MIN: 0.0 $MAX: 8.0
  float green;   // $MIN: 0.0 $MAX: 8.0
  float blue;    // $MIN: 0.0 $MAX: 8.0
  float various; // $MIN: 0.0 $MAX: 8.0
  int preset;
} dt_iop_temperature_params_t;

One option would be to restrict the values to MIN: 0.1 or so, then we can avoid adding the protection everywhere.

If I misunderstood the question, please let me know.

[kofa73]

Any updates? There have been no new commits for a while, just resolving conflicts.

[TurboGit]

@kofa73 : This gets under the radar and I had many things to do for the AI integration. The discussion on pixl.us was difficult and my motivation was quite down for some time... I'm still a simple human :) I'll get back to this for 5.6.

[kofa73]

Had the bots go through the remaining issues that were genuinely new on this branch; the remaining ones existed in some shape or form already on master. Since this is huge enough, I'd continue with them once (if) this gets merged. Thanks for your patience.