feat: hook up resize for ironrdp-client crate #430

ibeckermayer · 2024-04-03T00:33:04Z

This commit adds the ability to resize the RDP session window on the fly using the DisplayControl channel.

It includes some known issues:

After a resize, session performance degrades.
Too large a resize can cause an InvalidChannelWidth or InvalidChannelHeight error.

See: https://www.loom.com/share/7cbfc5b0e02d4aff998939f54816d14b?sid=8e69fec2-aaa7-4a0a-ae67-d277a5000972

…receiving a ServerDeactivateAll on the I/O channel

sequences into a separate connection_activation module. This allows us to reuse them when we receive a Server Deactivate All.

`single_connect_step_read` and `single_connect_step_write`. Also adds a `ConnectionActivationSequence::reset_clone` method to aid in reusing the sequence for Deactivate All PDU handling. Passes `ConnectionActivationSequence` to `x224::Processor` to hand back upon receiving a Deactivate All PDU.

…:Windows::RDS::DisplayControl DVC to work.

…to us from the server via the BlockType in the BlockHeader, so it needn't be rigidly tracked explicitly in DecodingContext. This makes the RFX pipeline more flexible, which in turn allows us to seamlessly use it when we get another sync sequence mid-stream due to having fielded a Server Deactivate PDU.

…ess_dyvc to DrdynvcClient::process

client codebase. This uses `encode_dvc_data` to encode the DVC data responses. Currently no dynamic channels are added so the code is untested (WIP).

in the form of `DisplayControlClient`

I attempted to transfer everything in ironrdp_pdu::dvc::display into it, but it required more refactoring that I'd expected to get it to play nicely with the no_std possibility. This also removes the requirement of keeping track of the channel_id in each `DynamicVirtualChannel`, and instead lets the `DynamicChannelSet` take care of returning that as needed.

`DvcPduEncode` trait simply inherits `PduEncode`. It is a marker trait to indicate that the implementor, when encoded, is ready to be wrapped in DataFirst and/or Data PDU(s). `MonitorLayoutPdu` is a copy of the pdu by the same name in the `ironrdp-pdu` crate. It is moved (well, copied for now) to the `ironrdp-dvc` crate and implements `DvcPduEncode`.

added previously. Also creates a `DrdynvcPdu` enum that implements `SvcPduEncode`, and begins migrating `DataFirstPdu` and `DataPdu` to the new `PduEncode` paradigm. In the process, also: - Moved `DynamicVirtualChannel` and `DynamicChannelSet` from `client.rs` to `lib.rs` since they can ultimately be used by both client and server.

…rver.rs

…id of now superfluous DvcMessage<'a>

… This has been tested to work for screen resize. The refactored server side code has not been tested.

…bles testing in ironrdp-dvc by eliminating the test = false line in Cargo.toml

- Abstracts the CapabilitiesExchange and ConnectionFinalization sequences into a separate connection_activation module. This allows us to reuse them when we receive a Server Deactivate All. - Creates IoChannelPdu enum to be used to account for the potential of receiving a ServerDeactivateAll on the I/O channel

`single_connect_step_read` and `single_connect_step_write`. Also adds a `ConnectionActivationSequence::reset_clone` method to aid in reusing the sequence for Deactivate All PDU handling. Passes `ConnectionActivationSequence` to `x224::Processor` to hand back upon receiving a Deactivate All PDU.

…:Windows::RDS::DisplayControl DVC to work.

…to us from the server via the BlockType in the BlockHeader, so it needn't be rigidly tracked explicitly in DecodingContext. This makes the RFX pipeline more flexible, which in turn allows us to seamlessly use it when we get another sync sequence mid-stream due to having fielded a Server Deactivate PDU.

crates/ironrdp-connector/src/connection.rs

crates/ironrdp-connector/src/lib.rs

crates/ironrdp-displaycontrol/src/pdu/mod.rs

crates/ironrdp-pdu/src/codecs/rfx/header_messages.rs

crates/ironrdp-dvc/src/lib.rs

…nal dichotomy

…ust_display_size

CBenoit · 2024-04-26T02:11:19Z

crates/ironrdp-client/src/gui.rs

+                        // TODO: it should be possible to get the physical size here, however winit doesn't make it straightforward.
+                        // FreeRDP does it based on DPI reading grabbed via [`SDL_GetDisplayDPI`](https://wiki.libsdl.org/SDL2/SDL_GetDisplayDPI):
+                        // https://github.com/FreeRDP/FreeRDP/blob/ba8cf8cf2158018fb7abbedb51ab245f369be813/client/SDL/sdl_monitor.cpp#L250-L262
+                        let (physical_width, physical_height) = (0, 0);


question: Can’t you use this? https://docs.rs/winit/latest/winit/window/struct.Window.html#method.inner_size
Is it really fine to feed in 0s?

inner_size gives you a PhysicalSize, but that only gives you a "size represented in physical pixels." However without knowing the DPI (or other relation to a unit of length), we can't calculate the "physical size" that RDP wants, which is the width/height in millimeters.

