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

Alexander Scherbatiy alexandr.scherbatiy at oracle.com
Tue Nov 19 05:06:37 PST 2013


   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
      - MultiResolutionToolkitImage is moved from the LWCToolkit to 
sun.awt.image package
      - Preloading for the resolution variants is added
      - Algorithm for the retrieving resolution variants in 
g.drawImage() method is updated
      - The test is updated

    See more comments inline:

On 11/13/2013 11:28 PM, Jim Graham wrote:
> Hi Alexander,
>
> Please read my followup message on the wording for the SunHint 
> descriptions.  The version you used in this webrev contains multiple 
> conflicting uses of the word "default".
>
> The default value for the hint should be DEFAULT.  Note that Mike has 
> already stated that NSImage uses @2x even on non-retina displays so it 
> is not clear that the default should be different depending on devScale.
      The fix sets the resolution variant hint to default on Mac OS X 
and to off for other platforms now.
>
> Mike's latest response raises the question - if someone creates an 
> application with embedded @2x images, then they will get @2x on Macs 
> even if they have a non-retina display (if they use Cocoa). If we 
> follow the same principles here, then a developer developing a Java 
> app on a non-retina Mac would see the @2x images when they scale up 
> and might get the impression that the @2x images will be used on any 
> display with a transform, but the code to deal with them is only in 
> the Mac platform code.  Should this support be more universal than that?
>
> The logic for choosing the image scale is incorrect per my previous 
> email.  You special case only TRANSLATESCALE which ignores cases where 
> the transform was "scaled down to identity" on a retina display (i.e. 
> those cases should not default to devScale), and ignores any transform 
> with any non-scale components (i.e. g.scale(100,100); g.rotate(.001); 
> will use the default resolution even though they scaled it up a lot).
      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.

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

> The code in LWCToolkit.getImage() is not thread safe.  You do lock 
> around the cache in the putImage() method, but you could end up 
> replacing the image twice with 2 different scalable versions of the 
> image since the code in LWCToolkit decided whether to make the image 
> outside of any synchronization.
     Fixed.
>
> The File/URL code in LWCToolkit is not protected by security 
> permission checks like the code in SunToolkit.
    Fixed.
> The fixes to LWCToolkit require a network connection for every image 
> created to see if the @2x version exists.  Originally I thought that 
> would be an immense performance hit, but I see that a 
> url.openConnection() is done in SunToolkit to verify permissions. This 
> would, however, double the amount of network traffic for every image. 
> Also, I'm not sure if url.openConnection() is less overhead than the 
> openStream() method used in SunToolkit...?

     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.

     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.

     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.

    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.

   Thanks,
   Alexandr.

