From 2af6dcbe80b7bed0cd7b6481ff4e6a1759bc2c03 Mon Sep 17 00:00:00 2001 From: Kofa Date: Wed, 10 Dec 2025 19:25:38 +0100 Subject: [PATCH 01/27] wb: added new param late_correction to temperature.c, exposed as a checkbox; populate dev->chroma->late_correction based on checkbox-driven late_correction flag for 'legacy' modes (DT_IOP_TEMP_AS_SHOT, DT_IOP_TEMP_SPOT, DT_IOP_TEMP_USER) --- src/iop/temperature.c | 97 ++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 92 insertions(+), 5 deletions(-) diff --git a/src/iop/temperature.c b/src/iop/temperature.c index bdd51b34b18..b85a8989450 100644 --- a/src/iop/temperature.c +++ b/src/iop/temperature.c @@ -42,7 +42,7 @@ #include "common/colorspaces.h" #include "external/cie_colorimetric_tables.c" -DT_MODULE_INTROSPECTION(4, dt_iop_temperature_params_t) +DT_MODULE_INTROSPECTION(5, dt_iop_temperature_params_t) #define INITIALBLACKBODYTEMPERATURE 4000 @@ -71,6 +71,7 @@ typedef struct dt_iop_temperature_params_t float blue; // $MIN: 0.0 $MAX: 8.0 float various; // $MIN: 0.0 $MAX: 8.0 int preset; + gboolean late_correction; // $DEFAULT: FALSE $DESCRIPTION: "prepare data for color calibration" } dt_iop_temperature_params_t; typedef struct dt_iop_temperature_gui_data_t @@ -86,6 +87,7 @@ typedef struct dt_iop_temperature_gui_data_t GtkWidget *btn_d65_late; GtkWidget *temp_label; GtkWidget *balance_label; + GtkWidget *check_late_correction; int preset_cnt; int preset_num[54]; double mod_coeff[4]; @@ -141,6 +143,16 @@ int legacy_params(dt_iop_module_t *self, int preset; } dt_iop_temperature_params_v4_t; + typedef struct dt_iop_temperature_params_v5_t + { + float red; + float green; + float blue; + float various; + int preset; + gboolean late_correction; + } dt_iop_temperature_params_v5_t; + if(old_version == 2) { typedef struct dt_iop_temperature_params_v2_t @@ -178,6 +190,22 @@ int legacy_params(dt_iop_module_t *self, *new_version = 4; return 0; } + if(old_version == 4) + { + const dt_iop_temperature_params_v4_t *o = (dt_iop_temperature_params_v4_t *)old_params; + dt_iop_temperature_params_v5_t *n = malloc(sizeof(dt_iop_temperature_params_v5_t)); + + n->red = o->red; + n->green = o->green; + n->blue = o->blue; + n->various = NAN; + n->preset = DT_IOP_TEMP_UNKNOWN; + n->late_correction = FALSE; + *new_params = n; + *new_params_size = sizeof(dt_iop_temperature_params_v5_t); + *new_version = 5; + return 0; + } return 1; } @@ -720,7 +748,13 @@ void commit_params(dt_iop_module_t *self, If piece is disabled we always clear the trouble message and make sure chroma does know there is no temperature module. */ - chr->late_correction = p->preset == DT_IOP_TEMP_D65_LATE; + if(p->preset == DT_IOP_TEMP_D65_LATE) + chr->late_correction = TRUE; + else if(p->preset == DT_IOP_TEMP_D65) + chr->late_correction = FALSE; + else + chr->late_correction = p->late_correction; + chr->temperature = piece->enabled ? self : NULL; if(dt_pipe_is_preview(pipe) && !piece->enabled) dt_iop_clear_module_trouble_message(self); @@ -1147,9 +1181,44 @@ static void _update_preset(dt_iop_module_t *self, int mode) { dt_iop_temperature_params_t *p = self->params; dt_dev_chroma_t *chr = &self->dev->chroma; + dt_iop_temperature_gui_data_t *g = self->gui_data; + + const gboolean is_current_reference = (p->preset == DT_IOP_TEMP_D65_LATE) || + (p->preset == DT_IOP_TEMP_D65); + const gboolean is_new_mode_manual = (mode != DT_IOP_TEMP_D65_LATE) && + (mode != DT_IOP_TEMP_D65); + + if (is_current_reference && is_new_mode_manual) + { + // set iff color calibration active in adaptation mode + p->late_correction = (chr->adaptation != NULL); + } p->preset = mode; - chr->late_correction = mode == DT_IOP_TEMP_D65_LATE; + + if (mode == DT_IOP_TEMP_D65_LATE) + { + chr->late_correction = TRUE; + } + else if (mode == DT_IOP_TEMP_D65) + { + chr->late_correction = FALSE; + } + else + { + // For manual modes, use the parameter + chr->late_correction = p->late_correction; + } + + if (g && g->check_late_correction) + { + // Hide checkbox in reference modes (logic is enforced hardcoded) + // Show in manual modes (user has control) + gtk_widget_set_visible(g->check_late_correction, is_new_mode_manual); + + gtk_toggle_button_set_active(GTK_TOGGLE_BUTTON(g->check_late_correction), + p->late_correction); + } } void gui_update(dt_iop_module_t *self) @@ -1500,6 +1569,7 @@ void reload_defaults(dt_iop_module_t *self) dt_iop_temperature_params_t *p = self->params; d->preset = dt_is_scene_referred() ? DT_IOP_TEMP_D65_LATE : DT_IOP_TEMP_AS_SHOT; + d->late_correction = dt_is_scene_referred(); float *dcoeffs = (float *)d; for_four_channels(k) @@ -1725,8 +1795,10 @@ void gui_changed(dt_iop_module_t *self, GtkWidget *w, void *previous) _mul2temp(self, p, &g->mod_temp, &g->mod_tint); - dt_bauhaus_combobox_set(g->presets, DT_IOP_TEMP_USER); - _update_preset(self, DT_IOP_TEMP_USER); + if(w != g->check_late_correction) { + dt_bauhaus_combobox_set(g->presets, DT_IOP_TEMP_USER); + _update_preset(self, DT_IOP_TEMP_USER); + } } static void _btn_toggled(GtkGestureSingle *gesture, @@ -2146,12 +2218,27 @@ void gui_init(dt_iop_module_t *self) (g->scale_tint, _("color tint of the image, from magenta (value < 1) to green (value > 1)")); + g->check_late_correction = dt_bauhaus_toggle_from_params(self, "late_correction"); + + g_object_ref(g->check_late_correction); // Prevent destruction + GtkWidget *temp_parent = gtk_widget_get_parent(g->check_late_correction); + if(temp_parent) + gtk_container_remove(GTK_CONTAINER(temp_parent), g->check_late_correction); + + gtk_widget_set_tooltip_text(g->check_late_correction, + _("ensures the white balance coefficients are treated as a reference for the color calibration module.\n" + "keep this checked if you use the modern scene-referred workflow but want to manually adjust white balance.")); + GtkWidget *box_enabled = dt_gui_vbox(g->buttonbar, + g->check_late_correction, g->presets, g->finetune, temp_label_box, g->scale_k, g->scale_tint); + + g_object_unref(g->check_late_correction); + dt_gui_new_collapsible_section (&g->cs, "plugins/darkroom/temperature/expand_coefficients", From 3ca7e9d44fdbb1519f902feea1771123b4bc579e Mon Sep 17 00:00:00 2001 From: Kofa Date: Tue, 16 Dec 2025 12:25:27 +0100 Subject: [PATCH 02/27] wb: Updated the rest of the files, but channelmixerrgb is broken, everything is purple. --- src/iop/channelmixerrgb.c | 44 +++++++++++++++++++++++++------- src/iop/colorin.c | 26 +++++++++++++------ src/iop/highlights.c | 12 ++++----- src/iop/hlreconstruct/opposed.c | 24 +++++++++++------ src/iop/hlreconstruct/segbased.c | 12 ++++++--- 5 files changed, 83 insertions(+), 35 deletions(-) diff --git a/src/iop/channelmixerrgb.c b/src/iop/channelmixerrgb.c index 6ad8118e727..b0f8177c642 100644 --- a/src/iop/channelmixerrgb.c +++ b/src/iop/channelmixerrgb.c @@ -594,9 +594,12 @@ void init_presets(dt_iop_module_so_t *self) static gboolean _dev_is_D65_chroma(const dt_develop_t *dev) { const dt_dev_chroma_t *chr = &dev->chroma; - return chr->late_correction - ? dt_dev_equal_chroma(chr->wb_coeffs, chr->as_shot) - : dt_dev_equal_chroma(chr->wb_coeffs, chr->D65coeffs); + + // if late correction is active, colorin guarantees the pipe is D65-aligned + // regardless of the specific user coefficients. + if (chr->late_correction) return TRUE; + + return dt_dev_equal_chroma(chr->wb_coeffs, chr->D65coeffs); } static gboolean _area_mapping_active(const dt_iop_channelmixer_rgb_gui_data_t *g) @@ -2195,7 +2198,7 @@ void process(dt_iop_module_t *self, // re-run the detection at runtime… float x, y; dt_aligned_pixel_t custom_wb; - _get_white_balance_coeff(self, custom_wb); + copy_pixel(custom_wb, self->dev->chroma.wb_coeffs); if(find_temperature_from_raw_coeffs(&(self->dev->image_storage), custom_wb, &(x), &(y))) { @@ -2306,7 +2309,7 @@ int process_cl(dt_iop_module_t *self, // re-run the detection at runtime… float x, y; dt_aligned_pixel_t custom_wb; - _get_white_balance_coeff(self, custom_wb); + copy_pixel(custom_wb, self->dev->chroma.wb_coeffs); if(find_temperature_from_raw_coeffs(&(self->dev->image_storage), custom_wb, &(x), &(y))) { @@ -3093,7 +3096,10 @@ void commit_params(dt_iop_module_t *self, float x = p->x; float y = p->y; dt_aligned_pixel_t custom_wb; - _get_white_balance_coeff(self, custom_wb); + if(p->illuminant == DT_ILLUMINANT_CAMERA) + copy_pixel(custom_wb, self->dev->chroma.wb_coeffs); + else + _get_white_balance_coeff(self, custom_wb); illuminant_to_xy(p->illuminant, &(self->dev->image_storage), custom_wb, &x, &y, p->temperature, p->illum_fluo, p->illum_led); @@ -3538,7 +3544,10 @@ static gboolean _illuminant_color_draw(GtkWidget *widget, float y = p->y; dt_aligned_pixel_t RGB = { 0 }; dt_aligned_pixel_t custom_wb; - _get_white_balance_coeff(self, custom_wb); + if(p->illuminant == DT_ILLUMINANT_CAMERA) + copy_pixel(custom_wb, self->dev->chroma.wb_coeffs); + else + _get_white_balance_coeff(self, custom_wb); illuminant_to_xy(p->illuminant, &(self->dev->image_storage), custom_wb, &x, &y, p->temperature, p->illum_fluo, p->illum_led); illuminant_xy_to_RGB(x, y, RGB); @@ -3641,7 +3650,10 @@ static void _update_approx_cct(const dt_iop_module_t *self) float x = p->x; float y = p->y; dt_aligned_pixel_t custom_wb; - _get_white_balance_coeff(self, custom_wb); + if(p->illuminant == DT_ILLUMINANT_CAMERA) + copy_pixel(custom_wb, self->dev->chroma.wb_coeffs); + else + _get_white_balance_coeff(self, custom_wb); illuminant_to_xy(p->illuminant, &(self->dev->image_storage), custom_wb, &x, &y, p->temperature, p->illum_fluo, p->illum_led); @@ -3878,7 +3890,21 @@ void reload_defaults(dt_iop_module_t *self) d->adaptation = DT_ADAPTATION_CAT16; dt_aligned_pixel_t custom_wb; - if(!_get_white_balance_coeff(self, custom_wb)) + gboolean wb_retrieved = FALSE; + + // In modern mode, we want the actual coefficients (User/AsShot) to determine the CCT. + // In legacy mode, we rely on the helper which calculates a D65 ratio. + if (self->dev->chroma.late_correction && dt_image_is_matrix_correction_supported(img)) + { + copy_pixel(custom_wb, self->dev->chroma.wb_coeffs); + wb_retrieved = TRUE; + } + else + { + wb_retrieved = !_get_white_balance_coeff(self, custom_wb); + } + + if(wb_retrieved) { if(find_temperature_from_raw_coeffs(img, custom_wb, &(d->x), &(d->y))) d->illuminant = DT_ILLUMINANT_CAMERA; diff --git a/src/iop/colorin.c b/src/iop/colorin.c index 923d189500a..ad896d36f35 100644 --- a/src/iop/colorin.c +++ b/src/iop/colorin.c @@ -662,10 +662,15 @@ int process_cl(dt_iop_module_t *self, const dt_dev_chroma_t *chr = &self->dev->chroma; const gboolean corrected = chr->late_correction; - dt_aligned_pixel_t coeffs = { corrected ? chr->D65coeffs[0] / chr->as_shot[0] : 1.0f, - corrected ? chr->D65coeffs[1] / chr->as_shot[1] : 1.0f, - corrected ? chr->D65coeffs[2] / chr->as_shot[2] : 1.0f, - corrected ? chr->D65coeffs[3] / chr->as_shot[3] : 1.0f }; + dt_aligned_pixel_t coeffs; + for_four_channels(k) + { + if(corrected && chr->wb_coeffs[k] > 1e-6f) + coeffs[k] = chr->D65coeffs[k] / chr->wb_coeffs[k]; + else + coeffs[k] = 1.0f; + } + if(corrected) { for_four_channels(k) @@ -1208,10 +1213,15 @@ void process(dt_iop_module_t *self, const dt_dev_chroma_t *chr = &self->dev->chroma; const dt_iop_colorin_data_t *const d = piece->data; const gboolean corrected = chr->late_correction && d->type != DT_COLORSPACE_LAB; - const dt_aligned_pixel_t coeffs = { corrected ? chr->D65coeffs[0] / chr->as_shot[0] : 1.0f, - corrected ? chr->D65coeffs[1] / chr->as_shot[1] : 1.0f, - corrected ? chr->D65coeffs[2] / chr->as_shot[2] : 1.0f, - corrected ? chr->D65coeffs[3] / chr->as_shot[3] : 1.0f }; + dt_aligned_pixel_t coeffs; + for_four_channels(k) + { + if(corrected && chr->wb_coeffs[k] > 1e-6f) + coeffs[k] = chr->D65coeffs[k] / chr->wb_coeffs[k]; + else + coeffs[k] = 1.0f; + } + dt_dev_pixelpipe_t *pipe = piece->pipe; if(corrected) { diff --git a/src/iop/highlights.c b/src/iop/highlights.c index dc5a993ca3d..d87d12bb07a 100644 --- a/src/iop/highlights.c +++ b/src/iop/highlights.c @@ -679,9 +679,9 @@ int process_cl(dt_iop_module_t *self, dt_aligned_pixel_t clips = { clipper, clipper, clipper, clipper}; if(pipe->dsc.temperature.enabled && chr->late_correction) { - clips[0] *= chr->as_shot[0] / chr->D65coeffs[0]; - clips[1] *= chr->as_shot[1] / chr->D65coeffs[1]; - clips[2] *= chr->as_shot[2] / chr->D65coeffs[2]; + clips[0] *= chr->wb_coeffs[0] / chr->D65coeffs[0]; + clips[1] *= chr->wb_coeffs[1] / chr->D65coeffs[1]; + clips[2] *= chr->wb_coeffs[2] / chr->D65coeffs[2]; } dev_xtrans = dt_opencl_copy_host_to_device_constant(devid, sizeof(piece->xtrans), piece->xtrans); @@ -758,9 +758,9 @@ static void process_clip(dt_iop_module_t *self, dt_aligned_pixel_t clips = { clip, clip, clip, clip}; if(piece->pipe->dsc.temperature.enabled && chr->late_correction) { - clips[0] *= chr->as_shot[0] / chr->D65coeffs[0]; - clips[1] *= chr->as_shot[1] / chr->D65coeffs[1]; - clips[2] *= chr->as_shot[2] / chr->D65coeffs[2]; + clips[0] *= chr->wb_coeffs[0] / chr->D65coeffs[0]; + clips[1] *= chr->wb_coeffs[1] / chr->D65coeffs[1]; + clips[2] *= chr->wb_coeffs[2] / chr->D65coeffs[2]; } for(int row = 0; row < roi_out->height; row++) { diff --git a/src/iop/hlreconstruct/opposed.c b/src/iop/hlreconstruct/opposed.c index c712194a75f..409c6ac46e5 100644 --- a/src/iop/hlreconstruct/opposed.c +++ b/src/iop/hlreconstruct/opposed.c @@ -241,10 +241,14 @@ static float *_process_opposed(dt_iop_module_t *self, const dt_dev_chroma_t *chr = &self->dev->chroma; const gboolean late = chr->late_correction; - const dt_aligned_pixel_t correction = { late ? (float)(chr->D65coeffs[0] / chr->as_shot[0]) : 1.0f, - late ? (float)(chr->D65coeffs[1] / chr->as_shot[1]) : 1.0f, - late ? (float)(chr->D65coeffs[2] / chr->as_shot[2]) : 1.0f, - 1.0f }; + 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; + } const size_t iwidth = roi_in->width; const size_t iheight = roi_in->height; @@ -451,10 +455,14 @@ static cl_int process_opposed_cl(dt_iop_module_t *self, const dt_dev_chroma_t *chr = &self->dev->chroma; const gboolean late = chr->late_correction; - dt_aligned_pixel_t correction = { late ? (float)(chr->D65coeffs[0] / chr->as_shot[0]) : 1.0f, - late ? (float)(chr->D65coeffs[1] / chr->as_shot[1]) : 1.0f, - late ? (float)(chr->D65coeffs[2] / chr->as_shot[2]) : 1.0f, - 1.0f }; + 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; + } cl_int err = CL_MEM_OBJECT_ALLOCATION_FAILURE; cl_mem dev_chrominance = NULL; diff --git a/src/iop/hlreconstruct/segbased.c b/src/iop/hlreconstruct/segbased.c index 62545f631cb..20d33e117d3 100644 --- a/src/iop/hlreconstruct/segbased.c +++ b/src/iop/hlreconstruct/segbased.c @@ -466,10 +466,14 @@ static void _process_segmentation(dt_dev_pixelpipe_iop_t *piece, const dt_dev_chroma_t *chr = &piece->module->dev->chroma; const gboolean late = chr->late_correction; - const dt_aligned_pixel_t correction = { late ? (float)(chr->D65coeffs[0] / chr->as_shot[0]) : 1.0f, - late ? (float)(chr->D65coeffs[1] / chr->as_shot[1]) : 1.0f, - late ? (float)(chr->D65coeffs[2] / chr->as_shot[2]) : 1.0f, - 1.0f }; + 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; + } const int recovery_mode = d->recovery; const float strength = d->strength; From eb3ab9355237db53eceb6135c49ac7787b6a1af8 Mon Sep 17 00:00:00 2001 From: Kofa Date: Tue, 16 Dec 2025 13:50:20 +0100 Subject: [PATCH 03/27] wb: Undid changes to channelmixerrgb.c, will go carefully next time --- src/iop/channelmixerrgb.c | 44 ++++++++------------------------------- 1 file changed, 9 insertions(+), 35 deletions(-) diff --git a/src/iop/channelmixerrgb.c b/src/iop/channelmixerrgb.c index b0f8177c642..6ad8118e727 100644 --- a/src/iop/channelmixerrgb.c +++ b/src/iop/channelmixerrgb.c @@ -594,12 +594,9 @@ void init_presets(dt_iop_module_so_t *self) static gboolean _dev_is_D65_chroma(const dt_develop_t *dev) { const dt_dev_chroma_t *chr = &dev->chroma; - - // if late correction is active, colorin guarantees the pipe is D65-aligned - // regardless of the specific user coefficients. - if (chr->late_correction) return TRUE; - - return dt_dev_equal_chroma(chr->wb_coeffs, chr->D65coeffs); + return chr->late_correction + ? dt_dev_equal_chroma(chr->wb_coeffs, chr->as_shot) + : dt_dev_equal_chroma(chr->wb_coeffs, chr->D65coeffs); } static gboolean _area_mapping_active(const dt_iop_channelmixer_rgb_gui_data_t *g) @@ -2198,7 +2195,7 @@ void process(dt_iop_module_t *self, // re-run the detection at runtime… float x, y; dt_aligned_pixel_t custom_wb; - copy_pixel(custom_wb, self->dev->chroma.wb_coeffs); + _get_white_balance_coeff(self, custom_wb); if(find_temperature_from_raw_coeffs(&(self->dev->image_storage), custom_wb, &(x), &(y))) { @@ -2309,7 +2306,7 @@ int process_cl(dt_iop_module_t *self, // re-run the detection at runtime… float x, y; dt_aligned_pixel_t custom_wb; - copy_pixel(custom_wb, self->dev->chroma.wb_coeffs); + _get_white_balance_coeff(self, custom_wb); if(find_temperature_from_raw_coeffs(&(self->dev->image_storage), custom_wb, &(x), &(y))) { @@ -3096,10 +3093,7 @@ void commit_params(dt_iop_module_t *self, float x = p->x; float y = p->y; dt_aligned_pixel_t custom_wb; - if(p->illuminant == DT_ILLUMINANT_CAMERA) - copy_pixel(custom_wb, self->dev->chroma.wb_coeffs); - else - _get_white_balance_coeff(self, custom_wb); + _get_white_balance_coeff(self, custom_wb); illuminant_to_xy(p->illuminant, &(self->dev->image_storage), custom_wb, &x, &y, p->temperature, p->illum_fluo, p->illum_led); @@ -3544,10 +3538,7 @@ static gboolean _illuminant_color_draw(GtkWidget *widget, float y = p->y; dt_aligned_pixel_t RGB = { 0 }; dt_aligned_pixel_t custom_wb; - if(p->illuminant == DT_ILLUMINANT_CAMERA) - copy_pixel(custom_wb, self->dev->chroma.wb_coeffs); - else - _get_white_balance_coeff(self, custom_wb); + _get_white_balance_coeff(self, custom_wb); illuminant_to_xy(p->illuminant, &(self->dev->image_storage), custom_wb, &x, &y, p->temperature, p->illum_fluo, p->illum_led); illuminant_xy_to_RGB(x, y, RGB); @@ -3650,10 +3641,7 @@ static void _update_approx_cct(const dt_iop_module_t *self) float x = p->x; float y = p->y; dt_aligned_pixel_t custom_wb; - if(p->illuminant == DT_ILLUMINANT_CAMERA) - copy_pixel(custom_wb, self->dev->chroma.wb_coeffs); - else - _get_white_balance_coeff(self, custom_wb); + _get_white_balance_coeff(self, custom_wb); illuminant_to_xy(p->illuminant, &(self->dev->image_storage), custom_wb, &x, &y, p->temperature, p->illum_fluo, p->illum_led); @@ -3890,21 +3878,7 @@ void reload_defaults(dt_iop_module_t *self) d->adaptation = DT_ADAPTATION_CAT16; dt_aligned_pixel_t custom_wb; - gboolean wb_retrieved = FALSE; - - // In modern mode, we want the actual coefficients (User/AsShot) to determine the CCT. - // In legacy mode, we rely on the helper which calculates a D65 ratio. - if (self->dev->chroma.late_correction && dt_image_is_matrix_correction_supported(img)) - { - copy_pixel(custom_wb, self->dev->chroma.wb_coeffs); - wb_retrieved = TRUE; - } - else - { - wb_retrieved = !_get_white_balance_coeff(self, custom_wb); - } - - if(wb_retrieved) + if(!_get_white_balance_coeff(self, custom_wb)) { if(find_temperature_from_raw_coeffs(img, custom_wb, &(d->x), &(d->y))) d->illuminant = DT_ILLUMINANT_CAMERA; From 9d4825482be706e455080b0711a1d83f37ab6b36 Mon Sep 17 00:00:00 2001 From: Kofa Date: Wed, 17 Dec 2025 10:50:12 +0100 Subject: [PATCH 04/27] wb: Fixed publishing of late_correction in temperature.c --- src/iop/temperature.c | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/src/iop/temperature.c b/src/iop/temperature.c index b85a8989450..15659ba912e 100644 --- a/src/iop/temperature.c +++ b/src/iop/temperature.c @@ -103,6 +103,7 @@ typedef struct dt_iop_temperature_data_t { float coeffs[4]; int preset; + gboolean late_correction; } dt_iop_temperature_data_t; typedef struct dt_iop_temperature_global_data_t @@ -553,7 +554,7 @@ static inline void _publish_chroma(dt_dev_pixelpipe_iop_t *piece) d->coeffs[k] * piece->pipe->dsc.processed_maximum[k]; chr->wb_coeffs[k] = d->coeffs[k]; } - chr->late_correction = (d->preset == DT_IOP_TEMP_D65_LATE); + chr->late_correction = d->late_correction; } void process(dt_iop_module_t *self, @@ -744,6 +745,20 @@ void commit_params(dt_iop_module_t *self, d->preset = p->preset; + gboolean effective_late_correction = FALSE; + + if (p->preset == DT_IOP_TEMP_D65_LATE) + effective_late_correction = TRUE; + else if (p->preset == DT_IOP_TEMP_D65) + effective_late_correction = FALSE; + else + effective_late_correction = p->late_correction; + + d->late_correction = effective_late_correction; + + chr->late_correction = effective_late_correction; + + chr->temperature = piece->enabled ? self : NULL; /* Make sure the chroma information stuff is valid If piece is disabled we always clear the trouble message and make sure chroma does know there is no temperature module. From efa8df72bd8575b801ff980b396c810871987d1e Mon Sep 17 00:00:00 2001 From: Kofa Date: Wed, 17 Dec 2025 10:53:06 +0100 Subject: [PATCH 05/27] wb: added DT_ILLUMINANT_FROM_WB, separated find_temperature_from_raw_coeffs into find_temperature_from_wb_coeffs, find_temperature_from_as_shot_coeffs; added handling to channelmixerrgb.c + altered logic for 'late correction' check --- src/common/illuminants.h | 108 ++++++++++------ src/develop/develop.h | 6 +- src/iop/channelmixerrgb.c | 252 +++++++++++++++++++++++++++++--------- src/iop/temperature.c | 25 ++-- 4 files changed, 273 insertions(+), 118 deletions(-) diff --git a/src/common/illuminants.h b/src/common/illuminants.h index 5f9530677c8..32f593d66a5 100644 --- a/src/common/illuminants.h +++ b/src/common/illuminants.h @@ -34,6 +34,7 @@ typedef enum dt_illuminant_t DT_ILLUMINANT_BB = 6, // $DESCRIPTION: "Planckian (black body)" general black body radiator - not CIE standard DT_ILLUMINANT_CUSTOM = 7, // $DESCRIPTION: "custom" input x and y directly - bypass search DT_ILLUMINANT_CAMERA = 10,// $DESCRIPTION: "as shot in camera" read RAW EXIF for WB + DT_ILLUMINANT_FROM_WB = 11,// $DESCRIPTION: "as set in white balance module" read coefficients from the white balance module DT_ILLUMINANT_LAST, DT_ILLUMINANT_DETECT_SURFACES = 8, DT_ILLUMINANT_DETECT_EDGES = 9, @@ -215,19 +216,22 @@ static inline void illuminant_CCT_to_RGB(const float t, dt_aligned_pixel_t RGB) illuminant_xy_to_RGB(x, y, RGB); } +static inline gboolean find_temperature_from_wb_coeffs(const dt_image_t *img, const dt_aligned_pixel_t wb_coeffs, + float *chroma_x, float *chroma_y); // Fetch image from pipeline and read EXIF for camera RAW WB coeffs -static inline gboolean find_temperature_from_raw_coeffs(const dt_image_t *img, const dt_aligned_pixel_t custom_wb, +static inline gboolean find_temperature_from_as_shot_coeffs(const dt_image_t *img, const dt_aligned_pixel_t correction_ratios, float *chroma_x, float *chroma_y); -static inline int illuminant_to_xy(const dt_illuminant_t illuminant, // primary type of illuminant - const dt_image_t *img, // image container - const dt_aligned_pixel_t custom_wb, // optional user-set WB coeffs - float *x_out, float *y_out, // chromaticity output - const float t, // temperature in K, if needed - const dt_illuminant_fluo_t fluo, // sub-type of fluorescent illuminant, if needed - const dt_illuminant_led_t iled) // sub-type of led illuminant, if needed +static inline int illuminant_to_xy(const dt_illuminant_t illuminant, // primary type of illuminant + const dt_image_t *img, // image container + const dt_aligned_pixel_t correction_ratios, // optional D65 correction ratios derived from user-set coefficients + const dt_aligned_pixel_t wb_coeffs, // optional user-set WB coeffs (absolute) + float *x_out, float *y_out, // chromaticity output + const float t, // temperature in K, if needed + const dt_illuminant_fluo_t fluo, // sub-type of fluorescent illuminant, if needed + const dt_illuminant_led_t iled) // sub-type of led illuminant, if needed { /** * Compute the x and y chromaticity coordinates in Yxy spaces for standard illuminants @@ -298,9 +302,15 @@ static inline int illuminant_to_xy(const dt_illuminant_t illuminant, // primary } case DT_ILLUMINANT_CAMERA: { - // Detect WB from RAW EXIF + // Detect WB from RAW EXIF, correcting with D65/wb_coeff ratios if(img) - if(find_temperature_from_raw_coeffs(img, custom_wb, &x, &y)) break; + if(find_temperature_from_as_shot_coeffs(img, correction_ratios, &x, &y)) break; + } + case DT_ILLUMINANT_FROM_WB: + { + // Detect WB from user-provided coefficients + if(img) + if(find_temperature_from_wb_coeffs(img, wb_coeffs, &x, &y)) break; } case DT_ILLUMINANT_CUSTOM: // leave x and y as-is case DT_ILLUMINANT_DETECT_EDGES: @@ -385,31 +395,11 @@ static inline void matrice_pseudoinverse(float (*in)[3], float (*out)[3], int si } } - -static gboolean find_temperature_from_raw_coeffs(const dt_image_t *img, const dt_aligned_pixel_t custom_wb, - float *chroma_x, float *chroma_y) +// returns TRUE is OK, FALSE if failed +static gboolean get_CAM_to_XYZ(const dt_image_t * img, float(* CAM_to_XYZ)[3]) { - if(img == NULL) return FALSE; - if(!dt_image_is_matrix_correction_supported(img)) return FALSE; - - gboolean has_valid_coeffs = TRUE; - const int num_coeffs = (img->flags & DT_IMAGE_4BAYER) ? 4 : 3; - - // Check coeffs - for(int k = 0; has_valid_coeffs && k < num_coeffs; k++) - if(!dt_isnormal(img->wb_coeffs[k]) || img->wb_coeffs[k] == 0.0f) has_valid_coeffs = FALSE; - - if(!has_valid_coeffs) return FALSE; - - // Get white balance camera factors - dt_aligned_pixel_t WB = { img->wb_coeffs[0], img->wb_coeffs[1], img->wb_coeffs[2], img->wb_coeffs[3] }; - - // Adapt the camera coeffs with custom white balance if provided - // this can deal with WB coeffs that don't use the input matrix reference - if(custom_wb) - for(size_t k = 0; k < 4; k++) WB[k] *= custom_wb[k]; - - // Get the camera input profile (matrice of primaries) + if(img == NULL || CAM_to_XYZ == NULL) return FALSE; + // Get the camera input profile (matrice of primaries) - embedded or raw library DB float XYZ_to_CAM[4][3]; dt_mark_colormatrix_invalid(&XYZ_to_CAM[0][0]); @@ -438,21 +428,61 @@ static gboolean find_temperature_from_raw_coeffs(const dt_image_t *img, const dt if(!dt_is_valid_colormatrix(XYZ_to_CAM[0][0])) return FALSE; - // Bloody input matrices define XYZ -> CAM transform, as if we often needed camera profiles to output - // So we need to invert them. Here go your CPU cycles again. - float CAM_to_XYZ[4][3]; dt_mark_colormatrix_invalid(&CAM_to_XYZ[0][0]); matrice_pseudoinverse(XYZ_to_CAM, CAM_to_XYZ, 3); - if(!dt_is_valid_colormatrix(CAM_to_XYZ[0][0])) return FALSE; + return dt_is_valid_colormatrix(CAM_to_XYZ[0][0]); +} + +// returns FALSE if failed; TRUE if successful +static gboolean find_temperature_from_wb_coeffs(const dt_image_t *img, const dt_aligned_pixel_t wb_coeffs, + float *chroma_x, float *chroma_y) +{ + if(img == NULL || wb_coeffs == NULL) return FALSE; + if(!dt_image_is_matrix_correction_supported(img)) return FALSE; + + float CAM_to_XYZ[4][3]; + if(!get_CAM_to_XYZ(img, CAM_to_XYZ)) { + return FALSE; + } float x, y; - WB_coeffs_to_illuminant_xy(CAM_to_XYZ, WB, &x, &y); + WB_coeffs_to_illuminant_xy(CAM_to_XYZ, wb_coeffs, &x, &y); *chroma_x = x; *chroma_y = y; return TRUE; } +// returns FALSE if failed; TRUE if successful +static gboolean find_temperature_from_as_shot_coeffs(const dt_image_t *img, const dt_aligned_pixel_t correction_ratios, + float *chroma_x, float *chroma_y) +{ + if(img == NULL) return FALSE; + + gboolean has_valid_coeffs = TRUE; + const int num_coeffs = (img->flags & DT_IMAGE_4BAYER) ? 4 : 3; + + // Check coeffs + for(int k = 0; has_valid_coeffs && k < num_coeffs; k++) + if(!dt_isnormal(img->wb_coeffs[k]) || img->wb_coeffs[k] == 0.0f) has_valid_coeffs = FALSE; + + if(!has_valid_coeffs) return FALSE; + + // Get as-shot white balance camera factors (from raw) + // component wise raw-RGB * wb_coeffs should provide R=G=B for a neutral patch under + // the scene illuminant + dt_aligned_pixel_t WB = { img->wb_coeffs[0], img->wb_coeffs[1], img->wb_coeffs[2], img->wb_coeffs[3] }; + + // Adapt the camera coeffs with custom D65 coefficients if provided ('caveats' workaround) + // this can deal with WB coeffs that don't use the input matrix reference + // adaptation_ratios[k] = chr->D65coeffs[k] / chr->wb_coeffs[k] + if(correction_ratios) + for(size_t k = 0; k < 4; k++) WB[k] *= correction_ratios[k]; + // for a neutral surface, raw RGB * img->wb_coeffs would produce neutral R=G=B + + return find_temperature_from_wb_coeffs(img, WB, chroma_x, chroma_y); +} + DT_OMP_DECLARE_SIMD() static inline float planckian_normal(const float x, const float t) diff --git a/src/develop/develop.h b/src/develop/develop.h index bb4bcfc63f8..5260094a983 100644 --- a/src/develop/develop.h +++ b/src/develop/develop.h @@ -167,9 +167,9 @@ typedef struct dt_dev_viewport_t - the currently used wb_coeffs in temperature module - D65coeffs and as_shot are read from exif data f) - late_correction set by temperature if we want to process data as following - If we use the new DT_IOP_TEMP_D65_LATE mode in temperature.c and don#t have - any temp parameters changes later we can calc correction coeffs to modify - as_shot rgb data to D65 + If we use the new DT_IOP_TEMP_D65_LATE mode or enable this in user modes of temperature.c + and don't have any temp parameters changes later we can calc correction ratios + to take white-balanced (neutralized) data to D65 */ typedef struct dt_dev_chroma_t { diff --git a/src/iop/channelmixerrgb.c b/src/iop/channelmixerrgb.c index 6ad8118e727..62fd8b47d2a 100644 --- a/src/iop/channelmixerrgb.c +++ b/src/iop/channelmixerrgb.c @@ -423,7 +423,7 @@ void init_presets(dt_iop_module_so_t *self) p.illum_fluo = DT_ILLUMINANT_FLUO_F3; p.illum_led = DT_ILLUMINANT_LED_B5; p.temperature = 5003.f; - illuminant_to_xy(DT_ILLUMINANT_PIPE, NULL, NULL, &p.x, &p.y, p.temperature, + illuminant_to_xy(DT_ILLUMINANT_PIPE, NULL, NULL, NULL, &p.x, &p.y, p.temperature, DT_ILLUMINANT_FLUO_LAST, DT_ILLUMINANT_LED_LAST); p.red[0] = 1.f; @@ -594,9 +594,7 @@ void init_presets(dt_iop_module_so_t *self) static gboolean _dev_is_D65_chroma(const dt_develop_t *dev) { const dt_dev_chroma_t *chr = &dev->chroma; - return chr->late_correction - ? dt_dev_equal_chroma(chr->wb_coeffs, chr->as_shot) - : dt_dev_equal_chroma(chr->wb_coeffs, chr->D65coeffs); + return chr->late_correction || dt_dev_equal_chroma(chr->wb_coeffs, chr->D65coeffs); } static gboolean _area_mapping_active(const dt_iop_channelmixer_rgb_gui_data_t *g) @@ -612,14 +610,14 @@ static const char *_area_mapping_section_text(const dt_iop_channelmixer_rgb_gui_ return _area_mapping_active(g) ? _("area color mapping (active)") : _("area color mapping"); } -static gboolean _get_white_balance_coeff(const dt_iop_module_t *self, - dt_aligned_pixel_t custom_wb) +static gboolean _get_d65_correction_ratios(const dt_iop_module_t *self, + dt_aligned_pixel_t out_correction_ratios) { const dt_dev_chroma_t *chr = &self->dev->chroma; // Init output with a no-op for_four_channels(k) - custom_wb[k] = 1.0f; + out_correction_ratios[k] = 1.0f; if(!dt_image_is_matrix_correction_supported(&self->dev->image_storage)) return TRUE; @@ -639,7 +637,7 @@ static gboolean _get_white_balance_coeff(const dt_iop_module_t *self, if(valid_chroma && changed_chroma) { for_four_channels(k) - custom_wb[k] = (float)chr->D65coeffs[k] / chr->wb_coeffs[k]; + out_correction_ratios[k] = (float)chr->D65coeffs[k] / chr->wb_coeffs[k]; } return FALSE; } @@ -1251,7 +1249,7 @@ static void _check_if_close_to_daylight(const float x, float uv_test[2]; // Compute the test chromaticity from the daylight model - illuminant_to_xy(DT_ILLUMINANT_D, NULL, NULL, &xy_test[0], &xy_test[1], t, + illuminant_to_xy(DT_ILLUMINANT_D, NULL, NULL, NULL, &xy_test[0], &xy_test[1], t, DT_ILLUMINANT_FLUO_LAST, DT_ILLUMINANT_LED_LAST); xy_to_uv(xy_test, uv_test); @@ -1260,7 +1258,7 @@ static void _check_if_close_to_daylight(const float x, const float delta_daylight = dt_fast_hypotf(uv_test[0] - uv_ref[0], uv_test[1] - uv_ref[1]); // Compute the test chromaticity from the blackbody model - illuminant_to_xy(DT_ILLUMINANT_BB, NULL, NULL, &xy_test[0], &xy_test[1], t, + illuminant_to_xy(DT_ILLUMINANT_BB, NULL, NULL, NULL, &xy_test[0], &xy_test[1], t, DT_ILLUMINANT_FLUO_LAST, DT_ILLUMINANT_LED_LAST); xy_to_uv(xy_test, uv_test); @@ -2194,10 +2192,25 @@ void process(dt_iop_module_t *self, // change here, so we can't update the defaults. So we need to // re-run the detection at runtime… float x, y; - dt_aligned_pixel_t custom_wb; - _get_white_balance_coeff(self, custom_wb); + dt_aligned_pixel_t correction_ratios; + _get_d65_correction_ratios(self, correction_ratios); - if(find_temperature_from_raw_coeffs(&(self->dev->image_storage), custom_wb, &(x), &(y))) + if(find_temperature_from_as_shot_coeffs(&(self->dev->image_storage), correction_ratios, &(x), &(y))) + { + // Convert illuminant from xyY to XYZ + dt_aligned_pixel_t XYZ; + illuminant_xy_to_XYZ(x, y, XYZ); + + // Convert illuminant from XYZ to Bradford modified LMS + convert_any_XYZ_to_LMS(XYZ, data->illuminant, data->adaptation); + data->illuminant[3] = 0.f; + } + } + else if(data->illuminant_type == DT_ILLUMINANT_FROM_WB) + { + float x, y; + + if(find_temperature_from_wb_coeffs(&(self->dev->image_storage), self->dev->chroma.wb_coeffs, &(x), &(y))) { // Convert illuminant from xyY to XYZ dt_aligned_pixel_t XYZ; @@ -2305,10 +2318,33 @@ int process_cl(dt_iop_module_t *self, // change here, so we can't update the defaults. So we need to // re-run the detection at runtime… float x, y; - dt_aligned_pixel_t custom_wb; - _get_white_balance_coeff(self, custom_wb); + dt_aligned_pixel_t correction_ratios; + _get_d65_correction_ratios(self, correction_ratios); - if(find_temperature_from_raw_coeffs(&(self->dev->image_storage), custom_wb, &(x), &(y))) + if(find_temperature_from_as_shot_coeffs(&(self->dev->image_storage), correction_ratios, &(x), &(y))) + { + // Convert illuminant from xyY to XYZ + dt_aligned_pixel_t XYZ; + illuminant_xy_to_XYZ(x, y, XYZ); + + // Convert illuminant from XYZ to Bradford modified LMS + convert_any_XYZ_to_LMS(XYZ, d->illuminant, d->adaptation); + d->illuminant[3] = 0.f; + } + } + else if(d->illuminant_type == DT_ILLUMINANT_FROM_WB) + { + // The camera illuminant is a behaviour rather than a preset of + // values: it uses whatever is in the RAW EXIF. But it depends on + // what temperature.c is doing and needs to be updated + // accordingly, to give a consistent result. We initialise the + // CAT defaults using the temperature coeffs at startup, but if + // temperature is changed later, we get no notification of the + // change here, so we can't update the defaults. So we need to + // re-run the detection at runtime… + float x, y; + + if(find_temperature_from_wb_coeffs(&(self->dev->image_storage), self->dev->chroma.wb_coeffs, &(x), &(y))) { // Convert illuminant from xyY to XYZ dt_aligned_pixel_t XYZ; @@ -3022,6 +3058,38 @@ static void _preview_pipe_finished_callback(gpointer instance, dt_iop_module_t * dt_iop_gui_enter_critical_section(self); gtk_label_set_markup(GTK_LABEL(g->label_delta_E), g->delta_E_label_text); + + dt_iop_channelmixer_rgb_params_t *p = self->params; + if(p->illuminant == DT_ILLUMINANT_FROM_WB) + { + // The WB coefficients might have changed in the temperature module. + // We need to re-calculate the chromaticity (x, y) to update the GUI feedback. + float x = p->x; + float y = p->y; + + if(find_temperature_from_wb_coeffs(&(self->dev->image_storage), self->dev->chroma.wb_coeffs, &x, &y)) + { + p->x = x; + p->y = y; + + darktable.gui->reset++; + // Update derived GUI widgets (CCT label, color patch) + _update_approx_cct(self); + _update_illuminant_color(self); + + // Update Hue/Chroma sliders to match the new coordinates + dt_aligned_pixel_t xyY = { p->x, p->y, 1.f }; + dt_aligned_pixel_t Lch; + dt_xyY_to_Lch(xyY, Lch); + + if(Lch[1] > 0) + dt_bauhaus_slider_set(g->illum_x, rad2degf(Lch[2])); + dt_bauhaus_slider_set(g->illum_y, Lch[1]); + + darktable.gui->reset--; + } + } + dt_iop_gui_leave_critical_section(self); _set_trouble_messages(self); } @@ -3092,16 +3160,22 @@ void commit_params(dt_iop_module_t *self, // find x y coordinates of illuminant for CIE 1931 2° observer float x = p->x; float y = p->y; - dt_aligned_pixel_t custom_wb; - _get_white_balance_coeff(self, custom_wb); - illuminant_to_xy(p->illuminant, &(self->dev->image_storage), - custom_wb, &x, &y, p->temperature, p->illum_fluo, p->illum_led); + dt_aligned_pixel_t correction_ratios; + _get_d65_correction_ratios(self, correction_ratios); + dt_aligned_pixel_t wb_coeffs = {0.f}; + if(p->illuminant == DT_ILLUMINANT_FROM_WB) + { + for(int k = 0; k < 4; k++) + wb_coeffs[k] = self->dev->chroma.wb_coeffs[k]; + } + illuminant_to_xy(p->illuminant, &(self->dev->image_storage), correction_ratios, + wb_coeffs, &x, &y, p->temperature, p->illum_fluo, + p->illum_led); - // if illuminant is set as camera, x and y are set on-the-fly at + // if illuminant is from camera or user WB coefficients, x and y are set on-the-fly at // commit time, so we need to set adaptation too - if(p->illuminant == DT_ILLUMINANT_CAMERA) - _check_if_close_to_daylight(x, y, NULL, NULL, &(d->adaptation)); - + if(p->illuminant == DT_ILLUMINANT_CAMERA || p->illuminant == DT_ILLUMINANT_FROM_WB) + _check_if_close_to_daylight(x, y, NULL, NULL, &(d->adaptation)); d->illuminant_type = p->illuminant; // Convert illuminant from xyY to XYZ @@ -3244,6 +3318,7 @@ static void _update_illuminants(const dt_iop_module_t *self) break; } case DT_ILLUMINANT_CAMERA: + case DT_ILLUMINANT_FROM_WB: { gtk_widget_set_visible(g->adaptation, TRUE); gtk_widget_set_visible(g->temperature, FALSE); @@ -3536,11 +3611,18 @@ static gboolean _illuminant_color_draw(GtkWidget *widget, // camera RAW is chosen float x = p->x; float y = p->y; - dt_aligned_pixel_t RGB = { 0 }; - dt_aligned_pixel_t custom_wb; - _get_white_balance_coeff(self, custom_wb); - illuminant_to_xy(p->illuminant, &(self->dev->image_storage), custom_wb, - &x, &y, p->temperature, p->illum_fluo, p->illum_led); + dt_aligned_pixel_t RGB = { 0.f }; + dt_aligned_pixel_t correction_ratios; + _get_d65_correction_ratios(self, correction_ratios); + dt_aligned_pixel_t wb_coeffs = { 0.f }; + if(p->illuminant == DT_ILLUMINANT_FROM_WB) + { + for(int k = 0; k < 4; k++) + wb_coeffs[k] = self->dev->chroma.wb_coeffs[k]; + } + illuminant_to_xy(p->illuminant, &(self->dev->image_storage), correction_ratios, + wb_coeffs, &x, &y, p->temperature, p->illum_fluo, + p->illum_led); illuminant_xy_to_RGB(x, y, RGB); cairo_set_source_rgb(cr, RGB[0], RGB[1], RGB[2]); cairo_rectangle(cr, INNER_PADDING, margin, width, height); @@ -3640,10 +3722,17 @@ static void _update_approx_cct(const dt_iop_module_t *self) float x = p->x; float y = p->y; - dt_aligned_pixel_t custom_wb; - _get_white_balance_coeff(self, custom_wb); - illuminant_to_xy(p->illuminant, &(self->dev->image_storage), - custom_wb, &x, &y, p->temperature, p->illum_fluo, p->illum_led); + dt_aligned_pixel_t correction_ratios; + _get_d65_correction_ratios(self, correction_ratios); + dt_aligned_pixel_t wb_coeffs = { 0.f }; + if(p->illuminant == DT_ILLUMINANT_FROM_WB) + { + for(int k = 0; k < 4; k++) + wb_coeffs[k] = self->dev->chroma.wb_coeffs[k]; + } + illuminant_to_xy(p->illuminant, &(self->dev->image_storage), correction_ratios, + wb_coeffs, &x, &y, p->temperature, p->illum_fluo, + p->illum_led); dt_illuminant_t test_illuminant; float t = 5000.f; @@ -3841,6 +3930,24 @@ void init(dt_iop_module_t *self) d->red[0] = d->green[1] = d->blue[2] = 1.0; } +static void _add_or_remove_illuminant(const dt_iop_module_t *self, + const int illuminant, + const gboolean use_illuminant) +{ + // we only call this if g is not NULL + dt_iop_channelmixer_rgb_gui_data_t *g = self->gui_data; + const int pos = dt_bauhaus_combobox_get_from_value(g->illuminant, illuminant); + if(use_illuminant) + { + if(pos == -1) + dt_bauhaus_combobox_add_introspection(g->illuminant, NULL, + self->so->get_f("illuminant")->Enum.values, + illuminant, illuminant); + } + else if (pos != -1) + dt_bauhaus_combobox_remove_at(g->illuminant, pos); +} + void reload_defaults(dt_iop_module_t *self) { dt_iop_channelmixer_rgb_params_t *d = self->default_params; @@ -3877,10 +3984,10 @@ void reload_defaults(dt_iop_module_t *self) { d->adaptation = DT_ADAPTATION_CAT16; - dt_aligned_pixel_t custom_wb; - if(!_get_white_balance_coeff(self, custom_wb)) + dt_aligned_pixel_t correction_ratios; + if(!_get_d65_correction_ratios(self, correction_ratios)) { - if(find_temperature_from_raw_coeffs(img, custom_wb, &(d->x), &(d->y))) + if(find_temperature_from_as_shot_coeffs(img, correction_ratios, &(d->x), &(d->y))) d->illuminant = DT_ILLUMINANT_CAMERA; _check_if_close_to_daylight(d->x, d->y, &(d->temperature), &(d->illuminant), &(d->adaptation)); @@ -3908,16 +4015,9 @@ void reload_defaults(dt_iop_module_t *self) g->delta_E_label_text = NULL; } - const int pos = dt_bauhaus_combobox_get_from_value(g->illuminant, DT_ILLUMINANT_CAMERA); - if(dt_image_is_matrix_correction_supported(img) && !dt_image_is_monochrome(img)) - { - if(pos == -1) - dt_bauhaus_combobox_add_introspection(g->illuminant, NULL, - self->so->get_f("illuminant")->Enum.values, - DT_ILLUMINANT_CAMERA, DT_ILLUMINANT_CAMERA); - } - else - dt_bauhaus_combobox_remove_at(g->illuminant, pos); + const gboolean has_color_matrix = dt_image_is_matrix_correction_supported(img) && !dt_image_is_monochrome(img); + _add_or_remove_illuminant(self, DT_ILLUMINANT_CAMERA, has_color_matrix); + _add_or_remove_illuminant(self, DT_ILLUMINANT_FROM_WB, has_color_matrix); gui_changed(self, NULL, NULL); } @@ -3980,9 +4080,22 @@ void gui_changed(dt_iop_module_t *self, // illuminant = "as set in camera", temperature and // chromaticity are inited with the preset content when // illuminant is changed. - dt_aligned_pixel_t custom_wb; - _get_white_balance_coeff(self, custom_wb); - find_temperature_from_raw_coeffs(&(self->dev->image_storage), custom_wb, + dt_aligned_pixel_t correction_ratios; + _get_d65_correction_ratios(self, correction_ratios); + find_temperature_from_as_shot_coeffs(&(self->dev->image_storage), correction_ratios, + &(p->x), &(p->y)); + _check_if_close_to_daylight(p->x, p->y, &(p->temperature), NULL, &(p->adaptation)); + } + if(*prev_illuminant == DT_ILLUMINANT_FROM_WB) + { + // If illuminant was previously set with "as set in camera", + // when changing it, we need to ensure the temperature and + // chromaticity are inited with the correct values taken from + // camera EXIF. Otherwise, if using a preset defining + // illuminant = "as set in camera", temperature and + // chromaticity are inited with the preset content when + // illuminant is changed. + find_temperature_from_wb_coeffs(&(self->dev->image_storage), self->dev->chroma.wb_coeffs, &(p->x), &(p->y)); _check_if_close_to_daylight(p->x, p->y, &(p->temperature), NULL, &(p->adaptation)); } @@ -3998,14 +4111,23 @@ void gui_changed(dt_iop_module_t *self, { // Get camera WB and update illuminant dt_aligned_pixel_t custom_wb; - _get_white_balance_coeff(self, custom_wb); - const gboolean found = find_temperature_from_raw_coeffs(&(self->dev->image_storage), + _get_d65_correction_ratios(self, custom_wb); + const gboolean found = find_temperature_from_as_shot_coeffs(&(self->dev->image_storage), custom_wb, &(p->x), &(p->y)); _check_if_close_to_daylight(p->x, p->y, &(p->temperature), NULL, &(p->adaptation)); if(found) dt_control_log(_("white balance successfully extracted from raw image")); } + if(p->illuminant == DT_ILLUMINANT_FROM_WB) + { + const gboolean found = find_temperature_from_wb_coeffs(&(self->dev->image_storage), + self->dev->chroma.wb_coeffs, &(p->x), &(p->y)); + _check_if_close_to_daylight(p->x, p->y, &(p->temperature), NULL, &(p->adaptation)); + + if(found) + dt_control_log(_("white balance successfully extracted from white balance module")); + } #ifdef AI_ACTIVATED else if(p->illuminant == DT_ILLUMINANT_DETECT_EDGES || p->illuminant == DT_ILLUMINANT_DETECT_SURFACES) @@ -4033,17 +4155,19 @@ void gui_changed(dt_iop_module_t *self, // illuminant to allow swapping modes if(p->illuminant != DT_ILLUMINANT_CUSTOM - && p->illuminant != DT_ILLUMINANT_CAMERA) + && p->illuminant != DT_ILLUMINANT_CAMERA + && p->illuminant != DT_ILLUMINANT_FROM_WB) { // We are in any mode defining (x, y) indirectly from an // interface, so commit (x, y) explicitly - illuminant_to_xy(p->illuminant, NULL, NULL, &(p->x), &(p->y), + illuminant_to_xy(p->illuminant, NULL, NULL, NULL, &(p->x), &(p->y), p->temperature, p->illum_fluo, p->illum_led); } if(p->illuminant != DT_ILLUMINANT_D && p->illuminant != DT_ILLUMINANT_BB - && p->illuminant != DT_ILLUMINANT_CAMERA) + && p->illuminant != DT_ILLUMINANT_CAMERA + && p->illuminant != DT_ILLUMINANT_FROM_WB) { // We are in any mode not defining explicitly a temperature, so // find the the closest CCT and commit it @@ -4128,7 +4252,7 @@ void gui_changed(dt_iop_module_t *self, // If "as shot in camera" illuminant is used, CAT space is forced automatically // therefore, make the control insensitive dt_bauhaus_combobox_set_from_value(g->adaptation, p->adaptation); - gtk_widget_set_sensitive(g->adaptation, p->illuminant != DT_ILLUMINANT_CAMERA); + gtk_widget_set_sensitive(g->adaptation, p->illuminant != DT_ILLUMINANT_CAMERA && p->illuminant != DT_ILLUMINANT_FROM_WB); _declare_cat_on_pipe(self, FALSE); @@ -4204,15 +4328,23 @@ static void _auto_set_illuminant(dt_iop_module_t *self, // find x y coordinates of illuminant for CIE 1931 2° observer float x = p->x; float y = p->y; + dt_aligned_pixel_t correction_ratios; + _get_d65_correction_ratios(self, correction_ratios); + dt_aligned_pixel_t wb_coeffs = { 0.f }; + if(p->illuminant == DT_ILLUMINANT_FROM_WB) + { + for(int k = 0; k < 4; k++) + wb_coeffs[k] = self->dev->chroma.wb_coeffs[k]; + } + illuminant_to_xy(p->illuminant, &(self->dev->image_storage), correction_ratios, + wb_coeffs, &x, &y, p->temperature, p->illum_fluo, + p->illum_led); + dt_adaptation_t adaptation = p->adaptation; - dt_aligned_pixel_t custom_wb; - _get_white_balance_coeff(self, custom_wb); - illuminant_to_xy(p->illuminant, &(self->dev->image_storage), - custom_wb, &x, &y, p->temperature, p->illum_fluo, p->illum_led); // if illuminant is set as camera, x and y are set on-the-fly at // commit time, so we need to set adaptation too - if(p->illuminant == DT_ILLUMINANT_CAMERA) + if(p->illuminant == DT_ILLUMINANT_CAMERA || p->illuminant == DT_ILLUMINANT_FROM_WB) _check_if_close_to_daylight(x, y, NULL, NULL, &adaptation); // Convert illuminant from xyY to XYZ diff --git a/src/iop/temperature.c b/src/iop/temperature.c index 15659ba912e..bc3a0b8f397 100644 --- a/src/iop/temperature.c +++ b/src/iop/temperature.c @@ -196,11 +196,7 @@ int legacy_params(dt_iop_module_t *self, const dt_iop_temperature_params_v4_t *o = (dt_iop_temperature_params_v4_t *)old_params; dt_iop_temperature_params_v5_t *n = malloc(sizeof(dt_iop_temperature_params_v5_t)); - n->red = o->red; - n->green = o->green; - n->blue = o->blue; - n->various = NAN; - n->preset = DT_IOP_TEMP_UNKNOWN; + memcpy(n, o, sizeof(dt_iop_temperature_params_v4_t)); n->late_correction = FALSE; *new_params = n; *new_params_size = sizeof(dt_iop_temperature_params_v5_t); @@ -747,15 +743,14 @@ void commit_params(dt_iop_module_t *self, gboolean effective_late_correction = FALSE; - if (p->preset == DT_IOP_TEMP_D65_LATE) + if(p->preset == DT_IOP_TEMP_D65_LATE) effective_late_correction = TRUE; - else if (p->preset == DT_IOP_TEMP_D65) + else if(p->preset == DT_IOP_TEMP_D65) effective_late_correction = FALSE; else effective_late_correction = p->late_correction; d->late_correction = effective_late_correction; - chr->late_correction = effective_late_correction; chr->temperature = piece->enabled ? self : NULL; @@ -1203,7 +1198,7 @@ static void _update_preset(dt_iop_module_t *self, int mode) const gboolean is_new_mode_manual = (mode != DT_IOP_TEMP_D65_LATE) && (mode != DT_IOP_TEMP_D65); - if (is_current_reference && is_new_mode_manual) + if(is_current_reference && is_new_mode_manual) { // set iff color calibration active in adaptation mode p->late_correction = (chr->adaptation != NULL); @@ -1211,11 +1206,11 @@ static void _update_preset(dt_iop_module_t *self, int mode) p->preset = mode; - if (mode == DT_IOP_TEMP_D65_LATE) + if(mode == DT_IOP_TEMP_D65_LATE) { chr->late_correction = TRUE; } - else if (mode == DT_IOP_TEMP_D65) + else if(mode == DT_IOP_TEMP_D65) { chr->late_correction = FALSE; } @@ -1225,10 +1220,9 @@ static void _update_preset(dt_iop_module_t *self, int mode) chr->late_correction = p->late_correction; } - if (g && g->check_late_correction) + if(g && g->check_late_correction) { - // Hide checkbox in reference modes (logic is enforced hardcoded) - // Show in manual modes (user has control) + // show the checkbox only in modes where user can adjust multipliers gtk_widget_set_visible(g->check_late_correction, is_new_mode_manual); gtk_toggle_button_set_active(GTK_TOGGLE_BUTTON(g->check_late_correction), @@ -2234,8 +2228,7 @@ void gui_init(dt_iop_module_t *self) _("color tint of the image, from magenta (value < 1) to green (value > 1)")); g->check_late_correction = dt_bauhaus_toggle_from_params(self, "late_correction"); - - g_object_ref(g->check_late_correction); // Prevent destruction + g_object_ref(g->check_late_correction); // prevent destruction GtkWidget *temp_parent = gtk_widget_get_parent(g->check_late_correction); if(temp_parent) gtk_container_remove(GTK_CONTAINER(temp_parent), g->check_late_correction); From 9f02277f216585c97849f43f483a0bac87ab44f9 Mon Sep 17 00:00:00 2001 From: Kofa Date: Sat, 28 Feb 2026 14:23:02 +0100 Subject: [PATCH 06/27] wb: fix switch-case fall-through in illuminants.h#illuminant_to_xy; validate wb_coeffs for find_temperature_from_wb_coeffs as well as in find_temperature_from_as_shot_coeffs; fix copy-pasted comment in channelmixerrgb.c#gui_changed --- src/common/illuminants.h | 36 +++++++++++++++++++++++------------- src/iop/channelmixerrgb.c | 17 +++++++---------- 2 files changed, 30 insertions(+), 23 deletions(-) diff --git a/src/common/illuminants.h b/src/common/illuminants.h index 32f593d66a5..8300b2c7110 100644 --- a/src/common/illuminants.h +++ b/src/common/illuminants.h @@ -303,14 +303,20 @@ static inline int illuminant_to_xy(const dt_illuminant_t illuminant, / case DT_ILLUMINANT_CAMERA: { // Detect WB from RAW EXIF, correcting with D65/wb_coeff ratios - if(img) - if(find_temperature_from_as_shot_coeffs(img, correction_ratios, &x, &y)) break; + if(img && find_temperature_from_as_shot_coeffs(img, correction_ratios, &x, &y)) + break; + + // xy calculation failed + return FALSE; } case DT_ILLUMINANT_FROM_WB: { // Detect WB from user-provided coefficients - if(img) - if(find_temperature_from_wb_coeffs(img, wb_coeffs, &x, &y)) break; + if(img && find_temperature_from_wb_coeffs(img, wb_coeffs, &x, &y)) + break; + + // xy calculation failed + return FALSE; } case DT_ILLUMINANT_CUSTOM: // leave x and y as-is case DT_ILLUMINANT_DETECT_EDGES: @@ -395,7 +401,7 @@ static inline void matrice_pseudoinverse(float (*in)[3], float (*out)[3], int si } } -// returns TRUE is OK, FALSE if failed +// returns TRUE if OK, FALSE if failed static gboolean get_CAM_to_XYZ(const dt_image_t * img, float(* CAM_to_XYZ)[3]) { if(img == NULL || CAM_to_XYZ == NULL) return FALSE; @@ -433,12 +439,22 @@ static gboolean get_CAM_to_XYZ(const dt_image_t * img, float(* CAM_to_XYZ)[3]) return dt_is_valid_colormatrix(CAM_to_XYZ[0][0]); } +static inline gboolean _wb_coeffs_invalid(const dt_aligned_pixel_t wb_coeffs, const int num_coeffs) +{ + for(int k = 0; k < num_coeffs; k++) + if(!dt_isnormal(wb_coeffs[k]) || wb_coeffs[k] == 0.0f) return TRUE; + + return FALSE; +} + // returns FALSE if failed; TRUE if successful static gboolean find_temperature_from_wb_coeffs(const dt_image_t *img, const dt_aligned_pixel_t wb_coeffs, float *chroma_x, float *chroma_y) { if(img == NULL || wb_coeffs == NULL) return FALSE; if(!dt_image_is_matrix_correction_supported(img)) return FALSE; + // it's enough to check the first 3 here, as WB_coeffs_to_illuminant_xy only uses indices 0 to 2 + if(_wb_coeffs_invalid(wb_coeffs, 3)) return FALSE; float CAM_to_XYZ[4][3]; if(!get_CAM_to_XYZ(img, CAM_to_XYZ)) { @@ -458,15 +474,9 @@ static gboolean find_temperature_from_as_shot_coeffs(const dt_image_t *img, cons float *chroma_x, float *chroma_y) { if(img == NULL) return FALSE; - - gboolean has_valid_coeffs = TRUE; const int num_coeffs = (img->flags & DT_IMAGE_4BAYER) ? 4 : 3; - // Check coeffs - for(int k = 0; has_valid_coeffs && k < num_coeffs; k++) - if(!dt_isnormal(img->wb_coeffs[k]) || img->wb_coeffs[k] == 0.0f) has_valid_coeffs = FALSE; - - if(!has_valid_coeffs) return FALSE; + if(_wb_coeffs_invalid(img->wb_coeffs, num_coeffs)) return FALSE; // Get as-shot white balance camera factors (from raw) // component wise raw-RGB * wb_coeffs should provide R=G=B for a neutral patch under @@ -475,7 +485,7 @@ static gboolean find_temperature_from_as_shot_coeffs(const dt_image_t *img, cons // Adapt the camera coeffs with custom D65 coefficients if provided ('caveats' workaround) // this can deal with WB coeffs that don't use the input matrix reference - // adaptation_ratios[k] = chr->D65coeffs[k] / chr->wb_coeffs[k] + // correction_ratios[k] = chr->D65coeffs[k] / chr->wb_coeffs[k] if(correction_ratios) for(size_t k = 0; k < 4; k++) WB[k] *= correction_ratios[k]; // for a neutral surface, raw RGB * img->wb_coeffs would produce neutral R=G=B diff --git a/src/iop/channelmixerrgb.c b/src/iop/channelmixerrgb.c index 62fd8b47d2a..d05221906ba 100644 --- a/src/iop/channelmixerrgb.c +++ b/src/iop/channelmixerrgb.c @@ -4088,13 +4088,10 @@ void gui_changed(dt_iop_module_t *self, } if(*prev_illuminant == DT_ILLUMINANT_FROM_WB) { - // If illuminant was previously set with "as set in camera", - // when changing it, we need to ensure the temperature and - // chromaticity are inited with the correct values taken from - // camera EXIF. Otherwise, if using a preset defining - // illuminant = "as set in camera", temperature and - // chromaticity are inited with the preset content when - // illuminant is changed. + // If illuminant was previously "as set in white balance module", + // (x, y) were computed at runtime from the WB module's coefficients + // and p->x, p->y may hold stale values. Recompute them now so + // the new illuminant mode starts from the correct chromaticity. find_temperature_from_wb_coeffs(&(self->dev->image_storage), self->dev->chroma.wb_coeffs, &(p->x), &(p->y)); _check_if_close_to_daylight(p->x, p->y, &(p->temperature), NULL, &(p->adaptation)); @@ -4110,10 +4107,10 @@ void gui_changed(dt_iop_module_t *self, if(p->illuminant == DT_ILLUMINANT_CAMERA) { // Get camera WB and update illuminant - dt_aligned_pixel_t custom_wb; - _get_d65_correction_ratios(self, custom_wb); + dt_aligned_pixel_t correction_ratios; + _get_d65_correction_ratios(self, correction_ratios); const gboolean found = find_temperature_from_as_shot_coeffs(&(self->dev->image_storage), - custom_wb, &(p->x), &(p->y)); + correction_ratios, &(p->x), &(p->y)); _check_if_close_to_daylight(p->x, p->y, &(p->temperature), NULL, &(p->adaptation)); if(found) From 4165b0274c442b5f13e7ca23ecf508b6a59ac19c Mon Sep 17 00:00:00 2001 From: Kofa Date: Sat, 28 Feb 2026 14:48:17 +0100 Subject: [PATCH 07/27] wb: safe division in _get_d65_correction_ratios --- src/iop/channelmixerrgb.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/iop/channelmixerrgb.c b/src/iop/channelmixerrgb.c index d05221906ba..40af849688c 100644 --- a/src/iop/channelmixerrgb.c +++ b/src/iop/channelmixerrgb.c @@ -637,7 +637,12 @@ static gboolean _get_d65_correction_ratios(const dt_iop_module_t *self, if(valid_chroma && changed_chroma) { for_four_channels(k) - out_correction_ratios[k] = (float)chr->D65coeffs[k] / chr->wb_coeffs[k]; + { + if(chr->wb_coeffs[k] > 1e-6f) + out_correction_ratios[k] = chr->D65coeffs[k] / chr->wb_coeffs[k]; + else + out_correction_ratios[k] = 1.0f; + } } return FALSE; } From 152a69382514a51cbb5469ac08abba57738b5dc9 Mon Sep 17 00:00:00 2001 From: Kofa Date: Sat, 28 Feb 2026 15:08:24 +0100 Subject: [PATCH 08/27] wb: added _get_corrected_illuminant_xy to deduplicate repeated code --- src/iop/channelmixerrgb.c | 65 +++++++++++++-------------------------- 1 file changed, 21 insertions(+), 44 deletions(-) diff --git a/src/iop/channelmixerrgb.c b/src/iop/channelmixerrgb.c index 40af849688c..c4aea57c53c 100644 --- a/src/iop/channelmixerrgb.c +++ b/src/iop/channelmixerrgb.c @@ -647,6 +647,23 @@ static gboolean _get_d65_correction_ratios(const dt_iop_module_t *self, return FALSE; } +static void _get_corrected_illuminant_xy(const dt_iop_module_t *self, + const dt_iop_channelmixer_rgb_params_t *p, + float *x, float *y) +{ + dt_aligned_pixel_t correction_ratios; + _get_d65_correction_ratios(self, correction_ratios); + dt_aligned_pixel_t wb_coeffs = { 0.f }; + if(p->illuminant == DT_ILLUMINANT_FROM_WB) + { + for(int k = 0; k < 4; k++) + wb_coeffs[k] = self->dev->chroma.wb_coeffs[k]; + } + illuminant_to_xy(p->illuminant, &(self->dev->image_storage), correction_ratios, + wb_coeffs, x, y, p->temperature, p->illum_fluo, + p->illum_led); +} + DT_OMP_DECLARE_SIMD(aligned(input, output:16) uniform(compression, clip)) static inline void _gamut_mapping(const dt_aligned_pixel_t input, @@ -3165,17 +3182,7 @@ void commit_params(dt_iop_module_t *self, // find x y coordinates of illuminant for CIE 1931 2° observer float x = p->x; float y = p->y; - dt_aligned_pixel_t correction_ratios; - _get_d65_correction_ratios(self, correction_ratios); - dt_aligned_pixel_t wb_coeffs = {0.f}; - if(p->illuminant == DT_ILLUMINANT_FROM_WB) - { - for(int k = 0; k < 4; k++) - wb_coeffs[k] = self->dev->chroma.wb_coeffs[k]; - } - illuminant_to_xy(p->illuminant, &(self->dev->image_storage), correction_ratios, - wb_coeffs, &x, &y, p->temperature, p->illum_fluo, - p->illum_led); + _get_corrected_illuminant_xy(self, p, &x, &y); // if illuminant is from camera or user WB coefficients, x and y are set on-the-fly at // commit time, so we need to set adaptation too @@ -3617,17 +3624,7 @@ static gboolean _illuminant_color_draw(GtkWidget *widget, float x = p->x; float y = p->y; dt_aligned_pixel_t RGB = { 0.f }; - dt_aligned_pixel_t correction_ratios; - _get_d65_correction_ratios(self, correction_ratios); - dt_aligned_pixel_t wb_coeffs = { 0.f }; - if(p->illuminant == DT_ILLUMINANT_FROM_WB) - { - for(int k = 0; k < 4; k++) - wb_coeffs[k] = self->dev->chroma.wb_coeffs[k]; - } - illuminant_to_xy(p->illuminant, &(self->dev->image_storage), correction_ratios, - wb_coeffs, &x, &y, p->temperature, p->illum_fluo, - p->illum_led); + _get_corrected_illuminant_xy(self, p, &x, &y); illuminant_xy_to_RGB(x, y, RGB); cairo_set_source_rgb(cr, RGB[0], RGB[1], RGB[2]); cairo_rectangle(cr, INNER_PADDING, margin, width, height); @@ -3727,17 +3724,7 @@ static void _update_approx_cct(const dt_iop_module_t *self) float x = p->x; float y = p->y; - dt_aligned_pixel_t correction_ratios; - _get_d65_correction_ratios(self, correction_ratios); - dt_aligned_pixel_t wb_coeffs = { 0.f }; - if(p->illuminant == DT_ILLUMINANT_FROM_WB) - { - for(int k = 0; k < 4; k++) - wb_coeffs[k] = self->dev->chroma.wb_coeffs[k]; - } - illuminant_to_xy(p->illuminant, &(self->dev->image_storage), correction_ratios, - wb_coeffs, &x, &y, p->temperature, p->illum_fluo, - p->illum_led); + _get_corrected_illuminant_xy(self, p, &x, &y); dt_illuminant_t test_illuminant; float t = 5000.f; @@ -4330,17 +4317,7 @@ static void _auto_set_illuminant(dt_iop_module_t *self, // find x y coordinates of illuminant for CIE 1931 2° observer float x = p->x; float y = p->y; - dt_aligned_pixel_t correction_ratios; - _get_d65_correction_ratios(self, correction_ratios); - dt_aligned_pixel_t wb_coeffs = { 0.f }; - if(p->illuminant == DT_ILLUMINANT_FROM_WB) - { - for(int k = 0; k < 4; k++) - wb_coeffs[k] = self->dev->chroma.wb_coeffs[k]; - } - illuminant_to_xy(p->illuminant, &(self->dev->image_storage), correction_ratios, - wb_coeffs, &x, &y, p->temperature, p->illum_fluo, - p->illum_led); + _get_corrected_illuminant_xy(self, p, &x, &y); dt_adaptation_t adaptation = p->adaptation; From 894894b7ff0348945eb6d4e4c0ee161bb6549ccd Mon Sep 17 00:00:00 2001 From: Kofa Date: Fri, 6 Mar 2026 19:16:58 +0100 Subject: [PATCH 09/27] wb: fix illuminants.h switch-case fall-through introduced when DT_ILLUMINANT_CAMERA was added --- src/common/illuminants.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/common/illuminants.h b/src/common/illuminants.h index 8300b2c7110..99a89343ae4 100644 --- a/src/common/illuminants.h +++ b/src/common/illuminants.h @@ -298,7 +298,8 @@ static inline int illuminant_to_xy(const dt_illuminant_t illuminant, / // Model valid for T in [1667 ; 25000] K CCT_to_xy_blackbody(t, &x, &y); if(y != 0.f && x != 0.f) break; - // else t is out of bounds -> use custom/original values (next case) + // else t is out of bounds -> use custom/original values + return FALSE; } case DT_ILLUMINANT_CAMERA: { From e00edaa0f5ef7f7dee5586e29e1ed50a0c4f9624 Mon Sep 17 00:00:00 2001 From: Kofa Date: Sun, 5 Apr 2026 09:00:42 +0200 Subject: [PATCH 10/27] wb: renamed find_temperature_from_... to find_illuminant_xy_from_...; comment maintenance --- src/common/illuminants.h | 15 ++++++++------- src/iop/channelmixerrgb.c | 34 +++++++++++++++------------------- 2 files changed, 23 insertions(+), 26 deletions(-) diff --git a/src/common/illuminants.h b/src/common/illuminants.h index 99a89343ae4..fc74b0e1710 100644 --- a/src/common/illuminants.h +++ b/src/common/illuminants.h @@ -216,11 +216,12 @@ static inline void illuminant_CCT_to_RGB(const float t, dt_aligned_pixel_t RGB) illuminant_xy_to_RGB(x, y, RGB); } -static inline gboolean find_temperature_from_wb_coeffs(const dt_image_t *img, const dt_aligned_pixel_t wb_coeffs, +// Derive illuminant chromaticity from WB module coefficients +static inline gboolean find_illuminant_xy_from_wb_coeffs(const dt_image_t *img, const dt_aligned_pixel_t wb_coeffs, float *chroma_x, float *chroma_y); // Fetch image from pipeline and read EXIF for camera RAW WB coeffs -static inline gboolean find_temperature_from_as_shot_coeffs(const dt_image_t *img, const dt_aligned_pixel_t correction_ratios, +static inline gboolean find_illuminant_xy_from_as_shot_coeffs(const dt_image_t *img, const dt_aligned_pixel_t correction_ratios, float *chroma_x, float *chroma_y); @@ -304,7 +305,7 @@ static inline int illuminant_to_xy(const dt_illuminant_t illuminant, / case DT_ILLUMINANT_CAMERA: { // Detect WB from RAW EXIF, correcting with D65/wb_coeff ratios - if(img && find_temperature_from_as_shot_coeffs(img, correction_ratios, &x, &y)) + if(img && find_illuminant_xy_from_as_shot_coeffs(img, correction_ratios, &x, &y)) break; // xy calculation failed @@ -313,7 +314,7 @@ static inline int illuminant_to_xy(const dt_illuminant_t illuminant, / case DT_ILLUMINANT_FROM_WB: { // Detect WB from user-provided coefficients - if(img && find_temperature_from_wb_coeffs(img, wb_coeffs, &x, &y)) + if(img && find_illuminant_xy_from_wb_coeffs(img, wb_coeffs, &x, &y)) break; // xy calculation failed @@ -449,7 +450,7 @@ static inline gboolean _wb_coeffs_invalid(const dt_aligned_pixel_t wb_coeffs, co } // returns FALSE if failed; TRUE if successful -static gboolean find_temperature_from_wb_coeffs(const dt_image_t *img, const dt_aligned_pixel_t wb_coeffs, +static gboolean find_illuminant_xy_from_wb_coeffs(const dt_image_t *img, const dt_aligned_pixel_t wb_coeffs, float *chroma_x, float *chroma_y) { if(img == NULL || wb_coeffs == NULL) return FALSE; @@ -471,7 +472,7 @@ static gboolean find_temperature_from_wb_coeffs(const dt_image_t *img, const dt_ } // returns FALSE if failed; TRUE if successful -static gboolean find_temperature_from_as_shot_coeffs(const dt_image_t *img, const dt_aligned_pixel_t correction_ratios, +static gboolean find_illuminant_xy_from_as_shot_coeffs(const dt_image_t *img, const dt_aligned_pixel_t correction_ratios, float *chroma_x, float *chroma_y) { if(img == NULL) return FALSE; @@ -491,7 +492,7 @@ static gboolean find_temperature_from_as_shot_coeffs(const dt_image_t *img, cons for(size_t k = 0; k < 4; k++) WB[k] *= correction_ratios[k]; // for a neutral surface, raw RGB * img->wb_coeffs would produce neutral R=G=B - return find_temperature_from_wb_coeffs(img, WB, chroma_x, chroma_y); + return find_illuminant_xy_from_wb_coeffs(img, WB, chroma_x, chroma_y); } diff --git a/src/iop/channelmixerrgb.c b/src/iop/channelmixerrgb.c index c4aea57c53c..ed79aa8f60b 100644 --- a/src/iop/channelmixerrgb.c +++ b/src/iop/channelmixerrgb.c @@ -2217,7 +2217,7 @@ void process(dt_iop_module_t *self, dt_aligned_pixel_t correction_ratios; _get_d65_correction_ratios(self, correction_ratios); - if(find_temperature_from_as_shot_coeffs(&(self->dev->image_storage), correction_ratios, &(x), &(y))) + if(find_illuminant_xy_from_as_shot_coeffs(&(self->dev->image_storage), correction_ratios, &(x), &(y))) { // Convert illuminant from xyY to XYZ dt_aligned_pixel_t XYZ; @@ -2230,9 +2230,11 @@ void process(dt_iop_module_t *self, } else if(data->illuminant_type == DT_ILLUMINANT_FROM_WB) { + // Same logic as above (we depend on the coefficients from white balance, + // which don't come from the EXIF this time). float x, y; - if(find_temperature_from_wb_coeffs(&(self->dev->image_storage), self->dev->chroma.wb_coeffs, &(x), &(y))) + if(find_illuminant_xy_from_wb_coeffs(&(self->dev->image_storage), self->dev->chroma.wb_coeffs, &(x), &(y))) { // Convert illuminant from xyY to XYZ dt_aligned_pixel_t XYZ; @@ -2343,7 +2345,7 @@ int process_cl(dt_iop_module_t *self, dt_aligned_pixel_t correction_ratios; _get_d65_correction_ratios(self, correction_ratios); - if(find_temperature_from_as_shot_coeffs(&(self->dev->image_storage), correction_ratios, &(x), &(y))) + if(find_illuminant_xy_from_as_shot_coeffs(&(self->dev->image_storage), correction_ratios, &(x), &(y))) { // Convert illuminant from xyY to XYZ dt_aligned_pixel_t XYZ; @@ -2356,17 +2358,11 @@ int process_cl(dt_iop_module_t *self, } else if(d->illuminant_type == DT_ILLUMINANT_FROM_WB) { - // The camera illuminant is a behaviour rather than a preset of - // values: it uses whatever is in the RAW EXIF. But it depends on - // what temperature.c is doing and needs to be updated - // accordingly, to give a consistent result. We initialise the - // CAT defaults using the temperature coeffs at startup, but if - // temperature is changed later, we get no notification of the - // change here, so we can't update the defaults. So we need to - // re-run the detection at runtime… + // Same logic as above (we depend on the coefficients from white balance, + // which don't come from the EXIF this time). float x, y; - if(find_temperature_from_wb_coeffs(&(self->dev->image_storage), self->dev->chroma.wb_coeffs, &(x), &(y))) + if(find_illuminant_xy_from_wb_coeffs(&(self->dev->image_storage), self->dev->chroma.wb_coeffs, &(x), &(y))) { // Convert illuminant from xyY to XYZ dt_aligned_pixel_t XYZ; @@ -3089,7 +3085,7 @@ static void _preview_pipe_finished_callback(gpointer instance, dt_iop_module_t * float x = p->x; float y = p->y; - if(find_temperature_from_wb_coeffs(&(self->dev->image_storage), self->dev->chroma.wb_coeffs, &x, &y)) + if(find_illuminant_xy_from_wb_coeffs(&(self->dev->image_storage), self->dev->chroma.wb_coeffs, &x, &y)) { p->x = x; p->y = y; @@ -3979,7 +3975,7 @@ void reload_defaults(dt_iop_module_t *self) dt_aligned_pixel_t correction_ratios; if(!_get_d65_correction_ratios(self, correction_ratios)) { - if(find_temperature_from_as_shot_coeffs(img, correction_ratios, &(d->x), &(d->y))) + if(find_illuminant_xy_from_as_shot_coeffs(img, correction_ratios, &(d->x), &(d->y))) d->illuminant = DT_ILLUMINANT_CAMERA; _check_if_close_to_daylight(d->x, d->y, &(d->temperature), &(d->illuminant), &(d->adaptation)); @@ -4074,7 +4070,7 @@ void gui_changed(dt_iop_module_t *self, // illuminant is changed. dt_aligned_pixel_t correction_ratios; _get_d65_correction_ratios(self, correction_ratios); - find_temperature_from_as_shot_coeffs(&(self->dev->image_storage), correction_ratios, + find_illuminant_xy_from_as_shot_coeffs(&(self->dev->image_storage), correction_ratios, &(p->x), &(p->y)); _check_if_close_to_daylight(p->x, p->y, &(p->temperature), NULL, &(p->adaptation)); } @@ -4084,7 +4080,7 @@ void gui_changed(dt_iop_module_t *self, // (x, y) were computed at runtime from the WB module's coefficients // and p->x, p->y may hold stale values. Recompute them now so // the new illuminant mode starts from the correct chromaticity. - find_temperature_from_wb_coeffs(&(self->dev->image_storage), self->dev->chroma.wb_coeffs, + find_illuminant_xy_from_wb_coeffs(&(self->dev->image_storage), self->dev->chroma.wb_coeffs, &(p->x), &(p->y)); _check_if_close_to_daylight(p->x, p->y, &(p->temperature), NULL, &(p->adaptation)); } @@ -4101,16 +4097,16 @@ void gui_changed(dt_iop_module_t *self, // Get camera WB and update illuminant dt_aligned_pixel_t correction_ratios; _get_d65_correction_ratios(self, correction_ratios); - const gboolean found = find_temperature_from_as_shot_coeffs(&(self->dev->image_storage), + const gboolean found = find_illuminant_xy_from_as_shot_coeffs(&(self->dev->image_storage), correction_ratios, &(p->x), &(p->y)); _check_if_close_to_daylight(p->x, p->y, &(p->temperature), NULL, &(p->adaptation)); if(found) dt_control_log(_("white balance successfully extracted from raw image")); } - if(p->illuminant == DT_ILLUMINANT_FROM_WB) + else if(p->illuminant == DT_ILLUMINANT_FROM_WB) { - const gboolean found = find_temperature_from_wb_coeffs(&(self->dev->image_storage), + const gboolean found = find_illuminant_xy_from_wb_coeffs(&(self->dev->image_storage), self->dev->chroma.wb_coeffs, &(p->x), &(p->y)); _check_if_close_to_daylight(p->x, p->y, &(p->temperature), NULL, &(p->adaptation)); From 077fd7acfac0b6f7c8afd3dc111942da140bab43 Mon Sep 17 00:00:00 2001 From: Kofa Date: Sun, 31 May 2026 12:27:51 +0200 Subject: [PATCH 11/27] Doc updates --- dev-doc/IOP_Module_API.md | 29 ++- dev-doc/Module_Lifecycle.md | 504 ++++++++++++++++++++++++++++++++++++ 2 files changed, 532 insertions(+), 1 deletion(-) create mode 100644 dev-doc/Module_Lifecycle.md diff --git a/dev-doc/IOP_Module_API.md b/dev-doc/IOP_Module_API.md index f287d395852..d0a716c2a97 100644 --- a/dev-doc/IOP_Module_API.md +++ b/dev-doc/IOP_Module_API.md @@ -483,7 +483,13 @@ Access in `process_cl()` via `self->global_data`. ### `reload_defaults()` - Per-Image Defaults -Called when switching images. Update defaults based on image properties: +Called when switching images (and on first load). Its purpose is to make `self->default_params` image-specific — most modules have defaults that depend on whether the image is RAW, HDR, monochrome, etc., and those cannot be compile-time constants. + +When invoked through the wrapper `dt_iop_reload_defaults()`, the harness calls `dt_iop_load_default_params()` after the module's own `reload_defaults()` returns, copying `default_params` into `self->params`. So on the wrapper path, writing to `default_params` inside `reload_defaults()` immediately affects `self->params` as well. Most callers use the wrapper (e.g. `_dt_dev_load_pipeline_defaults()`, the per-module Reset button, instance duplication). + +**Caveat — direct calls bypass only the framework copy, not the module body.** A few sites invoke `module->reload_defaults(module)` directly instead of through the wrapper (for example `develop.c` calls `temperature->reload_defaults(temperature)` to refresh WB side-effects). Those callers do **not** get the automatic `dt_iop_load_default_params()`, so the wholesale `default_params → self->params` copy does not happen. This does **not** mean `self->params` is left untouched: the module's own `reload_defaults()` body may still write specific `self->params` fields directly. `temperature.reload_defaults()` does exactly this — it writes `p->preset` and `p->late_correction` (via `p = self->params`) in lockstep with `default_params`, while the WB coefficient array is written to `default_params` only. On a direct call, the result is therefore a *partial* update: the fields the body touches change in `self->params`, the rest do not, and no history entry is recorded. If you add such a call, prefer `dt_iop_reload_defaults()`, or audit exactly which `self->params` fields the body mutates and sync the rest yourself. + +#### Job 1 (all modules that implement this): compute image-specific default params ```c void reload_defaults(dt_iop_module_t *self) @@ -496,8 +502,29 @@ void reload_defaults(dt_iop_module_t *self) } ``` +Example: `exposure.c` disables itself for non-RAW images by setting `self->default_enabled = FALSE`. The exposure value in `default_params` stays at its static value because exposure is image-type-agnostic beyond that flag. + Common checks: `dt_image_is_raw()`, `dt_image_is_hdr()`, `dt_image_is_ldr()`, `dt_image_is_monochrome()`, `dt_image_is_bayerRGB()`. +#### Job 2 (some modules): write shared inter-module data as a side effect + +Some modules use `reload_defaults()` to populate shared state in `dev->chroma` (or similar structures) that other modules depend on. This happens regardless of whether the module is enabled or has a history entry. + +Example: `temperature.c` always computes the camera's as-shot WB coefficients and D65 reference coefficients from the image's EXIF data and writes them into `dev->chroma.as_shot[]` and `dev->chroma.D65coeffs[]`. `channelmixerrgb.c` reads these values during its own `commit_params()` to perform chromatic adaptation. If `reload_defaults()` did not run unconditionally, `channelmixerrgb` would have no access to the camera's native WB — even when the white balance module itself is disabled or absent from the history. + +#### Where `default_params` is consumed + +**Initial image load.** Inside `dt_dev_read_history_ext()`, `_dt_dev_load_pipeline_defaults()` calls `reload_defaults()` for every module (in reverse pipe order) and copies each `default_params` into `params`. The function then adds the workflow default modules and any auto-presets, and builds `dev->history` **directly from the database rows** — it does *not* call `dt_dev_pop_history_items`. A module with a history row runs with the saved params from that row; a module without one keeps the image-specific `default_params` just loaded. This is true on both the first open of an image and the reopen of an edited one; the only difference is whether the database has any rows for that module. + +**Undo / redo / history-panel navigation.** This is the separate path that uses `dt_dev_pop_history_items_ext()`, which does two passes: + +1. Resets all modules: `module->params = module->default_params` — so modules absent from the replayed range start from their image-specific default, not garbage. +2. Replays history entries: `module->params = hist->params` for each entry up to the target position. + +So `default_params` is consumed in two ways: as the starting point for modules with no (or not-yet-replayed) history, and as the value the per-module "Reset to defaults" button restores. It also sets the visual "default" markers on GUI sliders (via `dt_bauhaus_slider_set_default()`). + +For the full sequence — load order, the `dev->chroma` side effects, and the reverse-iteration hazard — see `Module_Lifecycle.md`. + ### `change_image()` - Reset GUI State for the New Image Called on an image switch, after `reload_defaults()` and before the new image's params reach `gui_update()`. Switching image does not tear down every module: each module's base instance — the one with the lowest `multi_priority` — is kept, GUI and all, and reused for the new image (`src/views/darkroom.c`). Anything in `gui_data` that describes the *old* image — a cached curve, a selected region, a pending readout — therefore survives unless you clear it here: diff --git a/dev-doc/Module_Lifecycle.md b/dev-doc/Module_Lifecycle.md new file mode 100644 index 00000000000..7337e1844db --- /dev/null +++ b/dev-doc/Module_Lifecycle.md @@ -0,0 +1,504 @@ +# Module Lifecycle: What Happens When the User Interacts + +`IOP_Module_API.md` documents the callback API: what each function signature means and what +a module must implement. This document is its companion. It describes *when* those callbacks +fire and *what else* the system does around them — pipe change flags, cache invalidation, +history replay, and cross-module data flow. + +After reading the API doc you can implement a module. After reading this one you should also +have a mental model of what darktable does when the user loads an image, drags a slider, +toggles a module, resets it, or undoes an edit. + +References point to code by **file, function, and the relevant block** (an `if`, `for`, or +`while`), and use short snippets where that is clearer. They avoid line numbers, which drift +as the source changes. + +--- + +## 1. Background concepts + +These are the moving parts the later sections rely on. Each one gets a short definition, not +a full reference. + +### History stack (`dt_dev_history_item_t`) + +An ordered list of per-module parameter snapshots on `dt_develop_t`. `dev->history_end` is +the active cursor: only the entries in the range `[0, history_end)` are "live". Note that a +history item's **`enabled` flag is stored separately from `params`** — it is not part of the +params buffer. Toggling a module on or off and editing its parameters are therefore two +different kinds of history change. + +### Pipe types + +Three pixelpipes run the same module chain independently, at different resolutions: + +- **full** — the main darkroom image +- **preview** — the low-resolution thumbnail (used for the histogram and navigation) +- **preview2** — an optional second-window preview + +Each is marked dirty and re-run on its own, often in parallel on separate threads. "Mark all +pipes dirty" below means all of the pipes that exist for the current session. + +### Pipe change flags + +These are set on `pipe->changed`. They tell `dt_dev_pixelpipe_change()` (pixelpipe_hb.c) +which resync strategy to use when the pipeline next runs. The constants are defined in +pixelpipe_hb.h. + +| Flag | Meaning | Strategy | +|---|---|---| +| `DT_DEV_PIPE_TOP_CHANGED` | Only the topmost history item's params changed | `synch_top`: re-commit that one module; upstream piece hashes stay untouched | +| `DT_DEV_PIPE_SYNCH` | All items need a param resync, topology unchanged | `synch_all`: re-commit every history item in order | +| `DT_DEV_PIPE_REMOVE` | A module was added or removed; topology must be rebuilt | full `cleanup_nodes` + `create_nodes` + `synch_all` | +| `DT_DEV_PIPE_ZOOMED` | Zoom or pan only | skip the param sync; only the `modify_roi_in`/`modify_roi_out` chain re-runs | + +`synch_all` re-commits every module, but a module whose hash did not change still produces a +cache hit, so its `process()` is skipped. In other words, the flag controls *committing*, not +*reprocessing*. Reprocessing is decided per module by the cache (see §6). + +### Pipe status flags + +`DIRTY` (needs reprocessing), `RUNNING` (a thread is processing it now), `VALID` (output is +current), `INVALID` (unusable, for example during teardown). + +### `focus_hash` + +A hash of the currently focused module widget, updated by `dt_iop_request_focus()`. Inside +`_dev_add_history_item_ext()` (develop.c), the relevant test is roughly: + +```c +if(dev->focus_hash != hist->focus_hash) { /* force a new history entry */ } +``` + +If the user moved focus since the last commit, a **new** history entry is forced even when +the same module is edited again. The practical effect is that the edit then takes the `SYNCH` +path instead of the cheaper `TOP_CHANGED` path. If you write deferred or batched parameter +changes, keep this in mind, because it changes how much of the pipe re-commits. + +### Pixelpipe cache + +The cache is hash-based with lazy invalidation. The key for a module's output is the +**cumulative hash** of every upstream `piece->hash` value (each computed in +`dt_iop_commit_params()` from the op name, instance, params, and blend_params), combined with +the relevant color-profile information. It is *not* a hash of one module's data on its own. +The hashing happens in the basic-hash helper in pixelpipe_cache.c, which seeds the hash from +the image id and pipe profiles and then walks all modules up to the requested position. + +When the key matches a cached entry, the output is reused and `process()` is skipped. Stale +entries are not actively deleted on a param change; they simply stop being looked up, and the +fixed-size LRU evicts them later. + +### `dev->chroma` (shared WB/CAT state) + +A struct on `dt_develop_t` used by white balance (`temperature.c`, the producer) and color +calibration (`channelmixerrgb.c`, the consumer) to communicate. There is **no signal**; +synchronization is implicit, through the order in which `commit_params()` runs (see §4 and +§5). Temperature has two distinct write sites: + +- `reload_defaults()` writes `as_shot[]` and `D65coeffs[]`. This happens once per image load, + **even if temperature is disabled or absent from the history**, so channelmixerrgb always + has the camera's native WB reference available. +- `commit_params()` writes `wb_coeffs[]`, `chr->temperature`, and `chr->late_correction`, on + every parameter change. + +`channelmixerrgb.commit_params()` reads these values. + +### `module->params` vs `module->default_params` + +`default_params` is **image-specific**: it is set by `reload_defaults()` (see +`IOP_Module_API.md`). `params` is the **live editing state**. +`dt_dev_pop_history_items_ext()` resets every module to its `default_params` and then replays +the history entries on top. As a result, a module with no history entry runs with its +image-specific defaults, not with compile-time constants. + +--- + +## 2. Image loading + +A pristine (never edited) image and one with saved history both go through the **same** +`dt_dev_load_image()` → `dt_dev_read_history_ext()` chain. The "no history" versus "has +history" distinction is handled **inside** `dt_dev_read_history_ext()`, not by different +callers. + +### Load sequence (`dt_dev_load_image()`, develop.c) + +1. **`_dt_dev_load_raw()`** triggers the rawspeed decode through the mipmap cache (this + blocks), then reads the decoded image struct into `dev->image_storage`. The large mipmap + buffer is released right after the decode; `image_storage` keeps the metadata. +2. Mark all pipes `DIRTY` and set `loading = TRUE`. +3. **`dt_iop_load_modules()`** builds the IOP module list (`dev->iop`). +4. **`dt_dev_read_history_ext()`** does everything else — defaults, presets, and history + replay. See below. + +`dt_dev_load_image()` itself does not touch `dev->chroma` and does not load defaults. All of +that happens inside `dt_dev_read_history_ext()`. + +### Inside `dt_dev_read_history_ext()` (develop.c) + +This function applies saved history by **building `dev->history` directly from the database**. +It does *not* call `pop_history_items`. The reset-then-replay `pop` path is the undo/redo +path (§3f), not the initial-load path. + +**For every image** — this is the `if(!no_image)` block, and it runs whether or not the image +has saved history: + +- Clear the scratch `memory.history` table. +- **`dt_dev_reset_chroma()`** clears part of the shared WB state (§5 lists exactly which + fields): `temperature` and `adaptation` become `NULL`, and `wb_coeffs[]` is reset to 1.0. +- **`_dt_dev_load_pipeline_defaults()`** calls `dt_iop_reload_defaults()` on every module in + **reverse** pipe order (§5 explains the direction). This sets each module's image-specific + `default_params`. Because it goes through the wrapper, it also copies them into `params` + (via `dt_iop_load_default_params()`). Temperature additionally populates + `dev->chroma.as_shot[]` and `D65coeffs[]` as a side effect. +- **`_dev_add_default_modules()`** prepends the workflow-mandated modules into the in-memory + history. +- **`_dev_auto_apply_presets()`** applies auto-presets. It is gated on the + `DT_IMAGE_AUTO_PRESETS_APPLIED` image flag, **not** on `change_timestamp == -1` (that test + guards only the separate legacy pre-3.0 WB recovery branch). It is a no-op for an image + whose presets were already applied, so only a first import actually gains preset entries + here. +- **`_dev_merge_history()`** merges the in-memory default and preset history into + `main.history`, so the database read below picks it up. + +**Then, for all images,** the same database-read loop runs. For each `main.history` row it +allocates a `hist` item, copies the params (falling back to the module's `default_params` +when the row has none), sets `hist->enabled`, appends to `dev->history`, and increments +`history_end`: + +```c +memcpy(hist->params, hist->module->default_params, hist->module->params_size); +hist->enabled = TRUE; +... +dev->history = g_list_append(dev->history, hist); +dev->history_end++; +``` + +Modules with no database row keep the image-specific `default_params` set earlier by +`_dt_dev_load_pipeline_defaults()`. The `enabled` flag is carried per entry, separately from +params. + +After the loop, if both `temperature` and `channelmixerrgb` are present, the function calls +`temperature->reload_defaults(temperature)` **directly**, to resync the WB/CAT shared state: + +```c +if(temperature && channelmixerrgb) + temperature->reload_defaults(temperature); +``` + +This direct call is a deliberate wrapper bypass: it skips the framework's wholesale +`default_params → params` copy (`dt_iop_load_default_params()`). It still refreshes +`default_params` and the `dev->chroma` reference data, and temperature's own body also writes +a few `self->params` fields directly (`preset`, `late_correction`). So `params` is partially +updated, not fully resynced — see the `reload_defaults` caveat in `IOP_Module_API.md`. + +The function then re-reads `history_end` from `main.images`, and — when the GUI is attached — +calls `dt_dev_pipe_synch_all()` and `dt_dev_invalidate_all()` so the pipes will commit and +reprocess. The pipes stay `DIRTY`, and the pipeline threads run on the next redraw (§6). + +--- + +## 3. Editing operations + +### 3a. Adjust a slider — the common `TOP_CHANGED` path + +1. The bauhaus slider fires its value-change handler (`bauhaus.c`), which calls + `dt_iop_gui_changed(module, widget, &prev)` directly. +2. The module's `gui_changed()` runs (module-specific; it may update dependent widgets). +3. `dt_dev_add_history_item_target()` reaches `_dev_add_history_item_ext()` (develop.c), which + chooses one of two branches: + - **`TOP_CHANGED`**: the same module is already at the top of the history, + `dev->focus_hash == hist->focus_hash`, and only the params differ. The params are + updated in place and `pipe->changed |= DT_DEV_PIPE_TOP_CHANGED`. + - **`SYNCH`**: anything else (a different module, or focus changed since the last commit). + A new history entry is pushed and `pipe->changed |= DT_DEV_PIPE_SYNCH`. +4. `dt_dev_invalidate_all()` marks all pipes `DIRTY` and increments `dev->timestamp`. +5. `DT_SIGNAL_DEVELOP_HISTORY_CHANGE` is emitted **only when `need_end_record` is true**, not + on every slider tick. It is tied to the undo-record start/end pairing, so one logical edit + emits the signal once. +6. On the pipeline thread, `dt_dev_pixelpipe_change()` dispatches on the flag: + - `TOP_CHANGED` → `dt_dev_pixelpipe_synch_top()`: runs `commit_params()` for that one + module only; upstream piece hashes are not recomputed. + - `SYNCH` → `dt_dev_pixelpipe_synch_all()`: runs `commit_params()` for every history item + in order. Upstream modules re-commit, but an unchanged hash still hits the cache. +7. The pipeline runs: modules with unchanged hashes reuse their cached output; the changed + module and everything downstream of it reprocess. + +### 3b. Enable / disable a module + +1. The toggle button fires its `toggled` signal, which reaches `_gui_off_callback()` + (imageop.c). +2. The callback sets `module->enabled` and calls `dt_dev_add_history_item()`. The + `hist->enabled` flag is recorded **separately from params**. +3. `pipe->changed |= DT_DEV_PIPE_SYNCH`, because an enable/disable propagates downstream. +4. A full `synch_all` runs on the next pipeline pass. + +### 3c. Reset to defaults + +1. The reset button fires `button-press-event`, which reaches `_gui_reset_callback()` + (imageop.c). +2. `dt_iop_reload_defaults(module)` recomputes the image-specific `default_params` and copies + them into `module->params` (this is the wrapper path, so the copy does happen). +3. `dt_iop_gui_update(module)` syncs the widgets to the new params. +4. `dt_dev_add_history_item(module->dev, module, TRUE)` adds or updates a **single** history + entry. It does not rebuild the whole stack. +5. `pipe->changed |= DT_DEV_PIPE_SYNCH`. + +### 3d. Add a module instance (+ button) + +1. `dt_iop_gui_duplicate()` (imageop.c) starts the duplication. +2. It records the base module's state: `dt_dev_add_history_item(base->dev, base, FALSE)`. +3. `dt_dev_module_duplicate()` creates the new instance in `dev->iop`. +4. `dt_iop_reload_defaults(new_module)` sets the image-specific defaults for the new instance. +5. `dt_dev_add_history_item(new_module->dev, new_module, TRUE)` adds a new history entry. +6. `dt_dev_pixelpipe_rebuild()` sets `pipe->changed |= DT_DEV_PIPE_REMOVE`, which forces a + full topology rebuild — the node graph changed, so it is rebuilt from scratch. + +### 3e. Remove a module instance (− button) + +`dt_dev_module_remove()` (develop.c) does the teardown. Inside its `if(dev->gui_attached)` +block — true in the interactive darkroom — it **prunes the history**. It walks `dev->history` +and, for each entry whose module is the one being removed, frees the item, unlinks it, and +decrements `history_end`: + +```c +if(module == hist->module) +{ + dt_dev_free_history_item(hist); + dev->history = g_list_delete_link(dev->history, elem); + dev->history_end--; +} +``` + +It then removes the module from `dev->iop`. A `REMOVE` flag triggers a full topology rebuild +(`cleanup_nodes` + `create_nodes`). + +So a removed instance's history entries are **actively deleted**, not merely skipped. In a +headless context, where `gui_attached` is false, this pruning block does not run — but +interactive removal is the case this document covers. + +### 3f. Undo / redo / history-panel navigation + +This is a **distinct path** from the initial image load. During an active editing session it +is the main way `module->params` are rewritten from saved entries. + +1. The user clicks a history entry, or presses Ctrl+Z / Ctrl+Y. +2. `dt_dev_reload_history_items()` (develop.c) sets `dev->history_end` to the target position. +3. `dt_dev_pop_history_items(dev, 0)` resets all modules to their `default_params`. +4. The history is re-read (from memory or the database). +5. `dt_dev_pop_history_items(dev, history_end)` replays up to the target position. +6. `dt_dev_invalidate_all()` marks the pipes dirty, which leads to a full pipeline rerun. + +The two `pop_history_items` calls live **here**, in `dt_dev_reload_history_items()`, not in +`dt_dev_read_history_ext()`. Apart from the initial image load, this is the only path that +rewrites `module->params` from saved history. The `SYNCH` flag ensures every module +re-commits after the replay. + +--- + +## 4. Cross-module interactions + +### 4a. Temperature (WB) → ChannelMixerRGB (through `dev->chroma`) + +There is no signal; synchronization is implicit and ordered by iop_order: + +- During `synch_all` or `synch_top`, `dt_iop_commit_params()` runs in pipe order. +- `temperature.commit_params()` writes `dev->chroma.wb_coeffs[]`, `chr->late_correction`, and + the `chr->temperature` pointer. +- `channelmixerrgb.commit_params()` runs **after** it and reads `wb_coeffs[]` to build its + chromatic-adaptation matrix. +- As a once-per-image-load side effect, `temperature.reload_defaults()` writes + `dev->chroma.as_shot[]` and `D65coeffs[]` **unconditionally** — even when temperature is + disabled or absent from the history — because channelmixerrgb needs the camera's native WB + reference to compute its default illuminant. +- For late correction, temperature sets `chr->late_correction` and channelmixerrgb reads it. + A WB-mode switch raises `SYNCH`, so both re-commit in order. + +### 4b. Color-profile change (colorin / colorout) + +This follows the same flow as a slider adjust: `TOP_CHANGED` or `SYNCH`, depending on +`focus_hash` and on whether it is an in-place update. There is no `REMOVE`, because the +topology does not change. `commit_params()` rebuilds the color transforms; no cross-module +signal is needed. Because the transform changes the pixels, all downstream cache hashes change +and everything downstream reprocesses. + +--- + +## 5. Pipeline ordering asymmetry + +Two operations walk the module list in **opposite** directions, which is a common source of +confusion. + +- **`commit_params()` (through `synch_all`)** iterates the history in history-list order. For + auto-applied presets this is effectively pipe order: temperature commits first and writes + `dev->chroma.wb_coeffs[]`; channelmixerrgb commits second and reads it. One caveat: + `synch_top` re-commits **only** the topmost history item, and relies on `wb_coeffs` still + being present in `dev->chroma` from a previous full commit. If temperature has never been + committed in the current session, `wb_coeffs` may be wrong on a `synch_top`. +- **`_dt_dev_load_pipeline_defaults()`** iterates in **reverse** pipe order (last module + first): + + ```c + for(const GList *modules = g_list_last(dev->iop); + modules; + modules = g_list_previous(modules)) + { + dt_iop_reload_defaults(modules->data); + } + ``` + + So channelmixerrgb's `reload_defaults()` runs **before** temperature's. + +### Why reverse? (and what it is *not*) + +The source gives no rationale for the reverse direction. Checked against the code, it is +**not** a chromatic-adaptation (CAT) deduplication mechanism, despite the intuitive theory +that "the last instance should claim the CAT first." CAT-token ownership is in fact +**order-independent**, for three reasons: + +- `_declare_cat_on_pipe()` (channelmixerrgb.c) resolves a conflict between multiple + `channelmixerrgb` instances through `dt_iop_is_first_instance()`. Its conflict branch can + take the token away from a previous claimant: + + ```c + if(chr->adaptation == NULL) + chr->adaptation = self; // first to register wins + else if(chr->adaptation == self) + ; // already ours + else if(dt_iop_is_first_instance(self->dev->iop, self)) + chr->adaptation = self; // earlier in the pipe: take over + ``` + + So the **first instance in `dev->iop` always ends up owning the token**, regardless of who + registered first. Tracing two eligible instances shows that forward iteration would dedup + the *defaults* more cleanly than reverse: reverse leaves both instances with active-CAT + defaults until a later commit re-resolves the token. +- `_declare_cat_on_pipe()` returns early when `gui_data == NULL`, so it is a no-op during + headless default loading. It therefore cannot be the reason the loop runs in reverse. +- Duplicate `channelmixerrgb` instances do not exist yet when + `_dt_dev_load_pipeline_defaults()` runs at image load. They are created later, by user + action or history replay. + +So the safe statement is: defaults load in reverse pipe order, and CAT dedup is handled by +`_declare_cat_on_pipe()` plus `dt_iop_is_first_instance()` — **not** by the loop direction. +The real hazard the reverse order creates is producer/consumer staleness for modules that read +shared state in `reload_defaults()`, described next. + +### What `dt_dev_reset_chroma()` actually resets + +`dt_dev_reset_chroma()` (develop.c) clears only three fields: + +```c +chr->adaptation = NULL; +chr->temperature = NULL; +for(int c = 0; c < 4; c++) chr->wb_coeffs[c] = 1.0f; +``` + +It does **not** reset `D65coeffs`, `as_shot`, or `late_correction`. Those are initialized only +by `dt_dev_init_chroma()`, at dev-context creation — not on each image switch. + +`channelmixerrgb.reload_defaults()` calls `_get_d65_correction_ratios()`, which reads +`dev->chroma.D65coeffs[]` and `as_shot[]`. Because temperature's `reload_defaults()` has not +run yet (reverse order) and `dt_dev_reset_chroma()` leaves those fields alone, channelmixerrgb +can read **stale values from the previously loaded image**. The visible effect: the default +illuminant xy that the Reset button restores can be computed from the wrong camera's daylight +reference right after an image switch. + +The `wb_coeffs` staleness (which affects active params through `commit_params()`) **is** fixed +by `dt_dev_reset_chroma()`. The `D65coeffs` / `as_shot` / `late_correction` staleness (which +affects **defaults only**) remains. It is a known limitation, not a mitigated bug. + +### Developer rule + +A module's `reload_defaults()` **cannot** assume that earlier-in-pipe modules have already +populated the shared `dev->chroma` state. The field that `dt_dev_reset_chroma()` clears +(`wb_coeffs`) is safe to read, because it holds a neutral 1.0. The fields it does **not** clear +(`D65coeffs`, `as_shot`, `late_correction`) may still hold stale data from the previous image +when `reload_defaults()` runs. + +--- + +## 6. Pipeline execution detail + +How pixels actually flow once the pipes are marked dirty: + +- `dt_dev_process_image_job()` (develop.c) is the entry point. It locks the pipe and captures + `dev->timestamp`. +- `dt_dev_pixelpipe_process()` calls `_dev_pixelpipe_process_rec()` (pixelpipe_hb.c), which + recurses from the last module back toward the input, pulling each module's input on demand. +- For each module, the cache key is the **cumulative hash** of all upstream `piece->hash` + values (each set in `dt_iop_commit_params()` from op, instance, params, and blend_params), + plus the color-profile information. On a key match, the cached output is reused and + `process()` is skipped. On a miss, `process()` (or `process_cl()` on the GPU) runs and the + result is stored. +- `modify_roi_in()` and `modify_roi_out()` run on every module during the ROI-propagation + phase, before processing, translating between the output-side and input-side regions. Under + the `ZOOMED` flag only the ROIs change — the module hashes do not — so cache hits are + possible for any module whose cached output region still covers the requested region. +- The cache is a fixed-size LRU, and eviction is automatic. There is no explicit invalidation + on a param change; a changed hash simply misses. +- After all modules finish, `pipe->status` becomes `VALID` and a finished signal (for example + `DT_SIGNAL_DEVELOP_PREVIEW_PIPE_FINISHED`) triggers the GTK redraw. +- The full and preview pipes run identical logic at different input resolutions and can run in + parallel on separate threads. + +--- + +## Key symbols + +Grouped by file. Look these up by name; the surrounding block is described in the relevant +section above. + +**develop.c** + +| Symbol | Role | +|---|---| +| `dt_dev_load_image` | Top-level image load | +| `dt_dev_read_history_ext` | Loads defaults/presets and builds `dev->history` from the DB | +| `_dt_dev_load_pipeline_defaults` | Calls `reload_defaults` on all modules, in reverse pipe order | +| `dt_dev_reset_chroma` | Clears `adaptation`, `temperature`, `wb_coeffs` only | +| `dt_dev_init_chroma` | Initializes the full `dev->chroma`, at dev-context creation | +| `dt_dev_add_history_item` / `_dev_add_history_item_ext` | Add/update a history entry; choose `TOP_CHANGED` vs `SYNCH` (the `focus_hash` test lives here) | +| `dt_dev_pop_history_items` / `_ext` | Reset to defaults, then replay history (used by undo/redo) | +| `dt_dev_reload_history_items` | Undo/redo and history-panel navigation | +| `dt_dev_module_remove` | Removes a module instance and prunes its history entries | +| `dt_dev_invalidate_all` | Marks all pipes dirty, bumps `dev->timestamp` | +| `_dev_auto_apply_presets` | Auto-presets; gated on `DT_IMAGE_AUTO_PRESETS_APPLIED` | +| `dt_dev_process_image_job` | Pixel-processing entry point | + +**imageop.c** + +| Symbol | Role | +|---|---| +| `dt_iop_reload_defaults` | Wrapper: calls the module's `reload_defaults()` then `dt_iop_load_default_params()` | +| `dt_iop_load_default_params` | Copies `default_params` into `params` | +| `dt_iop_commit_params` | Translates params into `piece->data`; sets `piece->hash` | +| `dt_iop_gui_changed` | Slider/widget change entry point | +| `dt_iop_is_first_instance` | True for the earliest instance of an op in `dev->iop` | +| `_gui_off_callback` | Enable/disable toggle handler | +| `_gui_reset_callback` | Reset-to-defaults handler | +| `dt_iop_gui_duplicate` | Add a module instance | + +**pixelpipe_hb.c / .h** + +| Symbol | Role | +|---|---| +| `DT_DEV_PIPE_*` flags | Pipe change flags (defined in pixelpipe_hb.h) | +| `dt_dev_pixelpipe_change` | Dispatches on the change flag | +| `dt_dev_pixelpipe_synch_top` | Re-commits the top module only | +| `dt_dev_pixelpipe_synch_all` | Re-commits all history items in order | +| `_dev_pixelpipe_process_rec` | Recursive per-module processing | + +**Other** + +| Symbol | File | Role | +|---|---|---| +| Basic-hash helper (cumulative cache hash) | pixelpipe_cache.c | Builds the cache key from image id, profiles, and all modules up to a position | +| `_declare_cat_on_pipe` | channelmixerrgb.c | Claims/resolves the CAT token | +| `temperature.commit_params` | temperature.c | Writes `dev->chroma.wb_coeffs` | +| `channelmixerrgb.commit_params` | channelmixerrgb.c | Reads `dev->chroma` | + +## See also + +- `IOP_Module_API.md` — the callback API reference (signatures, the two `reload_defaults` + jobs). +- `pixelpipe_architecture.md` — pipeline architecture, including the ordering-asymmetry note. From 98725b69fe12b117f72b5974c8218a353dc97de2 Mon Sep 17 00:00:00 2001 From: Kofa Date: Sun, 31 May 2026 16:19:24 +0200 Subject: [PATCH 12/27] Doc updates: wb + color calibration --- dev-doc/IOP_Module_API.md | 5 +- dev-doc/Module_Groups.md | 2 +- dev-doc/Module_Lifecycle.md | 199 ++++++++++++------------------ dev-doc/README.md | 1 + dev-doc/pixelpipe_architecture.md | 8 +- 5 files changed, 88 insertions(+), 127 deletions(-) diff --git a/dev-doc/IOP_Module_API.md b/dev-doc/IOP_Module_API.md index d0a716c2a97..0200373694e 100644 --- a/dev-doc/IOP_Module_API.md +++ b/dev-doc/IOP_Module_API.md @@ -4,6 +4,7 @@ This guide documents the functions that darktable Image Operation (IOP) modules See also: - [Pixelpipe Architecture](pixelpipe_architecture.md) for pipeline data flow and caching. +- [Module Lifecycle](Module_Lifecycle.md) for when these callbacks fire and what the system does around them. - [Introspection System](introspection.md) for parameter management. - [GUI Architecture](GUI.md) for GUI events, callbacks, and widget reparenting. - [GUI Threading](GUI_Threading.md) for sharing `gui_data` between the GTK and pipe worker threads. @@ -512,6 +513,8 @@ Some modules use `reload_defaults()` to populate shared state in `dev->chroma` ( Example: `temperature.c` always computes the camera's as-shot WB coefficients and D65 reference coefficients from the image's EXIF data and writes them into `dev->chroma.as_shot[]` and `dev->chroma.D65coeffs[]`. `channelmixerrgb.c` reads these values during its own `commit_params()` to perform chromatic adaptation. If `reload_defaults()` did not run unconditionally, `channelmixerrgb` would have no access to the camera's native WB — even when the white balance module itself is disabled or absent from the history. +For the complete producer/consumer map, the reverse-order default-loading reasoning, and which `dev->chroma` fields each module actually reads, see [iop/wb_and_colorcalibration](iop/wb_and_colorcalibration/README.md). + #### Where `default_params` is consumed **Initial image load.** Inside `dt_dev_read_history_ext()`, `_dt_dev_load_pipeline_defaults()` calls `reload_defaults()` for every module (in reverse pipe order) and copies each `default_params` into `params`. The function then adds the workflow default modules and any auto-presets, and builds `dev->history` **directly from the database rows** — it does *not* call `dt_dev_pop_history_items`. A module with a history row runs with the saved params from that row; a module without one keeps the image-specific `default_params` just loaded. This is true on both the first open of an image and the reopen of an edited one; the only difference is whether the database has any rows for that module. @@ -523,7 +526,7 @@ Example: `temperature.c` always computes the camera's as-shot WB coefficients an So `default_params` is consumed in two ways: as the starting point for modules with no (or not-yet-replayed) history, and as the value the per-module "Reset to defaults" button restores. It also sets the visual "default" markers on GUI sliders (via `dt_bauhaus_slider_set_default()`). -For the full sequence — load order, the `dev->chroma` side effects, and the reverse-iteration hazard — see `Module_Lifecycle.md`. +For the full sequence — load order, the `dev->chroma` side effects, and the reverse-iteration hazard — see [Module_Lifecycle.md](Module_Lifecycle.md). ### `change_image()` - Reset GUI State for the New Image diff --git a/dev-doc/Module_Groups.md b/dev-doc/Module_Groups.md index 7fb27a9c69b..331076785bf 100644 --- a/dev-doc/Module_Groups.md +++ b/dev-doc/Module_Groups.md @@ -9,7 +9,7 @@ Darktable uses a tabs-based interface in the right panel. Each tab corresponds t - **Technical**: Basic corrections (demosaic, lens, crop). - **Grading**: Color and tone grading. - **Effects**: Artistic effects. -- **Quick Access Panel**: (Special case, detailed in `Quick_Access_Panel.md`). +- **Quick Access Panel**: (Special case, detailed in [Quick_Access_Panel.md](Quick_Access_Panel.md)). ## Implementation (`modulegroups.c`) diff --git a/dev-doc/Module_Lifecycle.md b/dev-doc/Module_Lifecycle.md index 7337e1844db..044e95233f8 100644 --- a/dev-doc/Module_Lifecycle.md +++ b/dev-doc/Module_Lifecycle.md @@ -1,7 +1,7 @@ # Module Lifecycle: What Happens When the User Interacts -`IOP_Module_API.md` documents the callback API: what each function signature means and what -a module must implement. This document is its companion. It describes *when* those callbacks +[IOP_Module_API.md](IOP_Module_API.md) documents the callback API: what each function signature +means and what a module must implement. This document is its companion. It describes *when* those callbacks fire and *what else* the system does around them — pipe change flags, cache invalidation, history replay, and cross-module data flow. @@ -22,7 +22,9 @@ a full reference. ### History stack (`dt_dev_history_item_t`) -An ordered list of per-module parameter snapshots on `dt_develop_t`. `dev->history_end` is +An ordered list of per-module parameter snapshots on +[`dt_develop_t`](pixelpipe_architecture.md#dt_develop_t) (the main darkroom session state). +`dev->history_end` is the active cursor: only the entries in the range `[0, history_end)` are "live". Note that a history item's **`enabled` flag is stored separately from `params`** — it is not part of the params buffer. Toggling a module on or off and editing its parameters are therefore two @@ -54,7 +56,8 @@ pixelpipe_hb.h. `synch_all` re-commits every module, but a module whose hash did not change still produces a cache hit, so its `process()` is skipped. In other words, the flag controls *committing*, not -*reprocessing*. Reprocessing is decided per module by the cache (see §6). +*reprocessing*. Reprocessing is decided per module by the cache (see +[section 6](#6-pipeline-execution-detail)). ### Pipe status flags @@ -63,7 +66,9 @@ current), `INVALID` (unusable, for example during teardown). ### `focus_hash` -A hash of the currently focused module widget, updated by `dt_iop_request_focus()`. Inside +A module is **focused** when the user opens its panel or clicks into one of its widgets; the +darkroom tracks a single focused module at a time. `dt_iop_request_focus()` records that module +and updates `dev->focus_hash`, a hash of the focused module's widget. Inside `_dev_add_history_item_ext()` (develop.c), the relevant test is roughly: ```c @@ -86,27 +91,25 @@ the image id and pipe profiles and then walks all modules up to the requested po When the key matches a cached entry, the output is reused and `process()` is skipped. Stale entries are not actively deleted on a param change; they simply stop being looked up, and the -fixed-size LRU evicts them later. +fixed-size LRU evicts them later. For the full caching model see +[pixelpipe_architecture.md](pixelpipe_architecture.md#pipeline-caching). ### `dev->chroma` (shared WB/CAT state) A struct on `dt_develop_t` used by white balance (`temperature.c`, the producer) and color calibration (`channelmixerrgb.c`, the consumer) to communicate. There is **no signal**; -synchronization is implicit, through the order in which `commit_params()` runs (see §4 and -§5). Temperature has two distinct write sites: - -- `reload_defaults()` writes `as_shot[]` and `D65coeffs[]`. This happens once per image load, - **even if temperature is disabled or absent from the history**, so channelmixerrgb always - has the camera's native WB reference available. -- `commit_params()` writes `wb_coeffs[]`, `chr->temperature`, and `chr->late_correction`, on - every parameter change. - -`channelmixerrgb.commit_params()` reads these values. +synchronization is implicit, through the order in which `commit_params()` runs (see +[section 4](#4-cross-module-interactions) and [section 5](#5-pipeline-ordering-asymmetry)). +Temperature writes the camera WB reference in `reload_defaults()` and the live coefficients in +`commit_params()`; channelmixerrgb reads them. The full field-by-field producer/consumer map and +the reverse-load reasoning live in their own doc — see +[wb_and_colorcalibration](iop/wb_and_colorcalibration/README.md). ### `module->params` vs `module->default_params` `default_params` is **image-specific**: it is set by `reload_defaults()` (see -`IOP_Module_API.md`). `params` is the **live editing state**. +[IOP_Module_API.md](IOP_Module_API.md#reload_defaults---per-image-defaults)). `params` is the +**live editing state**. `dt_dev_pop_history_items_ext()` resets every module to its `default_params` and then replays the history entries on top. As a result, a module with no history entry runs with its image-specific defaults, not with compile-time constants. @@ -124,7 +127,9 @@ callers. 1. **`_dt_dev_load_raw()`** triggers the rawspeed decode through the mipmap cache (this blocks), then reads the decoded image struct into `dev->image_storage`. The large mipmap - buffer is released right after the decode; `image_storage` keeps the metadata. + buffer is released right after the decode; `image_storage` keeps the metadata. (The *mipmap + cache* holds pre-scaled copies of each image at several resolutions, used for fast thumbnail + and preview generation — see [mipmap](https://en.wikipedia.org/wiki/Mipmap).) 2. Mark all pipes `DIRTY` and set `loading = TRUE`. 3. **`dt_iop_load_modules()`** builds the IOP module list (`dev->iop`). 4. **`dt_dev_read_history_ext()`** does everything else — defaults, presets, and history @@ -135,18 +140,21 @@ that happens inside `dt_dev_read_history_ext()`. ### Inside `dt_dev_read_history_ext()` (develop.c) -This function applies saved history by **building `dev->history` directly from the database**. -It does *not* call `pop_history_items`. The reset-then-replay `pop` path is the undo/redo -path (§3f), not the initial-load path. +This function applies saved history by **building `dev->history` directly from the database** — +it allocates one history item per stored row and appends it to the list. (This is a different +mechanism from the reset-then-replay path that undo/redo uses later; that one is described in +[section 3f](#3f-undo-redo-and-history-panel-navigation).) **For every image** — this is the `if(!no_image)` block, and it runs whether or not the image has saved history: - Clear the scratch `memory.history` table. -- **`dt_dev_reset_chroma()`** clears part of the shared WB state (§5 lists exactly which - fields): `temperature` and `adaptation` become `NULL`, and `wb_coeffs[]` is reset to 1.0. +- **`dt_dev_reset_chroma()`** clears part of the shared WB state + ([section 5](#5-pipeline-ordering-asymmetry) lists exactly which fields): `temperature` and + `adaptation` become `NULL`, and `wb_coeffs[]` is reset to 1.0. - **`_dt_dev_load_pipeline_defaults()`** calls `dt_iop_reload_defaults()` on every module in - **reverse** pipe order (§5 explains the direction). This sets each module's image-specific + **reverse** pipe order ([section 5](#5-pipeline-ordering-asymmetry) explains the direction). + This sets each module's image-specific `default_params`. Because it goes through the wrapper, it also copies them into `params` (via `dt_iop_load_default_params()`). Temperature additionally populates `dev->chroma.as_shot[]` and `D65coeffs[]` as a side effect. @@ -189,11 +197,13 @@ This direct call is a deliberate wrapper bypass: it skips the framework's wholes `default_params → params` copy (`dt_iop_load_default_params()`). It still refreshes `default_params` and the `dev->chroma` reference data, and temperature's own body also writes a few `self->params` fields directly (`preset`, `late_correction`). So `params` is partially -updated, not fully resynced — see the `reload_defaults` caveat in `IOP_Module_API.md`. +updated, not fully resynced — see the `reload_defaults` caveat in +[IOP_Module_API.md](IOP_Module_API.md#reload_defaults---per-image-defaults). The function then re-reads `history_end` from `main.images`, and — when the GUI is attached — calls `dt_dev_pipe_synch_all()` and `dt_dev_invalidate_all()` so the pipes will commit and -reprocess. The pipes stay `DIRTY`, and the pipeline threads run on the next redraw (§6). +reprocess. The pipes stay `DIRTY`, and the pipeline threads run on the next redraw +([section 6](#6-pipeline-execution-detail)). --- @@ -276,7 +286,7 @@ So a removed instance's history entries are **actively deleted**, not merely ski headless context, where `gui_attached` is false, this pruning block does not run — but interactive removal is the case this document covers. -### 3f. Undo / redo / history-panel navigation +### 3f. Undo, redo, and history-panel navigation This is a **distinct path** from the initial image load. During an active editing session it is the main way `module->params` are rewritten from saved entries. @@ -299,19 +309,14 @@ re-commits after the replay. ### 4a. Temperature (WB) → ChannelMixerRGB (through `dev->chroma`) -There is no signal; synchronization is implicit and ordered by iop_order: - -- During `synch_all` or `synch_top`, `dt_iop_commit_params()` runs in pipe order. -- `temperature.commit_params()` writes `dev->chroma.wb_coeffs[]`, `chr->late_correction`, and - the `chr->temperature` pointer. -- `channelmixerrgb.commit_params()` runs **after** it and reads `wb_coeffs[]` to build its - chromatic-adaptation matrix. -- As a once-per-image-load side effect, `temperature.reload_defaults()` writes - `dev->chroma.as_shot[]` and `D65coeffs[]` **unconditionally** — even when temperature is - disabled or absent from the history — because channelmixerrgb needs the camera's native WB - reference to compute its default illuminant. -- For late correction, temperature sets `chr->late_correction` and channelmixerrgb reads it. - A WB-mode switch raises `SYNCH`, so both re-commit in order. +There is no signal; synchronization is implicit and ordered by iop_order. Because temperature is +upstream of channelmixerrgb, a full `synch_all` runs `commit_params()` in pipe order: +`temperature.commit_params()` writes `dev->chroma.wb_coeffs[]` first and +`channelmixerrgb.commit_params()` reads it afterward to build its chromatic-adaptation matrix. +(`synch_top` re-commits only the topmost history item, so it is not evidence that an upstream +producer has just run.) The full producer/consumer field map, the once-per-load `reload_defaults` +side effects, and why reverse-order default loading stays correct are covered in +[wb_and_colorcalibration](iop/wb_and_colorcalibration/README.md). ### 4b. Color-profile change (colorin / colorout) @@ -326,16 +331,14 @@ and everything downstream reprocesses. ## 5. Pipeline ordering asymmetry Two operations walk the module list in **opposite** directions, which is a common source of -confusion. +confusion. (See also the +[ordering-asymmetry note](pixelpipe_architecture.md#pipeline-ordering-asymmetry) in +pixelpipe_architecture.md.) -- **`commit_params()` (through `synch_all`)** iterates the history in history-list order. For - auto-applied presets this is effectively pipe order: temperature commits first and writes - `dev->chroma.wb_coeffs[]`; channelmixerrgb commits second and reads it. One caveat: - `synch_top` re-commits **only** the topmost history item, and relies on `wb_coeffs` still - being present in `dev->chroma` from a previous full commit. If temperature has never been - committed in the current session, `wb_coeffs` may be wrong on a `synch_top`. -- **`_dt_dev_load_pipeline_defaults()`** iterates in **reverse** pipe order (last module - first): +- **`commit_params()` (through `synch_all`)** iterates the history in history-list order, which + is effectively pipe order: an upstream module commits before a downstream one, so any state it + writes into shared structures is in place when the downstream module reads it. +- **`_dt_dev_load_pipeline_defaults()`** iterates in **reverse** pipe order (last module first): ```c for(const GList *modules = g_list_last(dev->iop); @@ -346,74 +349,20 @@ confusion. } ``` - So channelmixerrgb's `reload_defaults()` runs **before** temperature's. - -### Why reverse? (and what it is *not*) - -The source gives no rationale for the reverse direction. Checked against the code, it is -**not** a chromatic-adaptation (CAT) deduplication mechanism, despite the intuitive theory -that "the last instance should claim the CAT first." CAT-token ownership is in fact -**order-independent**, for three reasons: - -- `_declare_cat_on_pipe()` (channelmixerrgb.c) resolves a conflict between multiple - `channelmixerrgb` instances through `dt_iop_is_first_instance()`. Its conflict branch can - take the token away from a previous claimant: - - ```c - if(chr->adaptation == NULL) - chr->adaptation = self; // first to register wins - else if(chr->adaptation == self) - ; // already ours - else if(dt_iop_is_first_instance(self->dev->iop, self)) - chr->adaptation = self; // earlier in the pipe: take over - ``` - - So the **first instance in `dev->iop` always ends up owning the token**, regardless of who - registered first. Tracing two eligible instances shows that forward iteration would dedup - the *defaults* more cleanly than reverse: reverse leaves both instances with active-CAT - defaults until a later commit re-resolves the token. -- `_declare_cat_on_pipe()` returns early when `gui_data == NULL`, so it is a no-op during - headless default loading. It therefore cannot be the reason the loop runs in reverse. -- Duplicate `channelmixerrgb` instances do not exist yet when - `_dt_dev_load_pipeline_defaults()` runs at image load. They are created later, by user - action or history replay. - -So the safe statement is: defaults load in reverse pipe order, and CAT dedup is handled by -`_declare_cat_on_pipe()` plus `dt_iop_is_first_instance()` — **not** by the loop direction. -The real hazard the reverse order creates is producer/consumer staleness for modules that read -shared state in `reload_defaults()`, described next. - -### What `dt_dev_reset_chroma()` actually resets - -`dt_dev_reset_chroma()` (develop.c) clears only three fields: - -```c -chr->adaptation = NULL; -chr->temperature = NULL; -for(int c = 0; c < 4; c++) chr->wb_coeffs[c] = 1.0f; -``` - -It does **not** reset `D65coeffs`, `as_shot`, or `late_correction`. Those are initialized only -by `dt_dev_init_chroma()`, at dev-context creation — not on each image switch. - -`channelmixerrgb.reload_defaults()` calls `_get_d65_correction_ratios()`, which reads -`dev->chroma.D65coeffs[]` and `as_shot[]`. Because temperature's `reload_defaults()` has not -run yet (reverse order) and `dt_dev_reset_chroma()` leaves those fields alone, channelmixerrgb -can read **stale values from the previously loaded image**. The visible effect: the default -illuminant xy that the Reset button restores can be computed from the wrong camera's daylight -reference right after an image switch. - -The `wb_coeffs` staleness (which affects active params through `commit_params()`) **is** fixed -by `dt_dev_reset_chroma()`. The `D65coeffs` / `as_shot` / `late_correction` staleness (which -affects **defaults only**) remains. It is a known limitation, not a mitigated bug. + So a downstream module's `reload_defaults()` runs **before** an upstream one's. The source + documents no rationale for the direction. ### Developer rule A module's `reload_defaults()` **cannot** assume that earlier-in-pipe modules have already -populated the shared `dev->chroma` state. The field that `dt_dev_reset_chroma()` clears -(`wb_coeffs`) is safe to read, because it holds a neutral 1.0. The fields it does **not** clear -(`D65coeffs`, `as_shot`, `late_correction`) may still hold stale data from the previous image -when `reload_defaults()` runs. +populated shared state (such as `dev->chroma`), because under reverse iteration they have not run +yet. Shared state that a `reload_defaults()` depends on must therefore be **reset to a neutral +value before the reverse default-load** — which the framework does via `dt_dev_reset_chroma()` +just before `_dt_dev_load_pipeline_defaults()`. + +The worked example — how white balance and color calibration share `dev->chroma`, and why this +reset keeps color calibration's defaults image-local despite the reverse order — is in +[wb_and_colorcalibration](iop/wb_and_colorcalibration/README.md). --- @@ -426,7 +375,9 @@ How pixels actually flow once the pipes are marked dirty: - `dt_dev_pixelpipe_process()` calls `_dev_pixelpipe_process_rec()` (pixelpipe_hb.c), which recurses from the last module back toward the input, pulling each module's input on demand. - For each module, the cache key is the **cumulative hash** of all upstream `piece->hash` - values (each set in `dt_iop_commit_params()` from op, instance, params, and blend_params), + values (each set in + [`dt_iop_commit_params()`](IOP_Module_API.md#commit_params---transform-ui-parameters-into-processing-data) + from op, instance, params, and blend_params), plus the color-profile information. On a key match, the cached output is reused and `process()` is skipped. On a miss, `process()` (or `process_cl()` on the GPU) runs and the result is stored. @@ -455,8 +406,7 @@ section above. | `dt_dev_load_image` | Top-level image load | | `dt_dev_read_history_ext` | Loads defaults/presets and builds `dev->history` from the DB | | `_dt_dev_load_pipeline_defaults` | Calls `reload_defaults` on all modules, in reverse pipe order | -| `dt_dev_reset_chroma` | Clears `adaptation`, `temperature`, `wb_coeffs` only | -| `dt_dev_init_chroma` | Initializes the full `dev->chroma`, at dev-context creation | +| `dt_dev_reset_chroma` | Resets shared WB state to neutral before reverse default-load | | `dt_dev_add_history_item` / `_dev_add_history_item_ext` | Add/update a history entry; choose `TOP_CHANGED` vs `SYNCH` (the `focus_hash` test lives here) | | `dt_dev_pop_history_items` / `_ext` | Reset to defaults, then replay history (used by undo/redo) | | `dt_dev_reload_history_items` | Undo/redo and history-panel navigation | @@ -473,7 +423,6 @@ section above. | `dt_iop_load_default_params` | Copies `default_params` into `params` | | `dt_iop_commit_params` | Translates params into `piece->data`; sets `piece->hash` | | `dt_iop_gui_changed` | Slider/widget change entry point | -| `dt_iop_is_first_instance` | True for the earliest instance of an op in `dev->iop` | | `_gui_off_callback` | Enable/disable toggle handler | | `_gui_reset_callback` | Reset-to-defaults handler | | `dt_iop_gui_duplicate` | Add a module instance | @@ -493,12 +442,16 @@ section above. | Symbol | File | Role | |---|---|---| | Basic-hash helper (cumulative cache hash) | pixelpipe_cache.c | Builds the cache key from image id, profiles, and all modules up to a position | -| `_declare_cat_on_pipe` | channelmixerrgb.c | Claims/resolves the CAT token | -| `temperature.commit_params` | temperature.c | Writes `dev->chroma.wb_coeffs` | -| `channelmixerrgb.commit_params` | channelmixerrgb.c | Reads `dev->chroma` | + +For the white-balance ↔ color-calibration symbols (`temperature.*` / `channelmixerrgb.*` and the +`dev->chroma` helpers), see +[wb_and_colorcalibration](iop/wb_and_colorcalibration/README.md#key-symbols). ## See also -- `IOP_Module_API.md` — the callback API reference (signatures, the two `reload_defaults` - jobs). -- `pixelpipe_architecture.md` — pipeline architecture, including the ordering-asymmetry note. +- [IOP_Module_API.md](IOP_Module_API.md) — the callback API reference (signatures, the two + [`reload_defaults`](IOP_Module_API.md#reload_defaults---per-image-defaults) jobs). +- [pixelpipe_architecture.md](pixelpipe_architecture.md) — pipeline architecture, including the + [ordering-asymmetry note](pixelpipe_architecture.md#pipeline-ordering-asymmetry). +- [wb_and_colorcalibration](iop/wb_and_colorcalibration/README.md) — the white-balance ↔ + color-calibration interaction through `dev->chroma` (worked example of the ordering principle). diff --git a/dev-doc/README.md b/dev-doc/README.md index 54525eaa0a5..99e38038228 100644 --- a/dev-doc/README.md +++ b/dev-doc/README.md @@ -9,6 +9,7 @@ This guide covers building Image Operation (IOP) modules for darktable's darkroo |------|-------------| | **[IOP_Module_API.md](IOP_Module_API.md)** | Module API reference: params_t vs data_t, processing, commit_params, lifecycle | | **[pixelpipe_architecture.md](pixelpipe_architecture.md)** | Pixelpipe data flow, caching, ROI, ordering asymmetry | +| **[Module_Lifecycle.md](Module_Lifecycle.md)** | When callbacks fire: image load, edits, history replay, pipe flags, cross-module data flow | | **[introspection.md](introspection.md)** | Introspection system for parameters and GUI | | **[Shortcuts.md](Shortcuts.md)** | The Action/Shortcut system and `dt_action_def_t` | | **[Module_Groups.md](Module_Groups.md)** | Module grouping explanation and `default_group()` | diff --git a/dev-doc/pixelpipe_architecture.md b/dev-doc/pixelpipe_architecture.md index d3b768b50ef..306efb95598 100644 --- a/dev-doc/pixelpipe_architecture.md +++ b/dev-doc/pixelpipe_architecture.md @@ -4,6 +4,10 @@ The **pixelpipe** is the core image processing engine of darktable. It is respon ## Core Structures +### `dt_develop_t` +Defined in `src/develop/develop.h`. +This is the main darkroom session state — one per open image. Besides the pixelpipes (below) it holds the module list (`dev->iop`, a `GList` of `dt_iop_module_t`), the history stack (`dev->history` plus the `dev->history_end` cursor), and shared inter-module state such as `dev->chroma` (white-balance / chromatic-adaptation data passed from `temperature.c` to `channelmixerrgb.c`). Most lifecycle operations — image load, history replay, parameter commits — act on this structure. For the event-by-event walkthrough see [Module_Lifecycle.md](Module_Lifecycle.md). + ### `dt_dev_pixelpipe_t` Defined in `src/develop/pixelpipe_hb.h`. This structure represents a single instance of a processing pipeline. A `dt_develop_t` (the main development state) holds several pipes: - `dev->full.pipe`: Main darkroom center view (via `dt_dev_viewport_t full`). @@ -132,9 +136,9 @@ Two key pipeline operations iterate modules in **different** orders: - **`commit_params()`** runs in **forward** pipe order (e.g., temperature before channelmixerrgb). This is the normal processing direction. - **`_dt_dev_load_pipeline_defaults()`** runs in **reverse** pipe order (e.g., channelmixerrgb before temperature). This happens during history reset and default loading. -This asymmetry matters for modules that communicate via shared state. For example, `temperature.c` writes white balance coefficients into `dev->chroma.wb_coeffs`, and `channelmixerrgb.c` reads them during `commit_params()`. During forward processing, temperature commits first and the data is available. But during reverse-order default loading, channelmixerrgb runs first — before temperature has written its values. This caused a bug where stale values from a previous image or history state influenced the defaults. +This asymmetry matters for modules that communicate via shared state. For example, `temperature.c` writes white balance coefficients into `dev->chroma.wb_coeffs`, and `channelmixerrgb.c` reads them during `commit_params()`. During forward processing, temperature commits first and the data is available. During reverse-order default loading, channelmixerrgb runs first — before temperature has refreshed its values — so any shared state a `reload_defaults()` depends on must be reset to a neutral value beforehand. -**Consequence:** Shared state (like `dev->chroma`) must be properly reset before reverse-order iteration to prevent this class of ordering-dependent bug. +**Consequence:** Shared state (like `dev->chroma`) must be reset before reverse-order iteration. The framework does this via `dt_dev_reset_chroma()` immediately before `_dt_dev_load_pipeline_defaults()`; the neutral `wb_coeffs` it writes is what keeps the dependent defaults image-local. For the full white-balance ↔ color-calibration interaction, see [iop/wb_and_colorcalibration](iop/wb_and_colorcalibration/README.md). ## Introspection Connection From 18eb4dc5d8f01b186ea5705e29f8e0f71b40af18 Mon Sep 17 00:00:00 2001 From: Kofa Date: Sun, 31 May 2026 20:24:04 +0200 Subject: [PATCH 13/27] wb: removed 'as-shot to reference' (same as 'as shot' + 'prepare data for color calibration' (late_correction)); fixed behaviour when WB disabled; removed unused code --- src/common/exif.cc | 2 +- src/common/illuminants.h | 25 --------- src/develop/develop.h | 4 +- src/iop/temperature.c | 106 +++++++++++++-------------------------- 4 files changed, 37 insertions(+), 100 deletions(-) diff --git a/src/common/exif.cc b/src/common/exif.cc index 68576c259c5..b88c0399ca8 100644 --- a/src/common/exif.cc +++ b/src/common/exif.cc @@ -2275,7 +2275,7 @@ static bool _exif_decode_exif_data(dt_image_t *img, Exiv2::ExifData &exifData) dt_dng_illuminant_t illu[3] = { DT_LS_Unknown, DT_LS_Unknown, DT_LS_Unknown }; // make sure for later testing fallback later via - // `find_temperature_from_raw_coeffs` if there is no valid illuminant + // `find_illuminant_xy_from_as_shot_coeffs` if there is no valid illuminant dt_mark_colormatrix_invalid(&img->d65_color_matrix[0]); #if EXIV2_TEST_VERSION(0,27,4) diff --git a/src/common/illuminants.h b/src/common/illuminants.h index fc74b0e1710..be937386979 100644 --- a/src/common/illuminants.h +++ b/src/common/illuminants.h @@ -513,31 +513,6 @@ static inline float planckian_normal(const float x, const float t) } -DT_OMP_DECLARE_SIMD() -static inline void blackbody_xy_to_tinted_xy(const float x, const float y, const float t, const float tint, - float *x_out, float *y_out) -{ - // Move further away from planckian locus in the orthogonal direction, by an amount of "tint" - const float n = planckian_normal(x, t); - const float norm = sqrtf(1.f + n * n); - *x_out = x + tint * n / norm; - *y_out = y - tint / norm; -} - - -DT_OMP_DECLARE_SIMD() -static inline float get_tint_from_tinted_xy(const float x, const float y, const float t) -{ - // Find the distance between planckian locus and arbitrary x y chromaticity in the orthogonal direction - const float n = planckian_normal(x, t); - const float norm = sqrtf(1.f + n * n); - float x_bb, y_bb; - CCT_to_xy_blackbody(t, &x_bb, &y_bb); - const float tint = -(y - y_bb) * norm; - return tint; -} - - DT_OMP_DECLARE_SIMD() static inline void xy_to_uv(const float xy[2], float uv[2]) { diff --git a/src/develop/develop.h b/src/develop/develop.h index 5260094a983..6d434700a12 100644 --- a/src/develop/develop.h +++ b/src/develop/develop.h @@ -167,8 +167,8 @@ typedef struct dt_dev_viewport_t - the currently used wb_coeffs in temperature module - D65coeffs and as_shot are read from exif data f) - late_correction set by temperature if we want to process data as following - If we use the new DT_IOP_TEMP_D65_LATE mode or enable this in user modes of temperature.c - and don't have any temp parameters changes later we can calc correction ratios + If we enable this for non-D65 modes of temperature.c and don't have any + temp parameters changes later we can calc correction ratios to take white-balanced (neutralized) data to D65 */ typedef struct dt_dev_chroma_t diff --git a/src/iop/temperature.c b/src/iop/temperature.c index bc3a0b8f397..5ed52e030a7 100644 --- a/src/iop/temperature.c +++ b/src/iop/temperature.c @@ -52,7 +52,7 @@ DT_MODULE_INTROSPECTION(5, dt_iop_temperature_params_t) #define DT_IOP_LOWEST_TINT 0.135 #define DT_IOP_HIGHEST_TINT 2.326 -#define DT_IOP_NUM_OF_STD_TEMP_PRESETS 5 +#define DT_IOP_NUM_OF_STD_TEMP_PRESETS 4 // If you reorder presets combo, change this consts #define DT_IOP_TEMP_UNKNOWN -1 @@ -60,7 +60,6 @@ DT_MODULE_INTROSPECTION(5, dt_iop_temperature_params_t) #define DT_IOP_TEMP_SPOT 1 #define DT_IOP_TEMP_USER 2 #define DT_IOP_TEMP_D65 3 -#define DT_IOP_TEMP_D65_LATE 4 static void _gui_sliders_update(dt_iop_module_t *self); @@ -84,9 +83,7 @@ typedef struct dt_iop_temperature_gui_data_t GtkWidget *btn_asshot; //As Shot GtkWidget *btn_user; GtkWidget *btn_d65; - GtkWidget *btn_d65_late; GtkWidget *temp_label; - GtkWidget *balance_label; GtkWidget *check_late_correction; int preset_cnt; int preset_num[54]; @@ -193,11 +190,21 @@ int legacy_params(dt_iop_module_t *self, } if(old_version == 4) { + const int legacy_d65_late = 4; const dt_iop_temperature_params_v4_t *o = (dt_iop_temperature_params_v4_t *)old_params; dt_iop_temperature_params_v5_t *n = malloc(sizeof(dt_iop_temperature_params_v5_t)); memcpy(n, o, sizeof(dt_iop_temperature_params_v4_t)); - n->late_correction = FALSE; + if(o->preset == legacy_d65_late) + { + n->preset = DT_IOP_TEMP_AS_SHOT; + n->late_correction = TRUE; + } + else + { + n->preset = o->preset > legacy_d65_late ? o->preset - 1 : o->preset; + n->late_correction = FALSE; + } *new_params = n; *new_params_size = sizeof(dt_iop_temperature_params_v5_t); *new_version = 5; @@ -741,30 +748,21 @@ void commit_params(dt_iop_module_t *self, d->preset = p->preset; - gboolean effective_late_correction = FALSE; - - if(p->preset == DT_IOP_TEMP_D65_LATE) - effective_late_correction = TRUE; - else if(p->preset == DT_IOP_TEMP_D65) - effective_late_correction = FALSE; - else - effective_late_correction = p->late_correction; + const gboolean effective_late_correction = + p->preset == DT_IOP_TEMP_D65 ? FALSE : p->late_correction; d->late_correction = effective_late_correction; - chr->late_correction = effective_late_correction; + // When the WB module is disabled it applies no coefficients (wb_coeffs are + // published as 1.0 above), so it makes no promise that the data is white + // balanced. Do not ask colorin (or any other consumer) to "finish" a + // correction that never started, otherwise disabling WB would still pull the + // image to D65 via D65coeffs/1.0. + chr->late_correction = piece->enabled ? effective_late_correction : FALSE; - chr->temperature = piece->enabled ? self : NULL; /* Make sure the chroma information stuff is valid If piece is disabled we always clear the trouble message and make sure chroma does know there is no temperature module. */ - if(p->preset == DT_IOP_TEMP_D65_LATE) - chr->late_correction = TRUE; - else if(p->preset == DT_IOP_TEMP_D65) - chr->late_correction = FALSE; - else - chr->late_correction = p->late_correction; - chr->temperature = piece->enabled ? self : NULL; if(dt_pipe_is_preview(pipe) && !piece->enabled) dt_iop_clear_module_trouble_message(self); @@ -1182,7 +1180,6 @@ static inline const char *_preset_to_str(const int preset) case DT_IOP_TEMP_SPOT: return "by spot"; case DT_IOP_TEMP_USER: return "user defined"; case DT_IOP_TEMP_D65: return "camera reference"; - case DT_IOP_TEMP_D65_LATE: return "as shot to reference"; default: return "other"; } } @@ -1193,10 +1190,8 @@ static void _update_preset(dt_iop_module_t *self, int mode) dt_dev_chroma_t *chr = &self->dev->chroma; dt_iop_temperature_gui_data_t *g = self->gui_data; - const gboolean is_current_reference = (p->preset == DT_IOP_TEMP_D65_LATE) || - (p->preset == DT_IOP_TEMP_D65); - const gboolean is_new_mode_manual = (mode != DT_IOP_TEMP_D65_LATE) && - (mode != DT_IOP_TEMP_D65); + const gboolean is_current_reference = p->preset == DT_IOP_TEMP_D65; + const gboolean is_new_mode_manual = mode != DT_IOP_TEMP_D65; if(is_current_reference && is_new_mode_manual) { @@ -1206,17 +1201,13 @@ static void _update_preset(dt_iop_module_t *self, int mode) p->preset = mode; - if(mode == DT_IOP_TEMP_D65_LATE) - { - chr->late_correction = TRUE; - } - else if(mode == DT_IOP_TEMP_D65) + if(mode == DT_IOP_TEMP_D65) { chr->late_correction = FALSE; } else { - // For manual modes, use the parameter + // For non-D65 modes, use the parameter chr->late_correction = p->late_correction; } @@ -1234,9 +1225,6 @@ void gui_update(dt_iop_module_t *self) { dt_iop_temperature_gui_data_t *g = self->gui_data; dt_iop_temperature_params_t *p = self->params; - dt_iop_temperature_params_t *d = self->default_params; - - d->preset = dt_is_scene_referred() ? DT_IOP_TEMP_D65_LATE : DT_IOP_TEMP_AS_SHOT; const gboolean true_monochrome = dt_image_monochrome_flags(&self->dev->image_storage) & DT_IMAGE_MONOCHROME; @@ -1269,13 +1257,7 @@ void gui_update(dt_iop_module_t *self) gboolean found = FALSE; const dt_dev_chroma_t *chr = &self->dev->chroma; // is this a "as shot" white balance? - if(dt_dev_equal_chroma((float *)p, chr->as_shot) && (p->preset == DT_IOP_TEMP_D65_LATE)) - { - dt_bauhaus_combobox_set(g->presets, DT_IOP_TEMP_D65_LATE); - found = TRUE; - } - - else if(dt_dev_equal_chroma((float *)p, chr->as_shot)) + if(dt_dev_equal_chroma((float *)p, chr->as_shot)) { dt_bauhaus_combobox_set(g->presets, DT_IOP_TEMP_AS_SHOT); p->preset = DT_IOP_TEMP_AS_SHOT; @@ -1423,8 +1405,6 @@ void gui_update(dt_iop_module_t *self) p->preset == DT_IOP_TEMP_USER); gtk_toggle_button_set_active(GTK_TOGGLE_BUTTON(g->btn_d65), p->preset == DT_IOP_TEMP_D65); - gtk_toggle_button_set_active(GTK_TOGGLE_BUTTON(g->btn_d65_late), - p->preset == DT_IOP_TEMP_D65_LATE); _color_temptint_sliders(self); _color_rgb_sliders(self); @@ -1577,8 +1557,8 @@ void reload_defaults(dt_iop_module_t *self) dt_iop_temperature_params_t *d = self->default_params; dt_iop_temperature_params_t *p = self->params; - d->preset = dt_is_scene_referred() ? DT_IOP_TEMP_D65_LATE : DT_IOP_TEMP_AS_SHOT; - d->late_correction = dt_is_scene_referred(); + d->preset = p->preset = DT_IOP_TEMP_AS_SHOT; + d->late_correction = p->late_correction = dt_is_scene_referred(); float *dcoeffs = (float *)d; for_four_channels(k) @@ -1687,7 +1667,8 @@ void reload_defaults(dt_iop_module_t *self) { for_four_channels(k) dcoeffs[k] = as_shot[k]; - d->preset = p->preset = DT_IOP_TEMP_D65_LATE; + d->preset = p->preset = DT_IOP_TEMP_AS_SHOT; + d->late_correction = p->late_correction = TRUE; } else { @@ -1697,6 +1678,7 @@ void reload_defaults(dt_iop_module_t *self) dcoeffs[2] = coeffs[2]/coeffs[1]; dcoeffs[3] = coeffs[3]/coeffs[1]; dcoeffs[1] = 1.0f; + d->late_correction = p->late_correction = FALSE; } } } @@ -1734,7 +1716,6 @@ void reload_defaults(dt_iop_module_t *self) dt_bauhaus_combobox_add(g->presets, C_("white balance", "user modified")); // old "camera neutral", reason: better matches intent dt_bauhaus_combobox_add(g->presets, C_("white balance", "camera reference")); - dt_bauhaus_combobox_add(g->presets, C_("white balance", "as shot to reference")); g->preset_cnt = DT_IOP_NUM_OF_STD_TEMP_PRESETS; memset(g->preset_num, 0, sizeof(g->preset_num)); @@ -1744,7 +1725,6 @@ void reload_defaults(dt_iop_module_t *self) _gui_sliders_update(self); dt_bauhaus_combobox_set(g->presets, p->preset); - gtk_toggle_button_set_active(GTK_TOGGLE_BUTTON(g->btn_d65_late), p->preset == DT_IOP_TEMP_D65_LATE); gtk_toggle_button_set_active(GTK_TOGGLE_BUTTON(g->btn_asshot), p->preset == DT_IOP_TEMP_AS_SHOT); gtk_toggle_button_set_active(GTK_TOGGLE_BUTTON(g->btn_user), FALSE); gtk_toggle_button_set_active(GTK_TOGGLE_BUTTON(g->btn_d65), FALSE); @@ -1823,7 +1803,6 @@ static void _btn_toggled(GtkGestureSingle *gesture, const int preset = togglebutton == g->btn_asshot ? DT_IOP_TEMP_AS_SHOT : togglebutton == g->btn_d65 ? DT_IOP_TEMP_D65 : - togglebutton == g->btn_d65_late ? DT_IOP_TEMP_D65_LATE : togglebutton == g->btn_user ? DT_IOP_TEMP_USER : 0; if(!gtk_toggle_button_get_active(GTK_TOGGLE_BUTTON(togglebutton))) @@ -1865,8 +1844,6 @@ static void _preset_tune_callback(GtkWidget *widget, dt_iop_module_t *self) pos == DT_IOP_TEMP_USER); gtk_toggle_button_set_active(GTK_TOGGLE_BUTTON(g->btn_d65), pos == DT_IOP_TEMP_D65); - gtk_toggle_button_set_active(GTK_TOGGLE_BUTTON(g->btn_d65_late), - pos == DT_IOP_TEMP_D65_LATE); gboolean show_finetune = FALSE; dt_dev_chroma_t *chr = &self->dev->chroma; @@ -1892,9 +1869,6 @@ static void _preset_tune_callback(GtkWidget *widget, dt_iop_module_t *self) case DT_IOP_TEMP_D65: // camera reference d65 _temp_params_from_array(p, chr->D65coeffs); break; - case DT_IOP_TEMP_D65_LATE: // as shot wb just for now - _temp_params_from_array(p, chr->as_shot); - break; default: // camera WB presets { gboolean found = FALSE; @@ -2171,21 +2145,11 @@ void gui_init(dt_iop_module_t *self) (g->btn_d65, _("set white balance to camera reference point\nin most cases it should be D65")); - g->btn_d65_late = dt_iop_togglebutton_new(self, - N_("settings"), - N_("as shot to reference"), NULL, - G_CALLBACK(_btn_toggled), FALSE, 0, 0, - dtgtk_cairo_paint_bulb_mod, NULL); - gtk_widget_set_tooltip_text - (g->btn_d65_late, - _("set white balance to as shot and later correct to camera reference point,\nin most cases it should be D65")); - // put buttons at top. fill later. g->buttonbar = dt_gui_hbox(dt_gui_expand(g->btn_asshot), dt_gui_expand(g->colorpicker), dt_gui_expand(g->btn_user), - dt_gui_expand(g->btn_d65), - dt_gui_expand(g->btn_d65_late)); + dt_gui_expand(g->btn_d65)); dt_gui_add_class(g->buttonbar, "dt_iop_toggle"); g->presets = dt_bauhaus_combobox_new(self); @@ -2235,7 +2199,7 @@ void gui_init(dt_iop_module_t *self) gtk_widget_set_tooltip_text(g->check_late_correction, _("ensures the white balance coefficients are treated as a reference for the color calibration module.\n" - "keep this checked if you use the modern scene-referred workflow but want to manually adjust white balance.")); + "keep this checked for non-reference white balance in the modern scene-referred workflow.")); GtkWidget *box_enabled = dt_gui_vbox(g->buttonbar, g->check_late_correction, @@ -2300,9 +2264,9 @@ void gui_cleanup(dt_iop_module_t *self) void gui_reset(dt_iop_module_t *self) { dt_iop_temperature_gui_data_t *g = self->gui_data; - dt_iop_temperature_params_t *d = self->default_params; + dt_iop_temperature_params_t *p = self->params; - const int preset = d->preset = dt_is_scene_referred() ? DT_IOP_TEMP_D65_LATE : DT_IOP_TEMP_AS_SHOT; + const int preset = p->preset; dt_iop_color_picker_reset(self, TRUE); gtk_toggle_button_set_active(GTK_TOGGLE_BUTTON(g->btn_asshot), @@ -2311,8 +2275,6 @@ void gui_reset(dt_iop_module_t *self) preset == DT_IOP_TEMP_USER); gtk_toggle_button_set_active(GTK_TOGGLE_BUTTON(g->btn_d65), preset == DT_IOP_TEMP_D65); - gtk_toggle_button_set_active(GTK_TOGGLE_BUTTON(g->btn_d65_late), - preset == DT_IOP_TEMP_D65_LATE); _color_finetuning_slider(self); _color_rgb_sliders(self); From 588cecec8fa39fff57bbc6ec5df74656a4d4168b Mon Sep 17 00:00:00 2001 From: Kofa Date: Sat, 6 Jun 2026 22:26:04 +0200 Subject: [PATCH 14/27] wb: make sure color calibration / white balance warning is shown when WB disabled temperature.c: keep the module handle published even when disabled (comment in develop.h already promised 'always available for GUI reports'). commit params nulled it when disabled -> early exit in channelmixerrgb.c _set_trouble_messages -> 'white balance missing' never shown. channelmixerrgb.c: fix potential null pointer dereference if worker sets chr->temperature to null; minor rename/cleanup --- src/common/illuminants.h | 6 ++-- src/iop/channelmixerrgb.c | 61 ++++++++++++++++++++++----------------- src/iop/temperature.c | 16 ++++++---- 3 files changed, 47 insertions(+), 36 deletions(-) diff --git a/src/common/illuminants.h b/src/common/illuminants.h index be937386979..b38f8b4594b 100644 --- a/src/common/illuminants.h +++ b/src/common/illuminants.h @@ -343,15 +343,15 @@ static inline int illuminant_to_xy(const dt_illuminant_t illuminant, / static inline void WB_coeffs_to_illuminant_xy(const float CAM_to_XYZ[4][3], const dt_aligned_pixel_t WB, float *x, float *y) { - // Find the illuminant chromaticity x y from RAW WB coeffs and camera input matrice + // Find the illuminant chromaticity x y from RAW WB coeffs and camera input matrix dt_aligned_pixel_t XYZ, LMS; // Simulate white point, aka convert (1, 1, 1) in camera space to XYZ - // warning : we multiply the transpose of CAM_to_XYZ since the pseudoinverse transposes it + // warning: we multiply the transpose of CAM_to_XYZ since the pseudoinverse transposes it XYZ[0] = CAM_to_XYZ[0][0] / WB[0] + CAM_to_XYZ[1][0] / WB[1] + CAM_to_XYZ[2][0] / WB[2]; XYZ[1] = CAM_to_XYZ[0][1] / WB[0] + CAM_to_XYZ[1][1] / WB[1] + CAM_to_XYZ[2][1] / WB[2]; XYZ[2] = CAM_to_XYZ[0][2] / WB[0] + CAM_to_XYZ[1][2] / WB[1] + CAM_to_XYZ[2][2] / WB[2]; - // Matrices white point is D65. We need to convert it for darktable's pipe (D50) + // Matrix's white point is D65. We need to convert it for darktable's pipe (D50) static const dt_aligned_pixel_t D65 = { 0.941238f, 1.040633f, 1.088932f, 0.f }; const float p = powf(1.088932f / 0.818155f, 0.0834f); diff --git a/src/iop/channelmixerrgb.c b/src/iop/channelmixerrgb.c index ed79aa8f60b..03b070d5cd7 100644 --- a/src/iop/channelmixerrgb.c +++ b/src/iop/channelmixerrgb.c @@ -2010,20 +2010,27 @@ static void _set_trouble_messages(dt_iop_module_t *self) const dt_develop_t *dev = self->dev; const dt_dev_chroma_t *chr = &dev->chroma; + // module handle snapshots: dt_dev_reset_chroma() may NULL them + // from the pipe worker thread. single aligned pointer loads can't tear; + // the pointed-to modules are only freed on this (main) thread, + // so the snapshots stay valid for this call + dt_iop_module_t *const temperature = chr->temperature; + dt_iop_module_t *const adaptation = chr->adaptation; + dt_print(DT_DEBUG_PIPE | DT_DEBUG_VERBOSE, "trouble message for %s%s : temp=%p adapt=%p", - self->op, dt_iop_get_instance_id(self), chr->temperature, chr->adaptation); + self->op, dt_iop_get_instance_id(self), temperature, adaptation); // in temperature module we make sure this is only presented if temperature is enabled - if(!chr->temperature) + if(!temperature) { - if(chr->adaptation) - dt_iop_clear_module_trouble_message(chr->adaptation); + if(adaptation) + dt_iop_clear_module_trouble_message(adaptation); return; } - if(!chr->adaptation) + if(!adaptation) { - dt_iop_clear_module_trouble_message(chr->temperature); + dt_iop_clear_module_trouble_message(temperature); dt_iop_clear_module_trouble_message(self); return; } @@ -2033,32 +2040,32 @@ static void _set_trouble_messages(dt_iop_module_t *self) && !(p->illuminant == DT_ILLUMINANT_PIPE || p->adaptation == DT_ADAPTATION_RGB) && !dt_image_is_monochrome(&dev->image_storage); - const gboolean temperature_enabled = chr->temperature && chr->temperature->enabled; - const gboolean adaptation_enabled = chr->adaptation && chr->adaptation->enabled; + const gboolean temperature_enabled = temperature->enabled; + const gboolean adaptation_enabled = adaptation->enabled; - // our first and biggest problem : white balance module is being - // clever with WB coeffs - const gboolean problem1 = valid - && chr->adaptation == self + // this instance does CAT while WB uses an incompatible setup (neither camera reference + // D65, nor another mode with late_correction) + const gboolean wb_applied_twice = valid + && adaptation == self && temperature_enabled && !_dev_is_D65_chroma(dev); - // our second biggest problem : another channelmixerrgb instance is doing CAT - // earlier in the pipe and we don't use masking here. - const gboolean problem2 = valid + // another channelmixerrgb instance is doing CAT + // earlier in the pipe, and we don't use masking here + const gboolean double_cat = valid && adaptation_enabled && temperature_enabled - && chr->adaptation != self + && adaptation != self && !g->is_blending; - // our third and minor problem: white balance module is not active but default_enabled - // and we do chromatic adaptation in color calibration - const gboolean problem3 = valid + // white balance is off on an image that needs it (default_enabled), but we rely on + // it to pre-process the data (neutralizing coeffs + late_correction, or D65 coeffs) + const gboolean wb_missing = valid && adaptation_enabled && !temperature_enabled - && chr->temperature->default_enabled; + && temperature->default_enabled; - if(problem2) + if(double_cat) { dt_iop_set_module_trouble_message(self, _("double CAT applied"), @@ -2070,9 +2077,9 @@ static void _set_trouble_messages(dt_iop_module_t *self) return; } - if(problem1) + if(wb_applied_twice) { - dt_iop_set_module_trouble_message(chr->temperature, + dt_iop_set_module_trouble_message(temperature, _("white balance applied twice (details)"), _("the color calibration module is enabled and already provides\n" "chromatic adaptation.\n" @@ -2090,9 +2097,9 @@ static void _set_trouble_messages(dt_iop_module_t *self) return; } - if(problem3) + if(wb_missing) { - dt_iop_set_module_trouble_message(chr->temperature, + dt_iop_set_module_trouble_message(temperature, _("white balance missing (details)"), _("this module is not providing a valid reference illuminant\n" "causing chromatic adaptation issues in color calibration.\n" @@ -2110,9 +2117,9 @@ static void _set_trouble_messages(dt_iop_module_t *self) return; } - if(chr->adaptation && chr->adaptation == self) + if(adaptation == self) { - dt_iop_clear_module_trouble_message(chr->temperature); + dt_iop_clear_module_trouble_message(temperature); dt_iop_clear_module_trouble_message(self); } } diff --git a/src/iop/temperature.c b/src/iop/temperature.c index 5ed52e030a7..8ba68835a5e 100644 --- a/src/iop/temperature.c +++ b/src/iop/temperature.c @@ -733,6 +733,8 @@ void commit_params(dt_iop_module_t *self, { for_four_channels(k) chr->wb_coeffs[k] = 1.0f; + // keep the module handle available for GUI reports (see below) + chr->temperature = self; return; } @@ -759,13 +761,15 @@ void commit_params(dt_iop_module_t *self, // image to D65 via D65coeffs/1.0. chr->late_correction = piece->enabled ? effective_late_correction : FALSE; - /* Make sure the chroma information stuff is valid - If piece is disabled we always clear the trouble message and - make sure chroma does know there is no temperature module. + /* Always publish the module handle for GUI reports, regardless of the + enabled state. The enabled state is tracked separately via + chr->temperature->enabled (module flag) and pipe->dsc.temperature.enabled + (pipe flag). This lets color calibration warn about a disabled white + balance module while it is still doing chromatic adaptation. Trouble + message state is owned entirely by _set_trouble_messages() in + channelmixerrgb, so we no longer clear it here. */ - chr->temperature = piece->enabled ? self : NULL; - if(dt_pipe_is_preview(pipe) && !piece->enabled) - dt_iop_clear_module_trouble_message(self); + chr->temperature = self; } void init_pipe(dt_iop_module_t *self, From 2ff84640e9daa37c96ff59534013b94b0eab01c2 Mon Sep 17 00:00:00 2001 From: Kofa Date: Sun, 7 Jun 2026 09:31:25 +0200 Subject: [PATCH 15/27] wb: don't modify p->x, p->y in _preview_pipe_finished_callback; switch left-over gui->reset to DT_ENTER|LEAVE_GUI_UPDATE --- src/iop/channelmixerrgb.c | 17 +++++------------ 1 file changed, 5 insertions(+), 12 deletions(-) diff --git a/src/iop/channelmixerrgb.c b/src/iop/channelmixerrgb.c index 03b070d5cd7..abe7918d96a 100644 --- a/src/iop/channelmixerrgb.c +++ b/src/iop/channelmixerrgb.c @@ -3087,23 +3087,16 @@ static void _preview_pipe_finished_callback(gpointer instance, dt_iop_module_t * dt_iop_channelmixer_rgb_params_t *p = self->params; if(p->illuminant == DT_ILLUMINANT_FROM_WB) { - // The WB coefficients might have changed in the temperature module. - // We need to re-calculate the chromaticity (x, y) to update the GUI feedback. - float x = p->x; - float y = p->y; + // WB coefficients might have changed temperature.c; recalculate xy and update the GUI + float x, y; if(find_illuminant_xy_from_wb_coeffs(&(self->dev->image_storage), self->dev->chroma.wb_coeffs, &x, &y)) { - p->x = x; - p->y = y; - - darktable.gui->reset++; - // Update derived GUI widgets (CCT label, color patch) + DT_ENTER_GUI_UPDATE(); _update_approx_cct(self); _update_illuminant_color(self); - // Update Hue/Chroma sliders to match the new coordinates - dt_aligned_pixel_t xyY = { p->x, p->y, 1.f }; + dt_aligned_pixel_t xyY = { x, y, 1.f }; dt_aligned_pixel_t Lch; dt_xyY_to_Lch(xyY, Lch); @@ -3111,7 +3104,7 @@ static void _preview_pipe_finished_callback(gpointer instance, dt_iop_module_t * dt_bauhaus_slider_set(g->illum_x, rad2degf(Lch[2])); dt_bauhaus_slider_set(g->illum_y, Lch[1]); - darktable.gui->reset--; + DT_LEAVE_GUI_UPDATE(); } } From e0affe49c26d341d1572c5ba55aa79df73f80c40 Mon Sep 17 00:00:00 2001 From: Kofa Date: Sun, 7 Jun 2026 09:34:40 +0200 Subject: [PATCH 16/27] wb: minor clean-up in channelmixerrgb (GUI struct field names for clarity), no need to allocate + copy xy in illuminants.h find_illuminant_xy_from_wb_coeffs; no need for 0 check after isnormal --- src/common/illuminants.h | 7 ++----- src/iop/channelmixerrgb.c | 20 ++++++++++---------- 2 files changed, 12 insertions(+), 15 deletions(-) diff --git a/src/common/illuminants.h b/src/common/illuminants.h index b38f8b4594b..42f27f19d04 100644 --- a/src/common/illuminants.h +++ b/src/common/illuminants.h @@ -444,7 +444,7 @@ static gboolean get_CAM_to_XYZ(const dt_image_t * img, float(* CAM_to_XYZ)[3]) static inline gboolean _wb_coeffs_invalid(const dt_aligned_pixel_t wb_coeffs, const int num_coeffs) { for(int k = 0; k < num_coeffs; k++) - if(!dt_isnormal(wb_coeffs[k]) || wb_coeffs[k] == 0.0f) return TRUE; + if(!dt_isnormal(wb_coeffs[k])) return TRUE; return FALSE; } @@ -463,10 +463,7 @@ static gboolean find_illuminant_xy_from_wb_coeffs(const dt_image_t *img, const d return FALSE; } - float x, y; - WB_coeffs_to_illuminant_xy(CAM_to_XYZ, wb_coeffs, &x, &y); - *chroma_x = x; - *chroma_y = y; + WB_coeffs_to_illuminant_xy(CAM_to_XYZ, wb_coeffs, chroma_x, chroma_y); return TRUE; } diff --git a/src/iop/channelmixerrgb.c b/src/iop/channelmixerrgb.c index abe7918d96a..f8005182f67 100644 --- a/src/iop/channelmixerrgb.c +++ b/src/iop/channelmixerrgb.c @@ -149,8 +149,8 @@ typedef struct dt_iop_channelmixer_rgb_gui_data_t GtkWidget *scale_grey_R, *scale_grey_G, *scale_grey_B; GtkWidget *normalize_R, *normalize_G, *normalize_B, *normalize_sat, *normalize_light, *normalize_grey; GtkWidget *color_picker; - float xy[2]; - float XYZ[4]; + float colorchecker_xy[2]; + float ai_wb_XYZ[4]; point_t box[4]; // the current coordinates, possibly non rectangle, of the bounding box for the color checker point_t ideal_box[4]; // the desired coordinates of the perfect rectangle bounding box for the color checker @@ -1729,8 +1729,8 @@ static void _extract_color_checker(const float *const restrict in, dt_D50_XYZ_to_xyY(illuminant_XYZ, illuminant_xyY); // save the illuminant in GUI struct for commit - g->xy[0] = illuminant_xyY[0]; - g->xy[1] = illuminant_xyY[1]; + g->colorchecker_xy[0] = illuminant_xyY[0]; + g->colorchecker_xy[1] = illuminant_xyY[1]; // and recompute back the LMS to be sure we use the parameters that // will be computed later @@ -2193,7 +2193,7 @@ void process(dt_iop_module_t *self, // as scratch space since we will be overwriting it afterwards // anyway _auto_detect_WB(in, out, data->illuminant_type, roi_in->width, roi_in->height, - ch, RGB_to_XYZ, g->XYZ); + ch, RGB_to_XYZ, g->ai_wb_XYZ); dt_dev_pixelpipe_cache_invalidate_later(piece->pipe, self->iop_order, "AI RGB: "); dt_iop_gui_leave_critical_section(self); } @@ -2984,8 +2984,8 @@ static void _commit_profile_callback(GtkWidget *widget, dt_iop_gui_enter_critical_section(self); - p->x = g->xy[0]; - p->y = g->xy[1]; + p->x = g->colorchecker_xy[0]; + p->y = g->colorchecker_xy[1]; p->illuminant = DT_ILLUMINANT_CUSTOM; _check_if_close_to_daylight(p->x, p->y, &p->temperature, NULL, NULL); @@ -3045,8 +3045,8 @@ static void _develop_ui_pipe_finished_callback(gpointer instance, dt_iop_module_ return; dt_iop_gui_enter_critical_section(self); - p->x = g->XYZ[0]; - p->y = g->XYZ[1]; + p->x = g->ai_wb_XYZ[0]; + p->y = g->ai_wb_XYZ[1]; dt_iop_gui_leave_critical_section(self); _check_if_close_to_daylight(p->x, p->y, @@ -4529,7 +4529,7 @@ void gui_init(dt_iop_module_t *self) g->delta_E_in = NULL; g->delta_E_label_text = NULL; - g->XYZ[0] = NAN; + g->ai_wb_XYZ[0] = NAN; #ifdef AI_ACTIVATED DT_CONTROL_SIGNAL_HANDLE(DT_SIGNAL_DEVELOP_UI_PIPE_FINISHED, _develop_ui_pipe_finished_callback); From 0383a86f93f298acf17006d99a1f00e8ed02eaa9 Mon Sep 17 00:00:00 2001 From: Kofa Date: Sun, 7 Jun 2026 10:44:32 +0200 Subject: [PATCH 17/27] wb: opposed.c hash: late_correction is a param of temperature (is before opposed), so already part of hash --- src/iop/hlreconstruct/opposed.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/iop/hlreconstruct/opposed.c b/src/iop/hlreconstruct/opposed.c index 409c6ac46e5..d459ff0e6ff 100644 --- a/src/iop/hlreconstruct/opposed.c +++ b/src/iop/hlreconstruct/opposed.c @@ -42,8 +42,7 @@ static dt_hash_t _opposed_hash(dt_dev_pixelpipe_iop_t *piece) { const dt_iop_highlights_data_t *d = piece->data; dt_hash_t hash = dt_dev_pixelpipe_piece_hash(piece, NULL, FALSE); - hash = dt_hash(hash, &d->clip, sizeof(d->clip)); - return dt_hash(hash, &piece->module->dev->chroma.late_correction, sizeof(int)); + return dt_hash(hash, &d->clip, sizeof(d->clip)); } static inline float _calc_linear_refavg(const float *in, const int color) From 7d2c708e9a163ee094c7b480fff588b9c40bd694 Mon Sep 17 00:00:00 2001 From: Kofa Date: Sun, 7 Jun 2026 15:18:26 +0200 Subject: [PATCH 18/27] wb: tiny clean-up: unnecessary parens, accidental duplication --- src/iop/channelmixerrgb.c | 10 +++++----- src/iop/temperature.c | 4 ++-- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/src/iop/channelmixerrgb.c b/src/iop/channelmixerrgb.c index f8005182f67..c6ef5d2890f 100644 --- a/src/iop/channelmixerrgb.c +++ b/src/iop/channelmixerrgb.c @@ -2224,7 +2224,7 @@ void process(dt_iop_module_t *self, dt_aligned_pixel_t correction_ratios; _get_d65_correction_ratios(self, correction_ratios); - if(find_illuminant_xy_from_as_shot_coeffs(&(self->dev->image_storage), correction_ratios, &(x), &(y))) + if(find_illuminant_xy_from_as_shot_coeffs(&(self->dev->image_storage), correction_ratios, &x, &y)) { // Convert illuminant from xyY to XYZ dt_aligned_pixel_t XYZ; @@ -2241,7 +2241,7 @@ void process(dt_iop_module_t *self, // which don't come from the EXIF this time). float x, y; - if(find_illuminant_xy_from_wb_coeffs(&(self->dev->image_storage), self->dev->chroma.wb_coeffs, &(x), &(y))) + if(find_illuminant_xy_from_wb_coeffs(&(self->dev->image_storage), self->dev->chroma.wb_coeffs, &x, &y)) { // Convert illuminant from xyY to XYZ dt_aligned_pixel_t XYZ; @@ -2352,7 +2352,7 @@ int process_cl(dt_iop_module_t *self, dt_aligned_pixel_t correction_ratios; _get_d65_correction_ratios(self, correction_ratios); - if(find_illuminant_xy_from_as_shot_coeffs(&(self->dev->image_storage), correction_ratios, &(x), &(y))) + if(find_illuminant_xy_from_as_shot_coeffs(&(self->dev->image_storage), correction_ratios, &x, &y)) { // Convert illuminant from xyY to XYZ dt_aligned_pixel_t XYZ; @@ -2369,7 +2369,7 @@ int process_cl(dt_iop_module_t *self, // which don't come from the EXIF this time). float x, y; - if(find_illuminant_xy_from_wb_coeffs(&(self->dev->image_storage), self->dev->chroma.wb_coeffs, &(x), &(y))) + if(find_illuminant_xy_from_wb_coeffs(&(self->dev->image_storage), self->dev->chroma.wb_coeffs, &x, &y)) { // Convert illuminant from xyY to XYZ dt_aligned_pixel_t XYZ; @@ -3183,7 +3183,7 @@ void commit_params(dt_iop_module_t *self, // if illuminant is from camera or user WB coefficients, x and y are set on-the-fly at // commit time, so we need to set adaptation too if(p->illuminant == DT_ILLUMINANT_CAMERA || p->illuminant == DT_ILLUMINANT_FROM_WB) - _check_if_close_to_daylight(x, y, NULL, NULL, &(d->adaptation)); + _check_if_close_to_daylight(x, y, NULL, NULL, &(d->adaptation)); d->illuminant_type = p->illuminant; // Convert illuminant from xyY to XYZ diff --git a/src/iop/temperature.c b/src/iop/temperature.c index 8ba68835a5e..636406a3532 100644 --- a/src/iop/temperature.c +++ b/src/iop/temperature.c @@ -1561,7 +1561,7 @@ void reload_defaults(dt_iop_module_t *self) dt_iop_temperature_params_t *d = self->default_params; dt_iop_temperature_params_t *p = self->params; - d->preset = p->preset = DT_IOP_TEMP_AS_SHOT; + d->preset = DT_IOP_TEMP_AS_SHOT; d->late_correction = p->late_correction = dt_is_scene_referred(); float *dcoeffs = (float *)d; @@ -1647,7 +1647,7 @@ void reload_defaults(dt_iop_module_t *self) STR_YESNO(another_cat_defined), daylights[0], daylights[1], daylights[2], as_shot[0], as_shot[1], as_shot[2]); - d->preset = p->preset = DT_IOP_TEMP_AS_SHOT; + d->preset = DT_IOP_TEMP_AS_SHOT; // White balance module doesn't need to be enabled for true_monochrome raws (like // for leica monochrom cameras). prepare_matrices is a noop as well, as there From 14b0925633a2278c154e3a77fe40d20f4a2688c3 Mon Sep 17 00:00:00 2001 From: Kofa Date: Sat, 19 Sep 2026 11:56:20 +0200 Subject: [PATCH 19/27] wb: removed _temp_array_from_params from reload_defaults, d's first 4 fields are the multipliers, aliased and set to 1 via dcoeffs, which matches the values used in the daylights declaration -> no-op; we won't crash --- src/iop/temperature.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/src/iop/temperature.c b/src/iop/temperature.c index 636406a3532..f967622a52a 100644 --- a/src/iop/temperature.c +++ b/src/iop/temperature.c @@ -1595,9 +1595,6 @@ void reload_defaults(dt_iop_module_t *self) double daylights[4] = {1.0, 1.0, 1.0, 1.0 }; double as_shot[4] = {1.0, 1.0, 1.0, 1.0 }; - // to have at least something and definitely not crash - _temp_array_from_params(daylights, d); - if(!_calculate_bogus_daylight_wb(self, daylights)) { // found camera matrix and used it to calculate bogus daylight wb @@ -1650,7 +1647,7 @@ void reload_defaults(dt_iop_module_t *self) d->preset = DT_IOP_TEMP_AS_SHOT; // White balance module doesn't need to be enabled for true_monochrome raws (like - // for leica monochrom cameras). prepare_matrices is a noop as well, as there + // for Leica 'Monochrom' cameras). prepare_matrices is a no-op as well, as there // isn't a color matrix, so we can skip that as well. if(!true_monochrome) @@ -1658,7 +1655,7 @@ void reload_defaults(dt_iop_module_t *self) if(self->gui_data) _prepare_matrices(self); - /* check if file is raw / hdr */ + /* check if file is color raw / sRaw */ if(is_raw) { // raw images need wb: From 90d817d3da283484ad3a0e0130ae7fa8a26a525b Mon Sep 17 00:00:00 2001 From: Kofa Date: Sat, 19 Sep 2026 12:24:36 +0200 Subject: [PATCH 20/27] wb: removed 2nd, redundant d->preset = DT_IOP_TEMP_AS_SHOT; from temperature.c reload_defaults --- src/iop/temperature.c | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/src/iop/temperature.c b/src/iop/temperature.c index f967622a52a..567345ed4a0 100644 --- a/src/iop/temperature.c +++ b/src/iop/temperature.c @@ -1572,7 +1572,7 @@ void reload_defaults(dt_iop_module_t *self) if(!self->dev || !dt_is_valid_imgid(self->dev->image_storage.id)) return; - const gboolean is_raw = + const gboolean is_color_raw = dt_image_is_matrix_correction_supported(&self->dev->image_storage); const gboolean true_monochrome = dt_image_monochrome_flags(&self->dev->image_storage) & DT_IMAGE_MONOCHROME; @@ -1593,7 +1593,6 @@ void reload_defaults(dt_iop_module_t *self) // we want these data in all cases to keep them in dev->chroma double daylights[4] = {1.0, 1.0, 1.0, 1.0 }; - double as_shot[4] = {1.0, 1.0, 1.0, 1.0 }; if(!_calculate_bogus_daylight_wb(self, daylights)) { @@ -1620,8 +1619,10 @@ void reload_defaults(dt_iop_module_t *self) } } + double as_shot[4] = {1.0, 1.0, 1.0, 1.0 }; + // Store EXIF WB coeffs - if(is_raw) + if(is_color_raw) { _find_coeffs(self, as_shot); as_shot[0] /= as_shot[1]; @@ -1644,8 +1645,6 @@ void reload_defaults(dt_iop_module_t *self) STR_YESNO(another_cat_defined), daylights[0], daylights[1], daylights[2], as_shot[0], as_shot[1], as_shot[2]); - d->preset = DT_IOP_TEMP_AS_SHOT; - // White balance module doesn't need to be enabled for true_monochrome raws (like // for Leica 'Monochrom' cameras). prepare_matrices is a no-op as well, as there // isn't a color matrix, so we can skip that as well. @@ -1656,7 +1655,7 @@ void reload_defaults(dt_iop_module_t *self) _prepare_matrices(self); /* check if file is color raw / sRaw */ - if(is_raw) + if(is_color_raw) { // raw images need wb: self->default_enabled = TRUE; From 0ed22e7eff321f8f631bc3b8fe711e7d7f4c1114 Mon Sep 17 00:00:00 2001 From: Kofa Date: Sun, 20 Sep 2026 09:12:15 +0200 Subject: [PATCH 21/27] wb: only enable late_correction for color raw by default --- src/iop/temperature.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/iop/temperature.c b/src/iop/temperature.c index 567345ed4a0..f12b6557cb6 100644 --- a/src/iop/temperature.c +++ b/src/iop/temperature.c @@ -1562,7 +1562,7 @@ void reload_defaults(dt_iop_module_t *self) dt_iop_temperature_params_t *p = self->params; d->preset = DT_IOP_TEMP_AS_SHOT; - d->late_correction = p->late_correction = dt_is_scene_referred(); + d->late_correction = p->late_correction = FALSE; float *dcoeffs = (float *)d; for_four_channels(k) @@ -1651,6 +1651,8 @@ void reload_defaults(dt_iop_module_t *self) if(!true_monochrome) { + // set up the matrices gui_data->XYZ_to_CAM, gui_data->CAM_to_XYZ, + // used for temp/tint adjustments if(self->gui_data) _prepare_matrices(self); @@ -1678,7 +1680,6 @@ void reload_defaults(dt_iop_module_t *self) dcoeffs[2] = coeffs[2]/coeffs[1]; dcoeffs[3] = coeffs[3]/coeffs[1]; dcoeffs[1] = 1.0f; - d->late_correction = p->late_correction = FALSE; } } } From adf3fa44530ec325fba8429fbf09df57a8de6832 Mon Sep 17 00:00:00 2001 From: Kofa Date: Sun, 20 Sep 2026 10:57:28 +0200 Subject: [PATCH 22/27] wb: read chr->adaptation->enabled via snapshot for thread safety --- src/iop/temperature.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/src/iop/temperature.c b/src/iop/temperature.c index f12b6557cb6..163be39f8e0 100644 --- a/src/iop/temperature.c +++ b/src/iop/temperature.c @@ -1199,8 +1199,14 @@ static void _update_preset(dt_iop_module_t *self, int mode) if(is_current_reference && is_new_mode_manual) { - // set iff color calibration active in adaptation mode - p->late_correction = (chr->adaptation != NULL); + // snapshot is NEEDED: dt_dev_reset_chroma() may NULL chr->adaptation from + // the pipe worker thread; modules are only freed on this (GTK) thread, + // so dereference is safe if not NULL + const dt_iop_module_t *const cat = chr->adaptation; + // set if and only if color calibration is registered as CAT handler and enabled + // (whether it is in adaptation mode is not testable here: channelmixerrgb's + // params are private) + p->late_correction = (cat != NULL) && cat->enabled; } p->preset = mode; @@ -1581,6 +1587,8 @@ void reload_defaults(dt_iop_module_t *self) if(!dt_is_scene_referred()) { + // can't use dev->chroma.adaptation: this runs before history replay, and + // it is never set without a GUI another_cat_defined = dt_history_check_module_exists(self->dev->image_storage.id, "channelmixerrgb", TRUE); From f7ac8cb931610377e655362ce1946e262a3c9a1a Mon Sep 17 00:00:00 2001 From: Kofa Date: Sun, 20 Sep 2026 14:43:04 +0200 Subject: [PATCH 23/27] wb: add a proxy to allow temperature.c to determine whether CAT is enabled; the previous "(cat != NULL) && cat->enabled" check did not know anything about what channelmixerrgb.c was doing. --- src/develop/develop.c | 20 ++++++++++++++++++++ src/develop/develop.h | 13 ++++++++++--- src/iop/channelmixerrgb.c | 28 +++++++++++++++++++++++----- src/iop/temperature.c | 9 +-------- 4 files changed, 54 insertions(+), 16 deletions(-) diff --git a/src/develop/develop.c b/src/develop/develop.c index 0d578a2d404..d0f552b526c 100644 --- a/src/develop/develop.c +++ b/src/develop/develop.c @@ -3753,6 +3753,26 @@ void dt_dev_exposure_handle_event(int n_press, gdouble delta, darktable.develop->proxy.exposure.handle_event(n_press, delta, state, is_blackpoint); } +gboolean dt_dev_cat_is_active(dt_develop_t *dev) +{ + // the proxy function pointer is set by the first color calibration gui_init(), + // so it stays NULL outside the darkroom + if(!dev || !dev->proxy.cat_is_active) + return FALSE; + + for(const GList *modules = dev->iop; modules; modules = g_list_next(modules)) + { + dt_iop_module_t *mod = modules->data; + + if(dt_iop_module_is(mod, "channelmixerrgb") + && mod->iop_order != INT_MAX + && dev->proxy.cat_is_active(mod)) + return TRUE; + } + + return FALSE; +} + void dt_dev_modulegroups_set(dt_develop_t *dev, const uint32_t group) { diff --git a/src/develop/develop.h b/src/develop/develop.h index 6d434700a12..cda654cb37d 100644 --- a/src/develop/develop.h +++ b/src/develop/develop.h @@ -259,11 +259,14 @@ typedef struct dt_develop_t /* proxy for communication between plugins and develop/darkroom */ struct { - // list of exposure iop instances, with plugin hooks, used by - // histogram dragging functions each element is - // dt_dev_proxy_exposure_t + // exposure iop hooks, used by histogram dragging functions dt_dev_proxy_exposure_t exposure; + // channelmixerrgb reports whether it is actually applying CAT; + // temperature asks through dt_dev_cat_is_active() + // thread contract: must only be called from the GTK main thread + gboolean (*cat_is_active)(struct dt_iop_module_t *cat); + // this module receives right-drag events if not already claimed struct dt_iop_module_t *rotate; @@ -534,6 +537,10 @@ void dt_dev_exposure_handle_event(int n_press, gdouble delta, GdkModifierType state, const gboolean is_blackpoint); +/** TRUE if a live color calibration instance is actually applying CAT. + GTK main thread only: the accessor reads params. */ +gboolean dt_dev_cat_is_active(dt_develop_t *dev); + /* * modulegroups plugin hooks */ diff --git a/src/iop/channelmixerrgb.c b/src/iop/channelmixerrgb.c index c6ef5d2890f..da154672f16 100644 --- a/src/iop/channelmixerrgb.c +++ b/src/iop/channelmixerrgb.c @@ -2003,9 +2003,19 @@ static void _validate_color_checker(const float *const restrict in, dt_free_align(patches); } -static void _set_trouble_messages(dt_iop_module_t *self) +static gboolean _applies_cat(const dt_iop_module_t *self) { + if(!self->enabled) + return FALSE; + const dt_iop_channelmixer_rgb_params_t *p = self->params; + + return !(p->illuminant == DT_ILLUMINANT_PIPE || p->adaptation == DT_ADAPTATION_RGB) + && !dt_image_is_monochrome(&self->dev->image_storage); +} + +static void _set_trouble_messages(dt_iop_module_t *self) +{ const dt_iop_channelmixer_rgb_gui_data_t *g = self->gui_data; const dt_develop_t *dev = self->dev; const dt_dev_chroma_t *chr = &dev->chroma; @@ -2035,10 +2045,7 @@ static void _set_trouble_messages(dt_iop_module_t *self) return; } - const gboolean valid = - self->enabled - && !(p->illuminant == DT_ILLUMINANT_PIPE || p->adaptation == DT_ADAPTATION_RGB) - && !dt_image_is_monochrome(&dev->image_storage); + const gboolean valid = _applies_cat(self); const gboolean temperature_enabled = temperature->enabled; const gboolean adaptation_enabled = adaptation->enabled; @@ -4508,6 +4515,15 @@ void color_picker_apply(dt_iop_module_t *self, _auto_set_illuminant(self, pipe); } +// TRUE if this instance is applying chromatic adaptation. reads params, so +// GTK main thread only +static gboolean _cat_proxy_is_active(dt_iop_module_t *self) +{ + if(!self || !self->dev) + return FALSE; + + return _applies_cat(self); +} void gui_init(dt_iop_module_t *self) { @@ -4842,6 +4858,8 @@ void gui_init(dt_iop_module_t *self) dt_gui_box_add(g->cs.container, g->checkers_list, g->optimize, g->safety, g->label_delta_E, dt_gui_hbox(dt_gui_align_right(g->button_validate), g->button_profile, g->button_commit)); + + darktable.develop->proxy.cat_is_active = _cat_proxy_is_active; } void gui_cleanup(dt_iop_module_t *self) diff --git a/src/iop/temperature.c b/src/iop/temperature.c index 163be39f8e0..84e2e42bafc 100644 --- a/src/iop/temperature.c +++ b/src/iop/temperature.c @@ -1199,14 +1199,7 @@ static void _update_preset(dt_iop_module_t *self, int mode) if(is_current_reference && is_new_mode_manual) { - // snapshot is NEEDED: dt_dev_reset_chroma() may NULL chr->adaptation from - // the pipe worker thread; modules are only freed on this (GTK) thread, - // so dereference is safe if not NULL - const dt_iop_module_t *const cat = chr->adaptation; - // set if and only if color calibration is registered as CAT handler and enabled - // (whether it is in adaptation mode is not testable here: channelmixerrgb's - // params are private) - p->late_correction = (cat != NULL) && cat->enabled; + p->late_correction = dt_dev_cat_is_active(self->dev); } p->preset = mode; From 6e76ff8b8f2eea94d538bd6f0e63ab0899c5900b Mon Sep 17 00:00:00 2001 From: Kofa Date: Sun, 20 Sep 2026 17:07:58 +0200 Subject: [PATCH 24/27] wb: prepare to store per-pipe WB settings: introduce dt_dev_wb_t --- dev-doc/IOP_Module_API.md | 2 +- dev-doc/Module_Lifecycle.md | 6 ++-- dev-doc/pixelpipe_architecture.md | 4 +-- src/common/illuminants.h | 2 +- src/develop/develop.c | 17 +++++++---- src/develop/develop.h | 6 ++-- src/develop/format.h | 10 +++++++ src/develop/pixelpipe_hb.c | 1 + src/develop/pixelpipe_hb.h | 5 ++++ src/iop/channelmixerrgb.c | 22 +++++++------- src/iop/colorin.c | 16 +++++------ src/iop/highlights.c | 16 +++++------ src/iop/hlreconstruct/opposed.c | 12 ++++---- src/iop/hlreconstruct/segbased.c | 6 ++-- src/iop/temperature.c | 48 +++++++++++++++---------------- 15 files changed, 98 insertions(+), 75 deletions(-) diff --git a/dev-doc/IOP_Module_API.md b/dev-doc/IOP_Module_API.md index 0200373694e..3776bf7f669 100644 --- a/dev-doc/IOP_Module_API.md +++ b/dev-doc/IOP_Module_API.md @@ -511,7 +511,7 @@ Common checks: `dt_image_is_raw()`, `dt_image_is_hdr()`, `dt_image_is_ldr()`, `d Some modules use `reload_defaults()` to populate shared state in `dev->chroma` (or similar structures) that other modules depend on. This happens regardless of whether the module is enabled or has a history entry. -Example: `temperature.c` always computes the camera's as-shot WB coefficients and D65 reference coefficients from the image's EXIF data and writes them into `dev->chroma.as_shot[]` and `dev->chroma.D65coeffs[]`. `channelmixerrgb.c` reads these values during its own `commit_params()` to perform chromatic adaptation. If `reload_defaults()` did not run unconditionally, `channelmixerrgb` would have no access to the camera's native WB — even when the white balance module itself is disabled or absent from the history. +Example: `temperature.c` always computes the camera's as-shot WB coefficients and D65 reference coefficients from the image's EXIF data and writes them into `dev->chroma.wb.as_shot[]` and `dev->chroma.wb.D65coeffs[]`. `channelmixerrgb.c` reads these values during its own `commit_params()` to perform chromatic adaptation. If `reload_defaults()` did not run unconditionally, `channelmixerrgb` would have no access to the camera's native WB — even when the white balance module itself is disabled or absent from the history. For the complete producer/consumer map, the reverse-order default-loading reasoning, and which `dev->chroma` fields each module actually reads, see [iop/wb_and_colorcalibration](iop/wb_and_colorcalibration/README.md). diff --git a/dev-doc/Module_Lifecycle.md b/dev-doc/Module_Lifecycle.md index 044e95233f8..d3b230db92c 100644 --- a/dev-doc/Module_Lifecycle.md +++ b/dev-doc/Module_Lifecycle.md @@ -151,13 +151,13 @@ has saved history: - Clear the scratch `memory.history` table. - **`dt_dev_reset_chroma()`** clears part of the shared WB state ([section 5](#5-pipeline-ordering-asymmetry) lists exactly which fields): `temperature` and - `adaptation` become `NULL`, and `wb_coeffs[]` is reset to 1.0. + `adaptation` become `NULL`, and `wb.coeffs[]` is reset to 1.0. - **`_dt_dev_load_pipeline_defaults()`** calls `dt_iop_reload_defaults()` on every module in **reverse** pipe order ([section 5](#5-pipeline-ordering-asymmetry) explains the direction). This sets each module's image-specific `default_params`. Because it goes through the wrapper, it also copies them into `params` (via `dt_iop_load_default_params()`). Temperature additionally populates - `dev->chroma.as_shot[]` and `D65coeffs[]` as a side effect. + `dev->chroma.wb.as_shot[]` and `D65coeffs[]` as a side effect. - **`_dev_add_default_modules()`** prepends the workflow-mandated modules into the in-memory history. - **`_dev_auto_apply_presets()`** applies auto-presets. It is gated on the @@ -311,7 +311,7 @@ re-commits after the replay. There is no signal; synchronization is implicit and ordered by iop_order. Because temperature is upstream of channelmixerrgb, a full `synch_all` runs `commit_params()` in pipe order: -`temperature.commit_params()` writes `dev->chroma.wb_coeffs[]` first and +`temperature.commit_params()` writes `dev->chroma.wb.coeffs[]` first and `channelmixerrgb.commit_params()` reads it afterward to build its chromatic-adaptation matrix. (`synch_top` re-commits only the topmost history item, so it is not evidence that an upstream producer has just run.) The full producer/consumer field map, the once-per-load `reload_defaults` diff --git a/dev-doc/pixelpipe_architecture.md b/dev-doc/pixelpipe_architecture.md index 306efb95598..0aa3996e687 100644 --- a/dev-doc/pixelpipe_architecture.md +++ b/dev-doc/pixelpipe_architecture.md @@ -136,9 +136,9 @@ Two key pipeline operations iterate modules in **different** orders: - **`commit_params()`** runs in **forward** pipe order (e.g., temperature before channelmixerrgb). This is the normal processing direction. - **`_dt_dev_load_pipeline_defaults()`** runs in **reverse** pipe order (e.g., channelmixerrgb before temperature). This happens during history reset and default loading. -This asymmetry matters for modules that communicate via shared state. For example, `temperature.c` writes white balance coefficients into `dev->chroma.wb_coeffs`, and `channelmixerrgb.c` reads them during `commit_params()`. During forward processing, temperature commits first and the data is available. During reverse-order default loading, channelmixerrgb runs first — before temperature has refreshed its values — so any shared state a `reload_defaults()` depends on must be reset to a neutral value beforehand. +This asymmetry matters for modules that communicate via shared state. For example, `temperature.c` writes white balance coefficients into `dev->chroma.wb.coeffs`, and `channelmixerrgb.c` reads them during `commit_params()`. During forward processing, temperature commits first and the data is available. During reverse-order default loading, channelmixerrgb runs first — before temperature has refreshed its values — so any shared state a `reload_defaults()` depends on must be reset to a neutral value beforehand. -**Consequence:** Shared state (like `dev->chroma`) must be reset before reverse-order iteration. The framework does this via `dt_dev_reset_chroma()` immediately before `_dt_dev_load_pipeline_defaults()`; the neutral `wb_coeffs` it writes is what keeps the dependent defaults image-local. For the full white-balance ↔ color-calibration interaction, see [iop/wb_and_colorcalibration](iop/wb_and_colorcalibration/README.md). +**Consequence:** Shared state (like `dev->chroma`) must be reset before reverse-order iteration. The framework does this via `dt_dev_reset_chroma()` immediately before `_dt_dev_load_pipeline_defaults()`; the neutral `wb.coeffs` it writes is what keeps the dependent defaults image-local. For the full white-balance ↔ color-calibration interaction, see [iop/wb_and_colorcalibration](iop/wb_and_colorcalibration/README.md). ## Introspection Connection diff --git a/src/common/illuminants.h b/src/common/illuminants.h index 42f27f19d04..dcb85f8f7ae 100644 --- a/src/common/illuminants.h +++ b/src/common/illuminants.h @@ -484,7 +484,7 @@ static gboolean find_illuminant_xy_from_as_shot_coeffs(const dt_image_t *img, co // Adapt the camera coeffs with custom D65 coefficients if provided ('caveats' workaround) // this can deal with WB coeffs that don't use the input matrix reference - // correction_ratios[k] = chr->D65coeffs[k] / chr->wb_coeffs[k] + // correction_ratios[k] = chr->wb.D65coeffs[k] / chr->wb.coeffs[k] if(correction_ratios) for(size_t k = 0; k < 4; k++) WB[k] *= correction_ratios[k]; // for a neutral surface, raw RGB * img->wb_coeffs would produce neutral R=G=B diff --git a/src/develop/develop.c b/src/develop/develop.c index d0f552b526c..0a15902a4a7 100644 --- a/src/develop/develop.c +++ b/src/develop/develop.c @@ -4379,19 +4379,26 @@ void dt_dev_reset_chroma(dt_develop_t *dev) chr->adaptation = NULL; chr->temperature = NULL; for_four_channels(c) - chr->wb_coeffs[c] = 1.0f; + chr->wb.coeffs[c] = 1.0f; +} + +void dt_dev_wb_set_neutral(dt_dev_wb_t *wb) +{ + wb->late_correction = FALSE; + for_four_channels(c) + { + wb->coeffs[c] = 1.0f; + wb->D65coeffs[c] = 1.0; + } } void dt_dev_init_chroma(dt_develop_t *dev) { dt_dev_reset_chroma(dev); dt_dev_chroma_t *chr = &dev->chroma; - chr->late_correction = FALSE; + dt_dev_wb_set_neutral(&chr->wb); for_four_channels(c) - { - chr->D65coeffs[c] = 1.0; chr->as_shot[c] = 1.0; - } } // clang-format off diff --git a/src/develop/develop.h b/src/develop/develop.h index cda654cb37d..3e07758953b 100644 --- a/src/develop/develop.h +++ b/src/develop/develop.h @@ -176,10 +176,8 @@ typedef struct dt_dev_chroma_t struct dt_iop_module_t *temperature; // always available for GUI reports struct dt_iop_module_t *adaptation; // set if one module is processing this without blending - dt_aligned_pixel_t wb_coeffs; // coeffs actually set by temperature - double D65coeffs[4]; // both read from exif data or "best guess" double as_shot[4]; - gboolean late_correction; + dt_dev_wb_t wb; } dt_dev_chroma_t; typedef struct dt_develop_t @@ -695,6 +693,8 @@ static inline struct dt_iop_module_t *dt_dev_gui_module(void) return darktable.develop ? darktable.develop->gui_module : NULL; } +void dt_dev_wb_set_neutral(dt_dev_wb_t *wb); + G_END_DECLS // clang-format off diff --git a/src/develop/format.h b/src/develop/format.h index 6536f66d8fc..66ead084663 100644 --- a/src/develop/format.h +++ b/src/develop/format.h @@ -26,6 +26,16 @@ struct dt_dev_pixelpipe_iop_t; struct dt_dev_pixelpipe_t; struct dt_iop_module_t; +// white balance state: what temperature applied, plus the fixed per-image +// references it is measured against +typedef struct dt_dev_wb_t +{ + dt_aligned_pixel_t coeffs; // coeffs actually set by temperature + // we want processing code never to read dev->chroma, so keep it here + double D65coeffs[4]; // both read from exif data or "best guess" + gboolean late_correction; +} dt_dev_wb_t; + typedef enum dt_iop_buffer_type_t { TYPE_UNKNOWN, TYPE_FLOAT, diff --git a/src/develop/pixelpipe_hb.c b/src/develop/pixelpipe_hb.c index 4d2769bb96e..72d73f55f95 100644 --- a/src/develop/pixelpipe_hb.c +++ b/src/develop/pixelpipe_hb.c @@ -325,6 +325,7 @@ static gboolean _dev_pixelpipe_init_cached(dt_dev_pixelpipe_t *pipe, memset(pipe->mask_distort_buf, 0, sizeof(pipe->mask_distort_buf)); memset(pipe->mask_distort_buf_size, 0, sizeof(pipe->mask_distort_buf_size)); pipe->mask_cache_size = 0; + dt_dev_wb_set_neutral(&pipe->wb); return dt_dev_pixelpipe_cache_init(pipe, entries, size, fraction); } diff --git a/src/develop/pixelpipe_hb.h b/src/develop/pixelpipe_hb.h index 9fd02c31395..9dc0d8baee1 100644 --- a/src/develop/pixelpipe_hb.h +++ b/src/develop/pixelpipe_hb.h @@ -198,6 +198,11 @@ typedef struct dt_dev_pixelpipe_t // and should be modified by process*(), if necessary. dt_iop_buffer_dsc_t dsc; + // white balance of the generation this pipe is rendering, written by + // temperature's commit_params. dev->chroma is shared by all pipes and can + // change while this one runs, so modules must read it from here, not from dev + dt_dev_wb_t wb; + /** work profile info of the image */ struct dt_iop_order_iccprofile_info_t *work_profile_info; /** input profile info **/ diff --git a/src/iop/channelmixerrgb.c b/src/iop/channelmixerrgb.c index da154672f16..5d6e8abdfd9 100644 --- a/src/iop/channelmixerrgb.c +++ b/src/iop/channelmixerrgb.c @@ -594,7 +594,7 @@ void init_presets(dt_iop_module_so_t *self) static gboolean _dev_is_D65_chroma(const dt_develop_t *dev) { const dt_dev_chroma_t *chr = &dev->chroma; - return chr->late_correction || dt_dev_equal_chroma(chr->wb_coeffs, chr->D65coeffs); + return chr->wb.late_correction || dt_dev_equal_chroma(chr->wb.coeffs, chr->wb.D65coeffs); } static gboolean _area_mapping_active(const dt_iop_channelmixer_rgb_gui_data_t *g) @@ -627,10 +627,10 @@ static gboolean _get_d65_correction_ratios(const dt_iop_module_t *self, return FALSE; const gboolean valid_chroma = - chr->D65coeffs[0] > 0.0 && chr->D65coeffs[1] > 0.0 && chr->D65coeffs[2] > 0.0; + chr->wb.D65coeffs[0] > 0.0 && chr->wb.D65coeffs[1] > 0.0 && chr->wb.D65coeffs[2] > 0.0; const gboolean changed_chroma = - chr->wb_coeffs[0] > 1.0f || chr->wb_coeffs[1] > 1.0f || chr->wb_coeffs[2] > 1.0f; + chr->wb.coeffs[0] > 1.0f || chr->wb.coeffs[1] > 1.0f || chr->wb.coeffs[2] > 1.0f; // Otherwise - for example because the user made a correct preset, find the // WB adaptation ratio @@ -638,8 +638,8 @@ static gboolean _get_d65_correction_ratios(const dt_iop_module_t *self, { for_four_channels(k) { - if(chr->wb_coeffs[k] > 1e-6f) - out_correction_ratios[k] = chr->D65coeffs[k] / chr->wb_coeffs[k]; + if(chr->wb.coeffs[k] > 1e-6f) + out_correction_ratios[k] = chr->wb.D65coeffs[k] / chr->wb.coeffs[k]; else out_correction_ratios[k] = 1.0f; } @@ -657,7 +657,7 @@ static void _get_corrected_illuminant_xy(const dt_iop_module_t *self, if(p->illuminant == DT_ILLUMINANT_FROM_WB) { for(int k = 0; k < 4; k++) - wb_coeffs[k] = self->dev->chroma.wb_coeffs[k]; + wb_coeffs[k] = self->dev->chroma.wb.coeffs[k]; } illuminant_to_xy(p->illuminant, &(self->dev->image_storage), correction_ratios, wb_coeffs, x, y, p->temperature, p->illum_fluo, @@ -2248,7 +2248,7 @@ void process(dt_iop_module_t *self, // which don't come from the EXIF this time). float x, y; - if(find_illuminant_xy_from_wb_coeffs(&(self->dev->image_storage), self->dev->chroma.wb_coeffs, &x, &y)) + if(find_illuminant_xy_from_wb_coeffs(&(self->dev->image_storage), self->dev->chroma.wb.coeffs, &x, &y)) { // Convert illuminant from xyY to XYZ dt_aligned_pixel_t XYZ; @@ -2376,7 +2376,7 @@ int process_cl(dt_iop_module_t *self, // which don't come from the EXIF this time). float x, y; - if(find_illuminant_xy_from_wb_coeffs(&(self->dev->image_storage), self->dev->chroma.wb_coeffs, &x, &y)) + if(find_illuminant_xy_from_wb_coeffs(&(self->dev->image_storage), self->dev->chroma.wb.coeffs, &x, &y)) { // Convert illuminant from xyY to XYZ dt_aligned_pixel_t XYZ; @@ -3097,7 +3097,7 @@ static void _preview_pipe_finished_callback(gpointer instance, dt_iop_module_t * // WB coefficients might have changed temperature.c; recalculate xy and update the GUI float x, y; - if(find_illuminant_xy_from_wb_coeffs(&(self->dev->image_storage), self->dev->chroma.wb_coeffs, &x, &y)) + if(find_illuminant_xy_from_wb_coeffs(&(self->dev->image_storage), self->dev->chroma.wb.coeffs, &x, &y)) { DT_ENTER_GUI_UPDATE(); _update_approx_cct(self); @@ -4087,7 +4087,7 @@ void gui_changed(dt_iop_module_t *self, // (x, y) were computed at runtime from the WB module's coefficients // and p->x, p->y may hold stale values. Recompute them now so // the new illuminant mode starts from the correct chromaticity. - find_illuminant_xy_from_wb_coeffs(&(self->dev->image_storage), self->dev->chroma.wb_coeffs, + find_illuminant_xy_from_wb_coeffs(&(self->dev->image_storage), self->dev->chroma.wb.coeffs, &(p->x), &(p->y)); _check_if_close_to_daylight(p->x, p->y, &(p->temperature), NULL, &(p->adaptation)); } @@ -4114,7 +4114,7 @@ void gui_changed(dt_iop_module_t *self, else if(p->illuminant == DT_ILLUMINANT_FROM_WB) { const gboolean found = find_illuminant_xy_from_wb_coeffs(&(self->dev->image_storage), - self->dev->chroma.wb_coeffs, &(p->x), &(p->y)); + self->dev->chroma.wb.coeffs, &(p->x), &(p->y)); _check_if_close_to_daylight(p->x, p->y, &(p->temperature), NULL, &(p->adaptation)); if(found) diff --git a/src/iop/colorin.c b/src/iop/colorin.c index ad896d36f35..8b08df731c9 100644 --- a/src/iop/colorin.c +++ b/src/iop/colorin.c @@ -661,12 +661,12 @@ int process_cl(dt_iop_module_t *self, } const dt_dev_chroma_t *chr = &self->dev->chroma; - const gboolean corrected = chr->late_correction; + const gboolean corrected = chr->wb.late_correction; dt_aligned_pixel_t coeffs; for_four_channels(k) { - if(corrected && chr->wb_coeffs[k] > 1e-6f) - coeffs[k] = chr->D65coeffs[k] / chr->wb_coeffs[k]; + if(corrected && chr->wb.coeffs[k] > 1e-6f) + coeffs[k] = chr->wb.D65coeffs[k] / chr->wb.coeffs[k]; else coeffs[k] = 1.0f; } @@ -675,7 +675,7 @@ int process_cl(dt_iop_module_t *self, { for_four_channels(k) { - pipe->dsc.temperature.coeffs[k] = chr->D65coeffs[k]; + pipe->dsc.temperature.coeffs[k] = chr->wb.D65coeffs[k]; // note: tiling takes care about processed_maximum pipe->dsc.processed_maximum[k] *= coeffs[k]; } @@ -1212,12 +1212,12 @@ void process(dt_iop_module_t *self, const dt_dev_chroma_t *chr = &self->dev->chroma; const dt_iop_colorin_data_t *const d = piece->data; - const gboolean corrected = chr->late_correction && d->type != DT_COLORSPACE_LAB; + const gboolean corrected = chr->wb.late_correction && d->type != DT_COLORSPACE_LAB; dt_aligned_pixel_t coeffs; for_four_channels(k) { - if(corrected && chr->wb_coeffs[k] > 1e-6f) - coeffs[k] = chr->D65coeffs[k] / chr->wb_coeffs[k]; + if(corrected && chr->wb.coeffs[k] > 1e-6f) + coeffs[k] = chr->wb.D65coeffs[k] / chr->wb.coeffs[k]; else coeffs[k] = 1.0f; } @@ -1227,7 +1227,7 @@ void process(dt_iop_module_t *self, { for_four_channels(k) { - pipe->dsc.temperature.coeffs[k] = chr->D65coeffs[k]; + pipe->dsc.temperature.coeffs[k] = chr->wb.D65coeffs[k]; // note: tiling takes care about processed_maximum pipe->dsc.processed_maximum[k] *= coeffs[k]; } diff --git a/src/iop/highlights.c b/src/iop/highlights.c index d87d12bb07a..ad2c40612a9 100644 --- a/src/iop/highlights.c +++ b/src/iop/highlights.c @@ -677,11 +677,11 @@ int process_cl(dt_iop_module_t *self, { const dt_dev_chroma_t *chr = &self->dev->chroma; dt_aligned_pixel_t clips = { clipper, clipper, clipper, clipper}; - if(pipe->dsc.temperature.enabled && chr->late_correction) + if(pipe->dsc.temperature.enabled && chr->wb.late_correction) { - clips[0] *= chr->wb_coeffs[0] / chr->D65coeffs[0]; - clips[1] *= chr->wb_coeffs[1] / chr->D65coeffs[1]; - clips[2] *= chr->wb_coeffs[2] / chr->D65coeffs[2]; + clips[0] *= chr->wb.coeffs[0] / chr->wb.D65coeffs[0]; + clips[1] *= chr->wb.coeffs[1] / chr->wb.D65coeffs[1]; + clips[2] *= chr->wb.coeffs[2] / chr->wb.D65coeffs[2]; } dev_xtrans = dt_opencl_copy_host_to_device_constant(devid, sizeof(piece->xtrans), piece->xtrans); @@ -756,11 +756,11 @@ static void process_clip(dt_iop_module_t *self, const uint8_t(*const xtrans)[6] = piece->xtrans; const dt_dev_chroma_t *chr = &self->dev->chroma; dt_aligned_pixel_t clips = { clip, clip, clip, clip}; - if(piece->pipe->dsc.temperature.enabled && chr->late_correction) + if(piece->pipe->dsc.temperature.enabled && chr->wb.late_correction) { - clips[0] *= chr->wb_coeffs[0] / chr->D65coeffs[0]; - clips[1] *= chr->wb_coeffs[1] / chr->D65coeffs[1]; - clips[2] *= chr->wb_coeffs[2] / chr->D65coeffs[2]; + clips[0] *= chr->wb.coeffs[0] / chr->wb.D65coeffs[0]; + clips[1] *= chr->wb.coeffs[1] / chr->wb.D65coeffs[1]; + clips[2] *= chr->wb.coeffs[2] / chr->wb.D65coeffs[2]; } for(int row = 0; row < roi_out->height; row++) { diff --git a/src/iop/hlreconstruct/opposed.c b/src/iop/hlreconstruct/opposed.c index d459ff0e6ff..31cf7321d00 100644 --- a/src/iop/hlreconstruct/opposed.c +++ b/src/iop/hlreconstruct/opposed.c @@ -239,12 +239,12 @@ static float *_process_opposed(dt_iop_module_t *self, const dt_aligned_pixel_t clips = { clipval * icoeffs[0], clipval * icoeffs[1], clipval * icoeffs[2]}; const dt_dev_chroma_t *chr = &self->dev->chroma; - const gboolean late = chr->late_correction; + const gboolean late = chr->wb.late_correction; 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]; + if(late && chr->wb.coeffs[k] > 1e-6f) + correction[k] = chr->wb.D65coeffs[k] / chr->wb.coeffs[k]; else correction[k] = 1.0f; } @@ -453,12 +453,12 @@ static cl_int process_opposed_cl(dt_iop_module_t *self, dt_aligned_pixel_t clips = { clipval * icoeffs[0], clipval * icoeffs[1], clipval * icoeffs[2], 1.0f}; const dt_dev_chroma_t *chr = &self->dev->chroma; - const gboolean late = chr->late_correction; + const gboolean late = chr->wb.late_correction; 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]; + if(late && chr->wb.coeffs[k] > 1e-6f) + correction[k] = chr->wb.D65coeffs[k] / chr->wb.coeffs[k]; else correction[k] = 1.0f; } diff --git a/src/iop/hlreconstruct/segbased.c b/src/iop/hlreconstruct/segbased.c index 20d33e117d3..895681f5f79 100644 --- a/src/iop/hlreconstruct/segbased.c +++ b/src/iop/hlreconstruct/segbased.c @@ -465,12 +465,12 @@ static void _process_segmentation(dt_dev_pixelpipe_iop_t *piece, const dt_aligned_pixel_t cube_coeffs = {cbrtf(clips[0]), cbrtf(clips[1]), cbrtf(clips[2]), 0.0f}; const dt_dev_chroma_t *chr = &piece->module->dev->chroma; - const gboolean late = chr->late_correction; + const gboolean late = chr->wb.late_correction; 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]; + if(late && chr->wb.coeffs[k] > 1e-6f) + correction[k] = chr->wb.D65coeffs[k] / chr->wb.coeffs[k]; else correction[k] = 1.0f; } diff --git a/src/iop/temperature.c b/src/iop/temperature.c index 84e2e42bafc..0b16f275238 100644 --- a/src/iop/temperature.c +++ b/src/iop/temperature.c @@ -555,9 +555,9 @@ static inline void _publish_chroma(dt_dev_pixelpipe_iop_t *piece) piece->pipe->dsc.temperature.coeffs[k] = d->coeffs[k]; piece->pipe->dsc.processed_maximum[k] = d->coeffs[k] * piece->pipe->dsc.processed_maximum[k]; - chr->wb_coeffs[k] = d->coeffs[k]; + chr->wb.coeffs[k] = d->coeffs[k]; } - chr->late_correction = d->late_correction; + chr->wb.late_correction = d->late_correction; } void process(dt_iop_module_t *self, @@ -732,7 +732,7 @@ void commit_params(dt_iop_module_t *self, if(self->hide_enable_button) { for_four_channels(k) - chr->wb_coeffs[k] = 1.0f; + chr->wb.coeffs[k] = 1.0f; // keep the module handle available for GUI reports (see below) chr->temperature = self; return; @@ -741,7 +741,7 @@ void commit_params(dt_iop_module_t *self, for_four_channels(k) { d->coeffs[k] = tcoeffs[k]; - chr->wb_coeffs[k] = piece->enabled ? d->coeffs[k] : 1.0f; + chr->wb.coeffs[k] = piece->enabled ? d->coeffs[k] : 1.0f; } // 4Bayer images not implemented in OpenCL yet @@ -759,7 +759,7 @@ void commit_params(dt_iop_module_t *self, // balanced. Do not ask colorin (or any other consumer) to "finish" a // correction that never started, otherwise disabling WB would still pull the // image to D65 via D65coeffs/1.0. - chr->late_correction = piece->enabled ? effective_late_correction : FALSE; + chr->wb.late_correction = piece->enabled ? effective_late_correction : FALSE; /* Always publish the module handle for GUI reports, regardless of the enabled state. The enabled state is tracked separately via @@ -987,9 +987,9 @@ static void _color_rgb_sliders(dt_iop_module_t *self) //real (ish) //we consider daylight wb to be "reference white" const double white[3] = { - 1.0 / chr->D65coeffs[0], - 1.0 / chr->D65coeffs[1], - 1.0 / chr->D65coeffs[2], + 1.0 / chr->wb.D65coeffs[0], + 1.0 / chr->wb.D65coeffs[1], + 1.0 / chr->wb.D65coeffs[2], }; const float rchanmul = dt_bauhaus_slider_get(g->scale_r); @@ -1002,8 +1002,8 @@ static void _color_rgb_sliders(dt_iop_module_t *self) dt_bauhaus_slider_set_stop (g->scale_r, 0.0, white[0]*0.0, white[1]*gchanmul, white[2]*bchanmul); dt_bauhaus_slider_set_stop - (g->scale_r, chr->D65coeffs[0]/rchanmulmax, - white[0]*chr->D65coeffs[0], white[1]*gchanmul, white[2]*bchanmul); + (g->scale_r, chr->wb.D65coeffs[0]/rchanmulmax, + white[0]*chr->wb.D65coeffs[0], white[1]*gchanmul, white[2]*bchanmul); dt_bauhaus_slider_set_stop (g->scale_r, 1.0, white[0]*1.0, white[1]*(gchanmul/gchanmulmax), white[2]*(bchanmul/bchanmulmax)); @@ -1011,8 +1011,8 @@ static void _color_rgb_sliders(dt_iop_module_t *self) dt_bauhaus_slider_set_stop (g->scale_g, 0.0, white[0]*rchanmul, white[1]*0.0, white[2]*bchanmul); dt_bauhaus_slider_set_stop - (g->scale_g, chr->D65coeffs[1]/bchanmulmax, - white[0]*rchanmul, white[1]*chr->D65coeffs[1], white[2]*bchanmul); + (g->scale_g, chr->wb.D65coeffs[1]/bchanmulmax, + white[0]*rchanmul, white[1]*chr->wb.D65coeffs[1], white[2]*bchanmul); dt_bauhaus_slider_set_stop (g->scale_g, 1.0, white[0]*(rchanmul/rchanmulmax), white[1]*1.0, white[2]*(bchanmul/bchanmulmax)); @@ -1020,8 +1020,8 @@ static void _color_rgb_sliders(dt_iop_module_t *self) dt_bauhaus_slider_set_stop (g->scale_b, 0.0, white[0]*rchanmul, white[1]*gchanmul, white[2]*0.0); dt_bauhaus_slider_set_stop - (g->scale_b, chr->D65coeffs[2]/bchanmulmax, - white[0]*rchanmul, white[1]*gchanmul, white[2]*chr->D65coeffs[2]); + (g->scale_b, chr->wb.D65coeffs[2]/bchanmulmax, + white[0]*rchanmul, white[1]*gchanmul, white[2]*chr->wb.D65coeffs[2]); dt_bauhaus_slider_set_stop (g->scale_b, 1.0, white[0]*(rchanmul/rchanmulmax), white[1]*(gchanmul/gchanmulmax), white[2]*1.0); @@ -1057,9 +1057,9 @@ static void _color_temptint_sliders(dt_iop_module_t *self) const dt_dev_chroma_t *chr = &self->dev->chroma; //we consider daylight wb to be "reference white" const double dayligh_white[3] = { - 1.0 / chr->D65coeffs[0], - 1.0 / chr->D65coeffs[1], - 1.0 / chr->D65coeffs[2], + 1.0 / chr->wb.D65coeffs[0], + 1.0 / chr->wb.D65coeffs[1], + 1.0 / chr->wb.D65coeffs[2], }; double cur_coeffs[4] = {0.0}; @@ -1206,12 +1206,12 @@ static void _update_preset(dt_iop_module_t *self, int mode) if(mode == DT_IOP_TEMP_D65) { - chr->late_correction = FALSE; + chr->wb.late_correction = FALSE; } else { // For non-D65 modes, use the parameter - chr->late_correction = p->late_correction; + chr->wb.late_correction = p->late_correction; } if(g && g->check_late_correction) @@ -1268,7 +1268,7 @@ void gui_update(dt_iop_module_t *self) } // is this a "D65 white balance"? - else if(dt_dev_equal_chroma((float *)p, chr->D65coeffs)) + else if(dt_dev_equal_chroma((float *)p, chr->wb.D65coeffs)) { dt_bauhaus_combobox_set(g->presets, DT_IOP_TEMP_D65); p->preset = DT_IOP_TEMP_D65; @@ -1417,7 +1417,7 @@ void gui_update(dt_iop_module_t *self) "used preset", NULL, self, DT_DEVICE_NONE, NULL, NULL, "preset='%s': D65 %.3f %.3f %.3f, AS-SHOT %.3f %.3f %.3f", _preset_to_str(p->preset), - chr->D65coeffs[0], chr->D65coeffs[1], chr->D65coeffs[2], chr->as_shot[0], chr->as_shot[1], chr->as_shot[2]); + chr->wb.D65coeffs[0], chr->wb.D65coeffs[1], chr->wb.D65coeffs[2], chr->as_shot[0], chr->as_shot[1], chr->as_shot[2]); dt_gui_update_collapsible_section(&g->cs); @@ -1636,7 +1636,7 @@ void reload_defaults(dt_iop_module_t *self) for_four_channels(k) { chr->as_shot[k] = as_shot[k]; - chr->D65coeffs[k] = daylights[k]; + chr->wb.D65coeffs[k] = daylights[k]; } dt_print(DT_DEBUG_PARAMS, @@ -1824,7 +1824,7 @@ static void _btn_toggled(GtkGestureSingle *gesture, "toggled preset", NULL, self, DT_DEVICE_NONE, NULL, NULL, "preset='%s': D65 %.3f %.3f %.3f, AS-SHOT %.3f %.3f %.3f", _preset_to_str(preset), - chr->D65coeffs[0], chr->D65coeffs[1], chr->D65coeffs[2], chr->as_shot[0], chr->as_shot[1], chr->as_shot[2]); + chr->wb.D65coeffs[0], chr->wb.D65coeffs[1], chr->wb.D65coeffs[2], chr->as_shot[0], chr->as_shot[1], chr->as_shot[2]); } static void _preset_tune_callback(GtkWidget *widget, dt_iop_module_t *self) @@ -1869,7 +1869,7 @@ static void _preset_tune_callback(GtkWidget *widget, dt_iop_module_t *self) _temp_params_from_array(p, g->mod_coeff); break; case DT_IOP_TEMP_D65: // camera reference d65 - _temp_params_from_array(p, chr->D65coeffs); + _temp_params_from_array(p, chr->wb.D65coeffs); break; default: // camera WB presets { From 279bf201c3ef2cf99028a8690819d94df5eac258 Mon Sep 17 00:00:00 2001 From: Kofa Date: Wed, 23 Sep 2026 19:37:27 +0200 Subject: [PATCH 25/27] wb: publish per-pipe WB settings --- src/iop/temperature.c | 58 ++++++++++++++++++++++++++++++------------- 1 file changed, 41 insertions(+), 17 deletions(-) diff --git a/src/iop/temperature.c b/src/iop/temperature.c index 0b16f275238..e7842ec354b 100644 --- a/src/iop/temperature.c +++ b/src/iop/temperature.c @@ -560,6 +560,25 @@ static inline void _publish_chroma(dt_dev_pixelpipe_iop_t *piece) chr->wb.late_correction = d->late_correction; } +// the white balance this pipe renders with, for modules later in the pipe that +// complete the correction (colorin, highlights, color calibration). built from +// this piece: dev->chroma is rewritten by the other pipes' commits and +// processing. D65coeffs is the exception, only reload_defaults() writes it +static inline void _publish_pipe_wb(dt_dev_pixelpipe_iop_t *piece) +{ + const dt_iop_temperature_data_t *const d = piece->data; + const dt_dev_chroma_t *chr = &piece->module->dev->chroma; + dt_dev_wb_t *wb = &piece->pipe->wb; + + for_four_channels(k) + { + wb->coeffs[k] = piece->enabled ? d->coeffs[k] : 1.0f; + wb->D65coeffs[k] = chr->wb.D65coeffs[k]; + } + // if disabled, nothing for a later module to finish + wb->late_correction = piece->enabled && d->late_correction; +} + void process(dt_iop_module_t *self, dt_dev_pixelpipe_iop_t *piece, const void *const ivoid, @@ -724,16 +743,25 @@ void commit_params(dt_iop_module_t *self, dt_iop_temperature_data_t *d = piece->data; float *tcoeffs = (float *)p; - if(self->hide_enable_button) - piece->enabled = FALSE; - dt_dev_chroma_t *chr = &self->dev->chroma; if(self->hide_enable_button) { + // true monochrome: nothing to white balance + piece->enabled = FALSE; + + // commit and publish neutral data, so nothing reads undefined or stale values for_four_channels(k) + { + d->coeffs[k] = 1.0f; chr->wb.coeffs[k] = 1.0f; - // keep the module handle available for GUI reports (see below) + } + chr->wb.late_correction = d->late_correction = FALSE; + d->preset = p->preset; + + _publish_pipe_wb(piece); + // publish for channelmixerrgb's warning logic - details at the end + // of this function chr->temperature = self; return; } @@ -750,25 +778,21 @@ void commit_params(dt_iop_module_t *self, d->preset = p->preset; + // no late correction in 'camera reference' mode const gboolean effective_late_correction = p->preset == DT_IOP_TEMP_D65 ? FALSE : p->late_correction; d->late_correction = effective_late_correction; - // When the WB module is disabled it applies no coefficients (wb_coeffs are - // published as 1.0 above), so it makes no promise that the data is white - // balanced. Do not ask colorin (or any other consumer) to "finish" a - // correction that never started, otherwise disabling WB would still pull the - // image to D65 via D65coeffs/1.0. + // When 'temperature' is disabled it applies no coefficients (wb.coeffs are + // published as 1.0 above). Do not ask colorin (or any other consumer) to + // "finish" a correction that never started, otherwise disabling WB would + // still pull the image to D65 via D65coeffs/1.0. chr->wb.late_correction = piece->enabled ? effective_late_correction : FALSE; + _publish_pipe_wb(piece); - /* Always publish the module handle for GUI reports, regardless of the - enabled state. The enabled state is tracked separately via - chr->temperature->enabled (module flag) and pipe->dsc.temperature.enabled - (pipe flag). This lets color calibration warn about a disabled white - balance module while it is still doing chromatic adaptation. Trouble - message state is owned entirely by _set_trouble_messages() in - channelmixerrgb, so we no longer clear it here. - */ + // Always publish the module handle for channelmixerrgb _set_trouble_messages, + // regardless of the enabled state. This lets color calibration warn about a disabled + // white balance module while it is still doing chromatic adaptation. chr->temperature = self; } From 342eb5a0b69ca2e9c6e65c927071d60689ee1620 Mon Sep 17 00:00:00 2001 From: Kofa Date: Sun, 27 Sep 2026 12:07:32 +0200 Subject: [PATCH 26/27] wb: read per-pipe WB settings from pipe threads --- src/develop/pixelpipe_hb.c | 1 + src/iop/channelmixerrgb.c | 69 ++++++++++++++++---------------- src/iop/colorin.c | 22 +++++----- src/iop/highlights.c | 21 +++++----- src/iop/hlreconstruct/opposed.c | 28 +++++++------ src/iop/hlreconstruct/segbased.c | 15 +++---- 6 files changed, 81 insertions(+), 75 deletions(-) diff --git a/src/develop/pixelpipe_hb.c b/src/develop/pixelpipe_hb.c index 72d73f55f95..7fab14ecb54 100644 --- a/src/develop/pixelpipe_hb.c +++ b/src/develop/pixelpipe_hb.c @@ -784,6 +784,7 @@ void dt_dev_pixelpipe_synch_all(dt_dev_pixelpipe_t *pipe, dt_develop_t *dev) double start = dt_get_debug_wtime(); dev->cropping.exposer = NULL; + dt_print_pipe(DT_DEBUG_PARAMS, "synch all module defaults", pipe, NULL, DT_DEVICE_NONE, NULL, NULL); diff --git a/src/iop/channelmixerrgb.c b/src/iop/channelmixerrgb.c index 5d6e8abdfd9..01254b03d7a 100644 --- a/src/iop/channelmixerrgb.c +++ b/src/iop/channelmixerrgb.c @@ -591,10 +591,9 @@ void init_presets(dt_iop_module_so_t *self) self->version(), &p, sizeof(p), TRUE, DEVELOP_BLEND_CS_RGB_SCENE); } -static gboolean _dev_is_D65_chroma(const dt_develop_t *dev) +static gboolean _dev_is_D65_chroma(const dt_dev_wb_t *wb) { - const dt_dev_chroma_t *chr = &dev->chroma; - return chr->wb.late_correction || dt_dev_equal_chroma(chr->wb.coeffs, chr->wb.D65coeffs); + return wb->late_correction || dt_dev_equal_chroma(wb->coeffs, wb->D65coeffs); } static gboolean _area_mapping_active(const dt_iop_channelmixer_rgb_gui_data_t *g) @@ -611,10 +610,9 @@ static const char *_area_mapping_section_text(const dt_iop_channelmixer_rgb_gui_ } static gboolean _get_d65_correction_ratios(const dt_iop_module_t *self, + const dt_dev_wb_t *wb, dt_aligned_pixel_t out_correction_ratios) { - const dt_dev_chroma_t *chr = &self->dev->chroma; - // Init output with a no-op for_four_channels(k) out_correction_ratios[k] = 1.0f; @@ -623,14 +621,14 @@ static gboolean _get_d65_correction_ratios(const dt_iop_module_t *self, return TRUE; // If we use D65 there are unchanged corrections - if(_dev_is_D65_chroma(self->dev)) + if(_dev_is_D65_chroma(wb)) return FALSE; const gboolean valid_chroma = - chr->wb.D65coeffs[0] > 0.0 && chr->wb.D65coeffs[1] > 0.0 && chr->wb.D65coeffs[2] > 0.0; + wb->D65coeffs[0] > 0.0 && wb->D65coeffs[1] > 0.0 && wb->D65coeffs[2] > 0.0; const gboolean changed_chroma = - chr->wb.coeffs[0] > 1.0f || chr->wb.coeffs[1] > 1.0f || chr->wb.coeffs[2] > 1.0f; + wb->coeffs[0] > 1.0f || wb->coeffs[1] > 1.0f || wb->coeffs[2] > 1.0f; // Otherwise - for example because the user made a correct preset, find the // WB adaptation ratio @@ -638,8 +636,8 @@ static gboolean _get_d65_correction_ratios(const dt_iop_module_t *self, { for_four_channels(k) { - if(chr->wb.coeffs[k] > 1e-6f) - out_correction_ratios[k] = chr->wb.D65coeffs[k] / chr->wb.coeffs[k]; + if(wb->coeffs[k] > 1e-6f) + out_correction_ratios[k] = wb->D65coeffs[k] / wb->coeffs[k]; else out_correction_ratios[k] = 1.0f; } @@ -648,16 +646,17 @@ static gboolean _get_d65_correction_ratios(const dt_iop_module_t *self, } static void _get_corrected_illuminant_xy(const dt_iop_module_t *self, + const dt_dev_wb_t *wb, const dt_iop_channelmixer_rgb_params_t *p, float *x, float *y) { dt_aligned_pixel_t correction_ratios; - _get_d65_correction_ratios(self, correction_ratios); + _get_d65_correction_ratios(self, wb, correction_ratios); dt_aligned_pixel_t wb_coeffs = { 0.f }; if(p->illuminant == DT_ILLUMINANT_FROM_WB) { for(int k = 0; k < 4; k++) - wb_coeffs[k] = self->dev->chroma.wb.coeffs[k]; + wb_coeffs[k] = wb->coeffs[k]; } illuminant_to_xy(p->illuminant, &(self->dev->image_storage), correction_ratios, wb_coeffs, x, y, p->temperature, p->illum_fluo, @@ -2055,7 +2054,7 @@ static void _set_trouble_messages(dt_iop_module_t *self) const gboolean wb_applied_twice = valid && adaptation == self && temperature_enabled - && !_dev_is_D65_chroma(dev); + && !_dev_is_D65_chroma(&self->dev->chroma.wb); // another channelmixerrgb instance is doing CAT // earlier in the pipe, and we don't use masking here @@ -2139,10 +2138,11 @@ void process(dt_iop_module_t *self, const dt_iop_roi_t *const roi_out) { dt_iop_channelmixer_rbg_data_t *data = piece->data; + dt_dev_pixelpipe_t *pipe = piece->pipe; const dt_iop_order_iccprofile_info_t *const work_profile = - dt_ioppr_get_pipe_current_profile_info(self, piece->pipe); + dt_ioppr_get_pipe_current_profile_info(self, pipe); const dt_iop_order_iccprofile_info_t *const input_profile = - dt_ioppr_get_pipe_input_profile_info(piece->pipe); + dt_ioppr_get_pipe_input_profile_info(pipe); dt_iop_channelmixer_rgb_gui_data_t *g = self->gui_data; if(!dt_iop_have_required_input_format(4 /*we need full-color pixels*/, @@ -2151,7 +2151,7 @@ void process(dt_iop_module_t *self, return; // image has been copied through to output and module's // trouble flag has been updated - if(dt_pipe_is_preview(piece->pipe)) + if(dt_pipe_is_preview(pipe)) _declare_cat_on_pipe(self, FALSE); dt_colormatrix_t RGB_to_XYZ; @@ -2179,7 +2179,7 @@ void process(dt_iop_module_t *self, #ifdef AI_ACTIVATED gboolean exit = FALSE; #endif - if(g->run_profile && dt_pipe_is_preview(piece->pipe)) + if(g->run_profile && dt_pipe_is_preview(pipe)) { dt_iop_gui_enter_critical_section(self); _extract_color_checker(in, out, roi_in, g, RGB_to_XYZ, @@ -2192,7 +2192,7 @@ void process(dt_iop_module_t *self, if(data->illuminant_type == DT_ILLUMINANT_DETECT_EDGES || data->illuminant_type == DT_ILLUMINANT_DETECT_SURFACES) { - if(dt_pipe_is_full(piece->pipe)) + if(dt_pipe_is_full(pipe)) { // detection on full image only dt_iop_gui_enter_critical_section(self); @@ -2201,7 +2201,7 @@ void process(dt_iop_module_t *self, // anyway _auto_detect_WB(in, out, data->illuminant_type, roi_in->width, roi_in->height, ch, RGB_to_XYZ, g->ai_wb_XYZ); - dt_dev_pixelpipe_cache_invalidate_later(piece->pipe, self->iop_order, "AI RGB: "); + dt_dev_pixelpipe_cache_invalidate_later(pipe, self->iop_order, "AI RGB: "); dt_iop_gui_leave_critical_section(self); } @@ -2229,7 +2229,7 @@ void process(dt_iop_module_t *self, // re-run the detection at runtime… float x, y; dt_aligned_pixel_t correction_ratios; - _get_d65_correction_ratios(self, correction_ratios); + _get_d65_correction_ratios(self, &pipe->wb, correction_ratios); if(find_illuminant_xy_from_as_shot_coeffs(&(self->dev->image_storage), correction_ratios, &x, &y)) { @@ -2248,7 +2248,7 @@ void process(dt_iop_module_t *self, // which don't come from the EXIF this time). float x, y; - if(find_illuminant_xy_from_wb_coeffs(&(self->dev->image_storage), self->dev->chroma.wb.coeffs, &x, &y)) + if(find_illuminant_xy_from_wb_coeffs(&(self->dev->image_storage), pipe->wb.coeffs, &x, &y)) { // Convert illuminant from xyY to XYZ dt_aligned_pixel_t XYZ; @@ -2321,7 +2321,7 @@ void process(dt_iop_module_t *self, // run dE validation at output if(self->dev->gui_attached && g) - if(g->run_validation && dt_pipe_is_preview(piece->pipe)) + if(g->run_validation && dt_pipe_is_preview(pipe)) { _validate_color_checker(out, roi_out, g, RGB_to_XYZ, XYZ_to_RGB, XYZ_to_CAM); g->run_validation = FALSE; @@ -2339,10 +2339,11 @@ int process_cl(dt_iop_module_t *self, dt_iop_channelmixer_rbg_data_t *const d = piece->data; const dt_iop_channelmixer_rgb_global_data_t *const gd = self->global_data; + const dt_dev_pixelpipe_t *pipe = piece->pipe; const dt_iop_order_iccprofile_info_t *const work_profile = - dt_ioppr_get_pipe_current_profile_info(self, piece->pipe); + dt_ioppr_get_pipe_current_profile_info(self, pipe); - if(dt_pipe_is_preview(piece->pipe)) + if(dt_pipe_is_preview(pipe)) _declare_cat_on_pipe(self, FALSE); if(d->illuminant_type == DT_ILLUMINANT_CAMERA) @@ -2357,7 +2358,7 @@ int process_cl(dt_iop_module_t *self, // re-run the detection at runtime… float x, y; dt_aligned_pixel_t correction_ratios; - _get_d65_correction_ratios(self, correction_ratios); + _get_d65_correction_ratios(self, &pipe->wb, correction_ratios); if(find_illuminant_xy_from_as_shot_coeffs(&(self->dev->image_storage), correction_ratios, &x, &y)) { @@ -2376,7 +2377,7 @@ int process_cl(dt_iop_module_t *self, // which don't come from the EXIF this time). float x, y; - if(find_illuminant_xy_from_wb_coeffs(&(self->dev->image_storage), self->dev->chroma.wb.coeffs, &x, &y)) + if(find_illuminant_xy_from_wb_coeffs(&(self->dev->image_storage), pipe->wb.coeffs, &x, &y)) { // Convert illuminant from xyY to XYZ dt_aligned_pixel_t XYZ; @@ -2396,7 +2397,7 @@ int process_cl(dt_iop_module_t *self, return err; } - const int devid = piece->pipe->devid; + const int devid = pipe->devid; const int width = roi_in->width; const int height = roi_in->height; @@ -3185,7 +3186,7 @@ void commit_params(dt_iop_module_t *self, // find x y coordinates of illuminant for CIE 1931 2° observer float x = p->x; float y = p->y; - _get_corrected_illuminant_xy(self, p, &x, &y); + _get_corrected_illuminant_xy(self, &pipe->wb, p, &x, &y); // if illuminant is from camera or user WB coefficients, x and y are set on-the-fly at // commit time, so we need to set adaptation too @@ -3627,7 +3628,7 @@ static gboolean _illuminant_color_draw(GtkWidget *widget, float x = p->x; float y = p->y; dt_aligned_pixel_t RGB = { 0.f }; - _get_corrected_illuminant_xy(self, p, &x, &y); + _get_corrected_illuminant_xy(self, &self->dev->chroma.wb, p, &x, &y); illuminant_xy_to_RGB(x, y, RGB); cairo_set_source_rgb(cr, RGB[0], RGB[1], RGB[2]); cairo_rectangle(cr, INNER_PADDING, margin, width, height); @@ -3727,7 +3728,7 @@ static void _update_approx_cct(const dt_iop_module_t *self) float x = p->x; float y = p->y; - _get_corrected_illuminant_xy(self, p, &x, &y); + _get_corrected_illuminant_xy(self, &self->dev->chroma.wb, p, &x, &y); dt_illuminant_t test_illuminant; float t = 5000.f; @@ -3980,7 +3981,7 @@ void reload_defaults(dt_iop_module_t *self) d->adaptation = DT_ADAPTATION_CAT16; dt_aligned_pixel_t correction_ratios; - if(!_get_d65_correction_ratios(self, correction_ratios)) + if(!_get_d65_correction_ratios(self, &self->dev->chroma.wb, correction_ratios)) { if(find_illuminant_xy_from_as_shot_coeffs(img, correction_ratios, &(d->x), &(d->y))) d->illuminant = DT_ILLUMINANT_CAMERA; @@ -4076,7 +4077,7 @@ void gui_changed(dt_iop_module_t *self, // chromaticity are inited with the preset content when // illuminant is changed. dt_aligned_pixel_t correction_ratios; - _get_d65_correction_ratios(self, correction_ratios); + _get_d65_correction_ratios(self, &self->dev->chroma.wb, correction_ratios); find_illuminant_xy_from_as_shot_coeffs(&(self->dev->image_storage), correction_ratios, &(p->x), &(p->y)); _check_if_close_to_daylight(p->x, p->y, &(p->temperature), NULL, &(p->adaptation)); @@ -4103,7 +4104,7 @@ void gui_changed(dt_iop_module_t *self, { // Get camera WB and update illuminant dt_aligned_pixel_t correction_ratios; - _get_d65_correction_ratios(self, correction_ratios); + _get_d65_correction_ratios(self, &self->dev->chroma.wb, correction_ratios); const gboolean found = find_illuminant_xy_from_as_shot_coeffs(&(self->dev->image_storage), correction_ratios, &(p->x), &(p->y)); _check_if_close_to_daylight(p->x, p->y, &(p->temperature), NULL, &(p->adaptation)); @@ -4320,7 +4321,7 @@ static void _auto_set_illuminant(dt_iop_module_t *self, // find x y coordinates of illuminant for CIE 1931 2° observer float x = p->x; float y = p->y; - _get_corrected_illuminant_xy(self, p, &x, &y); + _get_corrected_illuminant_xy(self, &self->dev->chroma.wb, p, &x, &y); dt_adaptation_t adaptation = p->adaptation; diff --git a/src/iop/colorin.c b/src/iop/colorin.c index 8b08df731c9..69224b5ff28 100644 --- a/src/iop/colorin.c +++ b/src/iop/colorin.c @@ -660,13 +660,13 @@ int process_cl(dt_iop_module_t *self, return dt_opencl_enqueue_copy_image(devid, dev_in, dev_out, CLIMG_ORIGIN, CLIMG_ORIGIN, region); } - const dt_dev_chroma_t *chr = &self->dev->chroma; - const gboolean corrected = chr->wb.late_correction; + const dt_dev_wb_t *wb = &pipe->wb; + const gboolean corrected = wb->late_correction; dt_aligned_pixel_t coeffs; for_four_channels(k) { - if(corrected && chr->wb.coeffs[k] > 1e-6f) - coeffs[k] = chr->wb.D65coeffs[k] / chr->wb.coeffs[k]; + if(corrected && wb->coeffs[k] > 1e-6f) + coeffs[k] = wb->D65coeffs[k] / wb->coeffs[k]; else coeffs[k] = 1.0f; } @@ -675,7 +675,7 @@ int process_cl(dt_iop_module_t *self, { for_four_channels(k) { - pipe->dsc.temperature.coeffs[k] = chr->wb.D65coeffs[k]; + pipe->dsc.temperature.coeffs[k] = wb->D65coeffs[k]; // note: tiling takes care about processed_maximum pipe->dsc.processed_maximum[k] *= coeffs[k]; } @@ -1210,24 +1210,24 @@ void process(dt_iop_module_t *self, ivoid, ovoid, roi_in, roi_out)) return; - const dt_dev_chroma_t *chr = &self->dev->chroma; + dt_dev_pixelpipe_t *pipe = piece->pipe; + const dt_dev_wb_t *wb = &pipe->wb; const dt_iop_colorin_data_t *const d = piece->data; - const gboolean corrected = chr->wb.late_correction && d->type != DT_COLORSPACE_LAB; + const gboolean corrected = wb->late_correction && d->type != DT_COLORSPACE_LAB; dt_aligned_pixel_t coeffs; for_four_channels(k) { - if(corrected && chr->wb.coeffs[k] > 1e-6f) - coeffs[k] = chr->wb.D65coeffs[k] / chr->wb.coeffs[k]; + if(corrected && wb->coeffs[k] > 1e-6f) + coeffs[k] = wb->D65coeffs[k] / wb->coeffs[k]; else coeffs[k] = 1.0f; } - dt_dev_pixelpipe_t *pipe = piece->pipe; if(corrected) { for_four_channels(k) { - pipe->dsc.temperature.coeffs[k] = chr->wb.D65coeffs[k]; + pipe->dsc.temperature.coeffs[k] = wb->D65coeffs[k]; // note: tiling takes care about processed_maximum pipe->dsc.processed_maximum[k] *= coeffs[k]; } diff --git a/src/iop/highlights.c b/src/iop/highlights.c index ad2c40612a9..a01b445e51c 100644 --- a/src/iop/highlights.c +++ b/src/iop/highlights.c @@ -675,13 +675,13 @@ int process_cl(dt_iop_module_t *self, } else // (dmode == DT_IOP_HIGHLIGHTS_CLIP) { - const dt_dev_chroma_t *chr = &self->dev->chroma; dt_aligned_pixel_t clips = { clipper, clipper, clipper, clipper}; - if(pipe->dsc.temperature.enabled && chr->wb.late_correction) + const dt_dev_wb_t *wb = &pipe->wb; + if(pipe->dsc.temperature.enabled && wb->late_correction) { - clips[0] *= chr->wb.coeffs[0] / chr->wb.D65coeffs[0]; - clips[1] *= chr->wb.coeffs[1] / chr->wb.D65coeffs[1]; - clips[2] *= chr->wb.coeffs[2] / chr->wb.D65coeffs[2]; + clips[0] *= wb->coeffs[0] / wb->D65coeffs[0]; + clips[1] *= wb->coeffs[1] / wb->D65coeffs[1]; + clips[2] *= wb->coeffs[2] / wb->D65coeffs[2]; } dev_xtrans = dt_opencl_copy_host_to_device_constant(devid, sizeof(piece->xtrans), piece->xtrans); @@ -754,13 +754,14 @@ static void process_clip(dt_iop_module_t *self, else { const uint8_t(*const xtrans)[6] = piece->xtrans; - const dt_dev_chroma_t *chr = &self->dev->chroma; dt_aligned_pixel_t clips = { clip, clip, clip, clip}; - if(piece->pipe->dsc.temperature.enabled && chr->wb.late_correction) + dt_dev_pixelpipe_t *pipe = piece->pipe; + const dt_dev_wb_t *wb = &pipe->wb; + if(pipe->dsc.temperature.enabled && wb->late_correction) { - clips[0] *= chr->wb.coeffs[0] / chr->wb.D65coeffs[0]; - clips[1] *= chr->wb.coeffs[1] / chr->wb.D65coeffs[1]; - clips[2] *= chr->wb.coeffs[2] / chr->wb.D65coeffs[2]; + clips[0] *= wb->coeffs[0] / wb->D65coeffs[0]; + clips[1] *= wb->coeffs[1] / wb->D65coeffs[1]; + clips[2] *= wb->coeffs[2] / wb->D65coeffs[2]; } for(int row = 0; row < roi_out->height; row++) { diff --git a/src/iop/hlreconstruct/opposed.c b/src/iop/hlreconstruct/opposed.c index 31cf7321d00..53e55c88ec0 100644 --- a/src/iop/hlreconstruct/opposed.c +++ b/src/iop/hlreconstruct/opposed.c @@ -231,20 +231,21 @@ static float *_process_opposed(dt_iop_module_t *self, const uint8_t(*const xtrans)[6] = (const uint8_t(*const)[6])piece->xtrans; const uint32_t filters = piece->filters; - const dt_iop_buffer_dsc_t *dsc = &piece->pipe->dsc; + dt_dev_pixelpipe_t *pipe = piece->pipe; + const dt_iop_buffer_dsc_t *dsc = &pipe->dsc; const gboolean wbon = dsc->temperature.enabled; const dt_aligned_pixel_t icoeffs = { wbon ? dsc->temperature.coeffs[0] : 1.0f, wbon ? dsc->temperature.coeffs[1] : 1.0f, wbon ? dsc->temperature.coeffs[2] : 1.0f}; const dt_aligned_pixel_t clips = { clipval * icoeffs[0], clipval * icoeffs[1], clipval * icoeffs[2]}; - const dt_dev_chroma_t *chr = &self->dev->chroma; - const gboolean late = chr->wb.late_correction; + const dt_dev_wb_t *wb = &pipe->wb; + const gboolean late = wb->late_correction; dt_aligned_pixel_t correction; for_four_channels(k) { - if(late && chr->wb.coeffs[k] > 1e-6f) - correction[k] = chr->wb.D65coeffs[k] / chr->wb.coeffs[k]; + if(late && wb->coeffs[k] > 1e-6f) + correction[k] = wb->D65coeffs[k] / wb->coeffs[k]; else correction[k] = 1.0f; } @@ -257,7 +258,7 @@ static float *_process_opposed(dt_iop_module_t *self, const size_t msize = dt_round_size((size_t) mwidth, 8) * dt_round_size(mheight, 8); const dt_hash_t opphash = _opposed_hash(piece); - const gboolean fullpipe = dt_pipe_is_full(piece->pipe); + const gboolean fullpipe = dt_pipe_is_full(pipe); dt_aligned_pixel_t chrominance = {0.0f, 0.0f, 0.0f, 0.0f}; if(opphash == img_opphash) @@ -440,11 +441,12 @@ static cl_int process_opposed_cl(dt_iop_module_t *self, dt_iop_highlights_data_t *d = piece->data; const dt_iop_highlights_global_data_t *gd = self->global_data; - const int devid = piece->pipe->devid; + dt_dev_pixelpipe_t *pipe = piece->pipe; + const int devid = pipe->devid; const uint32_t filters = piece->filters; const float clipval = highlights_clip_magics[DT_IOP_HIGHLIGHTS_OPPOSED] * d->clip; - const dt_iop_buffer_dsc_t *dsc = &piece->pipe->dsc; + const dt_iop_buffer_dsc_t *dsc = &pipe->dsc; const gboolean wbon = dsc->temperature.enabled; const dt_aligned_pixel_t icoeffs = { wbon ? dsc->temperature.coeffs[0] : 1.0f, wbon ? dsc->temperature.coeffs[1] : 1.0f, @@ -452,13 +454,13 @@ static cl_int process_opposed_cl(dt_iop_module_t *self, dt_aligned_pixel_t clips = { clipval * icoeffs[0], clipval * icoeffs[1], clipval * icoeffs[2], 1.0f}; - const dt_dev_chroma_t *chr = &self->dev->chroma; - const gboolean late = chr->wb.late_correction; + const dt_dev_wb_t *wb = &pipe->wb; + const gboolean late = wb->late_correction; dt_aligned_pixel_t correction; for_four_channels(k) { - if(late && chr->wb.coeffs[k] > 1e-6f) - correction[k] = chr->wb.D65coeffs[k] / chr->wb.coeffs[k]; + if(late && wb->coeffs[k] > 1e-6f) + correction[k] = wb->D65coeffs[k] / wb->coeffs[k]; else correction[k] = 1.0f; } @@ -474,7 +476,7 @@ static cl_int process_opposed_cl(dt_iop_module_t *self, float *claccu = NULL; const dt_hash_t opphash = _opposed_hash(piece); - const gboolean fullpipe = dt_pipe_is_full(piece->pipe); + const gboolean fullpipe = dt_pipe_is_full(pipe); const int fastcopymode = (opphash == img_opphash) && !img_oppclipped; if(!fastcopymode) diff --git a/src/iop/hlreconstruct/segbased.c b/src/iop/hlreconstruct/segbased.c index 895681f5f79..35c0009b9c6 100644 --- a/src/iop/hlreconstruct/segbased.c +++ b/src/iop/hlreconstruct/segbased.c @@ -457,20 +457,21 @@ static void _process_segmentation(dt_dev_pixelpipe_iop_t *piece, { const uint8_t(*const xtrans)[6] = (const uint8_t(*const)[6])piece->xtrans; const uint32_t filters = piece->filters; - const gboolean fullpipe = dt_pipe_is_full(piece->pipe); + const dt_dev_pixelpipe_t *pipe = piece->pipe; + const gboolean fullpipe = dt_pipe_is_full(pipe); const float clipval = MAX(0.1f, highlights_clip_magics[DT_IOP_HIGHLIGHTS_SEGMENTS] * d->clip); - const dt_aligned_pixel_t icoeffs = { piece->pipe->dsc.temperature.coeffs[0], piece->pipe->dsc.temperature.coeffs[1], piece->pipe->dsc.temperature.coeffs[2]}; + const dt_aligned_pixel_t icoeffs = { pipe->dsc.temperature.coeffs[0], pipe->dsc.temperature.coeffs[1], pipe->dsc.temperature.coeffs[2]}; const dt_aligned_pixel_t clips = { clipval * icoeffs[0], clipval * icoeffs[1], clipval * icoeffs[2]}; const dt_aligned_pixel_t cube_coeffs = {cbrtf(clips[0]), cbrtf(clips[1]), cbrtf(clips[2]), 0.0f}; - const dt_dev_chroma_t *chr = &piece->module->dev->chroma; - const gboolean late = chr->wb.late_correction; + const dt_dev_wb_t *wb = &pipe->wb; + const gboolean late = wb->late_correction; dt_aligned_pixel_t correction; for_four_channels(k) { - if(late && chr->wb.coeffs[k] > 1e-6f) - correction[k] = chr->wb.D65coeffs[k] / chr->wb.coeffs[k]; + if(late && wb->coeffs[k] > 1e-6f) + correction[k] = wb->D65coeffs[k] / wb->coeffs[k]; else correction[k] = 1.0f; } @@ -479,7 +480,7 @@ static void _process_segmentation(dt_dev_pixelpipe_iop_t *piece, const int recovery_closing[NUM_RECOVERY_MODES] = { 0, 0, 0, 2, 2, 0, 2}; const int recovery_close = recovery_closing[recovery_mode]; - const int segmentation_limit = (piece->pipe->iwidth * piece->pipe->iheight) * sqrf(piece->pipe->iscale) / 4000; // 250 segments per mpix + const int segmentation_limit = (pipe->iwidth * pipe->iheight) * sqrf(pipe->iscale) / 4000; // 250 segments per mpix const size_t pwidth = dt_round_size(roi_in->width / 3, 2) + 2 * HL_BORDER; const size_t pheight = dt_round_size(roi_in->height / 3, 2) + 2 * HL_BORDER; From 0925cdd87c3d5f02438b56fc5ef35cf8c1a3a65f Mon Sep 17 00:00:00 2001 From: Kofa Date: Sun, 27 Sep 2026 15:25:03 +0200 Subject: [PATCH 27/27] wb: don't write chr->wb.coeffs/late_correction at processing time --- dev-doc/Module_Lifecycle.md | 376 +++++++++++++++--------------- dev-doc/pixelpipe_architecture.md | 11 +- src/develop/format.h | 4 + src/iop/temperature.c | 22 +- 4 files changed, 206 insertions(+), 207 deletions(-) diff --git a/dev-doc/Module_Lifecycle.md b/dev-doc/Module_Lifecycle.md index d3b230db92c..aaa50636c29 100644 --- a/dev-doc/Module_Lifecycle.md +++ b/dev-doc/Module_Lifecycle.md @@ -1,9 +1,9 @@ # Module Lifecycle: What Happens When the User Interacts [IOP_Module_API.md](IOP_Module_API.md) documents the callback API: what each function signature -means and what a module must implement. This document is its companion. It describes *when* those callbacks -fire and *what else* the system does around them — pipe change flags, cache invalidation, -history replay, and cross-module data flow. +means and what a module must implement. This document is its companion. It describes *when* +those callbacks fire and *what else* the system does around them: pipe change flags, cache +invalidation, history replay, and cross-module data flow. After reading the API doc you can implement a module. After reading this one you should also have a mental model of what darktable does when the user loads an image, drags a slider, @@ -18,28 +18,27 @@ as the source changes. ## 1. Background concepts These are the moving parts the later sections rely on. Each one gets a short definition, not -a full reference. +a full reference. [Core_Concepts.md](Core_Concepts.md) introduces the objects they act on: +dev, module instance, pipe, and piece. ### History stack (`dt_dev_history_item_t`) -An ordered list of per-module parameter snapshots on -[`dt_develop_t`](pixelpipe_architecture.md#dt_develop_t) (the main darkroom session state). -`dev->history_end` is -the active cursor: only the entries in the range `[0, history_end)` are "live". Note that a -history item's **`enabled` flag is stored separately from `params`** — it is not part of the -params buffer. Toggling a module on or off and editing its parameters are therefore two -different kinds of history change. +An ordered list of per-module parameter snapshots on the dev +([`dt_develop_t`](pixelpipe_architecture.md#dt_develop_t)). `dev->history_end` is the active +cursor: only the entries in the range `[0, history_end)` are "live". Note that a history item's +**`enabled` flag is stored separately from `params`**: it is not part of the params buffer. ### Pipe types -Three pixelpipes run the same module chain independently, at different resolutions: +The darkroom dev has three pixelpipes. They run the same module chain independently, at +different resolutions: -- **full** — the main darkroom image -- **preview** — the low-resolution thumbnail (used for the histogram and navigation) -- **preview2** — an optional second-window preview +- **full**: the main darkroom image +- **preview**: the low-resolution image used for the navigation thumbnail and the histogram +- **preview2**: the optional second window Each is marked dirty and re-run on its own, often in parallel on separate threads. "Mark all -pipes dirty" below means all of the pipes that exist for the current session. +pipes dirty" below means all of the pipes that exist for the dev. ### Pipe change flags @@ -49,158 +48,141 @@ pixelpipe_hb.h. | Flag | Meaning | Strategy | |---|---|---| -| `DT_DEV_PIPE_TOP_CHANGED` | Only the topmost history item's params changed | `synch_top`: re-commit that one module; upstream piece hashes stay untouched | -| `DT_DEV_PIPE_SYNCH` | All items need a param resync, topology unchanged | `synch_all`: re-commit every history item in order | +| `DT_DEV_PIPE_TOP_CHANGED` | Only the topmost history item changed | `synch_top`: re-commit that one module; the other pieces are not touched | +| `DT_DEV_PIPE_SYNCH` | All items need a param resync, topology unchanged | `synch_all`: commit every piece's defaults, then every active history item in history order | | `DT_DEV_PIPE_REMOVE` | A module was added or removed; topology must be rebuilt | full `cleanup_nodes` + `create_nodes` + `synch_all` | -| `DT_DEV_PIPE_ZOOMED` | Zoom or pan only | skip the param sync; only the `modify_roi_in`/`modify_roi_out` chain re-runs | +| `DT_DEV_PIPE_ZOOMED` | Zoom or pan only | no param sync; only the ROIs change | `synch_all` re-commits every module, but a module whose hash did not change still produces a cache hit, so its `process()` is skipped. In other words, the flag controls *committing*, not *reprocessing*. Reprocessing is decided per module by the cache (see [section 6](#6-pipeline-execution-detail)). -### Pipe status flags +### Pipe status -`DIRTY` (needs reprocessing), `RUNNING` (a thread is processing it now), `VALID` (output is -current), `INVALID` (unusable, for example during teardown). +`pipe->status` is one of `DT_DEV_PIXELPIPE_DIRTY` (needs reprocessing), `RUNNING` (a thread is +processing it now), `VALID` (finished with a valid result) or `INVALID` (finished with an +unusable result). ### `focus_hash` -A module is **focused** when the user opens its panel or clicks into one of its widgets; the -darkroom tracks a single focused module at a time. `dt_iop_request_focus()` records that module -and updates `dev->focus_hash`, a hash of the focused module's widget. Inside -`_dev_add_history_item_ext()` (develop.c), the relevant test is roughly: +Despite its name, `dev->focus_hash` is a flag. `dt_iop_request_focus()` sets it to `TRUE` when +the user moves focus to a different module (opens its panel or clicks into one of its widgets), +and `dt_dev_reload_history_items()` sets it back to `FALSE`. Each new history item stores the +value it had when the item was created. + +`_dev_add_history_item_ext()` (develop.c) compares the two. Editing the module that is already +at the top of the history normally updates that item in place, but when `dev->focus_hash` +differs from the item's stored value and the params differ, it pushes a new item instead: ```c -if(dev->focus_hash != hist->focus_hash) { /* force a new history entry */ } +|| ((dev->focus_hash != hist->focus_hash) // or if focused out and in + // but only add item if there is a difference at all for the same module + && (... memcmp(hist->params, module->params, module->params_size))) ``` -If the user moved focus since the last commit, a **new** history entry is forced even when -the same module is edited again. The practical effect is that the edit then takes the `SYNCH` -path instead of the cheaper `TOP_CHANGED` path. If you write deferred or batched parameter -changes, keep this in mind, because it changes how much of the pipe re-commits. +A new item takes the `SYNCH` path instead of the cheaper `TOP_CHANGED` path. If you write +deferred or batched parameter changes, keep this in mind, because it changes how much of the +pipe re-commits. ### Pixelpipe cache The cache is hash-based with lazy invalidation. The key for a module's output is the **cumulative hash** of every upstream `piece->hash` value (each computed in -`dt_iop_commit_params()` from the op name, instance, params, and blend_params), combined with -the relevant color-profile information. It is *not* a hash of one module's data on its own. -The hashing happens in the basic-hash helper in pixelpipe_cache.c, which seeds the hash from -the image id and pipe profiles and then walks all modules up to the requested position. +`dt_iop_commit_params()` from the op name, instance, params, blend params and mask), combined +with the image id, the color-profile information and the requested ROI. It is *not* a hash of +one module's data on its own. `_dev_pixelpipe_cache_basichash()` in pixelpipe_cache.c walks all +enabled modules up to the requested position. When the key matches a cached entry, the output is reused and `process()` is skipped. Stale entries are not actively deleted on a param change; they simply stop being looked up, and the fixed-size LRU evicts them later. For the full caching model see [pixelpipe_architecture.md](pixelpipe_architecture.md#pipeline-caching). -### `dev->chroma` (shared WB/CAT state) - -A struct on `dt_develop_t` used by white balance (`temperature.c`, the producer) and color -calibration (`channelmixerrgb.c`, the consumer) to communicate. There is **no signal**; -synchronization is implicit, through the order in which `commit_params()` runs (see -[section 4](#4-cross-module-interactions) and [section 5](#5-pipeline-ordering-asymmetry)). -Temperature writes the camera WB reference in `reload_defaults()` and the live coefficients in -`commit_params()`; channelmixerrgb reads them. The full field-by-field producer/consumer map and -the reverse-load reasoning live in their own doc — see -[wb_and_colorcalibration](iop/wb_and_colorcalibration/README.md). - ### `module->params` vs `module->default_params` `default_params` is **image-specific**: it is set by `reload_defaults()` (see [IOP_Module_API.md](IOP_Module_API.md#reload_defaults---per-image-defaults)). `params` is the -**live editing state**. -`dt_dev_pop_history_items_ext()` resets every module to its `default_params` and then replays -the history entries on top. As a result, a module with no history entry runs with its -image-specific defaults, not with compile-time constants. +**live editing state**. `dt_dev_pop_history_items_ext()` resets every module to its +`default_params` and then replays the history entries on top. As a result, a module with no +history entry runs with its image-specific defaults, not with compile-time constants. --- ## 2. Image loading A pristine (never edited) image and one with saved history both go through the **same** -`dt_dev_load_image()` → `dt_dev_read_history_ext()` chain. The "no history" versus "has +`dt_dev_load_image()` -> `dt_dev_read_history_ext()` chain. The "no history" versus "has history" distinction is handled **inside** `dt_dev_read_history_ext()`, not by different callers. ### Load sequence (`dt_dev_load_image()`, develop.c) -1. **`_dt_dev_load_raw()`** triggers the rawspeed decode through the mipmap cache (this - blocks), then reads the decoded image struct into `dev->image_storage`. The large mipmap - buffer is released right after the decode; `image_storage` keeps the metadata. (The *mipmap - cache* holds pre-scaled copies of each image at several resolutions, used for fast thumbnail - and preview generation — see [mipmap](https://en.wikipedia.org/wiki/Mipmap).) +1. **`_dt_dev_load_raw()`** triggers the raw decode through the mipmap cache (this blocks), + then reads the decoded image struct into `dev->image_storage`. The large mipmap buffer is + released right after the decode; `image_storage` keeps the metadata. (The *mipmap cache* + holds pre-scaled copies of each image at several resolutions, used for fast thumbnail and + preview generation.) 2. Mark all pipes `DIRTY` and set `loading = TRUE`. 3. **`dt_iop_load_modules()`** builds the IOP module list (`dev->iop`). -4. **`dt_dev_read_history_ext()`** does everything else — defaults, presets, and history +4. **`dt_dev_read_history_ext()`** does everything else: defaults, presets, and history replay. See below. -`dt_dev_load_image()` itself does not touch `dev->chroma` and does not load defaults. All of -that happens inside `dt_dev_read_history_ext()`. +`dt_dev_load_image()` itself does not load defaults. That happens inside +`dt_dev_read_history_ext()`. ### Inside `dt_dev_read_history_ext()` (develop.c) -This function applies saved history by **building `dev->history` directly from the database** — -it allocates one history item per stored row and appends it to the list. (This is a different -mechanism from the reset-then-replay path that undo/redo uses later; that one is described in -[section 3f](#3f-undo-redo-and-history-panel-navigation).) +This function applies saved history by **building `dev->history` directly from the +database**: it allocates one history item per stored row and appends it to the list. (This is +a different mechanism from the reset-then-replay path that undo/redo uses later; that one is +described in [section 3f](#3f-undo-redo-and-history-panel-navigation).) -**For every image** — this is the `if(!no_image)` block, and it runs whether or not the image -has saved history: +**For every image** (the `if(!no_image)` block, which runs whether or not the image has saved +history): - Clear the scratch `memory.history` table. -- **`dt_dev_reset_chroma()`** clears part of the shared WB state - ([section 5](#5-pipeline-ordering-asymmetry) lists exactly which fields): `temperature` and - `adaptation` become `NULL`, and `wb.coeffs[]` is reset to 1.0. +- **`dt_dev_reset_chroma()`** resets part of the shared white balance state, `dev->chroma` + (see [section 5](#5-pipeline-ordering-asymmetry) for why). - **`_dt_dev_load_pipeline_defaults()`** calls `dt_iop_reload_defaults()` on every module in - **reverse** pipe order ([section 5](#5-pipeline-ordering-asymmetry) explains the direction). - This sets each module's image-specific - `default_params`. Because it goes through the wrapper, it also copies them into `params` - (via `dt_iop_load_default_params()`). Temperature additionally populates - `dev->chroma.wb.as_shot[]` and `D65coeffs[]` as a side effect. + **reverse** pipe order ([section 5](#5-pipeline-ordering-asymmetry) explains the + consequences). This sets each module's image-specific `default_params`. Because it goes + through the wrapper, it also copies them into `params` (via `dt_iop_load_default_params()`). + Some modules also write shared state here: white balance stores the camera's reference + coefficients in `dev->chroma`. - **`_dev_add_default_modules()`** prepends the workflow-mandated modules into the in-memory history. - **`_dev_auto_apply_presets()`** applies auto-presets. It is gated on the `DT_IMAGE_AUTO_PRESETS_APPLIED` image flag, **not** on `change_timestamp == -1` (that test - guards only the separate legacy pre-3.0 WB recovery branch). It is a no-op for an image - whose presets were already applied, so only a first import actually gains preset entries - here. + guards only the separate legacy pre-3.0 white balance recovery branch). It is a no-op for an + image whose presets were already applied, so only a first import actually gains preset + entries here. - **`_dev_merge_history()`** merges the in-memory default and preset history into `main.history`, so the database read below picks it up. **Then, for all images,** the same database-read loop runs. For each `main.history` row it allocates a `hist` item, copies the params (falling back to the module's `default_params` -when the row has none), sets `hist->enabled`, appends to `dev->history`, and increments -`history_end`: +when the row has none, which is how auto-applied presets are stored), sets `hist->enabled`, +appends to `dev->history`, and increments `history_end`: ```c -memcpy(hist->params, hist->module->default_params, hist->module->params_size); -hist->enabled = TRUE; +if(param_length == 0) + memcpy(hist->params, hist->module->default_params, hist->module->params_size); ... dev->history = g_list_append(dev->history, hist); dev->history_end++; ``` Modules with no database row keep the image-specific `default_params` set earlier by -`_dt_dev_load_pipeline_defaults()`. The `enabled` flag is carried per entry, separately from -params. - -After the loop, if both `temperature` and `channelmixerrgb` are present, the function calls -`temperature->reload_defaults(temperature)` **directly**, to resync the WB/CAT shared state: - -```c -if(temperature && channelmixerrgb) - temperature->reload_defaults(temperature); -``` +`_dt_dev_load_pipeline_defaults()`. -This direct call is a deliberate wrapper bypass: it skips the framework's wholesale -`default_params → params` copy (`dt_iop_load_default_params()`). It still refreshes -`default_params` and the `dev->chroma` reference data, and temperature's own body also writes -a few `self->params` fields directly (`preset`, `late_correction`). So `params` is partially -updated, not fully resynced — see the `reload_defaults` caveat in +After the loop, in the `none` workflow, if both white balance (`temperature`) and color +calibration (`channelmixerrgb`) are enabled in the history, the function calls +`temperature->reload_defaults(temperature)` **directly**. This bypasses the wrapper, so +`params` is only partially updated; see the `reload_defaults()` caveat in [IOP_Module_API.md](IOP_Module_API.md#reload_defaults---per-image-defaults). -The function then re-reads `history_end` from `main.images`, and — when the GUI is attached — +The function then re-reads `history_end` from `main.images`, and, when the GUI is attached, calls `dt_dev_pipe_synch_all()` and `dt_dev_invalidate_all()` so the pipes will commit and reprocess. The pipes stay `DIRTY`, and the pipeline threads run on the next redraw ([section 6](#6-pipeline-execution-detail)). @@ -209,27 +191,33 @@ reprocess. The pipes stay `DIRTY`, and the pipeline threads run on the next redr ## 3. Editing operations -### 3a. Adjust a slider — the common `TOP_CHANGED` path - -1. The bauhaus slider fires its value-change handler (`bauhaus.c`), which calls - `dt_iop_gui_changed(module, widget, &prev)` directly. -2. The module's `gui_changed()` runs (module-specific; it may update dependent widgets). -3. `dt_dev_add_history_item_target()` reaches `_dev_add_history_item_ext()` (develop.c), which - chooses one of two branches: - - **`TOP_CHANGED`**: the same module is already at the top of the history, - `dev->focus_hash == hist->focus_hash`, and only the params differ. The params are - updated in place and `pipe->changed |= DT_DEV_PIPE_TOP_CHANGED`. - - **`SYNCH`**: anything else (a different module, or focus changed since the last commit). - A new history entry is pushed and `pipe->changed |= DT_DEV_PIPE_SYNCH`. +All edits below end in `_dev_add_history_item_ext()` (develop.c), which chooses between two +branches: + +- **update in place** (`TOP_CHANGED`): the module is already at the top of the history, and + the `focus_hash` test above does not ask for a new item. The item's params and `enabled` + flag are overwritten and `pipe->changed |= DT_DEV_PIPE_TOP_CHANGED`. +- **new item** (`SYNCH`): anything else, such as a different module or instance. A new history + entry is pushed and `pipe->changed |= DT_DEV_PIPE_SYNCH`. + +### 3a. Adjust a slider: the common `TOP_CHANGED` path + +1. For a widget created with a `_from_params` helper, the bauhaus value-change handler + (`bauhaus.c`) writes the new value into `self->params` and calls + `dt_iop_gui_changed(module, widget, &prev)`. A manually connected widget calls + `dt_dev_add_history_item()` from its own callback instead. +2. `dt_iop_gui_changed()` runs the module's `gui_changed()` (module-specific; it may update + dependent widgets), then `dt_dev_add_history_item_target()`. +3. `_dev_add_history_item_ext()` chooses one of the two branches above. 4. `dt_dev_invalidate_all()` marks all pipes `DIRTY` and increments `dev->timestamp`. 5. `DT_SIGNAL_DEVELOP_HISTORY_CHANGE` is emitted **only when `need_end_record` is true**, not on every slider tick. It is tied to the undo-record start/end pairing, so one logical edit emits the signal once. 6. On the pipeline thread, `dt_dev_pixelpipe_change()` dispatches on the flag: - - `TOP_CHANGED` → `dt_dev_pixelpipe_synch_top()`: runs `commit_params()` for that one - module only; upstream piece hashes are not recomputed. - - `SYNCH` → `dt_dev_pixelpipe_synch_all()`: runs `commit_params()` for every history item - in order. Upstream modules re-commit, but an unchanged hash still hits the cache. + - `TOP_CHANGED` -> `dt_dev_pixelpipe_synch_top()`: runs `commit_params()` for that one + module only. + - `SYNCH` -> `dt_dev_pixelpipe_synch_all()`: commits every piece's defaults, then every + active history item in history order. Unchanged hashes still hit the cache. 7. The pipeline runs: modules with unchanged hashes reuse their cached output; the changed module and everything downstream of it reprocess. @@ -239,19 +227,18 @@ reprocess. The pipes stay `DIRTY`, and the pipeline threads run on the next redr (imageop.c). 2. The callback sets `module->enabled` and calls `dt_dev_add_history_item()`. The `hist->enabled` flag is recorded **separately from params**. -3. `pipe->changed |= DT_DEV_PIPE_SYNCH`, because an enable/disable propagates downstream. -4. A full `synch_all` runs on the next pipeline pass. +3. If the module is already at the top of the history, its item is updated in place + (`TOP_CHANGED`); otherwise a new item is pushed (`SYNCH`). ### 3c. Reset to defaults -1. The reset button fires `button-press-event`, which reaches `_gui_reset_callback()` - (imageop.c). +1. The reset button in the module header reaches `_gui_reset_callback()` (imageop.c). 2. `dt_iop_reload_defaults(module)` recomputes the image-specific `default_params` and copies them into `module->params` (this is the wrapper path, so the copy does happen). 3. `dt_iop_gui_update(module)` syncs the widgets to the new params. 4. `dt_dev_add_history_item(module->dev, module, TRUE)` adds or updates a **single** history - entry. It does not rebuild the whole stack. -5. `pipe->changed |= DT_DEV_PIPE_SYNCH`. + entry, with the same `TOP_CHANGED` / `SYNCH` choice as any other edit. It does not rebuild + the whole stack. ### 3d. Add a module instance (+ button) @@ -261,12 +248,12 @@ reprocess. The pipes stay `DIRTY`, and the pipeline threads run on the next redr 4. `dt_iop_reload_defaults(new_module)` sets the image-specific defaults for the new instance. 5. `dt_dev_add_history_item(new_module->dev, new_module, TRUE)` adds a new history entry. 6. `dt_dev_pixelpipe_rebuild()` sets `pipe->changed |= DT_DEV_PIPE_REMOVE`, which forces a - full topology rebuild — the node graph changed, so it is rebuilt from scratch. + full topology rebuild: the node graph changed, so it is rebuilt from scratch. -### 3e. Remove a module instance (− button) +### 3e. Remove a module instance (- button) `dt_dev_module_remove()` (develop.c) does the teardown. Inside its `if(dev->gui_attached)` -block — true in the interactive darkroom — it **prunes the history**. It walks `dev->history` +block (true in the interactive darkroom) it **prunes the history**. It walks `dev->history` and, for each entry whose module is the one being removed, frees the item, unlinks it, and decrements `history_end`: @@ -279,66 +266,77 @@ if(module == hist->module) } ``` -It then removes the module from `dev->iop`. A `REMOVE` flag triggers a full topology rebuild -(`cleanup_nodes` + `create_nodes`). +It then removes the module from `dev->iop`, and the pipes are marked for a topology rebuild +(`REMOVE`: `cleanup_nodes` + `create_nodes`). So a removed instance's history entries are **actively deleted**, not merely skipped. In a -headless context, where `gui_attached` is false, this pruning block does not run — but +headless context, where `gui_attached` is false, this pruning block does not run, but interactive removal is the case this document covers. ### 3f. Undo, redo, and history-panel navigation -This is a **distinct path** from the initial image load. During an active editing session it -is the main way `module->params` are rewritten from saved entries. +These are **distinct paths** from the initial image load. During an active editing session +they are the main way `module->params` are rewritten from saved entries. Both end in +`dt_dev_pop_history_items()`, which calls `dt_dev_pop_history_items_ext()` to reset all +modules to their `default_params` and replay the history up to the given position, updates +every module's GUI, then calls `dt_dev_pipe_synch_all()` (or `dt_dev_pixelpipe_rebuild()` when +the module order changed) and `dt_dev_invalidate_all()`. -1. The user clicks a history entry, or presses Ctrl+Z / Ctrl+Y. -2. `dt_dev_reload_history_items()` (develop.c) sets `dev->history_end` to the target position. -3. `dt_dev_pop_history_items(dev, 0)` resets all modules to their `default_params`. -4. The history is re-read (from memory or the database). -5. `dt_dev_pop_history_items(dev, history_end)` replays up to the target position. -6. `dt_dev_invalidate_all()` marks the pipes dirty, which leads to a full pipeline rerun. +**Clicking an entry in the history panel** (`libs/history.c`) calls +`dt_dev_pop_history_items(dev, num)` directly, with the clicked position. -The two `pop_history_items` calls live **here**, in `dt_dev_reload_history_items()`, not in -`dt_dev_read_history_ext()`. Apart from the initial image load, this is the only path that -rewrites `module->params` from saved history. The `SYNCH` flag ensures every module -re-commits after the replay. +**Undo and redo** (`_pop_undo()` in `libs/history.c`) swap in the saved history list and +`history_end`, mark the pipes for a rebuild, write the history to the database, and call +`dt_dev_reload_history_items()` (develop.c), which: + +1. clears `dev->focus_hash`, +2. calls `dt_dev_pop_history_items(dev, 0)` to reset all modules to their `default_params`, +3. drops the history items beyond `history_end`, and re-reads the history from the database, +4. calls `dt_dev_pop_history_items(dev, history_end)` to replay up to the target position. --- ## 4. Cross-module interactions -### 4a. Temperature (WB) → ChannelMixerRGB (through `dev->chroma`) +### 4a. White balance -> color calibration (through `dev->chroma`) + +White balance (`temperature.c`) and color calibration (`channelmixerrgb.c`) share state in +`dev->chroma`; there is no signal. White balance writes the camera's reference coefficients in +`reload_defaults()` and the coefficients it applies in `commit_params()`; color calibration +reads them in `reload_defaults()`, `commit_params()` and `process()`. + +Commit order does not guarantee that white balance commits first: `synch_all` replays the +history in history order, and `synch_top` re-commits only the top item. Color calibration +therefore recomputes its white-balance-dependent illuminants (`DT_ILLUMINANT_CAMERA`, +`DT_ILLUMINANT_FROM_WB`) in `process()`, which runs after all the pipe's commits are done. -There is no signal; synchronization is implicit and ordered by iop_order. Because temperature is -upstream of channelmixerrgb, a full `synch_all` runs `commit_params()` in pipe order: -`temperature.commit_params()` writes `dev->chroma.wb.coeffs[]` first and -`channelmixerrgb.commit_params()` reads it afterward to build its chromatic-adaptation matrix. -(`synch_top` re-commits only the topmost history item, so it is not evidence that an upstream -producer has just run.) The full producer/consumer field map, the once-per-load `reload_defaults` -side effects, and why reverse-order default loading stays correct are covered in -[wb_and_colorcalibration](iop/wb_and_colorcalibration/README.md). +The field-by-field map, the per-pipe copy `pipe->wb`, and why reverse-order default loading +stays correct are in [iop/wb_and_colorcalibration](iop/wb_and_colorcalibration/README.md). ### 4b. Color-profile change (colorin / colorout) -This follows the same flow as a slider adjust: `TOP_CHANGED` or `SYNCH`, depending on -`focus_hash` and on whether it is an in-place update. There is no `REMOVE`, because the -topology does not change. `commit_params()` rebuilds the color transforms; no cross-module -signal is needed. Because the transform changes the pixels, all downstream cache hashes change -and everything downstream reprocesses. +This follows the same flow as a slider adjust: `TOP_CHANGED` or `SYNCH`, depending on the +history and `focus_hash`. There is no `REMOVE`, because the topology does not change. +`commit_params()` rebuilds the color transforms; no cross-module signal is needed. Because the +transform changes the pixels, all downstream cache hashes change and everything downstream +reprocesses. --- ## 5. Pipeline ordering asymmetry -Two operations walk the module list in **opposite** directions, which is a common source of +Three operations walk the modules in **different** orders, which is a common source of confusion. (See also the [ordering-asymmetry note](pixelpipe_architecture.md#pipeline-ordering-asymmetry) in pixelpipe_architecture.md.) -- **`commit_params()` (through `synch_all`)** iterates the history in history-list order, which - is effectively pipe order: an upstream module commits before a downstream one, so any state it - writes into shared structures is in place when the downstream module reads it. -- **`_dt_dev_load_pipeline_defaults()`** iterates in **reverse** pipe order (last module first): +- **Processing** runs in pipe order: an upstream module's `process()` finishes before a + downstream one starts. +- **`commit_params()` (through `synch_all`)** first commits every piece's defaults in pipe + order, then replays the history items in **history order**. History order is the order of + the user's edits, not pipe order, so a downstream module can commit before an upstream one. +- **`_dt_dev_load_pipeline_defaults()`** iterates in **reverse** pipe order (last module + first): ```c for(const GList *modules = g_list_last(dev->iop); @@ -354,15 +352,16 @@ pixelpipe_architecture.md.) ### Developer rule -A module's `reload_defaults()` **cannot** assume that earlier-in-pipe modules have already -populated shared state (such as `dev->chroma`), because under reverse iteration they have not run -yet. Shared state that a `reload_defaults()` depends on must therefore be **reset to a neutral -value before the reverse default-load** — which the framework does via `dt_dev_reset_chroma()` -just before `_dt_dev_load_pipeline_defaults()`. +Neither `reload_defaults()` nor `commit_params()` can assume that earlier-in-pipe modules have +already written shared state for the current image or edit. Only `process()` runs in pipe +order. -The worked example — how white balance and color calibration share `dev->chroma`, and why this -reset keeps color calibration's defaults image-local despite the reverse order — is in -[wb_and_colorcalibration](iop/wb_and_colorcalibration/README.md). +Shared state that a `reload_defaults()` depends on must therefore be **reset to a neutral +value before the reverse default-load**. The framework does this for white balance: it calls +`dt_dev_reset_chroma()` just before `_dt_dev_load_pipeline_defaults()`. The neutral white +balance coefficients make color calibration compute its default illuminant from the image's own +as-shot coefficients, instead of from values left by the previous image. The details are in +[iop/wb_and_colorcalibration](iop/wb_and_colorcalibration/README.md). --- @@ -370,21 +369,19 @@ reset keeps color calibration's defaults image-local despite the reverse order How pixels actually flow once the pipes are marked dirty: -- `dt_dev_process_image_job()` (develop.c) is the entry point. It locks the pipe and captures - `dev->timestamp`. +- `dt_dev_process_image_job()` (develop.c) is the entry point. It locks the pipe and records + `dev->timestamp` as the pipe's input timestamp. - `dt_dev_pixelpipe_process()` calls `_dev_pixelpipe_process_rec()` (pixelpipe_hb.c), which recurses from the last module back toward the input, pulling each module's input on demand. -- For each module, the cache key is the **cumulative hash** of all upstream `piece->hash` - values (each set in - [`dt_iop_commit_params()`](IOP_Module_API.md#commit_params---transform-ui-parameters-into-processing-data) - from op, instance, params, and blend_params), - plus the color-profile information. On a key match, the cached output is reused and - `process()` is skipped. On a miss, `process()` (or `process_cl()` on the GPU) runs and the - result is stored. +- For each module, the cache key is the **cumulative hash** described in + [section 1](#pixelpipe-cache). On a key match, the cached output is reused and `process()` is + skipped. On a miss, `process()` (or `process_cl()` on an OpenCL device) runs and the result + is stored. - `modify_roi_in()` and `modify_roi_out()` run on every module during the ROI-propagation phase, before processing, translating between the output-side and input-side regions. Under - the `ZOOMED` flag only the ROIs change — the module hashes do not — so cache hits are - possible for any module whose cached output region still covers the requested region. + the `ZOOMED` flag only the ROIs change, not the module hashes. The ROI is part of the cache + key, so a module gets a cache hit only for a region it has already produced, for example + after zooming back. - The cache is a fixed-size LRU, and eviction is automatic. There is no explicit invalidation on a param change; a changed hash simply misses. - After all modules finish, `pipe->status` becomes `VALID` and a finished signal (for example @@ -406,10 +403,10 @@ section above. | `dt_dev_load_image` | Top-level image load | | `dt_dev_read_history_ext` | Loads defaults/presets and builds `dev->history` from the DB | | `_dt_dev_load_pipeline_defaults` | Calls `reload_defaults` on all modules, in reverse pipe order | -| `dt_dev_reset_chroma` | Resets shared WB state to neutral before reverse default-load | +| `dt_dev_reset_chroma` | Resets shared white balance state before the reverse default-load | | `dt_dev_add_history_item` / `_dev_add_history_item_ext` | Add/update a history entry; choose `TOP_CHANGED` vs `SYNCH` (the `focus_hash` test lives here) | -| `dt_dev_pop_history_items` / `_ext` | Reset to defaults, then replay history (used by undo/redo) | -| `dt_dev_reload_history_items` | Undo/redo and history-panel navigation | +| `dt_dev_pop_history_items` / `_ext` | Reset to defaults, then replay history up to a position | +| `dt_dev_reload_history_items` | Undo/redo: reset, re-read the history, replay | | `dt_dev_module_remove` | Removes a module instance and prunes its history entries | | `dt_dev_invalidate_all` | Marks all pipes dirty, bumps `dev->timestamp` | | `_dev_auto_apply_presets` | Auto-presets; gated on `DT_IMAGE_AUTO_PRESETS_APPLIED` | @@ -422,7 +419,8 @@ section above. | `dt_iop_reload_defaults` | Wrapper: calls the module's `reload_defaults()` then `dt_iop_load_default_params()` | | `dt_iop_load_default_params` | Copies `default_params` into `params` | | `dt_iop_commit_params` | Translates params into `piece->data`; sets `piece->hash` | -| `dt_iop_gui_changed` | Slider/widget change entry point | +| `dt_iop_gui_changed` | Entry point for `_from_params` widget changes | +| `dt_iop_request_focus` | Changes the focused module; sets `dev->focus_hash` | | `_gui_off_callback` | Enable/disable toggle handler | | `_gui_reset_callback` | Reset-to-defaults handler | | `dt_iop_gui_duplicate` | Add a module instance | @@ -433,25 +431,27 @@ section above. |---|---| | `DT_DEV_PIPE_*` flags | Pipe change flags (defined in pixelpipe_hb.h) | | `dt_dev_pixelpipe_change` | Dispatches on the change flag | -| `dt_dev_pixelpipe_synch_top` | Re-commits the top module only | -| `dt_dev_pixelpipe_synch_all` | Re-commits all history items in order | +| `dt_dev_pixelpipe_synch_top` | Re-commits the top history item only | +| `dt_dev_pixelpipe_synch_all` | Commits all defaults, then all active history items | | `_dev_pixelpipe_process_rec` | Recursive per-module processing | **Other** | Symbol | File | Role | |---|---|---| -| Basic-hash helper (cumulative cache hash) | pixelpipe_cache.c | Builds the cache key from image id, profiles, and all modules up to a position | +| `_dev_pixelpipe_cache_basichash` | pixelpipe_cache.c | Builds the cache key from image id, profiles, and all modules up to a position | +| `_pop_undo` | libs/history.c | Undo/redo handler | -For the white-balance ↔ color-calibration symbols (`temperature.*` / `channelmixerrgb.*` and the -`dev->chroma` helpers), see -[wb_and_colorcalibration](iop/wb_and_colorcalibration/README.md#key-symbols). +For the white balance and color calibration symbols (`temperature.*`, `channelmixerrgb.*` and +the `dev->chroma` helpers), see +[iop/wb_and_colorcalibration](iop/wb_and_colorcalibration/README.md#key-symbols). ## See also -- [IOP_Module_API.md](IOP_Module_API.md) — the callback API reference (signatures, the two +- [IOP_Module_API.md](IOP_Module_API.md): the callback API reference (signatures, the two [`reload_defaults`](IOP_Module_API.md#reload_defaults---per-image-defaults) jobs). -- [pixelpipe_architecture.md](pixelpipe_architecture.md) — pipeline architecture, including the +- [pixelpipe_architecture.md](pixelpipe_architecture.md): pipeline architecture, including the [ordering-asymmetry note](pixelpipe_architecture.md#pipeline-ordering-asymmetry). -- [wb_and_colorcalibration](iop/wb_and_colorcalibration/README.md) — the white-balance ↔ - color-calibration interaction through `dev->chroma` (worked example of the ordering principle). +- [iop/wb_and_colorcalibration](iop/wb_and_colorcalibration/README.md): the white balance and + color calibration interaction through `dev->chroma` (worked example of the ordering + principle). diff --git a/dev-doc/pixelpipe_architecture.md b/dev-doc/pixelpipe_architecture.md index 0aa3996e687..cec9c0e73d9 100644 --- a/dev-doc/pixelpipe_architecture.md +++ b/dev-doc/pixelpipe_architecture.md @@ -5,8 +5,7 @@ The **pixelpipe** is the core image processing engine of darktable. It is respon ## Core Structures ### `dt_develop_t` -Defined in `src/develop/develop.h`. -This is the main darkroom session state — one per open image. Besides the pixelpipes (below) it holds the module list (`dev->iop`, a `GList` of `dt_iop_module_t`), the history stack (`dev->history` plus the `dev->history_end` cursor), and shared inter-module state such as `dev->chroma` (white-balance / chromatic-adaptation data passed from `temperature.c` to `channelmixerrgb.c`). Most lifecycle operations — image load, history replay, parameter commits — act on this structure. For the event-by-event walkthrough see [Module_Lifecycle.md](Module_Lifecycle.md). +Defined in `src/develop/develop.h`. The dev: the working context for processing one image at a time. The darkroom has one, reused when switching images; exports, thumbnails and some other features create their own (see [Core_Concepts.md](Core_Concepts.md)). Besides the pixelpipes (below) it holds the module list (`dev->iop`, a `GList` of `dt_iop_module_t`), the history stack (`dev->history` plus the `dev->history_end` cursor), and shared inter-module state such as `dev->chroma` (white balance data passed from `temperature.c` to `channelmixerrgb.c`). Most lifecycle operations (image load, history replay, parameter commits) act on this structure. For the event-by-event walkthrough see [Module_Lifecycle.md](Module_Lifecycle.md). ### `dt_dev_pixelpipe_t` Defined in `src/develop/pixelpipe_hb.h`. This structure represents a single instance of a processing pipeline. A `dt_develop_t` (the main development state) holds several pipes: @@ -133,12 +132,12 @@ The pixelpipe is designed to be threaded. Two key pipeline operations iterate modules in **different** orders: -- **`commit_params()`** runs in **forward** pipe order (e.g., temperature before channelmixerrgb). This is the normal processing direction. -- **`_dt_dev_load_pipeline_defaults()`** runs in **reverse** pipe order (e.g., channelmixerrgb before temperature). This happens during history reset and default loading. +- **`commit_params()`** (through `synch_all`) first commits every piece's defaults in forward pipe order, then replays the history items in **history order**, which is the order of the user's edits. So a downstream module can commit before an upstream one. Only processing itself is guaranteed to run in pipe order. +- **`_dt_dev_load_pipeline_defaults()`** runs in **reverse** pipe order (e.g., channelmixerrgb before temperature). It is called when an image is loaded (`dt_dev_read_history_ext()`). -This asymmetry matters for modules that communicate via shared state. For example, `temperature.c` writes white balance coefficients into `dev->chroma.wb.coeffs`, and `channelmixerrgb.c` reads them during `commit_params()`. During forward processing, temperature commits first and the data is available. During reverse-order default loading, channelmixerrgb runs first — before temperature has refreshed its values — so any shared state a `reload_defaults()` depends on must be reset to a neutral value beforehand. +This matters for modules that communicate via shared state. For example, `temperature.c` writes white balance coefficients into `dev->chroma.wb.coeffs`, and `channelmixerrgb.c` reads them in `reload_defaults()`, `commit_params()` and `process()`. During reverse-order default loading, channelmixerrgb runs first, before temperature has refreshed its values, so any shared state a `reload_defaults()` depends on must be reset to a neutral value beforehand. During history replay, channelmixerrgb may commit before temperature, so it recomputes its white-balance-dependent illuminant in `process()`. -**Consequence:** Shared state (like `dev->chroma`) must be reset before reverse-order iteration. The framework does this via `dt_dev_reset_chroma()` immediately before `_dt_dev_load_pipeline_defaults()`; the neutral `wb.coeffs` it writes is what keeps the dependent defaults image-local. For the full white-balance ↔ color-calibration interaction, see [iop/wb_and_colorcalibration](iop/wb_and_colorcalibration/README.md). +**Consequence:** Shared state (like `dev->chroma`) must be reset before reverse-order iteration. The framework does this via `dt_dev_reset_chroma()` immediately before `_dt_dev_load_pipeline_defaults()`; the neutral `wb.coeffs` it writes is what keeps the dependent defaults image-local. See [Module_Lifecycle.md](Module_Lifecycle.md#5-pipeline-ordering-asymmetry), and for the full white balance and color calibration interaction, [iop/wb_and_colorcalibration](iop/wb_and_colorcalibration/README.md). ## Introspection Connection diff --git a/src/develop/format.h b/src/develop/format.h index 66ead084663..fd540b6cb5c 100644 --- a/src/develop/format.h +++ b/src/develop/format.h @@ -62,6 +62,10 @@ typedef struct dt_iop_buffer_dsc_t struct { gboolean enabled; + + /** white balance multiplied into this buffer. pipe->wb.coeffs keeps what + temperature applied; this copy is rewritten to D65coeffs by colorin when + it completes the late correction */ dt_aligned_pixel_t coeffs; } temperature; diff --git a/src/iop/temperature.c b/src/iop/temperature.c index e7842ec354b..ae0ae674106 100644 --- a/src/iop/temperature.c +++ b/src/iop/temperature.c @@ -543,11 +543,9 @@ static inline void scaled_copy_4wide(float *const outp, outp[c] = inp[c] * coeffs[c]; } -static inline void _publish_chroma(dt_dev_pixelpipe_iop_t *piece) +static inline void _update_pipe_dsc(dt_dev_pixelpipe_iop_t *piece) { const dt_iop_temperature_data_t *const d = piece->data; - dt_iop_module_t *self = piece->module; - dt_dev_chroma_t *chr = &self->dev->chroma; piece->pipe->dsc.temperature.enabled = piece->enabled; for_four_channels(k) @@ -555,28 +553,26 @@ static inline void _publish_chroma(dt_dev_pixelpipe_iop_t *piece) piece->pipe->dsc.temperature.coeffs[k] = d->coeffs[k]; piece->pipe->dsc.processed_maximum[k] = d->coeffs[k] * piece->pipe->dsc.processed_maximum[k]; - chr->wb.coeffs[k] = d->coeffs[k]; } - chr->wb.late_correction = d->late_correction; } // the white balance this pipe renders with, for modules later in the pipe that // complete the correction (colorin, highlights, color calibration). built from -// this piece: dev->chroma is rewritten by the other pipes' commits and -// processing. D65coeffs is the exception, only reload_defaults() writes it +// this piece: dev->chroma is rewritten by the other pipes' commits. +// D65coeffs is the exception, only reload_defaults() writes it static inline void _publish_pipe_wb(dt_dev_pixelpipe_iop_t *piece) { const dt_iop_temperature_data_t *const d = piece->data; const dt_dev_chroma_t *chr = &piece->module->dev->chroma; - dt_dev_wb_t *wb = &piece->pipe->wb; + dt_dev_pixelpipe_t *pipe = piece->pipe; for_four_channels(k) { - wb->coeffs[k] = piece->enabled ? d->coeffs[k] : 1.0f; - wb->D65coeffs[k] = chr->wb.D65coeffs[k]; + pipe->wb.coeffs[k] = piece->enabled ? d->coeffs[k] : 1.0f; + pipe->wb.D65coeffs[k] = chr->wb.D65coeffs[k]; } // if disabled, nothing for a later module to finish - wb->late_correction = piece->enabled && d->late_correction; + pipe->wb.late_correction = piece->enabled && d->late_correction; } void process(dt_iop_module_t *self, @@ -685,7 +681,7 @@ void process(dt_iop_module_t *self, } } - _publish_chroma(piece); + _update_pipe_dsc(piece); } #ifdef HAVE_OPENCL @@ -725,7 +721,7 @@ int process_cl(dt_iop_module_t *self, CLARG(dev_coeffs), CLARG(filters), CLARG(dev_xtrans)); if(err != CL_SUCCESS) goto error; - _publish_chroma(piece); + _update_pipe_dsc(piece); error: dt_opencl_release_mem_object(dev_coeffs);