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

Alexander Scherbatiy alexandr.scherbatiy at oracle.com
Thu Nov 21 06:15:45 PST 2013

    Could you review the updated fix:

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

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


>             ...jim

More information about the awt-dev mailing list