feat: add H.264 hardware decoding - #26
Conversation
|
I laughed when I read Thanks for submitting it though, cool stuff. I had a rough implementation of decoding working a long time ago but decided to keep the scope relatively smaller by focusing only on encoding. That said, I'm completely open to adding decoding support. It will probably take some time to go over it. For full transparency: I'll probably review the public API and use LLMs to review the internals. |
|
Take your time @hgaiser - and feel free to be strict on demands/wants! Also a random 🎉 - pixelforge decoding works under Windows as well :P
|
|
@hgaiser Sorry for the bump, I'd just like to know when you believe you'll have the time and energy to look atleast into surface-level changes (i.e. the public/dev-facing API) 🤔 It doesn't have to be reviewed all at once, see what immediately "sticks out" or even irks you and I'll make the changes 🫡 I can also rebase against current |
|
No worries, fair question. I started looking into this yesterday, it's high on my to-do list :) I'll try to get to it "soon" 🙄 |
|
I was wondering: would it make sense to have a similar asynchronous behavior here, comparable to the encode pipeline? It could look something like: // Producer thread: feeds packets, never blocks on output
let producer = std::thread::spawn(move || {
for au in access_units(&stream) {
decoder.decode(au, pts).unwrap(); // just accepts the packet
}
decoder.flush().unwrap(); // signal end of stream
});
// Main/consumer thread: awaits decoded frames
loop {
match decoder.download().await {
Ok(Some(frame)) => render(frame),
Ok(None) => break,
Err(e) => return Err(e.into()),
}
} |
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
e45d7c5 to
42e9b89
Compare
The decode surface spoke H.26x: `access_units` parses Annex B NAL units and slice headers, which AV1 (OBU temporal units) and VP9 (superframes) do not have, and the frame fields named H.264 concepts directly. - Replace the free `access_units` function with `Decoder::split`, dispatched through `DecoderApi` so each codec supplies its own framing and the caller never restates the codec. The H.264 splitter moves to `decoder::h264`. - Rename `DecodedFrame::poc` to `display_order` and document it as the codec's own ordering value (POC, AV1 order hint). - Rename `DecodedFrame::is_idr` to `is_keyframe`, matching the encoder's `EncodedPacket::is_key_frame`. - Neutralize the decode docs: "coded frame" instead of "access unit". Verified against ffmpeg on RADV GFX1200: base, bframes, multislice and zerolatency all decode byte-identical to `ffmpeg -pix_fmt nv12`. That machine has no `VK_LAYER_KHRONOS_validation`, so the run did not exercise the validation layers. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Frames were valid only until the next `decode`/`flush` call: the reorder pool reclaimed every handed-out image at the start of the next batch. That contract cannot survive a decoder that runs ahead of its consumer, which is what the asynchronous decode API needs. - Split the freshly decoded picture (`DecodedPicture`, internal, lives in a DPB slot) from the frame handed to the caller (`DecodedFrame`, owns its storage). - `DecodedFrame` now carries a `FramePin` that releases its pool image on drop, through a `ReleaseQueue` so the frame can be dropped on any thread. The pool reclaims released images the next time it needs one. - Pool images are reused when their frame is dropped rather than at a fixed point in the decode loop, so a display-order frame stays valid as long as the caller holds it. `DecodedFrame` is no longer `Clone`. - Warn instead of silently dangling if the decoder is dropped while frames are still alive. Decode-order frames still borrow the DPB image directly and keep the old validity rule; pinning those is the next commit. Verified on RADV GFX1200: display-order output for base, bframes, multislice and zerolatency is byte-identical to `ffmpeg -pix_fmt nv12`. That machine has no `VK_LAYER_KHRONOS_validation`, so the run did not exercise the validation layers. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… copy Decode-order output handed the caller the decoder's own DPB image and hoped they were done with it before the next `decode` call. Now the slot is pinned for as long as the frame lives, and the session reserves spare slots so decoding can continue while the caller holds frames. - `DecodeConfig::with_output_depth` (default `DEFAULT_OUTPUT_DEPTH` = 2, the encoder's pipeline depth) reserves that many DPB slots beyond what the stream's reference count needs. - `SlotPins` tracks which slots handed-out frames hold. `DecodeDpb` skips them when allocating, however the reference rules mark them, and `decode` blocks on the condition variable when every slot is busy but a pinned one could still come back. Releases are eager, since a decode may be waiting. - Frames fall back to a pool copy when pinning is impossible: a driver without `DPB_AND_OUTPUT_COINCIDE` (the picture lands in one shared output image), or a device whose DPB slot limit leaves no room to spare. - Session creation now asks for `max_active_references` explicitly instead of `slot_count - 1`, so the output reservation does not inflate the active reference count, and reports an error if the device cannot supply what the stream needs. Verified on RADV GFX1200 (7 DPB slots, 2 reserved for output, coincide=true, so the zero-copy path is the one exercised): display-order output for base, bframes, multislice and zerolatency stays byte-identical to `ffmpeg -pix_fmt nv12`, decode-order output is unchanged from before this commit, `decode_adopted` matches on its own device, and encode-to-decode roundtrip does 30 frames in and 30 out. Note: nestripc-1 has no `VK_LAYER_KHRONOS_validation`, so none of these runs exercised the validation layers. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`enable_validation` loaded the layer but registered no messenger, so the layer had nowhere to report and its findings were silently dropped. Enabling validation looked like it worked while verifying nothing: a run with a real VUID violation would have been indistinguishable from a clean one. Create a `VK_EXT_debug_utils` messenger alongside the layer and map its severities onto tracing levels (error/warn as-is, layer info at debug, verbose at trace), so `RUST_LOG` controls the volume. The callback always returns `VK_FALSE`, leaving the offending call to proceed. Contexts adopted from a caller's instance get no messenger, since reporting there belongs to whoever created the instance. Falls back with a warning when the extension is unavailable, the same way a missing layer already does. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`decode` recorded one picture into a single shared command buffer, submitted it and waited on the fence before returning, so the GPU sat idle while the CPU parsed the next picture and vice versa. It now records, submits and returns a `DecodeFuture`, mirroring `Encoder::encode`. - `decoder::pipeline` holds `DECODE_PIPELINE_DEPTH` (2) slots, each with its own coded-data staging buffer, decode command buffer and fence, plus a transfer command buffer and fence for the reorder copy. A slot is busy from submit until its work completes, which is what makes its buffers safe to record over. - Two timeline semaphores: decodes chain on one to stay in DPB order, and each reorder copy waits on both its own decode and the previous copy. Chaining the copies is what lets one fence stand for a whole batch. - A completion thread waits on fences, accumulates each call's frames and resolves its future. Only the calling thread touches queues and timelines. - `download` and `copy_frame_to_planes` drain in-flight decodes first when the frame borrows a DPB image. Those copies move the image's layout, and later pictures may still be reading it as a reference; the transfer queue has no dependency on the decode queue otherwise. Pool-backed frames skip the drain. - `SlotSync` moves from `encoder::pipeline` to `video`, shared by both directions rather than written twice. `DecodeConfig::output_depth` now also bounds how many futures a decode-order caller should keep pending, since an unresolved future holds frames and each frame holds a DPB slot. Documented on the setter and in `examples/decode_h264`, which keeps two batches in flight. Measured on RADV GFX1200, 600 frames of 320x240 with no readback, best of three: display order 1992 -> 2365 fps, decode order 1799 -> 2005 fps. Output is unchanged: display order stays byte-identical to `ffmpeg -pix_fmt nv12` on base, bframes, multislice and zerolatency, decode order matches the pre-pipeline bytes, and `verify_planes`, `decode_adopted` and the encode-to-decode roundtrip all pass with the validation layers enabled and silent. Adopted devices must now enable `timelineSemaphore` as well as `synchronization2`; documented on `build_from_existing_decode` and `DeviceRequirements`, and enabled in both adopting examples. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Intel's ANV decoded almost nothing: every picture came back as flat gray with a few macroblocks in the top-left corner, no validation error, no driver message. ffmpeg's Vulkan decoder is byte-exact on the same driver, which ruled out the driver being incapable. The difference is the start code. The leading zero byte of `00 00 00 01` is legal Annex B and RADV accepts a slice offset pointing at it, but ANV does not recover from it. ffmpeg emits three bytes for exactly this reason. Also log the negotiated bitstream alignments in the session line, which is what made it possible to rule out an alignment mismatch (ANV asks for 32/1, and the range already starts at 0). Verified byte-identical to `ffmpeg -pix_fmt nv12` on both GPUs now, display and decode order, validation layers enabled and silent: - Intel Arc A310 (ANV, needs `ANV_DEBUG=video-decode,video-encode`): was wrong on all four streams, now correct. - Radeon RX 9060 XT (RADV): unchanged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Both directions now run a pipelined submit loop, which made the genuinely common parts visible. Rather than a second copy of each, they move to `video` alongside `SlotSync`: - `TimelineChain` replaces the hand-rolled semaphore/next/last triple in three places (encode submissions, decode submissions, reorder copies). It separates reserving a signal value from committing it, because advancing the chain for a submit that failed would leave every later submission waiting on a value nothing signals. The encoder had that ordering right and the decoder now cannot get it wrong. - `create_command_pool`, `allocate_command_buffers` and `create_fence` replace four hand-rolled copies of the same Vulkan boilerplate across both pipelines and both directions' setup. `decoder/codec.rs` had grown to 1815 lines, well past what AGENTS.md asks for, and mixed three concerns. Split along its seams, and renamed to `common` so `codec` is free for the codec trait that mirrors `encoder::codec`: - `decoder/common.rs` (563): the video session and the per-decoder state. - `decoder/frames.rs` (550): frame ownership, pool images, DPB slot pins, and display-order reordering. - `decoder/transfer.rs` (716): readback and copies, which share the property of running on the transfer queue rather than the decode queue. No behaviour change, and verified as such: byte-identical output on both GPUs, display and decode order, validation layers enabled and silent. Intel Arc A310 (ANV) and Radeon RX 9060 XT (RADV) agree with each other and with `ffmpeg -pix_fmt nv12` on all four streams. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The crate described itself as encode-only, with decoding listed as a TODO. - `lib.rs`: decoding section with a worked example, `Decode` column in the codec table, the pipelined behaviour and the zero-copy decode-order option, and the `ANV_DEBUG` note Intel Arc needs. Re-export the decoder types at the crate root next to the encoder ones. README regenerated from it. - CHANGELOG: an Unreleased section covering the decoder, the async pipeline, the validation messenger and the new `timelineSemaphore` requirement for adopted devices. - AGENTS.md: the documented `cargo readme` invocation did not reproduce the committed README (`--no-indent-headings` flattens every section to `#`), so regenerating it churned every heading. Corrected, and added how to verify a decode against ffmpeg, the `PIXELFORGE_VALIDATION` and `RUST_LOG` pairing, the warning that a missing validation layer makes silence meaningless, and the note that `testdata/test_frames.yuv` is an unfetched LFS pointer. Also log the reported decode capability flags, which is how the coincide and layered-DPB branches a device actually takes can be told apart. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
CI runners have no Vulkan driver at all, and the tests treated that as a failure: one asserted the error was not an instance-creation error, precisely the error you get with no driver. They were written to avoid passing vacuously, which is the right instinct, but a missing driver is a fact about the environment rather than something the test can act on. Distinguish the three cases explicitly: a video-capable device (the `supports_decode` contract must hold), a driver without video queues (a typed `NoSuitableDevice`), and no driver at all (skip). Match on the error variant instead of substring-matching its message, and keep the strictness available through `PIXELFORGE_REQUIRE_VULKAN=1`, which turns a skip back into a failure on a machine that is supposed to have a driver. Verified both ways by pointing the loader at a nonexistent ICD: the tests skip and pass, and fail loudly with `PIXELFORGE_REQUIRE_VULKAN=1`.
…th CI `cargo doc` runs with `-D warnings` in CI, and rustdoc rejects an explicit link target whose label already resolves to it. `OutputOrder` is re-exported at the crate root, so the label alone resolves. Also point AGENTS.md at the exact README command CI compares against (`diff --brief <(cargo readme) README.md`) rather than a variant of it, so regenerating locally cannot disagree with the check.
209025c to
3674057
Compare
|
WHEW, okay I made sure to go through and pretty much refactor the PR to be more like the encoding-side.. thank heck for LLM help on this one 😅 Not to mention, the encode-API-like pipelining does give benefits, since I got no RTX 2060 anymore I temporarily rented a cloud server with L4 for testing and verifying, also found out that Intel Arc has Vulkan Video support, but behind sneaky Results on decode speeds with pipelining API'fication:
(Arc sees almost triple speeds, whew) So basically, H.264-only since adding other codecs would bloat this way too much, but I've verified with all 3 major GPU brands and different hardware configs. Decode API is now more codec-agnostic as well to make future codec-support work less painful. @hgaiser - it's a lot, but I tried my best to test various cases and hardware 👍 |
|
Those results look promising :o Regarding the new changes, they look much better, and closer to what the encode pipeline currently does. I have my questions about the I think we might need to split the decoder in a sink and a source, since a packet from the network could, in theory, lead to N frames (where // Split is optional, but necessary for multi threaded applications.
let (mut sink, mut source) = Decoder::new(context, DecodeConfig::h264())?.split();
// Producer (optionally a thread): feed only, blocks only on backpressure
for chunk in network_or_file {
sink.decode(chunk, pts)?; // owns framing; buffers partial pictures
}
sink.finish()?; // EOF: drains reorder buffer, unblocks consumer
// Consumer (optionally a thread) : pull frames as they're ready
while let Some(frame) = source.next_frame().await? {
render(&frame); // zero-copy GPU image
// drop(frame) -> storage returned
}There are currently three ways to get decoded frames ( I would like |

PR changes
Adds decoding support, scoped to H.264 for now so there's not too much to review at once.. though I feel it already is 😅
LLM's summary:
Comment
Whew.. this has been atleast a few weeks worth of work, I ended up getting a claude subscription since I was losing my mind over H.264 codec parsing and dealing with all the reference frame management, it's just not fun and goes over my brain's capacity to handle 😓
This is very much a draft, meaning WIP, RFC and all. API could be better IMO, and I'd like some feedback and guidance on what you wish for @hgaiser !
Currently codec parsing and actual decode are done separately for API user:
incase someone wants to do their own parsing or do some magic by intercepting parsed data. When adding AV1 decoding in future, the parsing API needs changing to be more codec-agnostic though.
I've verified it works though in a real use-case, here's a picture showing pixelforge decoding integrated into a native app for playing Nestri streams, H.264 is decoded and shown perfectly fine

It's a lot to unpack, despite trying to limit the scope to H.264 decoding for now, there's a lot of boilerplate and logic needed for even just that, I apologize for the size of this PR 😅
Certain code parts are somewhat oriented towards H.26X codecs, so when adding AV1 decoding in future, it will need adjusting to be more clean and sane.
Aside from decoder changes, the encoder
pub fn new..was poked a bit to change the "b-frames unsupported" assertion into anErrreturn instead, allowing to gracefully handle that. Though I can revert that one since it's overreaching a bit here.