A damaged AV1 frame stops killing the whole client (and a BOM stops erasing every setting) #97

Merged

enricobuehler merged 3 commits from worktree-rav1d-single-frame-context-abort into main 2026-08-07 16:20:58 +00:00

Conversation 1 Commits 3 Files Changed 4

+344 -52

3 Commits

Author	SHA1	Message	Date
enricobuehler	bca63cd9ff	docs(client/video): name the upstream rav1d issue next to the workaround ... memorysafety/rav1d#1497, filed with the one-line fix and a reproducer that needs no capture — any AV1 stream with one temporal unit removed. Written down where the setting is, because the next person to read `av1_settings` and wonder whether the floor is still needed should be able to check rather than re-derive it.	2026-08-07 17:48:54 +02:00
enricobuehler	eb9203b115	fix(client/settings): a byte order mark stops silently erasing every setting ... `Set-Content -Encoding UTF8` writes a UTF-8 BOM, and every Windows how-to reaches for it, so `%APPDATA%\punktfunk\client-windows-settings.json` edited from a shell arrives with `EF BB BF` in front of the `{`. serde_json rejects that at byte 0 — correctly, JSON has no BOM — and `.and_then(|s| serde_json::from_str(&s).ok())` turned the refusal into `Default`. Every setting in the file, gone, with the file plainly correct on screen and not one word anywhere about why. Cost an hour on 08-07: a `codec: "av1"` edit was ignored and the client negotiated HEVC. The obvious suspects — the negotiation, the caps, the host — were all working exactly as designed. So the mark is stripped, which is what every other JSON consumer on Windows does. But the BOM is only the instance; the bug is the `.ok()`, which hides a trailing comma, a truncated write and a hand-edit typo just as completely. Those now cost one `warn!` naming the file and serde's own line and column. A file that cannot be READ at all is reported too, and for the same reason: PowerShell's `-Encoding Unicode` writes UTF-16LE, `read_to_string` rejects it as invalid UTF-8, and that lands in exactly the same hole. The RESULT is deliberately unchanged — `Default`, never an error. Nothing about streaming may hinge on a settings file being readable, and refusing to start because one is malformed would be a worse failure than the one being fixed. A missing file stays silent, because that is just first run. All three of this client's JSON stores share the loader, because all three had the identical line: the settings file, the known-hosts store (where a BOM silently unpairs every host) and the profiles catalog.	2026-08-07 17:45:38 +02:00
enricobuehler	32ff93199f	fix(client/video): a damaged AV1 frame stops killing the whole client ... The software rung aborted the process — not the session, the process — the first time a 4K AV1 stream lost a frame. Reproduced on .21 twice on 08-07, `SIGABRT` a few hundred milliseconds after "first frame decoded". It was never about 4K, and it was never our bitstream. rav1d 1.1.0 kills the process on ANY decode error while it holds a single frame context. `rav1d_submit_frame`'s `c.fc.len() == 1` branch calls `rav1d_decode_frame` inline; that always finishes in `rav1d_decode_frame_exit`, which does an unconditional `mem::take(&mut f.frame_hdr)` (decode.rs:4873); and then, only if the decode returned `Err`, the same branch re-enters a local `on_error` whose first act is `f.frame_hdr.as_ref().unwrap()` (decode.rs:4997) — on the `None` the teardown just left. The panic unwinds into `dav1d_send_data`, which is `extern "C"`, so it is `panic_cannot_unwind` → `abort()`: no `catch_unwind` at our call site, no rung demotion and no `NoSoftwareRung` refusal can catch it. The same code is in upstream `main` today, and 1.1.0 is the newest release, so there is no version to bump to. 4K was only where an error first HAPPENED. The CPU rung cannot keep up at 3840x2160 (35-39 fps against a 60 fps stream), so the receive backlog stopped draining, `pump::data` flushed it and jumped to live, and the next AU referenced frames nobody had decoded. libdav1d gives the identical verdict on the identical capture — 13 frames, then "Invalid data found when processing input" — and simply carries on. At 1080p the rung keeps up, nothing is ever flushed, no AU is ever damaged, and the same code ran for years without anybody seeing this. So the fix is to stop asking rav1d for the configuration whose error path is broken. `c.fc.len() > 1` never calls `rav1d_decode_frame` at all: it hands the frame to `rav1d_task_frame_init` and errors come back through `cached_error` / `task_thread.retval` as ordinary `EINVAL`s, which the pump already answers with a keyframe request. Measured, against the captured 4K stream: n_threads=8 max_frame_delay=1 -> n_fc=1 -> ABORT n_threads=1 max_frame_delay=1 -> n_fc=1 -> ABORT n_threads=1 max_frame_delay=2 -> n_fc=1 -> ABORT <- proves the rule n_threads=8 max_frame_delay=2 -> n_fc=2 -> 13 pictures, EINVAL, survives n_threads=8 max_frame_delay=0 -> n_fc=3 -> survives The third row is why `n_threads` grows a floor of two as well as the delay: `n_fc` is `min(max_frame_delay, n_threads)`, so one decode thread silently puts the whole thing back on the aborting path. That row is also what rules out the theory this investigation started with — pinning threads to 1 was the suspected trigger, and it makes things WORSE, so the tile workers are innocent and the single frame context is the entire defect. Two frame contexts would normally cost a frame of latency, and this does not, because `decode` now drains PAST the first `EAGAIN`. `rav1d_get_picture` only reaches its blocking `drain_picture` on a call whose own `drain` flag is already set, and that flag is set by the PREVIOUS `get_picture` and cleared by every `send_data` that carried bytes — so the first `EAGAIN` after a send does not mean "no picture for this AU", it means "ask again", and this AU's frame comes out of the second call. Stopping at the first `None` is what a single-frame-context reading of dav1d's API teaches, and it would have put the pipeline two frames behind while looking perfectly healthy. Measured over 14 temporal units at `n_fc = 2`: stopping at the first `None` produces nothing at all for units 0 and 1; draining past it produces one frame per unit from unit 0, at 20-42 ms per unit against `n_fc = 1`'s 21-53 ms. Not a trade — same cadence, slightly faster, because the tile workers overlap the drain. `Av1Software::new` then asks rav1d itself, through `dav1d_get_frame_delay`, what those settings actually bought, and refuses to open a decoder that would run with one frame context. That is not a restatement of the arithmetic: it is `get_num_threads`' own answer, so it stays right if rav1d's derivation changes. It is there because the failure it guards is uniquely quiet — an edit that reinstates `n_fc = 1` costs nothing at build time, nothing in the tests and nothing on a clean link, and then kills the client the first time a frame arrives damaged. Losing the rung is recoverable; `abort()` is not. On glass, .21, 35-second sessions, `PUNKTFUNK_DECODER=software`: 4K60 AV1 before: SIGABRT on the second frame, every run after: exit 0, 0 panics, 35-39 fps, 1204 frames, decode_failed=0, and 13 decode errors recovered from across 17 backlog flushes — the exact condition that used to abort, survived thirteen times in one session 1080p AV1 after: 40 fps, decode p50 2.2 ms (2.1 ms before the change) What this does NOT buy: rav1d has other `unwrap()`s, and because its whole public surface is dav1d's `extern "C"` ABI — every internal `rav1d_*` entry point is `pub(crate)` — no in-process guard can turn one of them into anything but an abort. This removes the one we hit and can prove; it does not make the CPU rung panic-proof, and the evidence table says so. Reported upstream with a self-contained reproducer: the in-tree `test-25fps.ivf.av1` vector with one temporal unit dropped aborts rav1d at `n_fc = 1`, survives at `n_fc = 2`, and libdav1d decodes it with 145 error reports and no crash.	2026-08-07 17:45:21 +02:00