darktable-org / darktable-org/darktable
toneequal: gui_post_expose() initialises the graph cache from the wrong widget
Nobody has claimed this yet.
- Dominant language
- C
- Stars
- 13.1k
- Forks
- 1.4k
- Avg merge
- 22h 14m
- Merged PRs (30d)
- 198
Description
Found by code analysis, 2026-09-03.
Severity: very low — latent. gui_post_expose() fills the equalizer graph's
geometry cache from the allocation of the module's top-level box instead of the graph
drawing area. The wrong values are real and provable, but no path was found that reads
them before they are overwritten, so nothing observable is claimed here. See Why
nothing breaks today, which is the part of this report a triager should read first.
The reason to file it anyway is that the call site is provably a leftover: the identical
call in area_draw() was corrected in 2020 and this copy was not, and the call is dead
weight in its own right — gui_post_expose() reads nothing that _init_drawing()
produces.
The defect
src/iop/toneequal.c has two callers of _init_drawing() (:2420-2559). They disagree
about which widget the graph belongs to:
2268: if(!g->graph_valid)
2269: if(!_init_drawing(self, self->widget, g)) // gui_post_expose()
2270: return;
2604: if(!_init_drawing(self, widget, g)) // area_draw(), widget == g->area
2605: return FALSE;
_init_drawing() derives the whole cache from that argument:
2425: gtk_widget_get_allocation(widget, &g->allocation);
...
2447: g->context = gtk_widget_get_style_context(widget);
...
2466: g->inset = g->inner_padding + darktable.bauhaus->quad_width;
2468: g->graph_width = g->allocation.width - g->inset - 2.0 * g->line_height;
2470: g->graph_height = g->allocation.height - g->inset - 2.0 * g->line_height;
2471: g->gradient_left_limit = 0.0;
2472: g->gradient_right_limit = g->graph_width;
2473: g->gradient_top_limit = g->graph_height + 2 * g->inner_padding;
2474: g->gradient_width = g->gradient_right_limit - g->gradient_left_limit;
2475: g->legend_top_limit = -0.5 * g->line_height - 2.0 * g->inner_padding;
2476: g->x_label = g->graph_width + g->sign_width + 3.0 * g->inner_padding;
and it also sizes the cached Cairo surface from it (:2429-2430) and paints the CSS
background over it (:2478).
The two widgets are not the same object and not the same size. At the end of
gui_init(), self->widget is the module's outer box:
3246: self->widget = dt_gui_vbox(g->notebook,
3247: dt_gui_hbox(dt_gui_expand(dt_ui_label_new(_("display exposure mask"))),
3248: g->show_luminance_mask));
g->area is a plain gtk_drawing_area_new() (:3119), wrapped in a vbox (:3120) and
packed into the advanced notebook page (:3117, :3150). So self->widget's
allocation spans the notebook tab strip, the whole of whichever page is current, and the
"display exposure mask" row underneath — where the drawing area is one vexpanding child
inside one of those pages. graph_height computed from it is far too large; the style
context belongs to the module box's CSS node rather than the #toneeqgraph one.
Why nothing breaks today
Three things have to line up before a wrong graph_width / graph_height / inset
reaches a user, and the third does not.
1. gui_post_expose() never reads what it just cached. Its body (:2242-2347) uses
g->cursor_valid, g->interpolation_valid, g->has_focus, g->luminance_valid,
g->cursor_exposure, g->cursor_pos_x / _y, g->factors, g->sigma,
centers_params[] and g->area_active_node. It draws through the cairo_t *cr the
darkroom hands it, not through g->cr. Not one field _init_drawing() writes is read
by the function that calls it.
2. area_draw() re-initialises unconditionally, so the cache is repaired before the
graph is ever painted. The graph_valid test there is commented out, with the reason
left in the source:
2599: // Init or refresh the drawing cache
2600: //if(!g->graph_valid)
2601:
2602: // this can be cached and drawn just once, but too lazy to debug a
2603: // cache invalidation for Cairo objects
2604: if(!_init_drawing(self, widget, g))
3. The only outside consumers of the geometry are the drawing area's own input
callbacks, which cannot fire before that repair. Outside _init_drawing() and
area_draw(), graph_width / graph_height / inset are read at exactly two places:
area_motion_notify():2890-2891 and :2900-2905, and area_leave_notify():2807-2808.
Both are connected to g->area (:3133), so the pointer has to be over the drawing
area, which means GTK has mapped and drawn it, which means area_draw() has run. The
node hit-test in area_motion_notify():2908-2921 is additionally gated on
g->valid_nodes_x, which only init_nodes_x() sets and only area_draw():2705 calls.
That leaves a narrow window with nothing in it. It opens when gui_post_expose() runs
before the graph area has ever been drawn — the module focused with the simple or
masking page current, since a GTK3 notebook maps and allocates only the current page,
so the advanced page's drawing area gets no draw. graph_valid is FALSE at that point
(gui_cache_init():1355, reached only from gui_init():3068), so :2269 fires and
writes the module-box geometry. Switching to the advanced page then maps the area and
draws it, and area_draw() overwrites every field with the correct values before the
first motion event can be handled.
So this is filed as a latent defect, not a bug with a reproducer. If someone can
construct an event order where a motion event on g->area precedes its first draw, the
consequence is a graph whose node hit-testing and drag scaling use a much-too-tall
coordinate system; I could not construct one, and none of this was run.
Why it is nevertheless wrong rather than deliberate
Both call sites passed self->widget when the gui_post_expose() one was added
(280f5e0ced, 2019-09-23, "init drawing if not already initialized in gui_post_expose").
e0ca58114b (2020-11-01, "don't force square aspect on graph in tone equaliser") changed
area_draw() to pass its own widget argument and left the gui_post_expose() copy
alone:
- if(!_init_drawing(self->widget, g)) return FALSE; // this can be cached and drawn just once, ...
+ if(!_init_drawing(widget, g)) return FALSE; // this can be cached and drawn just once, ...
Before that commit the area was dtgtk_drawing_area_new_with_aspect_ratio(1.0) and the
two allocations were closer to interchangeable. After it they are not, and the surviving
call has been passing the module box ever since.
Two smaller signs the site is vestigial:
_init_drawing()has a single exit,return TRUE(:2557). Theif(!_init_drawing(...))
guards at:2269and:2604can never take their failure branch, soarea_draw()'s
return FALSEat:2605is unreachable too.g->graph_validhas exactly one reader,:2268. Nothing else in the file consults it;
area_draw()deliberately does not.
Suggested fix
Delete the call, and with it the flag it exists to serve:
if(fail) return;
- if(!g->graph_valid)
- if(!_init_drawing(self, self->widget, g))
- return;
-
// Re-read the exposure in case it has changed. ...
gui_post_expose() loses nothing, per point 1 above, and area_draw() is unaffected
because it never tested the flag. g->graph_valid then becomes write-only and can go
with it (declaration :303, init :1355, write :2555) — or be left in place if a
future change means to restore the caching that :2600 disabled.
The minimal alternative, passing GTK_WIDGET(g->area) instead of self->widget, makes
the argument correct but does not make the call useful, and in the one scenario that
reaches it the area is unallocated, so the cache would be filled from GTK's 1x1 default
rather than from the module box — no better, just differently wrong.
Related reports
- #22068, #22091, #22121, #22133 are the other
toneequalreports in the
corpus. All four concernluminance_valid/gui_lock/commit_params()
interactions; none touches_init_drawing(),graph_validor the graph geometry, and
this report shares no code with them. - Not checked against upstream — no
ghin this environment, so a pre-existing
GitHub issue cannot be ruled out.
Environment
Verified line by line against 3c73bf2aaa (branch dt-lockcheck; src/ matches the
2026-09-03 upstream rebase). Line numbers differ slightly from row 44 of
discipline-gap-validated.md, which was written against 4d9e40f30e: the
gui_post_expose() call is :2269 with the guard at :2268, area_draw()'s call is
:2604 with the commented-out guard at :2600, graph_valid = TRUE is :2555, and the
geometry block is :2466-2476 rather than :2463-2474. Static analysis only — nothing
here was built or run.
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Read src/iop/toneequal.c, starting with gui_post_expose(), area_draw(), and _init_drawing(); compare the two callers and the graph_valid declaration, initialization, and write. Confirm that area_draw() remains the only useful cache initialization, then make the minimal cleanup described in the issue and verify the file builds successfully.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- c
- Domain
- desktop
- Issue type
- Bug
- Difficulty
- 2/5
- Estimated time
- 1-3 hours
- Activity status
- Active
- Clarity
- Clearly specified
- Newbie friendliness
- 68/100