feat(acceptor): honor the client-requested desktop size#1373
Open
clintcan wants to merge 1 commit into
Open
Conversation
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
An
ironrdp-serveralways serves the desktop size reported by itsRdpServerDisplay, regardless of what the client asked for. When that sizediffers 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:1024x768echoes the server's size, not1024x768(this matchesFreeRDP's
rdp_apply_bitmap_capability_set, and mstsc follows the samedesktopResizeFlagcontract). The client's own requested resolution is carriedonly 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_sizeduring 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_sizecall(the Confirm Active echo now equals the adopted size).
Opt-in, default off → no behavior change for existing servers.
API
ironrdp-acceptor: newAcceptor::set_honor_client_desktop_size(bool)(additive). When set, the
BasicSettingsWaitInitialstep adopts the CoreData 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 sharedset_bitmap_desktop_sizehelper, and adds a small pure
validate_desktop_sizehelper.ironrdp-server: new builder methodRdpServerBuilder::with_honor_client_desktop_size(bool), backed by a newRdpServerOptions::honor_client_desktop_sizefield, forwarded to eachconnection's acceptor in
run_connection.Notes / questions for review
RdpServerOptions(theconfig bag, next to
max_request_size) rather than threading a newparameter through
RdpServer::newasdisplay_suppresseddoes. That adds apublic field to
RdpServerOptions, which is a minor-version breaking changefor anyone constructing it by struct literal (the builder is unaffected).
Happy to move it to a
newparameter instead if you'd prefer to keepRdpServerOptionsstable — let me know which you'd like.ironrdp-acceptor's lib setstest = false, so I didn't addinline 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) forironrdp-acceptorandironrdp-server --features egfxagainst currentmaster. Exercised through adownstream server with real clients:
/size:1024x768— legacy bitmap path, session negotiated at1024×768 from the first frame.
/size:1280x800then reconnect/size:800x600— H.264/EGFX,surface + encoder created at the requested size each connection.
client presents 1:1, typing latency gone.