Draft: dev-doc: document sharing gui_data between GUI and pipe threads - #21912
Draft
kofa73 wants to merge 12 commits into
Draft
Draft: dev-doc: document sharing gui_data between GUI and pipe threads#21912kofa73 wants to merge 12 commits into
kofa73 wants to merge 12 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
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.
kofa73
force-pushed
the
dev-doc/gui-data-sharing-pr
branch
from
August 26, 2026 17:25
0578d2f to
fce1611
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: