[Win32] Ensure consistent image data returned for filename/data provider

Due to the on-demand creation of image handles, there is not necessarily
a handles anymore from which image data is retrieved when requesting is
via the getImageData(...) methods. This results in potentially different
kinds of image data (including different anti-aliasing results)
depending on whether a handle has already been created for an image at
the given zoom or not.

This change adapts the Image implementation for ImageDataProvider and
ImageFileNameProvider to always use the image data retrieved from a
native handle. To this end, it temporarily creates a handle if
necessary. In order to avoid repeated loading and handle creation for
the same source of image, a cache for the already retrieved image data
is introduced. For both the consistent retrieval of image data and the
avoidance of repeated loading of image data according test cases are
added.

Author: HeikoKlare

bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/graphics/Image.java

@@ -16,6 +16,7 @@
 
 import java.io.*;
 import java.util.*;
+import java.util.function.*;
 
 import org.eclipse.swt.*;
 import org.eclipse.swt.internal.*;
@@ -2176,8 +2177,9 @@ public boolean equals(Object otherProvider) {
 }
 
 private abstract class BaseImageProviderWrapper<T> extends DynamicImageProviderWrapper {
+	private final Map<Integer, ImageData> cachedImageData = new HashMap<>();
+
 	protected final T provider;
-	private final Map<Integer, Rectangle> zoomToBounds = new HashMap<>();
 
 	BaseImageProviderWrapper(T provider, Class<T> expectedClass) {
 		checkProvider(provider, expectedClass);
@@ -2190,17 +2192,38 @@ Object getProvider() {
 	}
 
 	@Override
-	protected Rectangle getBounds(int zoom) {
-		if (zoomLevelToImageHandle.containsKey(zoom)) {
-			ImageHandle imgHandle = zoomLevelToImageHandle.get(zoom);
-			return new Rectangle(0, 0, imgHandle.width, imgHandle.height);
-		}
-		if (!zoomToBounds.containsKey(zoom)) {
-			ImageData imageData = getImageData(zoom);
-			Rectangle rectangle = new Rectangle(0, 0, imageData.width, imageData.height);
-			zoomToBounds.put(zoom, rectangle);
+	final ImageData getImageData(int zoom) {
+		Function<Integer, ImageData> imageDataRetrival = zoomToRetrieve -> {
+			ImageHandle handle = initializeHandleFromSource(zoomToRetrieve);
+			ImageData data = handle.getImageData();
+			destroyHandleForZoom(zoomToRetrieve);
+			return data;
+		};
+		return cachedImageData.computeIfAbsent(zoom, imageDataRetrival);
+	}
+
+	@Override
+	final ImageHandle getImageMetadata(int zoom) {
+		ImageData cachedData = cachedImageData.remove(zoom);
+		if (cachedData != null) {
+			return init(cachedData, zoom);
 		}
-		return zoomToBounds.get(zoom);
+		return initializeHandleFromSource(zoom);
+	}
+
+	private ImageHandle initializeHandleFromSource(int zoom) {
+		ElementAtZoom<ImageData> imageDataAtZoom = loadImageData(zoom);
+		ImageData imageData = scaleImageData(imageDataAtZoom.element(), zoom, imageDataAtZoom.zoom());
+		imageData = adaptImageDataIfDisabledOrGray(imageData);
+		return init(imageData, zoom);
+	}
+
+	protected abstract ElementAtZoom<ImageData> loadImageData(int zoom);
+
+	@Override
+	protected Rectangle getBounds(int zoom) {
+		ImageData imageData = getImageData(zoom);
+		return new Rectangle(0, 0, imageData.width, imageData.height);
 	}
 }
 
@@ -2210,7 +2233,7 @@ private class ImageFileNameProviderWrapper extends BaseImageProviderWrapper<Imag
 	}
 
 	@Override
-	ImageData getImageData(int zoom) {
+	protected ElementAtZoom<ImageData> loadImageData(int zoom) {
 		ElementAtZoom<String> fileForZoom = DPIUtil.validateAndGetImagePathAtZoom(provider, zoom);
 		ImageHandle nativeInitializedImage;
 		if (zoomLevelToImageHandle.containsKey(fileForZoom.zoom())) {
@@ -2226,23 +2249,7 @@ ImageData getImageData(int zoom) {
 			imageDataAtZoom = new ElementAtZoom<>(nativeInitializedImage.getImageData(), fileForZoom.zoom());
 			destroyHandleForZoom(zoom);
 		}
-		ImageData imageData = scaleIfNecessary(imageDataAtZoom, zoom);
-		imageData = adaptImageDataIfDisabledOrGray(imageData);
-		return imageData;
-	}
-
-	private ImageData scaleIfNecessary(ElementAtZoom<ImageData> imageDataAtZoom, int zoom) {
-		if (imageDataAtZoom.zoom() != zoom) {
-			return scaleImageData(imageDataAtZoom.element(), zoom, imageDataAtZoom.zoom());
-		}
-		return imageDataAtZoom.element();
-	}
-
-	@Override
-	ImageHandle getImageMetadata(int zoom) {
-		ImageData imageData = getImageData(zoom);
-		init(imageData, zoom);
-		return zoomLevelToImageHandle.get(zoom);
+		return imageDataAtZoom;
 	}
 
 	@Override
@@ -2455,18 +2462,8 @@ private class ImageDataProviderWrapper extends BaseImageProviderWrapper<ImageDat
 	}
 
 	@Override
-	ImageData getImageData(int zoom) {
-		ElementAtZoom<ImageData> data = DPIUtil.validateAndGetImageDataAtZoom (provider, zoom);
-		return scaleImageData(data.element(), zoom, data.zoom());
-	}
-
-	@Override
-	ImageHandle getImageMetadata(int zoom) {
-		ElementAtZoom<ImageData> imageCandidate = DPIUtil.validateAndGetImageDataAtZoom (provider, zoom);
-		ImageData resizedData = scaleImageData(imageCandidate.element(), zoom, imageCandidate.zoom());
-		ImageData newData = adaptImageDataIfDisabledOrGray(resizedData);
-		init(newData, zoom);
-		return zoomLevelToImageHandle.get(zoom);
+	protected ElementAtZoom<ImageData> loadImageData(int zoom) {
+		return DPIUtil.validateAndGetImageDataAtZoom (provider, zoom);
 	}
 
 	@Override

tests/org.eclipse.swt.tests/JUnit Tests/org/eclipse/swt/tests/junit/Test_org_eclipse_swt_graphics_Image.java

@@ -20,13 +20,19 @@
 import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertNotNull;
 import static org.junit.Assert.assertNull;
+import static org.junit.Assert.assertSame;
 import static org.junit.Assert.assertTrue;
 import static org.junit.Assert.fail;
+import static org.junit.Assume.assumeFalse;
+import static org.junit.Assume.assumeTrue;
 
 import java.io.File;
 import java.io.IOException;
 import java.io.InputStream;
 import java.net.URL;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.util.Comparator;
 import java.util.function.Consumer;
 
 import org.eclipse.swt.SWT;
@@ -1262,4 +1268,58 @@ public void test_updateWidthHeightAfterDPIChange() {
 		DPIUtil.setDeviceZoom(deviceZoom);
 	}
 }
+
+@Test
+public void test_imageDataIsCached() {
+	assumeTrue("On-demand image creation only implemented for Windows", SwtTestUtil.isWindows);
+	String imagePath = getPath("collapseall.png");
+	ImageFileNameProvider imageFileNameProvider = __ -> {
+		return imagePath;
+	};
+	Image fileNameProviderImage = new Image(display, imageFileNameProvider);
+	assertSame(fileNameProviderImage.getImageData(100), fileNameProviderImage.getImageData(100));
+}
+
+@Test
+public void test_imageDataSameViaDifferentProviders() {
+	assumeFalse("Cocoa generates inconsistent image data", SwtTestUtil.isCocoa);
+	String imagePath = getPath("collapseall.png");
+	ImageFileNameProvider imageFileNameProvider = __ -> {
+		return imagePath;
+	};
+	ImageDataProvider dataProvider = __ -> {
+		try (InputStream imageStream = Files.newInputStream(Path.of(imagePath))) {
+			return new ImageData(imageStream);
+		} catch (IOException e) {
+		}
+		return null;
+	};
+	Image fileNameProviderImage = new Image(display, imageFileNameProvider);
+	Image dataProviderImage = new Image(display, dataProvider);
+	ImageData dataFromFileNameProviderImage = fileNameProviderImage.getImageData(100);
+	ImageData dataFromImageDescriptorImage = dataProviderImage.getImageData(100);
+	assertEquals(0, imageDataComparator().compare(dataFromFileNameProviderImage, dataFromImageDescriptorImage));
+
+	fileNameProviderImage.dispose();
+	dataProviderImage.dispose();
+}
+
+private Comparator<ImageData> imageDataComparator() {
+	return Comparator.<ImageData>comparingInt(d -> d.width) //
+			.thenComparing(d -> d.height) //
+			.thenComparing((ImageData firstData, ImageData secondData) -> {
+				for (int x = 0; x < firstData.width; x++) {
+					for (int y = 0; y < firstData.height; y++) {
+						if (firstData.getPixel(x, y) != secondData.getPixel(x, y)) {
+							return -1;
+						}
+						if (firstData.getAlpha(x, y) != secondData.getAlpha(x, y)) {
+							return -1;
+						}
+					}
+				}
+				return 0;
+			});
+}
+
 }