Skip to content

feat(server): let RdpServerDisplay report its monitor count - #1918

Open
Greg Lamberson (glamberson) wants to merge 4 commits into
Devolutions:masterfrom
lamco-admin:feat/displaycontrol-monitor-count
Open

feat(server): let RdpServerDisplay report its monitor count#1918
Greg Lamberson (glamberson) wants to merge 4 commits into
Devolutions:masterfrom
lamco-admin:feat/displaycontrol-monitor-count

Conversation

@glamberson

Copy link
Copy Markdown
Contributor

Summary

  • Stacked on feat(displaycontrol): let a server handler override its capabilities #1917: DisplayControlBackend (ironrdp-server's internal DisplayControlHandler implementor) never overrode capabilities(), so it fell through to the single-monitor default regardless of how many monitors the consumer's RdpServerDisplay actually serves.
  • Added a default RdpServerDisplay::monitor_count() method (returns 1, matching today's behavior), fetched once alongside size() before the dynamic channels are attached.
  • Threaded the count into DisplayControlBackend so its capabilities() now reports the real value instead of the hardcoded literal.
  • A consumer serving more than one monitor can override monitor_count() to report the real total.

Validation

cargo xtask check fmt/lints/tests/typos/locks all pass.

Notes

@github-actions github-actions Bot added triage/overlap This issue or pull request already exists or overlaps kind/protocol Affects RDP or related protocol behavior maintainer-required Maintainer review or intervention is required risk/medium Behavioral change that does not substantially alter a core public API scope/core Touches the core architectural tier size/S Size: up to 199 counted lines and 5 files; exceeds XS in either measure labels Sep 8, 2026
@github-actions

github-actions Bot commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

Potential duplicate detected: #1917.

The ironrdp-displaycontrol hunk reproduces listed PR #1917: a defaulted DisplayControlHandler::capabilities() replacing the hardcoded (1, 3840, 2400) in start(), with the same method name, default values, and near-identical doc wording. This PR is a superset adding the server-side monitor_count plumbing that #1917 lacks, but its core change matches that listed pull request.

Maintainer review is required.

@CBenoit Benoît Cortier (CBenoit) removed triage/overlap This issue or pull request already exists or overlaps maintainer-required Maintainer review or intervention is required labels Sep 8, 2026
DisplayControlServer::start() hardcoded DisplayControlCapabilities::new(1,
3840, 2400), giving every server a fixed single-monitor cap with no way
to advertise more. Added a default trait method mirroring the existing
monitor_layout() pattern, so a handler serving multiple monitors can
override it; anything that doesn't override it keeps today's exact
behavior.
DisplayControlBackend, ironrdp-server's internal DisplayControlHandler
implementor, never overrode capabilities() (added by the previous
commit), so it fell through to the single-monitor default regardless
of how many monitors the consumer's RdpServerDisplay actually serves.

Added a default RdpServerDisplay::monitor_count() method (1, matching
today's behavior), fetched once alongside size() before the dynamic
channels are attached, and threaded into DisplayControlBackend so its
capabilities() now reports the real count.
@glamberson
Greg Lamberson (glamberson) force-pushed the feat/displaycontrol-monitor-count branch from b5fd825 to a17aebc Compare September 9, 2026 21:36
@github-actions github-actions Bot added triage/overlap This issue or pull request already exists or overlaps maintainer-required Maintainer review or intervention is required risk/high Substantial core public API impact, or fail-closed triage; needs maintainer-level scrutiny and removed risk/medium Behavioral change that does not substantially alter a core public API labels Sep 9, 2026

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Additive, low-risk change: RdpServerDisplay::monitor_count() (default 1) is snapshotted before channels attach and threaded through DisplayControlBackend so the DISPLAYCONTROL_CAPS_PDU advertises the real monitor count instead of a hardcoded 1. Default wire behavior is unchanged and the value is conformant under MS-RDPEDISP. Three accepted low-severity findings: the fallback rejects only counts above 1024 (0 is advertised as-is), the new wiring and fallback branch are untested, and monitor_count could be fetched inside attach_channels instead of threaded as a parameter. The displaycontrol capabilities() hunk belongs to stacked base PR #1917.

  1. [code-compressor] Fetch monitor_count inside attach_channels instead of threading it as a parameter — low 🟡 — crates/ironrdp-server/src/server.rs
    attach_channels already pulls every other backend input from self (cliprdr, sound, rdpdr factories, display Arc), and its only two call sites (serve_negotiated and run_connection_inner) are async with no other callers. Making it async and reading the count as its first statement removes the new parameter, both duplicated pre-fetch lines, and the thread-through, with identical behavior: the count is still read before any channel is attached or negotiation starts, and the extra await cannot deadlock because the display mutex is not held on these paths.

Comment thread crates/ironrdp-server/src/server.rs
Comment thread crates/ironrdp-server/src/server.rs
@github-actions github-actions Bot added ai-reviewed/1 One automated review completed and removed maintainer-required Maintainer review or intervention is required labels Sep 11, 2026
DisplayControlCapabilities::new only rejects max_num_monitors > 1024,
so an override returning 0 passed validation and encoded
MaxNumMonitors=0, advertising a server that supports no monitors.
Checked explicitly and folded into the existing warn-and-fall-back-to-1
path.
@github-actions github-actions Bot added the scope/cross-cutting Spans multiple architectural boundaries label Sep 11, 2026
@github-actions

github-actions Bot commented Sep 11, 2026

Copy link
Copy Markdown
Contributor

This pull request may overlap with #1917.

Both PRs replace DisplayControlServer::start()'s hardcoded (1, 3840, 2400) capabilities with a default DisplayControlHandler::capabilities() trait method that multi-monitor handlers can override, keeping prior behavior by default. #1917's summary matches this diff's displaycontrol changes almost exactly; this PR additionally adds RdpServerDisplay::monitor_count() plumbing in ironrdp-server.

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).

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Additive, behavior-preserving change: a defaulted RdpServerDisplay::monitor_count() and a defaulted DisplayControlHandler::capabilities() replace the hardcoded single-monitor caps PDU, with the count sampled once per connection and threaded into the private DisplayControlBackend, including warn-and-fallback for 0 and >1024 (verified: the PDU constructor rejects only >1024; 0 passes, so the explicit clamp is needed). Both attach paths (normal and preemption-winner) fetch the count; negotiate_candidate attaches no channels by design; tokio Mutex guards are per-statement temporaries, so no deadlock; all in-repo display implementors rely on defaults and keep identical wire behavior. Published two low-severity maintainability findings (doc semantics ambiguity, duplicated single-monitor constants); rejected two compression candidates that would remove a deliberately added diagnostic or rest on savings the sync capabilities() hook makes impossible.

Comment thread crates/ironrdp-server/src/display.rs Outdated
Comment thread crates/ironrdp-server/src/server.rs
@github-actions github-actions Bot added ai-reviewed/2 Final automated review completed and removed ai-reviewed/1 One automated review completed labels Sep 12, 2026
Reworded RdpServerDisplay::monitor_count()'s doc comment to state it's
a per-session capacity ceiling (MS-RDPEDISP MaxNumMonitors), not a
current topology report, since the PR body's "report the real total"
phrasing read as the latter and could mislead a consumer into treating
it as a live count that goes stale after the value is sampled once.

Added DisplayControlCapabilities::single_monitor(), following the
crate's existing named-constructor convention, and switched both
places that had reimplemented the (1, 3840, 2400) literal plus its
expect message to use it instead: the trait's default capabilities()
and the out-of-range fallback in DisplayControlBackend.
@github-actions github-actions Bot added maintainer-required Maintainer review or intervention is required risk/medium Behavioral change that does not substantially alter a core public API and removed risk/high Substantial core public API impact, or fail-closed triage; needs maintainer-level scrutiny labels Sep 12, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ai-reviewed/2 Final automated review completed kind/protocol Affects RDP or related protocol behavior maintainer-required Maintainer review or intervention is required risk/medium Behavioral change that does not substantially alter a core public API scope/core Touches the core architectural tier scope/cross-cutting Spans multiple architectural boundaries size/S Size: up to 199 counted lines and 5 files; exceeds XS in either measure triage/overlap This issue or pull request already exists or overlaps

Development

Successfully merging this pull request may close these issues.

2 participants