diff --git a/RELEASE_NOTES.md b/RELEASE_NOTES.md index baa8ec3403..daeab8d437 100644 --- a/RELEASE_NOTES.md +++ b/RELEASE_NOTES.md @@ -250,6 +250,14 @@ changes (where available). - Fixed corrupted output or a crash when an AI model returns more data than darktable reserved for it, affecting object masks and Lua models. +- Fixed automatic exposure rendering a black image when its raw histogram + was unavailable, and black-point limits ignoring camera exposure + compensation, which could invert the tonal range. + +- Fixed exposure's area mapping blowing out the image when its target + lightness was set to zero. Such a target cannot be reached, so the + correction is now left alone instead. + ## Lua ### API Version diff --git a/src/develop/develop.h b/src/develop/develop.h index 89dcda50c1..d5e72a78eb 100644 --- a/src/develop/develop.h +++ b/src/develop/develop.h @@ -97,6 +97,17 @@ typedef enum dt_clipping_preview_mode_t DT_CLIPPING_PREVIEW_SATURATION = 3 } dt_clipping_preview_mode_t; +// Cross-module access to the current settings of the exposure module, registered by +// its gui_init() and currently used by the histogram, channelmixerrgb and agx. +// +// Thread contract: must only be called from the GTK main thread -- the accessors read the +// exposure module's live `params` and the `module` pointer itself, both owned by that +// thread. See dev-doc/GUI_Threading.md, "Publishing gui_data Through a Proxy". +// +// TODO: Known violation tracked as #22005 (remove this when fixed): +// channelmixerrgb calls dt_dev_exposure_get_exposure() and dt_dev_exposure_get_black() +// from _extract_patches(), which runs in process() on the preview pipe thread during +// ColorChecker profiling. typedef struct dt_dev_proxy_exposure_t { struct dt_iop_module_t *module; @@ -511,6 +522,8 @@ void dt_dev_get_viewport_params(dt_dev_viewport_t *port, void dt_dev_configure(dt_dev_viewport_t *port); +/** the four calls below go through dt_dev_proxy_exposure_t: GTK main thread only, + see the thread contract on that struct */ /** get exposure level */ float dt_dev_exposure_get_exposure(dt_develop_t *dev); /** get final effective exposure level including compensations */ diff --git a/src/iop/exposure.c b/src/iop/exposure.c index f99a0c894e..4d771b9ad2 100644 --- a/src/iop/exposure.c +++ b/src/iop/exposure.c @@ -40,6 +40,7 @@ #include "gui/color_picker_proxy.h" #include "iop/iop_api.h" +// 'white' is the value that is mapped to 1.0 after exposure correction #define exposure2white(x) exp2f(-(x)) #define white2exposure(x) -dt_log2f(fmaxf(1e-20f, x)) @@ -86,7 +87,6 @@ typedef struct dt_iop_exposure_gui_data_t GtkLabel *deflicker_used_EC; GtkWidget *compensate_exposure_bias; GtkWidget *compensate_hilite_preserv; - float effective_exposure; // used to cache the final computed exposure float deflicker_computed_exposure; GtkWidget *spot_mode; @@ -103,7 +103,7 @@ typedef struct dt_iop_exposure_gui_data_t typedef struct dt_iop_exposure_data_t { dt_iop_exposure_params_t params; - int deflicker; + gboolean deflicker; float black; float scale; } dt_iop_exposure_data_t; @@ -437,15 +437,13 @@ static double _raw_to_ev(const uint32_t raw, return raw_ev; } -static void _compute_correction(dt_iop_module_t *self, - dt_iop_exposure_params_t *p, +static void _compute_deflicker_correction(dt_iop_exposure_params_t *p, dt_dev_pixelpipe_t *pipe, const uint32_t *const histogram, const dt_dev_histogram_stats_t *const histogram_stats, float *correction) { - *correction = EXPOSURE_CORRECTION_UNDEFINED; - + // preserve caller's correction if we cannot compute anything if(histogram == NULL) return; const double thr @@ -482,6 +480,7 @@ static void _process_common_setup(dt_iop_module_t *self, dt_iop_exposure_data_t *d = piece->data; d->black = d->params.black; + // the default is also the fallback for _compute_deflicker_correction below float exposure = d->params.exposure; if(d->deflicker) @@ -489,7 +488,7 @@ static void _process_common_setup(dt_iop_module_t *self, if(g) { // histogram is precomputed and cached - _compute_correction(self, &d->params, piece->pipe, + _compute_deflicker_correction(&d->params, piece->pipe, g->deflicker_histogram, &g->deflicker_histogram_stats, &exposure); } @@ -498,7 +497,7 @@ static void _process_common_setup(dt_iop_module_t *self, uint32_t *histogram = NULL; dt_dev_histogram_stats_t histogram_stats; _deflicker_prepare_histogram(self, &histogram, &histogram_stats); - _compute_correction(self, &d->params, piece->pipe, histogram, + _compute_deflicker_correction(&d->params, piece->pipe, histogram, &histogram_stats, &exposure); dt_free_align(histogram); } @@ -609,6 +608,45 @@ static float _get_highlight_bias(const dt_iop_module_t *self) return 0.0f; } +// The correction, in EV, that the module adds on top of the user's exposure parameter to +// account for the two biases recorded in EXIF. +// Both directions of the conversion (see below) go through this, so they +// cannot drift apart. +static inline float _exposure_compensation_ev(const dt_iop_module_t *const self, + const dt_iop_exposure_params_t *const p) +{ + float compensation = 0.0f; + + // compensate the correction the user dialed into the camera + if(p->compensate_exposure_bias) + compensation -= _get_exposure_bias(self); + + // undo the underexposure the camera applied automatically + if(p->compensate_hilite_pres) + compensation += _get_highlight_bias(self); + + return compensation; +} + +// The total exposure adjustment the pipe applies in manual mode: the slider plus the +// exposure compensation. commit_params() and the proxy accessor both go through this, +// so the value other modules read cannot drift away from the one that is processed. +static inline float _total_adjustment_ev(const dt_iop_module_t *const self, + const dt_iop_exposure_params_t *const p) +{ + return p->exposure + _exposure_compensation_ev(self, p); +} + +// The inverse: the value to store in p->exposure (i.e. where to move the exposure slider) +// so that the pipe ends up applying `total_adjustment_ev`. The GUI needs it because it +// reasons in terms of the total adjustment - black has to stay below the white point +// derived from it - but writes the user parameter (slider value). +static inline float _required_exposure_slider_ev(const dt_iop_module_t *const self, + const dt_iop_exposure_params_t *const p, + const float total_adjustment_ev) +{ + return total_adjustment_ev - _exposure_compensation_ev(self, p); +} void commit_params(dt_iop_module_t *self, dt_iop_params_t *p1, @@ -619,34 +657,18 @@ void commit_params(dt_iop_module_t *self, dt_iop_exposure_data_t *d = piece->data; d->params.black = p->black; - d->params.exposure = p->exposure; + d->params.exposure = _total_adjustment_ev(self, p); d->params.deflicker_percentile = p->deflicker_percentile; d->params.deflicker_target_level = p->deflicker_target_level; - // If exposure bias compensation has been required, add it on top of - // user exposure correction - if(p->compensate_exposure_bias) - d->params.exposure -= _get_exposure_bias(self); - - // If highlight preservation compensation has been required, add it on top of - // the previous compensation values -// d->params.compensate_hilite_pres = p->compensate_hilite_pres; - if(p->compensate_hilite_pres) - d->params.exposure += _get_highlight_bias(self); - - d->deflicker = 0; - - if (self->gui_data) - { - ((dt_iop_exposure_gui_data_t *)self->gui_data)->effective_exposure = d->params.exposure; - } + d->deflicker = FALSE; if(p->mode == EXPOSURE_MODE_DEFLICKER && dt_image_is_raw(&self->dev->image_storage) && self->dev->image_storage.buf_dsc.channels == 1 && self->dev->image_storage.buf_dsc.datatype == TYPE_UINT16) { - d->deflicker = 1; + d->deflicker = TRUE; } } @@ -762,12 +784,16 @@ void cleanup_global(dt_iop_module_so_t *self) self->data = NULL; } +// Set the exposure parameter (slider value) such that 'white' is mapped to 1.0 after +// commit_params() re-applies the compensations. Callers work in that domain +// because p->black does too - black is never compensated - so the black clamps can +// compare the two directly. static void _exposure_set_white(dt_iop_module_t *self, const float white) { dt_iop_exposure_params_t *p = self->params; - const float exposure = white2exposure(white); + const float exposure = _required_exposure_slider_ev(self, p, white2exposure(white)); if(p->exposure == exposure) return; p->exposure = exposure; @@ -803,7 +829,7 @@ static void _exposure_set_black(dt_iop_module_t *self, if(p->black == black) return; p->black = black; - if(p->black >= exposure2white(p->exposure)) + if(p->black >= exposure2white(_total_adjustment_ev(self, p))) { _exposure_set_white(self, p->black + 0.01); } @@ -821,10 +847,32 @@ static float _exposure_proxy_get_black(dt_iop_module_t *self) return p->black; } +// The exposure that is actually applied, for other modules to read through +// dev->proxy.exposure. Derived from the parameters on the caller's thread, like +// _exposure_proxy_get_exposure() and _exposure_proxy_get_black() above: a value cached +// by the pipe would be one commit behind whenever the caller asks right after a change. +// Reading params this way is only safe on the GTK thread, which owns them - see the +// thread contract on dt_dev_proxy_exposure_t in develop.h. static float _exposure_proxy_get_effective_exposure(dt_iop_module_t *self) { - const dt_iop_exposure_gui_data_t* const g = self->gui_data; - return g->effective_exposure; + const dt_iop_exposure_params_t *const p = self->params; + + if(p->mode == EXPOSURE_MODE_DEFLICKER) + { + // deflicker computes its correction from the raw histogram inside the pipe, so the + // value it caches is the only source; it stays undefined until the preview pipe has + // run once, and there is no GUI to cache it in outside the darkroom + const dt_iop_exposure_gui_data_t *const g = self->gui_data; + if(!g) return 0.f; + + dt_iop_gui_enter_critical_section(self); + const float computed = g->deflicker_computed_exposure; + dt_iop_gui_leave_critical_section(self); + + return computed == EXPOSURE_CORRECTION_UNDEFINED ? 0.f : computed; + } + + return _total_adjustment_ev(self, p); } static void _exposure_proxy_handle_event(int n_press, @@ -886,7 +934,7 @@ static void _auto_set_exposure(dt_iop_module_t *self, dt_dev_pixelpipe_t *pipe) dt_aligned_pixel_t Lab; dot_product(RGB, input_profile->matrix_in, XYZ); dt_XYZ_to_Lab(XYZ, Lab); - Lab[1] = Lab[2] = Lab[3] = 0.f; // make color grey to get only the equivalent lighness + Lab[1] = Lab[2] = Lab[3] = 0.f; // make color gray to get only the equivalent lightness dt_Lab_to_XYZ(Lab, XYZ); dt_XYZ_to_sRGB(XYZ, g->spot_RGB); @@ -905,28 +953,20 @@ static void _auto_set_exposure(dt_iop_module_t *self, dt_dev_pixelpipe_t *pipe) if(mode == DT_SPOT_MODE_MEASURE) { - // get the exposure setting - float expo = p->exposure; - - // If the exposure bias compensation is on, we need to add it to the user param - if(p->compensate_exposure_bias) - expo -= _get_exposure_bias(self); - - // If the highlight preservation mode is on, we need to add it to the user param - if(p->compensate_hilite_pres) - expo += _get_highlight_bias(self); + // the exposure the pipe applies, i.e. the user setting plus the compensations + const float exposure_adjustment = _total_adjustment_ev(self, p); - const float white = exposure2white(-expo); + const float gain = exp2f(exposure_adjustment); // apply the exposure compensation dt_aligned_pixel_t XYZ_out = {0.0f }; for(int c = 0; c < 3; c++) - XYZ_out[c] = XYZ[c] * white; + XYZ_out[c] = XYZ[c] * gain; // Convert to Lab for GUI feedback dt_aligned_pixel_t Lab_out; dt_XYZ_to_Lab(XYZ_out, Lab_out); - Lab_out[1] = Lab_out[2] = 0.f; // make it grey + Lab_out[1] = Lab_out[2] = 0.f; // make it gray // Return the values in sliders DT_ENTER_GUI_UPDATE(); @@ -950,20 +990,9 @@ static void _auto_set_exposure(dt_iop_module_t *self, dt_dev_pixelpipe_t *pipe) dt_aligned_pixel_t XYZ_target = { 0.f }; dt_Lab_to_XYZ(Lab_target, XYZ_target); - // Get the ratio - float white = XYZ[1] / XYZ_target[1]; - float expo = -white2exposure(white); - - // If the exposure bias compensation is on, we need to subtract it from the user param - if(p->compensate_exposure_bias) - expo -= _get_exposure_bias(self); - - // If the highlight preservation mode is on, we need to add it to the user param - if(p->compensate_hilite_pres) - expo += _get_highlight_bias(self); - - white = exposure2white(-expo); - _exposure_set_white(self, white); + // a near-black target cannot be matched, and a near-black sample explodes the ratio + if(XYZ[1] > 1e-5f && XYZ_target[1] > 1e-5f) + _exposure_set_white(self, XYZ[1] / XYZ_target[1]); } } @@ -977,6 +1006,19 @@ void color_picker_apply(dt_iop_module_t *self, } +// keep black under the white point the pipe will map to 1.0, so that +// white - black cannot reach zero or turn negative; the comparison is in the +// compensated domain because that is what the pipe uses, while p->black is +// never compensated +static void _clamp_black_below_white(dt_iop_module_t *self) +{ + const dt_iop_exposure_params_t *const p = self->params; + + const float white = exposure2white(_total_adjustment_ev(self, p)); + if(p->black >= white) + _exposure_set_black(self, white - 0.01); +} + void gui_changed(dt_iop_module_t *self, GtkWidget *w, void *previous) @@ -997,6 +1039,7 @@ void gui_changed(dt_iop_module_t *self, || self->dev->image_storage.buf_dsc.channels != 1 || self->dev->image_storage.buf_dsc.datatype != TYPE_UINT16) { + // force manual for unsupported image type p->mode = EXPOSURE_MODE_MANUAL; dt_bauhaus_combobox_set(g->mode, p->mode); gtk_widget_set_sensitive(GTK_WIDGET(g->mode), FALSE); @@ -1012,17 +1055,30 @@ void gui_changed(dt_iop_module_t *self, break; } } - else if(w == g->exposure) - { - const float white = exposure2white(p->exposure); - if(p->black >= white) - _exposure_set_black(self, white - 0.01); - } - else if(w == g->black) + + // black and the white point must not cross. to ensure that, adjust black if the user + // changed exposure controls, and adjust exposure if they moved black. only in manual + // mode: the manual white point is not applied in deflicker mode, but its hidden + // controls stay reachable via shortcuts, so manual white must not constrain the visible + // and active black + if(p->mode == EXPOSURE_MODE_MANUAL) { - const float white = exposure2white(p->exposure); - if(p->black >= white) - _exposure_set_white(self, p->black + 0.01); + if(w == g->black) + { + const float white = exposure2white(_total_adjustment_ev(self, p)); + if(p->black >= white) + _exposure_set_white(self, p->black + 0.01); + } + else if(w == g->mode + || w == g->exposure + || w == g->compensate_exposure_bias + || w == g->compensate_hilite_preserv) + { + // the white point moved, or manual mode was just entered (normally, or + // forced for an unsupported image) with a black left over from deflicker + // mode's white point + _clamp_black_below_white(self); + } } }