Three decode rungs were decoding into a surface they were predicting from #102
No Assignees
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: unom/punktfunk#102
Reference in New Issue
Block a user
Three decode rungs were writing decoded pictures into surfaces they were simultaneously predicting from. It was invisible on glass, it survived every green test suite we had, and on H.264 it hit ~99% of the frames of every stream a punktfunk host emits.
This started as the six follow-ups from #95/#96/#97 and turned into that.
The defect
A conversion released the pictures an access unit displaces inside the conversion, then assigned the decode target a slot.
SlotMap::assigntakes the lowest free slot — the one just vacated. So the submission named one surface as bothCurrPicTextureIndexand one of its own references.Fixed the same way everywhere: the plan gains
release_after_decode, and the caller applies removals only once the decode op is issued. It costs no slot.pf-vkdecode's AV1 path already had exactly this concept, which is why that one leg was always correct.decode_rps)Vulkan is the default rung on Linux and on Windows+NVIDIA, and H.264 is the fallback every client can land on. That is the release-blocking part.
Why nothing caught it
The vendored H.264 conformance vector shows zero occurrences. It reorders, and its level/VUI hand it a 7-frame DPB against 2 reference frames — both properties of that one vector. Our hosts emit the opposite shape: NVENC writes
max_num_ref_frames = 3andmax_dec_frame_buffering = 3, a DPB exactly as deep as its reference count, so the sliding window unmarks the oldest reference in the very access unit whose bump evicts it.Three tests turned out to be incapable of failing, each guarding the exact defect that shipped:
pf-vkdecodeasserted the aliasing as "the planner's normal behaviour" — it encoded the bug.pf-dxvadeccarried a vacuous predicate that compared an expression with itself.pf-vaadecasserted the decode target was never a reference while giving every picture a unique surface id — distinct integers cannot collide.Every fix here now carries a counterfactual that makes the bad ordering happen and proves it is caught.
New permanent gates
Host-shaped fixtures, because conformance vectors demonstrably cannot see this class of bug. All goldens cross-checked on two independent ffmpeg builds on two architectures.
lowdelay-640x480.h264(282 KB) — the stream that exposed itlowdelay-640x480.h265(270 KB) — HEVC's exemption held, 0/120, with a counterfactual that makes it 115/120lowdelay-3840x2160.ivf.av1(261 KB) — 4K is mandatory: 720p/1080p/1440p all measure a single tile. Which surfaced that every AV1 frame either rung had ever been measured against was single-tile, so the tile arrays had only ever been written at index 0.gpu_paritygoes 8 legs → 11. D3D11VA parity goes 5 → 8.VAAPI: from unmeasured to parity-proven
That rung had never had a pixel checked. It now has a
#[cfg(test)]readback (vaDeriveImage, falling back tovaCreateImage+vaGetImage) and all seven legs are bit-identical to libavcodec on RDNA3.The readback is kept off the production path structurally — the entry points are resolved by a test-only type that dlopens libva itself, the production
Libvagains no field, and a CPU test scans the file's own source to keep it that way. Zero-copy on the real path is unchanged.It also stopped dropping frames:
finishshowed onlyoutputs.last()and never flushed, so it delivered 225/204/45 of 250/250/50. Now 250/250/50. Two defects fell out of that:coloranddisplaywere stamped from the wrong access unit too (that is the in-band HDR switch), and concealment was about to start draining the queue, which would have zeroed the demotion streak and removed the ladder's escape hatch.VAAPI stays
verified = falseon purpose. Parity is no longer the gap — one vendor and no soak are. And flipping it is a routing change:native_rung_admittedwould put VAAPI ahead of Vulkan Video on every Linux AMD/Intel client, the Steam Deck included. That deserves its own decision.Also in here
The six original follow-ups: the H.264 DPB level-ceiling cliff named and guarded (
PlanWarning::LevelDerivedDpb); the CPU rung's process-isolation question decided and written down with its revisit trigger; the AV1 tile-aware sub-frame reader closed on measurement rather than left open; and the D3D11VA AV1 rung promoted toverifiedon two vendors.Verification
Every gate run on the merged tree, not per-branch.
.21— fmt,clippy --workspace --all-targets -D warnings, all unit suites,gpu_parity11/11.221— 8/8 D3D11VA parity legs on both GPUs.25— 39 tests, 0 failed, 0 ignored, every VAAPI parity legEach fix was also re-verified independently against pre-fix source: the new H.264 leg fails 117/120 at frame 3 without it, and the VAAPI harness was proved falsifiable by planting a real pitch defect.
⚠ Known and recorded, not fixed here: VAAPI is one vendor; rav1d is still not panic-proof (539 abort sites reachable across a C ABI);
pp.widthsends UpscaledWidth where libavcodec sends FrameWidth, identical only while superres is off.The evidence table has said "native VAAPI: has never decoded a frame anywhere (M6/M7)" since the rung was written. That is no longer true. Measured on `.25` (Radeon 780M / Phoenix1 RDNA3, radeonsi, Mesa 26.0.3, VA-API 1.23, Ubuntu 26.04 — headless, no display server needed): VAAPI AV1 rung constructed: native-vaapi av1 VAAPI AV1: 250 frames delivered, first 320x240 fourcc="NV12" modifier=0x200000010401b04 250 of 250 displayed frames, first try, on the same vendored vector the Vulkan and D3D11VA AV1 legs walk. The count matters as more than a smoke test: the vector carries 274 coded frames in 250 temporal units — 24 units carry two, and those extras are HIDDEN (decoded, referenced, never shown) — so 250 delivered is this rung agreeing with the other two about which frames are output. A tiled AMD DRM modifier rather than a linear one says the surface is a real decode target, not a fallback. Two changes, both in the rung's own file. **The probe never asked about AV1.** `probe_this_machines_libva` walked H.264 High, HEVC Main and HEVC Main 10 and stopped there, which is part of why "never decoded a frame" could stand so long without anyone noticing what had not been asked. It now covers both AV1 profiles, and this box answers: H.264 High: VLD decode AV1 Profile 0: VLD decode HEVC Main: VLD decode AV1 Profile 1: no (VAProfile not supported) Profile 1 being refused is correct — 4:4:4 AV1, which radeonsi does not do — and it is the negative case that proves the probe reports rather than assumes. **`av1_decodes_the_vendored_vector_on_this_machines_vaapi`** is the decode itself, `#[ignore]`d beside the probe. It is deliberately WEAKER than the Vulkan and D3D11VA AV1 legs, and the docs say so rather than letting the name imply parity: those two hash every frame against libavcodec's goldens because both can read their decoded surface back. This rung hands out a DRM-PRIME dmabuf whose memory the driver tiles, so there is no CPU-readable image to hash without adding a vaDeriveImage/vaGetImage path that production neither uses nor wants. So it asserts what can be asserted honestly — every temporal unit accepted, the right number of frames back, each a real exported surface of the right shape, the first flagged as a keyframe — and it is NOT frame-hash parity. Promoting this rung to `verified` still wants parity, and parity wants a readback path first. It fails loudly rather than skipping when the device has no AV1 entry point. It is `#[ignore]`d, so it only runs when someone points it at a box that is supposed to have one, and a silent pass there is exactly the invisible-failure mode this program exists to end. Gates: on `.25`, fmt clean, `clippy -p pf-client-core --all-targets -D warnings` green under the Linux cfg where this rung actually compiles, the whole lib suite 167/167, and all 11 VAAPI tests green with `--include-ignored`. Workspace fmt + clippy + lib suite also green in the Linux container. ⚠ Not touched here on purpose: the evidence table in `video.rs`. Its VAAPI row still reads "never decoded a frame anywhere" and now understates what is known — but a parallel agent is editing that same file for the D3D11VA AV1 row, so the row is left for whoever lands second to update once, rather than conflicting. Note for anyone reproducing on `.25`: it has no system SDL3 and no passwordless sudo, so the test binary links only with `--features sdl3/build-from-source` (SDL3 is gamepads, irrelevant to decode; production Linux still links the system one). Its disk sits at ~99% full, and the tree there is a `git archive` export with no `.git`, so `git apply`/`git checkout --` silently do nothing.`fd6241a2` made HEVC's freedom from the release-ordering defect falsifiable on CPU and recorded what was still missing: no low-delay HEVC stream was vendored, so the exemption rested on a structural argument plus one throwaway measurement. This vendors the stream, and the exemption HELD. **The fixture.** `lowdelay-640x480.h265`, 270 KB, 120 pictures — `punktfunk-host spike --source synthetic --codec h265 --width 640 --height 480 --fps 60 --seconds 2 --bitrate 1` on .21 (NVENC, RTX 5070 Ti, driver 610.57.04). Deliberately the H.264 sibling's resolution and frame count: the two are then directly comparable, 640 and 480 are both multiples of MinCbSizeY so there is no conformance window and a hash mismatch can only be decode rather than readback geometry, and 270 KB sits alongside the 282 KB already accepted for H.264. Goldens are libavcodec's software decode, cross-checked BIT-IDENTICAL across ffmpeg n8.1.2 (Arch, x86_64) and 8.1.1 (Homebrew, macOS arm64), 120 of 120 digests distinct. **The exemption held, measured rather than argued.** `sps_max_dec_pic_buffering_minus1 = 4` against the four pictures 8.3.2 keeps marked in steady state, `sps_max_num_reorder_pics = 0`, `numRefL0 = 1` — a five-picture DPB filled exactly by four references plus the current picture. 115 of the 120 access units retire a picture, and `removed ∩ dpb_refs` is **0 of 120**. A 300-picture 1080p stream from the same host reports the same shape: 295 retirements, 0 intersections. It is the encoder and not the resolution, exactly as for H.264. **A zero proves nothing on its own, so the fixture is pinned by its counterfactual.** `test-25fps.h264` reported zero for two milestones while every stream we ship aliased on 99% of its frames. So the guarantee here is not "we looked and it was fine": hand `plan_to_dxva_h265` the marked DPB as it stood BEFORE `decode_rps` — the mutation a snapshot move would cause, reconstructed exactly as `dpb_refs(N-1) ∪ {stored(N-1)}` — and the alias appears on **115 of 120** access units, driven through the real conversion rather than through planner arithmetic. If a regeneration ever produced a stream that reordered, or a DPB deeper than its reference count, that 115 collapses to 0 and the tests say so instead of continuing to pass. **The two rungs are exempt for different reasons, and the asymmetry is now a gate.** DXVA binds the whole marked DPB — `RefPicList` is spec-defined that way, and an RFI long-term anchor has to survive in it — so its exemption really is `H265Planner`'s snapshot ordering, one call away from being untrue. `plan_to_vk_h265` never reads `dpb_refs` at all: `pReferenceSlots` is the slots the operation uses, so it binds the current RPS sets, which `decode_rps` itself derives and which therefore cannot name a picture that same RPS just dropped. A new test feeds that conversion the identical widened snapshot and asserts nothing changes, so a future change making the Vulkan rung bind the marked DPB — a legitimate thing to want, since a *Foll* anchor invisible to the hardware is the RFI failure shape — fails loudly instead of silently acquiring the defect. What the Vulkan pixel leg adds is therefore NOT aliasing coverage, and its docs say so: it is the first HEVC frame either rung has decoded from our own encoder, under a DPB that retires and reissues a slot on 115 of 120 access units back to back, where the vendored vector's reordering keeps that eviction slack. Legs: `low_delay_host_h265_every_frame_hashes_bit_identical_to_libavcodec` on the Vulkan rung (10 ignored legs now, up from 9) and on the D3D11VA rung, plus three non-ignored CPU guards that run in ordinary CI. Verified: 10/10 Vulkan parity legs on .21 (RTX 5070 Ti, 610.57.04), the new one 120/120 bit-identical; workspace clippy `-D warnings` and `cargo fmt --all --check` clean on .21.Every other decode rung earns `verified` with frame-hash parity against libavcodec. VAAPI could not: it hands out a DRM-PRIME dmabuf whose memory the driver tiles, so nothing could read its decoded pixels back, and all four of its legs sat at "never frame-hash parity-checked". That was never bookkeeping. The D3D11VA AV1 rung decoded 250 frames, streamed 4K60 through a clean five-minute soak, and produced WRONG PIXELS for 186 of 250 frames on NVIDIA and 245 of 250 on Intel. It looked perfect on glass; only the goldens caught it, and the same defect turned out to be in H.264 on two other rungs. VAAPI was the one rung where that class of bug could still be sitting with nothing able to see it. It is not. Measured on .25 (Radeon 780M, RDNA3, radeonsi, Mesa 26.0.3, VA-API 1.23) on 2026-08-08, against the SAME golden files the Vulkan and D3D11VA rungs are held to, read across the crate boundary rather than copied: H.264 vendored vector 250/250 bit-identical (7 from the flush) H.264 our host, low-delay 640x480 120/120 bit-identical (3 from the flush) H.265 vendored vector 250/250 bit-identical (2 from the flush) H.265 our host, low-delay 640x480 120/120 bit-identical (0 from the flush) HEVC Main 10, P010 50/50 bit-identical (2 from the flush) AV1 vendored vector 250/250 delivered of 274 decoded, and display frame 0 byte-identical to libavcodec's own PIXELS AV1 our host, 4K two-tile 60/60 bit-identical ⚠ ONE vendor. AMD/radeonsi only; no Intel iHD box has run these legs. The readback that made it possible: * `pf-vaadec`'s `va` module gains `VAImage` and `VAImageFormat`, hand-declared with every size and offset measured off libva 2.23.0's real headers by `layout-probe.c` and pinned as compile-time assertions — the same discipline the decode buffers already keep. The trap: `VAImage::width`/`height` are 16-bit, so `data_size` sits at 60 and not at the 64 counting 32-bit fields gives, and every field after them is two bytes earlier than it looks. * `pack_two_plane` is the pure geometry — the crop to the picture, the padding columns dropped per row, and the chroma plane taken from the driver's OWN `offsets[1]` rather than from `pitch * display_height`, which is the 1088-row smear this program has already paid for once. It needs no device, so ten CPU tests cover it on macOS and in the container. * `video_vaapi_native::parity` drives the seven streams above through the production entry point and hashes what the rung DELIVERS, in delivery order, tail included — so the delivery path is under test as well as the decode, and a frame's surface comes from its own release token rather than from an inference about which pool entry holds which picture. THE READBACK CANNOT REACH THE PRODUCTION PATH, and that is structural rather than a promise. `vaDeriveImage`, `vaCreateImage`, `vaGetImage`, `vaMapBuffer` and the rest are resolved by a `#[cfg(test)]` type that dlopens libva itself; the production `Libva` gains no field; `sha2` is a dev dependency. A CPU test scans this file's own source and fails if any of those symbols is dlsym'd outside the harness, so a refactor cannot quietly undo it. Derive is not guaranteed, so both routes are implemented and neither is optional: `vaDeriveImage` first, `vaCreateImage` + `vaGetImage` as the fallback (which also detiles), and if neither yields the pool's own fourcc the leg FAILS naming what the driver gave it. There is no skip path — a parity test that passes because it could not read anything is the failure mode this program has been bitten by three times. Both answer on radeonsi, the first frame of every leg is read through BOTH and they must agree, and `PF_VAAPI_READBACK=getimage` reproduces the H.264 leg's 250/250 through the copying route alone, so the fallback is exercised rather than merely written. And it can fail — proven, not asserted. Planting the real geometry defect this driver's layout makes visible (rows read contiguously, ignoring the 512-byte pitch behind a 320-wide picture) fails at display frame 0 with the full localisation: 68312 luma and 14998 chroma samples differing, max |delta| 255, luma bounding box (0,1)..(319,239) — and with the goldens forced through one route, 250/250 diverging with "suspect the readback geometry". `compare` and `localise` also have CPU counterfactuals, and a hardware leg proves the readback reads real and DISTINCT pixels and localises a one-byte flip to the exact pixel. ⚠ One thing the hardware legs do NOT cover, found by planting the other defect and watching it do nothing: radeonsi's decode surfaces for every fixture here have no VERTICAL padding — `offsets[1]` is exactly `pitch * height` — so the chroma-plane trap is untested on this driver, and `pf-vaadec`'s `reading_chroma_at_the_display_height_would_have_been_caught` is the only place it is checked at all. `probe_this_machines_readback_routes` now prints the derived layout and says which of the two it is, so the next driver answers for itself instead of being assumed.