Skip to content

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

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

Greg Lamberson (glamberson) wants to merge 2 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

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. The cost is that a byte-identical repeat of an earlier H.264 PDU, such as a periodic keyframe of an unchanged screen, is now sent in full where ZGFX would have matched it, and the with_compression doc says so.

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 needs-review A human reviewer is the current next actor labels 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 removed the needs-author-action The pull request author is the current next actor label Oct 8, 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 risk/medium Behavioral change that does not substantially alter a core public API labels Oct 8, 2026
@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
@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
Marc-André Moreau (mamoreau-devolutions) pushed a commit that referenced this pull request Oct 9, 2026
…2001)

The ZGFX compressor keeps its history in a Vec and, once the history
reaches its 2.5 MB limit, add_to_history drains the front of the Vec and
walks every entry of the match table to rebase and prune positions.
add_to_history runs once per literal byte and once per match, so after
the history fills, every emitted token costs a copy of the whole window
plus a pass over the table. With ClearCodec output from a 1280x800
desktop fed through one Compressor, the first three frames took 10 to 51
ms each, and the next three took 0.38, 4.9 and 6.8 seconds. #1344
bounded the hash table, which is a different limit, and this shows up on
any long session that uses CompressionMode::Auto or Always. The
compressor came from my own #1097.

This keeps the history as a fixed ring of HISTORY_SIZE bytes addressed
by absolute stream position, the same layout the decompressor already
uses with FixedCircularBuffer. Appending is a copy into the ring, the
match table stores absolute positions, and a candidate is skipped when
it has fallen out of the window or is further back than
MAX_MATCH_DISTANCE, so nothing has to be rebased. The encoded output
format is unchanged, and so is the rule in MS-RDPEGFX 3.1.9.1.2 that
every output byte, including those of segments sent uncompressed, is
recorded in the history.

With the same input the output is the same size on every frame, and the
frames after the history fills now take 9 to 15 ms each.

A new test compresses more than twice HISTORY_SIZE through one
Compressor, in segments that repeat earlier content, and decompresses
every segment with one Decompressor, which covers matches that reach
across the wrap point. A second test checks the ring directly, including
a push larger than the ring. The xtask fmt, lints, tests, typos and
locks checks pass.

#2003 builds on this: it sends H.264 surface commands uncompressed and
records their bytes in the ring.
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.
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 scope/cross-cutting Spans multiple architectural boundaries size/M Size: up to 449 counted lines and 10 files; exceeds S in either measure needs-review A human reviewer is the current next actor and removed needs-review A human reviewer is the current next actor 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 labels Oct 9, 2026

This branch was successfully deployed

1 active deployment
llm-providers — 0672f955 Deployed Oct 9, 2026 by glamberson via Classify pull request #2291
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 scope/cross-cutting Spans multiple architectural boundaries size/M Size: up to 449 counted lines and 10 files; exceeds S in either measure

Development

Successfully merging this pull request may close these issues.

2 participants