Bug 564097 - [GTK][HiDPI] Cairo auto scaling causing scaling problems
Summary: [GTK][HiDPI] Cairo auto scaling causing scaling problems
Status: VERIFIED FIXED
Alias: None
Product: Platform
Classification: Eclipse Project
Component: SWT (show other bugs)
Version: 4.15   Edit
Hardware: PC Linux
: P3 normal (vote)
Target Milestone: 4.17   Edit
Assignee: Soraphol (Paul) Damrongpiriyapong CLA
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2020-06-08 15:18 EDT by Soraphol (Paul) Damrongpiriyapong CLA
Modified: 2021-07-27 09:21 EDT (History)
6 users (show)

See Also:


Attachments
Screenshots of the various cases of cairoAutoScale (595.01 KB, application/zip)
2020-06-08 15:18 EDT, Soraphol (Paul) Damrongpiriyapong CLA
no flags Details
Screen shot on X11 after dpi changed from 200% to 100% (60.84 KB, image/png)
2020-06-19 04:35 EDT, Sravan Kumar Lakkimsetti CLA
no flags Details
Ubuntu Gnome X11 200% (154.36 KB, image/png)
2020-07-01 02:35 EDT, Sravan Kumar Lakkimsetti CLA
no flags Details
Ubuntu Gnome X11 100% (76.94 KB, image/png)
2020-07-01 02:36 EDT, Sravan Kumar Lakkimsetti CLA
no flags Details
Snippet 373 Ubuntu 100% to 200% switch (75.84 KB, image/png)
2020-07-02 23:27 EDT, Sravan Kumar Lakkimsetti CLA
no flags Details
Snippet 373 Ubuntu 200% to 100% switch (35.42 KB, image/png)
2020-07-02 23:33 EDT, Sravan Kumar Lakkimsetti CLA
no flags Details
Snippet 367 at 200% (104.19 KB, image/png)
2020-07-07 11:32 EDT, Sravan Kumar Lakkimsetti CLA
no flags Details
eclipse from 200% to 100% (105.79 KB, image/png)
2020-07-07 11:34 EDT, Sravan Kumar Lakkimsetti CLA
no flags Details
Wayland pre-patch (3.08 MB, video/webm)
2020-07-31 15:05 EDT, Soraphol (Paul) Damrongpiriyapong CLA
no flags Details
Wayland post-patch (5.34 MB, video/webm)
2020-07-31 15:05 EDT, Soraphol (Paul) Damrongpiriyapong CLA
no flags Details
Wayland post-patch (200 -> 100) (5.41 MB, video/webm)
2020-08-04 10:31 EDT, Soraphol (Paul) Damrongpiriyapong CLA
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Soraphol (Paul) Damrongpiriyapong CLA 2020-06-08 15:18:57 EDT
Created attachment 283198 [details]
Screenshots of the various cases of cairoAutoScale

Hello, I have been looking through the plenty of scaling bugs that have been popping up recently. I have traced it back to a change in Bug 507020. I am not 100% sure if this is the root cause but it does affect a lot of the behavior from my experience. 

I have been testing on GNOME Wayland, and by setting the scale factor of my second monitor (non-HiDPI) to 200%. 

I want to explain what I have experienced through testing. Currently, at 200% scale factor Eclipse renders as seen in the screenshot - "200%trueEclipse(current)". What seems to be happening is that the bounds of the components have been scaled property (you can click at on the buttons), but some parts of the UI is just being scaled and rendered on the top left. This is with the cairoAutoScale set to true. 

With cairoAutoScale set to false the rendering seems to be perfect (see 200%falseEclipse). Only problem here is that the icons are small in the toolbar, but I believe this is a separate problem. 

Now, another bug (563410) that has been found is from a user using the -Dswt.autoScale=150 flag to increase the size of the icons (assuming for the toolbar). I have also attached a screenshot of that as well (see 200%falseEclipse150DswtFlag). Here the icons that used to be clear (eg. Package explorer icon are now scaled again and blurry). However the fact that Eclipse UI is still intact really convinced me that having cairoAutoScale set to true causes the black bars/scaling issues we have been seeing. 

