Fix off-by-one errors in monitor resize logic #2219
Conversation
|
Here's what I think is happening. MSTSC is "too honest" -- It's giving the width and height in true, 1-based terms, but when we compute sizes, we want the zero based index. I think this works, though upon reading the spec I'm not sure there's any light shed upon this issue. Thanks for finding this, and I'm sorry you keep finding residual bugs in my work! |
|
Also, you're declaring width and height inside the function. I thought, due to our focus on compatibility with old-school C compilers, that we needed to declare all of our variables at the beginning of the function? Or is that just polite convention? |
|
Regarding the old-school compilers - the only problem we have with Jay's old SPARC build machine is that it can't cope with this:- for (int i = 0; <etc>)Other than that, there are plenty of instances of declarations after statements in the codebase, and also '//' comments which came in with C99. There's more context in #1722 |
|
I don't think the spec isn't consistent in describing rectangles. Some bits of [MS-RDPBCGR] describe rectangles with (left, top, width, height) and other bits use (left, top, right, bottom). One example of the latter is TS_MONITOR_DEF. It uses the word 'inclusive' for the right and bottom. The width is a number of pixels, and 'right and 'bottom' will be inclusive. So for a 4x4 monitor:- Width is 4, left is 0, and right is 3, Height is 4, top is 0 and bottom is 3. In symbols, As for finding bugs, I can only apologise for not spotting this in the review. I'll try to get this PR fixed up today so we can talk about it. Next week is going to be tricky for me though as it's a bit full-on with family. |
|
@Nexarian - I've rejigged the code and the tests to fix the off-by-one. CI now passes. I've added some macros to As i mentioned, I'm going to be struggling for time next week. I'll keep an eye on my email though in case you want this one and #2216 merging. |
|
I'm not sure if this is now correct because it changes session_width and session_height net calculations. Take a look at what |
|
Yes - good question, I take the point about the unit tests, which is a good reason to have them. I think the key to understanding this is to start with how the widths and heights of monitors stack up with multiple arrangements. There's nothing special about monitors, in that things just add up. So if I get a deck of 52 cards, and put it on top of another deck of 52 cards, I get a total height of 104 cards. Similarly, a monitor of 2160 pixels in top of another monitor of 1024 pixels gives a total height of 3184 pixels. Let's go with an example closer to home. I'll stick with your hex monitor test, and reduce each monitor to 5 x 3 pixels, so we can see what's going on. Please excuse the ASCII art. There are probably better tools to use! The monitors are arranged like this, if I remember correctly. +-----+-----+-----+ | 1 | 2 | 3 | +-----+-----+-----+ | 4 | 5 | 6 | +-----+-----+-----+ To my mind, the pixel counts for 5x3 monitors look like this, using (0,0) for the top left pixel of each monitor:- <------------15------------->
0 1 2 3 4 0 1 2 3 4 0 1 2 3 4
^ 0 . . . . . @ @ @ @ @ . . . . .
| 1 . . . . . @ @ @ @ @ . . . . .
| 2 . . . . . @ @ @ @ @ . . . . .
6 0 @ @ @ @ @ . . . . . @ @ @ @ @
| 1 @ @ @ @ @ . . . . . @ @ @ @ @
V 2 @ @ @ @ @ . . . . . @ @ @ @ @
The composite width and height is a natural consequence of summing the individual monitor widths and heights:- CompositeWidth = Width1 + Width2 + Width3 = 5 + 5 + 5 = 15 CompositeHeight = Height1 + Height2 = 3 + 3 = 6 The individual monitor co-ordinates work out as follows:-
Taking the above table and scaling up to (3840x2160) gives:-
Total composite size = (3840 + 3840 + 3840, 2160 + 2160) = (11520, 4320) Does that make sense? |
|
@matt335672 It makes sense, but I think the next thing we need to do is determine what the Windows RDP server does. I suspect that it does the +1 thing. It's the reference implementation, and, logic aside, we need to make sure XRDP matches that setup. What I can do is test a remote connection to Windows with a FreeRDP connection that has multiple monitors configured, and see what it returns. In the past I've done this to figure out how to get the egfx branch to work with the Microsoft clients. |
|
@Nexarian I agree that we should match the windows RDP server behavior. I can help with getting the values from a connection between a Windows RDP server and a client. I've been working on an tool that does RDP mitm and pdu parsing that works with Windows RDP to be able to understand the reference implemention more easily. |
|
By all means do some more investigation - we need a common understanding of what's happening. The existing code is producing odd behaviour in a Windows 10 client VM I have with two screens. If I use this to test #1928, I get scroll bars appearing on my VM windows which suggests the Linux desktop is being sized to a bigger physical size than is available. I can produce some screenshots of this if you like. |
|
I see this too, actually, and that's actually a good enough reason to convince me. Go ahead and merge this in, and I'll rebase my changes off of it tomorrow and we can keep experimenting. Your reasoning is sound. |
|
@Nexarian I was able to capture an RDP session with GFX multimon and resize between a Win10 RDP server and MSTSC as the client. My tool was able to parse the initial monitor config, but I ran into issues trying to parse the session up to the point of the monitor resize. For reference my monitors in this session are: 1920x1080 and 3840x2160. I hope this session capture is useful anyway. |
|
Thank both - I'm sure we'll be continuing this conversation later. |
Just found this while testing PR #2175 against #1928
First thing to say is that as a reviewer of this I missed it completely.
I was running on a Windows VM with two screens at 1920x1200 and 1280x1024. I was getting messages like this in the log:-
There are clearly some other issues here around the state logging, but that isn't the point of this PR. The point is all the sizes are off-by-one.
I've traced this down to 4a0db63. The code in
libxrdp_process_monitor_stream()is using the monitor width as the monitor right side, and the monitor height as the monitor bottom. I think these values-are-off-by-one. The code below addresses this.@Nexarian - your thoughts please? Sorry I missed this in the review. Also, the tests for the CI are broken. I'm happy to fix these, as it will give me a chance to review this again.