Improve stability of xorgxrdp when resolution changes during a session ("dynamic resolution") #183

Nexarian · 2021-02-25T06:22:42Z

During dynamic resize, it's possible for the capture timer to trigger while the screen's memory buffer is not in a proper state for the given codec used. The resize algorithm updates the screen to be compatible for "vanilla" remoting with no codecs, but both RFX and H264 have special configurations to the shmem (Shared Memory) buffer that will cause the capture routines to crash if they run before the resizing handshake sequence is complete.

This patches the issue in two ways:

While clientCon->suppress_output is used by the client to configure suppression remotely in special circumstances, previously clientCon->client_info.suppress_output was unused. So repurpose that for internal driver use to prevent capture from occurring while the buffer is in an indeterminate state.
Update the _rdpClientCon data structure to include an enumeration that gives a high-level status of the shmem buffer. I may have missed places in the driver where this needs to be updated as well, but for resizing it works.

I could envision further enhancements to the _rdpClientCon data structure that make this easier to access, such as breaking out the "shmem" and all related variables into a child struct that is part of _rdpClientCon, so the root namespace of the structure is not as cluttered (It's starting to get to be a lot).

I've tested this extensively with FreeRDP and the /rfx switch (Not /gfx just yet as that's still in development), and this should not change the way the driver behaves in any way until the changes to xrdp itself are added that enable resizing. I've also verified this works with the Microsoft Remote Desktop Mac OS and Windows 10 clients. Note that MSTSC doesn't support the feature.

matt335672 · 2021-02-25T10:18:55Z

Thanks for this. I'll take a few days for me to get round to looking at it I'm afraid but I will definitely do so.

matt335672

After removing the client_info.suppress_output references, this reads much easier.

matt335672 · 2021-03-01T10:24:22Z

module/rdpClientCon.h

@@ -49,6 +49,14 @@ struct rdpup_os_bitmap
    int stamp;
 };

+enum sharedMemoryStatus {


Coding standard on the wiki tells us not to use camelCase. Suggest shared_memory_status instead.

I know shed-loads of stuff in this file already uses camelCase, but I think we ought to stick to the standard.

Yep, updated!

matt335672 · 2021-03-01T10:26:48Z

module/rdpClientCon.h

@@ -49,6 +49,14 @@ struct rdpup_os_bitmap
    int stamp;
 };

+enum sharedMemoryStatus {
+    UNINITIALIZED = 0,


For all of these values I'd suggest a small prefix (e.g. SMS_ for shared memory status so we get things like SMS_ACTIVE). Two reasons for this

the enumeration values are in the global namespace rather than the enum namespace, so this helps avoid name collisions on common adjectives like UNINITIALIZED or ACTIVE

using old-school tools like grep, it's easy to find all instances of the enumerated type.

Good call, updated!

Initially I got a bit confused by "SMS", as shared memory is usually addressed as "SHM". I'm nitpicking I know but if we are referencing old-school tools like grep then maybe we can use "SHM" or "SHMS" where I have a slight preference for the first ;-)

Hi @bolkedebruin - thanks for taking the time to look at this.

From my perspective, any prefix is better than none, so both of your suggestions look fine to me - I pulled SMS_ out of my hat as a suggestion. I think this is @Nexarian's call.

If SHM is a C-style convention I'm happy to adopt it. SMS feels weird to me because it sounds like text messaging, I will update.

https://man7.org/linux/man-pages/man3/shm_open.3.html

Think so ;-)

matt335672 · 2021-03-03T10:33:07Z

@metalefty, @jsorg71 - could one of you comment on this PR?

I'm happy with the general shape of it, but I'd really appreciate another pair of eyes looking at this.

Thanks

- Based on https://github.com/jsorg71/xrdp/tree/dynamic_monitor - Tested with xorgxrdp - Tested with vnc - Only works with single monitor. Depends on neutrinolabs/xorgxrdp#183

- Based on https://github.com/jsorg71/xrdp/tree/dynamic_monitor - Tested with xorgxrdp - Tested with vnc - Only works with single monitor. - Update documentation to clarify the difference between MSTSC and Microsoft Remote Desktop. Depends on neutrinolabs/xorgxrdp#183

