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

HiDPI: Physical size from xrandr may be unreliable (can be 0mm x 0mm) #543

Closed
PeterCxy opened this issue Jun 1, 2018 · 23 comments · Fixed by #544
Closed

HiDPI: Physical size from xrandr may be unreliable (can be 0mm x 0mm) #543

PeterCxy opened this issue Jun 1, 2018 · 23 comments · Fixed by #544
Labels
B - bug Dang, that shouldn't have happened C - in progress Implementation is proceeding smoothly D - easy Likely easier than most tasks here DS - x11

Comments

@PeterCxy
Copy link

PeterCxy commented Jun 1, 2018

I ran into this issue when trying to run Alacritty, which uses winit, on my GPD Pocket with ArchLinux running GNOME 3.28.2 on Xorg. When starting the program, winit will report the following error:

[winit X11 error] XError {
    description: "BadValue (integer parameter out of range for operation)",
    error_code: 2,
    request_code: 12,
    minor_code: 0
}

Further investigation showed that XConnection::get_output_info returned inf for the HiDPI factor. Reading the source code I realized that the value is calculated from the resolution and the physical size of the screen, which are all fetched from xrandr. Trying to run xrandr directly to see what's happening, and I ended up with

Screen 0: minimum 320 x 200, current 2496 x 1560, maximum 8192 x 8192
DSI-1 connected primary 2496x1560+0+0 right (normal left inverted right x axis y axis) 0mm x 0mm
   1200x1920     60.38*+
   1920x1200     59.88    59.95  
   1920x1080     60.01    59.97    59.96    59.93  
   1600x1200     60.00  
   1680x1050     59.95    59.88  
   1400x1050     59.98  
   1600x900      59.99    59.94    59.95    59.82  
   1280x1024     60.02  
   1400x900      59.96    59.88  
   1280x960      60.00  
   1440x810      60.00    59.97  
   1368x768      59.88    59.85  
   1280x800      59.99    59.97    59.81    59.91  
   1280x720      60.00    59.99    59.86    59.74  
   1024x768      60.04    60.00  
   960x720       60.00  
   928x696       60.05  
   896x672       60.01  
   1024x576      59.95    59.96    59.90    59.82  
   960x600       59.93    60.00  
   960x540       59.96    59.99    59.63    59.82  
   800x600       60.00    60.32    56.25  
   840x525       60.01    59.88  
   864x486       59.92    59.57  
   700x525       59.98  
   800x450       59.95    59.82  
   640x512       60.02  
   700x450       59.96    59.88  
   640x480       60.00    59.94  
   720x405       59.51    58.99  
   684x384       59.88    59.85  
   640x400       59.88    59.98  
   640x360       59.86    59.83    59.84    59.32  
   512x384       60.00  
   512x288       60.00    59.92  
   480x270       59.63    59.82  
   400x300       60.32    56.34  
   432x243       59.92    59.57  
   320x240       60.05  
   360x202       59.51    59.13  
   320x180       59.84    59.32

Note that the physical size reported by xrandr is absurd, since it is shown as 0mm x 0mm. However, using xdpyinfo I got the normal physical size reported without problem:

screen #0:
  dimensions:    2496x1560 pixels (207x130 millimeters)
  resolution:    306x305 dots per inch
  depths (7):    24, 1, 4, 8, 15, 16, 32
  root window id:    0x16f
  depth of root window:    24 planes

I believe this should be some issue with xrandr, however I have saw some other cases with similar problems without a fix:

https://bbs.archlinux.org/viewtopic.php?id=232288
http://xpra.org/trac/ticket/728

I also confirmed this problem with @farseerfc who also owns the same computer as mine.

Since all the other programs on my system work just fine (including the ones using GTK3, QT5, Java Swing, which all support HiDPI), I am wondering if it is possible for winit to fallback to some other mechanism to determine hidpi factor or simply set it to 1 if the physical screen size cannot be detected. Although this should be a problem of somewhere else, I think we can at least get winit to run on these strange devices...

