test(core): add real-world progressive RemoteFX difference fixtures from Haven by GlassOnTin · Pull Request #1848 · Devolutions/IronRDP

GlassOnTin · 2026-08-30T09:19:31Z

Summary

Follow-up regression pack for #1698 (RFX_TILE_DIFFERENCE flag support in Progressive RemoteFX) requested in #1240, in the same shape as the #1813 fixture pack.

Adds four real WireToSurface2 progressive RemoteFX binary captures from a live Windows session (Windows 11 24H2 KVM) recorded via Haven's EGFX dumper (rdp-cli). Each fixture contains difference-encoded TILE_FIRST blocks (flags & RFX_TILE_DIFFERENCE == 0x01 per MS-RDPRFX 2.2.2.3.1.2 and 3.1.8.1.7.1) emitted during dynamic GUI activity.

Fixtures

Vendored at crates/ironrdp-testsuite-core/test_data/egfx/haven/ with provenance and license documentation in README.md:

wts2_64x64_diff_2tiles.bin: 2 difference tiles at (3, 2) & (3, 3) (quality = 0xFF)
wts2_64x128_diff_3tiles.bin: 3 difference tiles at (3, 2), (3, 3), (3, 4) (quality = 0xFF)
wts2_37x560_diff_column_9tiles.bin: 9-tile vertical column along x = 19, y = 3..=11 (quality = 0xFF)
wts2_progressive_tile_first_mixed_25tiles.bin: mixed set of 16 base tiles (flags = 0x00) and 9 difference tiles (flags = 0x01, quality = 0x00)

Validation

Added wire parsing, block structure, and tile assertions in crates/ironrdp-testsuite-core/tests/egfx/wire_to_surface_real_world.rs.
Added decoder validation asserting that uninitialized difference tiles correctly report ProgressiveDecodeError::MissingTileReference.
All tests in ironrdp-testsuite-core pass.

…rom Haven Adds four binary fixtures captured from a live Windows session (Windows 11 24H2 KVM) via Haven's rdp-cli, exercising the RemoteFX Progressive TILE_FIRST difference-decoding path (flags & RFX_TILE_DIFFERENCE == 1) implemented in Devolutions#1698. - wts2_64x64_diff_2tiles.bin: 2 difference tiles at full quality - wts2_64x128_diff_3tiles.bin: 3 difference tiles at full quality - wts2_37x560_diff_column_9tiles.bin: 9-tile vertical column of diff tiles - wts2_progressive_tile_first_mixed_25tiles.bin: mixed 16 base + 9 diff tiles Includes wire-parsing assertions, tile flag checks, and ProgressiveDecoder missing-reference error handling tests in wire_to_surface_real_world.rs. Refs: Devolutions#1240, Devolutions#1698, Devolutions#1813

Benoît Cortier (CBenoit) · 2026-09-03T16:04:35Z

Hi GlassOnTin

Thank you for the extra real-world captures!

Can you fix the CI checks? I think the new reviewing pipeline should kick in as soon as it’s fixed

…nings

GlassOnTin · 2026-09-03T18:07:42Z

Fixed and pushed (a085ac4). The three difference-tile assertions built the expected y_idx with i as u16, which trips cast_possible_truncation and as_conversions under the -D warnings gate; they now use u16::try_from(i).expect(...). CI is re-running, the review pipeline should pick it up once green.

Benoît Cortier (CBenoit) · 2026-09-03T18:07:52Z

The reviewer pipeline is currently broken, while I’m fixing it, I requested a Copilot review instead

Copilot

🟡 Changes recommended

Moderate test-coverage gaps must be addressed before approval.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Adds four real-world Progressive RemoteFX difference fixtures and regression coverage.

Changes:

Vendors four Windows 11 captures.
Adds wire-format and decoder tests.
Documents fixture metadata, licensing, and provenance.

File summaries

File	Review
`crates/ironrdp-testsuite-core/tests/egfx/wire_to_surface_real_world.rs`	Needs stronger fixture assertions and successful difference-update decoding coverage.
`crates/ironrdp-testsuite-core/test_data/egfx/haven/README.md`	Needs corrected region metadata and complete reproducible provenance.

