Draft: dev-doc: document sharing gui_data between GUI and pipe threads - #21912
Draft
kofa73 wants to merge 64 commits into
Draft
Draft: dev-doc: document sharing gui_data between GUI and pipe threads#21912kofa73 wants to merge 64 commits into
kofa73 wants to merge 64 commits into
Conversation
kofa73
marked this pull request as draft
August 18, 2026 20:42
kofa73
force-pushed
the
dev-doc/gui-data-sharing-pr
branch
2 times, most recently
from
August 23, 2026 07:25
53484a6 to
2f1c4f1
Compare
kofa73
marked this pull request as ready for review
August 23, 2026 07:29
Collaborator
Author
|
The large amount of text added has now been extracted to its own file; the fix for #21891 is now taken into account. Some redundancy eliminated. |
kofa73
marked this pull request as draft
August 23, 2026 07:33
1 task
kofa73
force-pushed
the
dev-doc/gui-data-sharing-pr
branch
2 times, most recently
from
August 28, 2026 16:38
fce1611 to
231928f
Compare
GUI.md section 3 covered only updating the GUI from process(). It now covers both directions, and states the rule that was never written down: a widget callback may write gui_data, but needs the GUI critical section whenever a pipe thread also touches the field. Adds which callbacks the pipe drives (their static helpers included), the preconditions for touching gui_data from commit_params(), locking rules for dev->proxy accessors, and why a critical section must span the lifetime of the value rather than just the load. IOP_Module_API.md gets a matching note on commit_params() and a fixed anchor. Also states the framework-side teardown contract that the queued-callback advice rests on: no module GUI is torn down while a pipe is running, and dt_iop_gui_cleanup_module() nulls gui_data. That makes reading gui_data once in process() safe for the length of the run, and makes draining queued idle sources in gui_cleanup() sufficient rather than best-effort - but only if the drain loops, since a source that survives now hits a NULL dereference instead of freed memory.
GUI.md section 3 had grown to 476 of the file's 868 lines, more than the UI construction, event and reparenting sections together, and none of it is what a reader opening GUI.md for widget layout is looking for. Extract it verbatim to GUI_Threading.md, one topic per file as the rest of dev-doc already is. Heading levels drop by one; the text is otherwise unchanged, so the anchors and every cross-reference inside it still resolve. GUI.md keeps a short section 3 stating the problem and pointing at the new file, which leaves the section numbering intact. README.md, IOP_Module_API.md and New_Module_Guide.md described GUI.md as the thread-safety reference; they now name GUI_Threading.md instead.
The paragraph on draining queued sources claimed the survivor always dereferences a NULL gui_data. That only holds where the module struct outlives its GUI, which is the instance-delete and undo/redo path. Leaving the darkroom (src/views/darkroom.c) and switching image both free the struct right after dt_iop_gui_cleanup_module(), so the callback reads self->gui_data through a dangling pointer: it may fault, or return a stale pointer that still looks usable.
The paragraph said an undrained source dereferences NULL after an instance delete, but Pattern A tells the reader to put an if(!self->gui_data) check at the top of the callback, which catches exactly that case. Stated the two paths in terms of what the check does: it returns cleanly after a delete, and it is already too late after a darkroom exit or image switch, where reading self->gui_data is itself the use-after-free. Drop the claim that the freed read yields a usable stale pointer. It is undefined, and the value is unpredictable in both directions: cleanup nulls gui_data before the struct is freed, so the bytes often still read NULL until the chunk is reused.
Four corrections from review, all documentation/code-sync defects: - Image switching does not free every module. darkroom.c keeps each module's base instance across the switch: no gui_cleanup(), gui_data and gui_lock stay alive, and the instance is reused after reload_defaults() and change_image(). Only the extra instances are cleaned up and freed. The section described the switch as a darkroom-exit-shaped teardown, which made the NULL check look too late when for a base instance it is blind instead: it passes, and the survivor writes the previous image's value into the new image's widgets. Split the teardown into three shapes, and add change_image() as the second cancellation point, since gui_cleanup() never runs there. - default_colorspace() has a direct GTK-thread call site too: the color picker constructor calls it with NULL pipe and piece. It was described only as something default_blend_colorspace() forwards to. - overlay violates the no-GTK-from-a-pipe-thread rule in tree: _setup_overlay(), reached from process() whenever the overlay buffer has to be built, sets a tooltip on a gui_data widget and can queue a redraw. Named next to the denoiseprofile call-chain example, since the point of that paragraph is that the mistake hides one call down. - exposure is named as the proxy example, but neither its producer nor its accessor takes gui_lock and the accessor does not check gui_data. Say so, so the example is not read as a description of what exposure does. Also: only two of the four teardown sites use dt_dev_pixelpipe_stop_and_lock_all(); darkroom exit and the image switch take the same three mutexes directly.
GUI_Threading.md now tells authors to cancel queued idle sources in change_image(), because an image switch keeps each module's base instance and never runs its gui_cleanup(). The callback itself was documented nowhere: the API page named reload_defaults() as the image-switch callback and its lifecycle diagram went straight from it to gui_update(), and GUI.md's external-change path did not mention it either. An author following those pages would not know the hook exists. Add a change_image() section to IOP_Module_API.md covering both of its jobs — resetting gui_data that describes the old image, and cancelling work queued for it — and place it in the lifecycle diagram and in GUI.md's path C, inside the DT_ENTER_GUI_UPDATE() guard where it actually runs. Two precision fixes in GUI_Threading.md while here: the darkroom-exit shape applies to modules that have a GUI (hidden modules never get one), and it is pipe threads specifically that cannot queue behind the change_image() drain. Also fixed: the blank line 442ab77 left at the end of GUI.md, which git diff --check flags across the range. Not changed: New_Module_Guide.md's optional-function list. It is a six-item starter set that already omits most of the API and links to the full list; change_image() is not a callback a first module needs.
Three refinements to the section added in the previous commit: - Path C had the parameter load before DT_ENTER_GUI_UPDATE(). Both paths it describes enter the guard first: the image switch at darkroom.c:1498, before reload_defaults() and change_image(), with the history params copied afterwards by dt_dev_pop_history_items(); undo and preset loads through dt_dev_pop_history_items() itself, which enters the guard and then copies. Reordered, with reload_defaults() named alongside change_image() in the image-switch-only step. - "each module's base instance is kept, GUI and all" — only the visible base instance has a GUI to keep; dt_iop_is_hidden() modules never get one. - "gui_cleanup() does not run on this transition" was true of the retained instance and false of the transition: the extra instances do run it before being freed. Scoped to the retained instance. Also name rgblevels and rgbcurve, which implement change_image() too.
Found while checking the pages this branch touches. None of these are about gui_data threading; they are in the same files and are wrong against the current tree, so they are separated into this commit and can be dropped without affecting the rest. - IOP_Module_API.md said the framework hashes piece->data after commit_params() returns. dt_iop_commit_params() hashes the op name, instance, module->params, and the blend params and mask group when blending is on (src/develop/imageop.c). Nothing reads piece->data. Described what is hashed, and what the rest of the cache key adds (image id, pipe type, detail mask, the four pipe profiles, upstream piece hashes, ROI - dt_dev_pixelpipe_cache_hash()), because that is what decides which inputs an author has to worry about: pipe type and profiles are in the key already; a preference read in commit_params() is not. colorout is cited for both halves. - commit_params() said its job is to translate self->params. It gets the parameters as an argument, and the pipe's defaults sync passes default_params, not self->params (src/develop/pixelpipe_hb.c:765). - The lifecycle diagram had darkroom exit as gui_cleanup() first. The pipes are cleaned first: dt_dev_pixelpipe_cleanup_nodes() calls cleanup_pipe() for every piece (darkroom.c:4229, pixelpipe_hb.c:467), and only then does the module loop call gui_cleanup() (darkroom.c:4242). - Image Open omitted the second dt_iop_reload_defaults(), which darkroom setup calls after dt_iop_gui_init() (darkroom.c:3985). Marked simplified and both reloads shown. - gui_changed() was presented as unconditional in four places: GUI.md's prose and path A, README.md's flow, and imageop_gui.md's checklist. The callback is optional; dt_iop_gui_update() never calls it, and the widget dispatcher calls it only if(module->gui_changed) (src/develop/imageop.c:2440, :4321). Qualified in all four.
The single sequence claimed the framework opens the GUI-update guard before it loads the params. That is true of an image switch (darkroom.c:1498 before the history copy at develop.c:1717) and of undo and history navigation (dt_dev_pop_history_items() enters the guard, then copies), but false of a preset applied directly: dt_gui_presets_apply_preset() copies into module->params first and only then calls dt_iop_gui_update(), which is what opens the guard (presets.c:1076, :1116). Show the image switch in full, since it is the one with the extra steps this branch documents, and state the other two as differences from it. Also add the history-params load to the API quick reference's image switch line, which jumped from change_image() straight to gui_update(), and match its 'visible base instance' wording.
Two errors in the split added by the previous commit: - "Undo, redo, history navigation: the same, without reload_defaults() and change_image()" is true only of a parameter-only change. Undo or redo of a module add or delete creates or destroys instances and goes through dt_dev_reload_history_items() (src/libs/history.c:598-635), which can gui_init() and reload defaults; GUI_Threading.md already treats that route as a GUI teardown site, so path C contradicted it. - Copy/paste of history was named as an external-change source two paragraphs later but had no route here; it uses the same reload machinery (src/common/history.c:990), as do styles. Both are now one case: routes that can change the module list are not a params load. Also mark change_image() and gui_changed() conditional in the image-switch sequence - both are optional callbacks - and drop "params you did not ask for", which is wrong for a preset the user picked.
Follow-up to the caching correction in 68c71a5, which listed what the key adds without saying that it is a list of selected fields. Read as an inventory, it invites the conclusion that anything reachable through pipe or dev is covered. Name the ROI-dependent inputs the list omitted (ROI, scharr, picker sample), then say plainly that the key does not hash the pipe or the image record wholesale, with the three cases that catch people: image metadata other than the id (the id identifies the image, it does not version it - exposure reads exif_exposure_bias and exif_highlight_preservation), profile contents behind the hashed profile-info pointers, and process-global state such as a preference. pixelpipe_architecture.md's abbreviated inventory, which the API page links to for caching, had the same shape plus two omissions: the export profile and the ROI-based inputs. Corrected there too, since a reader sent to it for detail should not find a shorter list. Also add the note the quick-reference block was missing: its arrows show optional callbacks where a module that implements them is called. This commit and 68c71a5 are one droppable unit - both are corrections to text that predates this branch.
Read collectively the paragraph was right, but 'they can create and destroy instances' invited reading both halves into each route. Undo and redo of an add or delete do both (src/libs/history.c:481-536 creates, :413-445 cleans up); a history paste or style application adds what the incoming history needs and re-synchronises the rest (dt_dev_reload_history_items(), develop.c:1618-1678) without that teardown path. Split into two bullets.
The section used exposure's cached effective_exposure as the case that motivates the locking rule, but never stated the resolution: a value that can be recomputed from params and the image does not belong in gui_data at all, and then there is nothing to lock. Rewrite the passage around that lesson, with exposure showing both halves — manual mode derives, deflicker mode genuinely cannot and so locks both sides. The manual-mode half describes the module after darktable-org#21974, which has not landed; a visible note and a greppable TODO marker record that.
The new proxy section called exposure's whole deflicker branch a correct instance of Pattern A. Only the scalar is. gui_update() and gui_changed() free g->deflicker_histogram and rebuild it on the GTK thread while _process_common_setup() reads that pointer and its statistics on a pipe thread, neither side under gui_lock — in gui_update() the free even sits between two critical sections that guard other fields. Scope the claim to deflicker_computed_exposure and name the histogram as what it is: a live in-tree instance of the trap in "Short Is Not The Same As Correct". Also drop the premise that only the pipe computes the histogram; the GUI computes and caches it, and the pipe only builds its own when there is no GUI. Found by Codex in the 19-30 round.
A proxy accessor takes whatever instance its caller hands it, and one instance in the tree has gui_data without a dev: _init_module_so() builds a throw-away instance with dev == NULL at startup and runs dt_iop_gui_init() on it so the widgets can register their accelerators. Written self->dev->gui_attached && g, a guard would dereference NULL in its first operand, ahead of the test that decides the question; written the other way round the case cannot arise. colorequal and toneequal already order it that way. The same instance explains why the dev->proxy function pointers outlive every live instance — exposure's gui_cleanup() clears proxy.exposure.module but nothing clears the pointers — and why the in-tree callers resolve a live instance themselves rather than trusting the pointer.
The doc glossed self->dev->gui_attached as "the darkroom is live" and the guard block commented it "darkroom active". Both are false. The field is set once, when the dt_develop_t is created, and darktable.develop is given TRUE at src/common/darktable.c before the init_gui branch is reached, so it is TRUE under darktable-cli as well. dt_iop_gui_init() initialises gui_lock immediately before calling gui_init(), so a non-NULL gui_data already implies a usable lock, and any dev whose modules were loaded without a GUI leaves gui_data NULL regardless. Say that g != NULL is the whole guard and that gui_attached is kept as the prevailing in-tree form (about forty places under src/iop) and as a statement of intent. Same correction in IOP_Module_API.md, which repeated the advice, and point its cross-reference at the section that now explains it.
The text claimed a non-NULL gui_data already implies a usable lock. That holds on the dt_iop_gui_init() lifecycle but is not universal: when undo or redo has to recreate a deleted instance, _create_deleted_modules() in src/libs/history.c calls module->gui_init() directly on a module that dt_iop_load_module() has just memset to zero, so gui_lock is never passed to dt_pthread_mutex_init(). The same path installs an expander, which makes dt_dev_reload_history_items() skip the dt_iop_gui_init() that would otherwise repair it, and exposure's gui_init() enters the critical section during that very call. Say that it is the normal lifecycle rather than a guarantee, and name the exception. A module cannot detect or fix this from commit_params(); the note exists so the implication is not leaned on. Found by Codex in the 20-12 round.
"commit_params() must never block waiting for the GTK thread" contradicted the section that follows it, which requires commit_params() to take gui_lock — a mutex the GTK thread also holds, so acquiring it can block on exactly that thread. Say what the rule is actually about: never wait for the GTK main loop to run something. Brief contention on gui_lock is required and is not the hazard; a synchronous round-trip through the main loop is, because the GTK thread may itself be waiting on a pipe. Found by Codex in the 20-12 round.
The note opened "The manual-mode half of that ...", but the histogram paragraph now sits between the two bullets and the note, so "that" no longer has a clear antecedent. Point at the bullet directly, and say which two pieces of deflicker text describe the tree as it stands.
The data-flow diagram put "auto-callback (from_params) or manual callback" on one line and then had self->params updated and gui_changed() called for both. Only the from_params route does that: dt_iop_gui_changed() is reached from bauhaus only when the widget has a linked field, and it also adds the history item itself. A manual callback — channelmixer's red_callback() is typical — writes self->params and calls dt_dev_add_history_item() directly, and never reaches gui_changed(), so dependent visibility, sensitivity or labels go stale unless the module calls it. Split the two routes in the diagram. Found by Codex in the 20-12 round.
262ccac745 corrected the dt_bauhaus_toggle_from_params() section but not "Key Points to Remember" at the foot of the same file, which still told the reader that toggles need a manual gui_update() unlike sliders and comboboxes. So that commit replaced one wrong statement with two contradictory ones. State the distinction the corrected section draws: bound _from_params widgets, toggles included, are synced by dt_bauhaus_update_from_field(); unbound dt_iop_togglebutton_new() buttons are not. Found by Gemini in the 00-38 round.
GUI.md carried the same stale claim and the same bad cast that 262ccac745 and 91849f5 removed from imageop_gui.md: "you only need to manually sync toggle buttons", with a sample gui_update() casting a _from_params toggle through GTK_TOGGLE_BUTTON(). Bound _from_params widgets, toggles included, are synced by dt_bauhaus_update_from_field() before gui_update() runs, and a Bauhaus toggle derives from GtkDrawingArea. The example now syncs a dt_iop_togglebutton_new() button, which really is a GtkToggleButton (dtgtk/togglebutton.c), so the GTK setter shown is correct for it. Also two summaries that this PR's own material contradicts. GUI.md said gui_init() runs once per instance on entering the darkroom, and IOP_Module_API.md and README.md said gui_data exists only in darkroom mode; the startup accelerator probe, which GUI_Threading.md now documents as load-bearing for proxy accessors, calls gui_init() and allocates gui_data with dev == NULL before any view is loaded. And scope the word "safe" in the IOP_Module_API.md guard bullet to what it covers — the dev dereference — since g != NULL is not by itself proof that gui_lock was initialised. Findings 1-3 from Codex in the 00-38 round.
Both patterns are presented for copying, and each is missing the guard the
other one shows.
Pattern A's process() fragment queues its update with no guard at all, three
paragraphs after "Always check these conditions before scheduling a GUI update
from process()" and above a Common Mistakes entry that lists the omission as
WRONG ("No pipe type check at all ... every pipe that runs the module queues
its own update"). Since the document closes by recommending Pattern A over
Pattern B, that is the snippet a reader is most likely to copy.
Pattern B's callback lost the "if(g)" check the pre-branch text in GUI.md had,
while Pattern A kept its equivalent. It needs it more, not less: a source keyed
on the message cannot be cancelled by g_idle_remove_by_data(), so for a deleted
instance -- struct parked in dev->alliop, gui_data cleared -- the check is the
only thing before the memcpy dereferences NULL.
The same callback also reached its widget through msg->self->widget. That field
is destroyed and NULLed by dt_iop_gui_cleanup_module() immediately beside the
gui_data free, with an in-source comment saying it is nulled because
asynchronous signals can still carry the module. Routing every dereference
through g instead makes the one NULL check cover the whole callback body rather
than part of it.
Findings 4 and 3 of the 04-52 review.
README.md's "Key Functions to Implement" is the index a module author works from, and this PR made change_image() load-bearing without adding it: it gained a section of its own in IOP_Module_API.md, GUI_Threading.md requires draining queued idle sources there because gui_cleanup() does not run on every transition that invalidates the payload, and GUI.md's path C names it in the image-switch flow. The table listed thirteen other callbacks and not that one, so a reader who works from the index would not learn it exists -- which is exactly the reader who most needs the cancellation rule. change_image is declared OPTIONAL in src/iop/iop_api.h, called from src/views/darkroom.c on the image switch, and implemented by basicadj, retouch, rgblevels and rgbcurve. Finding 5 of the 04-52 review.
…file Recipe 6's gui_update() carried both of the errors this PR has now corrected elsewhere, and README.md:26 describes this file as the "Copy-paste patterns" one, so it is the worst place to leave them. The toggle line is the same defect 262ccac745, 91849f5 and b4ce0df removed from imageop_gui.md (twice) and GUI.md: a _from_params toggle is bound to its field and synced by dt_bauhaus_update_from_field() before gui_update() runs, and DtBauhausWidget derives from GTK_TYPE_DRAWING_AREA, so the GTK_TOGGLE_BUTTON() cast is a type error rather than redundant work. Switched to the unbound dt_iop_togglebutton_new() button the GTK setter is actually right for, with the same comment GUI.md now uses. And "**Always** call gui_changed()" is the rule this PR made conditional in the other three places that state it -- GUI.md, imageop_gui.md Key Point 7 and README.md's data flow diagram -- because gui_changed is OPTIONAL in src/iop/iop_api.h and dt_iop_gui_changed() guards on it. This file still said Always, in bold, over an example that calls it unconditionally. GUI_Recipes.md:88 is not a defect and is left alone: Recipe 3 builds that widget with dt_iop_togglebutton_new(), and GtkDarktableToggleButton really does derive from GTK_TYPE_TOGGLE_BUTTON (src/dtgtk/togglebutton.c). Finding 6 of the 04-52 review. Outside this PR's original diff; pulling it in rather than deferring it was Kofa's call.
"The Problem" claimed two directions -- pipe to GUI and GUI to pipe -- and the document then covered only those. There is a third in tree: one pipe publishes into gui_data and another pipe reads it, and the framework has a primitive for it, dt_dev_sync_pixelpipe_hash() (src/develop/develop.c). levels, globaltonemap, hazeremoval and colorreconstruction all use it, each passing &self->gui_lock; toneequal does the same with a helper of its own. A module that needs a property of the whole image cannot get it on the full pipe, which sees only the ROI, so it computes it on the preview pipe and hands it across. Four things about that idiom do not follow from anything else here: - g != NULL is not a test for a GUI. Nothing is displayed; the channel lives in gui_data and so exists only when a GUI does. The "processing must not depend on the GUI cache" rule still holds, and all four modules have the fallback. - process() legitimately blocks on another pipe. The document forbids waiting for the GTK main loop and says nothing about waiting for a pipe, which reads as forbidden by the same argument. It is not: the wait is bounded by pixelpipe_synchronization_timeout (opencl_synchronization_timeout on a GPU pipe), aborts on pipe shutdown, and falls back to computing the value. - gui_lock must not be held across the call. The primitive takes the lock it is handed on every polling iteration, so calling it inside a critical section self-deadlocks on the non-recursive mutex. It also takes dev->history_mutex underneath, via dt_dev_hash_plus(), but releases gui_lock first, so the two are never nested. - A scalar hands over cleanly and a heap object does not. colorreconstruction copies a frozen bilateral grid pointer out under the lock and thaws it after releasing, while the preview pipe's next run and gui_update() can free it -- named here as a live instance of the trap "Short Is Not The Same As Correct" describes, not as a shape to copy. Written up separately in new_source_bugs/. Finding 2 of the 04-52 review.
src/develop/preview_data.{c,h} is an in-tree service whose file comment says it
exists so that modules displaying a per-pixel value under the cursor stop
duplicating buffer management, hashing and locking. It takes the module's own
gui_lock internally in all seven of its data entry points, and its header
guarantees the property "Short Is Not The Same As Correct" derives by hand:
resize, fill and hash commit happen inside a single critical section, so the GUI
can never observe a resized but not-yet-filled buffer.
The document never mentioned it, so a reader who follows the document writes the
duplication the service was added to remove -- and this document's own reference
module, toneequal, is one of its two users; colorequal is the other. Added a
section after the hand-rolled derivation, with a pointer from the rules summary
at the top, and scoped to what it does replace: the plumbing for that one case,
not gui_lock for a module's other shared fields.
The same section also gained the failure mode the tuple rule does not cover: a
buffer refilled *in place* while the flag beside it still advertises the old
contents. Clearing the flag under the lock before the fill and committing hash
plus flag under the lock after is what toneequal does around
compute_luminance_mask(), with a comment saying why (src/iop/toneequal.c). That
is distinct from the pointer/dimension mismatch already described, and no
locking on the reading side catches it.
Finding 1 of the 04-52 review.
Self-review of 299e37d against src/develop/develop.c and the four modules. The timeout path was described as "on timeout the module computes the value itself and logs inconsistent output". Neither half is right. dt_dev_sync_pixelpipe_hash() returns TRUE after a timeout when the history stack has changed, because a reprocess is already queued; it fails only when it has not. And on failure levels, globaltonemap and hazeremoval do not recompute -- each one's fallback is keyed on the value being uninitialised (NAN, -FLT_MAX, DT_LEVELS_UNINIT), not on the wait having failed, so a timed-out consumer proceeds with whatever is in gui_data. Say that: the handshake is best-effort, and a module has to tolerate a stale value as well as a missing one. Also that a non-positive pixelpipe_synchronization_timeout turns the wait off entirely, with the primitive reporting success unchecked. And "cannot get it on the full pipe" is too absolute: hazeremoval takes the handover only when HQ processing is off, since with it on the full pipe does see the whole image. The claim that the other three modules "say so in a comment" is dropped rather than defended -- three of them do, but it added nothing.
…makes it Self-review of 2628840 against src/develop/preview_data.h. The section credited the whole service with "resize, fill and hash commit happen inside a single critical section". The header attaches that to dt_preview_data_store() only. The two-step form -- dt_preview_data_resize() followed by dt_preview_data_set_hash(), which exists precisely for fills too expensive to hold the lock across -- gives it up, and replaces it with a resize_cb() the service calls while still holding the lock so the module can drop its own validity flag atomically with the resize. That is the same discipline as the refill paragraph above it, so the two now point at each other. Also: "all seven of those" was counting entries in a list that named nine functions, two of which the same sentence had just excluded; dt_preview_data_invalidate() marks data stale rather than reporting on it; and the GTK-thread claim for dt_preview_data_get() now cites where colorequal makes it, in mouse_moved() and scrolled().
Self-review. The opening sentence and the rules list are restatements of the document, and both still described the two-direction version of it: "between the GTK main thread and the pixelpipe threads", listing three topics, and "for every field both sides touch". With a third direction and a framework service now in the body, "both sides" is a count that no longer holds and the topic list is short by two. This is the failure mode the last four rounds kept finding -- a summary keeping the shape of the text it summarised.
Self-review of 93410df. The section named mouse_moved() and scrolled() as the call sites. dt_preview_data_get() appears once in src/iop/colorequal.c, in scrolled(); mouse_moved() calls dt_preview_data_is_fresh(), which is a different entry point. The point being made -- a GTK-thread read while the pipe writes -- holds on the one real call site.
299e37d said "toneequal does the same thing with a helper of its own", carried over from the finding that prompted the section without being re-derived. It does not. toneequal's hash_set_get() (src/iop/toneequal.c) is a locked copy in either direction, and g->ui_preview_hash is both written and read inside the same dt_pipe_is_full() branch: the full pipe memoises its own luminance mask against its own upstream hash. Nothing waits for another pipe. Its preview branch publishes to the GUI through preview_data, which is the other new section's subject, not this one's. Replaced with the distinction that makes the module worth naming at all -- a hash beside data in gui_data is a freshness marker, and only a handover when something waits on it -- since a reader who opens toneequal after this section will otherwise take the two mechanisms for one.
toneequal has two caching branches and only one of them is what the refill rule is about. The full-pipe branch fills g->full_preview_buf, which the source comments as not GUI-accessed and deliberately unlocked; the invalidate-then- recompute-then-commit sequence is on the preview branch, whose buffer the GUI does read. Say which one. Also name the concrete instance of dt_preview_data_resize()'s resize callback while there: toneequal's _toneeq_preview_resized() drops g->luminance_valid and nothing else, under the lock the service is still holding, which is the whole of what that callback is for.
Gemini finding 1, accepted. GUI_Recipes.md Recipe 6 led into its example with "The callback is optional, so a module that has none has nothing to add:" and then showed a module that does implement one. The clause was a restatement of what imageop_gui.md Key Point 7 already says, so it is dropped rather than patched; the lead-in now matches GUI.md's wording for the same rule, which is the point of having fixed all four copies. Gemini finding 2, rejected on the facts -- see the reaction file -- but the sentence it misread is reworded anyway. "The only thing standing between a deleted instance and a NULL dereference" is correct: deleting an instance parks the struct in dev->alliop and frees only the GUI (src/develop/imageop.c, "don't delete the module, a pipe may still need it"), so gui_data is NULL and the struct is live. Gemini's falsification pass substituted darkroom exit for instance delete, which is a different shape this document separates two sections later. Since a careful reader made that substitution, the sentence now carries its own scope instead of relying on the later section for it.
"the tethering histogram" covered both of the view's histogram paths and only one of them belongs on that list. _expose_tethered_mode() (src/views/tethering.c) branches on cam->is_live_viewing: the live-view branch computes the histogram straight from the camera's live-view buffer, with no pixelpipe and no dev of its own -- it touches darktable.develop only to resolve profile info -- while the else branch runs the last captured image through dt_imageio_export_with_flags(), which does build its own dt_develop_t (src/imageio/imageio.c). Only the second is an instance of what this list enumerates. Found by Gemini in the 05-50 round. Its proposed fix was to drop the entry; that would have removed a real case, so the entry is qualified instead.
…them Kofa's call, 2026-08-27: a document that says "module X is broken, here is its code" goes stale the day X is fixed, and it drags the reader through details that belong to one module rather than to the pattern. Four passages named live in-tree defects. Each is now a generic "this would be wrong" snippet, cut down to the lines that carry the lesson. - The call-chain warning named denoiseprofile's unlocked variance readout and overlay's GTK calls reached from process() through a helper. Now one invented helper that does both, plus the part that was the real point and was only implied before: a mutex of your own serialises pipe threads against each other and orders nothing against the GTK main loop, so it does not make a widget call from a pipe thread safe. - The gui_lock lifecycle caveat named the undo/redo history route and the two functions involved. Now stated as what it is -- dt_iop_gui_init() is the wrapper that guarantees the pairing, a path that calls a module's gui_init() directly does not, and at least one route in tree does that today. - The inter-pipe section named colorreconstruction's frozen bilateral grid. Now a four-line snippet, with the part specific to this section spelled out: the hash handshake proves the publisher finished a matching run, not that it will not start another one and free the object while you read it. - The drain rule named exposure's single g_idle_remove_by_data(). Now the wrong version on its own. The reference to src/bauhaus/bauhaus.c stays: that one is cited for doing it right. All five defects are filed under new_source_bugs/ instead, including two not previously written up: the overlay GTK calls and the undo/redo path leaving gui_lock uninitialised. The proxy section keeps exposure by name -- separately decided; its divergence from master is already marked and scheduled by the darktable-org#21974 TODO.
Same call as a97113e, applied to the examples showing correct usage: a verbatim quote drags in one module's field names, helper names and problem domain, and goes stale when that module is refactored. Each is now the pattern with a reference beside it. - The commit_params() example was toneequal's, with its sigma/interpolation fields and a paragraph explaining that one line of it -- update_curve_lut() reading self->params rather than the p it was handed -- must not be copied. Now a generic version that takes p correctly, so the caveat becomes a rule stated once ("pass it p, not self->params") instead of an exception carved out of a quote. The reference to toneequal's commit_params() stays: the GUI/non-GUI split is that module's structure and is not going anywhere. - The two inter-pipe snippets were levels', with auto_levels[] and d->levels[]. Now g->published_value and d->value, which is all the shape needs, with src/iop/levels.c named as the module carrying both halves in one helper. - The invalidate-before-refill rule was prose plus a pointer at toneequal's compute_luminance_mask() branch. Now the six lines it actually is. - Dropped the enumeration of the four modules using dt_dev_sync_pixelpipe_hash() and the naming of which handler in colorequal calls dt_preview_data_get() -- both are lists that go out of date without teaching anything.
…s flaw The bullet named a module and asserted that nothing invalidates its cache when the EXIF fields it reads change -- a live-defect claim that goes stale if the module or the key changes, and that buries the instruction under the accusation. The gap is a property of the key, so state it that way, keep the citation as evidence that modules really do read those fields, and end with what a module author should do about it. The colorout bullet below already had that shape.
45e13fb replaced the toneequal commit_params() quote with a generic example, leaving the proxy section pointing at "the toneequal quote earlier in this document", which no longer exists. The claim it makes is unchanged -- the example still writes self->dev->gui_attached && g -- so only the pointer needed fixing. The inter-pipe section had the same problem in smaller form, calling its own now-generic snippet "the quote above".
…document This list is a restatement of the document, and restatements are where this PR has repeatedly lost a qualifier. Two entries were out of step after the de-naming pass. "GTK+ call directly in process()" implied the indirect case is fine, which is the opposite of what "Which Thread Am I On?" now spends a snippet on. It also carries the point from that snippet: a mutex of your own serialises pipe threads and orders nothing against the GTK main loop. And the list had no entry for the pointer-load-protected-pointee-not trap, though "Short Is Not The Same As Correct" is a whole section about it and the inter-pipe section now refers back to it. Added, with the three remedies named so the entry does not stop at the diagnosis. The recursive-lock entry now names dt_dev_sync_pixelpipe_hash() as a framework helper that takes the lock you hand it, since "a locking helper" otherwise reads as something only your own code can be.
The refill snippet added in 45e13fb shows the writing side only, and a reader who takes it at face value can test the flag under the lock, release, and then read the buffer -- which is the pointer-load-protected-pointee-not trap from the top of the same section, reached by a different route. The flag is only worth having if the reader holds it across the use, or copies out under it. Said explicitly rather than left to the reader to connect.
1. The inter-pipe section said the hash match "is what makes the value the right
one for this render". It is weaker than that, in three ways that all matter to
a module author. _dev_wait_hash() (src/develop/develop.c) snapshots the
publisher's hash under the lock, releases, and only then compares against a
freshly computed dt_dev_hash_plus() -- so nothing ties the payload the
consumer subsequently reads to the hash that matched, and the publisher can
run again in between. The success return also covers a disabled timeout and a
post-timeout history change ("pretend that everything is fine"). And
dt_dev_hash_plus() folds selected piece hashes only; it is not the cache key's
render identity. Stated as evidence rather than proof.
2. "Every one of those seven takes your module's gui_lock internally, so you
neither take it nor have to know it is there" oversold the service.
dt_preview_data_is_fresh() (src/develop/preview_data.c) walks the live
preview_pipe->nodes list inside that lock, and the topology is freed under
pipe->busy_mutex and rebuilt under dev->history_mutex by a pipe worker
(src/develop/pixelpipe_hb.c). gui_lock covers the service's own fields and
stabilises nothing outside them. Scoped, since a reader who believes the
original sentence will call it from anywhere.
3. The refill rule cited toneequal as doing "this", where "this" was the whole
producer-plus-consumer protocol. Its producer half is exact; its GUI readers
test luminance_valid after leaving the critical section and then read
g->pd.buf, width and height raw (src/iop/toneequal.c). Citation scoped to the
producer half, which is what it was there to show.
4. dt_preview_data_get_hash() was grouped with dt_preview_data_is_fresh() as
reporting "whether what is stored still matches the current pipe state". It
compares nothing; it returns the stored hash so the caller can compare
(src/develop/preview_data.c). Separated.
Findings 2 and 3 are also source defects, filed under new_source_bugs/ as
preview-data-is-fresh-walks-live-pipe-nodes.md and
toneequal-readers-ignore-luminance-valid-after-unlock.md. Neither is described
as a defect in the document, per the de-naming rule -- 2 is stated as the limit
of what the lock covers, 3 as the scope of a citation.
Follow-up to 22f2c1a. The publishing snippet's comment said payload and hash go in together "so a consumer can never see a hash its payload does not match", which is exactly the guarantee the paragraph above it had just withdrawn. What one critical section actually buys is that no reader catches the pair half-updated; it says nothing about a consumer that verified a hash, released, and read the payload after the publisher ran again. Comment scoped to atomicity. The fallback bullet then restated "the handshake is best-effort" a third time; it now points at the limits stated once, earlier.
The citation added in 22f2c1a said toneequal "commits hash and flag together", which is what the snippet above it does and not what the module does. src/iop/toneequal.c calls dt_preview_data_set_hash(), which takes and releases gui_lock on its own (src/develop/preview_data.c), and then opens a second critical section to set g->luminance_valid. Two sections, not one -- citing it as the model for the one-section rule undercut the rule. Its version is still correct, and the reason is worth having: it commits the hash before raising the flag, so a reader landing between them sees a fresh hash with the flag down and waits. The reverse order would publish a valid flag over a hash that had not been committed. Said that way the discrepancy teaches something instead of being an error in a citation. Found by Gemini in the 07-00 targeted round -- the only finding of that round on the Gemini side; it passed the other three areas explicitly.
22f2c1a scoped "every one of those seven takes gui_lock internally" to the service's own fields, which was the right correction and still not quite right. dt_preview_data_is_fresh() tests pd->buf in its early return before it takes the lock at all (src/develop/preview_data.c), and dt_preview_data_store() and dt_preview_data_resize() free and replace that field while holding it. So even "for the service's own buffer and hash you neither take it nor have to know it is there" claimed more than the code gives. Dropped the "nor have to know it is there" half, and added the precheck beside the topology walk in the paragraph that already states the limits, rather than starting a second list of caveats. Found by Codex in the 07-00 targeted round -- its only finding; it dropped five candidates and passed all four areas otherwise.
cfd6c4d softened this bullet but kept its example: a module deriving its result from EXIF gets no invalidation when those fields change. Kofa rejected the premise. Exposure bias and highlight preservation are capture data written at import, and the darkroom reads them from dev->image_storage, a copy taken in _dt_dev_load_raw() and refreshed only through dt_dev_reload_image(), which forces a resync anyway (src/develop/develop.c). No cache line survives a change to them, so there was never a hazard to warn about and no reason to hash EXIF. The hazard is on the database-backed side. dt_variables_expand() takes the rating from the live image-cache record and the colour labels, tags and user metadata from their own tables, on every call (src/common/variables.c), none of it from the snapshot. The rating and the labels can be changed from within the darkroom; the tagging panel is there under a preference. Kofa then asked whether an IOP API hook exists for folding a value into the key without persisting it, as colorout does with the export profile. It does not: dt_iop_commit_params() builds piece->hash after the callback returns and assigns it unconditionally (src/develop/imageop.c), and iop_api.h declares nothing else. The remedy paragraph now states that and gives the three routes that remain -- module->params, the fixed pipe fields the framework hashes, and explicit invalidation -- replacing a one-line remedy whose advice ("fold it into params") does not work for a tag list. Reviewed by Gemini and Codex in the 16-01 round, five findings, all accepted: the two metadata signals carry no image id and one of them fires for tag-dictionary edits touching no image, so they are a coarse trigger, not a ready one; rasterfile hashes its own params, not the file it loaded, so it was cited for something it does not do; the rating is a live cache read rather than a query; the user-metadata editor is lighttable and tethering only; and "put it in params" needed to name module->params, since this section tells the author eight lines earlier to work from the callback argument instead. Both reviewers re-derived the capture-side conclusion and the negative claim about the API surface and passed them; Codex drafted a finding against the capture side and dropped it.
kofa73
force-pushed
the
dev-doc/gui-data-sharing-pr
branch
from
August 28, 2026 16:39
231928f to
b9ce6ff
Compare
kofa73
force-pushed
the
dev-doc/gui-data-sharing-pr
branch
from
August 28, 2026 20:10
6ff6436 to
210a556
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
GUI.md section 3 covered only updating the GUI from process(). It now covers both directions, and states the rule that was never written down: a widget callback may write gui_data, but needs the GUI critical section whenever a pipe thread also touches the field.
Adds which callbacks the pipe drives (their static helpers included), the preconditions for touching gui_data from commit_params(), locking rules for dev->proxy accessors, and why a critical section must span the lifetime of the value rather than just the load. IOP_Module_API.md gets a matching note on commit_params() and a fixed anchor.
Draft, needs editing: