Fixes some problems with monitor hotplug #2942
Merged
Conversation
This file contains 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
|
My mistake - we are sending the monitor configuration. It's just hidden away in xrdp_caps_send_demand_active(). |
|
Out of time today, but working on adding multi-monitor support around |
Nexarian
reviewed
Feb 9, 2024
| @@ -1153,15 +1153,6 @@ libxrdp_reset(struct xrdp_session *session, | |||
| return 0; | |||
| } | |||
|
|
|||
| /* if same (and only one monitor on client) don't need to do anything */ | |||
There was a problem hiding this comment.
Why remove this? It doesn't appear to affect multi-mon?
There was a problem hiding this comment.
Good question!
The answer is that at the moment, when we go from two monitors to one, client_info->multimon gets reset. As a result this test succeeds and we don't send the signal back to the state machine that the resize has completed.
There was a problem hiding this comment.
Well then I utterly failed at making this call do what I thought it did, thanks for removing it then!
Nexarian
reviewed
Feb 9, 2024
vnc/vnc.c
Outdated
| @@ -2133,6 +2133,7 @@ lib_mod_set_param(struct vnc *v, const char *name, const char *value) | |||
| (const struct xrdp_client_info *) value; | |||
|
|
|||
| g_free(v->client_layout.s); | |||
| v->client_layout.count = 0; | |||
There was a problem hiding this comment.
Should we also set v->client_layout.s to NULL here?
There was a problem hiding this comment.
Quite right, although I'm probably going to make v->client_layout.s static anyway as it simplifies the logic quite a bit at the cost of a bit more memory.
There was a problem hiding this comment.
Memory is cheap, bugs are not :)
Nexarian
reviewed
Feb 9, 2024
xrdp/xrdp_mm.c
Outdated
| /* Update the client_info structure with the new description | ||
| * and tell the module so it can communicate the new | ||
| * screen layout to the backend */ | ||
| sync_dynamic_monitor_data(wm, &(description->description)); |
|
Added some comments. |
|
Thanks @Nexarian I've got most of Monday to get on with this. One of the architectural problems we've got is the
I'm going to look into splitting this call into two:-
How does that sound? |
|
I agree that the So yes, doing more attention with that is definitely warranted, so yes sounds good! I'll wait on approving/merging this until you say "this is ready to merge" or some such. |
|
I've done the following (in a separate branch):-
It's still a bit messy, so I've got nothing to release yet. @Nexarian - a question for you. The X server resize is currently done by calling two methods on the module - |
|
@matt335672 I wrote this a long time ago, but the reasons are either:
If you can find a way to combine them without breaking anything, go for it. |
|
Added a commit which is simple to understand, and necessary to get the monitor layout PDU working correctly with mstsc.exe. Informally, I think I've cracked the monitor plugging/unplugging issue now - at least it seems to work on my laptop with a single external monitor being plugged and unplugged. Current code is here. I need to work more on the commit history and do a lot more testing so these are subject to change. It's not really in a reviewable state, but comments welcome anyway:- xrdp: At the moment on my setup:-
I've added a new message (302) to the interface from xup to xorgxrdp to send over the monitor list. Consequently I can retire the resize desktop message (300) as it is no longer required. |
|
Now you know the pain that is resizing :) |
|
Thanks, I did a quick test on your branch of the GFX+Xorg scenario. To be precise, cherry-picked your commits to v0.10 branch. I found some resizing issues with using only 1 of N (>=2) monitors. Prerequisites:
xorgxrdp.10.log |
|
Thanks @metalefty. I'll take a look at that now. |
This fixes some monitor hotplug issues with non-GFX codepaths. 1) The server_version_message() was working on an out-of-date copy of the client_info. As a result, the X server and the window manager did not agree on the number of windows 2) As a result of 1), a memory leak was found in the VNC module.
From [MS-RCPBCGR] 3.3.5.12.1:- > ...The contents of this PDU SHOULD NOT be compressed. > > This PDU MUST NOT be sent to a client that has not indicated support for > it by setting the RNS_UD_CS_SUPPORT_MONITOR_LAYOUT_PDU flag (0x0040) > in the earlyCapabilityFlags field of the Client Core Data (section > 2.2.1.3.2). Also, 2.2.12.1 specifies the source channel must be zero. In testing, a compressed monitor layout PDU causes mstsc.exe to exit with a protocol error.
This stores what kind of resizing (if any) can be achieved with a Deactivation-Reactivation sequence.
This commit DOES NOT compile. This change alters these module interface calls:- 1) mod_server_monitor_resize() (Call from xrdp to module). Updated. 2) server_reset() (Call from module to xrdp). Replaced. The mod_server_monitor_resize() call is updated :- 1) to allow a monitor list to be passed in for a multimon resize 2) with an 'in_progress' return value which tells the caller whether or not to expect a callback. The server_reset() call served two purposes up until now:- 1) To allow a module to resize a single monitor session. There is no way to request a multi-monitor resize from the module 2) (with bpp == 0) To signal to the mm resize state machine that a server screen resize hsa finished. This is split into two calls:- 1) client_monitor_resize() to allow a mdule to request a multimon resize. 2) server_monitor_resize_done(). This is called by a module when a resize is completed.
Neutrinordp module now compiles with updated monitor resize interface
Significant updates for the VNC module:- 1) Support for the new API calls allowing both server and client multi-monitor resizes. 2) The s member variable of the vnc_screen_layout structure is no longer dynamically allocated. 3) The module server_width and server_height member variables are removed as these are just duplicating server_layout.total_width and server_layout.total_height. 4) When the server screens are resized, there is no need to restart the entire resize state machine as we already know at this point that the server supports resizing.
Input message 300 to xorgxrdp which communicated a screen size has been replaced with message 302 which sends a list of monitors to xorgxrdp. This simplifies the initialisation and resize logic somewhat, but a compatible version of xorgxrdp must be used
This commit compiles.
Clear egfx_up as soon as the channel starts to close so that xrdp_mm_draw_dirty() doesn't send data to a closed channel.
matt335672
force-pushed
the
fix_monitor_hotplug
branch
from
c0d7566 to
3ca3bd1
Compare
|
This is now ready for review and further testing. You will need neutrinolabs/xorgxrdp#285 as well. Fixed:-
Hopefully I've done my rebasing and merging correctly! |
|
Awesome, I'll take a look and test over the next few days. I really appreciate all the work you've done on this. Hopefully my code wasn't too maddening :) |
|
Your code's absolutely fine. I just hope mine is fine for you! A couple of maybe significant points:-
There's more we can do in this area:-
I've left both of these for now so we can concentrate on getting something out-of-the door. Also, I probably need to update your page on the wiki. |
|
Thanks! I'm not at home for several days so I have only one monitor built in laptop. I will be able to test it tonight if the TV in the hotel I stay at today has HDMI input. |
|
@Nexarian, @jsorg71 - I've just trying to get to grips with the GFX client resize as an alternative to using a Deactivate-Reactivate sequence, as specified in [MS-RDPEDISP] 1.3. I've a few questions in areas which you both know a lot more about than I do. I'd be grateful for any enlightenment. I think these changes are out-of-scope for this PR, but should be simple enough to add to another one.
My current thinking on the client resize from the xrdp_mm state machine is:-
What do you think? I'm sure I've failed to spot a few dragons here, but it should be reasonably simple to implement as pretty much all the code is already written, in one form or another. |
|
Awesome! Hot-unplug looks good to me but there are some other problems. Resizing is a pain as @Nexarian said :) I'm using your latest branches. Coldplug, that is to say, disconnect & reconnect with different monitor counts. When you have two monitors,
xorgxrdp.10.logThe second problem is hot-replug, hot-plug after hot-unplug. Monitor count changes 2 -> 1 -> 2 in this case. It doesn't return to the original 2-monitor state. The xrdp session has 2 monitors but mstsc doesn't spread to 2 monitors. Windows-to-Windows RDP works fine even in this case. I guess xrdp doesn't notify mstsc that the remote session has 2 monitors. The third problem is xrdp segfault. I couldn't find a clear procedure to reproduce yet. xrdp.log |
|
Thanks for that - very useful. It'll be tomorrow when I get to look at this. |
This prevents valgrind errors when resizing the screen to a larger size on GFX systems.
matt335672
force-pushed
the
fix_monitor_hotplug
branch
from
2a38a67 to
c21521f
Compare
|
Updates for xrdp and xorgxrdp The xorgxrdp commit should fix problems 1) and 2) above. I still getting some redraw problems on hot plug/unplug. The screen isn't always completely redrawn, and I've not tracked down why yet. |
|
Now it works better. What about merging and test? |
|
Give me a while to track down the redraw problem. As I write this it's 9AM in the UK and I've got the whole working day to look at this. I've had a think about it over the weekend, and I think I can see what's happening. In GFX mode the screen buffer in xrdp_wm is needed for login, but is unused when xorgxrdp is loaded, as the module writes directly to the client, through the encoder thread. My theory is that when we resize the screen, there's a window where the unused screen buffer can be copied to the client, overwriting what's been put there by xorgxrdp. xorgxrdp doesn't know this has happened, and in progressive mode it just updates bits of the framebuffer rather than the whole lot. This presesnts as a failed redraw. This explains the messages I was getting from valgrind, and also why I was seeing screen corruption (rather than just black areas) when I added the code to clear down a resized bitmap. I need to verify that my explanation is correct, and I also need to figure out the best way to disable the xrdp_wm screen buffer in GFX mode when using xorgxrdp. I think the buffer is still needed for VNC, but I'm not certain. |
In GFX mode, if we're using xorgxrdp, frame updates are send directly from the client, bypassing the screen buffer in xrdp_mm. This commit only allows the xrdp_mm screen buffer to be invalidated if the painter has drawn into it since the module was loaded. This prevents the unused (and invalid) frame buffer being pushed to the client in GFX mode with xorgxrdp.
|
A couple more fixes which mix some overwriting issues have been pushed to xrdp and xorgxrdp branches. I'm still having problems with hot plug/unplug in GFX mode with xorgxrdp. VNC is fine and non-GFX xorgxrdp is fine.
Result is:-
|
|
@matt335672 What this looks like is some of the screen didn't get copied over correctly, which implies to me either:
Because RFX Pro does incremental frames, whenever a Or you may need to manually send the RFX_PRO_KEY message to the encoder directly from XRDP after everything is said and done. |
|
Thanks @Nexarian - that was very useful as it got me checking all the codepaths and then I had a 'Eureka' moment. When the shared memory and device memory are resized, we need to clear out the CRC fields as well. Otherwise we think memory areas match when in fact they don't, and they aren't being sent to the client. Hot plug/unplug on my test system is now working with GFX/xorgxrdp. I'm engineering a proper commit to xorgxrdp which I'll commit later this morning (UK time) |
|
@metalefty - just added a commit to branch I think this series of patches also resolves an issue @Nexarian mentioned somewhere which was than starting a GFX session and then connecting from a non-GFX client didn't work. I can't reproduce that here. I'm read to go for merge and test now. |
|
Thank! I will test it by myself. After that, I'll merge it and go for a test on user side. |
This was referenced Feb 20, 2024
|
Thanks for all your hard work on this @matt335672. I haven't had time to fully digest all the changes you've made, but it looks like a good foundation on which to build the next versions of GFX! |
|
Also: You did exactly what I did: Spend time at night tracing the data flow path through my head and on paper and on whiteboards, and trying to determine exactly what might be wrong. Sleeping on it, waking up in the middle of the night with an insight that you have to try, etc. It's really hard to debug in a Anyway, this explains, I think, why most Remote Desktop ecosystems do not support this behavior. It's really hard, but also now a big differentiator for using XRDP! |
|
I'm certainly looking forward to getting back to some more normal sleep patterns! It's been an interesting couple of weeks, and given me a bit of insight into some of the more murky corners of the codebase. I'm sure that will come in useful in the future. |
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.
See #2938
This fixes some monitor hotplug issues with non-GFX codepaths:-
non-GFX
Monitor removal for works OK after applying the patch. However, when adding a monitor we end up with a single screen on the client covering the whole desktop.
GFX
The removal of a monitor sort-of works, but there are major screen corruption problems. When adding a monitor we get a protocol error when we try to use the added monitor.
Analysis
When we add a monitor, or remove a monitor (leaving more than one) mstsc.exe generates a dynamic monitor update request with two monitors included. Up until now we have not been able to generate this use-case.
The problem happens when
libxrdp_reset()is called. This forces a single monitor at the client end for a multi-monitor setup. As a result, we can end up with the X server and the client not agreeing on the number of outputs, which is where the protocol errors come from.I think we're missing a monitor layout PDU from the server to the client. This should be sent after the demand active PDU - see [MSRDPBCGR] 1.3.11, in particular note 9.
There's one other GFX problem which may be cured by fixing the above. After a resize, I get valgrind memory errors:-
I think the next step is to update libxrdp_reset() to send the monitor configuration (if known) after the demand active PDU. This isn't going to be quick.
Thoughts and suggestions are extremely welcome.