Repository navigation
feat(egfx)!: composite surfaces mapped to RemoteApp windows - #2197
Marc-André Moreau (mamoreau-devolutions) merged 3 commits into
Conversation
|
Line counts: production code about +269/−14 (window mapping in the EGFX compositor and client, session/client output plumbing); tests +123/−2 ( |
There was a problem hiding this comment.
PR #2197 plumbs MS-RDPEGFX MapSurfaceToWindow/MapSurfaceToScaledWindow surfaces into RemoteApp window updates: the compositor records window mappings (mutually exclusive with output mappings), materializes clipped/scaled dirty regions at EndFrame into a budgeted ready_windows queue, and the session/client/FFI forward a new WindowSurfaceUpdates event. I independently verified the mapping state machine, budget accounting, scaling math, drain paths, tests, and generated enums; the implementation is sound and the plumbing is additive. Published 12 findings: one merged low-severity protocol-conformance concern (out-of-limit mappings are dropped and can leave a stale previous mapping where MS-RDPEGFX 3.3.5.20/23 require the mapping to be recorded), an FFI discriminator without a payload accessor, three test-coverage gaps (both remap transitions, oracle markers, property coverage of clip-then-scale), an intent-coverage note, three behavior-preserving simplifications, and three mechanical sty…
- [intent-drift] New window-mapping public surface is not covered by any intent file — low 🟡 — crates/ironrdp-egfx/src/compositor.rs
The diff introduces new public surface with no governing intent statement: no INTENT file exists for ironrdp-egfx, ironrdp-session, ironrdp-client, ironrdp-viewer, ironrdp-web, or ironrdp-testsuite-core, and the only intent file under a changed path (ffi/INTENT.md) does not decide this surface. Undecided items include compositor::WindowUpdate and its window-coordinate/scaling semantics, drain_window_updates drain semantics, the WindowSurfaceUpdates variants on ActiveStageOutput/RdpOutputEvent, the FreeRDP-style oversized-mapping ignore rule, and the .NET enum value 15. Impact is governance: future changes to this surface have no review criteria to cite. - [code-compressor] Duplicate drain-and-push block at both drain_graphics_pipeline call sites — low 🟡 — crates/ironrdp-session/src/active_stage.rs
The code at lines 254-260 and 633-639 is identical: destructure the (Option<InclusiveRectangle>, Vec<WindowUpdate>) tuple, conditionally push GraphicsUpdate, then conditionally push WindowSurfaceUpdates. Shaping drain_graphics_pipeline as (&mut self, image: &mut DecodedImage, stage_outputs: &mut Vec<ActiveStageOutput>) -> SessionResult<()> and performing both conditional pushes inside it removes the tuple return and roughly 12 duplicated lines while preserving output order (GraphicsUpdate then WindowSurfaceUpdates, before widen_to_full_refresh at the second site). An &mut parameter works at both call sites.
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.
| /// RemoteApp window pixels from graphics pipeline surfaces mapped to RAIL windows. | ||
| WindowSurfaceUpdates = 15, |
There was a problem hiding this comment.
[skeptical] FFI exposes the WindowSurfaceUpdates event type but no way to read its payload — low 🟡 — The diff adds the ActiveStageOutputType::WindowSurfaceUpdates discriminator and its match arm, but unlike every other payload-bearing variant (get_graphics_update, get_windowing_orders, get_monitor_layout, ...), no getter exposes the WindowUpdate pixels, region, window_id, or size. In a RemoteApp + EGFX session the compositor materializes and charges every window region each frame, the FFI maps it to an enum value and drops the payload, so the pixels are copied and charged against the budget purely to be discarded. The getter should land together with the discriminator, or the FFI enum change deferred.
There was a problem hiding this comment.
Declined. The .NET binding cannot reach this payload: FFI sessions do not negotiate RAIL (RailSupportLevel is empty), so servers never map surfaces to windows there. The discriminator keeps the FFI match exhaustive; a getter belongs with FFI RemoteApp support and needs a diplomat regeneration.
24a5ef1 to
903b74d
Compare
20d6c21 to
4e639ff
Compare
|
This pull request may overlap with #2201. PR 2201 projects RemoteApp windows in the viewer and lists notification icons, z-order, local move/size, window shapes and graphics-pipeline window surfaces as not yet handled. This PR implements those RAIL features (tray icons, z-order, move/size, shapes, icon cache, MapSurfaceToWindow presentation) in the ActiveX control plus the egfx/session plumbing, so the two share substantial RemoteApp windowing scope. 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). |
There was a problem hiding this comment.
PR #2197 composites EGFX surfaces mapped to RemoteApp windows (MapSurfaceToWindow/MapSurfaceToScaledWindow) into a new WindowUpdate queue drained via GraphicsPipelineClient::drain_window_updates, plumbed through ActiveStageOutput::WindowSurfaceUpdates, RdpOutputEvent, the FFI enums, with viewer/web consumers ignoring it and three new client-level tests. The core design is sound: window and output mappings are correctly mutually exclusive, the mapping validations mirror the oversized-mapping precedent, pixel math reuses the (genuinely needed) stride_width-corrected scaled copy, and the shared compositor budget is charged and released. Published findings: one latent protocol conformance gap (decoded alpha forwarded for window-mapped surfaces despite MS-RDPEGFX requiring alpha to be ignored), one open spec-interpretation question on scaled-window semantics, a real budget-starvation hole in the ironrdp-capture-replay consumer that never drains the new queue, four test-coverage gaps on the…
- [test] No test covers window/output mapping exclusivity or mapping an unknown surface — medium 🟠 — crates/ironrdp-testsuite-core/tests/egfx/client.rs
The PR establishes that a surface presents in exactly one place: map_surface_to_window clears surface.mapping and the output-mapping path clears surface.window. But none of the three new tests remaps a surface in either direction to pin that the stale mapping stops producing updates; scaled_window_mapping_scales_dirty_regions_to_the_target_size never asserts drain_output() is empty the way the unscaled test does; and the client's handle_map_surface_to_window unknown-surface guard (warn and leave the compositor untouched) is untested. A regression here double-presents or misroutes RemoteApp pixels, so one standalone transition test per direction plus a guard test is warranted. - [test] ActiveStageOutput::WindowSurfaceUpdates emission in both drain sites has no test — low 🟡 — crates/ironrdp-session/src/active_stage.rs
Both drain_graphics_pipeline call sites (the slow-path DVC-tunnel arm and the Fast-Path arm) now push WindowSurfaceUpdates when drain_window_updates is non-empty, and window pixels are deliberately kept out of the session image, but no test exercises either arm's emission or the image-untouched property. A wiring mistake in either arm would ship silently. A stage-level test driving a GraphicsPipelineClient far enough to assert emission from both arms would close it; if the DVC seam makes that impractical, the limitation should be stated rather than left implicit. - [code-compressor] Identical drain-and-emit block duplicated in process and process_dvc_tunnel — low 🟡 — crates/ironrdp-session/src/active_stage.rs
The drain_graphics_pipeline call plus the conditional GraphicsUpdate and WindowSurfaceUpdates pushes are duplicated verbatim at the two session entry points; this PR itself had to edit both identically to add the new output kind. Have drain_graphics_pipeline take &mut Vec<ActiveStageOutput> and push GraphicsUpdate/WindowSurfaceUpdates itself, dropping the tuple return; behavior is preserved since both sites perform the same pushes in the same order before widening. Removes the demonstrated divergence risk for the next pipeline output kind. - [rust-style] WindowMapping field invariants are not documented at the fields — low 🟡 — crates/ironrdp-egfx/src/compositor.rs
map_surface_to_window establishes the bounds materialize_window relies on (mapped area within the surface, target dimensions within MAX_OUTPUT_DIM, nonzero target lengths), but the WindowMapping struct carries none of this. Per the guide's invariant rule, document the constraints as INVARIANT: doc comments at the field declarations so readers of materialize_window can verify the clamping and scaling assumptions without backtracking to map_surface_to_window.
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.
| let data = if window.target_width == window.mapped_width && window.target_height == window.mapped_height { | ||
| copy_region(&surface.data, surface.width, source.left, source.top, w, h) | ||
| } else { | ||
| copy_scaled_region( | ||
| &surface.data, | ||
| surface.width, | ||
| (window.mapped_width, window.mapped_height), | ||
| OutputMapping { | ||
| origin: (0, 0), | ||
| target_width, | ||
| target_height, | ||
| }, | ||
| ®ion, | ||
| ) | ||
| }; |
There was a problem hiding this comment.
[protocol] Scaled-window mapping scales the mapped region to the target size rather than the entire surface — low 🟡 ❓ — MS-RDPEGFX 3.3.5.23 can be read as requiring the entire surface scaled to targetWidth/targetHeight (mirroring 3.3.5.22), while the implementation scales only the mappedWidth x mappedHeight region onto the full target area. The readings agree when the mapped region equals the surface size, the common case, and the specification corpus available here does not resolve the ambiguity. Publish as an open protocol-interpretation question for the author or a spec reference rather than a defect.
There was a problem hiding this comment.
Declined. MS-RDPEGFX 3.3.5.23 associates "the rectangular region (specified by the mappedWidth and mappedHeight fields)" with the window at the target size, so scaling the mapped area rather than the whole surface follows the text.
903b74d to
69ac450
Compare
4e639ff to
82d660a
Compare
69ac450 to
fbae712
Compare
82d660a to
8495bbf
Compare
e5bce84 to
50a489e
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Window updates discard Alpha-codec transparency, and targets exceeding the compositor budget can replace working mappings without producing pixels.
2 open issues
What changed in this PR
This PR extends the graphics compositor to deliver RemoteApp window pixels separately from desktop output, forwarding those updates through the session and client APIs.
Changes:
- Track window mappings and produce clipped, scaled window updates at frame completion.
- Add window-update events and update consumers and bindings for the new variants.
- Test mapping, scaling, rejection, and replay draining behavior.
| File | Description |
|---|---|
| ffi/src/session/mod.rs | Recognizes the new window-update output variant. |
| ffi/dotnet/Devolutions.IronRdp/Generated/RawActiveStageOutputType.cs | Adds the generated output enum value. |
| ffi/dotnet/Devolutions.IronRdp/Generated/ActiveStageOutputType.cs | Adds the generated output enum value. |
| crates/ironrdp-web/src/session.rs | Explicitly ignores RemoteApp window updates. |
| crates/ironrdp-viewer/src/app.rs | Explicitly ignores RemoteApp window updates. |
| crates/ironrdp-testsuite-core/tests/egfx/client.rs | Tests window mapping, scaling, and rejected mappings. |
| crates/ironrdp-session/src/active_stage.rs | Drains and emits window updates on both graphics transport paths. |
| crates/ironrdp-egfx/src/compositor.rs | Tracks window mappings and materializes window pixel regions. |
| crates/ironrdp-egfx/src/client.rs | Handles window mappings and exposes an update drain API. |
| crates/ironrdp-client/src/rdp.rs | Forwards window updates as client output events. |
| crates/ironrdp-capture-replay/src/routing.rs | Drains unused window updates and tests replay behavior. |
Review effort: Balanced
With the graphics pipeline in a RemoteApp session, Windows servers present each application window from its own surface through MapSurfaceToWindow or MapSurfaceToScaledWindow (MS-RDPEGFX 2.2.2.20 and 2.2.2.23). The client only forwarded these PDUs to handler callbacks, so nothing those surfaces drew reached the windows. The compositor now records window mappings and, at each EndFrame, materializes dirty regions of window-mapped surfaces into window updates: the RAIL window ID, the presented size and the changed region in window coordinates, scaled when the server asks for a different target size. GraphicsPipelineClient::drain_window_updates exposes them, the session emits ActiveStageOutput::WindowSurfaceUpdates and the client forwards RdpOutputEvent::WindowSurfaceUpdates. A mapped area larger than its surface is ignored, as in FreeRDP. BREAKING CHANGE: ActiveStageOutput and RdpOutputEvent have a new WindowSurfaceUpdates variant, and the .NET ActiveStageOutputType enum gains WindowSurfaceUpdates. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Window updates forced every pixel opaque, which discarded the alpha the Alpha codec writes; MS-RDPEGFX 3.3.5.1 ignores only the alpha of other codecs for window-mapped surfaces. A surface's alpha now starts opaque when the Alpha codec first writes it, window updates carry it from then on, and color bitmaps copy only red, green and blue into such a surface. A window target whose full update would not fit the compositor budget is now rejected like other unpresentable targets, so the surface keeps its previous mapping instead of losing every update. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Projected RemoteApp windows only followed the server's title, style, owner, geometry and show state. They had no icons, ignored window shapes, the notification area, z-order and activation, and the local move/size and cloak messages, and painted from the desktop frame even when the server drew a window through its own graphics pipeline surface. The control now builds icons from window icon orders and the icon cache and sets them on the title bar and taskbar; applies window and visibility rectangles as the window region; mirrors notification icons, tooltips and balloons in the local notification area and reports their mouse and balloon events back; applies the server z-order and active window without echoing activation; and hides or shows windows on Cloak. It advertises local move/size: a server-started move or resize runs the local modal loop. Keyboard moves, resizes and locally started frame or system-menu moves report the final rectangle with WindowMove, while a mouse move ends with the server's own end message (MS-RDPERP 3.2.5.2.7.3). Minimize, maximize and restore go to the server as system commands, and MinMaxInfo bounds the local tracking size. EGFX window surfaces paint the window from a per-window backing bitmap. The local system parameters (work area, taskbar, high contrast, accessibility keys and others) are sent at startup and again after WM_SETTINGCHANGE. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>

With the graphics pipeline in a RemoteApp session, Windows servers present each application window from its own surface through MapSurfaceToWindow or MapSurfaceToScaledWindow (MS-RDPEGFX 2.2.2.20 and 2.2.2.23). The client only forwarded these PDUs to handler callbacks, so nothing those surfaces drew reached the windows.
The compositor now records window mappings and, at each EndFrame, materializes dirty regions of window-mapped surfaces into window updates: the RAIL window ID, the presented size and the changed region in window coordinates, scaled when the server asks for a different target size. GraphicsPipelineClient::drain_window_updates exposes them, the session emits ActiveStageOutput::WindowSurfaceUpdates and the client forwards RdpOutputEvent::WindowSurfaceUpdates. A mapping the client cannot present, such as a mapped area larger than its surface, is ignored and the surface keeps its previous mapping; FreeRDP also refuses oversized mappings.
🤖 Generated with Claude Code
BREAKING CHANGE: ActiveStageOutput and RdpOutputEvent have a new WindowSurfaceUpdates variant, and the .NET ActiveStageOutputType enum gains WindowSurfaceUpdates.