If we hand back 0s here, they're simply ignored per the spec:

@ibeckermayer
I think passing in client (0, 0) is not optimal (it is not obvious that they will be ignored), maybe it would be great to change RdpInputEvent::Resize fields to have physical_dimensions: Option<(u32, u32)> instead of separate u32 fields? This will keep this implementation detail as close to PDU encode/decode code as possible

This could be fixed in follow-up PR, merging PR to unblock @ibeckermayer

crates/ironrdp-connector/src/connection_activation.rs

crates/ironrdp-pdu/src/codecs/rfx/header_messages.rs

Co-authored-by: Benoît Cortier <[email protected]>

pacmancoder

Looks good to me!

* Resize * ??? * ??? * Fixes to compile * Updates to latest IronRDP with dvc fix, cleans up some of the debug code * Hooks up resize listener on the client and sends changes as ClientScreenSpecs to the backend. Currently just gets logged in the WDS * Handles the new DeactivateAll ProcessorOutput * hooks up client_write_screen_resize * Renames tdp Connection Initialized to Connection Activated This message is now used both after the connection is first established and after the connection executes a Deactivation-Reactivation Sequence. This fits snugly in to our existing message flow and ensures that the fast path processor is updated appropriately whenever a Deactivation-Reactivation Sequence is executed due to a resize event. * Switch monitor orientation based on width/height * remove superfluous canvas size sync * Updates to the most up-to-date hash for IronRDP (not merged to master). Screen resize works in this commit, though its still the case that if you do so before the channel is announced by the rdp server, it will end the session * Updates to the most recent hash for IronRDP feat/dynamic-resize * Adds a mechanism to withhold sending resize requests to the RDP server until we're alerted by a new callback mechanism of the DisplayControlClient that the server has sent us its capabilities. This prevents an error where we send a resize before this event, which results either in an RDP error (if the channel is not opened) or a dropped event (if the server has not sent its capabilities yet). * Refactoring after update to the tip of Devolutions/IronRDP#424 * Adds windowOnResize to story * adds Determine Toolchain Versions and cache paths and Print versions steps to os-compatibility-test.yaml * fixes debug log formatting * Updates to local IronRDP and later IronRDP api * updates to IronRDP latest rev on Devolutions/IronRDP#430 * reverting mistaken changes * throttle -> debounce and naming changes * js renamings * updates to IronRDP master --------- Co-authored-by: Przemko Robakowski <[email protected]>

Isaiah Becker-Mayer added 30 commits March 19, 2024 20:51

Creates IoChannelPdu enum to be used to account for the potential of …

7776094

…receiving a ServerDeactivateAll on the I/O channel

Abstracts the CapabilitiesExchange and ConnectionFinalization

4dbb2b3

sequences into a separate connection_activation module. This allows us to reuse them when we receive a Server Deactivate All.

Sets desktop_resize_flag to true which is required for the Microsoft:…

55159db

…:Windows::RDS::DisplayControl DVC to work.

Adds todo in ironrdp-web

717d020

Proof of concept for the transition from legacy x224::Processor::proc…

d07d522

…ess_dyvc to DrdynvcClient::process

Adds a DynamicChannelSet (similar to the StaticChannelSet) to the DVC

5c22b53

client codebase. This uses `encode_dvc_data` to encode the DVC data responses. Currently no dynamic channels are added so the code is untested (WIP).

Adds a CommonPdu to consolidate common client/server pdu's for dvcs

8a5c5f1

Gets a DvcClientProcessor in full working condition

ffb4465

in the form of `DisplayControlClient`

Removes now no-longer-necesary is_drdynvc. Also fix compilation in se…

e93cea0

…rver.rs

Adds CreateResponsePdu, ClosePdu, and CapabilitiesResponsePdu, gets r…

b382e81

…id of now superfluous DvcMessage<'a>

Removes all legacy ironrdp-pdu/src/rdp/vc/dvc/ pdus from ironrdp-dvc.…

8ec38e1

… This has been tested to work for screen resize. The refactored server side code has not been tested.

Moves relevant DataFirst tests from ironrdp-pdu to ironrdp-dvc

c72391a

Moves relevant Data tests from ironrdp-pdu to ironrdp-dvc

c384c40

Simplify naming in tests

28c9f68

Moves relevant Create tests from ironrdp-pdu to ironrdp-dvc. Also ena…

266c735

…bles testing in ironrdp-dvc by eliminating the test = false line in Cargo.toml

Moves relevant Close tests from ironrdp-pdu to ironrdp-dvc.

856a0aa

Moves relevant Capabilities tests from ironrdp-pdu to ironrdp-dvc.

0dea439

Fix test panics

69cc89b

Delete as much legacy code as possible

ead24a5

Sets desktop_resize_flag to true which is required for the Microsoft:…

c4afa98

…:Windows::RDS::DisplayControl DVC to work.

Merge branch 'feat/deactivate-all' into feat/dynamic-resize

422a436

Consolidates the display module into ironrdp-dvc crate.

5f0840b