Hi Alexander:
SunHints:
The text for the descriptions on the hints is still not updated to the text I
asked for in a previous message. Since there were multiple messages on that I
thought I would paste the particular comment I was referring to:
- Descriptions for "on" and "off" should be something like "Use
resolution variants of images" and "Use only default resolution variants
of images" (and "Default resolution variant usage"). Most of the other
descriptions on hints are statements of what is going to happen or what
is going to be used rather than a simple 'the X state is Y'.
Reading this after the fact I realize that the word "default" is being used in
two different ways in the descriptions I proposed. Perhaps:
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).
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.
- In getTransformedDestImageSize - dx,dy,sx,sy - should be named dw,dh,sw,sh?
- 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:
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.
- 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" 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.
- 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.
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.
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.
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?
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.
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...
...jim
