feat(egfx): wire RemoteFX Progressive decode into WireToSurface2 dispatch - #1443
feat(egfx): wire RemoteFX Progressive decode into WireToSurface2 dispatch#1443truebest wants to merge 5 commits into
Conversation
There was a problem hiding this comment.
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
ProgressiveDecoderfield toGraphicsPipelineClientand decodeWireToSurface2intoBitmapUpdatecallbacks. - Clear progressive decoder context state on
ResetGraphicsandDeleteEncodingContext. - 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.
| let Some(surface) = self.surfaces.get(&pdu.surface_id) else { | ||
| warn!(surface_id = pdu.surface_id, "WireToSurface2 for unknown surface"); | ||
| return Ok(()); | ||
| }; |
There was a problem hiding this comment.
Fixed in 8d71776 — now returns pdu_other_err!("unknown surface in WireToSurface2"), matching handle_wire_to_surface1.
| Err(e) => { | ||
| warn!(error = ?e, "RFX progressive decode failed"); | ||
| return Err(pdu_other_err!("RFX progressive decode failed")); | ||
| } |
There was a problem hiding this comment.
Fixed in 8d71776 — lowercased to "rfx progressive decode failed".
| 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); | ||
| } |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
💡 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".
| destination_rectangle: ExclusiveRectangle { | ||
| left, | ||
| top, | ||
| right: left.saturating_add(64), | ||
| bottom: top.saturating_add(64), |
There was a problem hiding this comment.
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 👍 / 👎.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
💡 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, |
There was a problem hiding this comment.
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 👍 / 👎.
There was a problem hiding this comment.
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.
…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.
|
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 The code reads correctly and the automated review points all look addressed:
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. |
|
i can fix the conflicts. |
d51fe30 to
bf9e903
Compare
|
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. |
|
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? |
|
One bit of context on the first finding, since it landed after you opened this. #1460 added a client-side compositor to self.compositor.apply_bitmap(surface_id, dest_rect, &update.data);
self.handler.on_bitmap_updated(&update);
It also matters beyond this PR: #1461 consumes |
## 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.
|
I’m fine with any path forward you would prefer here. Greg Lamberson (@glamberson) should we merge #1460 first, and this one next? |
|
#1460 merged on 31 July as 9cd3695, and this branch already sits on it, so The other two are in as well. REGION blocks are processed only inside What is left is a rebase. The branch is 61 behind and conflicts in one file, truebest the rebase is over #1564 alone, one file. That commit is all that On ordering, the thing worth weighing is that landing this unblocks rendering |
…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.
31948c5 to
6d89e8d
Compare
There was a problem hiding this comment.
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.
- 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? - 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(); |
There was a problem hiding this comment.
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")); | ||
| } |
There was a problem hiding this comment.
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, | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
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, |
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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.
Problem
GraphicsPipelineClient::handle_pducurrently only forwardsWireToSurface2(the RemoteFX Progressive codec) tohandler.on_wire_to_surface2()and returns, without decoding it. This differs from the AVC420 path (decode_avc420) and from ClearCodec'sWireToSurface1dispatch (#1175), both of which decode intoBitmapUpdatecallbacks throughon_bitmap_updated.The
ProgressiveDecoderitself 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
GraphicsPipelineClientgains aprogressive_decoder: ProgressiveDecoderfield.WireToSurface2now decodes through it and emits each updated 64x64 tile as aBitmapUpdatevia the existingon_bitmap_updatedpath.PduResulterror instead of being logged and silently dropped — matchingdecode_avc420's behavior. Previously this would leave the session running with no further bitmap updates for the surface and no visible error.ResetGraphicsandDeleteEncodingContextnow clear the decoder's per-context tile state, since both destroy thecodec_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;
ResetGraphicsclears context state (verified via a REGION-only continuation failing afterward);DeleteEncodingContextdoes 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.