Double-buffering with recent HighDPI changes

[ptziegler]

This is a little bit of a concerning topic that popped up in the WindowBuilder project, after trying out the recent M2/M3. In short: WindowBuilder is making heavy use of double-buffering where the UI is first painted onto an Image and the result then to the underlying Canvas. For example in the CImageLabel.

The problem is that somewhere along those lines, the monitor-specific information is lost and everything is painted as if at 100% zoom. This is the example in the editor when at 125% zoom, with double-buffering:

And the same example, but without double-buffering.

As you can see, the label/icon is not scaled correctly and the whole area contains visual artifacts. The effect is even worse when you look at the entire editor, which is completely unusable:

Using double-buffering is not an uncommon technique, so I assume that is something other clients will experience as well. I don't get the impression that this was really taken into consideration and I'm not really sure how to solve this, outside of removing every and all instances where double-buffering is used...

Sadly, I'm not familiar with all the changes that accompanied this feature, so I suppose the point of this discussion is to primarily raise awareness for some of the weird little quirks that have been introduced.

[merks]

A test case snippet would of course help. This sounds very serious.

[ptziegler]

You ask and you shall receive:

package test;

import org.eclipse.swt.SWT;
import org.eclipse.swt.custom.CLabel;
import org.eclipse.swt.graphics.GC;
import org.eclipse.swt.graphics.Image;
import org.eclipse.swt.graphics.Point;
import org.eclipse.swt.graphics.Rectangle;
import org.eclipse.swt.layout.GridData;
import org.eclipse.swt.layout.GridLayout;
import org.eclipse.swt.widgets.Canvas;
import org.eclipse.swt.widgets.Composite;
import org.eclipse.swt.widgets.Display;
import org.eclipse.swt.widgets.Event;
import org.eclipse.swt.widgets.Listener;
import org.eclipse.swt.widgets.Shell;

public class Snippet {
	public static void main(String[] args) {
		System.setProperty("swt.autoScale.updateOnRuntime", "true");
		
		Shell shell = new Shell();
		shell.setSize(200, 200);
		shell.setLayout(new GridLayout(2, false));
		
		Display display = shell.getDisplay();
		shell.setBackground(display.getSystemColor(SWT.COLOR_BLUE));

		CImageLabel cImageLabel = new CImageLabel(shell, SWT.NONE);
		cImageLabel.setText("Test1");
		cImageLabel.setLayoutData(new GridData(SWT.FILL, SWT.FILL, true, false));

		CLabel label = new CLabel(shell, SWT.NONE);
		label.setText("Test2");
		label.setLayoutData(new GridData(SWT.FILL, SWT.FILL, true, false));
		
		shell.open();
		while(!display.isDisposed()) {
			display.readAndDispatch();
		}
	}

	public static class CImageLabel extends Canvas {
		private static final int SPACE = 5;
		private Image m_image;
		private String m_text;

		////////////////////////////////////////////////////////////////////////////
		//
		// Constructor
		//
		////////////////////////////////////////////////////////////////////////////
		public CImageLabel(Composite parent, int style) {
			super(parent, style | SWT.NO_BACKGROUND);
			addListener(SWT.Dispose, new Listener() {
				@Override
				public void handleEvent(Event event) {
					if (m_backImage != null) {
						m_backImage.dispose();
						m_backImage = null;
					}
				}
			});
			addListener(SWT.Paint, new Listener() {
				@Override
				public void handleEvent(Event event) {
					doPaint(event.gc);
				}
			});
		}

		////////////////////////////////////////////////////////////////////////////
		//
		// Access
		//
		////////////////////////////////////////////////////////////////////////////
		public Image getImage() {
			return m_image;
		}

		public void setImage(Image image) {
			m_image = image;
			redraw();
		}

		public String getText() {
			return m_text;
		}

		public void setText(String text) {
			m_text = text;
			redraw();
		}

		////////////////////////////////////////////////////////////////////////////
		//
		// Paint
		//
		////////////////////////////////////////////////////////////////////////////
		private Image m_backImage;

