feat(egfx): wire RemoteFX Progressive decode into WireToSurface2 dispatch#1443
feat(egfx): wire RemoteFX Progressive decode into WireToSurface2 dispatch#1443truebest wants to merge 3 commits into
Conversation
…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.
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.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
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.
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.
…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.
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.
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.
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.
…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.
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.