Lastly, I want to add that I have also included the screenshots of the Snippet367, which Bug 507020 was trying to fix. I can't seem to see a difference between having the autoCairoScale equal to true or false. 

Again, I don't have a HiDPI monitor to really put this to the test. I would also love if I could get some pointers of where to go from here. It seems like multiple systems have the responsibility to scale and its not consistent.

Thanks.
Comment 1 Andrey Loskutov CLA 2020-06-08 15:26:01 EDT
Paul, can you please add links to the related bugs you are investigating to this one?
Comment 2 Soraphol (Paul) Damrongpiriyapong CLA 2020-06-08 15:51:01 EDT
https://bugs.eclipse.org/bugs/show_bug.cgi?id=541370
https://bugs.eclipse.org/bugs/show_bug.cgi?id=563410
https://bugs.eclipse.org/bugs/show_bug.cgi?id=563923

Seems to also be related to:
https://bugs.eclipse.org/bugs/show_bug.cgi?id=563699
Removing the flag will take away the assertion but instead a 
(Eclipse:32156): Gtk-WARNING **: 15:49:01.911: Negative content height -1, 
which may be because of of my small monitor. Not sure.
Comment 3 Sravan Kumar Lakkimsetti CLA 2020-06-08 22:54:54 EDT
There are two implementations of HIDPI
1. With cairo scaling
2. Without cairo scaling

1. With Cairo scaling
Comment 4 Sravan Kumar Lakkimsetti CLA 2020-06-09 00:13:22 EDT
Ignore my previous comment

There are two implementations of HIDPI
1. With cairo scaling
2. Without cairo scaling

1. With Cairo scaling (used by Gnome)
   In cairo surface we have a property called device_scale, we use this property to manipulate scaling. For example if you have an image with 100% scale, then we set this device_scale to 1. This way when this image needs to be rendered on different monitor the image will be scaled up or down accordingly by cairo. 
-Dswt.autoScale is irrelevant in this case. This is not considered at all.

2. Without Cairo scaling (used by KDE)
   We need to handle scaling ourselves. for this you can see a refreshImage call in GC.drawImage() this where the image gets scaled. Here we do scale all the widgets as well. This implementation is really problematic as odd scaling factors like 150% etc, you'll encounter lots of crashes caused by conversion of float to integer. 
-Dswt.autoScale works in this case

Background:
For eclipse to work on hidpi environment there are two things we need to take care of. 
1. Scaling on font.
2. Scaling of images

Scaling of font is handled by OS itself. As soon as we change the display scaling factor the font gets scaled automatically. This does not depend on cairo scaling

Scaling on Images is the difficult part. if you have cairo scaling available delegate the scaling task to cairo. this is what we used in case of gnome.

if cairo scaling is not available, use our own scaling algorithms. 

We had of problem of detecting whether cairo scaling is available or not. Bug 507020 addressed that issue.

