From 61ee2c166ab3ab3fb3ec94c0d3b199c33de799cc Mon Sep 17 00:00:00 2001 From: Heiko Klare Date: Wed, 8 Jul 2026 21:01:24 +0200 Subject: [PATCH] [Win32] Avoid redundant handle creation in GC.drawImage() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit GC.drawImage() operations use a temporary image handle mechanism that creates a bitmap handle scaled to the pixel size required for each drawing operation. When an image is not available at the requested zoom, the closest available zoom is used instead. Previously, a fresh handle was created for every new drawing size, even when the same underlying image data would be used. For example, an image only available at 100% zoom (such as a plain PNG without HiDPI variants) caused a new handle to be allocated and immediately discarded on every drawImage() call that requested a different size. This change introduces the concept of "nearest available zoom" on image providers: each provider now reports, for a given requested zoom, the effective zoom at which it would actually supply data. This information is stored alongside the cached temporary handle after each drawImage(). Before allocating a new temporary handle for a different draw size, the nearest available zoom is used to look up an already existing handle: first in the image's persistent handle cache, then by comparing it against the zoom recorded with the previously cached temporary handle. If a matching handle is found, it is reused instead of creating a new one. If no handle exists and the nearest available zoom is 100%, a persistent handle is created eagerly: this frees any image data previously retained for API calls such as getImageData() and makes the handle available through the regular persistent handle lookup for all subsequent calls. This applies to any image where consecutive drawImage() calls at different sizes map to the same underlying data — e.g. a 100%-only image drawn at 200% and 300%, or an image with 100% and 200% variants when drawing at sizes between 200% and 300%. Four regression tests are added to ImagesWin32Tests to verify handle reuse (positive cases), the absence of unintended reuse across different nearest-available-zoom regions (negative case), and that a persistent handle created outside of drawImage() is found and reused by the nearest-available-zoom lookup. Fixes https://github.com/eclipse-platform/eclipse.platform.swt/issues/3419 --- .../swt/graphics/ImagesWin32Tests.java | 136 +++++++++++++++++- .../win32/org/eclipse/swt/graphics/Image.java | 106 ++++++++++++-- 2 files changed, 225 insertions(+), 17 deletions(-) diff --git a/bundles/org.eclipse.swt/Eclipse SWT Tests/win32/org/eclipse/swt/graphics/ImagesWin32Tests.java b/bundles/org.eclipse.swt/Eclipse SWT Tests/win32/org/eclipse/swt/graphics/ImagesWin32Tests.java index 6b60c2a20df..cf48800de38 100644 --- a/bundles/org.eclipse.swt/Eclipse SWT Tests/win32/org/eclipse/swt/graphics/ImagesWin32Tests.java +++ b/bundles/org.eclipse.swt/Eclipse SWT Tests/win32/org/eclipse/swt/graphics/ImagesWin32Tests.java @@ -13,7 +13,8 @@ *******************************************************************************/ package org.eclipse.swt.graphics; -import static org.junit.Assert.assertEquals; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNotEquals; import org.eclipse.swt.*; import org.eclipse.swt.internal.*; @@ -30,9 +31,140 @@ public void testImageIconTypeShouldNotChangeAfterCallingGetHandleForDifferentZoo Image icon = Display.getDefault().getSystemImage(SWT.ICON_ERROR); try { Image.win32_getHandle(icon, 200); - assertEquals("Image type should stay to SWT.ICON", SWT.ICON, icon.type); + assertEquals(SWT.ICON, icon.type, "Image type should stay to SWT.ICON"); } finally { icon.dispose(); } } + + /** + * Tests that a GC.drawImage() handle is reused across consecutive calls at + * different pixel sizes when the image only provides 100% zoom data. Because + * every zoom request falls back to the same 100% data, the effective (nearest + * available) zoom is always 100%, and a freshly allocated handle should not be + * required for each different draw size. + *

