Feature Proposal: Rasterization of SVGs at Runtime for Eclipse Icons

[Michael5601]

See the following PR in Eclipse SWT for a detailed description of the changes.

Fixes eclipse-platform/eclipse.platform.swt#1438.

[HannesWell]

I just pushed the changes I made for/in our yesterdays 1:1 meeting, mainly adapt to the change that o.e.swt.svg is a fragment.

[github-actions]

Test Results

 1 824 files  ±0   1 824 suites  ±0   1h 40m 9s ⏱️ + 8m 17s
 7 918 tests ±0   7 690 ✅ ±0  228 💤 ±0  0 ❌ ±0 
23 841 runs  ±0  23 093 ✅ ±0  748 💤 ±0  0 ❌ ±0 

Results for commit 6c37f04. ± Comparison against base commit 037c5d7.

♻️ This comment has been updated with latest results.

[Michael5601]

I removed the added SVGs from this PR. Please see the new PR for the SVGs if you want to test this feature.

In the meantime I locally changed all PNGs I could find in Platform UI, Platform, JDT UI and SWT for testing the Performance. I had no problems whatsoever when starting Eclipse and it was also quite easy and fast to add the SVGs and change the paths. I did not delete any PNGs in this process but changed ".png" to ".svg" in most places.

I will report the performance data soon.

[HeikoKlare]

If I am not mistaken, I am afraid that the proposed change is a no-op.

The purpose of the PR is to make the providers capable of processing files that can be used for retrieving images in a different zoom than what is currently provided by patterns for raster graphics files (like @2x, 32x32 and so on).
However, all used methods just return an element when the requested zoom exactly matches a file according to the above mentioned patterns. This means, the introduced SourceAtZoom will only contain an element if the zoom of that element fits to the zoom that was passed to the method returning it. So the zoom information in SourceAtZoom is not needed, as it's implicitly given by the contained element not being null anyway.

What really needs to be changed is that when, e.g., requesting image data for a path not following any of the patterns (i.e., currently being treated as a file of 100% zoom) and passing a zoom of, e.g., 175%, it should still be possible to retrieve data in that zoom from that file if the file access itself is capable of processing the zoom (as is available for SVGs). This does currently not happen.

[Michael5601]

If I am not mistaken, I am afraid that the proposed change is a no-op.

The purpose of the PR is to make the providers capable of processing files that can be used for retrieving images in a different zoom than what is currently provided by patterns for raster graphics files (like @2x, 32x32 and so on). However, all used methods just return an element when the requested zoom exactly matches a file according to the above mentioned patterns. This means, the introduced SourceAtZoom will only contain an element if the zoom of that element fits to the zoom that was passed to the method returning it. So the zoom information in SourceAtZoom is not needed, as it's implicitly given by the contained element not being null anyway.

Is this only the case for the FileImageDescriptor? For the URLImageDescriptor the change in this PR should allow rasterization of SVGs in arbitrary sizes. I did not test it for the FileImageDescriptor yet and the URLImageDescriptor was also only called with the value 100 in my manual testing as the composite images were always generated with this value and then scaled afterwards.

[HeikoKlare]

The URLImageDescriptor is (also) affected. When having a descriptor for an SVG, none of the xpath/xname matches will apply. And if none of them matches, the original source will only be used if zoom is 100. I.e., you can only retrieve the image data at zoom 100 for an SVG. Maybe I miss something, but that's how I understand the current code.

Consider for example the CompositeImageDescriptor implementation:

eclipse.platform.ui/bundles/org.eclipse.jface/src/org/eclipse/jface/resource/CompositeImageDescriptor.java

Lines 134 to 144 in b6af990

	ImageData zoomed = descriptor.getImageData(zoom);
	if (zoomed != null) {
	cached = zoomed;
	cachedZoom = zoom;
	return zoomed;
	}
	if (zoom == 100) {
	return ImageDescriptor.getMissingImageDescriptor().getImageData(100);
	}
	ImageData data100 = descriptor.getImageData(100);

The first call will yield null if zoom is not 100, so as a fallback the 100% image data is retrieved later on.

[Michael5601]

The URLImageDescriptor is (also) affected. When having a descriptor for an SVG, none of the xpath/xname matches will apply. And if none of them matches, the original source will only be used if zoom is 100. I.e., you can only retrieve the image data at zoom 100 for an SVG. Maybe I miss something, but that's how I understand the current code.

Consider for example the CompositeImageDescriptor implementation:

eclipse.platform.ui/bundles/org.eclipse.jface/src/org/eclipse/jface/resource/CompositeImageDescriptor.java

Lines 134 to 144 in b6af990

	ImageData zoomed = descriptor.getImageData(zoom);
	if (zoomed != null) {
	cached = zoomed;
	cachedZoom = zoom;
	return zoomed;
	}
	if (zoom == 100) {
	return ImageDescriptor.getMissingImageDescriptor().getImageData(100);
	}
	ImageData data100 = descriptor.getImageData(100);

The first call will yield null if zoom is not 100, so as a fallback the 100% image data is retrieved later on.

The discussed problems should be fixed by commit 6a23f97. I utilized the new FileFormat.canLoadAtZoom() API from this change for the FileImageDescriptor and URLImageDescriptor. It is now checked in advance whether loading an image for a specific zoom level is possible. If so, the corresponding path without "e.g. @2x" notation is returned or the relevant SVG data is directly loaded at that zoom level.

[HannesWell]

After a reviewing session Michael and I agreed that the the canLoad() check should not be present for the zoom == 100 case in the ImageFileNameProviders of both descriptor types as they always have to return the path for exactly the requested file-zoom.
The SWT Image class subsequently then can scale the image, depending on the type of Image (i.e. static vs. dynamically sizeable), while rasterizing it with a matching factor that reflects the relation of target-zoom to file-zoom. That doesn't work for SVGs if the filenameProvider returns the default (100%) file if the zoom is e.g. 150.

The ImageDataProvider is different as it returns an ImageData object that represents the image as it will be finally, without any (lossless) post-processing.
So while the ImageDataProvider returns an exact representation of an image for the requested zoom, the ImageFilenameProvider returns only a suitable source of an image (i.e. the file) at the requested zoom. But since the latter does not allow to supply any information about the 'native-zoom' of a file, the returned file must exactly match the expectation of the caller, i.e. the specified zoom or otherwise the subsequent rasterization of an SVG will be conducted with the wrong scaling factor. Therefore the ImageFilenameProvider implementations must not be smart and must just behave as before.

However, all used methods just return an element when the requested zoom exactly matches a file according to the above mentioned patterns. This means, the introduced SourceAtZoom will only contain an element if the zoom of that element fits to the zoom that was passed to the method returning it. So the zoom information in SourceAtZoom is not needed, as it's implicitly given by the contained element not being null anyway.

That's actually right and I also checked this and step-by-step removed the SourceAtZoom and it showed that the passed zoom is actually not needed. At least except for the zoom == 100 case in the FileImgeDescriptor.getStream(int) method, but that could have been resolved by refactoring the code a bit and adapting it more to the URLImageDescriptor class.
With that it was possible to avoid the SourceAtZoom entirely.
Then only changes to load ImageData directly where necessary, which also reflects the findings described above that the ImageFilenameProviders should be unchanged.

[HannesWell]

I think this should have been the specified zoom. E.g. if I pass a path that contains a /16x16/ folder and a zoom of 200, the resulting desiredHeight/Width will be 32 and that's not the zoom of the result.

Suggested change

	return new SourceAtZoom<>(xPath, desiredHeight);
	return new SourceAtZoom<>(xPath, zoom);

Requested changes have been applied.

[HannesWell]

When the tests pass, I think this is ready for submission.

[Michael5601]

Thank you for the session @HannesWell. It was really insightful for me. :)

[Michael5601]

@HannesWell The tests passed. I also tested it manually again and it works perfectly. Would be great if we can submit.

For everyone interested in trying this out:
This PR allows consumers outside of SWT to use SVGs. Essentially this can be seen by the composite icons in the eclipse workspace.
To see changes the autoScaling-flag needs to be set to quarter or exact and one method needs to be adjusted in the class CompositeImageDescriptor as it works like a local implementation of the autoScaling-flag.
The method is supportsZoomLevel(int) and it must always return true to allow loading images for all zoom levels.
We will fix this behaviour in a follow-up, which was already discussed here.

[HannesWell]

The tests passed. I also tested it manually again and it works perfectly. Would be great if we can submit.

Great. Thanks for testing again!
I just fixed a minor flaw, where we didn't use the created BufferedInputStream but the originating stream directly. I also rearranged a few lines to have less changes in this PR.
Once the build succeeds I plan to submit this. Tomorrows I-build should then have this change available.

Thanks for all your awesome work on this topic!