Skip to content

feat(acceptor): honor the client-requested desktop size#1373

Open
clintcan wants to merge 1 commit into
Devolutions:masterfrom
clintcan:feat/honor-client-desktop-size
Open

feat(acceptor): honor the client-requested desktop size#1373
clintcan wants to merge 1 commit into
Devolutions:masterfrom
clintcan:feat/honor-client-desktop-size

Conversation

@clintcan

Copy link
Copy Markdown
Contributor

Problem

An ironrdp-server always serves the desktop size reported by its
RdpServerDisplay, regardless of what the client asked for. When that size
differs from the client's actual display resolution, the client rescales every
decoded frame — which on mstsc costs noticeable typing latency and, with H.264
over EGFX, contributes to audio drift. The only workaround today is to know the
client's resolution out-of-band and hard-code the display handler to match it.

The natural-looking hook — RdpServerDisplay::request_initial_size, added in
#1083 — can't actually be used to detect the client's request. It's fed the
desktop size from the client's Confirm Active, but per [MS-RDPBCGR]
2.2.1.13.2 a conformant client copies the server's Demand Active size into
its Confirm Active and echoes that back. Verified live: FreeRDP launched with
/size:1024x768 echoes the server's size, not 1024x768 (this matches
FreeRDP's rdp_apply_bitmap_capability_set, and mstsc follows the same
desktopResizeFlag contract). The client's own requested resolution is carried
only in the GCC Client Core Data of the MCS Connect Initial — which the
acceptor parses and currently discards — and the acceptor commits a size in
Demand Active before any server-side code runs.

(This is also, I think, the substance of the question Benoît Cortier (@CBenoit) raised on #1083
about whether request_initial_size during reactivation is the right shape.)

Approach

Adopt the client's Core Data desktop size in the acceptor, before Demand
Active is sent, gated behind an opt-in flag. Because it happens pre-Demand-
Active, the session is negotiated at the client's resolution from the start —
no Deactivation-Reactivation resize round trip. The display handler still
learns the negotiated size through the existing request_initial_size call
(the Confirm Active echo now equals the adopted size).

Opt-in, default off → no behavior change for existing servers.

API

  • ironrdp-acceptor: new Acceptor::set_honor_client_desktop_size(bool)
    (additive). When set, the BasicSettingsWaitInitial step adopts the Core
    Data size if it's within the protocol-legal range (200..=8192) and writes it
    into the server Bitmap capability set before Demand Active. Also factors the
    existing bitmap-capset desktop-size mutation (previously inline in
    new_deactivation_reactivation) into a shared set_bitmap_desktop_size
    helper, and adds a small pure validate_desktop_size helper.
  • ironrdp-server: new builder method
    RdpServerBuilder::with_honor_client_desktop_size(bool), backed by a new
    RdpServerOptions::honor_client_desktop_size field, forwarded to each
    connection's acceptor in run_connection.

Notes / questions for review

  1. Where the server option lives. I put it on RdpServerOptions (the
    config bag, next to max_request_size) rather than threading a new
    parameter through RdpServer::new as display_suppressed does. That adds a
    public field to RdpServerOptions, which is a minor-version breaking change
    for anyone constructing it by struct literal (the builder is unaffected).
    Happy to move it to a new parameter instead if you'd prefer to keep
    RdpServerOptions stable — let me know which you'd like.
  2. Tests. ironrdp-acceptor's lib sets test = false, so I didn't add
    inline unit tests (they wouldn't run). The two new helpers are pure and
    trivial; I verified the end-to-end behavior against real clients (below). If
    there's a preferred place for acceptor coverage, I'll add it there.

Testing

Built and clippy-clean (-D warnings) for ironrdp-acceptor and
ironrdp-server --features egfx against current master. Exercised through a
downstream server with real clients:

  • sdl-freerdp /size:1024x768 — legacy bitmap path, session negotiated at
    1024×768 from the first frame.
  • sdl-freerdp /size:1280x800 then reconnect /size:800x600 — H.264/EGFX,
    surface + encoder created at the requested size each connection.
  • mstsc full-screen on a 1920×1080 monitor — session served at 1920×1080,
    client presents 1:1, typing latency gone.

An ironrdp-server always serves the desktop size reported by its
RdpServerDisplay, regardless of what the client asked for. When that size
differs from the client's display resolution, the client rescales every
decoded frame -- which on mstsc costs typing latency and, with H.264 over
EGFX, contributes to audio drift.

The client's requested resolution is carried only in the GCC Client Core Data
of the MCS Connect Initial. The size echoed back in the client's Confirm
Active is, per MS-RDPBCGR 2.2.1.13.2, the value the client copied from the
server's Demand Active, so it cannot reveal what the client asked for
(verified: FreeRDP /size:1024x768 echoes the server's size). And the acceptor
commits a size in Demand Active before any server-side code runs.

So adopt the Core Data size in the acceptor, before Demand Active, behind an
opt-in flag. The session is then negotiated at the client's resolution from
the start, with no Deactivation-Reactivation resize. The display handler still
observes the negotiated size through request_initial_size.

- ironrdp-acceptor: add Acceptor::set_honor_client_desktop_size (additive);
  adopt the Core Data size (when within the protocol-legal 200..=8192 range)
  in BasicSettingsWaitInitial and write it into the server Bitmap capability
  set before Demand Active. Factor the existing inline bitmap-capset
  desktop-size mutation into a shared set_bitmap_desktop_size helper.
- ironrdp-server: add RdpServerBuilder::with_honor_client_desktop_size, backed
  by a new RdpServerOptions::honor_client_desktop_size field, forwarded to the
  acceptor in run_connection.

Opt-in, default off, so existing servers are unchanged.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

1 participant