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

wayland: implement fractional scaling #384

Closed
wants to merge 5 commits into from

Conversation

mahkoh
Copy link

@mahkoh mahkoh commented May 21, 2024

This PR implements fractional scaling.

On KDE and Jay, this makes IntelliJ render crisply at every scale that I have tested, including 125%, 133%, 150%, and 175%.

Under GNOME, IntelliJ will not render crisply at some scales including 175%. This seems to be a bug in GNOME since other applications are also affected by this.

Unfortunately there appear to be a number of "off by one" errors in the drawing logic of awt or swing or the IntelliJ toolkit that causes the interface to be off at certain scales. This is most easily seen at scale 150% where selecting or hovering over certain components causes them to move by one pixel.

Curiously, this seems to happen whenever there is some kind of overlay. For example, in the following screenshot you can see the IntelliJ quick-action tooltip that causes everything below it to shift down by one pixel.

20240521_07h42m42s_grim

One suspicion is that this is caused by client-side compositing that reads back the buffer and, after compositing, does not write back the pixels to the correct position. I'll have to investigate this further.


Additionally, this PR fixes some issues that I had to debug to make sure that they were not caused by my changes.

mahkoh added 3 commits May 21, 2024 07:15
Otherwise the component will hold on to the buffer which will leak it.

Signed-off-by: Julian Orth <[email protected]>
@mahkoh mahkoh mentioned this pull request May 21, 2024
@mahkoh mahkoh force-pushed the fractional-scale branch from f6aa5cf to 889e26c Compare May 21, 2024 09:55
@mahkoh
Copy link
Author

mahkoh commented May 21, 2024

The issue I mentioned above with the UI jumping around should now be fixed. It was indeed so that data was copied between different buffers of the same scale but the x and y coordinates were rounded multiple times in different ways which caused the copies to end up in incorrect places.

There is still an issue that some fill operations fill one pixel too much or too little. But it's only a minor inconvenience.

@mkartashev mkartashev self-requested a review May 22, 2024 07:01
@mahkoh
Copy link
Author

mahkoh commented May 22, 2024

wayland: don't commit partially incomplete buffers

Besides removing some amount of flickering, I believe this also fixes a case where IntelliJ would sometimes segfault when copying the damaged regions to the show buffer.

@mkartashev
Copy link
Collaborator

Under GNOME, IntelliJ will not render crisply at some scales including 175%. This seems to be a bug in GNOME since other applications are also affected by this.

Indeed; see https://mail.openjdk.org/pipermail/wakefield-dev/2024-May/000167.html
That,however, already precludes me from working further on this patch up until Gnome/Mutter has resolved those issues as the majority of IDEA users are running Gnome/Mutter. There's no sense in considerably complicating the code without at least some benefit for the great majority of users.

In fact, I implemented the support for fractional scaling (JBR-6969) a few days back and was profoundly disappointed with the overall quality of the result, see this note. With a hindsight, I shouldn't have expected much: after all, when you need a straight vertical line of one pixel and desktop's scale is 175%, you must draw that line 1.75 pixels wide. This will never be as crisp as 1 or 2 or 3 pixels wide.

At this point I believe over-scaling might be a better solution (see JBR-7193): paint at 2x or 4x and let the compositor down-scale to the proper size, assuming it will employ hardware acceleration and decent image shrinking algorithms. At this moment, when Wayland uses fractional scale JBR will slightly over-scale the resulting image (rounding upwards to the nearest integer), so some over-scaling is already present and it already produces better results than with no over-scaling at all.
For example, the left hand side of this image is produced by over-scaling to 200% and the right hand side at desktop scale of 150%:
image

There are several further issues with the patch; to name a few:

  1. When I run SwingSet2 (./build/linux-x86_64-server-release/images/jdk/demo/jfc/SwingSet2/SwingSet2.jar), it crashes with
