fix(egfx): keep no Progressive tile state for surfaces the compositor refused by willie · Pull Request #2133 · Devolutions/IronRDP

Willie Abrams (willie) · 2026-10-16T03:38:31Z

The client decoded RFX Progressive into surfaces the compositor refused. Nothing was painted, and their tile state grew outside the 256 MiB budget.

Those updates are now only parsed for the CONTEXT flag (ProgressiveDecoder::skip_bitmap), so a surface re-created after a reset still decodes the CONTEXT-less updates Windows sends. The first parseable skipped update also discards the tile state the surface had before it was refused.

Behavior changes:

A refused surface gets no Progressive on_bitmap_updated. Other codecs still call it.
An unparseable update to a refused surface is dropped and counted. Other errors in it go undetected.
A DIFFERENCE tile after a skipped update fails with MissingTileReference instead of using coefficients from before the refusal.
On master, re-creating a surface at over 16384 px leaves the old pixels in the compositor, so its updates are still decoded. feat(egfx)!: decode AVC444v2 #2112 (open) fixes that.

Copilot

🟡 Changes recommended

Skipped updates retain existing coefficients, allowing incorrect DIFFERENCE decoding when a surface is accepted again.

1 open finding

Clear stale progressive state when recreating budget-refused surfaces · New

What changed in this PR

Prevents Progressive decoding for surfaces refused by the compositor while retaining CONTEXT flags for later surface reuse.

Changes:

Adds a parse-only path for skipped Progressive updates.
Gates decoding on compositor surface availability.
Adds tests for skipped decoding and CONTEXT retention.

File	Description
crates/ironrdp-graphics/src/progressive.rs	Adds `skip_bitmap`, shared flag extraction, and tests.
crates/ironrdp-egfx/src/compositor.rs	Exposes whether a surface has pixels.
crates/ironrdp-egfx/src/client.rs	Skips refused-surface decoding and tests flag retention.

🧠 Review effort: Balanced

Willie Abrams (willie) · 2026-10-16T12:41:57Z

Checked against #2117. Merged together, they conflict in two places in progressive.rs, both lines next to each other, and one test here reads decoder.contexts, which #2117 renames to surfaces. With those fixed, the ironrdp-graphics and ironrdp-egfx tests pass. The changes don't interact: a refused surface keeps no tile state either way. If #2117 merges first, I'll rebase this one.

Marc-André Moreau (mamoreau-devolutions) · 2026-10-27T02:16:11Z

Willie Abrams (@willie) you can rebase and resolve conflicts

… refused A surface the compositor refuses (too large, or past its budget) is still tracked by the client, and WireToSurface2 decoded into it. Nothing was painted, but its tile state stayed until the surface was deleted, outside the compositor's budget. Those updates are now only parsed for their CONTEXT flag, through the new ProgressiveDecoder::skip_bitmap, so a surface re-created with the same id after a reset still decodes the CONTEXT-less updates Windows sends. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

github-actions

PR #2133 stops decoding RFX Progressive updates for surfaces the egfx compositor refused (e.g. 16384x16384 past the budget): a has_surface guard skips the decode, ProgressiveDecoder::skip_bitmap parses only for the CONTEXT band-layout flag while discarding the surface's tile state, and tests cover the skip, drop, and re-create-with-flag paths. Independently verified against head: the guard is correctly placed after the known-surface check, the retained-flag fallback matches decode_bitmap's documented behavior, the skipped teardown matches delete_surface's, and the diff's changed progressive_tile_stream call site compiles because head's helper (line 2769) already takes (u16, u16, RfxRectangle). No correctness, protocol-state, or memory-safety defects found. Remaining published findings are test-quality gaps (no // oracle: markers; the headline no-on_bitmap_updated behavior is never asserted; the tile-discard consequence is untested at the public seam), a maintainability duplication bet…

[skeptical] Supplied diff is internally inconsistent at the progressive_tile_stream call site; head itself is self-consistent — low 🟡 — crates/ironrdp-egfx/src/client.rs
The diff rewrites the seed call to progressive_tile_stream(0, 0, rect(0, 0, 64, 64)) with no helper-signature hunk, yet the base removal line uses four scalars. Head inspection resolves it: the helper at line 2769 takes (u16, u16, RfxRectangle) and every call site (2888, 3129, 3237, 3353, 3417) uses the rect form, so the PR code compiles and is coherent. The base-side line could not have compiled against that signature, indicating the supplied diff for client.rs is truncated or synthesized rather than the PR being wrong. No change to the PR is needed; only note the evidence gap for reviewers relying on the diff alone.

