feat(ui): list available displays with select

[Hazer]

Description

This is a PoC, and a proposal, for allowing the screen to be selected via a list, instead of having to search for an ID and write it down. Also, this PR doesn't cover it, but we could in the future, further improve it, and allow the client to request it and maybe remotely select the preferred display.

For this PR I took basically the same code from every display_names() and stripped it to just the basics, but keeping the check, fail-overs, and the same counting for IDs as the other methods use, so we have a 1:1 mapping. I took a bit more effort in some targets than others, but I focused in the targets I could build and run myself first.

All targets are implemented, but some probably will need some more work done, testers are really welcome.

I don't consider this as a breaking change, because:

1. The select uses the same value as the text input used, so it won't break even if it had been set in the past;
2. If for some reason, we fail to get a list, we fallback to the old text input, so if something breaks, the user can still use the old UI.

Depends on

• refactor(ui): break down config.html into smaller pieces #2491

Built and run so far

• macOS Intel
• macOS Apple Silicon
• x11
• Windows
• kms
• Cuda
• Wl

Screenshot

macOS

Windows

Linux AppImage x11

Type of Change

• New feature (non-breaking change which adds functionality)
• 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

[ReenigneArcher]

This looks like a nice addition. Thank you for the PR!

[ReenigneArcher]

This may cause conflicts with #2032

[FrogTheFrog]

@Hazer Heyo, as RA mentioned it will conflict with my PR a little. Could I ask you to change this structure:

  struct display_options_t {
    int id;
    std::string name;
    bool is_primary_display;
  };

to

  struct display_options_t {
    std::string id; //< An actual value for "output_name"
    std::string friendly_name; //< Some name for the end-user to identify this display
    bool is_primary_display;
  };

?

Also, on Windows primary display is a display that has origin coordinate of (0, 0). There can be multiple primary displays.

[Hazer]

This may cause conflicts with #2032

@ReenigneArcher That PR is amazing, maybe I'll try writing some of the macOS code needed in there, will that be a library with another repo or will be merged in the main code?

[ReenigneArcher]

@Hazer it will mostly be in a separate library... in order to not grow the Sunshine code by 20%. @FrogTheFrog is doing the heavy lifting. They are slowly porting things over. https://github.com/LizardByte/libdisplaydevice

[Hazer]

  struct display_options_t {
    std::string id; //< An actual value for "output_name"
    std::string friendly_name; //< Some name for the end-user to identify this display
    bool is_primary_display;
  };

Sure, I was already discontent with the id as it was and wanted to change it, was currently reading #221 to gather ideas for it, string is the way to go.

Also, on Windows primary display is a display that has origin coordinate of (0, 0). There can be multiple primary displays.

As I was writing this PR, "primary display" became less and less something simply achievable, it seems if we don't use x11, any pure wayland compositor, has no specific API for that, and people relies on asking the DE (KDE, GDM, etc) directly, so it's not doable, I was already thinking of removing it, or making it has any display that has origin coordinate of (0, 0), but I was trying to find out if it made sense for linux.

So meanwhile, you gave me the idea to rewrite it something like this:

   struct display_options_t {
     std::string id;
     std::string friendly_name;
     current_display_settings_t current_settings;
   };

   struct current_display_settings_t {
     display_device::display_origin_t origin; //< This doesn't exists yet, but could be a nice addition to the API
     display_device::display_mode_t mode;
     
     bool current_display_settings_t::is_primary() const {
         return origin.x == 0 && origin.y == 0;
         // or maybe even:
         return origin.is_primary();
     }
   };

@FrogTheFrog what do you think? Meanwhile your PR is not merged, I just create a similar struct with the same name, we just replace it when it arrives.

Also @FrogTheFrog, could your display_device::display_mode_t include a scaling_factor: float or something? This way we can differentiate Retina displays from others, and will be good for determining Linux HiDPI scaling too. How does Windows handle HiDPI scaling?

