Prepare the implementation of dynamically sized image file-formats

[HannesWell]

Required for

• Rasterization of SVGs at Runtime for Eclipse Icons  #1638

This is a purely internal change that also reduces the number of places where IOExceptions are handled (all int the same way) in FileFormat.isFileFormat() implementations.

@HeikoKlare do you want to review this? Should we aim to land this for this release?
CC @Michael5601

[github-actions]

Test Results

   506 files  ±0     506 suites  ±0   8m 8s ⏱️ +7s
 4 341 tests ±0   4 327 ✅ ±0   14 💤 ±0  0 ❌ ±0 
16 597 runs  ±0  16 488 ✅ ±0  109 💤 ±0  0 ❌ ±0 

Results for commit 27fa1a4. ± Comparison against base commit 2974864.

♻️ This comment has been updated with latest results.

[HeikoKlare]

Thank you for this preparation, @HannesWell!

Overall, the changes look sound to me. I only have minor comments regarding naming/formatting and one comment regarding behavior.

[HeikoKlare]

Should we aim to land this for this release?

For me, it would be fine to bring this into the upcoming release. I do not see any risk in it (give the adaptation I have mentioned in the review) and it would make preparation for the next release cycle in terms of follow-up PRs regarding SVG support easier.

[iloveeclipse]

I would postpone this to 4.36, given that we've just shipped M3, we are in RC phase and the change is not trivial.

[HannesWell]

Should we aim to land this for this release?

For me, it would be fine to bring this into the upcoming release. I do not see any risk in it (give the adaptation I have mentioned in the review) and it would make preparation for the next release cycle in terms of follow-up PRs regarding SVG support easier.

I second that.

I would postpone this to 4.36, given that we've just shipped M3, we are in RC phase and the change is not trivial.

It's right that this is not trivial, but in the end it's just moving around some small parts of the code. And the only really new code-path, i.e. the path in StaticImageFileFormat.loadFromByteStream() if zoom != 100 is currently not called because that method is only called with a zoom of 100.
Therefore I would say that it's not a risky change.

[HeikoKlare]

I also see no risk in the change since, as you explained, it is just some renaming and simple refactoring and new code paths are not executed at all. Still, I understand the concerns, in particular since we are beyond M3 and even though the change is unrisky, it does not look like that when only taking a quick look.

So I am not sure what to do. We should not push this through despite raised concerns. It's not worth to ask PMC for an advise/decision either (and you are part of that anyway 😉 ).

[laeubi]

@HeikoKlare as far as I understand the only reason is "make PRs happen earlier", what I usually do in this situation is:

• Fetch the PR into my fork
• Create a new branch based on this and push it with my PR
• later simply rebase my PR after this is merged

This also will give some more confidence that what is done here is actually the right thing, and as master is effectively locked now there is also no risk of "missing" things while master evolve, so I also think its better to wait (and maybe improve). Especially as many files (10) are affected by this PR.

Once master is open we can merge this right away and maybe even then more PRs.

[HannesWell]

So I am not sure what to do. We should not push this through despite raised concerns. It's not worth to ask PMC for an advise/decision either (and you are part of that anyway 😉 ).

While I agree with you that this change should be save, I think we should postpone it if others would be uncomfortable with having it, as it's not that important. As we had discussed already privately with Michael and as Christoph has described it the other PRs can include this PR's change temporarily.

[HeikoKlare]

Sure, I am completely fine with everything said. Just one remark on:

as far as I understand the only reason is "make PRs happen earlier"

For me, that's not what it is about for me. It is rather two other reasons:

1. Higher effort for creating and reviewing changes on top of another: I know how to do it (technically), we do that all the time during freeze, but still it produces additional effort.
2. Fine-grained feedback: the more changes we postpone to after the freeze period, the more is merged together in a short period of time making it more difficult to identify what has possibility cause regressions (or the other way round: which changes worked fine, e.g., in I-Builds) -- rapid feedback, small increments, ...

That does not outweigh accepting any risk, so don't take this as an argumentation for merging. It's just to explain why for me it's not only about getting things into a specific release but about efficiently using the time we have with (and without) freeze periods :-)

[HannesWell]

For Mac the implementation seems to be completely different than for the other platforms and it even looks to me that anything else than 100% and 200% is not considered?

[HeikoKlare]

From my understanding, MacOS/Cocoa is only capable of 100% and 200% displays and thus everything in the SWT implementation for MacOS is to 100/200 as well.

[HannesWell]

I have now implemented something that seemed suitable just from looking at the code on a Windows computer (no way to try it out), but I'm absolutely not sure if that's reasonable. And I have the impression that code should also be enhanced, because currently it loads the image twice in some cases which might be unnecessary and could be avoided.

In any case it would be good to get support from somebody who has a Mac at hand to try this out and to get some details.

[HeikoKlare]

