[RFC] XWayland Multi DPI support
Needs ReviewPublic

Authored by romangg on Jan 23 2019, 7:45 PM.

Details

Reviewers
None
Group Reviewers
KWin
Summary

This patch enables for all HiDPI aware X clients in our Wayland session Multi
DPI support.

The following patch to XWayland needs to be applied in order to work:
https://gitlab.freedesktop.org/xorg/xserver/merge_requests/111

More information about the internal mechanism can be found in the description
and commit message of this merge request.

Additionally for common browsers to work the workspace should set GDK_SCALE to
the maximum scale factor of the output configuration.

A next step would be to do this programmatically. For example workspace could
listen to a DBus signal sent by KWin when the maximum scale factor changes.

Watch it in action here: https://youtu.be/a8HPS3LbCCg

BUG: 389191

Test Plan

Manually

Diff Detail

Repository
R108 KWin
Branch
xwlScaling
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 7478
Build 7496: arc lint + arc unit
romangg created this revision.Jan 23 2019, 7:45 PM
Restricted Application added a project: KWin. · View Herald TranscriptJan 23 2019, 7:45 PM
Restricted Application added a subscriber: kwin. · View Herald Transcript
romangg requested review of this revision.Jan 23 2019, 7:45 PM
romangg edited the summary of this revision. (Show Details)Jan 23 2019, 7:51 PM

I'm still a bit skeptical of a few parts, it makes an assumption that X clients all scale themselves perfectly, something that really doesn't hold but I'll see how the discussion on the xwl thread pans out.

I don't like how the window positioning means we have a slightly weird mix of logical and native co-ordinates all positioning code.
I'm hoping there will be some feedback on your xwayland patch for ways to avoid that.

plugins/scenes/opengl/scene_opengl.cpp
1163

This needs some work, but I understand what you're doing.

Logically we should have some sort of:

if (bufferSize != windowLogicalSize * outputScale) {

setFilter(Linear)

}

Though I remember trying it and couldn't see a difference.

We can split that out this patch and ship that.

plugins/scenes/qpainter/scene_qpainter.cpp
284

I would expect one of the QPainter::setRenderHints will fix this.

As for the xwayland patch.

That's pretty much exactly what I had envisioned from my comment on the bugreport.
So +++. I like that it's opt-in. I think that's an important feature..

I wouldn't use the term "multi-dpi".
Generally we use that to mean "multiple outputs of different DPI". Which works in your video, but not because of anything in xwayland - xwayland just gets a single scale factor.

zzag added a subscriber: zzag.Jan 23 2019, 9:59 PM
zzag added inline comments.
abstract_output.cpp
98 ↗(On Diff #50141)

Could you please explain what it is doing?

plugins/scenes/opengl/scene_opengl.cpp
1486–1488

I think GL_LINEAR shouldn't be used in this case. Why mipmaps anyway? I don't see where the number of levels is set.

xcbutils.h
55–85

Please follow coding style, no short names.

64

It will return a wrong value if qreal is passed.

1816

Please follow coding style.

leezu added a subscriber: leezu.Jan 24 2019, 1:12 AM
romangg added inline comments.Jan 24 2019, 12:09 PM
abstract_output.cpp
98 ↗(On Diff #50141)

If scale is 1.000000000000 I wanted to make sure it's not ceiled up to 2. But it's probably not necessary and unrelated to the main idea here. So I'll remove it.

main_wayland.cpp
409

Pkgconfig wouldn't work on user installs, since it's something installed with dev packages.

I'm thinking about adding a -version argument to XWayland, that just returns its version in the raw format the same way it is returned by xcb_get_setup(c)->release_number above (for example 12099000) and then quits.

Then before launching XWayland with all arguments compositors would need to launch XWayland -version first and see if an error is returned because the argument is not known or if not, what's the version number.

Of course one could also just directly try to launch XWayland with the option, but then one would need to redo all the process afterwards in case it fails.

plugins/scenes/opengl/scene_opengl.cpp
1163

Good that you mention it. I forgot to talk about it in the description.

Indeed this stuff was one of the worse problems and I don't yet fully understand it. Without these changes including the generateMipmaps call the XWayland texture of a HiDPI client looked blurry on the LoDPI display.

This makes in some way sense since mip-maps are used for down-scaling. But why do we not have the same problem with Wayland native buffers of larger scale factor and parts of them being shown on LoDPI screens? There everything looks crisp although it also needs to be downscaled.

Maybe does the glViewport call in our EGl GBM backend do this automatically? But if yes, why not for the XWayland buffers. They should behave in this aspect exactly the same.

1486–1488

From my research the number of levels is not something to be set, but it's determined by the size of the texture to be mip-mapped. A 2x2 texture has 2 mip-maps (2x2, 1x1), a 4x4 texture has 3 (4x4, 2x2, 1x1).

Don't know why our code tries to set levels.

plugins/scenes/qpainter/scene_qpainter.cpp
284

You expected correctly. :)

The Antialising one didn't help, but SmoothPixmapTransform did.

romangg updated this revision to Diff 50179.Jan 24 2019, 12:11 PM
  • Use QPainter render hint
  • Remove ceil scale deduction
  • rename Xwl arg according to fdo merge request change
romangg marked 2 inline comments as done.Jan 24 2019, 12:13 PM
plugins/scenes/qpainter/scene_qpainter.cpp
284

Cool, that's relevant with just wayland scaling so you can ship a line adding that in a separate patch and I'll approve.

plugins/scenes/qpainter/scene_qpainter.cpp
284

I think we can just do this once at the start of the scene paint.

If pixmap sizes match it won't have any performance impact - and I think it should still be relevant for wayland on wayland if the client scale doesn't match the output scale.

ngraham added a subscriber: ngraham.May 6 2019, 9:53 PM