Develop T4b (part 4): each pipe plans from the viewport it latched - #1144
Conversation
… live one The darkroom worker latches dt_dev_roi_request_get() once per loop iteration and copies it into every pipe, before any resync or planning. Everything downstream on the pipeline thread then works from that one snapshot for the whole frame: _update_darkroom_roi() plans from it, dt_dev_pixelpipe_has_preview_output() compares against it, and iop/finalscale.c's commit_params() decides piece->enabled from it. Reading the record live at each consumer was the remaining half of the problem C3 set up the record to solve. The record made a single read coherent; it could not stop a frame being planned from two DIFFERENT reads, because the GUI thread republishes whenever the user zooms, pans or resizes -- which is precisely while a frame is being computed. The planner alone made four separate reads (natural_scale, scaling, processed size, box, centre), so a zoom landing between the first and the last produced a frame whose geometry never existed at any instant, internally consistent and correctly hashed. A pipe nobody latches -- export, thumbnail, snapshot -- keeps the neutral seed dt_dev_pixelpipe_init_cached() gives it, and the seed is deliberately NOT zeroed: scaling 1 and natural_scale -1 are what those pipes read off a headless dev before this record existed, and finalscale multiplies exactly those two. Zeros there would flip piece->enabled on every export. The generation is still consumed by nobody; folding it into the FULL pipe hash is the next commit, and the one with real risk. Verified: Release, Debug (-Werror), nofeatures build; cycles 0, layering 187; export A/B against the pre-tranche baseline 0 differing pixels on both pages -- which for this commit only proves the neutral seed is right, since export pipes never latch. Worth exercising in the GUI: zooming and panning DURING a slow recompute, which is the case the latch exists for.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| const dt_dev_roi_request_t request = pipe->roi_request; | ||
| if(IS_NULL_PTR(dev) || IS_NULL_PTR(pipe) || !dev->gui_attached || !request.valid) return FALSE; |
There was a problem hiding this comment.
Bug: The function dt_dev_pixelpipe_has_preview_output accesses pipe->roi_request before checking if pipe is NULL, leading to a crash during headless exports.
Severity: HIGH
Suggested Fix
Move the NULL check for the pipe parameter in dt_dev_pixelpipe_has_preview_output to be the first operation in the function, before pipe->roi_request is accessed.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: src/develop/develop.c#L407-L408
Potential issue: The function `dt_dev_pixelpipe_has_preview_output` dereferences the
`pipe` parameter to access `pipe->roi_request` before checking if `pipe` is `NULL`.
During headless operations like exports, `dev->preview_pipe` is not initialized and
remains `NULL`. When the `denoiseprofile` module calls this function with the `NULL`
`preview_pipe`, the application crashes due to the NULL pointer dereference.
Did we get this right? 👍 / 👎 to inform future reviews.
| || (abs(width - dt_dev_roi_request_preview_height(dev)) <= tol && abs(height - dt_dev_roi_request_preview_width(dev)) <= tol); | ||
| = (abs(width - request.preview_width) <= tol && abs(height - request.preview_height) <= tol) | ||
| || (abs(width - request.preview_height) <= tol && abs(height - request.preview_width) <= tol); | ||
| if(!dims_match) return FALSE; |
There was a problem hiding this comment.
Bug: A data race on pipe->roi_request occurs due to unsynchronized access between the GUI thread (read) and a worker thread (write), which was previously seqlock-protected.
Severity: MEDIUM
Suggested Fix
Reintroduce a synchronization mechanism, such as the previous seqlock via dt_dev_roi_request_get(), to protect reads and writes to pipe->roi_request from different threads. Avoid direct, unsynchronized access to this shared data.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: src/develop/develop.c#L448
Potential issue: A data race exists on the `pipe->roi_request` field. The GUI thread
reads this field via `_update_darkroom_roi` without any synchronization, while a worker
thread concurrently writes to it. The previous implementation used a seqlock via
`dt_dev_roi_request_get()` to protect against this, but the new code accesses the field
directly. This concurrent, unsynchronized access is undefined behavior and could lead to
visual glitches.
Did we get this right? 👍 / 👎 to inform future reviews.
The tranche plan ended with a step folding dt_dev_roi_request_t.generation into the FULL pipe's per-piece hash, so a cacheline planned from generation N could not satisfy a request planned from N+1. That step is not being taken, and the reasoning belongs next to the code rather than in a plan document nobody reads next to it. The guarantee already exists, by content instead of by version: piece->roi_in and piece->roi_out are folded two lines below, and every field of the record reaches them in _update_darkroom_roi(). Zoom and the fit scale become roi.scale as their product, the centre becomes roi.x/roi.y, the box and the processed size become roi.width/height. finalscale's piece->enabled is the one consumer that reads the record rather than the ROI, and it is a function of that same product -- and a disabled piece is skipped before the hash is computed, so a toggle changes the chain regardless. Folding the generation on top would add no discriminating power and could only over-invalidate: a republication advancing the generation while producing identical ROIs -- a sub-pixel pan rounding to the same roi.x, a box change the fminf clamp absorbs -- would rekey the entire FULL-pipe cache chain for pixels that are bit-identical. That is a silent performance regression no export A/B and no static gate can see, which is exactly what made this the risky step; the resolution is that it should not be taken at all. The contrast with mask_preview_settings_revision, hashed thirty lines below, is the useful part for a future reader: that one IS a revision fold, precisely because the state it stands for reaches no ROI. Comment only, no code change.
…everything else Two defects in C4, both found in review, neither reachable by the export A/B. FIRST, self-inflicted: dt_dev_pixelpipe_has_preview_output() read pipe->roi_request BEFORE its own IS_NULL_PTR(pipe) guard, because C4 hoisted the read to the top of the function. dev->preview_pipe is allocated only for a gui_attached dev, so any caller passing it on a headless dev dereferences NULL. The guard now comes first, as it did before C4 touched it. The path the report cited -- iop/denoiseprofile.c:401 -- turns out to be inside `#ifdef DEBUG_SCALES', dead in a normal build, and it names an undeclared `self', so it would not compile if that macro were defined. The reported mechanism was right and the cited caller is not live; the ordering is restored regardless, because a function's NULL guard exists to be the first thing it does. SECOND, and live: the latch was a plain struct assignment written by the darkroom worker and read by the GUI thread. dt_dev_pipelines_share_preview_output() reads it from the darkroom expose path (views/darkroom.c:859) through _update_darkroom_roi(), and every dt_dev_pixelpipe_has_preview_output() call from an IOP GUI callback does the same. A 56-byte struct copied across threads without synchronisation is a torn read -- the exact failure this tranche exists to remove, reintroduced one level down at the moment the record reached the pipe. The pipe now holds a dt_dev_roi_request_store_t and publishes through dt_dev_roi_request_latch(), read back through dt_dev_roi_request_of_pipe(): the same seqlock as the dev-level record, the geometry record and the viewport. A reader on any thread gets either the previous latch or the new one, never half of each, and the pipeline thread keeps seeing one frozen record for a whole run because only the worker publishes and only once per iteration. Verified: Release, Debug (-Werror), nofeatures build; cycles 0, layering 187; export A/B 0 differing pixels -- which, to be explicit, would not have caught either of these: one is behind a dead #ifdef and the other is a GUI-thread race.
|
Both findings fixed in The NULL-deref (HIGH) is real and was my regression: C4 hoisted the The cited caller, however, is not live: The data race (MEDIUM) is live and the more serious of the two. The pipe now holds a Neither defect was reachable by the export A/B (one behind a dead |
|
1 participant
Fourth step of the
dev->roidivision, on top of #1140, #1141 and #1143.The darkroom worker now latches
dt_dev_roi_request_get()once per loop iteration and copies it into every pipe, before any resync or planning. Everything downstream on the pipeline thread works from that one snapshot for the whole frame:_update_darkroom_roi()plans from it,dt_dev_pixelpipe_has_preview_output()compares against it, andfinalscale'scommit_params()decidespiece->enabledfrom it.Why the record alone wasn't enough
C3 made a single read coherent. It could not stop a frame being planned from two different reads — and the GUI thread republishes exactly when the user zooms, pans or resizes, which is precisely while a frame is being computed.
The planner alone made four separate reads:
natural_scale, thenscaling, then the processed size, then the box and the centre. A zoom landing between the first and the last produced a frame whose geometry never existed at any instant — internally consistent, correctly hashed, and therefore undetectable downstream. That is the same class as the tornx/ypair #1141 fixed, one level up: not a torn field but a torn sequence of reads.The neutral seed is load-bearing
A pipe nobody latches — export, thumbnail, snapshot — keeps what
dt_dev_pixelpipe_init_cached()seeds, and that seed is deliberately not zeroed:scaling = 1andnatural_scale = -1are what those pipes read off a headless dev before this record existed, andfinalscalemultiplies exactly those two to decide whether it enables itself. Zeros there would flippiece->enabledon every export.Verification
Release, Debug (
-Werror), nofeatures build.cycles 0,layering_violations 187, all gates pass. Export A/B against the pre-tranche baseline: 0 differing pixels on both pages — though for this commit that only proves the neutral seed is right, since export pipes never latch.The behaviour this commit exists for is not reachable from a headless export at all. Worth exercising in the GUI: zooming and panning during a slow recompute (a large raw, or a heavy module like diffuse), which is the case the latch is for.
Next
C5 folds the generation into the FULL pipe hash. That is the one with real risk in this tranche: a wrong change-gate silently invalidates the pipeline cache chain on every history commit, and neither the export A/B nor any static gate can see it, because export pipes hash zero there. I'll measure how often the generation actually advances during an editing session rather than relying on the usual checks.
🤖 Generated with Claude Code