Unfortunately, MacOS has no on-demand loading for image data at different zooms implemented yet. Thus, whenever possible, two representations of each image are created upon image instantiation. We need to implement this here as well, which is why the changes are generally fine.
I still have enhanced the changes with support for the constructors taking InputStream and String to also render multiple sizes of SVGs in case they refer to an SVG. In addition, I have revised the constructor accepting an ImageFileNameProvider to not unnecessary try to rescale the 200% image but just add a representation for it as is, like done in other constructors as well.

[HannesWell]

We have to discuss when we use the native implementation, which is not capable (currently) to render SVGs for different zooms and when to use the software implementations of the file-format.

At the moment I would say that for SVGs the software solution should be used if fileZoom!=targetZoom and for all other formats the currently used native impl should be used?
Alternatively one could always use the software SVG-renderer for consistency or we could make it configurable by a flag for the case fileZoom==targetZoom?

Deciding in advance if the stream is an SVG or not is possible (the stream is read into a byte-array below anyways) but not trivial.

[HeikoKlare]

I don't have a strong opinion here. I would be in favor the following:

• Use the SVG software solution as default. Why? We then have consistent behavior across the operating systems and will not run into Linux-specific issues because of the native rendering, thus reducing maintenance efforts.
• Make the native implementation (either for fileZoom==targetZoom or even for every zoom) optional. Maybe having a system property for SVG rendering with arguments "native" and "nativeAtDefault" or the like would suffice to capture every desired use case. On the other hand, every valid argument for system properties need to be maintained and considered everywhere necessary, so introducing another maintenance overhead which maybe no one ever needs. So maybe when setting a property to use native SVG support (on Linux), only rendering at 100% is supported to exactly cover the case is supported right now (and other resolutions are done by auto-scaling), unless someone implements anything additionally?

[HannesWell]

* Use the SVG software solution as default. Why? We then have consistent behavior across the operating systems and will not run into Linux-specific issues because of the native rendering, thus reducing maintenance efforts.

Agree.

* Make the native implementation (either for fileZoom==targetZoom or even for every zoom) optional. Maybe having a system property for SVG rendering with arguments "native" and "nativeAtDefault" or the like would suffice to capture every desired use case. On the other hand, every valid argument for system properties need to be maintained and considered everywhere necessary, so introducing another maintenance overhead which maybe no one ever needs. So maybe when setting a property to use native SVG support (on Linux), only rendering at 100% is supported to exactly cover the case is supported right now (and other resolutions are done by auto-scaling), unless someone implements anything additionally?

As far as I know the native implementation is used for performance reasons. And since only SVG is (currently) able to natively scale the image dynamically I think we should continue using the native implementation for all other file formats as is. And only for SVGs we should always render in software, scaling 'natively'.

[HeikoKlare]

I really like the way how it is implemented now. Using the native file format implementations wherever possible and only using the software rendering for SVGs on Linux is fine as implemented in this PR.
I am in favor of doing it like proposed and in case we see need for more configuration options on this, we can still adapt.

[HannesWell]

After a session with Heiko last week I have now updated this PR to further prepare implementation of dynamically sized image file-formats like SVGs in #1638 without the need to add any new API for that.

The basic idea is to apply the concept of the ImageFileNameProvider in a second step: After asking a ImageFileNameProvider for a file for a specific device-zoom, we ask the FileFormat to render the input for that device-zoom even if the file is for a lower zoom. Of course File-Formats are free to ignore that request and just render the image as usual, which also happens for all currently supported format. But the format for SVG can then later 'natively' scale up the image even if the file is for a zoom-factor of 100 (or any other as there is not really one zoom for an SVG).

Besides the Image(Device device, ImageFileNameProvider imageFileNameProvider constructor we have already implemented in that session I have now also implemented the same strategy for the other constructors that load from a 'file' source:

• Image(Device device, String filename)
• Image(Device device, InputStream stream)

For Windows this looks already good to me, but for Mac and Linux I think the implementation is not complete or needs some clarification.

[HeikoKlare]

Info: with the latest push, I have rebased the PR on top of master, in particular including an update of the ImageLoader implementation as that has been refactored into ImageLoader and InternalImageLoader on master.

[HeikoKlare]

Info: with the recent pushed I haved rebased the change onto preparation work in #1886 and restructured the commits to the following two:

1. Adapt FileFormat implementation to accept file and target zooms and be prepared for dynamic format types (SVGs), adapt InternalImageLoader accordingly and make ImageLoader fit to the new internal API
2. Adapt ImageLoader/ImageDataLoader to accept file and target zoom and make use of the enhanced API in the Image implementations.

The former would completely works without the latter.

[HeikoKlare]

I have extracted the second commit out of this PR to keep this preparatory work separated from the adoption in the OS-specific Image classes and merge them one after another. Follow-up PR is:

• Adopt usage of zoom-aware ImageLoader methods in Image class #1889