		private void doPaint(GC paintGC) {
			Rectangle clientArea = getClientArea();
			// prepare back image
			GC gc;
			{
				if (m_backImage == null || !m_backImage.getBounds().equals(clientArea)) {
					if (m_backImage != null) {
						m_backImage.dispose();
					}
					m_backImage = new Image(getDisplay(), clientArea.width, clientArea.height);
				}
				//
				gc = new GC(m_backImage);
				gc.setBackground(Display.getCurrent().getSystemColor(SWT.COLOR_RED));
				gc.setForeground(paintGC.getForeground());
				gc.fillRectangle(clientArea);
			}
			//
			Point textExtent = m_text == null ? new Point(0, 0) : gc.textExtent(m_text);
			Rectangle imageBounds = m_image == null ? new Rectangle(0, 0, 0, 0) : m_image.getBounds();
			//
			if (m_image != null) {
				int x = clientArea.x;
				int y = clientArea.y + (clientArea.height - imageBounds.height) / 2;
				gc.drawImage(m_image, x, y);
			}
			if (m_text != null) {
				int x = clientArea.x + imageBounds.width + SPACE;
				int y = clientArea.y + (clientArea.height - textExtent.y) / 2;
				gc.drawText(m_text, x, y);
			}
			// flush back image
			{
				paintGC.drawImage(m_backImage, 0, 0);
				gc.dispose();
			}
		}

		@Override
		public Point computeSize(int wHint, int hHint, boolean changed) {
			// prepare text size
			GC gc = new GC(this);
			Point textExtent = m_text == null ? new Point(0, 0) : gc.textExtent(m_text);
			gc.dispose();
			// prepare image size
			Rectangle imageBounds = m_image == null ? new Rectangle(0, 0, 0, 0) : m_image.getBounds();
			// calculate control size
			int width = imageBounds.width + SPACE + textExtent.x;
			int height = Math.max(imageBounds.height, textExtent.y);
			return new Point(width, height);
		}
	}
}

This is how it looks on my laptop. Red is the image, white the composite. Both are supposed to have the same size.

[ptziegler]

This is how it looks on my laptop. Red is the image, white the composite. Both are supposed to have the same size.

Looking at the screenshot, I noticed that the background of the CLabel is gray, while the background of the CImageLabel is white. Which means that the image is correctly sized. But the GC created for the image is painting at 100% zoom, not 125%!

[HeikoKlare]

So this seems to be the exact same scenario we had for the editor rulers: the back image (in red) is created at 100%. It should probably be replaced by one created with an ImageGcDrawer, like we did for those rulers. I am not sure the adaptation is 100% correct (I just did a quick test due to lack of more time), but the following at least works for me (including rescaling when changing monitor zoom). Maybe it requires more changes to be semantically fully correct. Note that the only thing I change compared to your snippet is how the back image is drawn, everything else stays the same:

package test;

import org.eclipse.swt.*;
import org.eclipse.swt.custom.*;
import org.eclipse.swt.graphics.*;
import org.eclipse.swt.layout.*;
import org.eclipse.swt.widgets.*;

public class Snippet {
	public static void main(String[] args) {
		System.setProperty("swt.autoScale.updateOnRuntime", "true");

		Shell shell = new Shell();
		shell.setSize(200, 200);
		shell.setLayout(new GridLayout(2, false));

		Display display = shell.getDisplay();
		shell.setBackground(display.getSystemColor(SWT.COLOR_BLUE));

		CImageLabel cImageLabel = new CImageLabel(shell, SWT.NONE);
		cImageLabel.setText("Test1");
		cImageLabel.setLayoutData(new GridData(SWT.FILL, SWT.FILL, true, false));

		CLabel label = new CLabel(shell, SWT.NONE);
		label.setText("Test2");
		label.setLayoutData(new GridData(SWT.FILL, SWT.FILL, true, false));

		shell.open();
		while(!display.isDisposed()) {
			display.readAndDispatch();
		}
	}

	public static class CImageLabel extends Canvas {
		private static final int SPACE = 5;
		private Image m_image;
		private String m_text;

		////////////////////////////////////////////////////////////////////////////
		//
		// Constructor
		//
		////////////////////////////////////////////////////////////////////////////
		public CImageLabel(Composite parent, int style) {
			super(parent, style | SWT.NO_BACKGROUND);
			addListener(SWT.Dispose, new Listener() {
				@Override
				public void handleEvent(Event event) {
					if (m_backImage != null) {
						m_backImage.dispose();
						m_backImage = null;
					}
				}
			});
			addListener(SWT.Paint, new Listener() {
				@Override
				public void handleEvent(Event event) {
					doPaint(event.gc);
				}
			});
		}

		////////////////////////////////////////////////////////////////////////////
		//
		// Access
		//
		////////////////////////////////////////////////////////////////////////////
		public Image getImage() {
			return m_image;
		}

		public void setImage(Image image) {
			m_image = image;
			redraw();
		}

		public String getText() {
			return m_text;
		}

		public void setText(String text) {
			m_text = text;
			redraw();
		}

		////////////////////////////////////////////////////////////////////////////
		//
		// Paint
		//
		////////////////////////////////////////////////////////////////////////////
		private Image m_backImage;

