Add support loading images with a size hint #2656
Conversation
Currently when loading a SVG image it always uses the size declared in the document but storing an SVG at the size to load the image is not really sufficient as it for example require to store the same SVG multiple times if one wants to load it at different sizes. This now adds a new parameter to the NativeImageLoader that allows to pass a sizeHintSupplier that when we get a dynamic image will load it at the supplied size. This can then later be used in JFace to retrive the target size from the URL.
Test Results 108 files - 7 108 suites - 7 13m 11s ⏱️ +57s For more details on these errors, see this check. Results for commit 1a98d48. ± Comparison against base commit cf6ee39. This pull request removes 56 tests. |
HeikoKlare
left a comment
HeikoKlare
left a comment
There was a problem hiding this comment.
Great to see further interest in adopting sophisticated image loading/SVG features.
But I have to admit that I do not understand the necessity for this change. In particular, the first sentence of the PR description is not true:
Currently when loading a SVG image it always uses the size declared in the document but storing an SVG at the size to load the image is not really sufficient as it for example require to store the same SVG multiple times if one wants to load it at different sizes.
There is already SVGFileFormat#loadFromByteStreamBySize() and NativeImageLodaer#load() accepting a requested size, so it's not clear to me why we need yet other methods that combine zoom and size information. Currently, they are properly separated as I either want to load an image at it's original size (maybe in a zoomed version) or I want to load it at a specific size. In case fallbacks from one to the other are needed, this can (and even is already now) implemented in the Image class (for the case you cannot load/rasterize an image at a specified size).
In my opinion, this PR needs a demonstration of an adoption of the changes to understand which use cases shall be supported. That would allow to assess whether the current proposal fits to those demands or how a fitting solution would need to look like.
|
As said the caller can not know if this is a fixed or dynamic sized image, I have pushed the corresponding Jface changes now here:
|
|
Thank you for sharing the JFace change. That helps to better understand the use case.
We already have No matter which way we go here, is it really intended that this now combines zoom and size information and falls back to zoom if the image is a raster graphics? E.g., when you request an image at 64x64 at 200% zoom, you want it to be loaded at 128x128 pixel if the image is an SVG and you want it at 200% of whatever the original size is if it's a raster image? At least that's how I understand the current implementation. But maybe I don't get the full picture yet. I am currently on mobile and will try to have another look next week when I am back in the office. |
|
@HeikoKlare I mostly tried to get it working as I don't understand the full zoom handling here that somehow uses two zoom factors. The bottom line is: one somehow needs to "hint" the SVG loader that even the SVG document uses e.g. 1024x1024 pixel as its "native" size it should be rendered as a 16x16px image and by how we use images I can't use any (mostly internal) images and of course zooming should work as well. So yes it is intentional that it does not applies to raster graphics (what usually it is good to store them at the target size to safe space and have maybe a low resolution variant. So given I have a hint of 16x16px and a current zoom of 200% the result should be an image of 32x32 pixel as if I would have been using two static images that are 16x16px and a |
|
I am still concerned that the proposal mixes up too many things, in particular in such a basic thing as FileFormat/ImageLoader. Here are some thoughts:
Even if we agreed that the current proposal implements expected behavior, I am still convinced that this should not and does not need to be embedded into FileFormat/ImageLoader. For a request of an image with specified size (and zoom), you can check via FileFormat if the image dynamically sizable and in that case just request the image data at the size (altered by zoom). If the image is not dynamically sizable, you can still just request the image for the zoom (which, as said above, does not sound reasonable to me) or deal with the situation in a different way (such as throwin in error). In any case, this should be completely possible on consumer side (i.e., in JFace). |
Because then JFace would need to know about zoom changes. I would ask this the other way round: Why does (currently) the ImageFormat at all must know the zoom at all if it all can be done by the caller? In anyway I would be happy if it would be the case, but wanted to note what was written here, that actually what one want is to override the native size (that is unknown to the caller of the function!) if possible, so everything should just "work as before", so if before a zoom level would be required I don't see why it should now be obsolete because then it would be obsolete in the first place.
That's why I explicitly made this as a hint to the provider. A classical raster image would always be loaded at its native size and I don't want to change this (by intention) but for example an ICO format or XPM both support different sizes in the same file as well so possibly could be using the hint as well even though not really broadly used in SWT anyways.
As said above, I don't aim at a "target size" at all that why I don't think the feature to raster an image at a given size is suitable. I just want it to tell (a hint) about the initial size at 100% so it can scale this on whatever demands for a zoomed image is given there as if the author of a SVG would have shrink the image at the first place.
That's for sure, but it was designed that way before and I dindt aimed to change this in general
The main problem is that even if I could I would completely need to redo what SWT does (and I think not everything is public API) because I literally can't know:
unless I try to load it, so the whole code of content-type detection and image file format selection has to be duplicated at JFace.
That's why I created the PRs so we can play around with that, I think the general idea (extract the size from the URL) is there so if you can came up with a JFace only solution that would be great and maybe help in further refine the proposal either in one way or the other. |
Maybe I still do not fully understand the goal yet. Wouldn't something like this work as a solution to eclipse-platform/eclipse.platform.ui#3431? (sure, that's not clean yet ...) private static ImageData loadImageFromStream(InputStream stream, int fileZoom, int targetZoom,
Supplier<Point> hintProvider) {
Point hintSize = hintProvider.get();
if (hintSize != null) {
byte[] storedStream;
try {
storedStream = stream.readAllBytes();
} catch (IOException e) {
throw new RuntimeException(e);
}
if (FileFormat.isDynamicallySizableFormat(new ByteArrayInputStream(storedStream))) {
return NativeImageLoader.load(new ByteArrayInputStream(storedStream), new ImageLoader(),
hintSize.x * targetZoom / fileZoom, hintSize.y * targetZoom / fileZoom);
}
}
return NativeImageLoader.load(new ElementAtZoom<>(stream, fileZoom), new ImageLoader(), targetZoom).get(0)
.element();
}
The JFace implementations for the image stuff knows about zooms already now and has reimplemented parts of what SWT already does. I know you say:
But when you pass the requested default size and the zoom to an image to be rasterized, how is that different from specifying a target size (which by definition is
As said in my previous comment, that's something that raises concerns as a potential API user. You expect the user to know that loading |
You are right that JFace uses already internal API, I have not noticed this yet and this is actually worse enough, as JFace imports SWT with Beside that this has some SWT implementation inherent problems:
Given that quite convoluted process it seems to code against a very specific internal implementation details, what make the situation even worse. That also was the reason why I initially though we should hand it down to SWT, so the loader can make sure to take a good decision instead of trying to do the same check (again) multiple times on multiple levels.
I must confess I don't understand the example, as no one will use it that way. A more consistent example would be I have The query parameter is more for the case that I previously have
We currently (and before) have expected that users understand they can save their files with |
|
It's true that having JFace uses SWT (internals) is not a good solution. I think the intention back then was to finally provide appropriate API in SWT once the relevant functionality for supporting SVGs and newly arising use cases are clear. The version range issue is something that was probably not taken into account. The essential problem is that it would be best if SWT would/could properly encapsulate image loading capabilities and just provide an appropriate API for image access (via the
I understand that use case, but still
Why do you think so? Why should no one want to use
The difference is that the |
If this ever will be not a (hypothetical) problem, then SWT could still return a scaled up version of that image. I just doubt it will ever be used that way.
But it in no way guarantees that I get the file at this zoom factor! I can use a 16x16px So for me this is a feature that builds a way for user make things better then before by a convention without any risk. It does not prevent users from getting wrong results if they put in wrong data and I would exspect such thing to be noticed on the very first start of their application. |
Not without fundamentally changing how it works: JFace bypasses the SWT
Yes, the user has to follow a contract (
Not sure what exactly you are referring to here, but at least the provision of correctly sized I have already given a proposal few comments above (#2656 (comment)) how the desired functionality could probably be implemented at consumer side (without judging if that technically good or not). I don't see any response on whether that would be correct (to understand if we are talking about the same goals at least). Please also note that the code even reflects the proposed inconsistent handling of the |
That's why I designed this as "best effort", but you broaden the scope to something that uses might want to reuse that as a generic rescaling capability what was never the goal. So yes given I would use it with something that is not scalable (be it be a restriction of JFace, SWT or the Image format) it would not work, and that's why it is a hint for the cases where I use it because it works.
You said as a user you would expect that non scalable images are scaled, so I assume you implied some kind of API/contract where no such thing exits or is intended.
I already wrote that this will probably work to the cost of doing the same checks two times. But I don't think it is godd because of the same reason you mentioned that is
So that's why I initially proposed to pass that information down to SWT so it can make use of any "useful stuff such as scaling capabilities" instead of bypass SWT even more, but the whole discussion now focus on the JFace part (that is how we get it from the URL), while initially you wanted the discussion to continue here (where it would not matter how the data is acquired at all). So I'm not sure if you just don't like the feature at all, don't think its useful for SWT to leverage that information or have something completely different in mind. |
|
Let's take a step back before this gets more offensive and clarify some things.
I don't have anything against that feature. Quite the contrary, it would of course be great to make more out of the capabilities coming in with SVGs. I even made concrete proposals for how to proceed, but the discussion seems to focus on something different. So to be clear: I do not want to block this but would like to see this move forward instead. But yes, I also raised concerns about the proposed API / query parameter and I agree that it took more room than it should here. So I would like to clarify that I still have those concerns but I would not block any progress because of them. I think what concerns me most about that is the attitude of effectively adding new API that only works for specific use cases that the user has to know about and not allowing to be the least bit concerned about that. We can still agree that it's fine to only support specific use cases (for now) and that we have to properly make users aware of that in some way, but why can't we properly discuss that with potential concrete proposals how to best deal with that and instead have to discuss inappropriate comparisons to argue that any concern is invalid? We had quite some discussions and revisions on the initial extensions for SVG support as well and threw away multiple proposals, and it was definitely worth it. So to summarize my concrete concern on the API: When introducing a kind of public API that allows to specific a size for an image to load (via a URL query parameter), I am concerned about doing it in way in which, as a consumer, you need to know about the specific cases in which that will work properly, as those cases that are not supported are not obvious. In particular, a user cannot know that a query parameter called The more relevant concern I have completely got out of focus and is about how the desired feature can be realized:
I actually made a concrete proposal to address the demand as "best effort" in #2656 (comment). And to be precise here as well: I did not propose to implement a generic rescaling capability, I just mentioned what users will expect when you allow them to request an image at a specific size. I think you can even simply that proposal to: private static ImageData loadImageFromStream(InputStream stream, int fileZoom, int targetZoom,
Supplier<Point> hintProvider) {
Point hintSize = hintProvider.get();
if (hintSize != null) {
return NativeImageLoader.load(stream, new ImageLoader(),
hintSize.x * targetZoom / fileZoom, hintSize.y * targetZoom / fileZoom);
}
return NativeImageLoader.load(new ElementAtZoom<>(stream, fileZoom), new ImageLoader(), targetZoom).get(0)
.element();
}To summarize some of my statements: the required functionality is nothing that, in my opinion, should be part of the
Since the second scenario can completely be realized at consumer side, there should be no need to provide minimal low-level API for it. One might discuss what is considered "too low level" for it, i.e., at which level reusable/convenience API is appropriate (e.g., would it be fine to have that at
The discussion is going on here because this PR is currently nothing to be used on its own but is basically just a prerequisites for the JFace PR. And having two discussions at different places on the same topic is not very helpful, so since we started here I wanted to avoid that we discuss the same things in the JFace PR again. |
2 participants
Currently when loading a SVG image it always uses the size declared in the document but storing an SVG at the size to load the image is not really sufficient as it for example require to store the same SVG multiple times if one wants to load it at different sizes.
This now adds a new parameter to the NativeImageLoader that allows to pass a sizeHintSupplier that when we get a dynamic image will load it at the supplied size. This can then later be used in JFace to retrive the target size from the URL.
@HeikoKlare can you take a look if it makes sense to you? I'm not completely sure about
fileZoomandtargetZoomhere...