AV1 never decoded on AMD, and a 401 ms host rebuild cost three minutes of bitrate #277

Merged

enricobuehler merged 6 commits from worktree-field-repairs-av1-abr into main 2026-08-17 07:49:19 +00:00

Conversation 0 Commits 6 Files Changed 14

+1055 -29

6 Commits

Author	SHA1	Message	Date
enricobuehler	1eef3c9799	Merge remote-tracking branch 'origin/main' into worktree-field-repairs-av1-abr	2026-08-17 01:48:29 +02:00
enricobuehler	44692a0dc1	feat(host,client): the host tells the client when it stopped the stream itself ... A Windows exclusive-topology eviction makes the host rebuild its capture ring and encoder in place. It takes a few hundred milliseconds and is entirely host-local: no packet is lost, the link never changes. But the client's bitrate controller decides on 750 ms report windows, and a window straddling that rebuild sees almost no stream. The 0.29 field log: 401 ms of rebuild produced a window reporting actual_kbps=390 against a 20 000 target with loss_ppm=0, which the controller read as congestion — x0.7 and slow start retired, three minutes at ~15 Mbps on a link that never dropped a packet. The client already knows how to throw a window away. `discard_abr_window` feeds the controller nothing, sends no LossReport (so a bogus window cannot spike the host's adaptive FEC) and closes the standing-latency detector as not-loss-free. It had exactly one cause: the tail of the client's own speed test. This adds the second, and it is the one party that actually knows — the host. `PipelineGap` (0x0A, host->client, 9 bytes) carries the rebuild's measured span. A DURATION, never an instant: host and client clocks are not in the same domain — 14.7 s apart in that same log — so an instant would need skew-correcting before it meant anything. The client anchors the gap to its own receive time and gap_ms is evidence for the log rather than an input to arithmetic. The span is read from the transition trace's own total, so the number the client logs is the total_ms on the host's trace line: one number, checkable from either end of a field report. Announced on both in-place rebuild paths, including the mode switch. The mode switch is NOT already covered by the client's `on_mode_switch` reset: that clears the learned caps, the three latency baselines and the proven mark, which does mute OWD/decode/encode for a few windows — but it touches neither `current_kbps`, `probing` nor `bad_windows`, and every signal that needs no baseline (an unrecoverable frame, a flush, heavy loss over a near-empty denominator, a keyframe-ask storm) still scores the straddling window, where one severe verdict costs the same x0.7 plus slow start. Deliberate limitation: only the window in flight is discarded. A rebuild long enough to straddle a window boundary damaged the previous window too, and that one is already decided. Retracting it would mean holding every window back by a window in case a gap follows — trading a rare over-reaction for a permanent one. Additive: no ABI bump, no wire break. 0x0A extends the contiguous 0x01-0x09 rate-control block its only consumer already lives in, and is not in the 0x30 clock block precisely because no clock domain is involved. A client that predates it hits its "unknown control message" arm and keeps today's behaviour; a host that predates it never sends one. Tests: wire round trip including cross-decode against the three identically-shaped rate-control messages either side of it (the type byte is the only thing keeping a gap from re-decoding as a SetBitrate), and an end-to-end pump test driving a real ControlTask over a real QUIC control stream into a real DataPump, asserting the straddling window produces no request and the next one reports normally. 464 core tests green, fmt and clippy clean. Not verified here: the Windows-gated eviction call site compiles on no platform available locally (aws-lc-sys needs windows.h to cross-compile). Type-checked by reading; owed a Windows runner build. Known gaps, stated rather than papered over: a mode-switch rebuild that FAILS keeps streaming the old mode and leaves its stall unannounced, and three other in-place rebuild sites are still silent — the Gaming/Desktop session switch, the ABR re-target's fallback open_video rebuild (~0.6 s, and self-inflicted: the controller causing the stall its next window reads as congestion), and reset_stalled_encoder.	2026-08-17 01:36:49 +02:00
enricobuehler	e50941c839	fix(win-display): the EDID unlock's expected no-op is not a warning ... Every host start on an AMD box logged four of these: WARN edid_lock: adl-unlock-mode-off adapter5.connector0[DP] ok=false rc=-8(ADL_ERR_NOT_SUPPORTED) The unlock is deliberately idempotent and runs over every connector, including the ones that were never pinned — and over all of them on a host recovering from an unclean exit. Some drivers answer NOT_SUPPORTED to "turn emulation off" where there is no emulation to turn off, so a perfectly healthy start emitted one warning per connector, saying nothing. Four standing warnings are how a log stops being read: these were the first thing to catch the eye in a field bundle whose actual fault was elsewhere. Scoped to the mode-off call on purpose. adl-unlock-remove is the call that actually clears a pin, so its rc is the one that means something, and it keeps its warning. No behaviour change — the unlock did and does the same thing; only its severity when it no-ops is now honest.	2026-08-17 00:10:18 +02:00
enricobuehler	abaac704d7	feat(client/abr): bound the learned climb ceiling by what the stream can use ... The climb ceiling is pure link capacity — `delivered_kbps * 0.7`, with no term for resolution, frame rate, codec or bit depth. The utilization gate cannot supply one either: a hardware encoder in CBR mode genuinely fills whatever target it is handed, and the field log shows utilization at 99 % the whole way up, so "the encoder could not use the rate" never fires. On a gigabit LAN the probe measured 939 Mbps and the session walked to 657 Mbps for 1440p120 — 1.49 bits per pixel — in 37 seconds. Getting there took the client's decode latency from 0.78 ms to 10 ms. `stream_ceiling_kbps` computes what the shape could plausibly use from pixel rate and a bits-per-pixel allowance that varies by codec generation, bit depth and chroma, and `set_ceiling` holds the measured link ceiling to it — the same funnel PUNKTFUNK_ABR_MAX_MBPS already passes through, and it logs both numbers whenever it binds, because a cap that silently trims what the link offered is the kind of thing nobody reports. Deliberately generous: this is a bound on the absurd, not a quality opinion. 1440p120 HEVC Main10 lands at ~414 Mbps — under the decode knee this session actually found (flat at ~396 Mbps delivered, 10 ms by ~461) — while 1080p60 HEVC keeps ~93 Mbps, above anything people run there. Tested from both directions, because a cap that trims a happy user is a regression nobody files. Binds only what the probe LEARNS. A negotiated start rate is a number the host resolved on purpose and is left alone; an explicit bitrate and every PyroWave session are outside the controller entirely and never reach here. Sessions that never call set_stream_cap behave exactly as before. This is a backstop, not the answer to "how much is enough" — that is content-dependent and only the encoder knows it, at minimum QP. It is the part that works without new host telemetry.	2026-08-17 00:10:18 +02:00
enricobuehler	88d071c033	fix(client/abr): a window that carried nothing cannot report the encoder as slow ... An exclusive-topology eviction on a Windows host rebuilds the capture ring and the encoder in place — 401 ms, entirely host-local, no packet lost. The client's 750 ms report window straddled one and recorded 390 kbps delivered against a 20 000 target, loss_ppm=0, no flush, and encode_mean_us=15063 against a ~2800 baseline. That cleared ENCODE_SEVERE_US, took the one-window path, and cost a x0.7 plus slow start for the rest of the session. Recovery is then +6 % per six clean windows, so the field session sat at ~15 Mbps for the three minutes it had left. encode_us is a per-AU host measurement averaged over the window. When almost no AUs flowed, the mean is taken over the handful that straddled whatever interrupted them, and their encode time carries that interruption rather than the cost of encoding at this rate. It is not a measurement, so it is now withheld entirely when the window is STARVED — the predicate that already existed for exactly this shape of window, hoisted above the signal scoring. Passed as absent rather than ignored, so it cannot teach the rolling-minimum baseline either. Deliberately narrow. The first attempt took the one-window shortcut away from every severe signal in a starved window and broke two tests that turned out to be right: loss, a flush and a dropped frame describe what reached the CLIENT and mean the same thing however little flowed, so the periodic-capture-stall case still backs off on one window as STARVED_DELIVERY_DIV's own comment requires. Only the host-encode signal is withheld, because only it is measured over AUs that did not exist. Slow start is likewise left alone: a starved window that is bad for a legitimate reason still ends it. Tests: the field window verbatim decides nothing and leaves slow start armed, and the same encode excursion in a window that actually carried its rate still backs off on one window — which is what proves the withheld sample never entered the baseline. 459 core tests green.	2026-08-17 00:10:18 +02:00
enricobuehler	a602e7cf91	fix(pf-bitstream): AV1 needs 32-bit reads, and the 31-bit cap belonged to the signed path ... Every AV1 session on an AMD host died after ~287 frames and silently fell back to H.265. The client log named it on the first access unit — "AV1 parse: more than 31 (32) bits were requested" — and then "No sequence header parsed yet" for every AU after, because the sequence header never parsed and each new keyframe re-hit the same wall. The vendored cros-codecs BitReader refused any read wider than 31 bits, "because that would break the read_bits_signed() function". True of the signed path's i32 accumulator, and misplaced: AV1 needs 32 bits in five places — timing_info's num_units_in_display_tick and time_scale, decoder_model_info's num_units_in_decoding_tick, and the variable-width buffer-delay and buffer_removal_time fields, whose lengths come from the stream and reach 32. AMF sets timing_info_present_flag; NVENC does not, which is why the rung's own evidence string ("one vendor, no soak") described a codec that had never once decoded on AMD. Upstream's BitWriter already accepted 32 bits, so the crate could emit a header it could not read back. Relaxing the guard alone would have been worse than the bug — three edits are required together: - the trailing mask is u32::MAX at 32. `1u32 << 32` overflows: a debug panic, and in release a mask of zero, i.e. a silent 0 return; - the byte cursor is advanced before the accumulation loop when it sits at zero remaining bits, which otherwise shifts by the full width and ORs the spent byte in. At <=31 bits the mask discarded those bits, so it was invisible; at 32 the mask is all-ones and cannot; - read_bits_signed carries its own > 31 guard, so widening the unsigned path does not silently widen the signed one into an overflow. This is the limit the original comment was actually protecting. That last guard made a latent panic reachable by test: the sign extension `-1 ^ ((1 << num_bits) - 1)` overflows at num_bits == 31, where 1i32 << 31 is i32::MIN and subtracting one from it panics in debug — a width the guard admits and upstream considered safe. Rewritten as `-1i32 << num_bits`, equal for every accepted width. Blast radius is provably AV1-only: neither H.264 nor H.265 has a read wider than 31 bits, literal or variable — every dynamic-width call site in the vendored tree is in the AV1 parser. The 52 upstream conformance tests (H.264/H.265/AV1/VP9) still pass unchanged. Tests: 32-bit reads byte-aligned, mid-byte, and entered on a spent cursor; 33 bits still refused; the signed path stops at 31 and still sign-extends; every width 1..=31 checked against an independent extraction across a spent-byte boundary; zero-width reads still consume nothing. End to end, an AV1 sequence header carrying timing_info now survives a synthesize/parse round trip — and reproduces the field error string exactly when the guard is reverted. Recorded as PROVENANCE deviation 8; owed upstream as a cros-codecs issue.	2026-08-17 00:10:18 +02:00