dtourolleandClaude Opus 5
2e6825ded0
Three places, because the finding has three audiences. The display spec's §1 table said FR-DSP-3 was unmeasured and FR-DSP-5 untagged. Both are now false, and §2's decision rule has fired. The body of §2 is left as written with the verdict quoted above it: a plan overtaken by its own evidence reads better in order than quietly edited into agreement with the outcome. TD-4 is the stage that misses the budget. Clarity's kernel is a fraction of the frame, so it reaches a 52-pixel radius at 4K and costs 34 ms — seven times the entire fused chain, for one slider. It is debt rather than a bug because the detail stage cannot yet write a target smaller than it reads, which `local_contrast`'s own documentation has said since it was written. The entry says plainly that tiles are the wrong tool for it, since that is exactly the conclusion a reader arriving from ARCH §5.3 would otherwise draw. TD-5 is the one nobody was looking for: composing the fused shader costs 2.8–5.2 ms of CPU per frame on a full chain, on the UI thread, which at 1920x1200 is more than the dispatch it precedes. The source depends only on the graph's structure — what `structure_hash` already identifies and what does not move during a drag — so the fix is the cache `AdjustPass` already keeps for compiled pipelines, one level up. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
248 lines
12 KiB
Markdown
248 lines
12 KiB
Markdown
# DarkRoom — Technical debt
|
||
|
||
**Status:** Living document · first written 2026-08-26
|
||
**Companion to:** [architecture.md](architecture.md)
|
||
|
||
Deliberate compromises: things the code does knowing they are wrong, because the alternative was
|
||
worse at the time. Each entry says what the debt is, what it cost to take on, what it would take to
|
||
pay off, and how you would know it had been paid.
|
||
|
||
Not a bug list. A bug is something nobody chose. Everything here was chosen, and the point of
|
||
writing it down is that the reasoning outlives whoever chose it — so the next person can tell a
|
||
constraint from an accident, and does not "fix" something load-bearing or preserve something that
|
||
has quietly stopped being necessary.
|
||
|
||
---
|
||
|
||
## TD-1 — The Android develop view reads pixels back through the CPU
|
||
|
||
**Breaks:** [architecture.md §12 / 6.1](architecture.md) — GPU results never round-trip through the
|
||
CPU — and AC-8, on Android only. Desktop is unaffected and keeps the zero-copy path.
|
||
|
||
### What it does
|
||
|
||
`DevelopSession::render` on Android runs the compute passes on the GPU as usual, then calls
|
||
`AdjustPass::export_pixels` and hands the frame to Slint as a `SharedPixelBuffer`. That is exactly
|
||
the GPU→CPU→GPU transfer §6.1 exists to forbid, and it is on the frame path.
|
||
|
||
### Why
|
||
|
||
Zero-copy needs Slint to draw with wgpu. On Android that means wgpu's Vulkan swapchain, which
|
||
hardcodes `preTransform = VK_SURFACE_TRANSFORM_IDENTITY_BIT_KHR`
|
||
([gfx-rs/wgpu#3345](https://github.com/gfx-rs/wgpu/issues/3345)) — wgpu-hal says so in a comment
|
||
beside the line.
|
||
|
||
On a tablet whose panel is mounted landscape, a portrait window then hands Android an unrotated
|
||
buffer, every present returns `VK_SUBOPTIMAL_KHR`, and frames arrive torn. Measured on the device,
|
||
same build, only the tablet rotated:
|
||
|
||
| orientation | `bufferTransform` | composition | result |
|
||
|---|---|---|---|
|
||
| landscape | `ROT_180` | `DEVICE (2)` | clean |
|
||
| portrait | `ROT_270` | `CLIENT (1)` | torn |
|
||
|
||
Setting `preTransform` is not a fix available to us: the field is a *promise* that the content is
|
||
already rotated, so honouring it needs the renderer to rotate what it draws, which wgpu cannot do
|
||
on Skia's behalf.
|
||
|
||
So the choice was never fast-develop against slow-develop. It was a develop view that costs a
|
||
readback against a grid that tears in the orientation a tablet is mostly held in.
|
||
|
||
### What it costs
|
||
|
||
Less than §6.1's headline numbers, because `render` fits the pass to the canvas before it runs — the
|
||
readback is at viewport resolution, not sensor resolution. The 7.43 ms at 4K in §12 is the ceiling,
|
||
not the bill. **It has not been measured on the device**, which is the first thing to do if the
|
||
develop view feels heavy on the tablet; do not assume this is the cause without a number.
|
||
|
||
### Paying it off
|
||
|
||
Any one of these removes it:
|
||
|
||
- wgpu implements pre-rotation (#3345), and Android goes back on `unstable-wgpu-29`.
|
||
- Slint's Skia Vulkan surface handles `preTransform` and Android uses that instead of OpenGL.
|
||
- Skia over OpenGL grows a way to sample an external texture that wgpu can write.
|
||
|
||
**Done when:** `ui/dr-ui/Cargo.toml` no longer scopes `renderer-femtovg-wgpu` and
|
||
`unstable-wgpu-29` to non-Android, the `#[cfg(target_os = "android")]` arm of
|
||
`DevelopSession::render` is gone, and the tablet is clean in portrait.
|
||
|
||
---
|
||
|
||
## TD-2 — Thumbnails are fetched one at a time
|
||
|
||
**Where:** `library::spawn_thumbnails` — the `for req in to_fetch` loop.
|
||
|
||
### What it does
|
||
|
||
The interactive thumbnail batch fetches serially: one image at a time, and two HTTP round trips
|
||
each (a header read, then the preview's byte range). A window of a few hundred cells is that many
|
||
sequential round trips against the server.
|
||
|
||
### Why it is debt rather than a bug
|
||
|
||
It is correct, and it was fast enough when a window was one screenful. It is the *ordering* that
|
||
kept it survivable: since `fetch_rank`, on-screen cells are requested first, so the cells a person
|
||
is looking at arrive first even though the queue as a whole is slow.
|
||
|
||
Portrait makes it worse by construction — a narrow window means smaller cells, more rows, and two
|
||
to three times as many cells on screen at once, all of them ahead of the ones below in a queue that
|
||
never runs more than one request.
|
||
|
||
### Paying it off
|
||
|
||
`spawn_thumbnail_sweep` already has the pattern: `SWEEP_LANES` disjoint lanes over a chunk, joined,
|
||
with the store written on the one thread that owns it. Striping a *priority-ordered* chunk across
|
||
lanes keeps `fetch_rank`'s ordering while running several requests at once.
|
||
|
||
Not done yet because it multiplies concurrent requests against the user's Nextcloud during a
|
||
scroll, and that is a behaviour change worth deciding on deliberately rather than inheriting from a
|
||
performance fix.
|
||
|
||
**Done when:** the interactive batch runs on more than one lane, priority order is preserved
|
||
across the lanes, and a slow server still cannot stall the visible cells behind offscreen ones.
|
||
|
||
---
|
||
|
||
## TD-3 — The thumbnail drain applies an unbounded batch on the UI thread
|
||
|
||
**Where:** `library_ui::drain_thumbnails` — the `loop` inside the timer callback.
|
||
|
||
### What it does
|
||
|
||
Every message queued when the timer fires is applied in that one callback, with no ceiling. On a
|
||
library whose thumbnails are already in the store, the worker delivers a whole window at once, so a
|
||
single callback can do hundreds of `to_slint_image` calls back to back — each an allocation and a
|
||
full RGBA copy — while the grid is mid-flick.
|
||
|
||
The copy cannot move off the UI thread: `slint::SharedPixelBuffer` is not `Send`, so decoded bytes
|
||
can only become an `Image` on the thread that draws. Only the *amount done per wake* is ours to
|
||
choose, and right now it is "all of it".
|
||
|
||
### Cost
|
||
|
||
Measured with a temporary probe, **debug build**, so treat the shape rather than the size:
|
||
|
||
| class | per thumbnail | × a 280-cell window |
|
||
|---|---|---|
|
||
| grid, 256 px | 1.93 ms | 539 ms |
|
||
| large, 512 px | 7.78 ms | 2.18 s |
|
||
|
||
A release measurement was started and never completed — do not quote these as release figures.
|
||
|
||
### Paying it off
|
||
|
||
A time budget per wake and a shorter interval: apply for a few milliseconds, return without
|
||
stopping the timer, and finish on the next tick. A batch then lands in frame-sized slices rather
|
||
than one lump between two frames. Draft written and discarded during the investigation; it is a
|
||
small change.
|
||
|
||
**Done when:** one wake of the drain cannot exceed a frame, and a fully-cached window still fills
|
||
in well under a second.
|
||
|
||
---
|
||
|
||
## TD-4 — The local-contrast base is computed at full render resolution
|
||
|
||
**Where:** `dr_pipeline::ops::local_contrast::LocalContrast::passes` — the `base` and `combine`
|
||
passes, and the stage that dispatches them, `dr_pipeline::detail`.
|
||
|
||
Breaks **FR-DSP-3** at large viewports. Measured, and the numbers are in
|
||
[frame-budget.md](frame-budget.md) §M3.
|
||
|
||
### What it does
|
||
|
||
Clarity's Gaussian σ is 1.2% of the frame's shorter edge, truncated at 2σ, so its kernel radius is
|
||
a property of the *viewport*: 29 px at 1920 × 1200, 38 px at 2560 × 1600, **52 px at 4K**. The two
|
||
separable passes therefore run 105 taps each over 8.3 M pixels at 4K, which is 1.7 billion texture
|
||
reads for one control.
|
||
|
||
| viewport | radius | clarity alone, p99 |
|
||
|---|---:|---:|
|
||
| 1920 × 1200 | 29 | 5.99 ms |
|
||
| 2560 × 1600 | 38 | 12.44 ms |
|
||
| 3840 × 2160 | 52 | **33.89 ms** |
|
||
|
||
RTX 3050 laptop, `examples/frame_budget`, fused dispatch reused so this is the convolutions alone.
|
||
Clarity is 97% of the cost of all four neighbourhood operations together at every size.
|
||
|
||
For scale: the entire fused chain — every point operation active, film stock included — costs
|
||
4.5 ms at the same 4K viewport. **A single slider is seven times the rest of the pipeline.**
|
||
|
||
### Why
|
||
|
||
Because the stage cannot do otherwise yet. `dr_pipeline::detail` dispatches every pass at the
|
||
render size; there is no way to express "read this target and write a smaller one". The module's own
|
||
documentation has said so since it was written:
|
||
|
||
> The right optimisation is a base computed at reduced resolution, which needs a detail stage that
|
||
> can write a smaller target than it reads; that is a change to `crate::detail`, not to this file.
|
||
|
||
It was the right call to ship the correct answer slowly rather than a fast approximation nobody had
|
||
checked — the halo behaviour is the hard part of this operation and it is tested.
|
||
|
||
### Not a tiling problem
|
||
|
||
Worth saying because ARCH §5.3 offers a tile cache and this is the stage that looks like it wants
|
||
one. It does not: a tiled convolution reads a halo per tile, so at a 52-pixel radius, 256-pixel
|
||
tiles would read (256 + 104)² instead of 256² — very nearly **twice** the taps.
|
||
[display-and-extension.md](display-and-extension.md) §2's decision rule was resolved on this
|
||
evidence; see [frame-budget.md](frame-budget.md).
|
||
|
||
### Paying it off
|
||
|
||
A detail pass that declares an output scale, so the base can be computed at a quarter resolution and
|
||
sampled back up in `combine`. A quarter-resolution base is 1/16 the pixels at 1/4 the radius —
|
||
about **1/64 of the work** — and is visually identical, because a base at σ = 26 px holds no content
|
||
above the quarter-resolution Nyquist to lose. Texture's σ is a decade finer and must stay at full
|
||
resolution; the scale therefore belongs on the `DetailPass`, not on the stage.
|
||
|
||
**Done when:** clarity at 100% is inside the frame budget at 3840 × 2160, the halo tests in
|
||
`tests/local_contrast.rs` still pass unchanged, and `examples/frame_budget`'s M3 table in
|
||
[frame-budget.md](frame-budget.md) has been rerun and committed.
|
||
|
||
---
|
||
|
||
## TD-5 — The fused shader is reassembled from strings on every frame
|
||
|
||
**Where:** `dr_pipeline::operation::compose_full`, called from `DevelopSession::render`.
|
||
|
||
### What it does
|
||
|
||
`EditGraph::compose` walks the active operations and formats a WGSL source string, per frame, on
|
||
the UI thread. On a full chain that is **2.8–5.2 ms** — at 1920 × 1200 it is larger than the entire
|
||
fused dispatch it precedes, and on the chains that also carry a detail stage it is a third of what
|
||
is left of the 16 ms budget after the GPU has taken its share. It does not vary with resolution,
|
||
because it is not pixel work.
|
||
|
||
### Why
|
||
|
||
Because it was free until the chain got long. Composition was written when an edit was two or three
|
||
operations, and the cost is roughly linear in generated source: the colour mixer emits twelve hue bands
|
||
and the tone curve emits a spline evaluator, so a full chain is a large string built from scratch
|
||
sixty times a second.
|
||
|
||
### Paying it off
|
||
|
||
The generated source depends only on the *structure* of the graph — which is precisely what
|
||
`ComposedShader::structure_hash` already identifies, and precisely what does not change while a
|
||
slider is being dragged. `AdjustPass` relies on that already: it caches compiled pipelines against
|
||
that hash and does not recompile during a drag. Caching the source string against the same hash and
|
||
rebuilding only the uniforms — a handful of floats per operation — takes this to approximately
|
||
nothing on the path that needs it most.
|
||
|
||
The care needed is in what the hash covers. It deliberately excludes parameter *magnitudes*, so a
|
||
cache keyed on it is sound for the source and would be wrong for anything else in `ComposedShader`.
|
||
|
||
**Done when:** the `shader` column of [frame-budget.md](frame-budget.md)'s M1 table is under a
|
||
millisecond for the `point` and `all` chains, and the codegen tests still pass byte for byte.
|
||
|
||
---
|
||
|
||
## Related, and deliberately not here
|
||
|
||
The window-move rule, the grid's ordering index and the whole-library readout cache were *fixed*
|
||
rather than deferred — see the commits around `6d6ef8d`. They are mentioned only so that a reader
|
||
looking for "why was the grid slow" finds the answer in the code and its comments rather than
|
||
assuming it is still outstanding.
|