A damaged AV1 frame stops killing the whole client (and a BOM stops erasing every setting) #97
No Assignees
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: unom/punktfunk#97
Reference in New Issue
Block a user
Stacks on #95 (
worktree-av1-subframe-truncation) — the 4K AV1 repro needs that host fix, and this branch edits the samevideo.rsevidence-table note. Please merge #95 first; the base here is that branch, notmain.The defect
The software rung aborted the process — not the session — the first time a 4K AV1 stream lost a frame. Reproduced on
.21twice on 08-07,SIGABRTa few hundred ms after "first frame decoded".What it actually was
Not 4K, and not our bitstream.
rav1d 1.1.0 kills the process on any decode error while it holds a single frame context:
rav1d_submit_frame'sc.fc.len() == 1branch callsrav1d_decode_frameinline;rav1d_decode_frame_exit, which does an unconditionalmem::take(&mut f.frame_hdr)(decode.rs:4873);Err, the same branch re-enters a localon_errorwhose first act isf.frame_hdr.as_ref().unwrap()(decode.rs:4997) — on theNonethe teardown just left.The panic unwinds into
dav1d_send_data, which isextern "C", so it ispanic_cannot_unwind→abort(). Nocatch_unwindat our call site, no rung demotion, noNoSoftwareRungrefusal can catch it.The same code is in upstream
maintoday, and 1.1.0 is the newest release — there is no version to bump to. rav1d's whole public surface is dav1d's C ABI (every internalrav1d_*entry point ispub(crate)), so there is no unwinding entry point to call instead.4K was only where an error first happened: the CPU rung cannot keep up at 3840×2160 (35–39 fps against 60), the receive backlog stopped draining,
pump::dataflushed 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, thenInvalid data found when processing input— and simply carries on. At 1080p the rung keeps up, nothing is flushed, no AU is damaged, and nobody ever saw this.The fix
Stop asking rav1d for the configuration whose error path is broken.
c.fc.len() > 1never callsrav1d_decode_frameat all — errors come back throughcached_error/task_thread.retvalas ordinaryEINVALs, which the pump already answers with a keyframe request.EINVAL, survivesRow 3 is why
n_threadsgrows a floor of two as well as the delay:n_fc = min(max_frame_delay, n_threads), so one decode thread silently reinstates the abort. It is also what rules out the theory this investigation started with — pinning threads to 1 was the suspected trigger, and it makes things worse. The tile workers are innocent; the single frame context is the entire defect.Two frame contexts normally cost a frame of latency, and this does not.
decodenow drains past the firstEAGAIN:rav1d_get_pictureonly reaches its blockingdrain_pictureon a call whose owndrainflag is already set, and that flag is set by the previousget_pictureand cleared by everysend_datathat carried bytes. So the firstEAGAINafter a send means "ask again", not "no picture" — this AU's frame comes out of the second call. Over 14 temporal units atn_fc = 2: stopping at the firstNoneproduces nothing for units 0 and 1; draining past it produces one frame per unit from unit 0, at 20–42 ms/unit againstn_fc = 1's 21–53 ms. Same cadence, slightly faster, because the tile workers overlap the drain.Av1Software::newthen asks rav1d itself (dav1d_get_frame_delay) what those settings bought and refuses to open a one-frame-context decoder. Not a restatement of the arithmetic — it isget_num_threads' own answer. Losing the rung is recoverable;abort()is not.On glass (.21, 35 s sessions,
PUNKTFUNK_DECODER=software)SIGABRTon the second frame, every rundecode_failed=0, and 13 decode errors recovered from across 17 backlog flushesThe 4K row is the point: the exact condition that used to abort happened thirteen times in one session and the client recovered from every one.
What this does not buy
rav1d has other
unwrap()s, and because its public surface isextern "C", no in-process guard can turn one into anything but an abort. This removes the one we hit and can prove. The evidence table now says exactly that, instead of the old "ABORTS the process on 4K".Upstream
Reported with a self-contained reproducer that needs no proprietary capture: the in-tree
test-25fps.ivf.av1vector with one temporal unit dropped aborts rav1d atn_fc = 1, survives atn_fc = 2, and libdav1d decodes it with 145 error reports and no crash. The upstream fix is one line —is_some_andinstead ofunwrap— and with it applied locally,n_fc = 1returns a cleanEINVALand decodes 13 pictures, matching libdav1d exactly.Second, separable commit — the settings BOM
Set-Content -Encoding UTF8writes a UTF-8 BOM. serde_json rejects it at byte 0, and.and_then(|s| from_str(&s).ok())turned that refusal intoDefault— every setting gone, file plainly correct on screen, nothing logged. Cost an hour on 08-07 when acodec: "av1"edit was ignored and the client negotiated HEVC.The mark is now stripped, and the
.ok()that hid it — which hides trailing commas, truncated writes and typos just as completely — now costs onewarn!naming the file and serde's line/column. An unreadable file (-Encoding Unicodewrites UTF-16LE, whichread_to_stringrejects) is reported too. The result is deliberately unchanged (Default, never an error), and a missing file stays silent because that is just first run. All three JSON stores share the loader — settings, known-hosts (where a BOM silently unpairs every host) and profiles — because all three had the identical line.Gates
cargo fmt --all -- --check,cargo clippy --workspace --all-targets -- -D warnings,cargo test -p pf-client-core --lib(166 passed),cargo test -p pf-bitstream -p pf-vkdecode— all green in the Linux container.New tests:
the_av1_rung_never_opens_rav1d_with_a_single_frame_context,one_decode_thread_would_put_the_rung_back_on_the_aborting_path,a_bom_does_not_turn_a_settings_file_into_defaults. The existingsoftware_av1_decodes_and_reports_its_sequence_colouris now also the latency guard — itsframes == unitsover 250 temporal units is what says the drain loop still returns this AU's picture in this call.Boxes restored: host drop-in removed and
punktfunk-host.serviceback on/usr/bin/punktfunk-host, client settings byte-identical to the backup, captures and build worktree deleted.Filed upstream: memorysafety/rav1d#1497 — https://github.com/memorysafety/rav1d/issues/1497
It carries the root-cause walk-through, the one-line fix (
is_some_andfor theunwrap), and a reproducer that needs no capture from us: any AV1 stream with one temporal unit removed from the middle. Their own CLI reproduces it in one line, since it already exposes the knob:A third commit (
bca63cd9) writes that issue number next to the setting inav1_settings, so the next person to wonder whether the floor is still needed can check rather than re-derive it. Note that even if upstream ships the fix, the floor stays the right default — it is what makes a decoder error an error — only thebail!inAv1Software::newwould relax.Gates re-run after that commit: fmt clean, clippy clean, 166 tests pass.
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.`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.