Skip to content

fix(egfx): stop the desktop from tearing when the server resizes graphics output - #1977

Open
杨成锴 (asjdf) wants to merge 7 commits into
Devolutions:masterfrom
juanjiTech:upstream-egfx-reset-fix
Open

杨成锴 (asjdf) wants to merge 7 commits into
Devolutions:masterfrom
juanjiTech:upstream-egfx-reset-fix

Conversation

@asjdf

@asjdf 杨成锴 (asjdf) commented Sep 18, 2026 •

Copy link
Copy Markdown

Fixes screen tearing / ghosting after the server changes the graphics output size.

Rebased onto current master, which already resizes the session framebuffer on ResetGraphics via take_output_reset and reset_preserving_pointer before compositor deltas are applied. This PR does not change active_stage.rs. It fixes the EGFX/graphics decode path and ironrdp-web so the pixels written into that buffer are correct.

The bug

When the server resizes the graphics output (EGFX ResetGraphics, or a Deactivation-Reactivation Sequence), stale pixels survive the transition. What you see is part of the old desktop still on screen, or blocks that never repaint until something happens to overdraw them.

Several independent faults contribute; any one of them can produce the symptom alone:

  1. ResetGraphics dropped the wrong caches. The bitmap cache was cleared, but MS-RDPEGFX 3.3.5.14 only redefines the output buffer — cache slots are connection-scoped. Windows restores the desktop after a resolution change almost entirely from slots filled before the reset, so dropping them makes hundreds of CacheToSurface blits silent no-ops.
  2. Progressive tile references outlived the surfaces they belonged to. A later surface reusing the same id would difference against the previous desktop.
  3. SRL streams were decoded incorrectly. Zero-padding was not honoured and a zero run spanning a single event was over-consumed, so a run could terminate early and leave the tail of a tile unwritten — or the session aborted with Srl(MissingTerminator) on real Windows hosts.
  4. Session framebuffer sizing (already on upstream master). Deltas for the new geometry must land in a buffer already sized for the new output. Upstream now does this with take_output_reset + reset_preserving_pointer before composite_graphics_updates. This PR assumes that base; it does not reimplement it.

On top of that, the web client never opted into MS-RDPEGFX, and its canvas never followed a size the server picked on its own — so even with the protocol side correct, ironrdp-web still tore.

Where to look first

Most of the diff is not the fix. ironrdp-stress is a 1299-line live grading harness with no product code in it, ironrdp-testsuite-core adds integration tests, and roughly a third of the remaining product diff is comment recording spec reasoning. The behavioural changes in this PR are small; the table is everything that still differs from master.

# Where The change Why that fixes the tear
1 compositor.rs::reset stop clearing the bitmap cache on reset; charge allocated_bytes to surviving slots Windows repaints a resized desktop almost entirely with CacheToSurface from slots filled before the reset. Clearing them turns hundreds of those blits into silent no-ops, so the old pixels stay where they were.
2 client.rs::handle_reset_graphics and progressive.rs::clear_tile_references clear progressive tile references on reset; keep CONTEXT and the ClearCodec glyph cache A reset implicitly destroys every surface. Stale coefficient buffers make a later surface reusing the same id difference against the previous desktop. CONTEXT stays because 3.3.5.14 redefines only the output buffer and Windows does not re-send SYNC.
3 srl.rs honour zero-padding; consume zero runs one event at a time Without this, many real servers never get past the first EGFX frames (Srl(MissingTerminator)), and static tiles never refresh when streams end early.
4 ironrdp-web EGFX enable support_dyn_vc_gfx_protocol: true and register the EGFX DVC The browser client must enter the same pipeline these fixes live in.
5 sync_canvas_to_image match the HTML canvas to DecodedImage before drawing; full repaint when the backing store size changes ActiveStage::process can resize the image in the same frame; assigning canvas width/height clears it. Dirty-rectangle painting alone would leave the rest blank.

Rows 1–3 are protocol/decode faults; rows 4–5 are web embedding. Each of 1–3 can produce visible tearing or a dead session on its own.

What changed

Protocol (ironrdp-egfx, ironrdp-graphics) — this diff

  • ResetGraphics retains the bitmap cache, keeps the progressive CONTEXT and the ClearCodec glyph cache, and clears progressive tile references.
  • Deleting a surface drops that surface's progressive tile references, so a difference tile before a new base tile fails cleanly with MissingTileReference.
  • SRL decoding honours zero-padding and consumes a zero run one event at a time; erroneous terminator and eager cap logic is gone.
  • SessionErrorKind::Pdu carries the inner error; ProgressiveDecodeError implements core::error::Error.

