Conversation
GitHub Actions
Error
github-actions
Bot
added
kind/protocol
|
This pull request may overlap with #1977. Both address the client side of a server-initiated display resize via the graphics pipeline: this PR suppresses no-op Display Control resize requests and completes an in-flight resize when ResetGraphics resizes the framebuffer in the client session loop, while 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
Bot
added
the
needs-review
…#2007) Windows lists every dynamic channel it intends to move in its Soft-Sync request, including the ones the client declined with NO_LISTENER. Against a Windows 11 host the request lists channels 2, 6, 7, 8, 9, 10, 11 and 12 (CoreInput, MouseCursor, Graphics, Video, Geometry, ...), and only channel 7, the graphics pipeline, is open. `process_soft_sync_request` dropped a whole channel list as soon as one ID in it was not open. The tunnel was then never switched, and the channels the client had opened stayed on TCP while the server was already sending them on the tunnel (MS-RDPEDYC 3.2.5.3.1). Unopened channels are now skipped one by one, and the tunnel is switched for the rest. ## Testing - New `dvc::client::soft_sync_skips_channels_the_client_did_not_open` in `ironrdp-testsuite-core`. - Live, against a Windows 11 host over RDP-UDP version 2, with the viewer built from a branch that also carries the tunnel and client PRs of this series: the Soft-Sync request above now switches the tunnel, and the graphics pipeline moves onto it. ## Checks - `cargo fmt --all -- --check` - `cargo clippy --workspace --all-targets --features helper,__bench --locked -- -D warnings` - `cargo test --locked -p ironrdp-testsuite-core -p ironrdp-testsuite-extra`, plus the lib tests of the crates touched here - `cargo test --workspace --locked` on a branch that merges this PR with the other Windows interop PRs from this series - `typos` on the changed files ## Series These PRs port the Windows interop fixes and Linux backends from a downstream IronRDP fork, so the fork can be retired. Each one is based on `master` and can be reviewed and merged on its own. I also checked that all of them merge cleanly together in this order. - #2007 fix(dvc): Soft-Sync tunnel with declined channels - #2008 fix(session)!: channels and graphics on the tunnel - #2009 fix(rdpeudp): auto-detect on the tunnel - #2010 fix(graphics)!: SRL streams from Windows - #2011 fix(egfx): bitmap cache across ResetGraphics - #2012 feat(session): bandwidth measurements during the session - #2013 feat(client): graphics pipeline and RDP-UDP version options - #2014 fix(client): resize reconnects on the graphics pipeline - #2015 feat(client): transport event - #2016 feat(cliprdr): Linux clipboard backend - #2017 feat(rdpdr): printer on Linux and macOS Co-authored-by: AKolenda <testedemail2222@gmail.com>
Benoît Cortier (CBenoit)
added
automation-failed
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR #2014 adds client-side layout tracking to ResizeQueue to drop no-op Display Control resize requests and treats a framebuffer size change while a resize is in flight as completion via the graphics pipeline's ResetGraphics. Both fixes are well-motivated and correctly address the reported blanking reconnects on Windows hosts. Independent inspection confirms three remaining gaps: the completion heuristic accepts any framebuffer size change without correlating it to the in-flight request's dimensions while mark_in_flight eagerly records the layout as applied (so an out-of-band ResetGraphics or a declined request can permanently suppress later retries of an unapplied layout); scale-only resizes that leave pixel dimensions unchanged can never complete through this path and still fall back to a reconnect; and the deferred-request promotion path (pre-existing code) sends queued requests without re-checking whether they have become no-ops, reproducing the same timeout reconnect. A minor clea…
- [skeptical] No-op suppression is not applied when a deferred request is promoted for sending — medium 🟠 — crates/ironrdp-client/src/rdp.rs
asks_for_current_layout is consulted only when a fresh resize event arrives (line 3332). The promotion path (lines 4280-4295) sends a pending request unconditionally once in_flight is None and display control is ready, without re-checking whether it became a no-op. Counterexample verified in code: request A in flight, duplicate request B deferred at line 3338; A completes via the new ResetGraphics path, so framebuffer and layout now match B; B is promoted and sent, the server treats it as a no-op that never completes, and after DISPLAY_CONTROL_READY_TIMEOUT the session returns ReconnectWithNewSize at line 3641 - the same blanking reconnect this PR aims to remove. Re-applying the check in the promotion path is a small local change.
github-actions
Bot
added
ai-reviewed/1
A Display Control resize was only considered complete once the server ran a Deactivation-Reactivation Sequence. With the graphics pipeline, Windows completes it with a ResetGraphics declaring the new output size instead, and the session already follows that by resizing the framebuffer. The client kept waiting for a reactivation that never came, so after the deadline every resize ended in a reconnect that blanked the window. An in-flight resize is now complete once the framebuffer size changes. A request for the layout the server already has, which is what a window reports right after it opens, is a no-op for the server: it neither reactivates nor resets graphics, so it ran into the same deadline. Such a request is now dropped, along with any deferred request it supersedes. The comparison includes the scale factor and physical size, starting from the scale factor the connection was made with, so a DPI change at the same pixel size is still sent.
bd7c7f3 to
b336e85
Compare
|
The account paying for this security review has reached its Codex usage limits. The payer can check the Codex usage dashboard. For personal accounts, using credits requires enabling “Use credits for security reviews” in Code review settings. If you do not manage the paying account, contact this repository's admins. |
github-actions
Bot
added
automation-failed
|
Updated and rebased onto current master in b336e85. All review findings are addressed, including the deferred-request no-op check from the review body. Explicit ResetGraphics notifications now cover TCP and reliable UDP, including unchanged pixel sizes. Validation: 23 client library tests, 10 session integration tests, targeted all-targets Clippy with warnings denied, workspace formatting, and diff checks pass. The separate public API automation failure is the shared fresh dependency-resolution problem addressed by #2071; that workflow repair must land on the base branch to affect this PR. |
When the graphics pipeline is active, Windows can finish a Display Control resize with ResetGraphics instead of a deactivation/reactivation sequence. The client now consumes an explicit session notification for that reset, preventing the timeout-driven reconnect that blanks the desktop.
Rebased onto current master, including #2008's shared TCP/UDP graphics drain. Adds
ActiveStage::take_graphics_output_reset()without changing existing processing methods.Validation
rustls,rdpdrand warnings denied.git diff --checkpass.