>
>             ...jim
>
> On 11/13/13 8:11 AM, Alexander Scherbatiy wrote:
>>
>>    Could you review the updated fix:
>>      http://cr.openjdk.java.net/~alexsch/8011059/webrev.07/
>>
>>    -  The default sun hint is added.
>>       However, it looks the same as the resolution variant ON hint right
>> now. It would better to leave the behavior on the non HiDPI displays the
>> same as it was before.
>>       So the resolution variant hint is disabled by default for non
>> HiDPI displays.
>>    - Resolution variant hints description is updated.
>>    - The logic operator in the  isHiDPIImage() method is formatted.
>>
>>     The @2x images are not preloaded in the LWCToolkit. It really can
>> cause image reloading after moving a window from a display from one
>> resolution to another.
>>     However, it is not clear during the MultiResolutionImage creation
>> will the images be used on HiDPI displays or not.
>>     May be there should be a flag that enables the high resolution
>> images preloading.
>>
>>    The original image can be replaced by the high resolution one in the
>> paint() method. It causes that the observer could get an image which is
>> different from the original one.
>>    May be there is no any issue?  If a MultiResolutionImage is not used
>> then all works as before. If a user implements MultiResolutionImage may
>> be he needs to have an information
>>    about the actual drawn image in the observer even it is different
>> from the original.
>>
>>    Thanks,
>>    Alexandr.
>>
>>
>> On 11/12/2013 11:43 PM, Jim Graham wrote:
>>> Hi Alexander,
>>>
>>> Some minor issues with this fix:
>>>
>>> - All RenderingHints have a default setting where the system can
>>> choose for you.  The primary thing that the default settings allow is
>>> for the answer to be based off of another hint.  Often the QUALITY
>>> hint provides the swing vote if an individual hint is left "DEFAULT".
>>> That should probably also be the setting used for SG2D, and would
>>> hinge off of the devScale, for example, as the deciding factor.
>>>
>>> - 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'.
>>>
>>> - It looks like the transform is used in SG2D to decide if the hiDPI
>>> image should be used.  I'm not familiar with the Mac's native use of
>>> @2x, but I thought that they hinged the decision off of the "retina
>>> scale" of the display, not off of the current transform.  That way, if
>>> you scale down an icon it doesn't change its look when it reaches .5x
>>> scale (or .75x depending on the cutoff).  Personally, I think it is
>>> better to not use the transform to keep animations smooth, but I'm not
>>> sure what Apple did.
>>>
>>> - The logic in using the transform is also a bit murky.  I think if
>>> you set the scale on a retina display to exactly 1/2 it would use the
>>> HiDPI version even though the scale was 1:1.  Since I support not
>>> examining the transform there, I'm going to present this as another
>>> reason why we should just base it on devScale, but the logic could be
>>> fixed if we really plan to use the transform here.
>>>
>>> - The new logic in "isHiDPIImage()" is confusing because you line up
>>> logic operations from 2 different levels of parentheses.  I believe
>>> that one version of our style guidelines included a rule that allowed
>>> "indenting to parentheses level" and I would think that would be a
>>> good rule to apply here.  Or do something else to make the logic flow
>>> there less tricky to read.
>>>
>>> - Eventually we are going to need this support in more pipelines. I
>>> believe that Win8 already has parameters that affect choices of
>>> images, but they are only currently deployed on the Surface tablets
>>> (i.e. there are no supported high DPI displays for desktop that I'm
>>> aware of, but some of the Surface tablets ship with high DPI
>>> screens).  What would the impact be if we moved this into a more
>>> general class in src/share?  I suppose we might spend extra time
>>> looking for variants of images that we don't need?
>>>
>>> - Has any thought been given to the issues that someone raised with
>>> cursors?
>>>
>>> - Has any thought been given to my comments about MediaTracker and
>>> image observer states for multi-res images?  I don't see any attempt
>>> here to preload the @2x image.  The problem would probably only be
>>> seen on multi-headed systems where one display is retina and one is
>>> not - you would find the images suddenly reloading when you move the
>>> window to the other screen and the application might not expect that
>>> to happen.  Which image is the Observer registered on?  Since the
>>> image is handed to the Observer, will an application be surprised when
>>> their observer gets a handle to an image they've never seen? Is it an
>>> issue if one of the "alternate resolution variant" images leaks into
>>> an application's "hands" via the observer callbacks?
>>>
>>>             ...jim
>>>
>>> On 11/11/13 7:59 AM, Alexander Scherbatiy wrote:
>>>>
>>>>   Could you review the updated fix:
>>>>      http://cr.openjdk.java.net/~alexsch/8011059/webrev.06/
>>>>
>>>>    Only internal API is exposed:
>>>>    - MultiResolutionImage interface with method
>>>> "getResolutionVariant(int width, int height)" is added to the
>>>> com.sun.awt package
>>>>    - Hints to switch on/off the resolution variant usage are added to
>>>> SunHints class
>>>>    - Test is updated to use the  MultiResolutionImage interface
>>>>
>>>>   Thanks,
>>>>   Alexandr.
>>>>
>>>> On 11/5/2013 3:16 PM, Alexander Scherbatiy wrote:
>>>>>
>>>>>    Thank you for the review.
>>>>>
>>>>>   Could you look at the updated fix:
>>>>> http://cr.openjdk.java.net/~alexsch/8011059/webrev.05/
>>>>>
>>>>>   - URL is parsed to protocol, host, port, and path parts in the
>>>>> LWCToolkit class.
>>>>>      I checked that URL(protocol, host, port, file) constructor
>>>>> correctly handles -1 port value.
>>>>>   - Only last file name after last '/' in the URL path  is converted
>>>>> to @2x name
>>>>>   - Tests that check correct URL and path translation to @2x names 
>>>>> are
>>>>> added to the ScalableImageTest
>>>>>
>>>>>   Thanks,
>>>>>   Alexandr.
>>>>>
>>>>>
>>>>> On 11/1/2013 12:46 AM, Peter Levart wrote:
>>>>>>
>>>>>> On 10/29/2013 05:45 PM, Alexander Scherbatiy wrote:
>>>>>>>> 2. I'm not sure that the proposed getScaledImageName()
>>>>>>>> implementation in ScalableToolkitImage works perfectly for URLs
>>>>>>>> like this:
>>>>>>>>
>>>>>>>> http://www.exampmle.com/dir/image
>>>>>>>>
>>>>>>>> In this case it will try to find 2x image here:
>>>>>>>>
>>>>>>>> http://www.example@2x.com/dir/image
>>>>>>>>
>>>>>>>> which doesn't look correct.
>>>>>>>        Fixed. Only path part of a URL is converted to path2x.
>>>>>>
>>>>>> Hi Alexander,
>>>>>>
>>>>>> URLs like this:
>>>>>>
>>>>>> http://www.example.com/dir.ext/image
>>>>>>
>>>>>> will still be translated to:
>>>>>>
>>>>>> http://www.example.com/dir@2x.ext/image
>>>>>>
>>>>>>
>>>>>> I think you need to search for last '.' after the last '/' in the
>>>>>> getScaledImageName();
>>>>>>
>>>>>>
>>>>>> Also the following code has some additional bugs:
>>>>>>
>>>>>>   853         static Image toScalableImage(Image image, URL url) {
>>>>>>   854
>>>>>>   855             if (url != null && !url.toString().contains("@2x")
>>>>>>   856                     && !(image instanceof
>>>>>> ScalableToolkitImage)) {
>>>>>>   857                 String protocol = url.getProtocol();
>>>>>>   858                 String host = url.getHost();
>>>>>>   859                 String file = url.getPath();
>>>>>>   860                 String file2x =*host 
>>>>>> +*getScaledImageName(file);
>>>>>>   861                 try {
>>>>>>   862                     URL url2x = new URL(protocol, host, 
>>>>>> file2x);
>>>>>>   863                     url2x.openStream();
>>>>>>   864                     return new ScalableToolkitImage(image,
>>>>>> getDefaultToolkit().getImage(url2x));
>>>>>>   865                 } catch (Exception ignore) {
>>>>>>   866                 }
>>>>>>   867             }
>>>>>>   868             return image;
>>>>>>   869         }
>>>>>>
>>>>>> Why are you prepending *host* to getScaledImageName(file) in line
>>>>>> 860? Let's take the following URL for example:
>>>>>>
>>>>>> http://www.example.com/dir/image.jpg
>>>>>>
>>>>>> protocol = "http"
>>>>>> host = "www.example.com"
>>>>>> file = "/dir/image.jpg"
>>>>>> file2x = "*www.example.com*/dir/image at 2x.jpg"
>>>>>> url2x =
>>>>>> URL("http://www.example.com*www.example.com*/dir/image@2x.jpg")
>>>>>>
>>>>>>
>>>>>> You are missing a part in URL (de)construction - the optional port!
>>>>>> For example in the following URL:
>>>>>>
>>>>>> http://www.example.com:8080/dir/image.jpg
>>>>>>
>>>>>> You should extract the port from original URL and use it in new URL
>>>>>> construction if present (!= -1).
>>>>>>
>>>>>>
>>>>>> I would also close the stream explicitly after probing for existence
>>>>>> of resource rather than delegating to GC which might not be promptly
>>>>>> and can cause resource exhaustion (think of MAX. # of open file
>>>>>> descriptors):
>>>>>>
>>>>>>         try (InputStream probe = url.openStream()) {}
>>>>>>
>>>>>>
>>>>>>
>>>>>> Regards, Peter
>>>>>>
>>>>>>
>>>>>
>>>>
>>



More information about the awt-dev mailing list