Skip to content

Ensure DrawImage deals with scenario ImageData not linearly scaled #2250

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

Closed
wants to merge 1 commit into from
Closed

Ensure DrawImage deals with scenario ImageData not linearly scaled #2250

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
84 changes: 84 additions & 0 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 @@ -25,6 +25,90 @@
@ExtendWith(WithMonitorSpecificScalingExtension.class)
class ImagesWin32Tests {

@Test
public void test_imageScaledFrom150_when125PercentImageUnavailable() {
Display display = Display.getDefault();
GC gc = new GC(display);
GCData gcData = gc.getGCData();
gcData.nativeZoom = 125;
ImageDataProvider imageDataProviderReturnsNullAt125 = zoom -> {
if (zoom == 125) {
return null;
}
float scaleFactor = zoom / 100f;
int scaledWidth = Math.round(16 * scaleFactor);
int scaledHeight = Math.round(16 * scaleFactor);
return new ImageData(scaledWidth, scaledHeight, 1, new PaletteData(new RGB(255, 0, 0)));
};
Image image = new Image(display, imageDataProviderReturnsNullAt125);
gc.drawImage(image, 0, 0, 16, 16, 0, 0, 16, 16);
gc.dispose();
image.dispose();
}

@Test
public void test_imageScaledFrom100_when125And150PercentImagesUnavailable() {
Display display = Display.getDefault();
GC gc = new GC(display);
GCData gcData = gc.getGCData();
gcData.nativeZoom = 125;
ImageDataProvider imageDataProviderReturnsNullAt125and150 = zoom -> {
if (zoom == 125) {
return null;
}
if (zoom == 150) {
return null;
}
float scaleFactor = zoom / 100f;
int scaledWidth = Math.round(16 * scaleFactor);
int scaledHeight = Math.round(16 * scaleFactor);
return new ImageData(scaledWidth, scaledHeight, 1, new PaletteData(new RGB(255, 0, 0)));
};
Image image = new Image(display, imageDataProviderReturnsNullAt125and150);
gc.drawImage(image, 0, 0, 16, 16, 0, 0, 16, 16);
gc.dispose();
image.dispose();
}

@Test
public void test_drawImage_when130PercentImageUnavailable() {
Display display = Display.getDefault();
GC gc = new GC(display);
GCData gcData = gc.getGCData();
gcData.nativeZoom = 130;
ImageDataProvider imageDataProviderReturnsNullAt125 = zoom -> {
if (zoom == 130){
return null;
}
float scaleFactor = zoom / 100f;
int scaledWidth = Math.round(16 * scaleFactor);
int scaledHeight = Math.round(16 * scaleFactor);
return new ImageData(scaledWidth, scaledHeight, 1, new PaletteData(new RGB(255, 0, 0)));
};
Image image = new Image(display, imageDataProviderReturnsNullAt125);
gc.drawImage(image, 0, 0, 16, 16, 0, 0, 16, 16);
gc.dispose();
image.dispose();
}

@Test
public void test_drawImageUsesWronglyScaledImageDataProvider() {
Display display = Display.getDefault();
GC gc = new GC(display);
GCData gcData = gc.getGCData();
gcData.nativeZoom = 125;
ImageDataProvider incorrectlyScaledimageDataProvider = zoom -> {
int scaleFactor = zoom / 100;
int scaledWidth = Math.round(16 * scaleFactor);
int scaledHeight = Math.round(16 * scaleFactor);
return new ImageData(scaledWidth, scaledHeight, 1, new PaletteData(new RGB(255, 0, 0)));
};
Image image = new Image(display, incorrectlyScaledimageDataProvider);
gc.drawImage(image, 0, 0, 16, 16, 0, 0, 16, 16);
gc.dispose();
image.dispose();
}

@Test
public void testImageIconTypeShouldNotChangeAfterCallingGetHandleForDifferentZoom() {
Image icon = Display.getDefault().getSystemImage(SWT.ICON_ERROR);
Expand Down
38 changes: 21 additions & 17 deletions bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/graphics/GC.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -990,7 +990,7 @@ public void drawImage (Image image, int srcX, int srcY, int srcWidth, int srcHei
if (image.isDisposed()) SWT.error(SWT.ERROR_INVALID_ARGUMENT);

int gcZoom = getZoom();
int srcImageZoom = calculateZoomForImage(gcZoom, srcWidth, srcHeight, destWidth, destHeight);
int srcImageZoom = calculateZoomForImage(image, gcZoom, srcWidth, srcHeight, destWidth, destHeight);
drawImage(image, srcX, srcY, srcWidth, srcHeight, destX, destY, destWidth, destHeight, gcZoom, srcImageZoom);
}

Expand All @@ -1003,25 +1003,29 @@ private Collection<Integer> getAllCurrentMonitorZooms() {
return Collections.emptySet();
}

private int calculateZoomForImage(int gcZoom, int srcWidth, int srcHeight, int destWidth, int destHeight) {
private int calculateZoomForImage(Image image, int gcZoom, int srcWidth, int srcHeight, int destWidth, int destHeight) {
int scaledImageZoom;
float imageScaleFactor = 1f * destWidth / srcWidth;
int imageZoom = Math.round(gcZoom * imageScaleFactor);
if (srcWidth == 1 && srcHeight == 1) {
// One pixel images can use the GC zoom
return gcZoom;
}
if (destWidth == srcWidth && destHeight == srcHeight) {
scaledImageZoom = gcZoom;
} else if (destWidth == srcWidth && destHeight == srcHeight) {
// unscaled images can use the GC zoom
return gcZoom;
}

float imageScaleFactor = 1f * destWidth / srcWidth;
int imageZoom = Math.round(gcZoom * imageScaleFactor);
if (getAllCurrentMonitorZooms().contains(imageZoom)) {
return imageZoom;
}
if (imageZoom > 150) {
return 200;
}
return 100;
scaledImageZoom = gcZoom;
} else if (getAllCurrentMonitorZooms().contains(imageZoom)) {
scaledImageZoom = imageZoom;
} else if (imageZoom > 150) {
scaledImageZoom = 200;
} else {
scaledImageZoom = 100;
}
Rectangle scaledBounds = image.getBounds(scaledImageZoom);
Rectangle unScaledBound = image.getBounds();
// validates if the image bounds are scaled up correctly as per required zoom, if it is not zoom is returned as per actual scaling factor.
float scalingFactor = (float) scaledBounds.height / unScaledBound.height;
scaledImageZoom = (int) (scalingFactor * 100);
return scaledImageZoom;
Comment on lines +1026 to +1028
Copy link
Contributor

@HeikoKlare HeikoKlare Jul 24, 2025

Choose a reason for hiding this comment

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

I like the overall approach and I think we could achieve to properly deal with the situation of non-linearly scaled image data. But I think the implementation would need to be a bit more sophisticated. With this approach, since the corrected scaledImageZoom may be an arbitrary value, we would potentially start loading images at arbitrary zooms and thus create more and more unwanted handles. In addition, there is no guarantee that the image data that the image yields for such a zoom have fitting width/height as it depends on which original image data (such as the 100%, 150% or 200% data for PNGs) are used and scaled.
I think what we might actually need to adapt is the srcWidth and srcHeight as they must not be treated as points with an ordinary point-to-pixel conversion based on the zoom if the zoom is not appropriate. However, if you have an image with non-linear image data, you do not even know to which the original point-based srcWidth and srcHeight should fit. We currently assume that it's the 100% version of the image, but it may also be the case that someone creats an ImageDataProvider that always yields image data at the same zoom, which could, for example, also be the current device zoom. If that is not 100%, there might be unexpected results.

I have just tested again the snippet in vi-eclipse/Eclipse-Platform#313 that led to an exception before and found that it now (silently) works. So I wonder whether we should focus on improving the API (adding a note to ImageDataProvider that implementations must return linearly scaled data) and rely on the just added strict check and adaptations of consumers (if necessary) instead of making the logic here more complex.

What do you think, @akoch-yatta @arunjose696?

Copy link
Contributor Author

@arunjose696 arunjose696 Jul 25, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I wonder whether we should focus on improving the API (adding a note to ImageDataProvider that implementations must return linearly scaled data) and rely on the just added strict check and adaptations of consumers (if necessary) instead of making the logic here more complex.

I think the better approach would be to document clearly in ImageDataProvider that implementations must return linearly scaled data. Adding complexity to drawImage to "fix" bad input risks hiding these issues rather than solving them at the actual ( a incorrect ImageDataProvider implementation).

Copy link
Contributor

@HeikoKlare HeikoKlare Jul 25, 2025

Choose a reason for hiding this comment

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

Fully agree. Just for the sake of completeness: the risk is of course that implementations of ImageDataProvider usually have been there for years already, so the contract improvement may stay unnoticed and the implementations will stay somehow incompatible. Still, I think that's the best we can do.
So, concrete proposal: We drop this PR and instead improve the contract of ImageDataProvider. We write a short news as a means to make aware of the potential issue and the contract adaptatino (as actually the assumption that image data is linearly scaled was always there but violations were silently accepted in a more or less resilient way).
@akoch-yatta would then also be good to have your final opinion when you are back before proceeding with that.

Copy link
Contributor

@akoch-yatta akoch-yatta Jul 28, 2025

Choose a reason for hiding this comment

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

I agree. Everything else seem like a workaround, that would only cover up the overall issue, that this contract was (at least implicitly) there from the introduction of HiDPI support, but not enforced.

}

private void drawImage(Image image, int srcX, int srcY, int srcWidth, int srcHeight, int destX, int destY,
Expand Down
Loading