dtourolle/DarkRoom
1
0
Fork 0
Code Issues 39 Pull Requests Actions Packages Projects Releases 20 Wiki Activity

Say how spot removal is going to work before writing it

Browse Source 
FR-DEV-8 is the last develop requirement with nothing behind it. The
pipeline it lands on already has most of the parts — the neighbourhood
stage, the mask crate's normalised coordinates, the gradient handles'
canvas drags, the merge-by-id rule — so the spec is mostly about the four
things that are genuinely new, and about the two places where the obvious
implementation is the wrong one: a Poisson solve is sixty dispatches per
spot, and a per-frame readback to find out what colour a sky is would undo
ARCH §6.1.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
dtourolle
2026-08-26 19:43:30 +02:00
co-authored by Claude Opus 5
parent f82c69bc6b
commit 8d8d6491ad
2 changed files with 554 additions and 9 deletions
Download Patch File Download Diff File Expand all files Collapse all files
docs/spot-removal.md
+545
View File
@@ -0,0 +1,545 @@
# Spot removal
**Status:** Draft · 2026-08-26
**Companion to:** [requirements.md](requirements.md) §3.3 FR-DEV-8 · [architecture.md](architecture.md) §5.2
The last develop feature the requirements ask for that nothing in the tree
implements. FR-DEV-8 states the shape — "non-destructive clone and heal spots
stored as parameters in the edit graph (target, radius, feather, source offset,
opacity, mode), with automatic source placement and manual override, plus a
visualise-spots mode" — and this document is how that lands on the pipeline
that exists now.
---
## 1. Why it is worth the work
Sensor dust is unavoidable with interchangeable lenses, and a dust spot is the
most common reason a photographer leaves a RAW editor for a pixel editor
mid-workflow. Every other develop operation in this application can be the best
one in its class and the workflow still breaks at the first frame with a mark on
the sky.
It is also, unusually, a feature whose cost has already been paid twice over.
The neighbourhood stage exists ([`crate::detail`](../core/dr-pipeline/src/detail.rs)),
the convention for storing geometry in normalised source coordinates exists
([`mask.rs`](../core/dr-pipeline/src/mask.rs)), the canvas-drag pattern exists
([`gradient.rs`](../ui/dr-ui/src/gradient.rs)), and the merge-by-id rule exists
([`sidecar.rs`](../core/dr-pipeline/src/sidecar.rs)). What is genuinely new is
small and is named in §3.
## 2. Non-goals
- **Not layer-based pixel editing.** §1.3 of the requirements excludes that and
this does not reopen it. A spot is a handful of numbers in the edit graph; no
pixels are stored, and the original file is never touched.
- **Not content-aware fill.** The source is a patch from the same photograph,
chosen by an offset. Synthesising texture that is not in the frame is a
different problem with a different budget.
- **Not a general clone brush.** A spot is a disc, not a stroke. A dragged
clone brush is expressible on top of this (a stroke *is* a run of discs) and
is deliberately left until the disc is finished and used.
- **Not automatic dust detection.** Finding spots without being asked is a
reasonable later feature and a bad first one: a false positive silently alters
a photograph, which is the failure this application must not have.
## 3. What is new, precisely
Four things, and it is worth being blunt about them because everything else in
this document is assembly of parts that already work:
1. **A detail pass with variable-length data.** Every [`DetailPass`] today
carries a `Vec<f32>` of uniforms fixed by its own structure. A spot list is
neither fixed nor small. §7.
2. **A neighbourhood operation whose reach is not a small kernel.** Every
existing pass declares a halo of a few pixels. A spot reads from wherever its
source is, which may be a third of the frame away. §5.3.
3. **Undo over something that is not a parameter.** [`History`] snapshots a
[`Preset`], which is a map of scalars — so mask edits are already outside
undo, and spots must not be. §10.4.
4. **A canvas mode that *creates* objects.** Crop edits one rect; local selects
a region; gradient drags an existing shape. Nothing yet makes a new thing
where the pointer went down. §10.
## 4. The model
```rust
/// TRACES: FR-DEV-8
pub struct Spot {
/// Stable across devices; see below.
pub id: String,
/// What is being covered, in normalised **source** coordinates.
pub centre: (f32, f32),
/// What covers it, as an offset from `centre` in **frame units**
/// (y spans 0..1, x spans 0..aspect — the mask convention).
pub offset: (f32, f32),
/// Fraction of the source frame's shorter edge.
pub radius: f32,
/// Fraction of `radius` over which the edge falls away. 0 is hard.
pub feather: f32,
/// How much of the patch is laid down. 1.0 is opaque.
pub opacity: f32,
pub mode: SpotMode, // Heal | Clone
pub enabled: bool,
}
```
**Units follow the mask rule, for the mask reason.** A length stored in pixels
is a length that means something different in the preview and in the export
([`RenderScale`]'s whole documentation is this argument). `centre` is normalised
source, so a crop, a zoom, a pan and a rotation move the spot with the
photograph and no arithmetic is needed to keep it there. `radius` is a fraction
of the **source** frame's shorter edge rather than of the rendered region, so
cropping does not resize a spot that was already placed — a dust mark is a fact
about the sensor, not about the composition.
**`offset` is a vector, not a second point.** Dragging the destination moves the
source with it, which is what a photographer expects when they nudge a spot half
a pixel and do not want to re-place the source. Moving the source alone is
editing `offset`.
**The id is derived, not counted.** `MaskStack::next_id` numbers layers, which
is fine for a stack a user names, and wrong here: two devices that each place a
spot offline would both produce `spot3`, and the merge in §11 would treat two
different marks as one. So the id is a short base-36 hash of the centre at
creation, and two devices that place a spot in the same place produce the same
id — which is the correct outcome, because they removed the same piece of dust.
```rust
pub struct SpotSet {
spots: Vec<Spot>, // in creation order; the order matters, see §5.2
}
```
### 4.1 Why it lives beside `ops`, not in it
The [`Operation`] trait takes a `ParamId` and returns an `f32`, and the whole
generic machinery above it — the panel, the sidecar, the presets, the history —
is built on that being true. A spot list is not scalars, and the trait says so
explicitly where it refuses a downcast for film tables.
[`EditGraph`] already holds three things that are not operations for exactly
this reason: `framing`, `masks` and `film`. `spots` is the fourth, and the
argument is the same one `masks` makes — a stack of layers is not a slider, and
folding it into the list would make every consumer that walks `ops` know that
some entries are not really operations.
Bounds, following `mask.rs`'s example of bounding what a sidecar can grow to:
| Constant | Value | Why |
|---|---|---|
| `MAX_SPOTS` | 64 | Beyond a few dozen the answer is to clean the sensor. Refuses rather than dropping, as `MaskStack::push` does. |
| `MAX_SOURCE_DISTANCE` | 0.5 | Frame units. Bounds the halo in §5.3, which is otherwise unbounded. |
| `DEFAULT_RADIUS` | 0.012 | About 25 px on a 24 MP frame's short edge — a dust mark. |
| `DEFAULT_FEATHER` | 0.35 | Fraction of the radius. Soft enough that a heal on a gradient sky has no visible boundary. |
## 5. Where it runs
### 5.1 First in the detail chain
ARCH §5.2 draws spot removal *after* texture and clarity and *before* sharpen
and NR. That diagram is already out of step with the operation set — the
`order:` keys in `core/dr-pipeline/ops/` put noise reduction at 110 and capture
sharpening at 120, ahead of clarity at 130 and texture at 140 — so it needs a
correction anyway, and the correction should put spot removal **first among the
neighbourhood passes**, at a notional order of 105.
The reason is the halo. Sharpening a dust spot before removing it amplifies its
edge, and the amplified edge is wider than the spot: the sharpening kernel has
already smeared a dark ring into pixels that the spot's own disc does not cover,
so the heal leaves a faint circle of over-sharpened background around a patch
that is otherwise perfect. Removing the mark first means every later pass sees a
photograph with no mark in it, which is also the photograph the photographer
thinks they are sharpening.
Since the spot set is not in `ops`, `EditGraph::compose_detail_for` splices its
passes in front of the ops' passes rather than sorting by a declared order. That
is a two-line change and it is stated here so nobody looks for a `spots.yaml`.
### 5.2 Rounds, because sources can read destinations
Every pass reads one texture and writes another. So within a single pass, every
spot reads the *unhealed* image — and a spot whose source overlaps an earlier
spot's destination copies the mark the earlier spot was removing.
The fix is not to run one pass per spot (64 dispatches for a frame that needs
one). It is to group: walking the spots in creation order, a spot joins the
current round unless its source disc intersects the destination disc of a spot
already in that round, in which case it opens a new one. One pass per round, and
the common case — spots scattered over a sky, sources near their own
destinations — is a single round. The grouping is plain CPU code over at most 64
discs and belongs in `SpotSet`, with a test that says an overlapping pair
produces two rounds and a disjoint pair produces one.
### 5.3 The halo, honestly
[`DetailPass::radius`] is "the furthest this pass reads from the pixel it
writes", and it exists so that ARCH §5.3's tile scheduler knows how far to grow
a tile. For a spot pass that is `max(|offset| + radius)` over the pass's spots,
in render pixels — which with `MAX_SOURCE_DISTANCE` at 0.5 can approach half the
frame.
That is a real cost and it should be written down rather than discovered: a
frame with a long-armed spot is close to untileable for that one pass, so the
tiled path will compute it whole-frame. Two things keep it affordable. The pass
is cheap per pixel (§6.4), and it is only the *spot* passes that carry the halo
— the sharpening pass after it still declares its three pixels and still tiles.
The alternative, clamping the source distance to something tile-sized, would
make the tool useless exactly where it is most needed: a mark on a face is
healed from the other cheek, and that is a long way.
## 6. What a spot does to the pixels
Both modes work on the same disc. For a pixel at render coordinate `p` inside a
spot centred at `d` with radius `r`, with the source at `s = d + offset`:
```
w = falloff(|p - d| / r) // 1 at the centre, 0 at the rim
patch = bilinear(source_texture, p - d + s)
c = mix(c, patch + membrane, w * opacity)
```
`falloff` is a smoothstep over the outer `feather` fraction of the radius; a
feather of 0 is a hard disc. `bilinear` is four `tap`s and two lerps, because
the generated preamble offers `textureLoad` only and the offset is fractional in
render space — it becomes a [`Helper`], deduplicated across passes like the
existing luminance helper.
`membrane` is what separates the two modes, and it is zero for `Clone`.
### 6.1 Heal, without a Poisson solve
The classic heal is Poisson blending: copy the *gradients* of the source and
solve for the image whose gradients they are, subject to matching the
destination on the boundary. Solved properly that is an iterative linear system
— tens of Jacobi passes over the disc — and each iteration is a dispatch in this
architecture. Sixty dispatches to remove a dust spot is not a frame budget.
The membrane that solve produces is smooth and interpolates the boundary
difference, and there is a closed form that approximates it to a quality nobody
has ever complained about in a photograph: **mean-value seamless cloning**
(Farbman et al., 2009). Sample the difference between destination and source
around the rim of the disc at `K` points, and interpolate those differences into
the interior with mean-value weights:
```
for k in 0..K:
b_k = tap(rim_k) - tap(rim_k + offset) // boundary difference
membrane = Σ λ_k(p) · b_k // λ from the rim geometry
```
For a circular boundary the mean-value weights reduce to something a shader can
evaluate directly — the interior point's angular position and distance to each
rim sample — so a heal costs `K` pairs of taps and a weighted sum, per pixel
*inside the disc only*. No iteration, no extra pass, no state.
What it buys: a patch that carries the source's texture and the destination's
colour and brightness, which is precisely the failure a plain clone has when a
sky is a gradient — the clone is right in texture and a quarter-stop wrong in
tone, and shows as a disc.
`K = 24` is the starting number. It is a uniform, so it is tunable without a
recompile, and the acceptance test in §12 is a measured one rather than a taste
one.
### 6.2 Why not compute the boundary statistics on the CPU
Because that means reading the rendered image back, and FR-DEV-4 forbids it in
the render path for reasons ARCH §6.1 spends a page on. A per-frame readback to
find out what colour a sky is would reintroduce exactly the stall the whole
architecture exists to avoid. The mean-value form needs no reduction at all,
which is most of why it is the right answer here.
### 6.3 Clone
`membrane = 0`. Kept because heal is wrong on a boundary: a spot straddling a
horizon healed by mean-value blending smears the horizon's contrast into the
disc, and the honest tool then is a straight copy from a matching part of the
frame. This is why FR-DEV-8 asks for both, and it costs one branch in the shader
and one segmented control in the panel.
### 6.4 Cost
Per pixel, per pass: a rejection test per spot in the pass (a squared distance
and a compare), and for the pixels actually inside a disc, `4 + 2K` taps. With
64 spots the rejection cost is the dominant term and it is about 64 × 4 ALU ops
on every pixel of the frame — call it a millisecond at 2 MP on integrated
graphics, which is affordable but not free.
If it proves not to be, the fix is the one `mask.rs` already uses for strokes: a
bounding box per spot and a dispatch sized to it. That needs the detail runner
to dispatch something other than the whole frame, which is a change to its
shape, and it is deliberately not being made until a measurement asks for it.
## 7. Getting a spot list to the GPU
[`DetailPass`] gains one field:
```rust
/// Per-instance data too large or too variable for the uniform block.
pub storage: Option<Vec<[f32; 4]>>,
```
and the generated preamble gains one binding:
```wgsl
@group(0) @binding(3) var<storage, read> instances: array<vec4<f32>>;
```
In `dr-gpu`, both bind group layouts gain a read-only storage entry at binding
3, and a pass that declares no storage binds a shared one-element dummy buffer.
wgpu permits a layout entry the shader does not use, so the two layouts stay two
rather than four, and no existing pass changes at all.
**A spot is two `vec4`s**: `(centre.x, centre.y, radius, feather)` and
`(offset.x, offset.y, opacity, flags)`, all in render pixels except `flags`,
converted on the CPU in `compose_detail_for` where the framing is in scope. This
matters: the shader never sees a normalised coordinate and never has to know
about crop, rotation or zoom — `Framing::output_at` does that map on the way in,
exactly as `gradient.rs` does it for handles. The framing is an affine
similarity, so a disc stays a disc and one radius scales by one factor:
`radius_px = radius × min(source_w, source_h) × scale.ratio()`.
**The source list does not recompile anything.** The WGSL is identical for one
spot and for sixty-four — the count is a uniform and the loop is over the
buffer — so `structure_hash` is unchanged as spots are placed, and placing the
tenth spot re-uploads a 512-byte buffer. This is the same property the fused
pass has for slider movement and it is worth a test that asserts
`cached_pipelines()` does not grow while spots are added.
**Rejected: packing spots into the uniform block.** It would touch no bind group
layout, which is genuinely attractive. It also requires the composer to emit
`vec4` uniform fields (it emits scalars), forces a fixed `MAX_SPOTS`-sized array
and its fixed upload cost into every spot pass, and gives the next operation
that wants a table — a LUT, a curve, a lens grid — nothing to build on. The
storage buffer is a few more lines once and useful again later.
**Invalidation.** The spot set folds into the `detail` key in
`EditGraph::invalidation`, alongside the detail operations. Dragging a spot
therefore re-runs the detail chain and *not* the fused colour pass or the
demosaic, which is exactly the reuse FR-DEV-3d asks for and is the difference
between a spot that follows the finger and one that stutters.
## 8. Automatic source placement
FR-DEV-8 asks for automatic placement with manual override. Two stages, because
the useful half is much cheaper than the good half.
**Stage one — a placed default.** A new spot's source is offset by `2.5 × radius`
in the direction that keeps it furthest inside the frame, biased towards the
frame centre. For dust on a sky, which is the overwhelming majority of spots,
this is right often enough to be worth having, and it is wrong in a way that is
immediately visible and one drag from fixed.
**Stage two — a scored search.** A compute dispatch per new spot scores candidate
offsets on two rings around the destination (say 32 candidates), each scored by
the sum of squared differences over the annulus just outside the destination
disc — the ring is what has to match, since the disc's interior is being
replaced anyway. Penalise candidates whose disc overlaps another spot's
destination, or the frame edge. The winner's offset is read back **once**, when
the spot is created, through `readback.rs` — a few hundred bytes, which is the
size the histogram already moves and completes in well under a frame — and
written into the spot.
Two rules about that readback, both of which are the difference between a
feature and a bug:
- It is **not** in the render loop. `readback.rs` blocks with a deadline, and
that is tolerable exactly once per placement and intolerable per frame. It
happens on the gesture, and the render that follows uses whatever the spot
currently holds.
- The result is **stored**, and the search is never re-run behind the user. A
spot whose source moved on its own when the file was reopened would be an edit
changing itself, and non-destructive editing means the sidecar decides what
the picture is.
If the readback fails, the stage-one default stands. There is no state in which
a spot has no source.
## 9. Visualising spots
FR-DEV-8's "visualise-spots mode" is two different things, and conflating them
is how one of them ends up missing:
**The overlay** — where the spots *are*. Circles for the destination, a fainter
circle for the source, a line between them for the selected spot. Drawn in Slint
over the canvas, alongside the gradient handles and by the same coordinate map,
so it costs the render path nothing and cannot leak into an export.
**The reveal** — where the spots *should be*. Lightroom's "Visualize Spots": a
high-contrast, desaturated view of the frame's high-frequency content, in which
sensor dust on a smooth sky is obvious and in a normal view is nearly invisible.
It is a detail pass appended to the chain:
```
c = abs(c - blur(c)) stretched by a threshold, greyscale, inverted
```
It is a **view**, not an edit. So it is not in the graph and not in the sidecar:
`compose_detail_for` takes a `DetailView` (`Normal` | `RevealSpots`) and the
export path passes `Normal`. A flag on the session would work until the day
somebody exports while the mode is on, and then it would produce a black-and-
white file that looks like corruption. Making the export call site name it is
what stops that from ever being possible.
## 10. Interaction
### 10.1 The mode
A third chip in `ModeStrip`, beside Crop and Local, and a `ViewMode::spots`.
The strip's own documentation already predicted this shape for the brush; the
spot tool is the same shape and arrives first.
### 10.2 Gestures
| Gesture | Effect |
|---|---|
| Tap / click on the photograph | Place a spot at the current radius, source auto-placed (§8), and select it |
| Drag from a spot's centre | Move the destination; the source follows |
| Drag from the source circle | Change the offset |
| Drag *out* from a fresh placement | Set the source directly, without the auto-placement |
| Scroll / pinch on a selected spot | Radius |
| Tap a spot | Select it; the panel scopes to it |
| `Delete` / `Backspace` | Remove the selected spot |
| Alt-click a spot | Remove it without selecting first |
| `Esc` | Leave the mode |
A drag is a displacement from the press, not a snap to the pointer — the rule
`gradient.rs` states and for the same reason: a finger-sized touch target snapped
to the pointer jumps by half a target the instant it is grabbed.
### 10.3 The panel
While a spot is selected, the adjust column shows radius, feather, opacity and
a Heal/Clone control for *that* spot, exactly as selecting a mask layer
re-scopes the column today. With nothing selected it shows the defaults new
spots will be created with, plus the reveal toggle.
### 10.4 Undo
[`History`] snapshots a [`Preset`], which is a parameter map — so today mask
edits are not undoable, and spot placement must not inherit that. `History`
should hold `(Preset, SpotSet)` and restore both.
That is a narrow change with a wide benefit: the same door lets the mask stack
join later, which closes a gap FR-DEV-5 has open right now. The coalescing rule
needs one addition — a drag of one spot's handle is one step, keyed by the spot
id in the same way a slider drag is keyed by its control — and placement,
deletion and mode changes each open a step of their own.
### 10.5 Touch
Every handle is a `Theme.touch-target`, per FR-UI-3. On a phone the destination
and source circles of a small spot overlap at that size, so the source handle is
drawn at a minimum arm length from the centre while the stored offset is
untouched — the trick `gradient.rs` uses with `MIN_ARM`, for the identical
reason: a handle that cannot be grabbed again is a one-way edit.
## 11. Persistence
One line per spot in the version block:
```
[version 8f04c0e2-…]
exposure.exposure = 0.75
spot.3f9k = 0.4213 0.2871 0.0120 0.35 0.0310 -0.0180 1 heal
```
Fields in order: `centre.x centre.y radius feather offset.x offset.y opacity
mode`. A line rather than a block because a spot is eight numbers and sixty-four
blocks would bury the rest of the file; a line *per spot* rather than one line
for the set because the line is the unit of merge and of a readable diff — the
same reasoning `write_strokes` gives for a line per stroke.
Coordinates are written at the same precision they are held at, as strokes are,
so a round trip is exact and two devices do not generate a diff of noise in the
sixth decimal.
**A malformed line costs that spot and not the file.** A truncated line is
dropped with a warning, exactly as `parse_stroke` drops a bad stroke: a spot
that silently lands somewhere the user never put it is worse than a spot that is
missing, because only one of the two is noticeable.
**Merge** follows `merge_masks` precisely: by id, disjoint survives, a spot both
sides edited resolves wholesale to the higher revision. Half of one device's
offset with the other's radius is a repair neither photographer made. Deletion
propagates through the base comparison exactly as a layer's does.
**Presets do not carry spots** in the first version — a preset is a look, and a
look does not include where the dust was. But dust is in the *same place on
every frame from that body*, which makes "copy spot removal to the selection"
genuinely valuable, and it is a stage of its own (§12, S7) rather than a
surprise inside the existing paste.
## 12. Stages
Each stage is shippable and each has something to look at. Test names are the
files they belong in.
**S1 — The model.** `spot.rs` in `dr-pipeline`: `Spot`, `SpotMode`, `SpotSet`,
the bounds, id derivation, round grouping (§5.2). No GPU, no UI.
*Tests:* `core/dr-pipeline/tests/spots.rs` — id stability across two identical
placements, `MAX_SPOTS` refuses rather than drops, overlapping sources produce
two rounds, disjoint produce one.
**S2 — Persistence.** Sidecar write, parse, round trip, merge.
*Acceptance:* a hand-written sidecar with three spots survives a load/save round
trip byte-identically, and two devices that each add a spot offline end with
both.
**S3 — The storage binding.** `DetailPass::storage`, the preamble's binding 3,
the dummy buffer, the two layouts.
*Acceptance:* every existing detail test still passes untouched, and a synthetic
pass reading the buffer gets what was uploaded.
**S4 — Clone.** The disc, the feather, the bilinear helper, the pass grouping,
the halo declaration, spliced first into the chain.
*Acceptance:* `core/dr-gpu/tests/spot_removal.rs` — a synthetic frame with a
black disc on a flat grey field is clean to within a tolerance after one clone
spot; the same edit at a one-quarter proxy and at full size land the disc in the
same *normalised* place; adding spots does not grow `cached_pipelines()`.
**S5 — Heal.** The mean-value membrane, the mode switch.
*Acceptance:* a dark spot on a linear grey **gradient** — the case clone fails —
is clean to within a tolerance, and the residual at the disc boundary is below
the residual a clone leaves by an order of magnitude. This is the measurement
that decides `K`.
**S6 — The tool.** `ViewMode::spots`, the chip, placement, handles, selection,
the panel scope, deletion, history carrying the spot set, the reveal view, the
stage-one source default.
*Acceptance:* a dust mark on a real frame is gone in one click, the edit survives
a restart, and undo takes it back.
**S7 — The rest of FR-DEV-8.** The scored source search (§8 stage two), and
copying a spot set across a selection.
S1–S6 is the requirement met in the sense a photographer would recognise; S7 is
the sentence in FR-DEV-8 about automatic placement met in the sense the document
means it.
## 13. Documents to amend
- **requirements.md** — FR-DEV-8 has no *Acceptance:* line; every other
requirement of its weight does. Proposed: *"a dust mark on a smooth sky is
removed in one click with no visible boundary at 1:1, the spot survives a
crop, a rotation and an export at another size, and the exported file matches
the preview."*
- **architecture.md §5.2** — the stage list is out of step with the `order:`
keys in `ops/` and does not show spot removal first among the neighbourhood
passes. §5.1 above is the correction.
- **traceability.md** — regenerated, as ever, rather than edited. FR-DEV-8's row
currently points only at two comments that mention it.
## 14. Open questions
1. **`K = 24`?** Decided by S5's measurement, not here.
2. **Does the reveal view belong to spot mode only,** or is it a view mode of
its own that a photographer can turn on while doing something else? It is
cheap to allow both; the risk is a mode nobody remembers turning on.
3. **Should a spot be clamped inside the crop?** A spot outside the current crop
costs nothing to render and is invisible, and re-cropping should bring it
back rather than find it deleted. Leaning strongly towards no clamp.
4. **One radius, or an ellipse?** Lightroom's spot tool is circular and its
users cope. An ellipse doubles the handle count for a case a second spot
already covers.