		private void doPaint(GC paintGC) {
			Rectangle clientArea = getClientArea();
			// prepare back image
			GC gc;
			{
				if (m_backImage == null || !m_backImage.getBounds().equals(clientArea)) {
					if (m_backImage != null) {
						m_backImage.dispose();
					}
					m_backImage = new Image(getDisplay(), (imageGc, width, height) -> {
						imageGc.setBackground(Display.getCurrent().getSystemColor(SWT.COLOR_RED));
						imageGc.setForeground(paintGC.getForeground());
						imageGc.fillRectangle(clientArea);
						//
						Point textExtent = m_text == null ? new Point(0, 0) : imageGc.textExtent(m_text);
						Rectangle imageBounds = m_image == null ? new Rectangle(0, 0, 0, 0) : m_image.getBounds();
						//
						if (m_image != null) {
							int x = clientArea.x;
							int y = clientArea.y + (clientArea.height - imageBounds.height) / 2;
							imageGc.drawImage(m_image, x, y);
						}
						if (m_text != null) {
							int x = clientArea.x + imageBounds.width + SPACE;
							int y = clientArea.y + (clientArea.height - textExtent.y) / 2;
							imageGc.drawText(m_text, x, y);
						}
					}, clientArea.width, clientArea.height);
				}
			}
			// flush back image
			paintGC.drawImage(m_backImage, 0, 0);
		}

		@Override
		public Point computeSize(int wHint, int hHint, boolean changed) {
			// prepare text size
			GC gc = new GC(this);
			Point textExtent = m_text == null ? new Point(0, 0) : gc.textExtent(m_text);
			gc.dispose();
			// prepare image size
			Rectangle imageBounds = m_image == null ? new Rectangle(0, 0, 0, 0) : m_image.getBounds();
			// calculate control size
			int width = imageBounds.width + SPACE + textExtent.x;
			int height = Math.max(imageBounds.height, textExtent.y);
			return new Point(width, height);
		}
	}
}

[HeikoKlare]

Thank you for the report, @ptziegler!

First of all, I see two issues here that need to be distinguished: the wrong image sizes and the drawing artifacts.

Wrong image sizes

Images being painted as if at 100% even though you are on a 125% zoom monitor appears to be an (unknown) bug. Either the WindowBuilder implementation is doing something strange that is incompatible with monitor-specific scaling or it may be a bug in SWT (most probably in the Image implementation). Since the Image implementation on Windows was significantly revised, it might be a regression in those changes, which is why we should definitely have a look before the upcoming release.
Can you test whether the image sizes are also wrong if monitor-specific scaling is disabled? If yes, I would expect the issue to be rooted in the Image class.

Drawing artifacts

This is actually an issue with custom rendering at swt.autoScale=quarter (it always was but probably no one used that setting) where only parts of a control are re-rendered. You will experience the same in GEF, which is why I already informed Alois that this the monitor-specific scaling will have such effects in GEF.
The underlying issue is twofold, but all is rooted in auto-scaling kind of being broken: first, SWT auto-scaling transforms coordinates into virtual points, with points being integer values and with the resolution of those values decreasing the higher the monitor zoom is. This is why even existing HiDPI support leads to scaling issues at 200%. Second, when also using fractional scale values (125% etc.), this becomes worse, as more rouding errors occur on integer value transformations. Since monitor-specific scaling requires "proper" handling of fractional scale values (i.e. swt.autoScale=quarter), such issues become worse with that feature. You should experience the same when using swt.autoScale=quarter with monitor-specific scaling disabled.
At the same time, all this auto-scaling functionaly only really makes sense to auto-scale controls but not to auto-scale stuff that is supposed to be drawn at pixel precision. This, e.g., applies to GEF diagrams (but also to many other custom-rendered things, such as even StyledText). We already have a solution for GEF, which is basically to disable auto scaling for the rendering area and make it behave as if at 100% zoom (and encoding the monitor zoom into the diagram zoom). This can / needs to be extended to a general solution for custom-rendered widgets and may also be applicable here. Until then, those artifacts are somehow "expected". When using such controls, it's probably best to disable monitor-specific scaling for now, until those control are adapted to properly work in HiDPI setups.

@akoch-yatta @fedejeanne fyi

[ptziegler]

Since the Image implementation on Windows was significantly revised, it might be a regression in those changes, which is why we should definitely have a look before the upcoming release.
Can you test whether the image sizes are also wrong if monitor-specific scaling is disabled? If yes, I would expect the issue to be rooted in the Image class.

