Repository navigation
feat(egfx): skip ZGFX compression for H.264 surface commands - #2003
Greg Lamberson (glamberson) wants to merge 3 commits into
Conversation
|
This pull request may overlap with #2001. Both replace the ZGFX compressor's Vec history and position rebasing with a fixed FixedCircularBuffer ring holding the last HISTORY_SIZE bytes, with the match table storing absolute stream positions and stale candidates skipped at lookup time. This PR additionally sends H.264 surface commands uncompressed via a new wrap_uncompressed_recorded helper, which This notice is advisory only. Automated review continues as usual, and how these pull requests relate is for maintainers and authors to decide. Note LLM-assisted content (no human feedback). |
There was a problem hiding this comment.
PR #2003 skips ZGFX match-searching for AVC420/AVC444/AVC444v2 WireToSurface1 PDUs, sending them in uncompressed segments while appending their bytes to the compressor history, and replaces the rebasing Vec history with a fixed ring keyed by absolute stream positions. Independently verified protocol conformance: MS-RDPEGFX 3.1.9.1.2 requires recording all output bytes including unencoded segments, the IronRDP Decompressor appends uncompressed segments to its history, wrap_uncompressed handles payloads over 65535 bytes via multipart segmentation, the ring's push/byte_back/holds_prefix_at/prefix_at arithmetic never reads out of window, and the find_best_match match_len < distance bound keeps reads inside the ring. Auto/Always fallback paths and Never mode preserve history parity. No correctness or wire-protocol defect found. Remaining issues are process and maintainability: the PR bundles the correctness-critical history-ring rewrite belonging to dependency #2001, the CompressionMode::A…
c3dd85f to
18bef31
Compare
There was a problem hiding this comment.
The PR sends H.264 WireToSurface1 PDUs (AVC420/AVC444/AVC444v2) uncompressed in Auto and Always modes via a new zgfx::wrap_uncompressed_recorded that wraps unencoded while recording the bytes in the compressor history, matching MS-RDPEGFX 3.1.9.1.2's requirement that every output byte enter the history. Independent verification confirms the codec-ID guard covers the H.264 paths, the new API pairs recording with wrapping, and the stacked ring-history changes keep distances within the receiver window. Two low-severity latent gaps remain, both confirmed in the head: the CompressionMode::Never arm of compress_and_wrap_egfx still wraps unencoded without recording (conformant only if a compressor is never mixed with Auto/Always), and the rewritten Auto/Always arm's compress-error fallback in drain_output also wraps unencoded without recording; today that path is unreachable because Compressor::compress constructs no Err, but the Result-typed signature leaves a silent history-desync corrupti…
Reduced coverage: optional reviewer code-compressor was unavailable.
18bef31 to
ae98f04
Compare
Once the 2.5 MB history filled, add_to_history drained the front of a Vec and rebased every match-table position on each literal and match, making every token cost O(history + table). Keep the history in a fixed ring addressed by absolute stream position, store absolute positions in the match table and skip candidates beyond the reachable window at lookup, so appends never move or rebase anything. The encoded format is unchanged.
H.264 bitstreams are already entropy coded, so searching AVC420, AVC444 and AVC444v2 WireToSurface1 PDUs for matches costs CPU on the caller's send path for almost no gain. Send them uncompressed in Auto and Always modes through the new zgfx::wrap_uncompressed_recorded, which also records their bytes in the history as MS-RDPEGFX 3.1.9.1.2 requires.
ae98f04 to
f20dd3e
Compare
There was a problem hiding this comment.
Verified the H.264 skip against pr-head: wrap_uncompressed_recorded records every output byte per MS-RDPEGFX 3.1.9.1.2, the ring-history invariants (window checks, boundary prefix indexing, distance semantics) hold, and the codec set and tests are correct. The change is protocol-conformant with no correctness defects found. Four low-severity items are published: the merge-order constraint on #2001 (refined, since the second commit does not compile without the ring, making a silent regression impossible), the forfeited cross-frame dedup of repeated H.264 payloads, the Never-mode doc gap for the new public recording path, and the duplicated history-in-step test. The byte_back inlining nit is rejected: the History-level debug assertion checks the dynamic valid window, which is strictly stronger than the ring's static capacity check, so the claimed identical debug behavior is wrong, and the code belongs to the stacked #2001 commit.
Name wrap_uncompressed_recorded in the Never warning and state its precondition, that the compressor history must hold every byte the receiver has seen. Note on with_compression that byte-identical repeats of H.264 PDUs are now sent in full. Drop a unit test that the api.rs test and the server H.264 test already cover.
Depends on #2001 (linear ZGFX history), which should merge first; until then this diff also shows its changes as the first commit, and the second commit alone is this change.
drain_output runs every PDU through the ZGFX compressor when the compression mode is Auto or Always, including WireToSurface1 PDUs that carry AVC420, AVC444 or AVC444v2. H.264 is already entropy coded, so I expect ZGFX to shrink it very little (Auto already falls back to uncompressed when compressing doesn't help, so what this saves is the CPU spent searching, not bandwidth), and that search runs on the caller's send path. The same session usually also carries ClearCodec or Planar tiles, which ZGFX shrinks a lot, so turning compression off to save the H.264 cost gives that up.
With this change those PDUs are wrapped uncompressed in Auto and Always modes and their bytes are appended to the compressor's history without searching for matches. The append is required: MS-RDPEGFX 3.1.9.1.2 says every output byte, including bytes from segments sent uncompressed, is recorded in the history, so skipping the append would make later back-references point at the wrong bytes. Never mode is unchanged.
The append is a new public function, zgfx::wrap_uncompressed_recorded, which wraps the data in an uncompressed segment and records its bytes in the ring from #2001 in one step, so the two can't be done separately; Compressor::record_uncompressed stays crate-private. The bytes are not bulk indexed, so the search is skipped, though the last two recorded bytes can still start a match because the next append indexes the positions that straddle the boundary. The with_compression and CompressionMode doc comments now say which PDUs are sent uncompressed.
A server test, run for Auto and Always, for AVC420, AVC444 and AVC444v2, and for an 8,000 and a 70,000 byte payload (the larger one is multipart), sends a ClearCodec frame, an H.264 frame and another ClearCodec frame and decompresses everything with one Decompressor. It checks that every PDU decodes, that the H.264 PDU went out uncompressed, and that both ClearCodec PDUs were compressed, the second one only decoding correctly if both sides recorded the H.264 bytes. A unit test in zgfx::api does the same for wrap_uncompressed_recorded. The xtask fmt, lints, tests, typos and locks checks pass.