Skip to content

feat(egfx): wire RemoteFX Progressive decode into WireToSurface2 dispatch - #1443

Open
truebest wants to merge 5 commits into
Devolutions:masterfrom
truebest:feat/egfx-progressive-client-decode
Open

feat(egfx): wire RemoteFX Progressive decode into WireToSurface2 dispatch#1443
truebest wants to merge 5 commits into
Devolutions:masterfrom
truebest:feat/egfx-progressive-client-decode

Conversation

@truebest

@truebest truebest commented Jul 13, 2026

Copy link
Copy Markdown
Contributor

Problem

GraphicsPipelineClient::handle_pdu currently only forwards WireToSurface2 (the RemoteFX Progressive codec) to handler.on_wire_to_surface2() and returns, without decoding it. This differs from the AVC420 path (decode_avc420) and from ClearCodec's WireToSurface1 dispatch (#1175), both of which decode into BitmapUpdate callbacks through on_bitmap_updated.

The ProgressiveDecoder itself already exists (landed via #1197) — this PR just wires it into the client dispatch, the way #1175 did for ClearCodec.

Discussed with Greg Lamberson (@glamberson) on #1158 first to avoid duplicating in-flight Lamco Development work; the client-side dispatch turned out to have fallen off their TODO after #1197 landed, so this fills that gap.

Changes

  • GraphicsPipelineClient gains a progressive_decoder: ProgressiveDecoder field.
  • WireToSurface2 now decodes through it and emits each updated 64x64 tile as a BitmapUpdate via the existing on_bitmap_updated path.
  • A decode failure now propagates as a terminal PduResult error instead of being logged and silently dropped — matching decode_avc420's behavior. Previously this would leave the session running with no further bitmap updates for the surface and no visible error.
  • ResetGraphics and DeleteEncodingContext now clear the decoder's per-context tile state, since both destroy the codec_context_id(s) scoped to them and a later reused id must not decode against stale tiles.

Testing

3 new integration tests: a malformed/first-frame-without-CONTEXT stream propagates an error; ResetGraphics clears context state (verified via a REGION-only continuation failing afterward); DeleteEncodingContext does the same for its specific context.

cargo test -p ironrdp-egfx: 17/17 passing. cargo fmt/cargo clippy --all-targets -- -D warnings: clean.

Found and fixed while running IronRDP against gnome-remote-desktop as an RDP server, which streams RemoteFX Progressive when AVC420 is unavailable.

Copilot AI review requested due to automatic review settings July 13, 2026 03:38

Copilot AI 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.

Pull request overview

This PR wires the existing RemoteFX Progressive (WireToSurface2) decode path into GraphicsPipelineClient so progressive tiles are decoded client-side and emitted via the existing on_bitmap_updated RGBA callback path, matching the AVC420/ClearCodec dispatch model. It also ensures progressive per-context state is cleared on ResetGraphics and DeleteEncodingContext, and adds tests to verify error propagation and state clearing.

Changes:

  • Add a ProgressiveDecoder field to GraphicsPipelineClient and decode WireToSurface2 into BitmapUpdate callbacks.
  • Clear progressive decoder context state on ResetGraphics and DeleteEncodingContext.
  • Add integration tests validating decode-error propagation and context reset semantics.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread crates/ironrdp-egfx/src/client.rs Outdated
Comment on lines +744 to +747
let Some(surface) = self.surfaces.get(&pdu.surface_id) else {
warn!(surface_id = pdu.surface_id, "WireToSurface2 for unknown surface");
return Ok(());
};

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 8d71776 — now returns pdu_other_err!("unknown surface in WireToSurface2"), matching handle_wire_to_surface1.

Comment on lines +757 to +760
Err(e) => {
warn!(error = ?e, "RFX progressive decode failed");
return Err(pdu_other_err!("RFX progressive decode failed"));
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 8d71776 — lowercased to "rfx progressive decode failed".

Comment on lines +763 to +780
for tile in tiles {
let left = tile.x_idx.saturating_mul(64);
let top = tile.y_idx.saturating_mul(64);
let update = BitmapUpdate {
surface_id: pdu.surface_id,
destination_rectangle: ExclusiveRectangle {
left,
top,
right: left.saturating_add(64),
bottom: top.saturating_add(64),
},
codec_id: Codec1Type::Uncompressed,
data: tile.pixels,
width: 64,
height: 64,
};
self.handler.on_bitmap_updated(&update);
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 8d71776 — each tile's rectangle/width/height is now clamped to the surface bounds and the RGBA buffer cropped via the existing crop_decoded_frame helper for edge tiles.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 4b60ea0081

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "Codex (@codex) review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "Codex (@codex) address that feedback".

Comment thread crates/ironrdp-egfx/src/client.rs Outdated
Comment on lines +768 to +772
destination_rectangle: ExclusiveRectangle {
left,
top,
right: left.saturating_add(64),
bottom: top.saturating_add(64),

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Clip progressive edge tiles to surface bounds

For surfaces whose dimensions are not multiples of 64, the rightmost or bottommost progressive tile is still decoded as a padded 64x64 tile, but this reports the update rectangle as left + 64/top + 64. For example, a 100px-wide surface receiving tile x_idx = 1 produces right = 128, violating BitmapUpdate's “within the surface” contract and causing renderers that trust the rectangle to blit past their surface buffer or reject the update. Clip the rectangle/width/height to the surface dimensions and crop the tile data for edge tiles.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 8d71776 — same fix as the other clipping comment on this thread: rectangle/width/height clamped to surface bounds, buffer cropped via crop_decoded_frame.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 8d71776435

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "Codex (@codex) review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "Codex (@codex) address that feedback".

let (surface_width, surface_height) = (surface.width, surface.height);

let tiles = match self.progressive_decoder.decode_bitmap(
pdu.codec_context_id,

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Scope progressive contexts by surface

When a server reuses the same codec_context_id on two live surfaces, this stores both progressive streams in the same decoder context because the surface id is dropped here. WireToSurface2Pdu and DeleteEncodingContextPdu both carry surface_id, so a context is associated with a surface; with the current key, decoding the second surface can reallocate/overwrite the first surface's tile state, and deleting one surface's context removes the other. Key the progressive state by (surface_id, codec_context_id) and delete the matching surface-scoped context.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed in d51fe30 — decode_bitmap()/delete_context() now also take surface_id and key contexts by (surface_id, codec_context_id). Added decoder_contexts_scoped_by_surface covering two surfaces reusing the same codec_context_id.

truebest added a commit to truebest/IronRDP that referenced this pull request Jul 13, 2026
…ntext scoping

Port the three issues found in code review of the upstream PR
(Devolutions#1443) to our own vendored copy, since this fork
already carries the same Progressive/WireToSurface2 client integration:

- handle_wire_to_surface2 now returns a terminal error for an unknown
  surface instead of silently dropping the update, matching
  handle_wire_to_surface1.
- Progressive tiles at the surface edge (dimensions not a multiple of 64,
  the common case) are now clipped to the surface bounds instead of
  claiming a BitmapUpdate rectangle that extends past it.
- ProgressiveDecoder now scopes context state by (surface_id,
  codec_context_id) instead of codec_context_id alone -- per MS-RDPEGFX,
  the id is scoped to its owning surface (RDPGFX_DELETE_ENCODING_CONTEXT_PDU
  carries both together), so two surfaces reusing the same id no longer
  collide.
@glamberson

Copy link
Copy Markdown
Contributor

Benoît Cortier (@CBenoit) worth pulling this into a slot when you can: it's a priority fix for us and the last codec gap on the graphics-pipeline client.

I went through it against gnome-remote-desktop, which sends the desktop as RemoteFX Progressive over WireToSurface2, exactly the path this wires up. grd's live Progressive stream decoded cleanly here with no decode failures, so this closes the grd gap from #1158 and it's the last piece the EGFX client needs to render a real grd session.

The code reads correctly and the automated review points all look addressed:

  • The (surface_id, codec_context_id) keying is the right model. codecContextId is scoped to its surface per MS-RDPEGFX (RDPGFX_DELETE_ENCODING_CONTEXT_PDU carries both), so keying on the id alone could alias two surfaces that legitimately reuse the same value. The added test covers it.
  • Propagating a decode failure as a terminal error, matching decode_avc420, is the right behavior. Silently dropping left the surface frozen with no signal, which is harder to diagnose than a clean failure.
  • Edge-tile clamping to the surface bounds and the context reset on ResetGraphics / DeleteEncodingContext are both correct.

Shape matches the ClearCodec dispatch from #1175. One process note: since this is truebest's first PR, the workflows are still gated, so it needs a maintainer approval to run CI. LGTM from my side.

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

LGTM!

I’ll see if I can rebase and fix the conflicts on my side

EDIT: I may be running out of time, this will go to Monday, but I would appreciate if you did it as well

@github-actions github-actions Bot added scope/core Touches the core architectural tier A-virtual-channel size/M Size: 150-399 lines of code labels Jul 31, 2026
@truebest

truebest commented Aug 2, 2026

Copy link
Copy Markdown
Contributor Author

i can fix the conflicts.

@truebest
truebest force-pushed the feat/egfx-progressive-client-decode branch from d51fe30 to bf9e903 Compare August 2, 2026 04:50
@github-actions github-actions Bot added size/M Size: 150-399 lines of code and removed size/M Size: 150-399 lines of code labels Aug 2, 2026
@CBenoit Benoît Cortier (CBenoit) added breaking-change Includes a breaking change, and requires special scrutiny at the boundaries risk/high Substantial core public API impact, or fail-closed triage; needs maintainer-level scrutiny ai-reviewed/1 One automated review completed labels Aug 3, 2026
@CBenoit

Copy link
Copy Markdown
Member

This review contains blocking findings: WireToSurface2 updates are callback-only and never reach compositor-backed drain_output, and REGION blocks outside FRAME_BEGIN/FRAME_END are decoded instead of ignored; progressive state retention after DeleteSurface also needs clarification. Tests were not run.

@github-actions github-actions Bot added maintainer-required Maintainer review or intervention is required risk/unknown and removed risk/high Substantial core public API impact, or fail-closed triage; needs maintainer-level scrutiny labels Aug 4, 2026
@truebest

truebest commented Aug 6, 2026

Copy link
Copy Markdown
Contributor Author

Benoît Cortier (@CBenoit) Before I update the PR, would you like all three findings addressed directly in #1443? I can make WireToSurface2 update the compositor, ignore REGION blocks outside FRAME_BEGIN/FRAME_END, and clear Progressive decoder state on DeleteSurface, with regression tests for each case. Or would you prefer any of these to be handled separately?

@glamberson

Copy link
Copy Markdown
Contributor

One bit of context on the first finding, since it landed after you opened this. #1460 added a client-side compositor to ironrdp-egfx, and every other decode path now feeds it as well as firing the callback. WireToSurface1 is the pattern, at crates/ironrdp-egfx/src/client.rs:829:

self.compositor.apply_bitmap(surface_id, dest_rect, &update.data);
self.handler.on_bitmap_updated(&update);

WireToSurface2 only does the second, so Progressive never reaches drain_output. Not something you missed; the compositor didn't exist when you wrote this.

It also matters beyond this PR: #1461 consumes drain_output to render into the session image, so Progressive stays invisible there until that call is added.

@github-actions github-actions Bot added size/XL Size: 800 or more lines of code and removed size/M Size: 150-399 lines of code breaking-change Includes a breaking change, and requires special scrutiny at the boundaries labels Aug 6, 2026
@github-actions github-actions Bot added size/L Size: 400-799 lines of code and removed size/XL Size: 800 or more lines of code labels Aug 6, 2026
Benoît Cortier (CBenoit) pushed a commit that referenced this pull request Aug 7, 2026
## Summary

`GraphicsPipelineHandler::capabilities()` defaulted to advertising
`CapabilitySet::V10_7`, which
tells the server AVC444 is available unless `AVC_DISABLED` is set.
`GraphicsPipelineClient` has no
AVC444 decoder — `handle_wire_to_surface1` routes `Codec1Type::Avc444`
and `Avc444v2` to
`on_unhandled_pdu`. The server picks one of the advertised sets and
prefers the most capable, so on
a host that supports AVC444 the client asked for a codec it then
discarded: every frame of the
desktop was lost, the session looked frozen, and nothing reached
`on_bitmap_updated` at all.

This drops V10.7 from the default. V8.1 keeps `AVC420_ENABLED`, which
*does* decode, so no H.264
capability is lost, and V8 remains the no-AVC fallback. V10.7 can come
back the moment AVC444
decodes — the doc comment and the new test both say so.

Closes #1563. This is item 5 of the #1464 tracking list ("advertise only
what we can decode", which
already names AVC444 as the example), addressed for the AVC444 case
only.

## Why the smaller set is not a downgrade

| set | AVC420 | AVC444 | decodable today |
|---|---|---|---|
| V10.7 (removed) | yes | yes | **no** — AVC444 has no decoder |
| V8.1 `AVC420_ENABLED` | yes | no | yes, via `decode_avc420` |
| V8 | no | no | yes (Uncompressed, ClearCodec, Planar) |

An alternative that keeps V10.x is to advertise it with `AVC_DISABLED`
set, but per
`CodecCapabilities::from_capability_set` that flag clears `avc420` as
well, so it gives up H.264
entirely. V8.1 is the better default while AVC444 is missing.

## Changes

- `crates/ironrdp-egfx/src/client.rs` — remove `V10_7` from the default
`capabilities()`; document
  why V10.x is absent and what has to happen before it returns.
- Two unit tests: the advertised sets imply no AVC444, and AVC420 stays
advertised. They assert
against `CodecCapabilities::from_capability_set` rather than naming
versions, so they keep holding
  if the set changes shape.
- `crates/ironrdp-testsuite-core/tests/egfx/client.rs` —
`client_keeps_avc_caps_with_decoder` now
  expects two sets instead of three.

## Validation

Reproduced and measured against Windows 11 Pro 25H2 (build 26200.8875),
no GPU (WARP software
rendering), same LAN, `base tcp rtt` 5 ms. Server-side counters sampled
with
`Get-Counter "\RemoteFX Graphics(*)\*" -SampleInterval 2 -MaxSamples 12`
while interacting:

| client | session res | output fps avg/max | avg encoding time |
|---|---|---|---|
| default caps, AVC444 selected | 3360x1930 | **0.00 / 0.00** | 4.50 ms
|
| FreeRDP `sdl-freerdp`, same host, minutes apart | 3360x1930 | 9.31 /
24.00 | 5.25 ms |

`frames skipped/second — insufficient server resources` and `—
insufficient client resources` were
both 0.00 and guest CPU stayed at 1–3%: the server was encoding and not
throttling. FreeRDP's
near-identical encoding time on the same pipeline confirms the server
side was healthy, which is
what places the fault in the client's advertisement.

Local checks, all clean:

- `cargo xtask check fmt -v`
- `cargo xtask check lints -v`
- `cargo xtask check tests -v` — 22 suites, 0 failed
- `cargo clippy -p ironrdp-egfx --all-targets --all-features` — 0
warnings
- `cargo doc -p ironrdp-egfx --no-deps` — the two remaining warnings
(`THIRD_PARTY_NOTICES`,
  `compositor` → private `Compositor`) predate this branch

`cargo xtask check typos -v` was skipped: `typos-cli` is not installed
in this environment.

## Deliberately not in this PR

- **AVC444 itself.** It needs YUV plane access that
`H264Decoder`/`DecodedFrame` do not expose —
the auxiliary stream is a packed chroma carrier, not an image, so it
cannot be reconstructed from
two RGBA buffers — plus a second decoder context, since each subframe is
its own H.264 sequence.
That is a public-API decision for the maintainers; I sketched a
non-breaking shape
(`fn decode_avc444(..) -> DecoderResult<DecodedFrame>` defaulting to an
error, implemented for
`OpenH264Decoder`) in #1563 and would rather agree on it before writing
the reconstruction.
I can validate a branch against the 25H2 host above; the counter harness
is scripted.
- **Diagnosability.** #1563 also suggests raising the "AVC444 codec not
yet implemented" `debug!` to
a first-occurrence `warn!` and adding a typed callback for undecodable
content. Both are useful
and both are separate from this fix, so I left them out rather than mix
concerns.
- **The no-decoder path.** With `h264_decoder: None` the filter leaves
V8 only and a Windows host
answers with RFX Progressive, which `on_wire_to_surface2` drops by
default — the same blank screen
by another route until #1443 lands. Out of scope here, noted for #1464
item 4.
@CBenoit

Copy link
Copy Markdown
Member

I’m fine with any path forward you would prefer here.

Greg Lamberson (@glamberson) should we merge #1460 first, and this one next?

@glamberson

Copy link
Copy Markdown
Contributor

#1460 merged on 31 July as 9cd3695, and this branch already sits on it, so
there is nothing to sequence there. Its compositor call is in here too:
WireToSurface2 now runs apply_bitmap alongside the callback, which was the first
of your three findings.

The other two are in as well. REGION blocks are processed only inside
FRAME_BEGIN/FRAME_END, and the Progressive context is cleared on DeleteSurface,
ResetGraphics and DeleteEncodingContext, each with a regression test.

What is left is a rebase. The branch is 61 behind and conflicts in one file,
crates/ironrdp-egfx/src/client.rs, against one commit: b7657bc, #1564, which
merged today and is the only thing to have touched that file since this branch's
merge base.

truebest the rebase is over #1564 alone, one file. That commit is all that
stands between this and merge.

On ordering, the thing worth weighing is that landing this unblocks rendering
rather than just decoding: #1461 consumes drain_output, so Progressive stays
invisible in the session image until the compositor call in here is on master.

…atch

GraphicsPipelineClient::handle_pdu currently only forwards WireToSurface2
(the progressive codec) to handler.on_wire_to_surface2() and returns,
without decoding it -- unlike AVC420 (decode_avc420) and ClearCode's
WireToSurface1 dispatch (Devolutions#1175), which both decode into BitmapUpdate
callbacks.

Add a progressive_decoder field to GraphicsPipelineClient, decode
WireToSurface2 streams through it, and emit each updated 64x64 tile as a
BitmapUpdate through the existing on_bitmap_updated path. A decode failure
now propagates as a terminal error instead of being logged and dropped,
matching decode_avc420's behavior. ResetGraphics and DeleteEncodingContext
now clear the decoder's per-context tile state, since both destroy the
codec_context_id(s) scoped to them and a later reused id must not decode
against stale tiles.
…iles

- handle_wire_to_surface2 now returns a terminal error for an unknown
  surface instead of logging and silently returning Ok(()), matching
  handle_wire_to_surface1's existing behavior.
- Lowercase the progressive-decode-failure error message to match this
  file's convention.
- Clip each tile's destination_rectangle/width/height to the surface
  bounds and crop the RGBA buffer with the existing crop_decoded_frame
  helper, so a surface whose dimensions aren't a multiple of 64 no longer
  gets a BitmapUpdate claiming pixels outside the surface.
ProgressiveDecoder keyed its per-context tile state by codec_context_id
alone. Per MS-RDPEGFX, codec_context_id is scoped to the surface that owns
it (RDPGFX_DELETE_ENCODING_CONTEXT_PDU carries both surface_id and
codec_context_id together), so two surfaces reusing the same
codec_context_id value would collide: decoding the second surface could
overwrite the first's tile state, and deleting one surface's context would
remove the other's.

decode_bitmap() and delete_context() now also take surface_id and key
contexts by (surface_id, codec_context_id). Added
decoder_contexts_scoped_by_surface to cover it.
@truebest
truebest force-pushed the feat/egfx-progressive-client-decode branch from 31948c5 to 6d89e8d Compare August 7, 2026 21:08
@github-actions github-actions Bot added breaking-change Includes a breaking change, and requires special scrutiny at the boundaries kind/protocol Changes how we encode/decode or interpret RDP wire packets risk/medium Behavioral change that does not substantially alter a core public API ai-reviewed/2 Final automated review completed and removed risk/unknown ai-reviewed/1 One automated review completed labels Aug 9, 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.

Wiring the progressive decoder into WireToSurface2 is well motivated; surface-scoped context keys, the delete_surface hook, frame bracketing and REGION clipping look sound. Two changes combine into a likely regression: handle_reset_graphics wipes the progressive contexts, and any decode error becomes an error that propagates out of DrdynvcClient::process, killing the session rather than the payload. The file's own comment records that xrdp and GNOME Remote Desktop send CONTEXT only once per context, so after a ResetGraphics the next payload hits MissingBlock("CONTEXT") and the session dies; new tests lock in both halves. The clipping region is also built with a quadratic union_rectangle loop over server-controlled REGION rect counts. Lower severity: reduce-extrapolate is read from CONTEXT while the REGION accessor is dead code, updates are labelled Codec1Type::Uncompressed, a second FRAME_BEGIN/END pair is silently dropped, and DeleteEncodingContext discards DecDwtQ state.

Protocol analysis: partially_accepted — Accepted and sharpened the CONTEXT-optional concern; propagation traced to DrdynvcClient::process (ironrdp-dvc/src/client.rs:368), so it is not channel-local. Accepted the ResetGraphics concern but rejected its rationale: handle_reset_graphics already clears every surface (client.rs:669-670), so the defect is losing the CONTEXT-derived flag, not tile coefficients. Accepted the reduce-extrapolate item: ProgressiveRegion::uses_reduce_extrapolate has no non-test caller. Rejected the frame_active item as moot, since cleared surfaces fail the surface lookup first. Kept DeleteEncodingContext at low severity. Missed: quadratic union_rectangle cost, Codec1Type mislabel, dropped second frame pair.

  1. question / medium — crates/ironrdp-graphics/src/progressive.rs
    use_reduce_extrapolate is taken from the RFX_PROGRESSIVE_CONTEXT block's flags. The PDU crate also defines ProgressiveRegion::uses_reduce_extrapolate (crates/ironrdp-pdu/src/codecs/rfx/progressive.rs:787) against the same FLAG_DWT_REDUCE_EXTRAPOLATE constant, and that accessor has no non-test caller anywhere in the tree. Two structurally different blocks cannot both be authoritative, and this flag selects the DWT band layout: choosing wrongly mismatches quantization offsets and inverse-DWT band sizes, so every tile in the frame decodes to incorrect pixels. The handoff reports that the CONTEXT flags field carries sub-band diffing rather than reduce-extrapolate. The behaviour predates this PR, but this PR first routes real server streams through it, and every new fixture sets flags: 0 on both blocks, so the tests cannot discriminate. Was CONTEXT-block sourcing validated against a server that sets the two flags differently, or should the REGION accessor be the source?
  2. non_blocking / low — crates/ironrdp-graphics/src/progressive.rs
    The frame_ended flag suppresses any FRAME_BEGIN after the first FRAME_END in a payload, so a second FRAME_BEGIN/REGION/FRAME_END group in the same bitmapData is dropped without decoding and without a warning. The in_frame guard alone already implements the rule that blocks outside a FRAME_BEGIN/FRAME_END pair are ignored, which is what decoder_ignores_regions_outside_frame exercises; frame_ended adds a stricter rule that discards data. The handoff reports the specification only says FRAME_BEGIN must appear once and that duplicate FRAME_END should be ignored, without prescribing this, so the extra state buys strictness against an already-misbehaving server at the cost of silently losing screen updates. Either drop the flag or log when it discards a group.

if let Some(ref mut decoder) = self.h264_decoder {
decoder.reset();
}
self.progressive_decoder.reset();

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.

blocking / critical: handle_reset_graphics now calls progressive_decoder.reset(), dropping every (surface_id, codec_context_id) entry and with it the use_reduce_extrapolate value derived from the CONTEXT block. The decoder's own comment (progressive.rs:1203-1207) records that xrdp and GNOME Remote Desktop send SYNC+CONTEXT only on the frame that establishes a context and omit it thereafter. RDPGFX_RESET_GRAPHICS is routine on monitor-layout or resolution change. After one, the server recreates its surfaces and resumes sending WireToSurface2 for the same codec_context_id with no CONTEXT block; the lookup at progressive.rs:1217-1221 misses and returns MissingBlock("CONTEXT"), which line 822 turns into a hard error. The comment three lines below deliberately does not reset the ClearCodec decoder for closely related reasoning. reset_graphics_clears_progressive_decoder_context asserts this behaviour, so the regression is locked in rather than caught.

Err(e) => {
warn!(error = ?e, "rfx progressive decode failed");
return Err(pdu_other_err!("rfx progressive decode failed"));
}

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.

blocking / high: Every ProgressiveDecodeError is converted to pdu_other_err!. That error leaves handle_pdu, then GraphicsPipelineClient::process, then DynamicVirtualChannel::process, and finally DrdynvcClient::process (crates/ironrdp-dvc/src/client.rs:368), so it is not contained to the graphics channel: one undecodable progressive payload terminates the RDP session. The decoder has many reachable error paths (MissingBlock, TileOutOfBounds, RLGR/SRL failures, dimension caps), several describing streams a client should tolerate rather than treat as fatal, and the payload is fully server-controlled. Before this PR WireToSurface2 was only forwarded to the handler, so nothing here could abort the session. Skipping the payload with the existing warn!, or degrading to a handler forward as the unsupported-codec arms of handle_wire_to_surface1 do, keeps the security posture without making decode strictness a liveness property. wire_to_surface2_decode_failure_propagates_error asserts the fatal behaviour using a stream that omits only the optional CONTEXT block.

bottom: bottom - 1,
});
}
}

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.

blocking / medium: clipping_region is built by calling Region::union_rectangle once per REGION rect. Each call runs split_bands over the whole current rectangle list, walks every band, then simplify(), so the loop is quadratic in region.rects.len(). numRects is a u16 read off the wire, bounded only by payload size at 8 bytes per rect, so a single ~512 KB REGION block yields ~65k inserts and on the order of 10^9 rectangle visits plus tens of thousands of allocations, and a payload may carry several REGION blocks. The per-tile loop below compounds it: intersect_rectangle is linear in the region's rectangle count and runs once per accumulated frame tile per REGION. This decoder was unreachable from the wire before this PR, so the change is what exposes the amplification, and the repository's stated posture is to treat parsing as a hostile-input surface. A bound on numRects, or clipping without materialising a full Region, would close it.

let update = BitmapUpdate {
surface_id: pdu.surface_id,
destination_rectangle,
codec_id: Codec1Type::Uncompressed,

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.

non_blocking / medium: Progressive updates are published on the public BitmapUpdate with codec_id: Codec1Type::Uncompressed. BitmapUpdate documents that field as "Codec that produced this update", and every other producer reports its real codec (Avc420, ClearCodec, Planar, Uncompressed), so handlers that branch or record on codec_id will silently attribute progressive output to an uncompressed WireToSurface1 command. Codec1Type has no progressive variant because progressive arrives as Codec2Type::RemoteFxProgressive, which is the underlying mismatch; the field's type or documentation needs to account for the WireToSurface2 source rather than borrow a wrong value. The BitmapUpdate doc comment at client.rs:188-189 also still says updates are delivered only when a WireToSurface1 PDU is processed.

"DeleteEncodingContext"
);
self.progressive_decoder
.delete_context(pdu.surface_id, pdu.codec_context_id);

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.

non_blocking / low: delete_context removes the whole ProgressiveContext, including the SurfaceTiles whose per-component coefficients TileState documents as DecDwtQ (progressive.rs:757) and which reconstruct_to_rgba accumulates across frames. The handoff reports that MS-RDPEGFX distinguishes progressive tile contexts, discardable when their codec context is deleted, from sub-band diffing tile contexts, which must survive until the surface is deleted; this implementation stores both lifetimes in one structure keyed by codec context. If a server deletes an encoding context and later sends difference tiles for the same surface, they reconstruct against zeroed coefficients. Unlike the ResetGraphics case the surface is not destroyed here, so the two lifetimes genuinely diverge. I could not confirm from the repository that servers rely on this, and the pre-existing delete_context already had this shape, so this warrants a follow-up rather than a block.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ai-reviewed/2 Final automated review completed breaking-change Includes a breaking change, and requires special scrutiny at the boundaries kind/protocol Changes how we encode/decode or interpret RDP wire packets maintainer-required Maintainer review or intervention is required risk/medium Behavioral change that does not substantially alter a core public API scope/core Touches the core architectural tier size/L Size: 400-799 lines of code

Development

Successfully merging this pull request may close these issues.

4 participants