@francesca64
Copy link
Member

Great job investigating this, but unfortunately you're one day too late: alacritty/alacritty#1341

It's interesting that xdpyinfo works, since that really does seem to suggest that there's a bug with RandR. xdpyinfo uses DisplayWidthMM, but we can't use that for anyone with multiple monitors, where the measurements will be the combined value of all monitors (since that's how multi-monitor is implemented using XRandR; it conjoins multiple monitors into one big core protocol screen).

number of screens:    1

screen #0:
  dimensions:    8320x1440 pixels (2191x379 millimeters)

Though, we could use this as an additional fallback for anyone with only one active output.

@francesca64 francesca64 added B - bug Dang, that shouldn't have happened DS - x11 C - in progress Implementation is proceeding smoothly D - easy Likely easier than most tasks here labels Jun 1, 2018
@PeterCxy
Copy link
Author

PeterCxy commented Jun 1, 2018

@francesca64 What about adding the ability to override the scale factor from some environment variable? I later realized that my GTK+ and QT programs work fine only because they read the scale factor from env variables that I have written to /etc/profile.

@francesca64
Copy link
Member

That sounds like a good idea. Which specific env vars?

@PeterCxy
Copy link
Author

PeterCxy commented Jun 1, 2018

@francesca64 AFAIK every toolkit have their own.. such as GTK's GDK_SCALE or QT's QT_SCALE_FACTOR. Maybe we could have something like WINIT_SCALE_FACTOR? I'm not sure if it is a good idea for winit to do such things or just leave this for the application developers though.

@chrisduerr
Copy link
Contributor

I've opened a PR to "fix" this by returning 1.0 when either width or height are 0. Adding an environment variable should be simple so I can add that to the PR if a consensus about it has been found. (PR: #544)

@chrisduerr
Copy link
Contributor

chrisduerr commented Jun 1, 2018

I think having a WINIT_SCALE_FACTOR might be a good idea, however one of the issues with having environmental variables for that kind of stuff is that it's hard to even know they exist. So they probably should be documented properly.

Users might not even know that an application uses winit so it always relies on at least the developer of the application knowing about it so they can refer to the variable when an issue is opened with their application.

@quininer
Copy link

quininer commented Jun 1, 2018

I would suggest that hidpi_factor return Option<f32>, but this is a break change.

@chrisduerr
Copy link
Contributor

With an Option or Result it would be possible for the application itself to actually do something about it. Just swallowing an error definitely doesn't seem perfect but introducing an API breakage and "forcing" consumers of the API to handle an option/error when 99% of the time they'd probably just go with using 1.0 as the DPI isn't optimal either. However calling hidpi_factor probably isn't a super common thing in an application, so handling that with an unwrap_or wouldn't introduce much additional complexity.

@francesca64
Copy link
Member

Option<f32> isn't viable. With the upcoming DPI-aware winit, the DPI factor is used internally in myriad places, and methods deal in logical pixels (which on X11 and Windows, have to be converted within winit), so having applications and winit doing math using two different DPI factors would be bad news.

Additionally, the DPI factor should be specified per-monitor, not per-window.

Considering that along with the following:

  • Getting the DPI factor can't fail on any of the other platforms
  • The issue seems to be the result of an XRandR bug (or incorrect EDID information), and is hopefully rare
  • We can try to fall back to DisplayWidthMM in the case of only having one monitor, which is probably most people
  • Most application developers are unlikely to do anything other than default to 1.0, or even know what a good solution would be

While I'm not ultimately opposed to allowing consumers to override DPI factors, correctly doing that would require some engineering, and it would likely only be desirable to support it on X11 (X11 doesn't really have a native concept of logical pixels, so it's easy to do whatever we want; taking the same approach on macOS would be quite a mess).

@PeterCxy
Copy link
Author