bolkedebruin · 2021-03-12T09:44:58Z

I'll be testing this with our user group (~400 users).

matt335672 · 2021-03-12T10:57:24Z

@bolkedebruin - thanks again for your generous offer of a testing community. I'm assuming you're also including neutrinolabs/xrdp#1820 in this?

Do you have any particular timescales in mind? Let us know if there's anything we can do to help from this end.

bolkedebruin · 2021-03-12T11:07:11Z

No problem. Yes of course we include that one as well and #167 sans the scroll detection. We will roll it out today to a small group to see if it actually works and then to the whole community next week.

- Based on https://github.com/jsorg71/xrdp/tree/dynamic_monitor - Tested with xorgxrdp - Tested with vnc - Only works with single monitor. - Update documentation to clarify the difference between MSTSC and Microsoft Remote Desktop. Depends on neutrinolabs/xorgxrdp#183

- Based on https://github.com/jsorg71/xrdp/tree/dynamic_monitor - Tested with xorgxrdp - Tested with vnc - Only works with single monitor. - Update documentation to clarify the difference between MSTSC and Microsoft Remote Desktop. - Does not include compatibility with /gfx at this time, which is still in testing. - Updates to include ms-rdpedisp.h header for the 2.2.2 specification of the protocol. Depends on neutrinolabs/xorgxrdp#183

…n ("dynamic resolution") During dynamic resize, it's possible for the capture timer to trigger while the screen's memory buffer is not in a proper state for the given codec used. This patches the issue so that the xorgxrdp driver won't crash, but I could envision further enhancements to the _rdpClientCon data structure that make this easier to access.

- Based on https://github.com/jsorg71/xrdp/tree/dynamic_monitor - Tested with xorgxrdp - Tested with vnc - Only works with single monitor. - Update documentation to clarify the difference between MSTSC and Microsoft Remote Desktop. - Does not include compatibility with /gfx at this time, which is still in testing. - Updates to include ms-rdpedisp.h header for the 2.2.2 specification of the protocol. - Adds new dynamic_monitor_layout struct that shares the number of monitors with xrdp_client_info.h Depends on neutrinolabs/xorgxrdp#183

- Based on https://github.com/jsorg71/xrdp/tree/dynamic_monitor - Tested with xorgxrdp - Tested with vnc - Only works with single monitor. - Update documentation to clarify the difference between MSTSC and Microsoft Remote Desktop. - Does not include compatibility with /gfx at this time, which is still in testing. - Updates to include ms-rdpedisp.h header for the 2.2.2 specification of the protocol. - Adds new dynamic_monitor_layout struct that shares the number of monitors with xrdp_client_info.h - Modification to the original resize setup that works with /gfx. Depends on neutrinolabs/xorgxrdp#183

- Based on https://github.com/jsorg71/xrdp/tree/dynamic_monitor - Tested with xorgxrdp - Tested with vnc - Only works with single monitor. - Update documentation to clarify the difference between MSTSC and Microsoft Remote Desktop. - Does not include compatibility with /gfx at this time, which is still in testing. - Updates to include ms-rdpedisp.h header for the 2.2.2 specification of the protocol. - Adds new dynamic_monitor_layout struct that shares the number of monitors with xrdp_client_info.h Depends on neutrinolabs/xorgxrdp#183

- Based on https://github.com/jsorg71/xrdp/tree/dynamic_monitor - Tested with xorgxrdp - Tested with vnc - Only works with single monitor. - Update documentation to clarify the difference between MSTSC and Microsoft Remote Desktop. - Compatible with NVENC and xorgxrdp_helper - Compatible with Nvidia. - Updates to include ms-rdpedisp.h header for the 2.2.2 specification of the protocol. - State machine for resizing. - Mechanisms to make sure the key frame is always sent on resize. - Adds new struct that shares the number of monitors with xrdp_client_info.h - Modification to the original resize setup that works with /gfx. - YAMI compat. - Updating to librfxcodec branch that is also a merge from mainline. Depends on neutrinolabs/xorgxrdp#183