Exception in thread "AWT-EventQueue-0" sun.java2d.InvalidPipeException: Invalid Image variant
	at java.desktop/sun.awt.image.SurfaceManager.getManager(SurfaceManager.java:82)
	at java.desktop/sun.java2d.SunGraphics2D.drawImage(SunGraphics2D.java:3480)
	at java.desktop/sun.java2d.SunGraphics2D.drawImage(SunGraphics2D.java:3449)
	at java.desktop/javax.swing.plaf.metal.MetalUtils$GradientPainter.paintImage(MetalUtils.java:309)
	at java.desktop/sun.swing.CachedPainter.paint0(CachedPainter.java:217)
	at java.desktop/sun.swing.CachedPainter.paint(CachedPainter.java:114)
	at java.desktop/javax.swing.plaf.metal.MetalUtils$GradientPainter.paint(MetalUtils.java:270)
	at java.desktop/javax.swing.plaf.metal.MetalUtils.drawGradient(MetalUtils.java:223)
	at java.desktop/javax.swing.plaf.metal.MetalMenuBarUI.update(MetalMenuBarUI.java:109)
...
  1. Stylepad (build/linux-x86_64-server-release/images/jdk/demo/jfc/Stylepad/Stylepad.jar) UI bounces during an interactive resize as if it changes its size for a fraction of a second and then it goes back again.
  2. There are quite a few changes in the shared code; so far we have been able to limit this to a tiny change in the repaint manager; changing shared code for the sake of one Toolkit is a big red flag and needs to be studied very carefully.
  3. I'm not sure what are all the ramifications of using one graphics config per window as opposed to per monitor. This also needs to be studied carefully.
  4. There are changes in the handling of the memory buffers, which are probably unrelated to fractional scaling. The current version (sans the patch) crashes under KDE; I will look into that separately.

@mahkoh
Copy link
Author

mahkoh commented May 23, 2024

That,however, already precludes me from working further on this patch up until Gnome/Mutter has resolved those issues as the majority of IDEA users are running Gnome/Mutter. There's no sense in considerably complicating the code without at least some benefit for the great majority of users.

I'm sure the vast majority of IDEA users don't use linux at all and yet IDEA has complicated its code to run on linux.

But in any case, the screenshots at the bottom of this message show that this PR also significantly improves results on GNOME.

In fact, I implemented the support for fractional scaling (JBR-6969) a few days back and was profoundly disappointed with the overall quality of the result, see this note.

I did test that code but the implementation seems to have been incorrect. The results were sometimes blurry even on non-GNOME compositors.

With a hindsight, I shouldn't have expected much: after all, when you need a straight vertical line of one pixel and desktop's scale is 175%, you must draw that line 1.75 pixels wide. This will never be as crisp as 1 or 2 or 3 pixels wide.

AWT will always round such things to integers. I.e. a 1 java-pixel line will be rounded up to 2 buffer-pixels when drawing at 175% scale. I can assure you that everything is 100% crisp on non-GNOME compositors.

This screenshot shows RustRover rendered at 175% scale:

20240523_12h19m16s_grim

Here is part of the image zoomed in:

zoomed-in

As you can see, everything is aligned to the pixel grid.

You will never get crisp results on any compositors if you force the compositor to downscale.

For example, the left hand side of this image is produced by over-scaling to 200% and the right hand side at desktop scale of 150%:

Did you create this screenshot with your branch or with the code in this PR? At 150% scale, GNOME is able to render fractional scaling 100% crisply on my system. Compare the screenshots I've taken (on GNOME) with this patch

gnome-150-frac

and without this patch

gnome-150-int

I believe the results with this patch are much better. So this patch would be advantageous even for GNOME users.

There are several further issues with the patch; to name a few:

I'll look into these but when I tested SwingSet2 it worked fine.

mahkoh added 2 commits May 23, 2024 13:42
This is required for fractional scaling since the generic code rounds
too much to perform pixel-perfect copies at all scales.

Signed-off-by: Julian Orth <[email protected]>
@mahkoh
Copy link
Author

