From aff8f0453120d9e235ec5382b96b33e128c5ffc0 Mon Sep 17 00:00:00 2001 From: Heiko Klare Date: Thu, 3 Apr 2025 18:02:26 +0200 Subject: [PATCH] [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. --- .../win32/org/eclipse/swt/graphics/Image.java | 79 +++++++++---------- .../Test_org_eclipse_swt_graphics_Image.java | 60 ++++++++++++++ 2 files changed, 98 insertions(+), 41 deletions(-) diff --git a/bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/graphics/Image.java b/bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/graphics/Image.java index 6844fe9b434..48cd8e27ee7 100644 --- a/bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/graphics/Image.java +++ b/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 extends DynamicImageProviderWrapper { + private final Map cachedImageData = new HashMap<>(); + protected final T provider; - private final Map zoomToBounds = new HashMap<>(); BaseImageProviderWrapper(T provider, Class 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 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 imageDataAtZoom = loadImageData(zoom); + ImageData imageData = scaleImageData(imageDataAtZoom.element(), zoom, imageDataAtZoom.zoom()); + imageData = adaptImageDataIfDisabledOrGray(imageData); + return init(imageData, zoom); + } + + protected abstract ElementAtZoom 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 loadImageData(int zoom) { ElementAtZoom 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 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 data = DPIUtil.validateAndGetImageDataAtZoom (provider, zoom); - return scaleImageData(data.element(), zoom, data.zoom()); - } - - @Override - ImageHandle getImageMetadata(int zoom) { - ElementAtZoom 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 loadImageData(int zoom) { + return DPIUtil.validateAndGetImageDataAtZoom (provider, zoom); } @Override diff --git a/tests/org.eclipse.swt.tests/JUnit Tests/org/eclipse/swt/tests/junit/Test_org_eclipse_swt_graphics_Image.java b/tests/org.eclipse.swt.tests/JUnit Tests/org/eclipse/swt/tests/junit/Test_org_eclipse_swt_graphics_Image.java index c16847179f3..1ddd87f5e5f 100644 --- a/tests/org.eclipse.swt.tests/JUnit Tests/org/eclipse/swt/tests/junit/Test_org_eclipse_swt_graphics_Image.java +++ b/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 imageDataComparator() { + return Comparator.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; + }); +} + }