libmacchina icon indicating copy to clipboard operation
libmacchina copied to clipboard

Windows: Implement `resolution`

Open Carterpersall opened this issue 2 years ago • 16 comments

Hello, I'm a contributor to https://github.com/lptstr/winfetch (you've talked to the maintainer in #112), and would like to start applying some of the experience I gained there to this project.

  • Uses a combination of my implementation in https://github.com/lptstr/winfetch/pull/156/ and this one
    • SetProcessDpiAwareness tells the OS to give the program the "real" resolution of each monitor instead of the scaled resolution
    • Uses EnumDisplayMonitors to get the information of each monitor
      • Uses a callback function to get the information on each monitor using GetMonitorInfoW and store the result in a vector

TODO:

  • ~[ ] Implement my method in https://github.com/lptstr/winfetch/pull/156~
    • ~Only works in admin instances of PowerShell, but I need to test if it's the same for Rust~
      • ~It is the same in Rust, and it seems that the crate WinReg does not anticipate a registry key requiring special permissions as it is not working properly~
    • Probably more effort than it's worth

Carterpersall avatar Mar 20 '23 18:03 Carterpersall

Why EnumDisplayMonitors instead of EnumDisplayDevicesW + EnumDisplaySettingsW (ENUM_CURRENT_SETTINGS)?

  1. EnumDisplayDevicesW is not affected by DpiAwareness, so no SetProcessDpiAwareness needed.
  2. EnumDisplayDevicesW doesn't use callback, so it is easier to use
  3. EnumDisplayDevicesW provides refresh rate

While EnumDisplayMonitors is still useful to fetch scaled resolutions. with SetProcessDpiAwareness(PROCESS_DPI_UNAWARE), if needed

CarterLi avatar Mar 21 '23 03:03 CarterLi

Why EnumDisplayMonitors instead of EnumDisplayDevicesW + EnumDisplaySettingsW (ENUM_CURRENT_SETTINGS)?

Because I didn't know it existed. The Win32 API is such a chaotic mess of useful functions that something always seems to slip by. I'll see if I can throw together another implementation using those functions.

Carterpersall avatar Mar 21 '23 14:03 Carterpersall

This is an example that fetchs both pixel resolutions, scaled resolutions and refresh rates.

https://github.com/LinusDierheimer/fastfetch/blob/b3da6b0e89c0decb9ea648e1d98a75fa6ac40225/src/detection/displayserver/displayserver_windows.c#L48

CarterLi avatar Mar 21 '23 15:03 CarterLi

There are other interesting things. For example: fetching OS caption without querying WMI: https://github.com/LinusDierheimer/fastfetch/blob/b3da6b0e89c0decb9ea648e1d98a75fa6ac40225/src/detection/os/os_windows.cpp#L34

CarterLi avatar Mar 21 '23 15:03 CarterLi

This is an example that fetchs both pixel resolutions, scaled resolutions and refresh rates.

LinusDierheimer/fastfetch@b3da6b0/src/detection/displayserver/displayserver_windows.c#L48

I got an implementation working, thanks for the tip though

There are other interesting things. For example: fetching OS caption without querying WMI: LinusDierheimer/fastfetch@b3da6b0/src/detection/os/os_windows.cpp#L34

Yeah, there are a lot of things that most programs default to calling WMI for that can be found by just using the registry. That's one thing I intend on working on

While working on the new implementation, I discovered that EnumDisplayDevicesW also returns the system's display adapters, giving yet another way to retrieve the system's GPUs.

Carterpersall avatar Mar 21 '23 17:03 Carterpersall

Implementation Speeds:

  • Command used: hyperfine --warmup 20 --prepare "cmd" --min-runs 500 ".\macchina.exe --show resolution"
    • Compiled in release mode

Plugged in:

  • EnumDisplayDevicesW
    • 14.5 ms
  • EnumDisplayMonitors
    • 14.1 ms
  • WMI
    • 203 ms

On Battery life:

  • EnumDisplayDevicesW
    • 15.2 ms
  • EnumDisplayMonitors
    • 13.5 ms
  • WMI
    • 227.2 ms

When heavily throttled (used power plans to downclock CPU to 1.06 GHz to simulate a slow computer)

  • EnumDisplayDevicesW
    • 49.2 ms
  • EnumDisplayMonitors
    • 47.3 ms
  • WMI
    • 697.3 ms

Carterpersall avatar Apr 13 '23 19:04 Carterpersall

Besides being slow, WMI Win32_VideoController reports one resolution per GPU adapter only. I see no benefits use WMI here.

CarterLi avatar Apr 14 '23 04:04 CarterLi

Besides being slow, WMI Win32_VideoController reports one resolution per GPU adapter only. I see no benefits use WMI here.

Yeah, I probably should've checked for that before adding it. I've removed the WMI implementation in the most recent commit

Carterpersall avatar Apr 14 '23 16:04 Carterpersall

@grtcdr I don't think I'm qualified to review this. I don't have enough experience with windows to comment on this. However I can help with the testing of this.