- Based on https://github.com/jsorg71/xrdp/tree/dynamic_monitor - Tested with xorgxrdp - Tested with vnc - Only works with single monitor. - Update documentation to clarify the difference between MSTSC and Microsoft Remote Desktop. - Compatible with NVENC and xorgxrdp_helper - Compatible with Nvidia. - Updates to include ms-rdpedisp.h header for the 2.2.2 specification of the protocol. - State machine for resizing. - Mechanisms to make sure the key frame is always sent on resize. - Adds new struct that shares the number of monitors with xrdp_client_info.h - Modification to the original resize setup that works with /gfx. - YAMI compat. - Updating to librfxcodec branch that is also a merge from mainline. - Made sure RFX progressive works. Depends on neutrinolabs/xorgxrdp#183

- Based on https://github.com/jsorg71/xrdp/tree/dynamic_monitor - Tested with xorgxrdp - Tested with vnc - Only works with single monitor. - Update documentation to clarify the difference between MSTSC and Microsoft Remote Desktop. - Compatible with NVENC and xorgxrdp_helper - Compatible with Nvidia. - Updates to include ms-rdpedisp.h header for the 2.2.2 specification of the protocol. - State machine for resizing. - Mechanisms to make sure the key frame is always sent on resize. - Adds new struct that shares the number of monitors with xrdp_client_info.h - Modification to the original resize setup that works with /gfx. - YAMI compat (For Intel hardware acceleration) - Updating to librfxcodec branch that is also a merge from mainline. - Made sure RFX progressive works. Depends on neutrinolabs/xorgxrdp#183

Nexarian mentioned this pull request Feb 25, 2021

Dynamic resolution update (resizing without reconnection) neutrinolabs/xrdp#448

Closed

Nexarian force-pushed the resize-stability-enhancement branch from ef89d7e to 2d879a8 Compare March 2, 2021 20:04

matt335672 reviewed Mar 3, 2021

View reviewed changes

Nexarian requested a review from matt335672 March 4, 2021 05:22

Nexarian mentioned this pull request Mar 4, 2021

Resolution switching without reconnecting. neutrinolabs/xrdp#1820

Merged

Nexarian force-pushed the resize-stability-enhancement branch from 0c3c8e3 to 78fd111 Compare March 7, 2021 07:25

matt335672 mentioned this pull request Mar 8, 2021

use damage to track any lost screen changes #186

Merged

Nexarian force-pushed the resize-stability-enhancement branch from dc8f6a4 to 0e46673 Compare March 14, 2021 22:42

metalefty approved these changes Apr 13, 2021

View reviewed changes

metalefty merged commit aff6ef8 into neutrinolabs:devel Apr 13, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve stability of xorgxrdp when resolution changes during a session ("dynamic resolution") #183

Improve stability of xorgxrdp when resolution changes during a session ("dynamic resolution") #183

Nexarian commented Feb 25, 2021

matt335672 commented Feb 25, 2021

matt335672 left a comment

matt335672 Mar 1, 2021

Nexarian Mar 3, 2021

matt335672 Mar 1, 2021

Nexarian Mar 3, 2021

bolkedebruin Mar 10, 2021

matt335672 Mar 10, 2021

Nexarian Mar 10, 2021

bolkedebruin Mar 12, 2021

Nexarian Mar 14, 2021

matt335672 commented Mar 3, 2021

bolkedebruin commented Mar 12, 2021

matt335672 commented Mar 12, 2021

bolkedebruin commented Mar 12, 2021

Improve stability of xorgxrdp when resolution changes during a session ("dynamic resolution") #183

Improve stability of xorgxrdp when resolution changes during a session ("dynamic resolution") #183

Conversation

Nexarian commented Feb 25, 2021

matt335672 commented Feb 25, 2021

matt335672 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matt335672 commented Mar 3, 2021

bolkedebruin commented Mar 12, 2021

matt335672 commented Mar 12, 2021

bolkedebruin commented Mar 12, 2021