There is another problem we have. Many times we create a blank image and then draw another image on top of that. When we are creating an image we create with 100% scale factor on all displays. So this causes blurred images in higher scaling factors see Bug 535065 and Bug 534422 The solution is to create blank image with current display scaling factor.
Comment 5 Soraphol (Paul) Damrongpiriyapong CLA 2020-06-09 09:28:35 EDT
Okay, I see. How come we have to use two different types of scaling? Why not just implement without Cairo since we already have the algorithms for it. I'm asking because from the screenshots, without Cairo Eclipse is perfect at 200%, but when its enable it creates very weird scaling (seems like the bounds are being scaled, but the actual UI is not)
Comment 6 Sravan Kumar Lakkimsetti CLA 2020-06-09 10:33:33 EDT
(In reply to Soraphol (Paul) Damrongpiriyapong from comment #5)
> Okay, I see. How come we have to use two different types of scaling? Why not
> just implement without Cairo since we already have the algorithms for it.
> I'm asking because from the screenshots, without Cairo Eclipse is perfect at
> 200%, but when its enable it creates very weird scaling (seems like the
> bounds are being scaled, but the actual UI is not)

From the screenshots I believe you are using KDE. In this case the cairo scaling should be off. If that is not off then its a bug. This was the first implementation I did. We didn't had cairo scaling available at the time of implementation

Try using Gnome. You'll see a big difference. This uses cairo scaling. This implementation has potential to support fractional scale factors like 125%, 150% etc

Here are the reasons for cairo scaling

1. Performance: using Cairo scaling is way faster.
2. cairo scaling has potential to support fractional scaling factors.
3. cairo scaling has potential to support dynamic scaling. like when I switch display scaling my application should redraw with higher/lower scale factor 

So using cairo scaling is the way forward. 

The problem I faced was I am not able to get to use cairo scaling in KDE environment. I always got device scale parameter 1 (100%) though the display was at 200%. In gnome I do get the device scale as 2 in case of 200% display.

So decision was taken to have two implementations. Here is the location where we determine whether to use cairo scaling or our own implementation https://git.eclipse.org/c/platform/eclipse.platform.swt.git/tree/bundles/org.eclipse.swt/Eclipse%20SWT/gtk/org/eclipse/swt/graphics/Device.java#n659. 

Here are my suggestions.
1. Try to debug the area I pointed and see cairo scaling is getting enabled. It should get enabled on gnome and not enabled on KDE.
2. Please enable cairo scaling on KDE also if possible.


Try using Snippet 367.
at 100% it should look like https://bugs.eclipse.org/bugs/attachment.cgi?id=279235
at 200% it should look like https://bugs.eclipse.org/bugs/attachment.cgi?id=279236

Target one case at a time in that snippet. That way you will be able to identify the problem. 

If you need any help on this please don't hesitate to contact me.
Comment 7 Soraphol (Paul) Damrongpiriyapong CLA 2020-06-09 11:05:49 EDT
I am actually using GNOME Wayland, that's why I think something is wrong. Those screenshots are from me commenting out the line you suggested in Device where we check if it is GNOME or not. 

After some more investigating, it looks like those screenshot appear that way because for Cairo, doing cairo_surface_get_device_scale will return the scale for the main display. Since I set the 200% on my second monitor, Cairo is incorrectly scaling with device scale 1. I believe that the majority of the rendering problems from the bugs may be because of this.
Comment 8 Soraphol (Paul) Damrongpiriyapong CLA 2020-06-09 11:07:32 EDT
I will try to see if I can get a patch for this, as well as it working on KDE as well.
Comment 9 Eclipse Genie CLA 2020-06-16 13:00:10 EDT
New Gerrit change created: https://git.eclipse.org/r/165008
Comment 10 Sravan Kumar Lakkimsetti CLA 2020-06-19 04:35:26 EDT
Created attachment 283352 [details]
Screen shot on X11 after dpi changed from 200% to 100%

Tested with X11 and wayland using Gnome

When I change dpi I get a popup on wayland asking for a restart. But on X11 I don't get that popup.

If you look at attached screen shot, you can see that most of the layouts have been scaled properly. its the images that did not scale. 

There are two types of images. Owner drawn, composite and static images. Looking at the screen shots I see that the static images are scaled. but owner drawn and composite images are not scaled properly. 

Please take a look at SWT.Paint event. Mostly this needs to be generated.
Comment 11 Soraphol (Paul) Damrongpiriyapong CLA 2020-06-19 17:03:14 EDT
Yes I notice this too, with Wayland, if you restart everything works. But you are correct that if you don't restart the icons are not scaled properly. 

Thanks for helping me here. I didn't know there were two types of images, and was confused by certain ones were scaled.

I will take a look into this on Monday.
Comment 13 Eclipse Genie CLA 2020-06-24 15:40:24 EDT
New Gerrit change created: https://git.eclipse.org/r/165440
Comment 14 Sravan Kumar Lakkimsetti CLA 2020-07-01 02:35:29 EDT
Created attachment 283459 [details]
Ubuntu Gnome X11 200%

There are two problems I noticed 
1. at 200% in Ubuntu gnome x11, In the editor the ruler area where error markers and line numbers are displayed, there are more borders displayed. you can see that in attached image
2. after switching to 100% I see some black bands and some scaled up images in the maximize/minimize/close button area of CTadFolder.

Apart from this I also see the following stack trace in the console
!ENTRY org.eclipse.ui 4 0 2020-07-01 11:58:09.126
!MESSAGE Unhandled event loop exception
!STACK 0
java.lang.IllegalArgumentException: Argument not valid
	at org.eclipse.swt.SWT.error(SWT.java:4704)
	at org.eclipse.swt.SWT.error(SWT.java:4638)
	at org.eclipse.swt.SWT.error(SWT.java:4609)
	at org.eclipse.swt.graphics.GC.drawImage(GC.java:860)
	at org.eclipse.swt.graphics.GC.drawImage(GC.java:844)
	at org.eclipse.swt.custom.CTabFolderRenderer.drawUnselected(CTabFolderRenderer.java:1655)
	at org.eclipse.swt.custom.CTabFolderRenderer.draw(CTabFolderRenderer.java:639)
	at org.eclipse.e4.ui.workbench.renderers.swt.CTabRendering.draw(CTabRendering.java:280)
	at org.eclipse.swt.custom.CTabFolder.onPaint(CTabFolder.java:2078)
	at org.eclipse.swt.custom.CTabFolder.lambda$0(CTabFolder.java:336)
	at org.eclipse.swt.widgets.EventTable.sendEvent(EventTable.java:89)
	at org.eclipse.swt.widgets.Display.sendEvent(Display.java:5684)
	at org.eclipse.swt.widgets.Widget.sendEvent(Widget.java:1424)
	at org.eclipse.swt.widgets.Widget.sendEvent(Widget.java:1450)
	at org.eclipse.swt.widgets.Widget.sendEvent(Widget.java:1433)
	at org.eclipse.swt.widgets.Control.gtk_draw(Control.java:3861)
	at org.eclipse.swt.widgets.Scrollable.gtk_draw(Scrollable.java:346)
	at org.eclipse.swt.widgets.Composite.gtk_draw(Composite.java:451)
	at org.eclipse.swt.widgets.Widget.windowProc(Widget.java:2232)
	at org.eclipse.swt.widgets.Control.windowProc(Control.java:6659)
	at org.eclipse.swt.widgets.Display.windowProc(Display.java:5918)
	at org.eclipse.swt.internal.gtk.GTK.gtk_main_do_event(Native Method)
	at org.eclipse.swt.widgets.Display.eventProc(Display.java:1486)
	at org.eclipse.swt.internal.gtk.OS.g_main_context_iteration(Native Method)
	at org.eclipse.swt.widgets.Display.readAndDispatch(Display.java:4444)
	at org.eclipse.e4.ui.internal.workbench.swt.PartRenderingEngine$5.run(PartRenderingEngine.java:1157)
	at org.eclipse.core.databinding.observable.Realm.runWithDefault(Realm.java:338)
	at org.eclipse.e4.ui.internal.workbench.swt.PartRenderingEngine.run(PartRenderingEngine.java:1046)
	at org.eclipse.e4.ui.internal.workbench.E4Workbench.createAndRunUI(E4Workbench.java:155)
	at org.eclipse.ui.internal.Workbench.lambda$3(Workbench.java:658)
	at org.eclipse.core.databinding.observable.Realm.runWithDefault(Realm.java:338)
	at org.eclipse.ui.internal.Workbench.createAndRunWorkbench(Workbench.java:557)
	at org.eclipse.ui.PlatformUI.createAndRunWorkbench(PlatformUI.java:154)
	at org.eclipse.ui.internal.ide.application.IDEApplication.start(IDEApplication.java:150)
	at org.eclipse.equinox.internal.app.EclipseAppHandle.run(EclipseAppHandle.java:203)
	at org.eclipse.core.runtime.internal.adaptor.EclipseAppLauncher.runApplication(EclipseAppLauncher.java:137)
	at org.eclipse.core.runtime.internal.adaptor.EclipseAppLauncher.start(EclipseAppLauncher.java:107)
	at org.eclipse.core.runtime.adaptor.EclipseStarter.run(EclipseStarter.java:401)
	at org.eclipse.core.runtime.adaptor.EclipseStarter.run(EclipseStarter.java:255)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.base/java.lang.reflect.Method.invoke(Method.java:566)
	at org.eclipse.equinox.launcher.Main.invokeFramework(Main.java:657)
	at org.eclipse.equinox.launcher.Main.basicRun(Main.java:594)
	at org.eclipse.equinox.launcher.Main.run(Main.java:1447)
	at org.eclipse.equinox.launcher.Main.main(Main.java:1420)
Comment 15 Sravan Kumar Lakkimsetti CLA 2020-07-01 02:36:15 EDT
Created attachment 283460 [details]
Ubuntu Gnome X11 100%
Comment 16 Sravan Kumar Lakkimsetti CLA 2020-07-02 23:27:23 EDT
Created attachment 283488 [details]
Snippet 373 Ubuntu 100% to 200% switch
Comment 17 Sravan Kumar Lakkimsetti CLA 2020-07-02 23:33:10 EDT
Created attachment 283489 [details]
Snippet 373 Ubuntu 200% to 100% switch

Tested Snippet 373 please see the attached screenshots.

Switching from 100% to 200% does generate a popup. See https://bugs.eclipse.org/bugs/attachment.cgi?id=283488&action=edit

Switching from 200% to 100% is a problem for the snippet. Popup is not displayed properly. Need to kill the java process. See the attached image with this comment

on the eclipse workbench in addition to comment 14, I don't see any popup for a restart
Comment 18 Soraphol (Paul) Damrongpiriyapong CLA 2020-07-03 10:39:28 EDT
Thank you for testing. This is weird behavior that you can see the popup on x11 in the snippet but not when using eclipse since they are both using the exact same mechanism to trigger it.


I will work on making dynamic scaling work. I believe there may be a way without making a new public method.
Comment 19 Sravan Kumar Lakkimsetti CLA 2020-07-07 11:32:19 EDT
Created attachment 283530 [details]
Snippet 367 at 200%
Comment 20 Sravan Kumar Lakkimsetti CLA 2020-07-07 11:34:27 EDT
Created attachment 283531 [details]
eclipse from 200% to 100%
Comment 21 Soraphol (Paul) Damrongpiriyapong CLA 2020-07-07 16:57:54 EDT
@Sravan Thank you for testing it out. I will look into what is causing these problems. I do know the source of that IllegalArgumentException but I am not sure of the purpose. Wondering if you could help me out with this? 


In GC.drawImage line 846:

if (simple) {
		srcWidth = destWidth = imgWidth;
		srcHeight = destHeight = imgHeight;
	} else {
		simple = srcX == 0 && srcY == 0 &&
			srcWidth == destWidth && destWidth == imgWidth &&
			srcHeight == destHeight && destHeight == imgHeight;
		if (srcX + srcWidth > imgWidth + 1 || srcY + srcHeight > imgHeight + 1) { //rounding error correction for hidpi
			SWT.error(SWT.ERROR_INVALID_ARGUMENT);
		}
	}
Comment 22 Sravan Kumar Lakkimsetti CLA 2020-07-08 10:32:47 EDT
(In reply to Soraphol (Paul) Damrongpiriyapong from comment #21)
> @Sravan Thank you for testing it out. I will look into what is causing these
> problems. I do know the source of that IllegalArgumentException but I am not
> sure of the purpose. Wondering if you could help me out with this? 
> 
> 
> In GC.drawImage line 846:
> 
> if (simple) {
> 		srcWidth = destWidth = imgWidth;
> 		srcHeight = destHeight = imgHeight;
> 	} else {
> 		simple = srcX == 0 && srcY == 0 &&
> 			srcWidth == destWidth && destWidth == imgWidth &&
> 			srcHeight == destHeight && destHeight == imgHeight;
> 		if (srcX + srcWidth > imgWidth + 1 || srcY + srcHeight > imgHeight + 1) {
> //rounding error correction for hidpi
> 			SWT.error(SWT.ERROR_INVALID_ARGUMENT);
> 		}
> 	}

This is an error check. drawImage() can draw partial images also. 
The coordinate system we use top left in the image is 0,0.
Lets look at these 4 parameters from source image

srcX - X co0rdinate of the starting point
srcY - Y coordinate of the starting point
srcWidth - how much width we need to copy
srcHeight - how high we need to copy

Now when you try to copy a whole image your source will be 0,0 and width and height will be image width and image height. 

if you want to copy only lower right quarter of the image then starting point will be (imgWidth/2, imgHeight/2) and you can copy imgWidth/2 of the image.

The above check is to verify srcX+srcWidth doesn't exceed image width.

Since we use integers here we end up having rounding errors when we calculate the actual coordinates on the screen during scaleup operation we multiply by 2 and during scale down we divide by 2. This causes rounding error when we have a image with a width as odd number like 17. For this we added + 1 to avoid these rounding errors.

Thinking more on this drawImage method actually scale the image. please see whether you can remove scaleup/scaledown operations on the source image parameters.
Comment 23 Soraphol (Paul) Damrongpiriyapong CLA 2020-07-08 15:24:39 EDT
Alright got it. I see what you are talking about, I removed the additional scaling up of the srcImage since we already do the refreshForZoom inside drawImage. From my testing I no longer get the exception. 

About the black bars, this one is weird. I'm guessing it has to do with the fact that the pixbuf for those parts are set to 0 by default on X11. However I was wondering if you have experienced the black bars without the changes I have made.
Comment 24 Sravan Kumar Lakkimsetti CLA 2020-07-08 15:32:16 EDT
(In reply to Soraphol (Paul) Damrongpiriyapong from comment #23)
> Alright got it. I see what you are talking about, I removed the additional
> scaling up of the srcImage since we already do the refreshForZoom inside
> drawImage. From my testing I no longer get the exception. 
> 
> About the black bars, this one is weird. I'm guessing it has to do with the
> fact that the pixbuf for those parts are set to 0 by default on X11. However
> I was wondering if you have experienced the black bars without the changes I
> have made.

I have seen this black bars earlier also. This happens on x11. There is a bug for that. See bug 535123
Comment 25 Soraphol (Paul) Damrongpiriyapong CLA 2020-07-31 15:05:21 EDT
Created attachment 283771 [details]
Wayland pre-patch
Comment 26 Soraphol (Paul) Damrongpiriyapong CLA 2020-07-31 15:05:49 EDT
Created attachment 283772 [details]
Wayland post-patch
Comment 27 Soraphol (Paul) Damrongpiriyapong CLA 2020-08-04 10:31:35 EDT
Created attachment 283795 [details]
Wayland post-patch (200 -> 100)
Comment 29 Soraphol (Paul) Damrongpiriyapong CLA 2020-08-12 11:58:41 EDT
Verified Eclipse SDK
Version: 2020-09 (4.17)
Build id: I20200811-1800
OS: Linux, v.5.7.9-200.fc32.x86_64, x86_64 / gtk 3.24.20
Java version: 11.0.7
Comment 30 Simeon Andreev CLA 2020-12-15 07:57:12 EST
Commit https://git.eclipse.org/c/platform/eclipse.platform.swt.git/commit/?id=a05ee0e05540ae5f068f127d687828dc3cdf0341 caused bug 569691, namely if a view action has a large icon the view toolbar height is now the height of that icon. Previously, this was the case only if the *first* action had a large icon.

Could you please comment on bug 569691 if this is intended?
Comment 31 Andrey Loskutov CLA 2021-07-21 04:13:07 EDT
(In reply to Eclipse Genie from comment #28)
> Gerrit change
> https://git.eclipse.org/r/c/platform/eclipse.platform.swt/+/165440 was
> merged to [master].
> Commit:
> http://git.eclipse.org/c/platform/eclipse.platform.swt.git/commit/
> ?id=a05ee0e05540ae5f068f127d687828dc3cdf0341

This introduced another regression, see bug 574938. Icons of a disabled button are not disabled anymore.