Session (ironrdp-session) — upstream base, not modified here

  • Framebuffer resize on reset: take_output_reset → reset_preserving_pointer → composite_graphics_updates, plus output-dimension limits via Compositor::materializable_output_size.

Web (ironrdp-web) — this diff

  • Enable the graphics pipeline and register the EGFX DVC.
  • Sync the canvas backing store to DecodedImage before the frame's GraphicsUpdate is drawn.
  • Repaint the full image when the canvas backing store actually changed; sync after reactivation when the image is replaced.
  • Canvas::resize reports whether the size changed; Session::desktop_size() tracks the size in effect.

Resize requests deliberately do not touch the canvas: it follows the size the server actually applies. Notably SuppressOutput/RefreshRect are not sent on reset — with RDPGFX those PDUs do not invalidate the surface cache (FreeRDP#12723). cargo run -p ironrdp-stress -- --redraw exists to re-check that against a real server.

Testing

Area Test
bitmap cache survives reset compositor::tests::reset_keeps_the_bitmap_cache
stale progressive tile references client::tests::reset_graphics_drops_tile_references_of_implicitly_destroyed_surfaces
reset + compositing integration session::active_stage::egfx_reset_resizes_the_image_before_compositing_the_same_payload (full ActiveStage + EGFX; fill outside the pre-reset image, inside the new one)
framebuffer + pointer on reset session::active_stage::reset_graphics_preserves_software_pointer_state (upstream; unchanged by this PR)
SRL decode progressive::tests::upgrade_pass_completes_when_the_srl_stream_ends_early, tile_upgrade_keeps_all_components_on_srl_error

Integration tests live in ironrdp-testsuite-core because ironrdp-session sets [lib] test = false.

Grading a real session

ironrdp-stress drives resolution changes against a live server and grades the decoded framebuffer (black / stale / seam metrics). Black and stale count toward failure so a fully painted but ghosted resize does not pass.

cargo run -p ironrdp-stress -- \
    --host <host> -u <user> --rounds 10 --out-dir /tmp/rdp-stress

On a Windows 11 host with RDPGFX, cycling 1300x820 ↔ 1828x1004, 5 rounds:

Build Result
this branch (on current master) 9/9 real resizes at stale_tiles=0.00%; frame size matches the requested dimensions
master without this PR opening EGFX frames abort with Srl(MissingTerminator) (fault 3), so resize grading never runs

The harness connects at the first --sizes entry, so round 1's opening step can show high stale % on an idle desktop (no server repaint) — a metric artefact, not leftover tearing.

A Display Control resize or a Ctrl+Alt+Del left the desktop torn: blocks frozen
on the previous image, regions that never refreshed again. Three separate faults,
all of them cases where the client was stricter than the protocol and stricter
than what Windows actually sends.

ResetGraphics dropped the EGFX bitmap cache. Cache slots are not surfaces: they
are connection-scoped, and MS-RDPEGFX 3.3.5.14 redefines only the output buffer,
so they survive a reset. Windows depends on it, restoring the desktop almost
entirely from slots filled before the reset. Dropping them made every one of
those CacheToSurface blits a silent no-op.

SRL decoding demanded a trailing zero byte and capped a zero run at one tile's
worth of coefficients. Windows sends neither: the encoder stops emitting bits
once the rest of a band is zero, and static regions produce runs far longer than
the cap. Both faults discarded whole tile updates, which is what surfaced as
blocks that never refresh. The stream is now read zero-padded past its end and a
run is consumed one event at a time, the way FreeRDP's progressive_rfx_srl_read
does. Over-reads are counted so a real desync still shows up in the logs.

ResetGraphics also kept the progressive difference-tile references owned by the
surfaces it implicitly destroys, so a reused surface id differenced against the
old desktop. Those go; the CONTEXT and the ClearCodec glyph cache stay, because
the server will not re-send them.

With the cache surviving, the session framebuffer has to follow the new output
before the compositor deltas from the same payload are applied — the server will
not send them twice. GraphicsPipelineClient::take_reset_graphics reports the
size for that, including same-size resets, since those still destroy every
surface.

Also implements core::error::Error for ProgressiveDecodeError and includes the
inner PDU error in SessionErrorKind::Pdu, so a decode failure is diagnosable
instead of collapsing to "PDU error".
The protocol-side fixes are only half of the tearing story for the web
client: `ironrdp-web` never opted into MS-RDPEGFX, and its canvas never
followed a size the server chose on its own.

Enable the graphics pipeline (`support_dyn_vc_gfx_protocol`) and register
the EGFX DVC, then keep the canvas in step with `DecodedImage`:

- Sync the backing store right before the frame's `GraphicsUpdate` is
  drawn, not after. `ActiveStage::process` can resize `image` to follow a
  ResetGraphics within the same frame, so a canvas synced afterwards
  would show that frame at the old size.
- Repaint the whole image whenever the resize actually happened. Setting
  `width`/`height` clears a canvas, so drawing only the frame's dirty
  regions would blank everything the server did not happen to repaint.
- Sync after a Deactivation-Reactivation Sequence too. That replaces
  `image` from inside the output loop, i.e. after this frame's sync
  already ran, and the next event can be an idle interval away.
- Make `resize` report whether the size changed, so an unchanged size
  stays a no-op instead of clearing and repainting every frame.

`Session::desktop_size()` now tracks the size in effect rather than the
one negotiated at connect, since both a reset and a reactivation change
it. Resize requests deliberately leave the canvas alone: it follows the
size the server actually applies, not the one that was asked for.
… harness that graded it

The framebuffer resize on EGFX `ResetGraphics` was the one fault in this branch with
no test behind it. Reverting it left the whole suite green, so nothing stopped a
later refactor from reordering it back into a torn desktop.

Cover it where it can actually run. `ironrdp-session` sets `[lib] test = false`, so
its inline `#[cfg(test)]` modules never execute under `cargo test --workspace`; the
test goes in `ironrdp-testsuite-core` next to the existing `composite_graphics_updates`
cases. It drives a real `ActiveStage` with an open EGFX channel and feeds one payload
carrying `ResetGraphics` plus the drawing that follows, with the fill placed outside
the old image and inside the new one — so it only survives if the resize happened
first. Reverting the fix fails it on the size assertion.

Also promote the resize-stability harness this branch was graded with from a local
script to `examples/rdp_stress.rs`. It talks to `ironrdp-session` directly over a real
connection, drives resolution changes, and grades the decoded framebuffer on black
tiles, stale tiles (the previous frame stretched over the new desktop, i.e. what
tearing looks like) and seam energy on the progressive tile grid. Stale now counts
toward failure alongside black: a resize that leaves the old picture behind is fully
painted and perfectly non-black, so grading on blackness alone reported success on
exactly the bug the harness exists to find. The settle loop and the verdict share one
predicate so they cannot drift.
Clarify SRL docs and test notes, use neutral wording in egfx/web comments, and remove reference_count_for_surface that existed only for debug fields.
@github-actions 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 scope/core Touches the core architectural tier scope/web Affects the web/WASM ecosystem size/XXL Size: 1300 or more counted lines or 50 or more files labels Sep 18, 2026
@github-actions github-actions Bot added breaking-change Includes a breaking change, and requires special scrutiny at the boundaries kind/protocol Affects RDP or related protocol behavior risk/high Substantial core public API impact, or fail-closed triage; needs maintainer-level scrutiny scope/cross-cutting Spans multiple architectural boundaries needs-review A human reviewer is the current next actor and removed risk/unknown Risk could not be determined automatically; needs maintainer-level scrutiny needs-review A human reviewer is the current next actor labels Sep 18, 2026
@github-actions

Copy link
Copy Markdown
Contributor

Automated review will not run because this contributor is not yet eligible under the automation policy.

Contributors become eligible after one qualifying IronRDP pull request is merged into master. Maintainer review is required.

@uchouT

Copy link
Copy Markdown
Contributor

Sorry for off-topic here. It's surprise to see my college alumni here :)

@meanaverage

Copy link
Copy Markdown
Contributor

Tested this against a real GNOME Remote Desktop server, in case it helps review (and #1446).

Setup: ironrdp-web built from this PR's head (50303b73) plus #2005 and #2006, embedded in a desktop app. The server is GNOME Remote Desktop 46.3 in headless mode (per-user gnome-remote-desktop-headless, gnome-shell --headless), Ubuntu 24.04, arm64. The connection goes through an RDCleanPath proxy over SSH.

What works:

  • Connect: the client advertises the graphics pipeline and grd accepts it (CapsAdvertise: Accepting capability set with version RDPGFX_CAPVERSION_8, Client cap flags: H264 (AVC444): false, H264 (AVC420): false). Frames arrive as Progressive; no H.264 is needed.
  • Live resize: a display-control monitor layout makes grd answer with ResetGraphics. The canvas follows it in the same session, larger and smaller, and the canvas-resized callback refits the view. 800×562 → 790×550 → 660×490, then 1480×1040 / 1600×1124 at 200%, with no tearing or stale regions in the decoded frames.
  • Input: keyboard and mouse reach the remote session (typed text verified on the server side).

One thing still needed for grd: #2005. grd sends AUDIO_PLAYBACK_DVC data right after its Create Request, even when the client declines the channel. Without #2005 the web session ends about a second after connecting with access to non existing DVC channel.

Tested with AI assistance; results are from automated end-to-end runs against the server.

@meanaverage

Copy link
Copy Markdown
Contributor

Tested this with GNOME Remote Desktop 46 (Ubuntu 24.04), which only accepts clients that use the graphics pipeline, so the web client can't connect to it without a change like this one.

Setup: ironrdp-web built from master with this PR merged, plus #2005, #2006, #2018, #2019 and #2020, embedded in tabby-rdp (a Tabby terminal plugin), and run through its end-to-end suites:

  • GNOME Remote Desktop 46: connects and decodes; live resize (Display Control answered with ResetGraphics) follows the host element with no stale pixels; typing, clipboard, file transfer and sound all work. All nine suites pass.
  • Windows 11: with the graphics pipeline now on for Windows as well: sign-in, frames, live resize, clipboard, sound and file transfer. Passes too.

Our own web-client patch for GNOME had enabled EGFX per connection and followed ResetGraphics in the same way; with this PR, we no longer need it. Merging it with #2020 only conflicts on adjacent struct fields in session.rs (both sides add one), which resolves by keeping both.

meanaverage (meanaverage) added a commit to meanaverage/tabby-rdp that referenced this pull request Sep 27, 2026
… #1977

- Sound in the web client is now Devolutions/IronRDP#2020, rebased onto
  master on its own, with unit tests.
- Devolutions/IronRDP#1977 (another contributor's) turns the graphics
  pipeline on for every web connection and follows ResetGraphics: with
  it and our other pull requests, all GNOME and Windows suites pass
  without our patch 5, which goes when it lands.
- The plugin sets the graphicsPipeline extension only when the IronRDP
  build has it, so it keeps working once that patch is gone.
@CBenoit

Copy link
Copy Markdown
Member

Hi 杨成锴 (@asjdf) meanaverage (@meanaverage)
Thank you for the contributions!

It seems you have some overlapping work. Can you recommend me a review order, and what you think we should focus review effort on?

@rajchauhan28

Copy link
Copy Markdown

Test report from Windows Server 2022 (Standard, build 20348.5622) as a multi-user desktop host, in case it helps with the review order. GNOME Remote Desktop and Windows 11 are covered above; this one isn't yet.

Setup. A small headless client built on ironrdp-session + ironrdp-egfx (blocking connector, like examples/screenshot.rs) with GraphicsPipelineClient and the handler's default capabilities, no H.264 decoder. The server confirms V8 { SMALL_CACHE } and sends RFX Progressive. Builds:

1. First screen, 1280×1024

Build Result
master, graphics pipeline off (16 bpp, as ironrdp-web does today) complete, 1.12–1.22 MB on the wire (2 runs)
master, graphics pipeline on session ends at the first upgrade pass: Srl(MissingTerminator)
master + #2010, or master + this PR complete, 0.32–0.50 MB on the wire (5 runs)

2. Live resize. A Display Control request after the first screen settles: 1280×1024 → 1024×768 → 1600×900. Windows answers with ResetGraphics each time, never Deactivate All. Each result is compared with a fresh connection at the same size.

Resized to master + #2010 (cache cleared on reset) master + this PR Fresh connection
1024×768 71.6% 0.0% 0.0%
1600×900 8.6% 0.0% 0.0%

On the review-order question: on Server 2022 both halves are needed. Without an SRL fix (#2010 or this PR), no graphics-pipeline session survives its first progressive upgrade pass. Without keeping the cache across ResetGraphics (#2011 or this PR), a resize with windows open leaves most of the screen unpainted. These runs don't cover this PR's ironrdp-web changes; the client was native.

We have been running an equivalent downstream patch in a browser client against Windows Server 2022 hosts since 27 September, and will drop it once this PR, or #2010 + #2011, lands.

Tested with AI assistance; the numbers come from automated runs against the server.

@asjdf

杨成锴 (asjdf) commented Sep 30, 2026 •

Copy link
Copy Markdown
Author

Hi Benoît Cortier (@CBenoit) — please merge this PR (#1977) as a single unit.

Merge order

Step PR Action
1 #1977 (this one) Review and merge whole. It is SRL + bitmap cache across ResetGraphics + progressive tile refs + ironrdp-web EGFX/canvas. Windows 11 does not get a graphics-pipeline session without the SRL half; a resize with windows open does not repaint without the cache half; ironrdp-web does not speak EGFX without the web half.
2 #2010 Close as superseded once #1977 lands. Same SRL bug, different decode (it still strips a trailing 0x00). Conflicts with this PR in srl.rs / progressive.rs.
3 #2011 Close as superseded once #1977 lands. Same bitmap-cache-across-reset fix, already in this PR’s compositor.rs.
4 Everything else Independent of this PR. #2005 (drop data for a declined DVC) is what GNOME Remote Desktop still needs after this lands; #2006 and the rest of that series can follow their own order.

Session framebuffer resize (take_output_reset / reset_preserving_pointer) is already on master; this PR does not change active_stage.rs.

Review effort: the product fix is small. Most of the diff is examples/rdp_stress.rs plus tests. Behaviour that still differs from master is the table in the PR body (compositor cache, tile refs, SRL, web EGFX, canvas sync). The SRL vs #2010 fork is the only design call; details below.

Windows 11 lab (this PR vs master)

Same grading harness (examples/rdp_stress.rs) compiled twice and linked against this branch vs master, so only the library under test changes. The host is a Windows 11 desktop with NLA/CredSSP. The RDP Negotiation Response came back with flags = 0x0f, including DYNVC_GFX_PROTOCOL_SUPPORTED (0x02), so the session really is on the graphics pipeline (RFX Progressive; we did not install an H.264 decoder).

Command shape:

cargo run --example=rdp_stress --features "session,connector,graphics,dvc,displaycontrol" -- \
    --host <host> -u <user> --sizes 1300x820,1828x1004 --rounds 5 --out-dir /tmp/rdp-stress

1300×820 and 1828×1004 are intentional: neither dimension is a multiple of 64, so every resize leaves partial tiles on the right and bottom edges. Black and stale tiles count as failure, so a fully painted but ghosted resize does not pass.

Build Result
this branch 9/9 real resizes at stale_tiles = 0.00%; when 1828×1004 is requested the decoded frame is 1828×1004
master session aborts on the opening EGFX frames with Srl(MissingTerminator), 2/2; resize grading never starts

The harness connects at the first --sizes entry, so round 1’s opening step can show a high stale % on an idle desktop (no server repaint). That is a metric artefact, not leftover tearing — we saw 95.49% on that first no-op step, then zero on every real resize.

A first smoke on the host’s then-current desktop size (so the first “resize” was a no-op) already showed the split: master died on Srl(MissingTerminator) during process frame; this branch completed the round.

On the same host we also isolated session framebuffer order (the 14 lines that are now take_output_reset + reset_preserving_pointer on master): with only that omitted, 10/10 steps failed, stale_tiles sat at 30–38%, the log filled with Dropping a compositor delta outside the image bounds … image_width=1300 image_height=820, and the PNG for a requested 1828×1004 was still 1300×820. That is why we say this PR and the session-side resize already on master are complementary, not duplicates.

On SRL: please do not keep “strip a trailing 0x00 if present”

That is the only reason not to take #2010 instead. The decoder on master still does this:

let Some((&terminator, payload)) = data.split_last() else {
    return Err(SrlError::MissingTerminator);
};
if terminator != 0 {
    return Err(SrlError::MissingTerminator);
}

Two failures, both from treating the last byte as a terminator rather than payload:

  • Last byte is not 0x00 — what this Windows 11 host sends on ordinary upgrade passes. MissingTerminator, session abort, 2/2 as in the table above. Server 2022 is the same.
  • Last byte happens to be 0x00 but is payload — those eight bits are discarded. The rest of the tile then fails as Truncated. We encoded that in treats_the_final_byte_as_payload: cutting a trailing zero used to waste its bits; decode_srl(&[0x84], …) (no terminator at all) must succeed. meanaverage (@meanaverage) independently saw the same sequence on fix(graphics)!: decode RFX Progressive SRL streams as Windows encodes them #2010: missing terminator first, then truncated after that check was removed.

MS-RDPEGFX 2.2.4.2.1.5.4 gives each component an explicit *SrlLen; 3.1.8.1.5 does not reserve the last byte. Our encoder still emits a trailing 0x00 (SrlEncoder::finish); Windows 11 does not. So the decoder has to treat every byte as payload and zero-pad past the end (same as FreeRDP BitStream_Fetch), not strip a zero when present.

The other half of this PR's srl.rs is consuming a zero run one event at a time with no 4096 cap. That is what keeps static tiles from being dropped; it is not the same as capping the run at one component.

Comment on lines +950 to +958
// `process()` may have resized `image` to follow ResetGraphics in the same
// frame. The canvas has to match *before* the GraphicsUpdate from that frame
// is drawn.
sync_canvas_to_image(
&mut gui,
&image,
&mut draw_buffer,
self.canvas_resized_callback.as_ref(),
)?;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

image may already have its new size here, but self.desktop_size is updated only when the queued GraphicsReset event is handled later. Should we update the Cell from image before this call so canvas_resized_callback sees the new size? The reactivation path seems to already be doing this in the right order.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Yes — GraphicsReset is dequeued on a later iteration than the process() that already resized image, so the callback was firing against the old Cell.

Pushed in e283f3e: the Cell is now copied from image immediately before sync_canvas_to_image, same order as the reactivation path.

Comment thread crates/ironrdp-session/src/lib.rs Outdated
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
match &self {
SessionErrorKind::Pdu(_) => write!(f, "PDU error"),
SessionErrorKind::Pdu(e) => write!(f, "PDU error: {e}"),

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

suggestion: Revert this, this is against the error conventions. See STYLE.md for explanations.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Reverted. SessionErrorKind::Pdu Display is "PDU error" again; the inner error stays on source(), matching Encode/Decode and STYLE.md.

@CBenoit Benoît Cortier (CBenoit) left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thank you both for the explainer.

I’ll start by reviewing and landing this PR.

Here are some comments from me.

ResetGraphics resizes DecodedImage in the same process() as the first
GraphicsUpdate, but GraphicsReset is dequeued on a later iteration.
Copy the image size into the Cell first so canvas_resized_callback and
desktop_size() agree. Also restore SessionErrorKind::Pdu Display to the
STYLE.md convention (inner error stays on source()).
@github-actions github-actions Bot added triage/overlap Possible overlap with another pull request; advisory only and removed needs-review A human reviewer is the current next actor labels Sep 30, 2026
@github-actions

Copy link
Copy Markdown
Contributor

This pull request may overlap with #2010.

Both touch crates/ironrdp-graphics/src/srl.rs and progressive.rs to decode Windows-encoded RFX Progressive SRL streams: no required trailing zero byte, zero-padded reads past the stream end, and zero runs longer than the encoder bound, with matching test renames.

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 automation-failed Exact-head automated classification or review failed or was unavailable label 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.

The PR's product changes (SRL zero-padding decoder, keeping the EGFX bitmap cache across ResetGraphics, clearing progressive tile references on reset, web-client EGFX enablement with canvas resize sync) are narrowly targeted and verified against the head tree; no new correctness, protocol, or safety defect was found. All published candidates are low-severity maintainability/documentation items: a spec-basis gap in the reset tile-reference comment, two pieces of test-only public API added to the released ironrdp-graphics crate (overread_bits, total_reference_count), redundant GraphicsReset event plumbing in ironrdp-web, duplicated Haven test scaffolding, and the placement of the 1,299-line rdp_stress harness in the facade crate's examples (published as an open question).

  1. [code-compressor] GraphicsReset event path duplicates the image-driven desktop_size/canvas sync — low 🟡 — crates/ironrdp-web/src/session.rs
    Verified in head: on_reset_graphics only fires when output_size is Some, and in that same ActiveStage::process call the image is already resized from take_output_reset; the post-outputs block (session.rs ~950-965) then sets self.desktop_size from image and calls sync_canvas_to_image before the queued GraphicsReset event is ever dequeued. The entire RdpInputEvent::GraphicsReset variant, its match arm (which only converts through u16 and can miss clamped sizes the image already reflects), the EgfxHandler struct, and the input_events_tx field threaded through ConnectParams/connect can be deleted — keeping at most a debug! in a no-op handler — with behavior preserved, removing one enum variant, one channel message type, and two struct fields of redundant plumbing.
  2. [code-compressor] Two new Haven tests duplicate the CONTEXT-only init and decode-expect-failure flow — low 🟡 — crates/ironrdp-testsuite-core/tests/egfx/wire_to_surface_real_world.rs
    haven_wts2_cold_decode_reproduces_missing_tile_reference and haven_wts2_mixed_25tiles_cold_decode_hits_missing_tile_reference each re-encode the identical ~25-line Sync/Context/FrameBegin/Region/FrameEnd init stream and repeat the same decode/match/panic/expect-failure shape; only the fixture, decode dimensions, and assertion precision differ. Extracting a context_only_init() helper and a cold_decode_error(decoder, pdu, w, h) helper (or a second #[case] on the existing rstest with an any-MissingTileReference expectation) collapses the second test to a few lines and removes drift risk as the init stream evolves. Behavior-preserving, test-only.

Comment on lines +705 to +710
// Tile coefficient buffers belong to those surfaces. Keeping them lets a
// later surface reuse the same id and difference against the previous
// desktop, which decodes as torn or duplicated tiles.
// CONTEXT / ClearCodec glyph cache stay: 3.3.5.14 only redefines the
// output buffer, and Windows will not re-send SYNC + CONTEXT.
self.progressive_decoder.clear_tile_references();

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.

[protocol] Tile references dropped on ResetGraphics rests on a surface-destruction rule the cited sections do not contain — low 🟡 — The new clear_tile_references call cites MS-RDPEGFX 2.2.2.14 and 3.3.5.14 as requiring that ResetGraphics destroys every surface, but in the corpus 3.3.5.14 only mandates resizing the graphics output buffer and 2.2.2.14 describes PDU fields; 3.3.1.3 instead requires sub-band diffing tile contexts to be preserved for the connection or until the associated surface is deleted. A spec-strict server that retains surfaces and reuses a surface id with difference tiles now gets MissingTileReference instead of preserved state. The behavior is safer than the previous silent cross-reset differencing and matches observed Windows, but the normative justification is absent and the protocol-visible error path for conformant-but-different servers is undocumented; the comment should be corrected or the caveat noted.

Comment on lines +134 to +140
/// Bits read past the end of the stream, i.e. how much of the tail was assumed to be zero.
///
/// A handful at the very end is normal (the encoder stops once the rest of a band is zero).
/// A large count means the decoder and the encoder disagree about the stream layout.
pub fn overread_bits(&self) -> u32 {
self.reader.overread_bits
}

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.

[skeptical + code-compressor] overread_bits instrumentation has no production reader despite its stated purpose — low 🟡 — The PR removes the only malformed-stream signals (MissingTerminator, Truncated) and makes the decoder silently zero-pad past the stream end; the replacement diagnostic overread_bits() is documented as keeping desyncs 'visible in the logs', but a repository search shows no caller outside its own unit test (counts_bits_read_past_the_end) — neither the progressive decode path nor any client logs the counter. As merged it is write-only state plus permanent public API surface on the released ironrdp-graphics crate. Either wire the count into a debug/warn at the decode call site or drop the field and accessor.

Comment on lines +1561 to +1565
/// Total retained difference-tile coefficient buffers across all surfaces.
#[must_use]
pub fn total_reference_count(&self) -> usize {
self.references.len()
}

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.

[skeptical + code-compressor] Public total_reference_count added to a released crate solely for one cross-crate test — low 🟡 — total_reference_count is a #[must_use] pub method on the production ProgressiveDecoder whose only callers are the two assertions in ironrdp-egfx's reset_graphics_drops_tile_references_of_implicitly_destroyed_surfaces test. The decoder-level guarantee can be asserted inside ironrdp-graphics's own tests (which see the private references field), and the egfx test can assert the observable outcome instead — a following difference tile failing with MissingTileReference, the failure class its doc comment already cites. As merged it adds permanent public API surface for test convenience.

Comment thread crates/ironrdp/examples/rdp_stress.rs Outdated
Comment on lines +1 to +40
//! Standalone RDP resize-stability stress harness.
//!
//! Connects straight to an RDP server over TCP + TLS/CredSSP, negotiates EGFX and
//! Display Control, then drives resolution changes and key injection while grading
//! the decoded framebuffer. No browser and no WASM: everything here talks to
//! `ironrdp-session` directly, so a failure points at the protocol/decode path
//! rather than at a client's canvas plumbing.
//!
//! Three metrics, all computed locally so they cannot be fooled by the code under test:
//!
//! - black tiles: how much of the picture is missing outright.
//! - stale tiles: how much of the picture is still the previous frame stretched over the
//! new desktop size, i.e. content the server never repainted after ResetGraphics.
//! This is what "torn"/"ghosted" looks like on screen.
//! - seam score: edge energy on the 64-pixel RemoteFX tile grid relative to tile
//! interiors. Mismatched tiles show up as a hard grid.
//!
//! # Usage example
//!
//! ```shell
//! cargo run --example=rdp_stress --features "session,connector,graphics,dvc,displaycontrol" -- \
//! --host rdp.example.com -u Administrator --rounds 10 --out-dir /tmp/rdp-stress
//! ```
//!
//! The password is read from `--password` or, preferably, the `RDP_PASSWORD` env var.

#![allow(unused_crate_dependencies)] // false positives because there is both a library and a binary
#![allow(clippy::print_stdout)]
// The grading code is percentage arithmetic over tile and pixel counts: every value is a
// small count or a 0..=100 ratio, so f32 has room to spare and a lost fraction of a
// percent cannot change a verdict.
#![allow(
clippy::as_conversions,
clippy::cast_precision_loss,
clippy::cast_possible_truncation
)]

use core::sync::atomic::{AtomicU32, Ordering};
use core::time::Duration;
use std::io::Write as _;

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.

[skeptical] 1,300-line interactive stress harness added to the facade crate's examples — low 🟡 ❓ — The example bundles a complete RDP client (CredSSP via sspi, TLS, keyboard injection, RDP_PASSWORD handling) plus a frame-grading engine into crates/ironrdp/examples, whose existing examples are small demos, while the repository keeps interactive tooling in dedicated crates (ironrdp-replay-client, ironrdp-capture-replay, ironrdp-bench). It is feature-gated with doc-scrape-examples=false and the PR body justifies it as the grading harness that produced the Windows 11 evidence, so this is a placement and long-term maintenance-surface question rather than a correctness defect; whether maintainers want a standalone tooling crate cannot be settled from the repository alone.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yeah, could be a dedicated crate at this point. 杨成锴 (@asjdf) do you think that could be a useful tool in general?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Agreed — it is useful beyond this PR as a live graphics-pipeline / Display Control grader (the class of bug unit tests cannot see), but it is not a small library example.

Moved in 77d05a6 to an unpublished ironrdp-stress crate, next to ironrdp-capture-replay / ironrdp-bench. Run with:

cargo run -p ironrdp-stress -- --host <host> -u <user> --rounds 10 --out-dir /tmp/rdp-stress

It still needs a real host, so it stays out of cargo test.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I would not call it a helper tool. It is a live integration test for the graphics pipeline: it drives Display Control against a real host and scores the framebuffer after ResetGraphics. That needs a real machine, so it stays an unpublished crate and out of cargo test.

@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
@meanaverage

Copy link
Copy Markdown
Contributor

Thanks Benoît Cortier (@CBenoit), and sorry for the slow reply. I see you've started on #1977, and that's the order I'd suggest too. My PRs don't touch its files, except #2020 (below).

On the SRL question 杨成锴 (@asjdf) raised: we fixed the same Windows 11 failure in our own build before #1977 came up, and ended up with the same decoding. The last byte is data, and zero-run code words are read one at a time, as values need them. So we'd take #1977's version over stripping a trailing zero.

After #1977, in this order:

  1. fix(dvc): drop data for a channel that is not open instead of failing #2005 (dvc, +47/−6): the one change GNOME Remote Desktop still needs after fix(egfx): stop the desktop from tearing when the server resizes graphics output #1977. grd sends AUDIO_PLAYBACK_DVC data on the channel the client declined, and master still ends the session over it about a second after connecting. It's the data-path counterpart of fix(dvc): switch a Soft-Sync tunnel that also lists declined channels #2007.
  2. fix(rdpsnd): echo the Training PDU's wPackSize in the Training Confirm #2019 (rdpsnd): makes the Training Confirm wPackSize follow the spec. Independent.
  3. fix(web): fit the canvas to the host element, not the window #2006 (web-client): fits the canvas to the host element instead of the window. Svelte only, independent.
  4. feat(web): audio playback (RDPSND) through an audioPlayback callback #2020 (web audio, +256/−3): sound from GNOME also needs fix(rdpsnd): echo the Training PDU's wPackSize in the Training Confirm #2019, but Windows works without it. It conflicts with fix(egfx): stop the desktop from tearing when the server resizes graphics output #1977, and with feat(web): expose enable_standard_rdp_security #2004, on adjacent lines in session.rs. I'll rebase onto whichever lands first.

#2018 is merged, thanks. Please leave #2026 out of the queue for now; I'll answer your design questions on that PR first.

Where to spend review effort: #2020. It's the only one that adds public API: an audioPlayback extension in iron-remote-desktop-rdp, plus the shape of its events. It also attaches a device-less RDPDR when audio is on and no printer is redirected, because Windows only starts playback once RDPDR is up. #2005, #2019 and #2006 are small, and each comes with a test that fails on master. I've addressed the automated review notes on #2019 and #2006.

examples/ is for small library demos. This is a live-host analysis tool in
the same class as ironrdp-capture-replay, so it belongs in its own
unpublished crate rather than the facade examples.
@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/high Substantial core public API impact, or fail-closed triage; needs maintainer-level scrutiny labels Oct 3, 2026

This branch was successfully deployed

1 active deployment
llm-providers — 77d05a64 Deployed Oct 3, 2026 by asjdf via Classify pull request #1593
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ai-reviewed/1 One automated review completed automation-failed Exact-head automated classification or review failed or was unavailable breaking-change Includes a breaking change, and requires special scrutiny at the boundaries kind/protocol Affects RDP or related protocol behavior risk/unknown Risk could not be determined automatically; needs maintainer-level scrutiny scope/core Touches the core architectural tier scope/cross-cutting Spans multiple architectural boundaries scope/web Affects the web/WASM ecosystem size/XXL Size: 1300 or more counted lines or 50 or more files triage/overlap Possible overlap with another pull request; advisory only

Development

Successfully merging this pull request may close these issues.

5 participants