Push a commit after addressing these findings. If no code change is needed, you may resolve inline threads and comment @github-actions review-ready to request human review.

github-actions · 2026-10-27T14:40:39Z

+        if !self.compositor.has_surface(pdu.surface_id) {
+            debug!(
+                surface_id = pdu.surface_id,
+                "WireToSurface2 for a surface without pixels -- not decoded"
+            );
+            if let Err(error) = self.progressive_decoder.skip_bitmap(pdu.surface_id, &pdu.bitmap_data) {
+                self.drop_update(pdu.codec_id, pdu.surface_id, pdu.bitmap_data.len(), &error);
+            }
+            return Ok(());
+        }


[protocol] Silent skip of WireToSurface2 rendering deviates from the client's MUST to progressively render a valid offscreen surface — low 🟡 — Per MS-RDPEGFX 3.3.5.2 the client MUST create a codec context and begin progressively rendering a valid offscreen surface, and 3.3.5.9 requires the bitmap to exist in the Offscreen Surface ADM element. Here the surface exists in the client registry but the compositor refused its allocation, a condition the spec does not model, so the new path neither renders nor creates progressive state while frame acknowledgements continue. The deviation is deliberate, has no wire-visible effect, and is a reasonable mitigation for a spec-unmodeled local limit, but it departs from MUST-level processing rules and should be documented as such.

Not changed: the comment above the check states the local reason, and the departure has no wire-visible effect.

🤖 Addressed by Claude Code

github-actions