mahkoh commented May 23, 2024

When I run SwingSet2 (./build/linux-x86_64-server-release/images/jdk/demo/jfc/SwingSet2/SwingSet2.jar), it crashes with

I've been able to reproduce this and it should be fixed now. This was caused by one of my last changes that I only tested with IntelliJ.

Stylepad (build/linux-x86_64-server-release/images/jdk/demo/jfc/Stylepad/Stylepad.jar) UI bounces during an interactive resize as if it changes its size for a fraction of a second and then it goes back again.

I've not been able to reproduce this. Stylepad behaves the same with or without this PR. Maybe this was fixed by my fix for the previous issue.

There are quite a few changes in the shared code; so far we have been able to limit this to a tiny change in the repaint manager; changing shared code for the sake of one Toolkit is a big red flag and needs to be studied very carefully.

There are two changes outside of pure-wayland code:

  • Changes to the repaint manager and its call sites.
  • Changes to SunGraphics2D

The changes to the repaint manager fix an issue with your previous changes where you would call wl_surface_commit from within the RepaintManager.paint function. This was incorrect. The RepaintManager has not finished drawing until after RepaintManager.endPaint has been called. This was the cause of significant flickering during resize (with or without fractional scaling). I don't see a way to do this without changing the call sites of the repaint manager as I've done in this PR.

The changes to SunGraphics2D are necessary to get accurate buffer copies when the scale is fractional. These need to be reviewed carefully.

There are changes in the handling of the memory buffers, which are probably unrelated to fractional scaling. The current version (sans the patch) crashes under KDE; I will look into that separately.

I believe this crash was caused by the following:

  1. the code acquires the draw lock
  2. the code checks if the show buffer must be resized
  3. the code resizes the show buffer
  4. the code releases the draw lock
  5. the code acquires the draw lock
  6. the code copies the draw buffer to the show buffer

If the draw buffer was resized between 4 and 5, this could segfault. My change makes it so that the draw lock is held for the entire time. (There is slightly more to it but this is the gist of it.)

@mahkoh mahkoh force-pushed the fractional-scale branch from 889e26c to 705a2e1 Compare May 23, 2024 12:06
@mkartashev
Copy link
Collaborator

If the draw buffer was resized between 4 and 5, this could segfault. My change makes it so that the draw lock is held for the entire time. (There is slightly more to it but this is the gist of it.)

Yes, makes sense. This needs to be addressed separately and urgently. I will see to that - JBR-7198

@mahkoh
Copy link
Author

mahkoh commented May 23, 2024

All of the commits except the last one are not essential to this PR. Each of them fixes a more or less serious bug but none are required for fractional scaling to work. If it would help moving this forward, I could extract them to separate pull requests. The fractional scaling commit does not touch any non-wayland code.

(Some of them might depend on your viewporter code so opening them in the wakefield repo would have to wait.)

Edit: Actually I guess the java2d: implement drawImage as a copy if src & dst have the same scale commit is rather important because otherwise IntelliJ will flicker with fractional scaling, but it's only a single function that should be easy to review.

@mkartashev
Copy link
Collaborator

I could extract them to separate pull requests

Let's start from the beginning: please, file bugs (one per problem), describing issues that you spotted, how to reproduce them and/or what specific problems do you see in the code, etc.

@mahkoh
Copy link
Author

mahkoh commented May 24, 2024

I was able to reproduce the last issue by using -Dsun.java2d.uiScale=1.5: https://youtrack.jetbrains.com/issue/JBR-7204/wayland-window-contents-jump-around-at-1.5-scale

@mkartashev mkartashev deleted the branch JetBrains:mkartash/JBR-7158 May 24, 2024 11:49
@mkartashev mkartashev closed this May 24, 2024
@mahkoh
Copy link
Author

mahkoh commented May 24, 2024

Annoyingly github doesn't seem to support re-targeting a PR when the base-branch is merged. I've opened #388 as a replacement.

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.

2 participants