+ * See https://github.com/eclipse-platform/eclipse.platform.swt/issues/3419 + */ + @Test + public void testDrawingHandleIsReusedForSingleZoomImageAtDifferentSizes() { + PaletteData palette = new PaletteData(0xFF0000, 0xFF00, 0xFF); + ImageData imageData = new ImageData(10, 10, 24, palette); + // Provider only has 100% data; returns null for every other zoom + Image image = new Image(Display.getDefault(), (ImageDataProvider) zoom -> zoom == 100 ? imageData : null); + long[] firstHandle = {0}; + long[] secondHandle = {0}; + try { + // 20x20 pixels → 200% zoom equivalent for a 10x10 base image + image.executeOnImageHandleAtBestFittingSize(h -> firstHandle[0] = h.handle(), 20, 20); + // 30x30 pixels → 300% zoom equivalent; provider still falls back to 100%, + // so the nearest available zoom is still 100% and the handle must be reused + image.executeOnImageHandleAtBestFittingSize(h -> secondHandle[0] = h.handle(), 30, 30); + assertNotEquals(0L, firstHandle[0], "First handle should be non-zero"); + assertEquals(firstHandle[0], secondHandle[0], + "Consecutive GC.drawImage() calls at different sizes should reuse the same " + + "handle when the nearest available zoom is the same (100% in this case)"); + } finally { + image.dispose(); + } + } + + /** + * Tests that a GC.drawImage() handle is reused across consecutive calls at + * different pixel sizes when the image provides data at 100% and 200% zoom. + * Sizes that both map to the 200% nearest available zoom (e.g. 200% and 250%) + * should share the same underlying handle without re-allocating it. + *

+ * See https://github.com/eclipse-platform/eclipse.platform.swt/issues/3419 + */ + @Test + public void testDrawingHandleIsReusedForTwoZoomImageAtSizesWithSameNearestZoom() { + PaletteData palette = new PaletteData(0xFF0000, 0xFF00, 0xFF); + ImageData imageData100 = new ImageData(10, 10, 24, palette); + ImageData imageData200 = new ImageData(20, 20, 24, palette); + // Provider has explicit data at 100% and 200%; returns null for anything else + Image image = new Image(Display.getDefault(), (ImageDataProvider) zoom -> zoom == 100 ? imageData100 : zoom == 200 ? imageData200 : null); + long[] firstHandle = {0}; + long[] secondHandle = {0}; + try { + // 20x20 pixels → exactly 200% zoom for the 10x10 base image; uses 200% data + image.executeOnImageHandleAtBestFittingSize(h -> firstHandle[0] = h.handle(), 20, 20); + // 25x25 pixels → 250% zoom equivalent; nearest available is 200%, so the + // previously cached 200% handle should be reused + image.executeOnImageHandleAtBestFittingSize(h -> secondHandle[0] = h.handle(), 25, 25); + assertNotEquals(0L, firstHandle[0], "First handle should be non-zero"); + assertEquals(firstHandle[0], secondHandle[0], + "Consecutive GC.drawImage() calls at different sizes should reuse the same " + + "handle when the nearest available zoom is the same (200% in this case)"); + } finally { + image.dispose(); + } + } + + /** + * Tests that GC.drawImage() handles differ when consecutive calls at different + * pixel sizes land in different nearest-available-zoom regions for an image + * that provides distinct data at 100% and 200%. A size mapping to 100% and a + * size mapping to 200% must not share the same native handle, as they would + * represent different pixel content. + *

