feat(server): handle SuppressOutput / RefreshRectangle and expose state by clintcan · Pull Request #1319 · Devolutions/IronRDP

clintcan · 2026-05-26T20:49:15Z

Summary

The server currently logs SuppressOutput and RefreshRectangle as Unexpected share data pdu and drops them. That leaves a server streaming high-bitrate EGFX/H.264 frames into a minimized client which accumulates them; on refocus the client must chew through the backlog before it can present the current frame, locking up its input dispatch for seconds.

This PR pattern-matches the two PDUs and flips a shared Arc<AtomicBool> so display backends can hold a clone and skip frame emission while the client has signaled it doesn't need updates.

mstsc sends `SuppressOutput { desktop_rect: None }` on minimize.
mstsc sends `SuppressOutput { Some(rect) }` and/or `RefreshRectangle` on refocus.

API

Two new methods on `RdpServer`:

`display_suppressed_handle() -> Arc` returns a clone of the shared flag so a backend can read it (typical pattern: backend spawns a thread that polls and skips encode/ship while the flag is `true`).
`set_display_suppressed_handle(handle: Arc)` replaces the internally-created flag with one the caller already shared with the display backend before constructing the server. Necessary because the backend is moved into the builder before the server exists; without a setter there's no way to give the same `Arc` to both ends.

Backward compatibility

`RdpServer::new()` signature unchanged (new field initialized internally).
Builder unchanged.
For existing users who don't call the new methods: zero behavior change. Only the log for these two PDUs drops from `warn` to `debug` (Unexpected → handled-and-logged).
All existing tests pass.

Real-world result

Verified on macOS / mstsc with H.264 over EGFX: minimizing mstsc for several minutes used to leave the desktop frozen and unresponsive for several seconds on refocus while it processed the buffered frames. With the flag honored downstream, refocus is instant. Shipped in macrdp v0.5.47.

Test plan

`cargo build -p ironrdp-server --features egfx`
`cargo clippy -p ironrdp-server --features egfx --all-targets -- -D warnings`
`cargo test -p ironrdp-server --features egfx` (5/5 pass)
`rustup run nightly cargo fmt --all -- --check`
Real-world verification on mstsc + macrdp (long minimize + refocus, disconnect/reconnect, cargo-build-under-load with YouTube)

The server currently logs `SuppressOutput` and `RefreshRectangle` as `Unexpected share data pdu` and drops them. That leaves a server streaming high-bitrate EGFX/H.264 frames into a minimized client which accumulates them; on refocus the client must chew through the backlog before it can present the current frame, locking up its input dispatch for seconds. Pattern-match the two PDUs and flip a shared `Arc<AtomicBool>` so display backends can hold a clone and skip frame emission while the client has signaled it doesn't need updates. mstsc sends `SuppressOutput { desktop_rect: None }` on minimize and `SuppressOutput { Some(rect) }` (and/or `RefreshRectangle`) on refocus. Exposed via: - `RdpServer::display_suppressed_handle()` returns a clone of the shared `Arc` so a backend can read it (typical pattern: backend spawns a thread that polls and skips encode/ship while the flag is set). - `RdpServer::set_display_suppressed_handle(handle)` replaces the internally-created flag with one the caller already shared with the display backend before constructing the server. Necessary because the backend is moved into the builder before the server exists; without a setter there's no way to give the same `Arc` to both ends. No behavior change for existing users: backends that don't read the flag keep working exactly as before; only the warning log for these two PDUs drops to `debug`. Real-world result on macOS / mstsc with H.264 over EGFX: minimizing mstsc for several minutes used to leave the desktop frozen and unresponsive for several seconds on refocus while it processed the buffered frames. With the flag honored, refocus is instant.

Copilot

Pull request overview

Adds explicit handling for SuppressOutput and RefreshRectangle share-data PDUs in the RDP server, exposing the resulting "display suppressed" state via a shared Arc<AtomicBool> so display backends (notably high-bitrate EGFX/H.264) can pause frame emission while a client is minimized and avoid a multi-second backlog stall on refocus.

Changes:

Add display_suppressed: Arc<AtomicBool> field to RdpServer, initialized to false in new().
Add display_suppressed_handle() getter and set_display_suppressed_handle() setter on RdpServer.
In handle_io_channel_data(), match SuppressOutput (set flag based on desktop_rect.is_none()) and RefreshRectangle (clear flag), instead of falling through to the Unexpected share data pdu warn branch.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

…onnection Two follow-ups from review: 1. **Advertise `refresh_rect_support` and `suppress_output_support` in the General Capability Set.** Per MS-RDPBCGR 2.2.7.1.1, a compliant client only sends these PDUs when the server advertises support. mstsc sends them regardless, but FreeRDP and other spec-following clients don't — they would never benefit from this fix without the flags. Setting both to `true` now that the handler is in place. 2. **Reset `display_suppressed` at the start of `run_connection`.** If a client disconnects while the flag is `true` (e.g., closes mstsc while minimized so the matching resume PDU never arrives), the next connection would start in a suppressed state and the display backend would silently drop frames until a fresh `RefreshRectangle` or `SuppressOutput { Some(rect) }` arrived. Resetting on each new connection also covers backends that share an externally-created `Arc<AtomicBool>` via `set_display_suppressed_handle()` — they get the same per-connection clean slate. Both addressing Copilot review comments on Devolutions#1319.