PeterCxy commented Jun 2, 2018

Then I would suggest just to allow overriding from WINIT_SCALE_FACTOR, since the other toolkits are already doing this and seem to be totally fine.

And also, sometimes we HiDPI users do set different scale factors to different programs, due to their different design. So I think allowing overriding through a environment variable would be more viable in this case.

@francesca64
Copy link
Member

Well, WINIT_SCALE_FACTOR, or WINIT_DPI_FACTOR? I'd like the env variable to be consistent with winit's naming scheme. Though, that would actually entail WINIT_HIDPI_FACTOR... I will admit that I'm more fond of "scale factor" in general, but I haven't wanted to bikeshed over it in #105, since that discussion is drawn-out as it is.

While this solution is imperfect (as it overrides for all monitors) it has the benefit of being easy to use and only requiring a few lines of code be added to winit, so I'm fine with it if it makes people happy.

@chrisduerr
Copy link
Contributor

I'd be fine with WINIT_HIDPI_FACTOR if that's most in line with winit's naming scheme, should we just go for that and be done with it?

@francesca64
Copy link
Member

francesca64 commented Jun 2, 2018

Sounds good to me. I'm guessing you don't want to add the DisplayWidthMM/DisplayHeightMM fallback yourself?

@chrisduerr
Copy link
Contributor

I can try.

@francesca64
Copy link
Member

Well, I wouldn't want to burden you. It would only take me 5 minutes to do myself. Though, the only hard thing is knowing that you need to get the screen number from XDefaultScreen.

@chrisduerr
Copy link
Contributor

chrisduerr commented Jun 2, 2018

Which platforms should support the WINIT_HIDPI_FACTOR variable? I could implement it for every platform but android/emscripten/ios/windows seem to just always return 1.0 as DPI.

I don't think env variables are super useful on android/ios/emscripten but I can still add it for completeness sake.

@francesca64
Copy link
Member

Don't worry about things currently returning 1.0, since I've already got a branch that fixes that (not on Android yet though; I'm still looking into that). However, for now I'd prefer we just stick to X11. macOS and Windows would require additional math and complexity with resizing (on X11, you only need to override the value stored in MonitorId, and everything will work). For mobile and web, I doubt overriding the DPI factor is a good idea; X11 is the only platform where the system-supplied DPI factor is merely a suggestion.

Though, I'm not sure about the situation with Wayland. You can ask @vberger about that (and more generally about the okay-ness of this design).

@chrisduerr
Copy link
Contributor

I'd welcome any comments on this @vberger, I'm probably just going to introduce these changes for X11 first. So that should be good until any other platform wants to hook into the env variable.

@chrisduerr
Copy link
Contributor

@francesca64 where would be the best place to document the env variable? I've added it to the changelog but it seems like this should probably be at least put into the documentation too. I can add it to winit::MonitorId::get_hidpi_factor and winit::Window::hidpi_factor but I'm not sure if these two places are ideal.

@francesca64
Copy link
Member

Those are basically the only places we have where you could put it, though if I'm not lazy I'll make some sort of DPI guide soon (no promises though!).

Just be sure to use the ## Platform-specific sub-header. It's tragically underused.

@chrisduerr
Copy link
Contributor

TIL that thing exists. I'll probably clean this up properly and add some documentation tomorrow. I might even get to adding the DisplayWidthMM/DisplayHeightMM stuff too. Thanks for the guidance!

@elinorbgr
Copy link
Contributor

elinorbgr commented Jun 3, 2018 via email

@chrisduerr
Copy link
Contributor

Sounds very reasonable to me. Would make sense then to just introduce it for X11, if anyone else is interested in the env variable it should be simple to add it at a later point too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
B - bug Dang, that shouldn't have happened C - in progress Implementation is proceeding smoothly D - easy Likely easier than most tasks here DS - x11
Development

Successfully merging a pull request may close this issue.

5 participants