feat(egfx)!: switch WireToSurface1Pdu rectangles to exclusive bounds#1238
feat(egfx)!: switch WireToSurface1Pdu rectangles to exclusive bounds#1238Greg Lamberson (glamberson) wants to merge 1 commit intoDevolutions:masterfrom
Conversation
MS-RDPEGFX 2.2.1.4.1 RDPGFX_RECT16 specifies the right and bottom fields as exclusive (one-past-end), matching FreeRDP's reference implementation. The struct previously used InclusiveRectangle, which interprets the same bytes as inclusive bounds and yields width/height that are one larger than the wire format intends. The mismatch was visible against modern Windows servers when ClearCodec tiles arrived: a 64x64 tile encoded as right=64, left=0 was being decoded as 65 wide. This commit changes WireToSurface1Pdu.destination_rectangle and BitmapUpdate.destination_rectangle to ExclusiveRectangle, updates the server-side construction sites (compute_dest_rect now adds 1 to convert from Avc420Region's inclusive bounds, send_uncompressed_frame drops the saturating_sub(1) workaround), and updates test fixtures to use exclusive coordinates. This is a breaking change to the ironrdp-egfx public API: handler implementations of GraphicsPipelineHandler::on_bitmap_updated will see ExclusiveRectangle instead of InclusiveRectangle, and any direct construction of WireToSurface1Pdu must use exclusive bounds. The wire format on the network is unchanged: this commit only fixes how the parsed bytes are interpreted in Rust types. Other RDPGFX_RECT16 uses in cmd.rs (SolidFill, SurfaceToSurface, SurfaceToCache, CacheToSurface) remain InclusiveRectangle for now and will follow as a separate PR.
Sister-env-var to EGFX_DUMP_DIR (which writes rendered PPM frames). This one writes the post-zgfx-decompressed byte slice for every ServerPdu the EGFX channel decodes, plus the legacy slow-path BitmapUpdateData payload, into <dir>/pdu_NNNN_<kind>.bin. Motivated by upstream IronRDP PR Devolutions/IronRDP#1238 — the maintainer asked for Server 2025 captures to validate the RDPGFX_RECT16 inclusive→exclusive type flip on WireToSurface1Pdu and BitmapUpdate. The dump is exactly what the parser saw, so feeding the bytes back through `<ServerPdu as Decode>::decode` is a deterministic reproduction the upstream test plan can pin against. pdu_kind_label is exhaustive (no `_` arm) so a future ironrdp version adding a new ServerPdu variant fails the build and forces us to extend the filename mapping.
Companion to the dumper added in bd2e52d. Reads a captured .bin (or batches a whole directory) and prints a one-line summary per file: ServerPdu variant, surface ids, codec ids, payload sizes — plus, for WireToSurface1, both the inclusive and the exclusive interpretation of width/height from the destination_rectangle. That dual print makes attached fixtures self-document what Devolutions/IronRDP#1238 is fixing: spec-correct bytes have right=W (exclusive), but today's ironrdp parses them as InclusiveRectangle and the trait method's width() returns W+1. Auto-detects mode from filename (slow_path_bitmap_update_*.bin → BitmapUpdateData, else ServerPdu); --egfx and --slow-path force a mode. Exit codes: 0 = all decoded, 1 = none decoded, 2 = mixed. Useful as a CI gate before sending captures upstream. Build with `cargo build --features host-cli --bin replay-egfx-pdu`. Codec1Type / Codec2Type matches are exhaustive so a future ironrdp codec addition fails the build here and forces an update.
|
Greg Lamberson (@glamberson) — Server 2025 capture confirms the exclusive interpretation, both against common tile sizes and against the codec's own internal dimension headers. Fixtures attached as binary assets on a small fixture-only release on the Haven side: https://github.com/GlassHaven/Haven/releases/tag/egfx-pr1238-fixtures-2026-04-30. Each file is the post-zgfx-decompressed bytes of a single
The 576×128 file is the strongest standalone reproducer: the same PDU's ClearCodec NSCodec sub-region header parses 576×128 internally — the codec's own metadata says 576×128, and the destination_rectangle's exclusive width agrees with that, while inclusive disagrees by one. So the PDU is internally self-consistent with the spec-correct exclusive interpretation; today's ironrdp parses the same bytes inconsistently. Across all 31 One scope note for the test plan: the dumper records what the parser sees, i.e. post-zgfx bytes. So these fixtures exercise everything from Happy to capture more (different codecs, larger tiles, AVC420) if useful. |
|
GlassOnTin Thanks for the find and assistance in confirmation. I'll review further when I get home. |
3 participants
Summary
Spotted by GlassOnTin during review of #1175 against Haven's downstream EGFX implementation and Windows Server 2025 captures: MS-RDPEGFX 2.2.1.4.1 RDPGFX_RECT16 specifies
rightandbottomas exclusive (one-past-end), butWireToSurface1Pdu.destination_rectanglewas typed asInclusiveRectangle, which interprets the same wire bytes as inclusive bounds. The result was a one-pixel discrepancy in width/height computed via the trait method, which surfaces on tiles where a precise dimension match matters (ClearCodec glyph cache hits, AVC420 macroblock alignment).The numerical fix landed in #1175 by computing
right - leftdirectly at the call sites. This PR is the type-level fix that makes the wire-format and the Rust type agree, so the trait methodRectangle::width()returns the spec-correct value without inline workarounds.Changes
WireToSurface1Pdu.destination_rectangle: ExclusiveRectangle(wasInclusiveRectangle)BitmapUpdate.destination_rectangle: ExclusiveRectangle(wasInclusiveRectangle) — public API forGraphicsPipelineHandler::on_bitmap_updatedconsumerscompute_dest_rectreturnsExclusiveRectangle, converts fromAvc420Region's inclusive bounds with+1on right and bottomsend_uncompressed_framedrops thedest_width.saturating_sub(1)/dest_height.saturating_sub(1)workaroundsright=3, bottom=3, nowright=4, bottom=4)Breaking change
Marked with
!in the conventional commit. The break is in two places:WireToSurface1Pdu: callers must now use exclusive bounds (right = width, notright = width - 1).GraphicsPipelineHandler::on_bitmap_updated: theBitmapUpdate.destination_rectanglefield type changes. The wire format on the network does not change; only the Rust interpretation does.Pre-1.0, and consistent with the project's stance on breaking unreleased API for correctness (per #1209 discussion). No crates.io consumers are affected today since
ironrdp-egfxhas not yet published.Out of scope
The other RDPGFX_RECT16 uses in
crates/ironrdp-egfx/src/pdu/cmd.rs(SolidFillPdu.rectangles,SurfaceToSurfacePdu.source_rectangle,SurfaceToCachePdu.source_rectangle,CacheToSurfacePdu.destinations) have the same wire-format mismatch and stayInclusiveRectanglein this PR. Each warrants its own audit and breaking-change announcement; will submit separate PRs for consideration of these additional spec-compliant breaking changes.The
Avc420Regionstruct stays inclusive (it is documented that way and is consumer-facing).compute_dest_rectconverts at the boundary.Test plan
cargo xtask check fmt -vcleancargo xtask check lints -vclean (workspace, all-targets, with helper + __bench features)cargo xtask check tests -vpasses (all egfx integration tests, including the ClearCodec / AVC420 / uncompressed paths)