[FrogTheFrog]

@FrogTheFrog what do you think? Meanwhile your PR is not merged, I just create a similar struct with the same name, we just replace it when it arrives.

After some deliberation, I have decided not to expose anything more from the inner API (including types apart from the needed ones) than :

device_info_map_t
enum_available_devices();

std::string
get_display_name(const std::string &device_id);

as we just don't need this information exposed to Sunshine at the moment (at least for Windows). Everything is done by passing the configuration anyway. So, the mode struct will not be available.

I would recommend keeping the boolean (as you're now exposing Windows specific origin point logic to Sunshine) or just dropping it altogether as you've wanted.

Also @FrogTheFrog, could your display_device::display_mode_t include a scaling_factor: float or something? This way we can differentiate Retina displays from others, and will be good for determining Linux HiDPI scaling too. How does Windows handle HiDPI scaling?

HiDPI is a standalone setting hidden somewhere like the HDR and is not influenced by the display mode.

That aside, please don't use the same namespace as I'm only working on Windows code and for other OS'es it will have noop implementation. Since I will not be able to replace it completely, it will really mess everything up for me.

[Hazer]

After some deliberation, I have decided not to expose anything more from the inner API [...]
as we just don't need this information exposed to Sunshine at the moment. [...] So, the mode struct will not be available.
That aside, please don't use the same namespace as I'm only working on Windows code and for other OS'es it will have noop implementation. Since I will not be able to replace it completely, it will really mess everything up for me.

@FrogTheFrog That kinda sucks for me, I was expecting to just import the display_device.h, replace my struct with yours, populate the struct myself and be done with it. This feels like unnecessary duplication here, as this information is quite generic, and I don't need any of the other implementation details, just the same structs, but oh well. I will duplicate.

I would recommend keeping the boolean (as you're now exposing Windows specific origin point logic to Sunshine) or just dropping it altogether as you've wanted.

Actually, I don't know, I think it's not a good idea for other platforms as a boolean either, it seems wayland can use the same origin logic, but via offset, and macOS can use the same origin logic but will ever only have one (0, 0) that will always be the primary, it's not windows related, I'm just not sure about x11 yet, but probably yes or will need a origin coordinate conversion, but same stuff. Also, with more information, I can show this in the list for user selection, for example, if the monitor friendly name is something I don't recognize, or maybe like my linux sample with "VirtualX" all over the place, the user will be able to see the current resolution, something like:

CABBAGE - 1920x1080 60hz scale 2.0 (-1920, 0) // HiDPI/Retina, left monitor
FQCADD - 1920x1080 120hz (-960, -1080) // top monitor
23MP55 - 1920x1080 60hz primary (0, 0)

And if I feel fancy, I could draw into Canvas all displays layed out using origin/resolution, so I'm not sure I want just the boolean anymore... But I'm still discovering what I'll really do.

[FrogTheFrog]

I would suggest to keep it simple for now (don't make the mistake I did...) and add new functionality little by little.

My other problem is that I have too much code in the PR already and while I'm not against your suggestions, I don't want it to become a scope creep where I keep adding and adding stuff non-stop. I just want to merge my stuff already :').

That being said I could add current display mode to device_info_map_t and position, just no scaling info yet though.

[FrogTheFrog]

Actually please find out what data you really want and then let's discuss it.

Also I think I've misunderstood it. If you want to reuse the "extended" device_info_t, then please do. I thought about something else...

(I really should go to sleep...)

[Hazer]

New proof-of-concept, only working in macOS right now:

• Copied many things from Add new API to modify display devices for Windows #2032 to try and feel what I could use;
• Rebased on my refactor(ui): break down config.html into smaller pieces #2491;
• Uses new component encapsulation
• Changes how the json is loaded, we now avoid mixing config_file data with rendering data
• Shows details for the current selected screen
• Still very experimental, but seems a bit better, thanks to @FrogTheFrog work
• Next probably the basics for Window to compile, then Linux

Screenshot

[Hazer]

(I really should go to sleep...)

Hope you got some rest, now I'm the one way past the bedtime

[FrogTheFrog]

(I really should go to sleep...)

Hope you got some rest, now I'm the one way past the bedtime

I got slightly sick :D

Got a few points/questions:

1. Should the scale factor be represented in a rational_t? Like we have for refresh rate (would have to rename the type into a generic one). I copied what Windows was doing (they hate floats for some reason) to save the settings and just wondering whether we should apply the same pattern.
2. Should the scale_factor really be part of the resolution_t and not exist on it's own in the current_settings? On Windows it is not considered a part of the display mode and persists between mode changes (like the HDR setting).
3. One pitfall of the rational_t is that the denominator always has to be non-zero. Otherwise you create a black hole when converting it to float (which you already did)...

[FrogTheFrog]

Stop commiting and go to sleep!

[Hazer]

@FrogTheFrog Hope your feeling better! I got some rest, and some food poisoning later on... So I got even more rest!

Those are great points, that I'm not sure how to answer myself, and I probably should do a closer look into windows and your work so far, it will be easier if I understand from the same PoV, but at first glance:

1.

Not sure, macOS handles as Double, Linux seems like a mess, x11 and wayland have different implementations, but it also seems to change between DEs, it seems Gnome uses Integer, while KDE uses Float and supports Fractional Scaling, while every other tool may have it's own standards, in my mind initially, double/float was fine. I think we can use the same pattern you're using with windows, but can you show me how to convert between double and rational_t? I was lazy to find out how do you do it properly :D

2.

I see what you mean, but I'm not sure yet, let's take my MacBook screen for example, 1280x800, @2x scaling, which means, my actual resolution is 2560x1600, that's my actual pixel count, but both "resolutions" are valid, what changes it's the scale, but if you see the macbook settings it says 1280x800, it's the default Retina (HiDPI) resolution.

So when streaming, that information is useful, as I may want to stream in 1280x800@1x or in native 1280x800@2x==2560x1600, and it seems deeply tied to resolution to me, but as you pointed out, it doesn't for windows, kinda backwards even, windows mostly always just renders 1:1 with the resolution and that's it. In my mind, they are tied, but in reality it may be an external like HDR.

The only issue is when changing "resolutions" in macOS, at least before Sonoma, we are only changing the scale actually, the native pixel count will still be 2560x1600 when I'm rendering my screen in "1440x900" (it's called 'scaled looking like 1440x900' in the settings), it just changes the scale to 1.??x.

Sooo, if we think in math terms it's represented as resolution * scale_factor == native_resolution, so I guess, part of the mode changes because macOS and Linux may use, but not part of resolution itself? Meanwhile windows will always be 1x scale, and may even hardcode this value? What do you think?

3.

ahhahaha yeah, I was sure I wasn't using the right values here, I just wanted to test it and be done with it for the day tbh, there's another APIs to get this value, so I wasn't even sure I should use this one. Actually, I need this info, given 60hz, which is my monitor refresh rate, what are the correct values for numerator and denominator the way it's currently planned?

====

I probably should try implementing the actual mode change for macOS as a PR for the libdisplay and actually map the available APIs for mode change in macOS, I already have some idea, it's mostly around the crusty CGDisplayMode CoreGraphics API, but I don't really have used it myself. It's good to refine the public API if I actually try to use the API in practice, here I mostly just reused your structs without much brain power in place.

PS: Sorry about this essay size

[FrogTheFrog]

Hmmm, conversion could look like this (maybe we should add a static method static rational_t fromFloat(float value) so that it can be improved later on):
numerator = static_cast<int>(value * 1000); denominator= 1000;
as for converting back, it's simply numerator/denominator (again, maybe in a const method to check for denominator!= 0).

We can hardcode it 1x for Windows (I will do the same until I figure how to read it), but I would still like to keep it out of the resolution_t and maybe even display_mode_t. It's not about whether it's part of the resolution (as a whole) or not, but whether if it's part of the OS API. For example, it needs to be changed separately on and Linux KDE (if I remember correctly) and Windows:

so I would like to keep it separated to that no unnecessary stuff is passed to functions (separation of concerns). Unless it's part of the MacOS API and then we need to think of a better structure.

It's up to you really. It can be numerator = 60; denominator= 1; or numerator = 60000; denominator= 1000;.

====

I'm still working on the libdisplaydevice so plz wait a little (or a lot if you can since I'm busy IRL) :D.

The plan is still to make it platform agnostic more or less and expose only the session_t (will be renamed) class with some additional methods (namely the enum_available_devices and get_display_name equivalent).
settings_t (will also be renamed) will have a shared interface between OS'es (kinda like it does right now) and will be responsible for applying the parsed_config_t.
Everything behind settings_t is up to you to decide how to implement or how it will look like. The methods in displaydevice.h will not be exposed directly and will be private.

With that said, I would recommend to start working on branch based on my PR since the logic will not change by a lot and you'll find some BS most likely that you will have to hack around (like the audio context capture I have to do on Windows).

[FrogTheFrog]

I think I am (or we) approaching this from a wrong point. We don't need to expose the mode structure at all (I would prefer to keep it hidden at this point since it's very OS dependent). I still want to reuse the resolution_t and other types in the code so it should remain as basic as possible.

