rgblevels writes its auto-levels request from the GTK thread without the lock

Is there an existing issue for this?

• I checked and did not find my issue in the already reported ones

Describe the bug

Found by Claude + Codex via code analysis.

Description

rgblevels can compute its levels automatically, over the whole image or over a
rectangle the user drags on the image. The work is done by the pixelpipe, and the GUI asks
for it through two fields in the shared GUI data: a state flag, call_auto_levels, and
the rectangle, box_cood. The flag is a small state machine — 0 idle, 1 requested,
-1 being computed, 2 result ready — and every transition on the pipe side is made
inside the critical section. On the GTK side only some writers do the same: the one that
turns the region picker off takes the lock and only asks for a new run when the state is
0, but button_released(), which is how a dragged rectangle is submitted, writes all
four rectangle values and then sets the flag to 1 with no lock at all. The result is
lost requests and dropped results — an auto-levels request made while a previous one is
still being computed can overwrite the -1 or 2 state and be silently discarded, or
discard the finished result before the GUI has stored it. The rectangle itself can also be
read half-updated by the pipe, giving levels computed over an area the user never
selected. Separately, g->channel, which says which of the R, G and B tabs is open, is
written by the GTK thread and read by the pipe with no lock on either side, so changing
tab while a run is in progress can apply the computed levels to a different channel than
the user was looking at. Nothing here is memory-unsafe: the channel value comes from the
notebook page number and stays within the array.

Evidence

All code is in src/iop/rgblevels.c.

button_released() runs on the GTK thread and writes both fields unlocked:

      g->box_cood[0] = g->posx_from;
      g->box_cood[1] = g->posy_from;
      g->box_cood[2] = g->posx_to;
      g->box_cood[3] = g->posy_to;
      dt_dev_distort_backtransform(darktable.develop, g->box_cood, 2);
      g->box_cood[0] /= darktable.develop->preview_pipe->iwidth;
      ...
      g->button_down = 0;
      g->call_auto_levels = 1;

      dt_dev_reprocess_all(self->dev);

The other GTK-side request, in _turn_selregion_picker_off(), shows the intended shape —
lock held, and a new run only started from the idle state:

  dt_iop_gui_enter_critical_section(self);
  if(g->call_auto_levels == 0)
  {
    g->box_cood[0] = g->box_cood[1] = g->box_cood[2] = g->box_cood[3] = 0.f;
    g->call_auto_levels = 1;
  }
  dt_iop_gui_leave_critical_section(self);

The pipe side, in process(), tests and advances the flag under the lock, but reads the
rectangle and the channel after releasing it:

    dt_iop_gui_enter_critical_section(self);
    if(g->call_auto_levels == 1 && !DT_IN_GUI_UPDATE())
    {
      g->call_auto_levels = -1;

      dt_iop_gui_leave_critical_section(self);

      memcpy(&g->params, p, sizeof(dt_iop_rgblevels_params_t));

      int box[4] = { 0 };
      _get_selected_area(self, piece, g, roi_in, box);      // reads g->box_cood, no lock
      _auto_levels((const float *const)ivoid, roi_in->width, roi_in->height, box,
                   &(g->params), g->channel, work_profile); // reads g->channel, no lock

      dt_iop_gui_enter_critical_section(self);
      g->call_auto_levels = 2;
      dt_iop_gui_leave_critical_section(self);
    }

process_cl() has the same shape. _get_selected_area() copies the four values out
without any lock:

    dt_boundingbox_t box_cood = { g->box_cood[0], g->box_cood[1],
                                  g->box_cood[2], g->box_cood[3] };

The result is picked up on the GTK thread when the preview pipe finishes, again under the
lock, which is what the 2 state is for:

  dt_iop_gui_enter_critical_section(self);
  if(g->call_auto_levels == 2)
  {
    g->call_auto_levels = -1;
    dt_iop_gui_leave_critical_section(self);
    memcpy(p, &g->params, sizeof(dt_iop_rgblevels_params_t));

An unlocked g->call_auto_levels = 1 from button_released() can land on any of these
states, including the -1 that means "a thread is working on this right now".

g->channel is written on the GTK thread in _tab_switch_callback() and in
gui_changed(), both without the lock:

  g->channel = (dt_iop_rgblevels_channel_t)page_num;

Suggested fix

Make button_released() follow the shape the module already uses elsewhere: take the
critical section, only start a new run when call_auto_levels is 0, and write the
rectangle and the flag inside it.

  dt_iop_gui_enter_critical_section(self);
  if(g->call_auto_levels == 0)
  {
    g->box_cood[0] = ...;
    ...
    g->call_auto_levels = 1;
  }
  dt_iop_gui_leave_critical_section(self);

Note that dt_dev_distort_backtransform() should stay outside the section — compute the
four values into locals first, then copy the locals in under the lock, so that no real work
is done while the lock is held.

On the pipe side, copy box_cood and channel into locals inside the section that is
already there, in the same place where the state is set to -1, and pass the locals to
_get_selected_area() and _auto_levels(). The GTK-side writes of g->channel in
_tab_switch_callback() and gui_changed() need the lock as well.

Steps to reproduce

No known problem reported before. Derived from reading the code, not carried out. The lost-request case has the widest
window, because the -1 and 2 states last as long as the computation and the redraw:

1. Open an image in the darkroom and enable rgb levels. Use a large image, and turn
   OpenCL off, so that the automatic computation takes a noticeable time.
2. Press the "auto region" button, then drag a rectangle inside the image and release the
   mouse. This submits the first request.
3. Immediately drag and release a second rectangle, before the image finishes updating.
4. Watch for the levels not changing at all, or changing to a result that matches neither
   of the two rectangles.
5. For the channel case, switch the R/G/B tab immediately after releasing the mouse, and
   watch which channel's levels change.

A build with ThreadSanitizer reports the unsynchronised accesses directly and does not
depend on the timing producing a visible result. A slow pipeline (HQ, large file, no OpenCL) may help reproducing.

Expected behavior

No response

Logfile | Screenshot | Screencast

No response

Commit

No response

Where did you obtain darktable from?

self compiled

darktable version

6783f7c4

What OS are you using?

Linux

What is the version of your OS?

Ubuntu 26.04

Describe your system

No response

Are you using OpenCL GPU in darktable?

None

If yes, what is the GPU card and driver?

No response

Please provide additional context if applicable. You can attach files too, but might need to rename to .txt or .zip

No response