Skip to content
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

feat: use output/display names for wlgrab #2885

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

vmsh0
Copy link

@vmsh0 vmsh0 commented Jul 17, 2024

Description

TL;DR: This is a proof-of-concept PR for supporting using output names with the Wayland grabber, instead of arbitrarily-generated numeric ID as it is currently the standard in Sunshine.

Long version:

Currently, output IDs are assigned following this logic:

  • get_registry() is called on the Wayland display, which causes the server to enumerate all global objects including wl_output objects (
    interface.listen(display.registry());
    )
  • while receiving the responses, the callback (
    interface_t::add_interface(wl_registry *registry, std::uint32_t id, const char *interface, std::uint32_t version) {
    ) singles out wl_output objects and saves the screen to an std::vector, implicitly assigning them numeric IDs in whatever order the Wayland server enumerates the objects

This is nondeterministic in two different ways:

  • There is no guarantee that a Wayland server reuses the same objects IDs and enumeration order for two different clients (i.e. might be unstable across Sunshine restarts)
  • There is no guarantee that across system reboots the Wayland server will enumerate outputs in the same order

This commit introduces support for indicating a wl_output/zxdg_output_v1 output name (e.g. eDP-1, HDMI-0 for wlroots based compositors) in the output_name Sunshine parameter. I think this improves the above situation in the following ways:

  • These names are explicitly specified in the interfaces to be stable within a Wayland session. While this is not as good as being stable across reboots (even though at a practical level this is also usually true, as these names are also often used in compositor configuration files), at least it allows to reconfigure Sunshine on-the-fly before starting it, by updating its configuration file in a compositor-dependent manner if necessary.
  • These same names are, in all Wayland compositors I have tried, the actual user-facing output names that are used in the tools. Thus, this allows users to configure Sunshine using the same output IDs that they use within other Wayland tools (e.g. wlr-randr, wf-recorder, compositor output configuration, etc), improving usability.
  • At the same time, this PR retains compatibility with the previously-employed numeric IDs.

Please note that this PR is a proof of concept, mainly to kickstart a conversation on the feature. If the feature is deemed desirable, before merging it, some improvements that can be made to this PR are:

  • The instructions on the web GUI should be updated to reflect this feature
  • The documentation should report this feature
  • It should be evaluated whether this is possible/feasible/desirable for other grabbers as well
  • It should be evaluated whether to require using a prefix to indicate an output name instead of a numeric ID, as theoretically a numeric ID is also a valid Wayland output name (although I have never seen one which is entirely numeric)

Screenshot

(not relevant)

Issues Fixed or Closed

This is somewhat related to #2490.

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Dependency update (updates to dependencies)
  • Documentation update (changes to documentation)
  • Repository update (changes to repository files, e.g. .github/...)

Checklist

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have added or updated the in code docstring/documentation-blocks for new or existing methods/components

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.

  • I want maintainers to keep my branch updated

@CLAassistant
Copy link

CLAassistant commented Jul 17, 2024

CLA assistant check
All committers have signed the CLA.

@vmsh0 vmsh0 marked this pull request as draft July 18, 2024 07:41
@vmsh0
Copy link
Author

vmsh0 commented Aug 20, 2024

Are there any comments or feedback on this PR?

@ReenigneArcher
Copy link
Member

Are there any comments or feedback on this PR?

I haven't even looked at it since it was marked draft.

@vmsh0
Copy link
Author

vmsh0 commented Aug 20, 2024

Are there any comments or feedback on this PR?

I haven't even looked at it since it was marked draft.

Fair enough :)

It's marked a draft because it doesn't include the necessary documentation effort, as before putting that effort in I would like some general feedback on the idea.

Is the project interested in the feature? Is my proposed implementation strategy ok? Etc

@ReenigneArcher
Copy link
Member

I think this is an improvement overall, but I say this as a non Linux user.

Also, any help on the Linux side is greatly appreciated. Most of the active devs are mostly focused on Windows.

This may affect #2490

@vmsh0
Copy link
Author

vmsh0 commented Aug 21, 2024

This may affect #2490

Good catch.

The main improvement of #2490 is to add a "friendly name" to the screen, in addition to an ID, and the relevant UI changes. However, it does not change how ids are computed. My PR, instead, does change how IDs are picked: the actual Wayland IDs can now be used instead of the made-up Sunshine IDs (while retaining the ability to recognize them). This is also the case for the macOS implementation, even though unfortunately the native IDs are not as user-friendly as the ones on Linux/Wayland. While I do not have the chance to perform testing on macOS, I can assume that the IDs are stable across applications and perhaps reboots,

Thus, this does to some degree interact with #2490, which perhaps should be merged first, and then this PR should be reworked to build upon the respective modified data structures to reimplement this functionality. Similar work could probably be carried out for other platforms, as I think this is in principle very useful and makes configuring Sunshine more intuitive and stable.

To abstract a bit, I think the interaction between these two PRs underlines the following design directions:

  1. Should Sunshine, in general, prefer native IDs when available, instead of the arbitrary numeric IDs that are generated now? This is the main design decision. As I have argued, I think it should.
  2. Should Sunshine support matching screens by criteria other than an ID -- either made-up or OS-generated? E.g. by resolution, refresh rate, type of physical port, virtual viewport/main screen status, etc? I.e. should the principle of having a more native and intuitive way to select a screen be brought even further?

I would be willing to contribute more work towards both the above directions, particularly on Linux (and perhaps Windows if no one else is interested in picking this up), if deemed worthwhile. However, I think that such structural decisions require a broader discussion and much more feedback. Is this the appropriate place to have such a discussion, or should I post this somewhere else?

@ReenigneArcher
Copy link
Member

Should Sunshine, in general, prefer native IDs when available, instead of the arbitrary numeric IDs that are generated now? This is the main design decision. As I have argued, I think it should.

Yes. There's also this for Windows: #2894 which uses https://github.com/LizardByte/libdisplaydevice ... currently that library is Windows only, but it could be extended to macOS and Linux as well. Linux probably being the more difficult one due to so many options... not saying this work needs to move to that library (but you can contribute there if you want), just pointing out that we are moving to more stable IDs for Windows as well. e.g. output_name = \\.\DISPLAY1 becomes output_name = {daeac860-f4db-5208-b1f5-cf59444fb768}.

Should Sunshine support matching screens by criteria other than an ID -- either made-up or OS-generated? E.g. by resolution, refresh rate, type of physical port, virtual viewport/main screen status, etc? I.e. should the principle of having a more native and intuitive way to select a screen be brought even further?

I'm not sure on that. It sounds like it would be complex and probably break easily. I think moving to a more stable ID is sufficient for now.

This has also been one of the longest standing issues with xorg: #221

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants