Skip to content

[Win32] Avoid redundant handle creation in GC.drawImage() by tracking effective zoom #3429

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
HeikoKlare merged 1 commit into from
Jul 22, 2026
+225 −17
Merged

[Win32] Avoid redundant handle creation in GC.drawImage() by tracking effective zoom #3429

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
136 changes: 134 additions & 2 deletions ...es/org.eclipse.swt/Eclipse SWT Tests/win32/org/eclipse/swt/graphics/ImagesWin32Tests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.*;
Expand All @@ -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.
* <p>
* 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.
* <p>
* 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);
Comment on lines +90 to +94

@r-mennig r-mennig Jul 21, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe the inverse test case could be added as well:

When providing imageData for 100% and 200% zoom, ensure that the handle is not reused when the first nearest available zoom is 100% and the second nearest available zoom is 200%

@HeikoKlare HeikoKlare Jul 21, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea. I have amended the commit with an according test (and an additional one testing the reuse of persisted handles to more completely cover the changes in this PR).

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.
* <p>
* 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.
* <p>
* 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();
}
}
}
106 changes: 91 additions & 15 deletions bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/graphics/Image.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand All @@ -238,23 +249,32 @@ private boolean isReusable(int width, int height) {
private Optional<InternalImageHandle> createHandleAtExactSize(int width, int height) {
Optional<ImageData> 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);

@r-mennig r-mennig Jul 21, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't a handle at exact size have zoom 100?

@HeikoKlare HeikoKlare Jul 21, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This zoom is only used for identifying if a temporary handle can be reused when requesting it for a specific zoom. But an image that can be created at exact size will be precisely created for every requested size, such that there won't be any reuse based on the zoom. So in practice, it does not really matter what we put here.

If there was a scenario where this is not the case (an image provider that gives you an image at exact size for a specific set of width/height combinations and only a fixed-size or zoom-based versions for the rest), you would need to know the exact size at 100% to identify if you should reuse the currently created handle for a specific width/height combination when requesting the image at 100%.

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> imageData = imageProvider.loadImageData(imageZoom);
temporaryHandleContainer = new TemporaryHandleForZoom(init(imageData.element(), -1), imageData.zoom());
return temporaryHandleContainer.handle();
}

}
Expand Down Expand Up @@ -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;
}
Expand Down Expand Up @@ -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.
Expand All @@ -2102,7 +2128,7 @@ protected boolean isPersistentImageHandleRequriedForImageData() {

ElementAtZoom<ImageData> getClosestAvailableImageData(int zoom) {
TreeSet<Integer> 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);
}
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -2249,6 +2280,11 @@ protected ElementAtZoom<ImageData> 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 {
Expand Down Expand Up @@ -2281,6 +2317,11 @@ protected ElementAtZoom<ImageData> 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 {
Expand Down Expand Up @@ -2325,6 +2366,14 @@ protected Optional<ImageData> 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 {
Expand Down Expand Up @@ -2388,6 +2437,11 @@ protected ElementAtZoom<ImageData> 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();
Expand Down Expand Up @@ -2533,6 +2587,7 @@ private class ImageFileNameProviderWrapper extends BaseImageProviderWrapper<Imag
newImageData(DPIUtil.getDeviceZoom());
}


@Override
protected ElementAtZoom<ImageData> loadImageData(int zoom) {
ElementAtZoom<String> fileForZoom = DPIUtil.validateAndGetImagePathAtZoom(provider, zoom);
Expand Down Expand Up @@ -2564,6 +2619,14 @@ protected ElementAtZoom<ImageData> 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);
Expand Down Expand Up @@ -2807,6 +2870,14 @@ protected Optional<ImageData> 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 {
Expand Down Expand Up @@ -2848,6 +2919,11 @@ protected ElementAtZoom<ImageData> 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();
Expand Down
Loading