diff --git a/docs/display-and-extension.md b/docs/display-and-extension.md index d9056aa..8623559 100644 --- a/docs/display-and-extension.md +++ b/docs/display-and-extension.md @@ -20,10 +20,10 @@ and the reason is that some of the work is done and untagged. | Requirement | Reality | |---|---| | FR-DSP-1 proxy rendering | **Done.** The develop view renders at viewport resolution, not source. | -| FR-DSP-2 tiled computation | **Absent.** The fused pass renders the whole viewport in one dispatch. | -| FR-DSP-3 interactive latency | **Unmeasured.** No frame budget is asserted anywhere. | -| FR-DSP-4 progressive refinement | **Absent.** Every render is full quality. | -| FR-DSP-5 zoom and pan | **Substantially done, untagged.** `Framing::view` shrinks the sampled region while the render target keeps its size, so zooming *raises* the resolution the pipeline works at. That is FR-DSP-5's requirement, arrived at without tiles. | +| FR-DSP-2 tiled computation | **Absent, and §2 now says it should stay that way.** Measured: the fused pass is inside the budget everywhere. See [frame-budget.md](frame-budget.md). | +| FR-DSP-3 interactive latency | **Measured and asserted** for the fused path — `core/dr-gpu/tests/frame_budget.rs`. Missed by one operation, clarity, for the reason recorded as TD-4. | +| FR-DSP-4 progressive refinement | **Absent**, and §4's condition did not fire. Every render is full quality and can afford to be. | +| FR-DSP-5 zoom and pan | **Done and tagged**, against tests that fail if the behaviour is removed — `core/dr-gpu/tests/zoom_resolution.rs`. `Framing::view` shrinks the sampled region while the render target keeps its size, so zooming *raises* the resolution the pipeline works at. That is FR-DSP-5's requirement, arrived at without tiles. | | FR-DSP-6 colour management | **Done.** Output space is a parameter of composition. | | FR-DSP-7 histogram and clipping | **Done**, GPU-side, no per-frame readback. | | FR-DSP-8 per-display colour | **Absent.** One transform, not per-display. | @@ -32,12 +32,25 @@ and the reason is that some of the work is done and untagged. Two of the five uncovered display requirements are therefore *measurement and tagging*, not construction. That is worth knowing before anyone plans a quarter around them. +**Both have since been done.** [frame-budget.md](frame-budget.md) holds the measurements §2 asks +for and the reading of its decision rule; the table above is updated to match. The rest of this +document is left as it was written, because a plan that has been overtaken by its own evidence is +more useful read in order than quietly edited into agreement. + --- ## 2. Measure before building tiles **FR-DSP-2 is the one requirement in this document that may not be worth satisfying as written.** +> **Resolved.** M1–M3 were run; the numbers and the verdict are in +> [frame-budget.md](frame-budget.md). The rule below fired for *rewrite*: every point-operation +> chain is inside 16 ms at the 99th percentile at every viewport size, fit and at 1:1, the widest +> being 4.5 ms of GPU at 4K. The measurement did find a stage that misses the budget — clarity's +> 52-pixel kernel, 34 ms at 4K — and tiling makes that stage *worse*, since a tiled convolution +> reads a halo per tile. It is recorded as TD-4 with the fix its own module already names. + + The requirement predates the fused-shader design. It assumes the pipeline is a chain of passes over a large buffer, where recomputing everything on each frame would be ruinous and tiles are the way out. What was built instead composes every active operation into **one dispatch over a diff --git a/docs/technical-debt.md b/docs/technical-debt.md index 69c76e5..0c6dda8 100644 --- a/docs/technical-debt.md +++ b/docs/technical-debt.md @@ -142,6 +142,103 @@ 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*