Skip to content

feat(egfx): skip ZGFX compression for H.264 surface commands - #2003

Open
Greg Lamberson (glamberson) wants to merge 3 commits into
Devolutions:masterfrom
lamco-admin:feat/egfx-skip-zgfx-for-h264
Open

Greg Lamberson (glamberson) wants to merge 3 commits into
Devolutions:masterfrom
lamco-admin:feat/egfx-skip-zgfx-for-h264

Conversation

@glamberson

@glamberson Greg Lamberson (glamberson) commented Sep 25, 2026 •

Copy link
Copy Markdown
Contributor

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.

@github-actions github-actions Bot added kind/protocol Affects RDP or related protocol behavior risk/high Substantial core public API impact, or fail-closed triage; needs maintainer-level scrutiny scope/core Touches the core architectural tier scope/cross-cutting Spans multiple architectural boundaries size/M Size: up to 449 counted lines and 10 files; exceeds S in either measure triage/overlap Possible overlap with another pull request; advisory only labels Sep 25, 2026
@github-actions

github-actions Bot commented Sep 25, 2026 •

Copy link
Copy Markdown
Contributor

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 #2001 does not mention.

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).

@github-actions github-actions Bot added the needs-review A human reviewer is the current next actor label Sep 25, 2026
@CBenoit Benoît Cortier (CBenoit) added automation-failed Exact-head automated classification or review failed or was unavailable and removed needs-review A human reviewer is the current next actor labels Sep 30, 2026

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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…

Comment thread crates/ironrdp-graphics/src/zgfx/compressor.rs
Comment thread crates/ironrdp-egfx/src/server.rs
Comment thread crates/ironrdp-graphics/src/zgfx/compressor.rs Outdated
Comment thread crates/ironrdp-graphics/src/zgfx/compressor.rs Outdated
Comment thread crates/ironrdp-graphics/src/zgfx/compressor.rs Outdated
@github-actions github-actions Bot added ai-reviewed/1 One automated review completed needs-author-action The pull request author is the current next actor and removed automation-failed Exact-head automated classification or review failed or was unavailable labels Sep 30, 2026
@github-actions github-actions Bot added risk/medium Behavioral change that does not substantially alter a core public API size/L Size: up to 899 counted lines and 20 files; exceeds M in either measure and removed needs-author-action The pull request author is the current next actor risk/high Substantial core public API impact, or fail-closed triage; needs maintainer-level scrutiny size/M Size: up to 449 counted lines and 10 files; exceeds S in either measure scope/cross-cutting Spans multiple architectural boundaries labels Oct 7, 2026

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread crates/ironrdp-graphics/src/zgfx/api.rs
Comment thread crates/ironrdp-egfx/src/server.rs
@github-actions github-actions Bot added ai-reviewed/2 Two automated reviews completed needs-author-action The pull request author is the current next actor and removed ai-reviewed/1 One automated review completed labels Oct 7, 2026
@github-actions github-actions Bot added automation-failed Exact-head automated classification or review failed or was unavailable risk/unknown Risk could not be determined automatically; needs maintainer-level scrutiny and removed needs-author-action The pull request author is the current next actor risk/medium Behavioral change that does not substantially alter a core public API labels Oct 8, 2026
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.
@github-actions github-actions Bot added risk/medium Behavioral change that does not substantially alter a core public API and removed risk/unknown Risk could not be determined automatically; needs maintainer-level scrutiny automation-failed Exact-head automated classification or review failed or was unavailable labels Oct 8, 2026

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread crates/ironrdp-graphics/src/zgfx/compressor.rs
Comment thread crates/ironrdp-egfx/src/server.rs
Comment thread crates/ironrdp-graphics/src/zgfx/api.rs
Comment thread crates/ironrdp-graphics/src/zgfx/compressor.rs Outdated
@github-actions github-actions Bot added ai-reviewed/3 Final automated review completed needs-author-action The pull request author is the current next actor and removed ai-reviewed/2 Two automated reviews completed labels Oct 8, 2026
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.
@github-actions github-actions Bot added risk/high Substantial core public API impact, or fail-closed triage; needs maintainer-level scrutiny needs-review A human reviewer is the current next actor and removed needs-author-action The pull request author is the current next actor risk/medium Behavioral change that does not substantially alter a core public API labels Oct 9, 2026

This branch was successfully deployed

1 active deployment
llm-providers — d22725e6 Deployed Oct 9, 2026 by glamberson via Classify pull request #2207
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ai-reviewed/3 Final automated review completed kind/protocol Affects RDP or related protocol behavior needs-review A human reviewer is the current next actor risk/high Substantial core public API impact, or fail-closed triage; needs maintainer-level scrutiny scope/core Touches the core architectural tier size/L Size: up to 899 counted lines and 20 files; exceeds M in either measure triage/overlap Possible overlap with another pull request; advisory only

Development

Successfully merging this pull request may close these issues.

2 participants