+ * See https://github.com/eclipse-platform/eclipse.platform.swt/issues/3419 + */ + @Test + public void testHandlesAreDifferentForTwoZoomImageAtDifferentNearestZooms() { + PaletteData palette = new PaletteData(0xFF0000, 0xFF00, 0xFF); + ImageData imageData100 = new ImageData(10, 10, 24, palette); + ImageData imageData200 = new ImageData(20, 20, 24, palette); + // Provider has explicit data at 100% and 200%; returns null for anything else + Image image = new Image(Display.getDefault(), (ImageDataProvider) zoom -> zoom == 100 ? imageData100 : zoom == 200 ? imageData200 : null); + long[] handle100Zone = {0}; + long[] handle200Zone = {0}; + try { + // 10x10 pixels → 100% zoom for the 10x10 base image; nearest available is 100% + image.executeOnImageHandleAtBestFittingSize(h -> handle100Zone[0] = h.handle(), 10, 10); + // 20x20 pixels → 200% zoom; nearest available is 200% → must differ from the + // 100% handle since the underlying pixel data is different + image.executeOnImageHandleAtBestFittingSize(h -> handle200Zone[0] = h.handle(), 20, 20); + assertNotEquals(0L, handle100Zone[0], "First handle should be non-zero"); + assertNotEquals(handle100Zone[0], handle200Zone[0], + "GC.drawImage() calls where the nearest available zoom differs must not " + + "reuse the same handle (100% data vs 200% data)"); + } finally { + image.dispose(); + } + } + + /** + * Tests that a persistent native handle already created via + * {@link Image#win32_getHandle(Image, int)} is found and reused by + * GC.drawImage() when the nearest available zoom for the requested draw size + * maps to the same zoom. This verifies that the + * {@code imageHandleManager.get(nearestAvailableZoom)} lookup is effective and + * avoids redundant handle allocation. + *