How about we have the following:

  enum class device_state_e {
    inactive,
    active,
    primary
  };

  enum class hdr_state_e {
    unknown,
    disabled,
    enabled
  };

  struct resolution_t {
    unsigned int width;
    unsigned int height;
  };

  struct rational_t {
    unsigned int numerator;
    unsigned int denominator;
  };

  struct position_t {
    int x;
    int y;
  };

  struct device_info_t {
    std::string display_name;
    std::string friendly_name;
    device_state_e device_state;
    hdr_state_e hdr_state;
    resolution_t resolution;
    rational_t refresh_rate;
    rational_t scaling; // resolution_scale, scale or what's the best name?
    position_t origin_point;
  };
  
  using device_info_map_t = std::map<std::string, device_info_t>;

[Hazer]

1/3

Ahh, thanks! Got it now, I had another thing in mind. In my case it will be numerator = screen.maximumFramesPerSecond; denominator= 1; then, as macOS already handles this as Integer in the decimal range. It has another way of doing it with fractionally values but this way is even less work.

2

I did investigate how CGDisplayMode works, and you're completely right. There's no scale there in macOS either, we cannot get neither the scale nor the native pixel resolution at this point, the scale is a property of Display/Screen and all the modes are already calculated with scale in mind. We could even say that the Scale changes WHEN the mode changes, not the other way.

