feat(activex): negotiate monitor topology - #1675
feat(activex): negotiate monitor topology#1675Marc-André Moreau (mamoreau-devolutions) wants to merge 7 commits into
Conversation
GitHub Actions
Active
github-actions
Bot
added
breaking-change
There was a problem hiding this comment.
Pull request overview
Adds ActiveX multi-monitor negotiation and exposes the resulting topology through compatibility APIs.
Changes:
- Plumbs monitor layouts into GCC Client Monitor Data.
- Adds ActiveX topology discovery, reporting, and resize restrictions.
- Adds tests, documentation, and single-monitor defaults.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
ffi/src/connector/config.rs |
Defaults monitor layout to none. |
crates/ironrdp/examples/screenshot.rs |
Updates example configuration. |
crates/ironrdp-web/src/session.rs |
Preserves single-monitor web behavior. |
crates/ironrdp-testsuite-extra/tests/e2e.rs |
Updates E2E configuration. |
crates/ironrdp-testsuite-core/tests/session/connection_activation.rs |
Updates activation test configuration. |
crates/ironrdp-testsuite-core/tests/connector/autodetect.rs |
Updates autodetect test configuration. |
crates/ironrdp-connector/src/lib.rs |
Exposes optional monitor layout. |
crates/ironrdp-connector/src/connection.rs |
Adds layout to GCC blocks. |
crates/ironrdp-client/src/config.rs |
Adds monitor-layout builder support. |
crates/ironrdp-activex/src/control.rs |
Implements ActiveX monitor negotiation and reporting. |
crates/ironrdp-activex/README.md |
Documents multi-monitor behavior. |
Suppressed comments (1)
crates/ironrdp-connector/src/connection.rs:1466
- The newly exposed public configuration accepts layouts above the protocol limit of 16 monitors.
ClientMonitorData::encodedeclares the full count/size but serializes only.take(16), so such a value produces a malformed GCC stream rather than theConnectorResulterror callers expect. Reject oversized layouts before placing them in the GCC blocks.
monitor: config.monitor_layout.clone(),
Map the ActiveX multi-monitor contract to GCC Client Monitor Data and the virtual desktop configuration sent to IronRDP. Validate local monitor geometry, retain negotiated topology for COM state reporting, and reject dynamic topology changes. The GDI renderer continues to present the virtual desktop as one composite framebuffer.
Keep requested topology separate from framebuffer confirmation so COM monitor APIs do not report an unhonored layout.\n\nRetain the multi-monitor resize restriction after a fallback while allowing a successfully queued single-monitor resize to clear stale topology state.\n\nAdd defaults for downstream connector configurations affected by the new monitor layout field.
Gate GCC extended client data on the server negotiation flag and\nactivate ActiveX topology only from the server Monitor Layout PDU.\n\nReject monitor counts that cannot be encoded and generate compliant\ninclusive server monitor bounds.
Store X.224 response flags in the connector instead of public state\nvariants so downstream protocol front ends retain source compatibility.
060cd48 to
1e8455b
Compare
GitHub Actions
Active
Log server monitor layouts in the viewer and clone finalization state\nfor the FFI wrapper now that it carries monitor layout data.
GitHub Actions
Active
Document the intentionally private negotiation flags field so strict Clippy preserves the established ClientConnector construction API.
GitHub Actions
Active
github-actions
Bot
added
ai-reviewed/1
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR wires GCC monitor topology negotiation end-to-end: gates Client Monitor Data / Message Channel Data / Multitransport Channel Data on EXTENDED_CLIENT_DATA_SUPPORTED (fixing prior spec-noncompliant unconditional sends), fixes inclusive/exclusive off-by-one monitor rectangle coordinates in the acceptor and a new ActiveX topology builder, replaces silent truncation with a hard encode error above 16 monitors, and adds Monitor Layout PDU handling to connection finalization plus a well-tested ActiveX multi-monitor path gated behind UseMultimon. Copy->Clone changes to activation/finalization state types are a necessary consequence of threading Option<MonitorLayoutPdu> through, not gratuitous. I independently verified the core protocol claims and found them accurate. Main residual concern: Monitor Layout PDUs arriving after finalization still hit ironrdp-session's catch-all disconnect path, newly reachable now that the client advertises SUPPORT_MONITOR_LAYOUT_PDU.
Protocol analysis: accepted — Re-checked the handoff's code claims against pr-head: encode() now errors above MONITOR_COUNT_MAX instead of the prior len()-vs-truncated-array mismatch; the acceptor's synthesized monitor uses width-1/height-1 for right/bottom; create_gcc_blocks gates monitor/message_channel/multi_transport_channel on response_flags.contains(EXTENDED_CLIENT_DATA_SUPPORTED) from the X.224 Connection Confirm; and the discrepancy note about ironrdp-session's catch-all disconnect on unhandled ShareDataPdu variants is accurate -- MonitorLayout is only handled inside WaitForResponse. All claims matched; no contradicting evidence found.
| ShareDataPdu::MonitorLayout(monitor_layout) => { | ||
| self.monitor_layout = Some(monitor_layout); | ||
| ConnectionFinalizationState::WaitForResponse | ||
| } |
There was a problem hiding this comment.
non_blocking / medium: ShareDataPdu::MonitorLayout is only handled while ConnectionFinalizationSequence remains in WaitForResponse; once finalization completes (Font Map received), ironrdp-session's active-stage dispatch (crates/ironrdp-session/src/x224/mod.rs, unmodified) has no arm for MonitorLayout and its catch-all branch returns an error, disconnecting the session. Before this PR the client never advertised SUPPORT_MONITOR_LAYOUT_PDU, so no server would send this PDU at all; this PR makes that flag reachable (crates/ironrdp-connector/src/connection.rs) for any config with a monitor_layout set, so a server that later resends a Monitor Layout PDU outside the finalization window (e.g. on a host display change) would now hard-disconnect an otherwise-healthy ActiveX multimon session. MS-RDPBCGR's cited text says the PDU is sent 'after the Demand Active PDU' but does not explicitly forbid later delivery, so this is a plausible but unconfirmed interop gap rather than a proven regression; worth a follow-up to at least ignore rather than error on a late MonitorLayout PDU.
| } | ||
| if extended_client_data_supported && config.monitor_layout.is_some() { | ||
| early_capability_flags |= ClientEarlyCapabilityFlags::SUPPORT_MONITOR_LAYOUT_PDU; | ||
| } |
There was a problem hiding this comment.
question / low: SUPPORT_MONITOR_LAYOUT_PDU is advertised only when both extended_client_data_supported and config.monitor_layout.is_some() hold, which is narrower than the flag's spec meaning (an unconditional 'client supports the Monitor Layout PDU' capability declaration, not tied to requesting a custom layout). Since connection_finalization.rs now handles MonitorLayout regardless of who set the flag, is the extra monitor_layout.is_some() gate intentional product scoping (only ActiveX multimon opts in) or should any client capable of finalization-time handling advertise the capability so servers that behave differently based on this flag alone (independent of Client Monitor Data) are not affected?
| device_scale_factor: u32, | ||
| } | ||
|
|
||
| const MAX_RDP_MONITORS: usize = 16; |
There was a problem hiding this comment.
non_blocking / low: MAX_RDP_MONITORS: usize = 16 duplicates the same protocol-defined maximum already encoded as the private MONITOR_COUNT_MAX constant in crates/ironrdp-pdu/src/gcc/monitor_data.rs (also touched by this PR). Exporting MONITOR_COUNT_MAX (or a public accessor) and reusing it here would remove the duplicated magic number and prevent the two limits from silently drifting apart in a future change.
Forward Monitor Layout PDUs received after activation through client output. This prevents an advertised capability from turning a permitted update into an unhandled PDU. Reactivation layouts now notify consumers as well.
github-actions
Bot
added
maintainer-required
|
This pull request is Please split it into focused pull requests that can each be reviewed on their own. When the parts build on each other, stacked pull requests let you open each one on top of the last without waiting for the one below to merge. Stacks require every branch to live in this repository, so from a fork, please open separate pull requests instead. Automated review resumes once the change is below the |
Negotiate the client monitor layout from UseMultimon and expose the confirmed remote topology through the ActiveX compatibility interface.
Forward Monitor Layout PDUs from activation, active sessions, and reactivation so advertised support does not terminate sessions.
Keep fallback reporting truthful when servers do not honor the request, while preserving single-monitor resize behavior and blocking multi-monitor resizing.
Do not send Client Monitor Extended Data; per-monitor DPI and orientation remain unavailable.