Could you review the updated fix:
http://cr.openjdk.java.net/~alexsch/8011059/webrev.09/
On 11/20/2013 3:55 AM, Jim Graham wrote:
Hi Alexander:
SunHints:
ON - Always use resolution-specific variants of images
OFF - Use only the standard resolution of an image
DEFAULT - Choose image resolutions based on a default heuristic
(perhaps add "(when available)" to ON?)
Note that I think you are still using the descriptions I asked for in
the first quotation which I then revised because they were confusing.
Please use the latter descriptions (ON, OFF, DEFAULT above).
Updated.
SG2D:
- If you rewrite getTransformedDestImageSize() to return the
resolution variant instead of a Dimension then you avoid having to
create a Dimension object that gets immediately tested for null again
and/or consumed by the caller. Just move the call to
getResolutionVariant() inside the helper method and rename it.
Updated.
- In getTransformedDestImageSize - dx,dy,sx,sy - should be named
dw,dh,sw,sh?
Updated.
- Also in getTransformedDestImageSize - the array of Point objects is
problematic because they are used for the return values and that means
early truncation/rounding, the methods that take arrays of doubles are
generally less problematic in terms of garbage generation. Also, you
can use deltaTransform() because you don't need to worry about
translation components since you only want the distance. Finally, why
not use the optimization I gave to just grab the matrix elements
directly and do the calculations on them instead of 3 separate point
transforms which have predictable outcomes? Here is the
recommendation again:
And even for the case of GENERAL transform, the above can be
simplified. The tx.deltaTransform method in that case simply loads
the array with:
{ m00, m10, m01, m11 }
and so the hypot methods are simply:
xScale = Math.hypot(tx.getScaleX(), tx.getShearY());
yScale = Math.hypot(tx.getShearX(), tx.getScaleY());
Multiply those values with dw,dh and you have your answer. Another way
to look at this is that your 3 transformed points were basically:
{ dx1 * m00 + dy1 * m01 + tx, dx1 * m10 + dy1 * m11 + ty }
{ dx2 * m00 + dy1 * m01 + tx, dx2 * m10 + dy1 * m11 + ty }
{ dx1 * m00 + dy2 * m01 + tx, dx1 * m10 + dy2 * m11 + ty }
Subtracting [1] - [0] for the distance calculation eliminates a lot of
terms and gives you:
Math.hypot((dx2 - dx1) * m00, (dx2 - dx1) * m10)
== (dx2 - dx1) * Math.hypot(m00, m10);
== (dx2 - dx1) * Math.hypot(tx.getScaleX(), tx.getShearY());
Subtracting [2] - [0] also reduces:
Math.hypot((dy2 - dy1) * m01, (dy2 - dy1) * m11);
== (dy2 - dy1) * Math.hypot(m01, m11);
== (dy2 - dy1) * Math.hypot(tx.getShearX(), tx.getScaleY());
- Also in getTransformeDestImageSize - the usage of the
transform.getType() is off, some of the values are bitmasks so a <=
comparison isn't the right comparison to use. I'd use the following
structure:
Updated.
To check if it is "identity-ish", I'd use:
((type & ~(TYPE_TRANSLATION | TYPE_FLIP)) == 0)
To check for scale, use:
((type & ~(TYPE_TRANSLATION | TYPE_FLIP | TYPE_SCALE_MASK)) == 0)
and then the distance/hypot equations are in the final else as they
are now.
Updated.
- In calculating scaledWidth/width and the cast to float - even though
I think the precedence order is on your side, I usually add extra
parentheses to highlight the order so that people reading the code
and/or modifying it don't mess it up. My cutoff on what is obvious to
a casual reader is usually "additive vs. multiplicative" and anything
more subtle than that I use parentheses just in case someone comes
along who is less familiar with the order and decides to interpose
some additional operations that get in the way of the proper
calculation. I would have written "((float) sW) / w"
Updated.
just to make it obvious.
In LWCToolkit.java:
- there is a lot of string manipulation going on for every image
fetch. I guess this would only hurt those who use the Toolkit as
their image storage (getting the image every time they need it), so
I'm not going to worry about it, but it would be good to eventually
get this integrated into the caching mechanism better so that a quick
lookup of a previously loaded image could return faster.
One thing I did not examine very closely was the transfer of hash
lookup and security code to the new utility class. You should get a
double-check from another engineer on that code.
Some more inline comments:
On 11/19/2013 5:06 AM, Alexander Scherbatiy wrote:
Could you review the updated fix:
http://cr.openjdk.java.net/~alexsch/8011059/webrev.08/
- Some related to the ToolkitImage staff is moved from
SunToolkit to ToolkitImageUtil
I'm not sure why this was done - it added a bunch of busy-work for
approving the webrev that I don't have time for. Hopefully someone
else can verify the cut/paste and refactoring.
I have reverted it back.
- Preloading for the resolution variants is added
I'm not familiar with the ImageIcon, but it looks like the use of it
for the resolution variant causes the resolution variant images to be
aggressively kicked off immediately rather than as the image is used.
For an application that loads 1000 images and then only renders 10 of
them, it will get 1000 4xsized images (@2x have 4x pixels) loaded for
990 images that would never have been loaded previously. Am I
misreading how the ImageIcon is used there?
The fix sets the resolution variant hint to default on Mac OS X
and to off for other platforms now.
I'm still not sure why it isn't just left as DEFAULT on all
platforms. The other platforms don't even create resolution images
anyway so their default is to just use the existing image which is
fine. We should not have platform issues affecting the default values
for hints.
A user can create custom MultiResolution image. If our target is
to show the resolution variant according to the current transform on all
platforms then it is ok.
The new logic uses the transform to get the destination image
width and height.
There is the trick question about the general transform type
because image sides are not equal after the transformation in this
case. So we could use only one side,
or maximum value from opposite sides or may be an average value.
It looks like you are supplying the independent w,h and the default
implementation of getResolutionVariant() is using "dispw <= imgw &&
disph <= imgh" which I think I asked Mike about and he said that was
correct. Is there another issue I'm missing?
The parameter passed to the getResVariant method is based on the
subimage size, but the method treats it as if it were based on the
full image size. Either compute the scaled size of the full image,
or inform the method of the sub-image being scaled, or simply pass
in the scales, otherwise you will get the default image for any
sub-image renderings of less than half of the original image even if
they are rendered at retina scales.
This is my fault. I really missed this part. It is updated in
the fix.
I think this is done in a fairly reasonable first approximation. It
works for the only case we have currently which is @2x images where
the incoming integers translate into outgoing integers, but we'll have
to revisit these exact calculations if we ever want to support
non-integer-scale resolution variants (I think Win8 might have a
number of non-integer scales).
The algorithm that gets the scaled image size is based on the
width and height of both images original and scaled. That means that
both images should be already preloaded.
In other case we need to draw the original image (because it is
not possible to know the destination image size) and wait for more
drawImage(Image ) calls to try to load the scaled image.
I don't like preloading them because we never preloaded the images
previously and this will be a huge memory concern, not to mention the
fact that we used to return nearly immediately from getImage() and now
we have to load an entire (4x pixels) image to just return from that
method. This all needs to be done asynchronously.
I reverted it back.
Also, the code in getImageWithResolutionVariants() appears to wrap the
image afresh every time it is called. If you call it twice with a
given string then the first time will make a regular image and store
it there, then make a scaled image and replace it with the scaled
image. The second call will retrieve the scaled image left by the
first call and then combine it with a second instance of the scaled
image to make a new doubly-nested scaled image and store that in the
hash.
fixed.
It seems that multi-resolution image preloading fixes image
drawing issue as well as issue with the image observers. An image
observer is not called from the g.drawImage()
method in case if both images images are preloded and the
original image does not have errors.
What about the other cases of errors? What if they switch back and
forth between the resolutions? What image is reported in the callback
for the non-scaled image?
If an image is preloaded and the base image does not have errors
than the image observer is not called. That was a point for the image
preloading.
What happens for MediaTracker?
The fix does not use url.openConnection() now. It tries to
preload the @2x image and if it is successful it preloads the
original image as well.
getImage() is not supposed to preload anything. Also, I didn't see
where it preloads the original image, just the resolution variant.
We do not need to preload the original image. The
MultiResolutionImage which is based on the original image should be
preloaded instead.
In the last fix the images are not preloaded. I read the previous
message there it is suggested to keep the original image in the
ToolkitImage.
However, a user can create it's own MultiResolutionImage
implementation. So I added the static containsResolutionVariant() method
and updated
necessary observers in our code. Usually the code "image!= img" is
changed to "MultiResolutionImage.containsResolutionVariant(image, img)"
and x, y, width, and height values are recalculated. The same
should be done in a user's code.
I also look at the custom cursor part of the HiDPI issues. A user
needs to prepare the MultiResolutionImage and we can use
CImage.createFromImages(List<Image> images)
method to create necessary NSImage. This means that it should be
possible to obtain all resolution variants from the
MultiResolutionImage.
I have included the "List<Image> getResolutionVariants()" method
to the MultiResolutionImage for that.
I was going to ask about that new method. Why does the developer need
to manually intercede here? They should be able to grab an image that
has an @2x variant, use MediaTracker or some other mechanism to make
sure it is loaded, then create the cursor and the code that creates
the cursor should notice the resolution variations and deal with it on
its own...
Mac OS X gets a necessary image based on the current transform. A
user can create its own MultiResolutionImage that contains images for
the scales, for example, 0.5, 0.7, 1, 3.2.
We need to know a list of all images to create the NSImage.
Thanks,
Alexandr.
...jim