uttarayan21 avatar Apr 24 '23 06:04 uttarayan21

Neither am I, that's okay. I'd appreciate it if someone else (@CarterLi, perhaps) gave a final review before merging.

grtcdr avatar Apr 24 '23 12:04 grtcdr

Sorry, but I'm not familiar with rust.

This is what I got on my MBP

image
  1. macchina should use the same format on different platforms, including on Windows. That is: pixelHxpixelV@refreshRatefps (as scaledHxscaledV). However the code seems to use another format
  2. I still see no benefits adding EnumDisplayMonitors as a backup, as EnumDisplaySettingsW usually won't fail (it's supported in Windows 2000+), but adding backups generally complicates the code.
  3. The code seems verbose. I don't think it's necessary to push everything into a vector

CarterLi avatar Apr 24 '23 17:04 CarterLi

1. macchina should use the same format on different platforms, including on Windows. That is: `pixelH`x`pixelV`@`refreshRate`fps (as `scaledH`x`scaledV`). However the code [seems to use another format](https://github.com/Macchina-CLI/libmacchina/pull/147/files#diff-3b7ba22f3645054b5b1c07651b4c378e01c0f8eda295e24e4bedf4e6e3a79382R232)

Yeah, I probably should've checked the current formatting when implementing it. I've changed the formatting of the output to match the rest of the program.

2. I still see no benefits adding `EnumDisplayMonitors` as a backup, as `EnumDisplaySettingsW` usually won't fail (it's supported in Windows 2000+), but adding backups generally complicates the code.

I can remove the second implementation if you wish, but I don't think it complicates the code much due to the clear separator between the implementations.

3. The code seems verbose. [I don't think it's necessary to push everything into a vector](https://github.com/Macchina-CLI/libmacchina/pull/147/files#diff-3b7ba22f3645054b5b1c07651b4c378e01c0f8eda295e24e4bedf4e6e3a79382R181)

A vector has to be used in the selected location due to the need for a while statement instead of an iterator. I was able to use an iterator instead of a vector for the resolution variable, but I found that the implementation caused a significant decrease in readability with no tangible speed improvement.

Carterpersall avatar Apr 25 '23 16:04 Carterpersall

I can remove the second implementation if you wish, but I don't think it complicates the code much due to the clear separator between the implementations.

It's useless. That's all.

A vector has to be used in the selected location due to the need for a while statement instead of an iterator. I was able to use an iterator instead of a vector for the resolution variable, but I found that the implementation caused a significant decrease in readability with no tangible speed improvement.

It has nothing to do with vectors or iterators. You call EnumDisplayDevicesW. You get a DISPLAY_DEVICEW instance. You pass DISPLAY_DEVICEW::DeviceName into EnumDisplaySettingsW. You are done.

https://github.com/CarterLi/fastfetch/blob/e5f851dcbb94de35c34fb8c5e3dd8300bb56a1cc/src/detection/displayserver/displayserver_windows.c#L49

CarterLi avatar Apr 25 '23 16:04 CarterLi

It has nothing to do with vectors or iterators. You call EnumDisplayDevicesW. You get a DISPLAY_DEVICEW instance. You pass DISPLAY_DEVICEW::DeviceName into EnumDisplaySettingsW. You are done.

CarterLi/fastfetch@e5f851d/src/detection/displayserver/displayserver_windows.c#L49

I see what you mean now, I have no idea why I didn't do it like that in the first place.

Carterpersall avatar Apr 25 '23 17:04 Carterpersall

There is actually another method by using HDC and GetDeviceCaps.

CarterLi/fastfetch@4d7cb25#diff-1f993416a36137ee7af25d94be4928701d89ca08d9ab322fcb3eb0b5ec6d80b6R39

I dont know if it is faster than EnumDisplayMonitors. The code is much simplier at least.

I created an implementation of this and the EnumDisplayDevicesW + EnumDisplaySettingsW combo and it seems that the combo is consistently faster by about 0.5ms

If the resolution is not scaled. Don't print (as {}x{})

That also should be fixed now

Carterpersall avatar Apr 26 '23 17:04 Carterpersall

I created an implementation of this

No. Although fastfetch has switched to QueryDisplayConfig, you don't have to. Just keep using EnumDisplayDevicesW and GetDeviceCaps.

DISPLAY_DEVICEW displayDevice;

//...

HDC hdc = CreateICW(displayDevice.DeviceName, NULL, NULL, NULL);
int width = GetDeviceCaps(hdc, HORZRES);
int height = GetDeviceCaps(hdc, VERTRES);
DeleteDC(hdc);

fastfetch has switched to QueryDisplayConfig because QueryDisplayConfig provides decimal refresh rates which is used by some monitors. In addition, QueryDisplayConfig provides monitor name, which can be used to match the information of fastfetch's brightness module (like the backlight readout in macchina).

image

If you don't need these features, you don't need QueryDisplayConfig

CarterLi avatar Apr 27 '23 04:04 CarterLi