Parse more physical monitor size information #2310
Conversation
|
👀 |
common/xrdp_client_info.h
Outdated
| int flags; | ||
|
|
||
| /* From 2.2.2.2.1 DISPLAYCONTROL_MONITOR_LAYOUT */ | ||
| /* From [MS-EDISP] 2.2.2.2.1 DISPLAYCONTROL_MONITOR_LAYOUT, or |
There was a problem hiding this comment.
Should we use the full name here: MS-RDPEDISP
There was a problem hiding this comment.
Yes we should - I'll fix it.
| * They can be used as a fallback for a single monitor session | ||
| * if physical sizes are not available in the monitor-specific | ||
| * data */ | ||
| unsigned int session_physical_width; /* in mm */ |
There was a problem hiding this comment.
Why not put session_physical_width (and height) into display_size_description? It contains the information for all the monitors in addition to summary information such as the size of the full session.
Also, this doesn't seem to have logic that would be at parity with session_height/width -- Those are for computing the height/width of the ENTIRE session. Is there a similar computation for the physical width/height of the ENTIRE session when it's provided for all monitors?
I think also, how do we deal with DPI if multiple monitors are provided that each have a different scale factor? Pick the highest DPI? Lowest?
There was a problem hiding this comment.
Funnily enough I started down that route. Eventually however it occurred to me that it didn't make much sense.
The physical size of the desktop is easy to understand for a single monitor, and actually has a use, which is to calculate the DPI for the monitor (in the absence of any other information). For multiple monitors however, which may not even be touching in any way, the overall physical size is not something that can be defined. For example, what would you do in a situation like this with a gap between the monitors?
+---------+ +---------+
| | | |
| 0 | | 1 |
| | | |
+---------+ +---------+
So eventually I removed the overall physical size from the display_size_description, and just left the values from the CORE message further down the structure as a fallback, in case extended monitor information isn't available. These could be renamed maybe, if it's not clear what is going on.
Dealing with the DPI on the login screen is easy - the login screen only appears on the primary monitor, so that's the only monitor we need to worry about.
| } | ||
| break; | ||
| /* CS_MCS_MSGCHANNEL 0xC006 | ||
| CS_MONITOR_EX 0xC008 |
There was a problem hiding this comment.
Because we're now processing CS_MONITOR_EX under the case SEC_TAG_CLI_MONITOR_EX: tag, so it no longer makes sense to have this in the comment.
| @param monitor_layout struct containing extended attributes | ||
| */ | ||
| static void | ||
| sanitise_extended_monitor_attributes(struct monitor_info *monitor_layout) |
There was a problem hiding this comment.
I like this refactor, especially because it helps clean up and clarify what extended monitor attributes are!
|
We'll need to merge this in conjunction with: neutrinolabs/xorgxrdp#224 -- Can you review that as well? |
To implement a scalable login screen, we need to be able to ascertain the DPI of the connected primary monitor. At present, in a multi-monitor situation, this information is available in the struct display_size_description, which can be searched for the primary monitor. This is only the case however if the Display Control Channel Extension is in use ([MS-RDPEDISP]), and a DISPLAYCONTROL_MONITOR_LAYOUT has been received. This PR retrieves physical monitor size information from the following two additional places. 1) The TS_UD_CS_CORE PDU. Physical size information is optionally included in this PDU for single-screen configurations. 2) The TS_UD_CS_MONITOR_EX PDU. This includes physical size information for multiple-screen configurations.
matt335672
force-pushed
the
physical_desktop_size
branch
from
d93c34a to
d5445e9
Compare
2 participants
Draft PR to parse more physical monitor information.
This is a precursor to a scalable login screen - we need to be able to calculate the DPI for the login screen monitor, and to do this, we need access to the physical monitor size.
I need to do more testing of this before it's ready for release, but I'm going to be a bit stuck for time for a couple of weeks.
@Nexarian - can you take a look and give me any informal comments you have?