The fix correctly stops decoding Progressive updates for compositor-refused surfaces: the WireToSurface2 guard is Progressive-only, skip_bitmap preserves the CONTEXT flag a re-created surface id needs, and the drop path mirrors existing error handling. Published findings cover the MS-RDPEGFX deviation of discarding tile state while the surface remains in the Offscreen ADM, the duplicated per-surface discard shared with delete_surface, stale tile state surviving unparseable skipped streams, four test-oracle gaps (non-decoding unasserted, flag value unchecked, missing // oracle: markers, no other-codec regression pin), and a debug!/trace! level mismatch. The other-codecs finding was refined: the invariant is structurally guaranteed because the guard sits only in the Progressive-specific WireToSurface2 handler.

Push a commit after addressing these findings. If no code change is needed, you may resolve inline threads and comment @github-actions review-ready to request human review.

github-actions · 2026-10-27T15:28:43Z

+    pub fn skip_bitmap(&mut self, surface_id: u16, bitmap_data: &[u8]) -> Result<(), ProgressiveDecodeError> {
+        let blocks = ironrdp_pdu::codecs::rfx::progressive::decode_progressive_stream(bitmap_data)?;
+        self.surfaces.remove(&surface_id);
+        self.references
+            .retain(|(reference_surface_id, _, _), _| *reference_surface_id != surface_id);
+        self.frame_tiles.remove(&surface_id);
+        if let Some(flag) = signalled_context_flag(&blocks) {
+            self.surface_context_flags.insert(surface_id, flag);
+        }
+        Ok(())
+    }


[protocol] skip_bitmap discards Progressive tile state of a surface that is still live in the Offscreen ADM — medium 🟠 — A compositor-refused surface stays in the client's surface map (handle_create_surface keeps it; no DeleteSurface arrives), so MS-RDPEGFX 3.3.5.2 requires progressively rendering its bitmapData and 3.3.1.3/3.1.8.1.4 require preserving sub-band diffing tile contexts until deletion. skip_bitmap instead removes the surface's tile grid and references while the surface is live, so later sub-band diffed or upgrading streams fail (the PR's own test shows MissingTileReference after a skip) and are dropped. The spec offers no way to decline an accepted surface, so this is a deliberate policy deviation; visual output is unaffected because the surface never paints. A previously accepted surface re-created over budget also silently loses decoded state without a DeleteSurface.

Not changed: refusing the surface already departs from rendering it, and keeping tile state for a surface that never paints is the unbounded growth this PR removes.

🤖 Addressed by Claude Code

github-actions · 2026-10-27T15:28:43Z

+    pub fn skip_bitmap(&mut self, surface_id: u16, bitmap_data: &[u8]) -> Result<(), ProgressiveDecodeError> {
+        let blocks = ironrdp_pdu::codecs::rfx::progressive::decode_progressive_stream(bitmap_data)?;
+        self.surfaces.remove(&surface_id);
+        self.references
+            .retain(|(reference_surface_id, _, _), _| *reference_surface_id != surface_id);
+        self.frame_tiles.remove(&surface_id);
+        if let Some(flag) = signalled_context_flag(&blocks) {
+            self.surface_context_flags.insert(surface_id, flag);
+        }
+        Ok(())
+    }


[skeptical] An unparseable skipped stream leaves stale tile state on a re-created refused surface — low 🟡 — skip_bitmap parses the stream first and returns Err before any state removal, so an unparseable update discards nothing. Since handle_create_surface never clears Progressive state when a surface id is re-created, a surface that decoded while accepted and was re-created over budget keeps its prior tile state indefinitely if the server only sends unparseable updates; only a parseable stream or DeleteSurface clears it. The retention is bounded and matches pre-PR behavior, but it narrows the PR body's claim that the first skipped update discards earlier tile state.

Not changed: the unparseable update is dropped, the state left is bounded and matches master, and the next parseable update clears it.

🤖 Addressed by Claude Code

github-actions · 2026-10-27T15:28:43Z

+        // A surface the compositor refused paints nothing, so it keeps no
+        // Progressive tile state either, only the CONTEXT flag a re-created
+        // surface of the same id falls back on.
+        if !self.compositor.has_surface(pdu.surface_id) {
+            debug!(
+                surface_id = pdu.surface_id,
+                "WireToSurface2 for a surface without pixels -- not decoded"
+            );
+            if let Err(error) = self.progressive_decoder.skip_bitmap(pdu.surface_id, &pdu.bitmap_data) {
+                self.drop_update(pdu.codec_id, pdu.surface_id, pdu.bitmap_data.len(), &error);
+            }
+            return Ok(());
+        }


[test] No regression test pins that non-Progressive codecs still decode on a refused surface — low 🟡 — Refined: the original premise that the has_surface guard sits where every codec passes is incorrect -- it lives only in handle_wire_to_surface2, which per MS-RDPEGFX carries RemoteFX Progressive, while Planar, ClearCodec, RemoteFx, AVC, and Uncompressed dispatch in handle_wire_to_surface1 (lines 1090-1114), untouched by this PR. The invariant that other codecs still decode on a refused surface is therefore structurally guaranteed today. A small example test over progressive_client_with_refused_surface would still pin it against a future refactor that extends the guard.

- Log the skip at trace!, since it repeats for every update to the surface. - Reuse delete_surface in skip_bitmap, keeping the CONTEXT flag. - Assert a refused surface gets no bitmap update, and mark each new test's oracle. - Check the CONTEXT flag value a skipped stream records, for both band layouts. - Keep one sentence per comment line. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

github-actions

Independent review confirms the fix is sound: RDPGFX WireToSurface2 payloads for surfaces the compositor refused are no longer decoded into unbounded Progressive tile state; ProgressiveDecoder::skip_bitmap parses only for the CONTEXT flag, reuses delete_surface to discard stale tile/sub-band state, and re-inserts the flag so a re-created surface still decodes CONTEXT-less streams. The compositor accessor is minimal and the client drop path reuses existing accounting. All five specialist candidates were verified against pr-head; four are published (three accepted, one refined), one is rejected because MS-RDPEGFX 2.2.4.2 governs CONTEXT non-repetition within a live context and does not decide the cross-re-creation retention this test covers, so the mutant marker is apt. Remaining items are low severity: a telemetry-only divergence in the dropped-update counter, an unnamed test-parameterization case, an import-hoisting nit, and a doc-comment fragment.

Push a commit after addressing these findings. If no code change is needed, you may resolve inline threads and comment @github-actions review-ready to request human review.

github-actions · 2026-10-27T16:16:34Z

+/// The band-layout flag a stream's CONTEXT block sets, if it has one.
+fn signalled_context_flag(blocks: &[ironrdp_pdu::codecs::rfx::progressive::ProgressiveBlock<'_>]) -> Option<bool> {
+    blocks.iter().find_map(|block| match block {
+        ironrdp_pdu::codecs::rfx::progressive::ProgressiveBlock::Context(ctx) => Some(ctx.uses_reduce_extrapolate()),
+        _ => None,
+    })
+}


[code-compressor] Hoist ProgressiveBlock and decode_progressive_stream into the existing top-level import — low 🟡 — The new helper signalled_context_flag spells out the full ironrdp_pdu::codecs::rfx::progressive:: path twice and skip_bitmap spells it for decode_progressive_stream, while decode_bitmap already imports exactly these two items via a function-local use. Moving both into the existing top-level use and deleting the local use removes the repetition with identical behavior; purely textual.

Not changed: hoisting ProgressiveBlock makes the function-local imports in two existing tests (progressive.rs 1791 and 3347) redundant, which fails clippy's -D warnings, so the change would spread into tests this PR doesn't touch.

🤖 Addressed by Claude Code

Also make the signalled_context_flag doc a sentence. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Willie Abrams (willie) marked this pull request as ready for review October 16, 2026 12:18

Copilot AI balanced review requested due to automatic review settings October 16, 2026 12:18

Copilot started reviewing on behalf of Willie Abrams (willie) October 16, 2026 12:19

View session

Willie Abrams (willie) temporarily deployed to llm-providers October 16, 2026 12:19 — with GitHub Actions Inactive

Copilot AI reviewed Oct 16, 2026

View reviewed changes

Comment thread crates/ironrdp-graphics/src/progressive.rs Resolved

Willie Abrams (willie) force-pushed the fix/progressive-refused-surface branch from cd1055c to fac13ec Compare October 27, 2026 13:55

Willie Abrams (willie) temporarily deployed to llm-providers October 27, 2026 13:55 — with GitHub Actions Inactive

github-actions Bot reviewed Oct 27, 2026

View reviewed changes

github-actions Bot added ai-reviewed/1 One automated review completed needs-author-action The pull request author is the current next actor labels Oct 27, 2026

Willie Abrams (willie) force-pushed the fix/progressive-refused-surface branch from fac13ec to 485883f Compare October 27, 2026 14:41

github-actions Bot removed the needs-author-action The pull request author is the current next actor label Oct 27, 2026

Willie Abrams (willie) temporarily deployed to llm-providers October 27, 2026 14:43 — with GitHub Actions Inactive

github-actions Bot added the scope/cross-cutting Spans multiple architectural boundaries label Oct 27, 2026

github-actions Bot reviewed Oct 27, 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 Oct 27, 2026

github-actions Bot added the needs-author-action The pull request author is the current next actor label Oct 27, 2026

Willie Abrams (willie) temporarily deployed to llm-providers October 27, 2026 15:38 — with GitHub Actions Inactive

github-actions Bot removed needs-author-action The pull request author is the current next actor scope/cross-cutting Spans multiple architectural boundaries labels Oct 27, 2026

github-actions Bot reviewed Oct 27, 2026

View reviewed changes

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 27, 2026

test(graphics): name the CONTEXT flag case on failure

079f216

Also make the signalled_context_flag doc a sentence. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Willie Abrams (willie) temporarily deployed to llm-providers October 27, 2026 16:46 — with GitHub Actions Inactive

github-actions Bot added needs-review A human reviewer is the current next actor and removed needs-author-action The pull request author is the current next actor labels Oct 27, 2026

github-actions Bot mentioned this pull request Oct 29, 2026

fix(graphics): read the DWT variant from REGION flags #2085

Merged

Conversation

Willie Abrams (willie) commented Oct 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Uh oh!

Willie Abrams (willie) commented Oct 16, 2026

Uh oh!

Marc-André Moreau (mamoreau-devolutions) commented Oct 27, 2026

Uh oh!

github-actions Bot left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Oct 27, 2026

Choose a reason for hiding this comment

Uh oh!

Willie Abrams (willie) Oct 27, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions Bot left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Oct 27, 2026

Choose a reason for hiding this comment

Uh oh!

Willie Abrams (willie) Oct 27, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

github-actions Bot Oct 27, 2026

Choose a reason for hiding this comment

Uh oh!

Willie Abrams (willie) Oct 27, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions Bot Oct 27, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

github-actions Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

github-actions Bot Oct 27, 2026

Choose a reason for hiding this comment

Uh oh!

Willie Abrams (willie) Oct 27, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

This branch was previously deployed

Reviewers

Uh oh!

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

Willie Abrams (willie) commented Oct 16, 2026 •

edited

Loading