Review details

Suppressed comments (1)

crates/ironrdp-testsuite-core/test_data/egfx/haven/README.md:32

WireToSurface2 has no destination rectangle, and the mixed capture does not contain the single bounding rectangle shown here. Its progressive REGION contains three rectangles: (1243, 192, 1280, 256), (80, 256, 139, 752), and (1243, 256, 1280, 752). Label these as region rectangles and record the actual list so the fixture metadata matches the wire data.

| File | PDU | Dest rect | Tiles | Tile Type & Difference Flags |

Files reviewed: 2/6 changed files
Comments generated: 3
Review effort level: Balanced

+    let diff_count = region
+        .tiles
+        .iter()
+        .filter(|t| match t {
+            ProgressiveTile::First(f) => (f.flags & TILE_FLAG_DIFFERENCE) != 0,
+            _ => false,
+        })
+        .count();
+
+    assert_eq!(diff_count, 9, "expected exactly 9 difference-encoded tiles");


+    let res = decoder.decode_bitmap(pdu.surface_id, pdu.codec_context_id, 1280, 800, &pdu.bitmap_data);
+    assert!(matches!(
+        res,
+        Err(ProgressiveDecodeError::MissingTileReference { x_idx: 3, y_idx: 2 })


+Binary fixtures captured from live Windows (Server 2025 and Windows 11 24H2 KVM) sessions via
+[Haven](https://github.com/GlassHaven/Haven)'s `rdp-cli`.
+Original captures by @GlassOnTin, recorded via Haven's EGFX PDU dumper (`EGFX_PDU_DUMP_DIR`).


github-actions

Test-only PR adding four real WireToSurface2 progressive RemoteFX captures with difference-encoded TILE_FIRST blocks, a README update, and five tests asserting PDU fields, tile indices/flags/quality, and the MissingTileReference error path. Assertions match the captures and the exercised decoder behavior already exists on the branch; no blocking defects found. All final findings are non-blocking: the new comments and README misattribute the difference-tile flag to MS-RDPRFX instead of MS-RDPEGFX, the README rewrite drops release/commit provenance for the vendored fixtures, the three difference-tile tests duplicate a shared assertion loop, the MissingTileReference init stream can shrink to a single Context block, and the mixed-fixture test leaves README-documented tile properties unchecked.

github-actions · 2026-09-04T02:00:36Z

+/// WireToSurface2 RemoteFX Progressive fixture with 2 difference-encoded `TILE_FIRST` tiles
+/// (`flags & RFX_TILE_DIFFERENCE == 1` per MS-RDPRFX 2.2.2.3.1.2). See
+/// `test_data/egfx/haven/README.md` for provenance.


[protocol] Test comments cite MS-RDPRFX sections that do not govern the difference-tile flag or missing-reference behavior — non_blocking / low: The doc comment attributes RFX_TILE_DIFFERENCE to MS-RDPRFX 2.2.2.3.1.2, and the comment at lines 217-218 claims MissingTileReference is required by MS-RDPRFX 3.1.8.1.7.1; those sections do not define the progressive tile difference flag or this error path (3.1.8.1.7.1 concerns RLGR entropy coding). The governing definitions are MS-RDPEGFX 2.2.4.2.1.5.4 (RFX_PROGRESSIVE_TILE_FIRST flags), 3.2.8.1.4 (sub-band diffing references), and 3.3.8.2.1.1 (difference-tile decoding), consistent with the crate's own progressive.rs docs citing MS-RDPEGFX 2.2.4.2. Assertions match the captures; only the citations need fixing.

github-actions · 2026-09-04T02:00:36Z

+Captures of live `WireToSurface2` PDUs carrying difference-encoded RemoteFX Progressive tiles
+(`TILE_FIRST` blocks with `flags & RFX_TILE_DIFFERENCE == 1` / `0x01` per MS-RDPRFX 2.2.2.3.1.2
+and 3.1.8.1.7.1), emitted during graphical refinement:


[protocol] Fixture README misattributes RFX_TILE_DIFFERENCE semantics to MS-RDPRFX — non_blocking / low: The new README section says the TILE_FIRST difference flag is per MS-RDPRFX 2.2.2.3.1.2 and 3.1.8.1.7.1. Those citations are not the governing source: the flag is specified for RFX_PROGRESSIVE_TILE_FIRST in MS-RDPEGFX 2.2.4.2.1.5.4, and difference-versus-reference decoding is specified in MS-RDPEGFX 3.2.8.1.4/3.3.8.2.1.1. The described fixture data matches the new tests, so only the citation needs correction.

github-actions · 2026-09-04T02:00:36Z

+Binary fixtures captured from live Windows (Server 2025 and Windows 11 24H2 KVM) sessions via
+[Haven](https://github.com/GlassHaven/Haven)'s `rdp-cli`.
+Original captures by @GlassOnTin, recorded via Haven's EGFX PDU dumper (`EGFX_PDU_DUMP_DIR`).


[general] README rewrite drops release tag and capture hashes documenting fixture provenance — non_blocking / low: The header previously tied the five pre-existing ClearCodec fixtures to a published Haven release (egfx-pr1238-fixtures-2026-04-30) with dumper/replay commits (bd2e52d7, c6cdcc49) and stated the bytes were unmodified from that release; the rewrite replaces this with a generic tool description, and the four new fixtures get no release or commit reference either. Restoring a release or commit pointer for both fixture sets keeps the vendored binaries traceable and independently verifiable.

github-actions · 2026-09-04T02:00:36Z

+/// WireToSurface2 RemoteFX Progressive fixture with 2 difference-encoded `TILE_FIRST` tiles
+/// (`flags & RFX_TILE_DIFFERENCE == 1` per MS-RDPRFX 2.2.2.3.1.2). See
+/// `test_data/egfx/haven/README.md` for provenance.
+#[test]
+fn wts2_64x64_diff_2tiles() {
+    let bytes = include_bytes!("../../test_data/egfx/haven/wts2_64x64_diff_2tiles.bin");
+    let GfxPdu::WireToSurface2(pdu) = decode(bytes) else {
+        panic!("expected WireToSurface2");
+    };
+    assert_eq!(pdu.surface_id, 0);
+    assert_eq!(pdu.codec_context_id, 18);
+    assert_eq!(pdu.codec_id, Codec2Type::RemoteFxProgressive);
+
+    let blocks = decode_progressive_stream(&pdu.bitmap_data).expect("decode progressive stream");
+    assert_eq!(blocks.len(), 3, "expected FrameBegin, Region, FrameEnd");
+
+    let ProgressiveBlock::Region(region) = &blocks[1] else {
+        panic!("expected Region block");
+    };
+    assert_eq!(region.tiles.len(), 2);
+
+    for (i, tile) in region.tiles.iter().enumerate() {
+        let ProgressiveTile::First(first) = tile else {
+            panic!("expected TileFirst at index {i}");
+        };
+        assert_ne!(
+            first.flags & TILE_FLAG_DIFFERENCE,
+            0,
+            "tile at ({}, {}) should have RFX_TILE_DIFFERENCE set",
+            first.x_idx,
+            first.y_idx
+        );
+        assert_eq!(first.quality, 0xFF);
+        assert_eq!(first.x_idx, 3);
+        assert_eq!(first.y_idx, 2 + u16::try_from(i).expect("tile index fits in u16"));
+    }
+}
+
+/// WireToSurface2 RemoteFX Progressive fixture with 3 difference-encoded `TILE_FIRST` tiles.
+#[test]
+fn wts2_64x128_diff_3tiles() {
+    let bytes = include_bytes!("../../test_data/egfx/haven/wts2_64x128_diff_3tiles.bin");
+    let GfxPdu::WireToSurface2(pdu) = decode(bytes) else {
+        panic!("expected WireToSurface2");
+    };
+    assert_eq!(pdu.surface_id, 0);
+    assert_eq!(pdu.codec_context_id, 24);
+    assert_eq!(pdu.codec_id, Codec2Type::RemoteFxProgressive);
+
+    let blocks = decode_progressive_stream(&pdu.bitmap_data).expect("decode progressive stream");
+    let ProgressiveBlock::Region(region) = &blocks[1] else {
+        panic!("expected Region block");
+    };
+    assert_eq!(region.tiles.len(), 3);
+
+    for (i, tile) in region.tiles.iter().enumerate() {
+        let ProgressiveTile::First(first) = tile else {
+            panic!("expected TileFirst at index {i}");
+        };
+        assert_ne!(
+            first.flags & TILE_FLAG_DIFFERENCE,
+            0,
+            "tile at ({}, {}) should have RFX_TILE_DIFFERENCE set",
+            first.x_idx,
+            first.y_idx
+        );
+        assert_eq!(first.quality, 0xFF);
+        assert_eq!(first.x_idx, 3);
+        assert_eq!(first.y_idx, 2 + u16::try_from(i).expect("tile index fits in u16"));
+    }
+}
+
+/// WireToSurface2 RemoteFX Progressive fixture with a 9-tile vertical strip (x=19, y=3..=11)
+/// of difference-encoded tiles.
+#[test]
+fn wts2_37x560_diff_column_9tiles() {
+    let bytes = include_bytes!("../../test_data/egfx/haven/wts2_37x560_diff_column_9tiles.bin");
+    let GfxPdu::WireToSurface2(pdu) = decode(bytes) else {
+        panic!("expected WireToSurface2");
+    };
+    assert_eq!(pdu.surface_id, 0);
+    assert_eq!(pdu.codec_context_id, 7);
+    assert_eq!(pdu.codec_id, Codec2Type::RemoteFxProgressive);
+
+    let blocks = decode_progressive_stream(&pdu.bitmap_data).expect("decode progressive stream");
+    let ProgressiveBlock::Region(region) = &blocks[1] else {
+        panic!("expected Region block");
+    };
+    assert_eq!(region.tiles.len(), 9);
+
+    for (i, tile) in region.tiles.iter().enumerate() {
+        let ProgressiveTile::First(first) = tile else {
+            panic!("expected TileFirst at index {i}");
+        };
+        assert_ne!(
+            first.flags & TILE_FLAG_DIFFERENCE,
+            0,
+            "tile at ({}, {}) should have RFX_TILE_DIFFERENCE set",
+            first.x_idx,
+            first.y_idx
+        );
+        assert_eq!(first.quality, 0xFF);
+        assert_eq!(first.x_idx, 19);
+        assert_eq!(first.y_idx, 3 + u16::try_from(i).expect("tile index fits in u16"));
+    }
+}


[code-compressor] Three difference-tile tests duplicate an identical decode-and-assert loop — non_blocking / low: wts2_64x64_diff_2tiles, wts2_64x128_diff_3tiles, and wts2_37x560_diff_column_9tiles repeat the same ~30-line body (PDU field asserts, stream decode, Region extraction, per-tile difference/quality/x_idx/y_idx checks), differing only in fixture path, context id (18/24/7), tile count (2/3/9), x index (3/3/19), and y base (2/2/3). A private helper taking those five values keeps one named test per fixture while removing roughly two thirds of the added lines, can zip y_base.. to drop the u16::try_from(i).expect boilerplate, and folding in the blocks.len() == 3 check hardens the two tests that currently just index blocks[1].

It's also possible to use rstest if applicable

Done — the three difference-fixture tests are now one rstest-parameterized test in 6802faa. Each case carries the fixture bytes, codec context id, tile count, and the tile column origin, so the per-fixture differences stay visible at the case list and the shared assertions live in one body. Net −39 lines, and the test runner still reports each case separately.

I left the rest of the file as-is: the mixed-tile and MissingTileReference tests have different assertion shapes, so they don't collapse into this parameterization.

github-actions · 2026-09-04T02:00:36Z

+    use ironrdp_pdu::codecs::rfx::RfxRectangle;
+    use ironrdp_pdu::codecs::rfx::progressive::{
+        ProgressiveBlock, ProgressiveContextPdu, ProgressiveFrameBeginPdu, ProgressiveFrameEndPdu, ProgressiveRegion,
+        ProgressiveSyncPdu, encode_progressive_stream,
+    };
+
+    let init_blocks = [
+        ProgressiveBlock::Sync(ProgressiveSyncPdu),
+        ProgressiveBlock::Context(ProgressiveContextPdu {
+            context_id: 0,
+            tile_size: 0x40,
+            flags: 0,
+        }),
+        ProgressiveBlock::FrameBegin(ProgressiveFrameBeginPdu {
+            frame_index: 0,
+            region_count: 1,
+        }),
+        ProgressiveBlock::Region(ProgressiveRegion {
+            tile_size: 0x40,
+            rects: vec![RfxRectangle {
+                x: 0,
+                y: 0,
+                width: 64,
+                height: 64,
+            }],
+            quant_vals: vec![],
+            quant_prog_vals: vec![],
+            flags: 0,
+            tiles: vec![],
+        }),
+        ProgressiveBlock::FrameEnd(ProgressiveFrameEndPdu),
+    ];
+    let init_stream = encode_progressive_stream(&init_blocks).unwrap();


[code-compressor] MissingTileReference test hand-builds a five-block init stream where one Context block suffices — non_blocking / low: decode_bitmap only needs the CONTEXT block to resolve use_reduce_extrapolate: it ignores Sync/FrameBegin/FrameEnd, and the empty Region contributes no state (no tiles; frame_tiles is empty for a fresh decoder), so encoding just ProgressiveBlock::Context { context_id: 0, tile_size: 0x40, flags: 0 } via encode_progressive_stream produces the identical MissingTileReference { x_idx: 3, y_idx: 2 } result while dropping ~20 lines and five imports. Keeping the test is worthwhile since it exercises MissingTileReference against real captured wire data.

github-actions · 2026-09-04T02:00:36Z

+    let diff_count = region
+        .tiles
+        .iter()
+        .filter(|t| match t {
+            ProgressiveTile::First(f) => (f.flags & TILE_FLAG_DIFFERENCE) != 0,
+            _ => false,
+        })
+        .count();
+
+    assert_eq!(diff_count, 9, "expected exactly 9 difference-encoded tiles");


[general] Mixed-fixture test only counts difference tiles, leaving README-documented tile properties unchecked — non_blocking / low: The README table pins the mixed fixture to 16 base tiles with flags=0x00 and 9 difference tiles with quality=0x00, but the test asserts only the 25-tile total and a difference count of 9. Asserting the base tiles' flags, the difference tiles' quality (0x00), and their coordinates would pin the fixture to the documented description; as written, drift that flips flags or qualities within the counted sets would pass while contradicting the README.

The three WireToSurface2 difference-fixture tests duplicated an identical ~30-line body, differing only in fixture, codec context id, tile count and tile column origin. Collapse them into one rstest-parameterized test. Suggested in Devolutions#1848 review.

Benoît Cortier (CBenoit)

Thank you!

…ting-session-default * upstream/master: (22 commits) ci(agents): sync shared review skills (Devolutions#1973) [DEVOPS-4678] chore(ci): drop the Artifactory cache refresh step (Devolutions#1972) feat(activex): present EGFX dirty regions (Devolutions#1874) fix(pdu): add missing error info codes (Devolutions#1966) ci(pr-automation): hand off after a clean review (Devolutions#1959) ci(pr-automation): link resolved PR in run summary (Devolutions#1960) ci(pr-automation): show automation mode in the run name (Devolutions#1958) ci(pr-automation): make overlap detection advisory (Devolutions#1957) feat(server): add ConnectionPolicy to reject a second connection (Devolutions#1913) feat(rdpeai): add RdpeaiServer orchestration for AUDIO_INPUT (Devolutions#1945) ci(pr-automation): clarify review repair failures (Devolutions#1956) ci(pr-automation): clarify workflow name and run title (Devolutions#1950) ci(pr-automation): delay maintainer handoff (Devolutions#1937) ci(pr-automation): reserve turns a repair can spend (Devolutions#1939) fix(egfx): do not track frames a suspended client cannot acknowledge (Devolutions#1912) feat(rdpeudp): MS-RDPEUDP version 1/2 reliable data transfer (Devolutions#1919) fix(viewer): map native physical keycodes (Devolutions#1929) ci(pr-automation): classify after final review (Devolutions#1936) fix(egfx): decode AVC420 with full-range BT.709 (Devolutions#1923) test(core): add real-world progressive RemoteFX difference fixtures from Haven (Devolutions#1848) ... # Conflicts: # crates/ironrdp-server/src/builder.rs # crates/ironrdp-server/src/server.rs

GlassOnTin mentioned this pull request Aug 30, 2026

Progressive RFX: RFX_TILE_DIFFERENCE flag not consumed in decode_first #1240

Closed

GlassOnTin temporarily deployed to llm-providers August 30, 2026 09:20 — with GitHub Actions Inactive

github-actions Bot added needs-review A human reviewer is the current next actor risk/unknown Risk could not be determined automatically; needs maintainer-level scrutiny size/M Size: up to 449 counted lines and 10 files; exceeds S in either measure labels Aug 30, 2026

Benoît Cortier (CBenoit) added the ai-review/allow-oversized label Sep 3, 2026

Benoît Cortier (CBenoit) temporarily deployed to llm-providers September 3, 2026 16:05 — with GitHub Actions Inactive

test(egfx): use u16::try_from for tile index to satisfy clippy -D war…

a085ac4

…nings

GlassOnTin temporarily deployed to llm-providers September 3, 2026 18:07 — with GitHub Actions Inactive

Benoît Cortier (CBenoit) requested a balanced review from Copilot September 3, 2026 18:07

Copilot started reviewing on behalf of Benoît Cortier (CBenoit) September 3, 2026 18:07

View session

Copilot AI reviewed Sep 3, 2026

View reviewed changes

Benoît Cortier (CBenoit) added ai-reviewed/1 One automated review completed and removed ai-review/allow-oversized labels Sep 3, 2026

Benoît Cortier (CBenoit) temporarily deployed to llm-providers September 4, 2026 01:32 — with GitHub Actions Inactive

github-actions Bot added kind/protocol Affects RDP or related protocol behavior risk/low Self-contained change with no cross-crate behavioral effect and removed risk/unknown Risk could not be determined automatically; needs maintainer-level scrutiny labels Sep 4, 2026

github-actions Bot reviewed Sep 4, 2026

View reviewed changes

github-actions Bot added ai-reviewed/2 Two automated reviews completed and removed ai-reviewed/1 One automated review completed labels Sep 4, 2026

github-actions Bot added risk/unknown Risk could not be determined automatically; needs maintainer-level scrutiny and removed risk/low Self-contained change with no cross-crate behavioral effect labels Sep 8, 2026

Benoît Cortier (CBenoit) approved these changes Sep 10, 2026

View reviewed changes

Benoît Cortier (CBenoit) merged commit 18445ad into Devolutions:master Sep 10, 2026
41 checks passed

Conversation

GlassOnTin commented Aug 30, 2026

Summary

Fixtures

Validation

Uh oh!

Benoît Cortier (CBenoit) commented Sep 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

GlassOnTin commented Sep 3, 2026

Uh oh!

Benoît Cortier (CBenoit) commented Sep 3, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Suppressed comments (1)

Uh oh!

github-actions Bot left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Sep 4, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Sep 4, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Sep 4, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Sep 4, 2026

Choose a reason for hiding this comment

Uh oh!

Benoît Cortier (CBenoit) Sep 4, 2026

Choose a reason for hiding this comment

Uh oh!

GlassOnTin Sep 8, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Sep 4, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Sep 4, 2026

Choose a reason for hiding this comment

Uh oh!

Benoît Cortier (CBenoit) left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

Benoît Cortier (CBenoit) commented Sep 3, 2026 •

edited

Loading