fix(egfx): decode AVC420 sent as an Annex B byte stream - #1986
Willie Abrams (willie) wants to merge 2 commits into
Conversation
MS-RDPEGFX defines the RFX_AVC420_BITMAP_STREAM bitstream as an Annex B byte stream, but OpenH264Decoder always read its input as 4-byte length-prefixed NAL units. Given Annex B, the first start code (00 00 00 01) was read as a 1-byte NAL and the following bytes as a bogus length, so the frame was dropped. GNOME Remote Desktop sends Annex B, so no AVC420 frame from it ever decoded. The decoder now converts only input that is a complete chain of length-prefixed NAL units ending exactly at the end of the buffer, and passes everything else to OpenH264 unchanged. Docs that said the wire format is length-prefixed are corrected, and the dead spec link is replaced. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
GitHub Actions
Active
github-actions
Bot
added
needs-review
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The format detector can misclassify and corrupt valid Annex B streams; start codes must take precedence and receive regression coverage.
Get a fresh assessment by requesting another Copilot review.
Review effort: Balanced
Findings: 1
Open (1)
What changed in this PR
Adds support for spec-compliant Annex B AVC420 streams while retaining legacy length-prefixed AVC compatibility.
Changes:
- Adds conditional format detection and conversion.
- Adds Annex B decoding tests.
- Corrects protocol and fuzzing documentation.
| File | Review |
|---|---|
crates/ironrdp-testsuite-core/tests/egfx/decode.rs |
Adds Annex B, AUD, and three-byte start-code tests; needs a framing-collision regression test. |
crates/ironrdp-fuzzing/src/oracles/mod.rs |
Updates fuzz-oracle comments, but two descriptions incorrectly claim production format-dispatch coverage. (2 nits) |
crates/ironrdp-egfx/src/pdu/avc.rs |
Corrects AVC420 wire-format documentation. |
crates/ironrdp-egfx/src/encode.rs |
Updates round-trip test documentation. |
crates/ironrdp-egfx/src/decode.rs |
Implements dual-format decoding, but AVC framing incorrectly takes precedence over valid Annex B start codes and can corrupt colliding streams. (Critical) |
|
Thanks for tracking this down, and for the careful write-up. You're right, and the mistake was mine: I wrote that decoder and the docs saying the wire format is length-prefixed. MS-RDPEGFX 2.2.4.4 says the opposite, twice: Both the RFX_AVC420_BITMAP_STREAM description and the avc420EncodedBitstream field are defined as an Annex B byte stream. I checked the branch locally. The 15 EGFX decode tests pass, and with the decoder forced back to the old always-length-prefixed behavior, exactly your three new Annex B tests fail. The complete-chain check is the right call over a start-code check for the reason you give; a downstream client of mine uses a start-code check and has the same ambiguity. One optional suggestion: The detection loop could live as a small public function next to avc_to_annex_b in pdu/avc.rs. Other H264Decoder implementations could then use the same check instead of writing their own, and the egfx_avc420_decode fuzz oracle could exercise it, since at the moment it only reaches the conversion. Thanks again. |
|
A correction to my comment above. The Copilot finding on decode.rs, which landed a few minutes before I posted, is right, and it cuts against what I said about the complete-chain check. An Annex B frame that is a single NAL unit behind a 3-byte start code reads as a valid one-unit length-prefixed chain whenever its size happens to line up: 00 00 01 67 ... at exactly 363 bytes, or a P slice 00 00 01 41 ... at exactly 325 bytes. Small single-slice P frames are what an idle screen produces, so this can happen in practice, and a corrupted P frame damages every frame after it until the next IDR. Since MS-RDPEGFX 2.2.4.4 makes Annex B the wire format, I think a recognized start code should win: Treat a buffer that begins with 00 00 00 01 or 00 00 01 as Annex B, and fall back to the chain check only when there is no start code. The remaining ambiguity then lands on length-prefixed senders whose first NAL unit is 256 to 511 bytes long, which is the non-conforming side. A regression test for the collision case, as Copilot suggests, would pin it down. |
A buffer that begins with 00 00 01 can also be a well-formed AVC buffer: 00 00 01 67 followed by 359 bytes reads as one 359-byte NAL unit. The chain check took that as AVC format and corrupted the frame. Since MS-RDPEGFX specifies Annex B, a leading start code now wins, and the chain check applies only to buffers without one. The check moves to a public `pdu::is_avc_format` next to `avc_to_annex_b`, so other H264Decoder implementations can share it, and the egfx_avc420_decode fuzz oracle now makes the same dispatch as OpenH264Decoder. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
GitHub Actions
Active
github-actions
Bot
added
kind/protocol
|
Automated review will not run because this contributor is not yet eligible under the automation policy. Contributors become eligible after one qualifying IronRDP pull request is merged into |
|
Test report from Windows Server 2022 (Standard, build 20348.5622, no GPU), native client, graphics pipeline. With the handler's default capabilities (V8.1 with AVC420, then V8), Server 2022 confirms V8.1 with AVC420 cleared and never sends H.264. Offering V10.x with AVC allowed gets V10.6, and full-screen motion (Chrome on testufo.com) then produces AVC420. Every Windows frame is an Annex B byte stream with 4-byte start codes, and each one begins with an access unit delimiter:
Results:
On the heuristic concern above: a Windows frame can't pass the complete-chain check, because the delimiter always makes the second "length" The frames decode, but they are then drawn in the wrong place, which is #2042. I've put the numbers there. Tested with AI assistance; the byte values and counts come from logging in an automated run against the server. |
|
Raj singh chauhan (@rajchauhan28) thanks for the test! |
AVC420 partial frame updates paint the wrong pixels. On the client decode path, `decode_avc420` decoded the `Avc420BitmapStream` but only ever used `stream.data` — the `regionRects` in `stream.rectangles` (the `RFX_AVC420_METABLOCK` from [MS-RDPEGFX] 2.2.4.4) were never read. `crop_decoded_frame` then copied a single block from the decoded frame's origin `(0,0)` and blitted it across the whole destination rectangle. So whenever a frame's changed regions did not happen to sit at the top-left corner, each region was filled with pixels lifted from `(0,0)`, and the space between regions got overwritten. That is the "horizontal lines and unfilled rectangle outlines" people see in GFX / H.264 (AVC420) mode once the server starts sending partial updates instead of full frames. The fix follows the spec: `regionRects` are the sub-regions that actually changed, each one takes its pixels from the *same* `(x, y)` in the decoded frame, and the PDU's destination rectangle is just their bounding box — not a copy source. `decode_avc420` now walks `stream.rectangles`, clips each rect to `surface ∩ frame`, copies `(x, y) → (x, y)` through a small new `copy_frame_region` helper, and emits one surface update per rectangle. A stream that carries no `regionRects` keeps the old single bounding-box path, so nothing changes for that case. The change is deliberately narrow — only the client decode path in `crates/ironrdp-egfx/src/client.rs`. AVC444, the server encode path, and the NAL-format work in #1986 are all left alone. For the test, a deterministic stand-in decoder stamps every pixel with its own coordinate (`R = x`, `G = y`), so a test can tell which source pixel actually landed where. It sends an AVC420 update with two disjoint regions, neither at the origin, whose bounding box is the destination rectangle, and checks that each region is drawn from its own coordinates. Before the fix it fails with a single origin-cropped update where two were expected; after the fix it gets one update per region, with region B at `(32, 32)` carrying its own pixels. `cargo test -p ironrdp-egfx` passes (54 tests) and `cargo clippy -p ironrdp-egfx --all-targets` is clean. This one is a co-fix. The root-cause analysis and the region-rects harness/recipe (`c06_avc420_region_rects.rs`) came from the issue author, @se-wo; this PR implements that recipe on the client decode path and turns the harness into an in-tree unit test. Glad to fold in @se-wo's original harness verbatim or adjust the attribution however maintainers prefer. Fixes #2042
There's no need; I'm using this project and quite enjoy how good this is and the potential it has. I am running tens of VMs on a Proxmox VE server with ample testing VMs, so if you need, I can help with other things too, like testing and stuff also. (This was out of the PR chain, but I wanted to say it just in case.) |
|
Greg Lamberson (@glamberson) both of your points are in 8fac24c: a leading 3- or 4-byte start code now means Annex B, and the length-prefix chain check only runs when there is none. The check is a public |
Problem
OpenH264Decoder::decodealways reads its input as AVC format (4-byte big-endian length-prefixed NAL units) and converts it to Annex B before handing it to OpenH264.MS-RDPEGFX defines the
RFX_AVC420_BITMAP_STREAMbitstream as "conforming to the byte stream format specified in [ITU-H.264-201201] Annex B". Servers that follow the spec send start codes, not lengths.When the decoder gets Annex B, the first start code
00 00 00 01is read as a NAL length of 1. The bytes after it are then read as the next length, which runs past the buffer, and the frame is dropped:GNOME Remote Desktop 50.2 and Windows Server 2022 both start every frame with an access unit delimiter (
00 00 00 01 09 30 00 00 00 01 ...), so no AVC420 frame from either one decodes.805306368is0x30000000, the bytes after the delimiter.Fix
A new public function,
pdu::is_avc_format, decides the format.OpenH264Decoder::decodeuses it:00 00 01or00 00 00 01): Annex B. Passed to OpenH264 unchanged.avc_to_annex_b_intoas before.The start code is checked first because the chain check alone misreads valid Annex B.
00 00 01 67read as a length is 359, so a 363-byte frame that starts with a 3-byte start code and an SPS also parses as a one-unit AVC buffer. The same happens to a 325-byte P slice (00 00 01 41), and small single-slice P frames are what an idle screen produces.Checking the start code first leaves one ambiguity: AVC senders whose first NAL unit is 1 byte or 256–511 bytes long are read as Annex B. Those senders don't follow the spec.
is_avc_formatsits next toavc_to_annex_binpdu/avc.rsso otherH264Decoderimplementations can use it, and theegfx_avc420_decodefuzz oracle runs it.Behavior changes to review
H264Decodertrait docs now say implementations may receive either format. Third-party decoders written against the old docs may only handle AVC input; they already fail against spec-conforming servers.ironrdp_egfx::pdu::is_avc_format.Docs
The docs that said the wire format is length-prefixed are corrected:
decode.rsmodule and trait docs,avc_to_annex_b,annex_b_to_avc,encode_avc420_bitmap_stream, the encoder round-trip test comment, and the two EGFX fuzz oracle comments. The spec link indecode.rsreturned 404 and now points to theRFX_AVC420_BITMAP_STREAMpage.encode.rsalready said the encoder produces Annex B, "the formatRFX_AVC420_BITMAP_STREAMcarries on the wire", and the glutin renderer (crates/ironrdp-glutin-renderer/src/surface.rs) already passes wire bytes straight to OpenH264. This change makes the decoder agree with both.Tests
Added to
crates/ironrdp-testsuite-core/tests/egfx/decode.rs:test_openh264_decode_annex_bmastertest_openh264_decode_annex_b_with_access_unit_delimitermastertest_openh264_decode_annex_b_three_byte_start_codesmastertest_openh264_decode_annex_b_that_also_parses_as_avc00 00 01 67frame that also parses as AVCmaster, and a chain-first checktest_is_avc_formatThe existing AVC tests pass unchanged. Three error-path test comments were updated to describe the new path.
Run locally on 8fac24c:
cargo xtask check fmt,cargo xtask check lintstyposon the changed cratescargo test -p ironrdp-testsuite-core --features openh264-bundled egfx: 83 passedcargo test -p ironrdp-egfx --features openh264-bundled: 55 passedTested against servers
ironrdp-egfx0.3.0 decodes AVC420 frames. Without it, none decode.🤖 Generated with Claude Code