Repository navigation
feat(connector)!: expose SUPPORT_DYN_VC_GFX_PROTOCOL early-cap flag for EGFX clients - #1237
Conversation
|
Reading through, this is the smallest possible fix and the doc-comment captures the foot-gun precisely. Without wiring a DvcClientProcessor for the Graphics channel, modern Windows servers stop sending legacy bitmap updates entirely and the desktop goes blank, which is exactly the GRD case in #1232 and the failure mode every downstream EGFX client hits. No conflict with #1098 (multitransport bootstrapping) on my read of the GCC block area, just two independent flags being OR'd in. Looks good from the server-side perspective too: any IronRDP server that already speaks EGFX (lamco-rdp-server, GRD) sees this flag and routes graphics through DVC as expected. |
27f3273 to
84ddffa
Compare
|
GlassOnTin Hi, could you rebase this? I'm trying to get this prioritized, and this would be a first step on the client side work I'm looking t. Thanks! |
84ddffa to
dbe5d82
Compare
|
Done — rebased onto current The only conflicts were in the test builders, where
|
|
Thank you! Benoît Cortier (@CBenoit) can you expedite this please? Thank you. My client issue will be posted later today. |
|
For reviewer context: I've opened #1472 as a sibling to this — a small |
…tionResult (#1488) Per [MS-RDPBCGR] 2.2.8.1.2, a client must not send fast-path input events unless the server advertised `INPUT_FLAG_FASTPATH_INPUT` or `INPUT_FLAG_FASTPATH_INPUT2` in its Input Capability Set. Today the connector discards the server's capability sets after Demand Active, so client code has no way to honour that requirement. This PR captures the server's Input capability flags during capabilities exchange, carries them through the `ConnectionFinalization`/`Finalized` activation states (so they are refreshed correctly across a Deactivation-Reactivation Sequence too), and surfaces them as `ConnectionResult::input_flags`. The session layer can then choose between fast-path and slow-path input per server. ### Motivation / real-world interop This is not theoretical: VirtualBox's VRDP server closes the connection outright on receiving a fast-path input PDU — its `VBox.log` reports ``` VRDP: Network packet length is incorrect 0x0004. Closing connection. ``` (a single fast-path scancode event is a 4-byte packet). VirtualBox never advertises fast-path input; its Demand Active offers `InputFlags(SCANCODES)` alone. mstsc and FreeRDP honour the negotiation and fall back to slow-path `TS_INPUT_PDU`s, which is why they work against VRDE. Haven (Android RDP client built on IronRDP) has been shipping this exact change as a vendored-connector patch since v5.86.1, with the slow-path fallback keyed off `ConnectionResult::input_flags`. Verified against a real VirtualBox 7.2.6 VRDE server: before the gate, the first arrow-key press killed the session with the log line above; with the gate, extended input sessions run clean, and a fast-path-capable server on the same host still takes the fast-path branch. (Discussed in #1158; this is the third and last piece Haven carries in its connector fork, alongside #1237 and #1472.) ### Changes - `connection_activation.rs`: capture `input_flags` from the `CapabilitySet::Input` in Server Demand Active (empty if absent); add the field to `ConnectionActivationState::{ConnectionFinalization, Finalized}`. - `connection.rs`: add `ConnectionResult::input_flags`, populated from the `Finalized` state. - Call sites in `ironrdp-client`, `ironrdp-web`, ffi, and the e2e test updated for the new variant field (all currently ignore it). ### Testing - Two new integration tests in `ironrdp-testsuite-core/tests/session/connection_activation.rs`: the fixture's Demand Active yields `SCANCODES | MOUSEX | UNICODE | FASTPATH_INPUT_2` in the `ConnectionFinalization` state, and a Demand Active with the Input capability stripped yields `InputFlags::empty()`. - `cargo check --workspace --all-targets`, `cargo clippy --workspace --all-targets`, and `cargo fmt --check` are clean; the 7 activation tests pass. Co-authored-by: GlassOnTin <GlassOnTin@users.noreply.github.com>
…or EGFX clients Adds an opt-in `Config::support_dyn_vc_gfx_protocol` (default `false`). When set, `SUPPORT_DYN_VC_GFX_PROTOCOL` is OR'd into the client's early capability flags, so the server may negotiate `Microsoft::Windows::RDS::Graphics` for surface-based graphics instead of the legacy slow-path bitmap protocol. BREAKING CHANGE: `Config` is not `#[non_exhaustive]`, so the new field breaks downstream struct literals. Same shape as Devolutions#1501, which added `auto_reconnect_cookie` to this struct. The default is `false` on purpose. A client that advertises the bit without wiring an EGFX-capable `DvcClientProcessor` for that channel gets modern Windows servers to stop sending legacy bitmap updates altogether and route everything over EGFX, leaving the desktop blank — so the doc comment spells out the three steps to enable it properly. Coverage: - create_gcc_blocks unit tests (inline mod at file end, per Devolutions#1527) assert the bit is clear by default, set when enabled, and that the opt-in's symmetric difference against the default flag set is exactly that one bit — so it cannot quietly disturb the others. Verified to fail: gating on `if false` reddens two of the three. - testsuite-extra e2e runs a full client<->server connect with the bit advertised. Scope stated in the test: it asserts negotiation completes, not that the server acted on it, since ironrdp-server does not surface the client's early capability flags.
1502311 to
d911125
Compare
|
Rebased onto current master and back to a single commit (d911125). While doing so I found that a push on Aug 5 had accidentally stacked five unrelated commits onto this branch (they belonged with #1541 and with session work that has since landed upstream), so anyone opening the PR since then saw a six-commit mixture instead of the one-line capability flag. Sorry about that; it is cleaned up now. Checked locally on the rebased branch: the fmt gate passes via cargo xtask check fmt, cargo check --workspace --all-targets is clean, and the connector unit tests pass including the three tests covering the flag (absent by default, set on opt-in, changes exactly one bit). Benoît Cortier (@CBenoit) the state since your Aug 3 review is unchanged in substance: the PR is marked breaking (feat(connector)!:) for the added Config field, matching the shape #1501 shipped with, and the default keeps the wire bit clear. Is there anything else you would like changed, or can this go into the review queue? Greg Lamberson (@glamberson) mentioned in July this was a first step for their client-side work, so there is still downstream interest beyond mine. |
…1462) ## Summary - `ironrdp-client` never registered `GraphicsPipelineClient`, so the native client did not open the `Microsoft::Windows::RDS::Graphics` channel and could not use the Graphics Pipeline. - Registers it as a DVC alongside Display Control and Echo, with a no-op `GraphicsPipelineHandler` (the compositor and session render do the work) and no H.264 decoder, so the client advertises only the non-AVC capability sets it can decode. ## Validation - `cargo xtask check fmt/lints/tests/typos/locks` all pass. ## Notes - Codec decoders (H.264 via `openh264-libloading`, ClearCodec #1175, Progressive #1443) and the connector early-cap flag #1237 layer on separately. - #1377 and #1460 have merged. This is now stacked on #1461 alone, so the merge order is #1461 then this, and #1461's commit appears in the diff until it lands. Its own change is 3 files, +12/-1. - Part of #1464. Motivated by #1446.
Marc-André Moreau (mamoreau-devolutions)
left a comment
There was a problem hiding this comment.
Approved. The capability flag wiring is protocol-conformant and preserves existing behavior by default. The inline notes are nonblocking follow-ups.
Note
Human-tuned, LLM-assisted content.
Point EGFX users to the supported GraphicsPipelineClient registration path and clarify that disabling the flag only leaves EGFX unadvertised. Move capability-bit coverage into the enabled core testsuite and decode the emitted Connect Initial PDU so normal CI exercises the assertion.
5bdb679
into
Devolutions:master
Currently,
early_capability_flagsin the GCC core data is built from a fixed set inconnection.rs. Clients that want to use the Graphics Pipeline Extension (MS-RDPEGFX) — by attaching aDvcClientProcessorforMicrosoft::Windows::RDS::Graphics— have no way to setSUPPORT_DYN_VC_GFX_PROTOCOLwithout forking the connector, and modern Windows servers won't open the EGFX channel unless the client advertises support.This PR adds an opt-in
Config.support_dyn_vc_gfx_protocol: bool(defaultfalse). When set, the flag is OR'd intoearly_capability_flagsalongside the existingWANT_32_BPP_SESSIONconditional. Existing consumers are unaffected; the doc comment includes a safety note that setting this without an EGFX implementation will cause Windows to stop sending legacy bitmap updates, leaving the desktop blank.Used downstream by Haven (an Android RDP/VNC client) which implements EGFX with ClearCodec + RemoteFxProgressive decoders; this lets us drop a vendored fork of
ironrdp-connectorwe currently carry just for this one flag.Default
falseto preserve current behaviour.cargo check --workspaceclean — six other in-treeConfig { … }builders updated with the default-false field.