I've attached a small example to Ed's comment. The problem is specific to the swt.autoScale.updateOnRuntime system property. If it is disabled, everything is properly sized.

Second, when also using fractional scale values (125% etc.), this becomes worse, as more rouding errors occur on integer value transformations. Since monitor-specific scaling requires "proper" handling of fractional scale values (i.e. swt.autoScale=quarter), such issues become worse with that feature. You should experience the same when using swt.autoScale=quarter with monitor-specific scaling disabled.

Just to avoid a misunderstanding. The rounding errors is something that I can see in the editor as well, but is not what is shown in the screenshot. This image tearing is primarily caused by the double-buffering problem because that also is used by the underlying FigureCanvas.

And to avoid another misunderstanding. While this looks like a GEF viewer, it is sadly the result of abusing a (probably) GEF 2.0 fork for well over a decade. So some things that work fine in GEF don't work in this viewer and vice versa. That said, some parts are using GEF and there the result is already better.

[HeikoKlare]

Thank you for the quick update, Patrick! I've quickly tweaked the snippet hopefully in accordance to what we did to the editor rulers, but due to lack of time, it's just a quick shot that seems to work but may still have some semantic flaws.

Sorry that I misunderstood your image regarding the drawing artifacts / rounding errors. Maybe the double-buffering problems in the FigureCanvas can be resolved in the same way as exemplified in the snippet (still you will probably then have the drawing artifacts remaining).

And I actually understood that the WindowBuilder does not use (a current version of) GEF. The problems you mention are of course ubiquitous / independent from GEF. I just mentioned GEF because it's a perfect example for where the scaling produces problems, and it's the one we have already taken a deep look into as we use GEF in our RCP tool.

[ptziegler]

Thank you for the quick update, Patrick! I've quickly tweaked the snippet hopefully in accordance to what we did to the editor rulers, but due to lack of time, it's just a quick shot that seems to work but may still have some semantic flaws.

To be honest, I was just about to write a similar message. I've also adapted the code to use the ImageGcDrawer and then everything is rendered correctly. Which pretty much confirms that the problem is related to using new GC(Image), as opposed to new Image((gc, w, h) -> {...}).

Which is fine, of course, though with the current state, this would mean that clients have to now double- check all instances where this pattern is used.

[HeikoKlare]

Great that you already came to the same conclusion 🙂

Which is fine, of course, though with the current state, this would mean that clients have to now double- check all instances where this pattern is used.

Yes, that's unfortunately necessary. There is no way around that because the required size of the image depends on the monitor to be drawn on and that one you only know when using the image not when creating it. That's why with monitor-specific scaling the usage of Image+GC instead of ImageGcDrawer will usually lead to unexpected results. Using an ImageGcDrawer allows to create the image on demand according to whatever zoom is required. In the snippet, the GC on which the image is drawn contains the zoom to be used for retrieving the image on the proper zoom, to the be rendered at that proper zoom on usage in the GC.
Without monitor-specific scaling, there is only a single zoom value for the whole application and throughout its whole lifecycle, which is why it worked before.

[merks]

You guys are really awesome.

[ptziegler]

That's why with monitor-specific scaling the usage of Image+GC instead of ImageGcDrawer will usually lead to unexpected results. Using an ImageGcDrawer allows to create the image on demand according to whatever zoom is required. In the snippet, the GC on which the image is drawn contains the zoom to be used for retrieving the image on the proper zoom, to the be rendered at that proper zoom on usage in the GC.

Which I just noticed has another unpleasant side-effect: Backwards compatibility.

If I decide to use the ImageGcDrawer, I also limit my application to only work with the latest Eclipse release. Which is fine for the platform itself, but not for downstream projects. I.e. I have three options:

1. I break backwards compatibility and use the ImageGcDrawer for crisp images.
2. I don't break backwards compatibility and accept blurry images.
3. I rewrite the entire painting logic.
   (4) I use some weird bytecode hack to dynamically create an ImageGcDrawer instance...

So effectively 3). In hindsight, I think that seeing the full impact only as part of the M2 was way too late, especially with the delay due to the Jenkins issues. Because now clients (meaning I) only have a few weeks to adapt to those changes. 😨

[HeikoKlare]

We plan to merge this for RC1. Since the actual issue was introduced with M2 and I would assume that many projects do probably not test that frequently and in particular with HiDPI setups, we should restore the existing behavior here. You explanations show that the use cases that may break otherwise are potentially not that seldom.

We had to slightly revise the change #2180 as otherwise it would have been broken in a different way. We've also set up a WindowBuilder environment and tested around with it (also with a state from 3 weeks ago that should still have the problematic image drawing in it). We were actually not able to fully reproduce the state we can see in your screenshots. In particular, we did not face any issues at 150% zoom at all. At 175%, it looked like this for me:

That issue is fixed also with the recent change of the mentioned PR. But it would be nice if could also test again, @ptziegler, or if you could share further information about how to reproduce the scenario you show in the screenshots to test in the same way as you did. Maybe I am missing some configuration.

[HeikoKlare]

I just saw that because I am using SWT master, I had this change in #2163
That already solved most of the issue (regarding blurriness). Just the wrong scaling at 175% and above as shown in the screenshot above remains.

[HeikoKlare]

I hope that we have now uncovered the essential regression and solved it with #2163 for now. It might be worth checking whether eclipse-gef/gef-classic#740 is (positively) affected by that as well.
We have already discussed how to further improve SWT to maybe avoid the adoption of ImageGcDrawer particular in cases where it's not that easy to achieve (as the image/GC is passed around to be drawn on making the encapsulation in an ImageGcDrawer very difficult). We hope to come up with a proposal how to achieve that for 4.37 M1.

As someone who was not involved in this whole feature, the effort it takes to do those adaptations wasn't clear (at least to me) and I think the fact that this will cause UI issues (if enabled) should've been publicised a little bit more. But I think that ship has sailed.

I fully agree with this. As mentioned, we were not aware that such patterns that require adaptations may be used that often by consumers and, at the same time, we did not intend to break any existing behavior (when not using monitor-specific scaling), which is why we did not consider the urgency so high. We were obvious wrong to some degree (in particular since no matter how few consumers there are, they should have been made better aware more early). So that's something we take aware for the future to more thoroughly take into consideration.

Consumers don't always have the option to disable UI scaling directly. Specifically if you're just developing an extension where disabling this feature would also disable it for any other feature, which is just a no-go. In such a case adapations are unavoidable.

Yes, you can only disable the scaling feature completely if you use any extension that would become unusable when having it enabled. Still, consumers should always be able to disable the scaling via the Eclipse preferences dialog. The issue I see and agree with you is that they would need to know what they need to do to fix such a UI issue.

I think in the end, it all simply boils to me being naive about my expectations. I assumed that when auto-scaling is enabled, the unchanged code would look meh at worst, which is sadly not what I got. But this not on you, or any of the other Platform committers, but 100% on me.

I completely understand that expectation. We tried to achieve that regarding monitor-specific scaling everything possible happens inside SWT, such that when enabling the feature consumers will have a good out-of-box experience. As you say, at worst some things may still look non-perfect (e.g., blurry), but no UI should completely break. There are some cases where this is conceptually not possible and we tried to use as good heuristics as possible to achieve a good experience without or until consumers are adapted. One such case is the handling of global coordinates (because without API breakage it is conceptually not possible to adapt the global coordinate system in a way that reflects monitor-specific scale values) and another such scenario are specific image use case like the double-buffering scenario. Hopefully, we now have a somehow acceptable intermediate state for the double-buffering scenario as well.

[ptziegler]

Thanks again for looking deeper into this topic 😄

I hope that we have now uncovered the essential regression and solved it with #2163 for now. It might be worth checking whether eclipse-gef/gef-classic#740 is (positively) affected by that as well.

The result with the latest SWT artifacts looks significantly better. Though I still wonder whether the current implementation of the BufferedGraphicsSource still makes sense when considering auto-scaling. As you said several times now, an GC created via new GC(image) doesn't retain a reference to the underlying monitor, which might then lead to visual artifacts if e.g. the zoom is changed. But that's a topic for GEF, not SWT.

[HeikoKlare]

The BufferedGraphicsSource seems to be one of such use cases we see where it's probably impossible to use a ImageGcDrawer to generate the image data at a proper zoom level. For such cases, we investigate whether it's possible to store operations executed on a GC in that GC to later on apply (again) for whatever zoom is required. So instead (or in addition to) executing a drawLine on a GC, the GC would store that operation and then execute it for the appropriate zoom as soon as the "result of the GC" is requested. So instead of encapsulating the GC operations inside an ImageGcDrawer, they would be stored in GC to be applied later on. That's what we also did for other resources like Region or Transform.
That could potentially relieve all consumers of Image+GC from adapting their code to conform with multi-zoom scenarios. That's a bit more precisely what I meant with my previous comment:

We have already discussed how to further improve SWT to maybe avoid the adoption of ImageGcDrawer particular in cases where it's not that easy to achieve (as the image/GC is passed around to be drawn on making the encapsulation in an ImageGcDrawer very difficult). We hope to come up with a proposal how to achieve that for 4.37 M1.