+ * See https://github.com/eclipse-platform/eclipse.platform.swt/issues/3419 + */ + @Test + public void testDrawImageReusesExistingPersistentHandleForNearestAvailableZoom() { + PaletteData palette = new PaletteData(0xFF0000, 0xFF00, 0xFF); + ImageData imageData = new ImageData(10, 10, 24, palette); + // Provider only has 100% data; every zoom falls back to 100% + Image image = new Image(Display.getDefault(), (ImageDataProvider) zoom -> zoom == 100 ? imageData : null); + try { + // Force creation of a persistent 100% handle (as would happen via GC.drawImage + // on a 100% zoom canvas or via Image.getImageData()) + long persistentHandle = Image.win32_getHandle(image, 100); + assertNotEquals(0L, persistentHandle, "Persistent handle should be non-zero"); + long[] drawHandle = {0}; + // 20x20 pixels → 200% zoom equivalent for the 10x10 base image; nearest + // available is still 100%, so the already-cached persistent handle must be + // returned without allocating a new one + image.executeOnImageHandleAtBestFittingSize(h -> drawHandle[0] = h.handle(), 20, 20); + assertEquals(persistentHandle, drawHandle[0], + "GC.drawImage() should reuse the existing persistent handle when the " + + "nearest available zoom matches the cached handle's zoom (100% here)"); + } finally { + image.dispose(); + } + } } 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 12a78ed8464..29e6fc94aef 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 @@ -201,28 +201,39 @@ public String toString() { } private class HandleAtSize { + record TemporaryHandleForZoom(DestroyableImageHandle handle, int zoom) {} + private InternalImageHandle handleContainer = null; - private DestroyableImageHandle temporaryHandleContainer = null; + private TemporaryHandleForZoom temporaryHandleContainer = null; private int requestedWidth = -1; private int requestedHeight = -1; public void destroy() { - if (temporaryHandleContainer != null) { - temporaryHandleContainer.destroy(); - temporaryHandleContainer = null; + TemporaryHandleForZoom previousHandle = reset(); + if (previousHandle != null) { + previousHandle.handle().destroy(); } + } + + private TemporaryHandleForZoom reset() { + TemporaryHandleForZoom previousHandle = temporaryHandleContainer; + temporaryHandleContainer = null; handleContainer = null; requestedWidth = -1; requestedHeight = -1; + return previousHandle; } public ImageHandle refresh(int width, int height) { if (!isReusable(width, height)) { - destroy(); + TemporaryHandleForZoom previousHandle = reset(); requestedWidth = width; requestedHeight = height; handleContainer = createHandleAtExactSize(width, height) - .orElseGet(() -> getOrCreateImageHandleAtClosestSize(width, height)); + .orElseGet(() -> getOrCreateImageHandleAtClosestSize(width, height, previousHandle)); + if (previousHandle != null && previousHandle.handle() != handleContainer) { + previousHandle.handle().destroy(); + } } return handleContainer; } @@ -238,23 +249,32 @@ private boolean isReusable(int width, int height) { private Optional createHandleAtExactSize(int width, int height) { Optional imageData = imageProvider.loadImageDataAtExactSize(width, height); if (imageData.isPresent()) { - temporaryHandleContainer = init(imageData.get(), -1); - return Optional.of(temporaryHandleContainer); + temporaryHandleContainer = new TemporaryHandleForZoom(init(imageData.get(), -1), 0); + return Optional.of(temporaryHandleContainer.handle()); } return Optional.empty(); } - private InternalImageHandle getOrCreateImageHandleAtClosestSize(int widthHint, int heightHint) { + private InternalImageHandle getOrCreateImageHandleAtClosestSize(int widthHint, int heightHint, TemporaryHandleForZoom previousHandle) { Rectangle bounds = getBounds(100); int imageZoomForWidth = 100 * widthHint / bounds.width; int imageZoomForHeight = 100 * heightHint / bounds.height; int imageZoom = DPIUtil.getZoomForAutoscaleProperty(Math.max(imageZoomForWidth, imageZoomForHeight)); - InternalImageHandle bestFittingHandle = imageHandleManager.get(imageZoom); - if (bestFittingHandle == null) { - ImageData bestFittingImageData = imageProvider.loadImageData(imageZoom).element(); - bestFittingHandle = temporaryHandleContainer = init(bestFittingImageData, -1); + int nearestAvailableZoom = imageProvider.nearestAvailableZoom(imageZoom); + InternalImageHandle bestFittingHandle = imageHandleManager.get(nearestAvailableZoom); + if (bestFittingHandle != null) { + return bestFittingHandle; + } + if (nearestAvailableZoom == 100) { + return getHandleInternal(100, 100); } - return bestFittingHandle; + if (previousHandle != null && previousHandle.zoom() == nearestAvailableZoom) { + temporaryHandleContainer = previousHandle; + return previousHandle.handle(); + } + ElementAtZoom imageData = imageProvider.loadImageData(imageZoom); + temporaryHandleContainer = new TemporaryHandleForZoom(init(imageData.element(), -1), imageData.zoom()); + return temporaryHandleContainer.handle(); } } @@ -957,6 +977,10 @@ public static long win32_getHandle (Image image, int zoom) { } ImageHandle getHandle (int targetZoom, int nativeZoom) { + return getHandleInternal(targetZoom, nativeZoom); +} + +InternalImageHandle getHandleInternal (int targetZoom, int nativeZoom) { if (isDisposed()) { return null; } @@ -2090,6 +2114,8 @@ protected boolean isPersistentImageHandleRequriedForImageData() { return false; } + abstract int nearestAvailableZoom(int zoom); + /** * Returns image data at the best-fitting available zoom for the given zoom. * The returned data will have a potential gray/disable style applied. @@ -2102,7 +2128,7 @@ protected boolean isPersistentImageHandleRequriedForImageData() { ElementAtZoom getClosestAvailableImageData(int zoom) { TreeSet availableZooms = new TreeSet<>(imageHandleManager.getAllZooms()); - int closestZoom = Optional.ofNullable(availableZooms.higher(zoom)).orElse(availableZooms.lower(zoom)); + int closestZoom = availableZooms.contains(zoom) ? zoom : Optional.ofNullable(availableZooms.higher(zoom)).orElse(availableZooms.lower(zoom)); ImageData imageData = imageHandleManager.get(closestZoom).getImageData(); return new ElementAtZoom<>(imageData, closestZoom); } @@ -2179,6 +2205,11 @@ protected DestroyableImageHandle newImageHandle(ZoomContext zoomContext) { ImageData resizedData = newImageData (zoomContext.targetZoom()); return newImageHandle(resizedData, zoomContext); } + + @Override + int nearestAvailableZoom(int zoom) { + return zoomForHandle; + } } private abstract class ImageFromImageDataProviderWrapper extends AbstractImageProviderWrapper { @@ -2249,6 +2280,11 @@ protected ElementAtZoom loadImageData(int zoom) { AbstractImageProviderWrapper createCopy(Image image) { return image.new PlainImageDataProviderWrapper(this.imageDataAtBaseZoom); } + + @Override + int nearestAvailableZoom(int zoom) { + return baseZoom; + } } private class MaskedImageDataProviderWrapper extends ImageFromImageDataProviderWrapper { @@ -2281,6 +2317,11 @@ protected ElementAtZoom loadImageData(int zoom) { AbstractImageProviderWrapper createCopy(Image image) { return image.new MaskedImageDataProviderWrapper(this.srcAt100, this.maskAt100); } + + @Override + int nearestAvailableZoom(int zoom) { + return 100; + } } private class ImageDataLoaderStreamProviderWrapper extends ImageFromImageDataProviderWrapper { @@ -2325,6 +2366,14 @@ protected Optional loadImageDataAtExactSize(int targetWidth, int targ } return Optional.empty(); } + + @Override + int nearestAvailableZoom(int zoom) { + if (ImageDataLoader.isDynamicallySizable(new ByteArrayInputStream(this.inputStreamData))) { + return zoom; + } + return FileFormat.DEFAULT_ZOOM; + } } private class PlainImageProviderWrapper extends AbstractImageProviderWrapper { @@ -2388,6 +2437,11 @@ protected ElementAtZoom loadImageData(int zoom) { return getClosestAvailableImageData(zoom); } + @Override + int nearestAvailableZoom(int zoom) { + return getClosestAvailableImageData(zoom).zoom(); + } + @Override protected DestroyableImageHandle newImageHandle(ZoomContext zoomContext) { int targetZoom = zoomContext.targetZoom(); @@ -2533,6 +2587,7 @@ private class ImageFileNameProviderWrapper extends BaseImageProviderWrapper loadImageData(int zoom) { ElementAtZoom fileForZoom = DPIUtil.validateAndGetImagePathAtZoom(provider, zoom); @@ -2564,6 +2619,14 @@ protected ElementAtZoom loadImageData(int zoom) { return adaptImageDataIfDisabledOrGray(imageDataAtZoom); } + @Override + int nearestAvailableZoom(int zoom) { + if (provider instanceof ImageDataAtSizeProvider) { + return zoom; + } + return DPIUtil.validateAndGetImagePathAtZoom(provider, zoom).zoom(); + } + @Override public int hashCode() { return Objects.hash(provider, styleFlag); @@ -2807,6 +2870,14 @@ protected Optional loadImageDataAtExactSize(int targetWidth, int targ } return Optional.empty(); } + + @Override + int nearestAvailableZoom(int zoom) { + if (provider instanceof ImageDataAtSizeProvider) { + return zoom; + } + return DPIUtil.validateAndGetImageDataAtZoom (provider, zoom).zoom(); + } } private class ImageGcDrawerWrapper extends DynamicImageProviderWrapper { @@ -2848,6 +2919,11 @@ protected ElementAtZoom loadImageData(int zoom) { return new ElementAtZoom<>(loadImageData(new ZoomContext(zoom)), zoom); } + @Override + int nearestAvailableZoom(int zoom) { + return zoom; + } + private ImageData loadImageData(ZoomContext zoomContext) { currentZoom = zoomContext; int targetZoom = zoomContext.targetZoom();