<AWT Dev> [OpenJDK 2D-Dev] [8] Review request for 8011059 [macosx] Make JDK demos look perfect on retina displays

Jim Graham james.graham at oracle.com
Tue Nov 19 15:55:18 PST 2013

Hi Alexander:


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).


- 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:
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...


More information about the awt-dev mailing list