Some clients (notably mstsc) send `SuppressOutput { desktop_rect: None }` during their connect handshake *before* their display surface is fully initialized. A backend that blindly honors the flag will block that first frame and leave the client with a half-initialized surface that doesn't recover on un-suppress — visible as a frozen desktop on first connect. Now that the server advertises support for these PDUs (so even spec- following clients like FreeRDP will start sending them), surface the caveat on `display_suppressed_handle()` so downstream backends know to defer honoring the flag until after the first frame has been delivered, and to debounce transient flaps under wire pressure.

Benoît Cortier (CBenoit) · 2026-05-27T11:14:27Z

+    /// Replace the server's "display suppressed" flag with one the caller
+    /// already shared with the display backend before the server was
+    /// constructed.
+    ///
+    /// Use this when the display backend needs to read the same flag the
+    /// per-connection PDU handler writes to: create one `Arc<AtomicBool>`
+    /// in the application, hand a clone to the display, then call this to
+    /// swap the server's internal default for that shared instance. Must
+    /// be called before any client connects (the flag is read by every
+    /// per-connection task; replacing it mid-session is racy).
+    pub fn set_display_suppressed_handle(&mut self, handle: Arc<AtomicBool>) {
+        self.display_suppressed = handle;
+    }


It’s acknowledged in the function’s docstring, but I don’t think it’s a great API because it’s easy to misuse.

I would suggest to take one of the following approaches:

Remove this method from RdpServer, and instead add a new method to RdpServerBuilder (it’s impossible for a session to be started before RdpServer is built; my preferred approach)

Add a runtime check, either with assert! or return a Result::Err when this method is called while a session is running.

Per review on Devolutions#1319: the `set_display_suppressed_handle` setter on `RdpServer` was easy to misuse (caller could race a running session by swapping the flag mid-connection). Moving it to the builder removes that footgun via the typestate — by the time `RdpServer` exists, the builder is consumed, so the handle can only be injected at construction. - Remove `RdpServer::set_display_suppressed_handle`. - Add `RdpServerBuilder::with_display_suppressed_handle(Arc<AtomicBool>)`. - Plumb through `BuilderDone` → `RdpServer::new()` as a new `Option<Arc<AtomicBool>>` parameter (`None` keeps the existing default of allocating an internal flag, preserving behavior for callers that use `display_suppressed_handle()` after construction). - `RdpServer::new` is now flagged with `#[expect(clippy::too_many_arguments)]` since it's reached the threshold; documented as an internal-detail positional ctor reached via the builder.

Without --features egfx, RdpServer::new has 7 args (the lint threshold), so the unconditional #[expect] from the previous commit was unfulfilled and failed -D warnings. cfg_attr keeps the expect strict only when egfx adds the 8th parameter.

Benoît Cortier (CBenoit) · 2026-05-27T13:18:18Z

+    #[expect(
+        clippy::too_many_arguments,
+        reason = "called via the builder; positional parameters are an internal detail"
+    )]
    pub fn new(


thought: I agree with the idea here, but new is currently pub, so we need to change that to pub(crate) in a follow up PR for the point to hold

Benoît Cortier (CBenoit)

LGTM! Thank you

Benoît Cortier (CBenoit) requested a review from Copilot May 27, 2026 09:21

Copilot started reviewing on behalf of Benoît Cortier (CBenoit) May 27, 2026 09:22 View session

Copilot AI reviewed May 27, 2026

View reviewed changes

Comment thread crates/ironrdp-server/src/server.rs

Comment thread crates/ironrdp-server/src/server.rs

Clint Christopher Canada added 2 commits May 27, 2026 18:40

Benoît Cortier (CBenoit) reviewed May 27, 2026

View reviewed changes

Clint Christopher Canada added 2 commits May 27, 2026 19:54

clintcan closed this May 27, 2026

clintcan reopened this May 27, 2026

clintcan force-pushed the feat-server-handle-suppress-output branch from a1bb1fd to 97f1b30 Compare May 27, 2026 12:56

Benoît Cortier (CBenoit) reviewed May 27, 2026

View reviewed changes

Benoît Cortier (CBenoit) approved these changes May 27, 2026

View reviewed changes

Benoît Cortier (CBenoit) merged commit aa7ff67 into Devolutions:master May 27, 2026
42 checks passed

Greg Lamberson (glamberson) mentioned this pull request May 27, 2026

feat(server): handle SuppressOutput, RefreshRectangle, FrameAcknowledge PDUs #1239

Closed

4 tasks

devolutionsbot mentioned this pull request May 28, 2026

chore(release): prepare for publishing #1340

Open

Greg Lamberson (glamberson) mentioned this pull request May 31, 2026

feat(server): expose NetworkAutoDetect RTT via a shared handle #1346

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(server): handle SuppressOutput / RefreshRectangle and expose state#1319

feat(server): handle SuppressOutput / RefreshRectangle and expose state#1319
Benoît Cortier (CBenoit) merged 5 commits into
Devolutions:masterfrom
clintcan:feat-server-handle-suppress-output

clintcan commented May 26, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Benoît Cortier (CBenoit) May 27, 2026

Uh oh!

Benoît Cortier (CBenoit) May 27, 2026 •

edited

Loading

Uh oh!

Benoît Cortier (CBenoit) left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

Conversation

clintcan commented May 26, 2026

Summary

API

Backward compatibility

Real-world result

Test plan

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Benoît Cortier (CBenoit) May 27, 2026

Choose a reason for hiding this comment

Uh oh!

Benoît Cortier (CBenoit) May 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Benoît Cortier (CBenoit) left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

Benoît Cortier (CBenoit) May 27, 2026 •

edited

Loading