dtourolleandClaude Opus 5
336a1fd296
Three compromises were taken deliberately over the last few days and none of
them was written anywhere a future reader would look. So `docs/technical-debt.md`,
and two corrections to the architecture document that the Android change made
untrue the moment it landed.
**TD-1, the Android readback.** §12/6.1 says GPU results never round-trip
through the CPU, and the develop view on Android now does exactly that. That
is worth recording as a breach with reasons rather than quietly leaving a
constraint the code no longer honours — the next person to read §6.1 and then
`DevelopSession::render` would otherwise conclude one of them is a mistake.
The entry carries the device measurements that forced it, why setting
`preTransform` is not a fix available to us, and the three separate things any
one of which would remove it.
**TD-2 and TD-3**, the serial thumbnail fetch and the unbounded drain, were
found while chasing the tearing and are still outstanding. Both have a known
shape for the fix; neither is a bug, and neither should be discovered again
from scratch.
The architecture document said the app renders "through wgpu to Vulkan on both
Linux and Android", which stopped being true at 6267802, and §12/6.1 claimed a
constraint with no exceptions. Both now say what the code does and point at
the debt entry for why.
The numbers are labelled with what they are. TD-1's readback cost has *not*
been measured on the device and says so, and TD-3's figures are from a debug
build and say so — a documented measurement that quietly turns out to be the
wrong build is worse than no measurement.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
151 lines
6.7 KiB
Markdown
151 lines
6.7 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.
|
||
|
||
---
|
||
|
||
## 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.
|