diff --git a/docs/architecture.md b/docs/architecture.md index 67e10ae..78a8a15 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -10,8 +10,10 @@ and records the decisions and constraints behind the design. ## 1. Overview -DarkRoom is a Rust application with a Slint interface, rendering through wgpu to Vulkan on both -Linux and Android. The design is organised around four ideas, each of which the rest of this +DarkRoom is a Rust application with a Slint interface. On Linux it renders through wgpu to Vulkan. +On Android it renders through Skia to OpenGL — not by preference but because wgpu's Vulkan +swapchain cannot pre-rotate, which tears a portrait window on a landscape-mounted panel +([technical-debt.md TD-1](technical-debt.md)). The compute passes are wgpu on both. The design is organised around four ideas, each of which the rest of this document elaborates: 1. **Pixels stay on the GPU.** From decode to display, image data never round-trips through the @@ -980,6 +982,13 @@ At 4K the shader finishes in 0.28 ms and then 7.15 ms is spent moving pixels thr 26× overhead that scales with area, which is why an uncapped window resize falls off a cliff. The constraint is not a stylistic preference; it is the dominant cost in the frame. +**One exception, on Android only, and it is debt rather than a revision.** The develop view there +reads the frame back rather than handing over a texture, because zero-copy requires Slint to draw +with wgpu and wgpu's Android swapchain tears a portrait window. The reasoning, the measurements +that forced it and what would remove it are in [technical-debt.md TD-1](technical-debt.md). The +constraint above still governs every other path, including the desktop develop view and the export +pipeline, and the Android exception is expected to be temporary. + ### 6.2 Tiling from day one Mobile GPUs have far less memory. Retrofitting tiling into a whole-image pipeline is a rewrite. diff --git a/docs/technical-debt.md b/docs/technical-debt.md new file mode 100644 index 0000000..69c76e5 --- /dev/null +++ b/docs/technical-debt.md @@ -0,0 +1,150 @@ +# 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. + +--- + +## 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.