diff --git a/bundles/org.eclipse.swt/Eclipse SWT/cocoa/org/eclipse/swt/graphics/Image.java b/bundles/org.eclipse.swt/Eclipse SWT/cocoa/org/eclipse/swt/graphics/Image.java index dbf1b852f5c..b7d01d158cc 100644 --- a/bundles/org.eclipse.swt/Eclipse SWT/cocoa/org/eclipse/swt/graphics/Image.java +++ b/bundles/org.eclipse.swt/Eclipse SWT/cocoa/org/eclipse/swt/graphics/Image.java @@ -1909,6 +1909,8 @@ void executeOnImageAtSizeBestFittingSize(Consumer imageAtBestFittingSizeC private class CachedImageAtSize { private Image image; + /** File already found not to be dynamically sizable, so re-reading it cannot help. */ + private String nonSizableFileName; public void destroy() { if (image != null) { @@ -1959,10 +1961,17 @@ private Optional loadImageDataAtExactSize(int targetWidth, int target } if (imageFileNameProvider != null) { String fileName = DPIUtil.validateAndGetImagePathAtZoom(imageFileNameProvider, 100).element(); - if (ImageDataLoader.isDynamicallySizable(fileName)) { - ImageData imageDataAtSize = ImageDataLoader.loadBySize(fileName, targetWidth, targetHeight); - return Optional.of(imageDataAtSize); + if (fileName.equals(nonSizableFileName)) { + return Optional.empty(); } + ImageDataLoader.AtSizeLoadResult imageDataAtSize = ImageDataLoader + .loadBySizeIfDynamicallySizable(fileName, targetWidth, targetHeight); + if (imageDataAtSize.staticFormat()) { + nonSizableFileName = fileName; + } else if (imageDataAtSize.data().isPresent()) { + nonSizableFileName = null; + } + return imageDataAtSize.data(); } return Optional.empty(); } diff --git a/bundles/org.eclipse.swt/Eclipse SWT/common/org/eclipse/swt/graphics/ImageDataLoader.java b/bundles/org.eclipse.swt/Eclipse SWT/common/org/eclipse/swt/graphics/ImageDataLoader.java index 5f3bbc118f8..5656ea1ca86 100644 --- a/bundles/org.eclipse.swt/Eclipse SWT/common/org/eclipse/swt/graphics/ImageDataLoader.java +++ b/bundles/org.eclipse.swt/Eclipse SWT/common/org/eclipse/swt/graphics/ImageDataLoader.java @@ -45,10 +45,6 @@ public static boolean canLoadAtZoom(String filename, int fileZoom, int targetZoo return ImageLoader.canLoadAtZoom(filename, fileZoom, targetZoom); } - static boolean isDynamicallySizable(String filename) { - return ImageLoader.isDynamicallySizable(filename); - } - static boolean isDynamicallySizable(InputStream stream) { return ImageLoader.isDynamicallySizable(stream); } @@ -71,10 +67,19 @@ public static ImageData loadBySize(InputStream stream, int width, int height) { return data; } - public static ImageData loadBySize(String filename, int width, int height) { - ImageData data = new ImageLoader().loadBySize(filename, width, height); - if (data == null) SWT.error(SWT.ERROR_INVALID_IMAGE); - return data; + public static AtSizeLoadResult loadBySizeIfDynamicallySizable(String filename, int width, int height) { + return new ImageLoader().loadBySizeIfDynamicallySizable(filename, width, height); + } + + /** + * Outcome of an at-size load attempt. {@code staticFormat} is only set when the file was + * positively identified as a format that cannot be loaded at an arbitrary size, so that a + * format which merely could not be determined is not remembered as non-sizable. + */ + record AtSizeLoadResult(Optional data, boolean staticFormat) { + + static final AtSizeLoadResult STATIC_FORMAT = new AtSizeLoadResult(Optional.empty(), true); + static final AtSizeLoadResult UNKNOWN_FORMAT = new AtSizeLoadResult(Optional.empty(), false); } } diff --git a/bundles/org.eclipse.swt/Eclipse SWT/common/org/eclipse/swt/graphics/ImageLoader.java b/bundles/org.eclipse.swt/Eclipse SWT/common/org/eclipse/swt/graphics/ImageLoader.java index 66e8e294f44..289e35499a2 100644 --- a/bundles/org.eclipse.swt/Eclipse SWT/common/org/eclipse/swt/graphics/ImageLoader.java +++ b/bundles/org.eclipse.swt/Eclipse SWT/common/org/eclipse/swt/graphics/ImageLoader.java @@ -55,6 +55,9 @@ */ public class ImageLoader { + /** Upper bound on the bytes a format signature check may consume before the stream is rewound. */ + private static final int SIGNATURE_LOOKAHEAD_LIMIT = 8192; + /** * the array of ImageData objects in this ImageLoader. * This array is read in when the load method is called, @@ -209,16 +212,6 @@ List> loadByZoom(String filename, int fileZoom, int tar return null; } -ImageData loadBySize(String filename, int width, int height) { - if (filename == null) SWT.error(SWT.ERROR_NULL_ARGUMENT); - try (InputStream stream = new FileInputStream(filename)) { - return loadBySize(stream, width, height); - } catch (IOException e) { - SWT.error(SWT.ERROR_IO, e); - } - return null; -} - static boolean canLoadAtZoom(String filename, int fileZoom, int targetZoom) { if (filename == null) SWT.error(SWT.ERROR_NULL_ARGUMENT); try (InputStream stream = new FileInputStream(filename)) { @@ -229,13 +222,29 @@ static boolean canLoadAtZoom(String filename, int fileZoom, int targetZoom) { return false; } -static boolean isDynamicallySizable(String filename) { - try (InputStream stream = new FileInputStream(filename)) { - return FileFormat.isDynamicallySizableFormat(stream); +/** + * Loads the image at the given size if the file is a dynamically sizable format, + * reading the file only once for both the format check and the load. + */ +ImageDataLoader.AtSizeLoadResult loadBySizeIfDynamicallySizable(String filename, int width, int height) { + if (filename == null) SWT.error(SWT.ERROR_NULL_ARGUMENT); + try (InputStream stream = new BufferedInputStream(new FileInputStream(filename))) { + stream.mark(SIGNATURE_LOOKAHEAD_LIMIT); + Optional sizable = FileFormat.isDynamicallySizableFormatIfKnown(stream); + if (sizable.isEmpty()) { + return ImageDataLoader.AtSizeLoadResult.UNKNOWN_FORMAT; + } + if (!sizable.get()) { + return ImageDataLoader.AtSizeLoadResult.STATIC_FORMAT; + } + stream.reset(); + ImageData data = loadBySize(stream, width, height); + if (data == null) SWT.error(SWT.ERROR_INVALID_IMAGE); + return new ImageDataLoader.AtSizeLoadResult(Optional.of(data), false); } catch (IOException e) { SWT.error(SWT.ERROR_IO, e); } - return false; + return ImageDataLoader.AtSizeLoadResult.UNKNOWN_FORMAT; } static boolean isDynamicallySizable(InputStream stream) { diff --git a/bundles/org.eclipse.swt/Eclipse SWT/common/org/eclipse/swt/internal/image/FileFormat.java b/bundles/org.eclipse.swt/Eclipse SWT/common/org/eclipse/swt/internal/image/FileFormat.java index dfd5e265199..a0f89f18074 100644 --- a/bundles/org.eclipse.swt/Eclipse SWT/common/org/eclipse/swt/internal/image/FileFormat.java +++ b/bundles/org.eclipse.swt/Eclipse SWT/common/org/eclipse/swt/internal/image/FileFormat.java @@ -73,8 +73,15 @@ private static Optional determineFileFormat(LEDataInputStream stream private static final int MAX_SIGNATURE_BYTES = 18 + 2; // e.g. Win-BMP or OS2-BMP plus a safety-margin public static boolean isDynamicallySizableFormat(InputStream is) { - Optional format = determineFileFormat(new LEDataInputStream(is, MAX_SIGNATURE_BYTES)); - return format.isPresent() && !(format.get() instanceof StaticImageFileFormat); + return isDynamicallySizableFormatIfKnown(is).orElse(Boolean.FALSE); + } + + /** + * @return empty if no format could be identified, so sizability is unknown + */ + public static Optional isDynamicallySizableFormatIfKnown(InputStream is) { + return determineFileFormat(new LEDataInputStream(is, MAX_SIGNATURE_BYTES)) + .map(format -> !(format instanceof StaticImageFileFormat)); } static abstract class StaticImageFileFormat extends FileFormat { diff --git a/bundles/org.eclipse.swt/Eclipse SWT/gtk/org/eclipse/swt/graphics/Image.java b/bundles/org.eclipse.swt/Eclipse SWT/gtk/org/eclipse/swt/graphics/Image.java index 4c8593108c6..030d83db103 100644 --- a/bundles/org.eclipse.swt/Eclipse SWT/gtk/org/eclipse/swt/graphics/Image.java +++ b/bundles/org.eclipse.swt/Eclipse SWT/gtk/org/eclipse/swt/graphics/Image.java @@ -970,6 +970,8 @@ void destroy() { private class CachedImageAtSize { private Image image; + /** File already found not to be dynamically sizable, so re-reading it cannot help. */ + private String nonSizableFileName; public void destroy() { if (image != null) { @@ -1020,10 +1022,17 @@ private Optional loadImageDataAtExactSize(int targetWidth, int target } if (imageFileNameProvider != null) { String fileName = DPIUtil.validateAndGetImagePathAtZoom(imageFileNameProvider, 100).element(); - if (ImageDataLoader.isDynamicallySizable(fileName)) { - ImageData imageDataAtSize = ImageDataLoader.loadBySize(fileName, targetWidth, targetHeight); - return Optional.of(imageDataAtSize); + if (fileName.equals(nonSizableFileName)) { + return Optional.empty(); } + ImageDataLoader.AtSizeLoadResult imageDataAtSize = ImageDataLoader + .loadBySizeIfDynamicallySizable(fileName, targetWidth, targetHeight); + if (imageDataAtSize.staticFormat()) { + nonSizableFileName = fileName; + } else if (imageDataAtSize.data().isPresent()) { + nonSizableFileName = null; + } + return imageDataAtSize.data(); } return Optional.empty(); } 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 098efadcb9d..f14904a12ff 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 @@ -2619,6 +2619,9 @@ protected Rectangle getBounds(int zoom) { } private class ImageFileNameProviderWrapper extends BaseImageProviderWrapper { + /** File already found not to be dynamically sizable, so re-reading it cannot help. */ + private String nonSizableFileName; + ImageFileNameProviderWrapper(ImageFileNameProvider provider) { super(provider, ImageFileNameProvider.class); // Checks for the contract of the passed provider require @@ -2871,12 +2874,17 @@ private long extractHandleForPixelFormat(int width, int height, int pixelFormat) @Override protected Optional loadImageDataAtExactSize(int targetWidth, int targetHeight) { String fileName = DPIUtil.validateAndGetImagePathAtZoom(this.provider, 100).element(); - if (ImageDataLoader.isDynamicallySizable(fileName)) { - ImageData imageDataAtSize = ImageDataLoader.loadBySize(fileName, targetWidth, targetHeight); - ImageData adaptedImageDataAtSize = adaptImageDataIfDisabledOrGray(imageDataAtSize); - return Optional.of(adaptedImageDataAtSize); + if (fileName.equals(nonSizableFileName)) { + return Optional.empty(); } - return Optional.empty(); + ImageDataLoader.AtSizeLoadResult imageDataAtSize = ImageDataLoader + .loadBySizeIfDynamicallySizable(fileName, targetWidth, targetHeight); + if (imageDataAtSize.staticFormat()) { + nonSizableFileName = fileName; + } else if (imageDataAtSize.data().isPresent()) { + nonSizableFileName = null; + } + return imageDataAtSize.data().map(Image.this::adaptImageDataIfDisabledOrGray); } } 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 020dd94af01..a53c943ee5c 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 @@ -34,6 +34,7 @@ import java.nio.file.Path; import java.util.List; import java.util.concurrent.atomic.AtomicInteger; +import java.util.concurrent.atomic.AtomicReference; import java.util.function.Consumer; import org.eclipse.swt.SWT; @@ -1224,5 +1225,67 @@ public void test_gcOnImageGcDrawer_imageDataAtNonDeviceZoom() { } } +/** + * Whether a file can be loaded at an arbitrary size never changes, so the scaled + * drawImage must determine it once instead of re-reading the file on every draw. + * Deleting the file after the first draw makes any further read fail loudly. + */ +@Test +public void test_drawImageAtSize_doesNotReReadFileOfNonSizableFormat() throws IOException { + Path file = tempFolder.resolve("volatile-collapseall.png"); + Files.copy(Path.of(getPath("collapseall.png")), file); + + Image image = new Image(display, file.toString()); + Image target = new Image(display, 64, 64); + GC gc = new GC(target); + try { + Rectangle bounds = image.getBounds(); + gc.drawImage(image, 0, 0, bounds.width * 2, bounds.height * 2); + + Files.delete(file); + + gc.drawImage(image, 0, 0, bounds.width * 2, bounds.height * 2); + gc.drawImage(image, 0, 0, bounds.width * 3, bounds.height * 3); + } finally { + gc.dispose(); + target.dispose(); + image.dispose(); + Files.deleteIfExists(file); + } +} + +/** + * An Image may be backed by different files over its lifetime, so what is known about + * one file must not be applied to the next one. + */ +@Test +public void test_drawImageAtSize_reevaluatesSizabilityWhenFileNameChanges() throws IOException { + Path sizableFile = tempFolder.resolve("switchable-collapseall.svg"); + Files.copy(Path.of(getPath("collapseall.svg")), sizableFile); + AtomicReference currentFile = new AtomicReference<>(getPath("collapseall.png")); + ImageFileNameProvider switchingProvider = zoom -> zoom == 100 ? currentFile.get() : null; + + Image image = new Image(display, switchingProvider); + Image target = new Image(display, 64, 64); + GC gc = new GC(target); + try { + // learns that the PNG cannot be loaded at an arbitrary size + gc.drawImage(image, 0, 0, 20, 20); + + currentFile.set(sizableFile.toString()); + gc.drawImage(image, 0, 0, 20, 20); + + // if the SVG were still treated as non-sizable, it would never be read at all + Files.delete(sizableFile); + assertThrows(SWTException.class, () -> gc.drawImage(image, 0, 0, 24, 24), + "the file of a sizable image must be read again for a new size"); + } finally { + gc.dispose(); + target.dispose(); + image.dispose(); + Files.deleteIfExists(sizableFile); + } +} + }