-
-
Notifications
You must be signed in to change notification settings - Fork 950
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add new API to modify display devices for Windows #2032
Conversation
Wonder does it work on hyper-v guest ? |
Should work, but you'll have to try it yourself. |
@FrogTheFrog Any artifact that I can test out ? |
You'll have to build yourself. |
I have a review in process, but there is a lot to review here so it might take me a little bit. In the meantime could you add the doc pages for the new source code files? |
I've added the rst files, but I get the following warnings:
No idea how to fix them... Maybe it does not like forward declaration? |
Yea, it doesn't like duplicates. I don't know how to solve it either at this point. This is what I've done in these cases. https://github.com/LizardByte/Sunshine/blob/nightly/docs/source/source_code/src/platform/linux/graphics.rst?plain=1 I guess, try to have the main files documented... and the secondary ones can have the todo. |
I can only include the main file at this point. Also, it seems to be having problems with templated macros (according to google) and there is no way to suppress warnings for NLOHMANN_JSON_SERIALIZE_ENUM out of the box it seems, so I have to also "todo" that file too... |
5721f82
to
a16089e
Compare
Squashed and rebased the branch |
@ReenigneArcher I have reached out to Frog explaining the main technical issues with trying to restore and swap monitors before and after stream, which I encountered with my script. What is the goal of this PR?I presume to improve the reliability of switching monitors, because currently the display names can change, causing Sunshine to break. However, the solution proposed does not actually improve reliability or fix the problem because: In Windows, Display IDs are subject to change due to the system's device enumeration process. This means that whenever there's a driver reinstallation or significant changes to the system, Windows will reassign new instance IDs to displays, based on their configuration and connection sequence. Consequently, these IDs are not permanent and should not be relied upon as unique identifiers. What does that mean?Using the device identifiers is just as unreliable as the display names and it will change, causing the same behavior as today. Once they change, Sunshine will be unable to swap to the monitor until the user resolves the issue by updating the new identifiers. I have had two others try the same thing and ultimately all of us end up back to the same issue, there's no way to reliably swap monitors in Windows, without saving the settings in a profile fashion and then restoring them, much like https://sourceforge.net/p/monitorswitcher/code/HEAD/tree/MonitorSwitcher/MonitorSwitcher/ This is something we could do in Sunshine of course, but its hard to implement in terms of making it easy for the user to do and it also doesn't work for Linux. |
My proposal here is to remove the display swap code portions and keep the resolution change and HDR portions as these are more reliable and users would benefit from including these in Sunshine. We could support the capability of swapping monitors but the only reliable way I've found is restoring a profile like mentioned earlier, more work needed in the PR for that basically. |
My counter arguments: The IDs changeYes, they indeed change after driver has been reinstalled. However, not by much. In my case only 1 number changes in the:
string. It's the one before the The IDs are used for lookup only and are "made up" by me, we could use anything arbitrary as long as we could identify the device. Maybe we could simply omit that changing number. Incorrect IDsI do not use ids that change every time the system is restarted like Removing the display swap code portionsThere is no need to do it in my opinion as we still need to use the IDs and it simply comes as a bonus (read the next section). Restoring the profilesThere is no need save the "complete" profiles as they are already saved by Windows. The way Windows saves the profiles is per an active display topology. Let's say you have 3 monitors A, B and C:
You MUST ensure that you are modifying the correct config first! So, if I switch to topology You can easily test this with Windows Settings app, simply change the resolution/position to something random in the current topology, then enable/disable some device and see that the resolution is now different from what you've configured before Now, if you connect a display My only question is what to do when we are reverting changes and the topology ConclusionWe still need to use IDs of some sort to first find out about the topology configuration we are dealing with, but we should agree on what is persistent enough. Keep in mind, I don't store previous settings for long, only until we can undo them (or try to). |
I did some investigation into the IDs and these were the results:
For the physical monitors the part Since I have this data (and some more available):
I could make my own ID, something like:
However, I sadly don't have 2 identical displays so I don't know if they would be unique enough (connectorInstance number should ensure the identical display uniqueness). I could maybe add some other EDID metadata, but I need help with this one... |
I have 3 Dell S2417DG monitors on my workstation and this is what they look like in WinObj. Interestingly the Container ID seems to be the most stable identifier for the monitors across connections (and even across GPU vendors). It's not perfect either but it's a lot better than the Device Instance ID. Here is the Device by Container view with non-present devices enabled to show previous instances of the monitors with different instance IDs. The bottom 2 are actually Microsoft Basic Display Adapter devices (probably created during a driver update or fresh install), so those won't be relevant to us. It looks like Microsoft actually provides built-in support for Container ID generation using EDID data with WDDM.
|
I did some more experimentation and here are the results (using https://github.com/FrogTheFrog/Sunshine/tree/id-test as a playground): To me it seems that the first part for
According to chatGPT Container ID is stable for physical devices @cgutman, but is bonkers on the virtual ones (since the don't have enough edid information I assume). ConnectorInstance seems to change when reconnecting devices physically, it seems to relate to GPU port somehow (could be unstable). So, I would suggest to use:
OR if we have
else:
Here I have connected my monitor with DisplayPort and HDMI at the same time:
and both edidUUID and ContainerID and are different and stable. Maybe we could just use:
With a fallback to
if edid is not available? EDIT: Here's the same output from the previous testing (with edidUUID this time): It seems very stable EDIT: The proposed id still seems to be unique. |
f38ad62
to
beb97fe
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your c++ skill level is far above mine so I can't provide much meaningful review on that part (further review will be needed from cgutman and/or other team members). The documentation blocks look good and the code seems well structured.
Once this gets merged are you going to available/willing to fix bugs that come up due to these changes? I'm a bit worried about that since this grows the project code by around 20%. If you could add any unit testing, that would be greatly appreciated and ease my worries a little.
Some methods that look like easy wins in testing:
- parse_resolution_string
- parse_refresh_rate_string
- parse_resolution_option
- parse_refresh_rate_option
- parse_hdr_option
- to_string
A virtual display is also prepped to be setup in the CI, it just needs to be enabled. Just remove this line to enable:
Sunshine/.github/workflows/CI.yml
Line 866 in 116e592
if: false # todo: DirectX11 is not available, so even software encoder fails |
And then you could likely unit test a lot more functions.
docs/source/about/advanced_usage.rst
Outdated
@@ -590,9 +590,35 @@ keybindings | |||
.. todo:: macOS |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any thoughts on this?
third-party/nlohmann_json
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you checkout and commit the latest release/tag of this submodule? https://github.com/nlohmann/json/releases
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be 9cca280a4d0ccf0c08f47a99aa71d1b0e52f8d03
"display_device_prep": "no_operation", | ||
"resolution_change": "automatic", | ||
"manual_resolution": "", | ||
"display_mode_remapping": "[]", | ||
"refresh_rate_change": "automatic", | ||
"manual_refresh_rate": "", | ||
"hdr_prep": "automatic", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All of the new config options should be added to the advanced_usage.rst
file. The order in that file should be the same as within the config ui.
"display_device_prep": "no_operation", | ||
"resolution_change": "automatic", | ||
"manual_resolution": "", | ||
"display_mode_remapping": "[]", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
display_mode_remapping
is out of order, according to the order they are above.
@@ -139,6 +139,23 @@ | |||
"controller_desc": "Allows guests to control the host system with a gamepad / controller", | |||
"credentials_file": "Credentials File", | |||
"credentials_file_desc": "Store Username/Password separately from Sunshine's state file.", | |||
"display_device_options_note_desc_win": "Windows saves various display settings for each combination of currently active displays.\nSunshine then applies changes to a display(-s) belonging to such a display combination.\nIf you disconnect a device which was active when Sunshine applied the settings, the changes can not be\nreverted back unless the combination can be activated again by the time Sunshine tries to revert changes!", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I'd prefer if \n
wasn't in the strings. The translators are not necessarily going to be programmers or understand what \n
does.
You could have a few keys for the same item though, just end them with an index number. Then put your line break in the html. I think I did this in another place already if you need an example.
Per discussion on discord. Would it make sense to migrate some of this into it's own re-usable library? Kind of like what is being done in #2315 for Linux input. It might be easier to expand support to Linux and macOS if it were separated. I can create a repo for it in our org? |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2032 +/- ##
=========================================
- Coverage 6.18% 5.59% -0.59%
=========================================
Files 86 86
Lines 17548 15346 -2202
Branches 8186 6962 -1224
=========================================
- Hits 1085 859 -226
+ Misses 14652 13509 -1143
+ Partials 1811 978 -833
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Mostly copied code from LizardByte#2032 to try a PoC for the API I want to use
Mostly copied code from LizardByte#2032 to try a PoC for the API I want to use
Mostly copied code from LizardByte#2032 to try a PoC for the API I want to use
I'm converting this to draft until libdisplaydevice is ready |
@FrogTheFrog Is this development canceled? 😳😌 |
No, it's being ported to a separate library with new unit-tests being added, so it's taking a lot of time. I have decided to rebase the current branch, however the |
Ah I was scared, your work is great it will simplify the configuration of sunshine for many people 🙂 And the possibility of mapping resolutions and frequencies to others is excellent! I think I understood that there was a new version that was going to be released soon, I suppose your development will not be in that version? Thanks 🙏🏼 |
Description
New changes allow for Sunshine to control display devices on Windows, such as:
It also moves away from the
\\.\DISPLAY1
-like configurable output names to proper that are pretty persistent (IDs changed a little after I reinstalled my GPU drivers, after DDU).The applied changes are saved additionally saved to a file in case the PC dies or something so that Sunshine can undo the changes once it is started again.
Screenshot
Example of the new options exposed to the user:
Type of Change
.github/...
)Checklist
Branch Updates
LizardByte requires that branches be up-to-date before merging. This means that after any PR is merged, this branch
must be updated before it can be merged. You must also
Allow edits from maintainers.
Known issues (all resolved)
IDD HDR driver displays high-contrast colors when the display is activated and was inactive before. Seems to be a driver issue, since it does not happen with HDR dongle. A workaround would be to reset all the HDR states to off and then on again, but I don't think we should implement this workaround for this.Sound fails to reset back to whatever was used before after the stream ends if a new display (without the same sink) was the only active display.