Regarding the UI, here are KDE, Ubuntu(Gnome or unity I guess) and macOS as example, and probably why my mind got stuck with the idea that it was correlated with resolution or mode:
kde:

gnome:

macOS 1280x800@2x (2560x1600):

macOS 1440x900@1.??x

How about we have the following:

Looks perfect to me. I'd use resolution_scale to make it explicit, and probably I'd add to device_info_t the std::string id, instead of only relying on it as the key for device_info_map_t, this way I could just pass this struct along and have everything I need, but it's not a necessary redundancy, it's just my preference.

I would recommend to start working on branch based on my PR

I can do that, but I can wait too, if we use the above structs as protocol, so libdisplaydevice exposes device_info_t and children, I can easily move forward without giving you trouble, be it here or actually starting a PR pointing to your PR.

Also, thanks for the patience and your time so far!

[FrogTheFrog]

Looks perfect to me. I'd use resolution_scale to make it explicit, and probably I'd add to device_info_t the std::string id, instead of only relying on it as the key for device_info_map_t, this way I could just pass this struct along and have everything I need, but it's not a necessary redundancy, it's just my preference.

This is a completely valid pattern and I don't mind, but can we please use device_id instead of id? I want to be explicit that this id belongs to a device and not a display. There is a difference on Windows...

