Fix 21974 applied exposure disagreement

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

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 */

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,14 +480,15 @@ 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)
   {
     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);
+    }
   }
 }