Move ImageData Scaling Impl to Image for all platforms #1933
Conversation
| * | ||
| * @since 3.130 | ||
| */ | ||
| public ImageData scaleToUsingSmoothScaling(Device device, int width, int height) { |
There was a problem hiding this comment.
What about completely integrating the functionality into ImageData, i.e., having this also applied when calling ImageData#scaledTo()? That's missing anyway (no one outside SWT can retrieve a smooth-scaled image as the functionality is internal to DPIUtil).
There was a problem hiding this comment.
I need to pass device to scaleToUsingSmoothScaling and internalizing it to scaledTo method means we need to do some API change because ImageData has no device context. Any suggestions?
There was a problem hiding this comment.
Any suggestions?
Anything that needs a device does not belong to ImageData, this class is explicitly for carry device independent data.
As the method creates an image internally anyways, why not put such functionality into the Image class?
There was a problem hiding this comment.
The same has been done in the previous PR: #1913
But it replicates the code from DPIUtil a little bit.
| public ImageData scaleToUsingSmoothScaling(Device device, int width, int height) { | ||
| Image original = new Image (device, (ImageDataProvider) zoom -> this); | ||
| /* Create a 24 bit image data with alpha channel */ | ||
| final ImageData resultData = new ImageData (width, height, 24, new PaletteData (0xFF, 0xFF00, 0xFF0000)); |
There was a problem hiding this comment.
What about other bit depth?
There was a problem hiding this comment.
ImageData is a part of common so it is indeed available for all the paltform. This PR just moves the whole smooth scaling block from DPIUtil to ImageData to make it able to call the GC.drawImage which takes pixels as an input so that we can avoid scaling up and scaling down leading to rounding error.
As per the previous implementation, it is always using 24 bits hence I didn't modify anything in the logic. The sole objective of this PR is to get rid of the rounding error through this refactoring.
| * | ||
| * @since 3.130 | ||
| */ | ||
| public ImageData scaleToUsingSmoothScaling(Device device, int width, int height) { |
There was a problem hiding this comment.
Assuming we have such method in Image class
| public ImageData scaleToUsingSmoothScaling(Device device, int width, int height) { | |
| public Image scaleTo(int width, int height) { |
- it could be implemented on all OS
- would not need to care about creating a new image or what device to use
- caller can decide if the image is to be used directly, or
ImageDatais to be extracted
This might also be useful outside of DPI use-case.
There was a problem hiding this comment.
I think there's been a misunderstanding. Initially there was no method called scaleToUsingSmoothScaling in Image. It was added via a previous PR last week: #1913
Following the merge, @HeikoKlare mentioned that it doesn't seem like a good design since we replicate the code and then the idea was to move the whole imagedata scaling block from DPIUtil to ImageData class itself and hence this PR reverts that commit and moves the block of code from DPIUtil to ImageData. And since ImageData is in the common package, it seems to be way more cleaner than the previous solution.
There was a problem hiding this comment.
As far as I can see the method is currently private in Image, this one is public in ImageData and it has many problems:
- It does not belong to
ImageData... whenever one wants to producesImageDatait should go through an image or create theImageDatabased on some other source - It seem to only be used/useful for Win32
- It has a bad naming for a public method (might be arguable) and I'm not sure if "smooth scaling" is commonly understood. For a public method I would expect it to be name
scale(...)any maybe accepting some flags that control the technique (e.g Using Antialias and so on)
So my suggestion would be to:
- make a public method in
Image, this also has the advantage that any platform specific optimization might be used, it also removes the need for some code / parameters and the caller does not need to create a new image (again) if it should be used as such. - name it more general
scaleand let it accept some SWT flags, so one can later enhance the method without the need for new API, for now it might only supportSWT.SMOOTHor evenSWT.DEFAULTto let the implementation decide the best algorithm to use.
There was a problem hiding this comment.
Also moving this to Image might not be feasible since Cursor also uses this method in DPIUtil and that doesn't involve any Image. Refer: Cursor:win32_getHandle
There was a problem hiding this comment.
Also moving this to Image might not be feasible since Cursor also uses this method in DPIUtil and that doesn't involve any Image
DPI Util can of course call code in Image if required, the code creates an image internally anyways so I do not see any drawbacks here.
There was a problem hiding this comment.
That's why I think the most suitable place for "good" scaling would be the
Imageclass ;-)Adding something to
ImageDatajust because there is one consumer using it does not seem a good reason.
My point was obviously a bit inconclusive: the reason why all consumers use Image to scale ImageData is not because that's how to do that but it's because ImageData does not provide a proper way of doing so. Note that what the DPIUtil method for auto-scaling image currently does is not to scale an image but to scale ImageData.
I'm not entirely sure if the
scalemethod inImageDatais a good idea at all and the best would probably to deprecate the method. Things like scaling and image are often better done at a dedicated place, as the code for doing so does not need access to any internals that justify it being part ofImageDataitself...
I have to admit that I don't understand that argument. From OO perspective, if I have some data class describing an image and I want to scale that data, why shouldn't that functionality be provided at that data object? Image is an entity wrapping an image representation with all it's metadata, OS-related information, auto-scaling capabilities etc. Everything related to processing those data should actually not be part of that class (but encapsulated in ImageLoader for the loading capabilities and ImageData for the data manipulation). ImageData is the pure data with the ability to scale according to a desired size (i.e., at a lower level of abstraction than the scaling according to zoom used by Image).
I fully agree that in the best case the current algorithm using an intermediate Image to scale the data based on OS capabilities will be replaced by a generic algorithm, but that's nothing that needs to be done now but could be done as a follow-up improvement.
And the scaledTo() method of course uses all kinds of internals of ImageData. It's just that those internals are not really internal as ImageData is a class without proper encapsulation (all fields being public mutable).
Do you mean using Display.getCurrent()?
Exactly.
So concrete proposal:
- Would it be possible to place the implementation in
ImageDatawithout any API changes? In that case I would be in favor of doing that (cc @HannesWell we have also already discussed moving this scaling functionality to ImageData). - If not, we should probably place the functionality inside Image consistently across the operating systems, in the best case factoring out the common part of the implementation in some utility. And we should then adapt relevant consumers (like in JFace, which we need to adapt anyway).
There was a problem hiding this comment.
the reason why all consumers use Image to scale ImageData is not because that's how to do that but it's because ImageData does not provide a proper way of doing so.
As one usually need a Image to do anything useful with an ImageData i don't think it is that people don't want to use an Image#scaleTo method .. there is just no one there :-)
I have to admit that I don't understand that argument. From OO perspective, if I have some data class describing an image and I want to scale that data, why shouldn't that functionality be provided at that data object?
Because scale is obviously not a trivial operation and there are different ways to scale an image. Just look at the wikipedia article that list numerous approaches. So whatever we implement here will be bad for someone, it will break someones expectation if we change it later on (either in terms of performance, behavior, memory consumption, usage of native handles).
Even worse, scaling does not scale the ImageData (like a transformation does) it returns a new instance so there is really no reason to do it at the data object itself as this is more a data-carrier of device independent properties than a real object with mutators. The same argument would hold true to save an image, but we have dedicated ImageLoader (sic!) for saving an ImageData.
If you look at the AWT counterpart BufferedImage it also does not contain such thing like scaling or saving, as all this is better handled externally.
It's just that those internals are not really internal as ImageData is a class without proper encapsulation
ImageData is just a data carrier and the javadoc also mention this. Due to historical and technical reasons SWT do not encapsulate all data well, this has to be best handled on a higher level.
I fully agree that in the best case the current algorithm using an intermediate Image to scale the data based on OS capabilities will be replaced by a generic algorithm, but that's nothing that needs to be done now but could be done as a follow-up improvement.
From my point of view, the "done as a follow-up improvement" is exactly what org.eclipse.swt.graphics.ImageData.scaledTo(int, int) is currently missing.
There was a problem hiding this comment.
As one usually need a
Imageto do anything useful with anImageDatai don't think it is that people don't want to use anImage#scaleTomethod .. there is just no one there :-)
Image#getImageData(zoom) is already there and used for exactly this (and could also be used by the mentioned place in JFace). And unfortunately ImageDescriptors (in particular the composite ones) try to do many things with just ImageData that should better be done with an Image ;-)
One argument we missed so far, which definitely makes scaling a capability that needs to be done by Image, is the fact that Image knows about the image data source and can retrieve an image in an according scale potentially without any need to perform scaling (by just loading it in proper scale, rasterizing an SVG or the like). This is something that cannot be achieved with any implementation of scaling outside Image (be it the ImageData class, some utility or anything else).
Given that, I withdraw my previous argumentation and agree with the solution of placing the functionality in Image: the image can best decide how to provide a scaled version. ImageData#scaledTo may then really be deprecated referring to the proper way of scaling across an Image instance.
Thank you for the discussion leading to this insight.
So I guess we agree that the functionality should be moved to the Image class then?
Regarding the issue with other consumers like Cursor: this rather seems to be a flaw anyway
- With the current API, we may create an Image for the ImageData and use its scaling capabilities. Yes, that is additional effort with an "unnecessary" temporary OS handle, but it then uses the "uniform" way of scaling.
- Initializing cursors with ImageData is a bad idea anyway. The data passed to the cursors is usually taken from some source (like an image descriptor), which could be capable of providing properly scaled images (i.e., based on an according data source like a 100%/200% raster graphics file or an SVG) whereas currently only scaling the given image data (potentially having a bad resolution) is possible.
There was a problem hiding this comment.
Image#getImageData(zoom) is already there and used for exactly this (and could also be used by the mentioned place in JFace).
I just wanted to mention that this method does only allow ratio-preserving scaling but not arbitrary one.
Assume you have a Window with a background image and you grow/shrink the window, then you maybe need to enlarge the one axis by a different factor than the other. Even though it is maybe not that common it is impossible to do so with zoom... but one could do it in two steps of course. Here as well, SVG might offer superior results if rendered directly into a stretched version instead of performing it on the pixel data. so an Image#getImageData(x,y) would still be valuable.
There was a problem hiding this comment.
Yes, that's a current limitation of all Image API: it is specific to auto-scaling (thus only capable of accepting a zoom value) and not capable of scaling to a specific width/height. We also discussed having a basic scaling concept of just passing desired height/width when implementing SVG support, as it would allow to render SVGs in arbitrary required size (like could be useful when zooming in diagrams that contain SVG images). But that's an extension independent of this change.
amartya4256
force-pushed
the
amartya4256/smooth_scaling_rounding_error_fix
branch
8 times, most recently
from
d38a969 to
e315648
Compare
This reverts commit 909a985. # Conflicts: # bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/graphics/Image.java # Conflicts: # bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/graphics/Image.java
amartya4256
force-pushed
the
amartya4256/smooth_scaling_rounding_error_fix
branch
2 times, most recently
from
f8fc354 to
76a8eb4
Compare
This commit contributes to moving the logic for scaling the ImageData from DPIUtil to Image for each platform and use their own paltform specific implementations. The method gets rid of the methods specific to scaling ImageData, which are autoScaleImageData, scaleImageData, autoScaleUp and autoScaleDown from DPIUtil. contributes to eclipse-platform#62 and eclipse-platform#127
amartya4256
force-pushed
the
amartya4256/smooth_scaling_rounding_error_fix
branch
from
76a8eb4 to
15c999a
Compare
|
@HeikoKlare , @laeubi are all your changes/suggestions met? Would you like to have this merged or would you rather wait for M2? |
amartya4256
changed the title
| } | ||
|
|
||
| private ImageData scaleUsingSmoothScaling(ImageData imageData, int width, int height) { | ||
| Image original = new Image (device, (ImageDataProvider) zoom -> imageData); |
There was a problem hiding this comment.
why do we create a new image here and not using this Image?
There was a problem hiding this comment.
Wouldn't that create an endless loop? When you try to create image data at a specific zoom by painting the image into a GC, which requires requesting the image data at that specific zoom, this will likely produce a stack overflow.
There was a problem hiding this comment.
The idea was that at the Image level we can access any internal API as required (including a painting the current ImageData instead of using ImageDataProvider).
| return scaledTo (imageData, zoom, 100, DPIUtil.getScalingType(imageData)); | ||
| } | ||
|
|
||
| private ImageData scaledTo(ImageData imageData, int targetZoom, int currentZoom, int scaleType) { |
There was a problem hiding this comment.
| private ImageData scaledTo(ImageData imageData, int targetZoom, int currentZoom, int scaleType) { | |
| private Image scaledTo(int targetZoom, int scaleType) { |
so the caller can operate on the Image (dispose it or use it or ...) also current zoom seems always 100, the image data can also better be fetched by the method itself
There was a problem hiding this comment.
This always being 100 is a specifics of the MacOS implementation. Independent from that making a scaledTo method return an Image would not really make sense, as there is no image being scaled (from which you could retrieve the data scaled according to different zooms again), but the data itself is scaled.
I think it would been better to have an (package private) |
This PR contributes to fixing the implementation of Smooth scaling of the ImageData to get rid of the rounding errors because of multiple scale ups and downs with fractional scale factor. The commit moves and modifies the DPIUtil::autoScaleImageData method implementation in ImageData class to adapt the same.
contributes to
#62 and #127