Anyway, that's all. Best of luck to us both!

[Hazer]

This is a completely valid pattern and I don't mind, but can we please use device_id instead of id? I want to be explicit that this id belongs to a device and not a display. There is a difference on Windows...

Sure, even better!

[codecov]

Codecov Report

Attention: Patch coverage is 0% with 54 lines in your changes are missing coverage. Please review.

Project coverage is 8.18%. Comparing base (4b6ff37) to head (b03b874).

Additional details and impacted files

@@            Coverage Diff             @@
##           nightly   #2490      +/-   ##
==========================================
+ Coverage     6.99%   8.18%   +1.19%     
==========================================
  Files           87      49      -38     
  Lines        17679    8181    -9498     
  Branches      8399    3818    -4581     
==========================================
- Hits          1237     670     -567     
+ Misses       13745    7125    -6620     
+ Partials      2697     386    -2311     

Flag	Coverage Δ
Linux	?
Windows	?
macOS-12	?
macOS-13	?
macOS-14	8.18% <0.00%> (-0.06%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
src/config.h	0.00% <ø> (ø)
src/platform/common.h	55.26% <ø> (+24.01%)	⬆️
src/display_device/dd.h	0.00% <0.00%> (ø)
src/confighttp.cpp	0.65% <0.00%> (-0.19%)	⬇️
src/platform/macos/display_options.mm	0.00% <0.00%> (ø)

... and 72 files with indirect coverage changes

[Hazer]

I'll split this PR next month or so, I think it will be better to postpone, now that I have a better understanding of the whole stack, and I have other improvements planned to be merged first.

[FrogTheFrog]

Heyo, I have reached a point where I am implementing the last function for the OS abstraction layer - the enumeration function. After thinking for a while, I would like to have the following structure in the end:

namespace display_device {
  /**
   * @brief The device's HDR state in the operating system.
   */
  enum class HdrState {
    Disabled,
    Enabled
  };

  /**
   * @brief Display's resolution.
   */
  struct Resolution {
    unsigned int m_width {};
    unsigned int m_height {};
  };

  /**
   * @brief An arbitrary point object.
   */
  struct Point {
    int m_x {};
    int m_y {};
  };

  /**
   * @brief Enumerated display device information.
   */
  struct EnumeratedDevice {
    /**
     * @brief Available information for the active display only.
     */
    struct Info {
      Resolution m_resolution {}; /**< Resolution of an active device. */
      float m_resolution_scale {}; /**< Resolution scaling of an active device. */
      float m_refresh_rate {}; /**< Refresh rate of an active device. */
      bool m_primary {}; /**< Indicates whether the device is a primary display. */
      Point m_origin_point {}; /**< A starting point of the display. */
      std::optional<HdrState> m_hdr_state {}; /**< HDR of an active device. */
    };

    std::string m_device_id {}; /**< A unique device ID used by this API to identify the device. */
    std::string m_display_name {}; /**< A logical name representing given by the OS for a display. */
    std::string m_friendly_name {}; /**< A human-readable name for the device. */
    std::optional<Info> m_info {}; /**< Additional information about an active display device. */
  };

  /**
   * @brief A list of EnumeratedDevice objects.
   */
  using EnumeratedDeviceList = std::vector<EnumeratedDevice>;
}  // namespace display_device

I have trimmed the fat and kept it simple (the denominator was a bad idea for the public interface...). Since it will now live in the library, I will no longer expose it as a JSON via NLOHMANN API, instead all the types will be serializable via the public toJson(...) and fromJson(...) (to hide the dependency).

As you can see, I am using the m_ with a snake case, which is not common in web development. I was planning on removing the prefix when converting to json string, but maybe it should also be converted to camel case? I would like to hear your opinion on this :D .