<AWT Dev> [OpenJDK 2D-Dev] Review request for 8029339 Custom MultiResolution image support on HiDPI displays

Alexander Scherbatiy alexandr.scherbatiy at oracle.com
Wed Apr 15 14:04:54 UTC 2015


   Hello,

  Could you review the updated fix:
    http://cr.openjdk.java.net/~alexsch/8029339/webrev.08/

  - SunGraphics2D is updated to calculate the resolution variant size 
according to the _BASE, _DPI_FIT, and _SIZE_FIT resolution rendering hint
  - MultiResolutionRenderingHints test is added

  Thanks,
  Alexandr.


On 4/7/2015 1:02 PM, Jim Graham wrote:
> This is an interesting suggestion that would let us keep the 
> implementation of the various hints centralized and simplify the 
> interface to the part of the mechanism that is most tied in to the 
> implementation specifics of the database of media variants - which is 
> housed in the MRI-implementing object.
>
> I'm not sure we ever identified any need to customize the logic of 
> "what size is needed for this render operation" beyond what we 
> expressed in the hints, and given that the platform defaults may not 
> be easy to express to an arbitrary implementation, it is probably 
> better to put that part of the logic in SG2D, controlled by the easy 
> to express hints and keep the current API both simple to implement and 
> flexible to use.  Even if there was a need to customize that part of 
> the operation (the "what size is relevant to this rendering operation" 
> decision) beyond what the hints suggest, it would be inappropriate to 
> tie that in to the "find me media" aspect of the MRI interface 
> anyway.  So, best to keep them separate and have the hints affect what 
> SG2D does and have the MRI focused on just storing (possibly creating) 
> and managing/finding the variants.
>
>             ...jim
>
> On 4/1/15 6:46 AM, Alexander Scherbatiy wrote:
>> On 3/27/2015 12:48 AM, Jim Graham wrote:
>>> Hi Alexander,
>>
>>    http://cr.openjdk.java.net/~alexsch/8029339/webrev.07/
>>    I have updated the fix according to the comments except 
>> RenderingHints.
>>
>>    RenderingHints are supposed to be set on Graphics2D and they have 
>> keys/values which are not relevant to the getResolutionVariant() method.
>>    Graphics objects chooses an alternative according to the set 
>> rendering hints.
>>    May be what SG2D should do just calculate dest image size for the 
>> given resolution variant hint (_BASE - base image size, _SIZE_FIT - 
>> including scale factor and graphics transform (Mac algorithm), 
>> _DPI_FIT - scaled according to the system DPI) and then pass them to 
>> the getResolutionVariant() method?
>>
>>   Thanks,
>>   Alexandr.
>>
>>>
>>> MRI.java:
>>>
>>> Who is the intended audience for the recommendation in the interface 
>>> to cache image variants?  Are we asking the developers who call the 
>>> methods on the interface to cache the answers? That would be unwise 
>>> because the list of variants may change over time for some MRIs.  
>>> Are we speaking to the implementer? If so, then I think that it is 
>>> unnecessary to remind implementers of basic implementation 
>>> strategies like "cache complicated objects".
>>>
>>> How about this wording for the getRV() method? - "Gets a specific 
>>> image that is the best variant to represent this logical image at 
>>> the indicated size and screen resolution."
>>>
>>> Perhaps we should clarify in the doc comments for the dimension 
>>> arguments in getRV() that the dimensions are measured in pixels?
>>>
>>> line 56 - delete blank line between doc comment and method declaration
>>>
>>> Also, it is probably overkill to document that the interface 
>>> getVariants() method returns an unmodifiable list. Responsible 
>>> implementations would probably use an unmodifiable list, but I don't 
>>> think it should be required by the interface.  We do need to specify 
>>> that it isn't required to be modifiable so that a caller doesn't 
>>> expect to be able to modify it, but that is a looser spec.  How 
>>> about "Gets a readable list of all resolution variants.  Note that 
>>> many implementations might return an unmodifiable list."?
>>>
>>> AbstractMIR.java:
>>>
>>> "provides a default implementation for the MRI" or "provides default 
>>> implementations of several methods in the <MRI> interface and the 
>>> <Image> class"?  Actually, I'll note that it provides none of the 
>>> implementations of the MRI methods so maybe it should be "provides 
>>> default implementations of several <Image> methods for classes that 
>>> want to implement the <MRI> interface"?
>>>
>>> In the doc example - wrap the list to make it unmodifiable
>>>
>>> The doc example could be made 2 or 3 lines shorter by simply 
>>> assuming the base image is in index 0 (it's just an example so I'm 
>>> not sure the flexibility needs to be a part of the example).
>>>
>>> getGraphics is allowed by the Image interface to return null. 
>>> Actually, the exact wording is that it can only be called for 
>>> "offscreen images" and a MRI is technically not "an offscreen 
>>> image".  Perhaps we should return null here since modifying the base 
>>> image is unlikely to modify the variants and arguably it would be 
>>> required by the wording in the method docs (even if the base image 
>>> was an offscreen, the MRI produced from it stops being an offscreen)?
>>>
>>> Are all of the empty "@Inherit" comments needed?  I thought 
>>> inheritance was the default?
>>>
>>> BaseMRI.java:
>>>
>>> "This class is [an] array-based implementation of the {AMRI} class" 
>>> (missing "an" and "the")
>>>
>>> We should probably include the comment about the sorting of the 
>>> images in the class comments as well as document that the algorithm 
>>> is a simple scan from the beginning for the first image large enough 
>>> (and default == last image).  The algorithm also might not work very 
>>> well if the images are not monotonically both wider and taller.  How 
>>> about adding this to the class comments:
>>>
>>> "The implementation will return the first image variant in the array 
>>> that is large enough to satisfy the rendering request. For best 
>>> effect the array of images should be sorted with each image being 
>>> both wider and taller than the previous image. The base image need 
>>> not be the first image in the array."
>>>
>>> getVariants() - you need to return an unmodifiable list. asList() 
>>> returns a list that "writes back" to the array.  You need to use 
>>> Collections.unmodifiableList(Arrays.asList(array)).
>>>
>>> RenderingHints.java:
>>>
>>> Where do we process this hint?  Don't we need to pass it to the 
>>> getVariant() method?
>>>
>>> I don't understand the hint values other than the one that always 
>>> uses the base image.  Default I guess gives us the ability to use 
>>> the Mac algorithm of "next larger size" on Mac and "based on Screen 
>>> DPI" on Windows, but the 3rd value "ON" is so vague as to not have 
>>> any meaning.  Perhaps we should just delete it, but then do we just 
>>> always do the Mac method? Or do we vaguely have our internal images 
>>> have a platform-specific method and the Abstract/Base classes just 
>>> do what they want with no control from the user?  In FX we are also 
>>> still struggling with this issue and we may likely just do what the 
>>> Mac does in all cases, but perhaps AWT needs to "behave like the 
>>> platform" more?  If we are going to have actual values, then we need 
>>> to have them do something, which means:
>>>
>>> - SG2D needs to track the hint just like we do the other hints that 
>>> affect our processing
>>> - MRI.getVariant() needs to have the hint as an argument
>>> - BaseMRI should probably do something with that hint
>>> - hint values should include:
>>>     - ..._DEFAULT - implementation gets to decide
>>>     - ..._BASE - always use the base image
>>>     - ..._SIZE_FIT - Mac algorithm of smallest image that is big enough
>>>     - ..._DPI_FIT - choose based on DPI of the screen, ignoring 
>>> transform
>>>
>>> line 978 - missing blank line between fields
>>>
>>> SG2D.java:
>>>
>>> - The interface says that you will be passing in the "logical DPI" 
>>> of the display, but here you are actually passing in the screen's 
>>> scale factor.
>>>
>>> BaseMRITest.java:
>>>
>>> - testBaseMRI also passes in a scale rather than a DPI to the MRI 
>>> method.
>>>
>>> - How does the modification test pass when the implementation 
>>> doesn't use unmodifiable lists?
>>>
>>> MRITest.java:
>>>
>>> - also uses scale rather than DPI in several places
>>>
>>>         ...jim
>>>
>>> On 3/13/15 6:34 AM, Alexander Scherbatiy wrote:
>>>>
>>>>    Hello,
>>>>
>>>>    Could you review the proposed API based on MultiresolutionImage
>>>> interface:
>>>>      http://cr.openjdk.java.net/~alexsch/8029339/webrev.06/
>>>>
>>>>   - return unmodifiable list comment is added to the
>>>> getResolutionVariants() method javadoc in MultiresolutionImage 
>>>> interface
>>>>   - base image size arguments are removed form the
>>>> getResolutionVariant(...) method in MultiresolutionImage interface
>>>>   - BaseMultiResolutionImage class that allows to create a
>>>> multi-resolution image based on resolution variant array is added
>>>>   - the test for the BaseMultiResolutionImage is added
>>>>
>>>>   Thanks,
>>>>   Alexandr.
>>>>
>>>> On 2/14/2015 3:23 AM, Jim Graham wrote:
>>>>> The second solution looks good.  I'd make it standard practice
>>>>> (perhaps even mentioned in the documentation) to return unmodifiable
>>>>> lists from the getVariants() method.  The Collections class provides
>>>>> easy methods to create these lists, and it sends a clear message to
>>>>> the caller that the list was provided for them to read, but not write
>>>>> to.  Otherwise they may add a new image to the list you provided them
>>>>> and wonder why it wasn't showing up.  Also, an unmodifiable list can
>>>>> be cached and reused for subsequent calls without having to create a
>>>>> new list every time.
>>>>>
>>>>> In getResolutionVariant() was there a reason why the base dimensions
>>>>> were provided as float?  The destination dimensions make sense as
>>>>> float since they could be the result of a scale, but the source
>>>>> dimensions are typically getWidth/getHeight() on the base image.  A
>>>>> related question would be if they are needed at all, since the
>>>>> implementation should probably already be aware of what the base 
>>>>> image
>>>>> is and what its dimensions are.  I'm guessing they are provided
>>>>> because the implementation in SG2D already knows them and it makes it
>>>>> easier to forward the implementation on to a shared (static?) method?
>>>>>
>>>>> With respect to default implementations, I take it that the 
>>>>> BaseMRI is
>>>>> along the pattern that we see in Swing for Base classes. Would it be
>>>>> helpful to provide an implementation (in addition or instead) that
>>>>> allows a developer to take a bunch of images and quickly make an MRI
>>>>> without having to override anything?  The implementations of
>>>>> getBaseImage() and getResolutionVariants() are pretty straightforward
>>>>> and a fairly reasonable default algorithm can be provided for
>>>>> getRV(dimensions).  This question is more of an idle question for my
>>>>> own curiosity than a stumbling block...
>>>>>
>>>>>             ...jim
>>>>>
>>>>> On 1/22/2015 6:49 AM, Alexander Scherbatiy wrote:
>>>>>>
>>>>>>    Hi Phil,
>>>>>>
>>>>>>    I have prepared two versions of the proposed API:
>>>>>>
>>>>>>    I) Resolution variants are added directly to the Image:
>>>>>> http://cr.openjdk.java.net/~alexsch/8029339/list/webrev.00
>>>>>>
>>>>>>    II)  MultiResolutionImage interface is used:
>>>>>> http://cr.openjdk.java.net/~alexsch/8029339/webrev.05
>>>>>>
>>>>>>    It could help to decide which way should be chosen for the the
>>>>>> multi-resolution image support.
>>>>>>
>>>>>>    Below are some comments:
>>>>>>
>>>>>>    1. High level goal:
>>>>>>       Introduce an API that allows to create and handle an image 
>>>>>> with
>>>>>> resolution variants.
>>>>>>
>>>>>>    2. What is not subject of the provided API
>>>>>>      - Scale naming convention for high-resolution images
>>>>>>      - Providing pixel scale factor for the screen/window
>>>>>>
>>>>>>    3. Use cases
>>>>>>     3.1 Loading and drawing high-resolution icons in IntelliJ IDEA
>>>>>>       A high-resolution image is loaded from resources and stored in
>>>>>> JBHiDPIScaledImage class  which is a subclass of the buffered image.
>>>>>>       The high-resolution image is used to create a disabled icon 
>>>>>> in the
>>>>>> IconLoader.getDisabledIcon(icon) method.
>>>>>> https://github.com/JetBrains/intellij-community/blob/master/platform/util/src/com/intellij/openapi/util/IconLoader.java 
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>     3.2 Loading and drawing high-resolution icons in NetBeans
>>>>>>       NetBeans does not have support for the high-resolution icons
>>>>>> loading.
>>>>>>       It loads an icon from the file system using
>>>>>> Toolkit.getDefaultToolkit().getImage(url) method or from resources
>>>>>>       by  ImageReader  and store it in ToolTipImage class which is
>>>>>> subclass of the buffered image.
>>>>>>       ImageUtilities.createDisabledIcon(icon) method creates a 
>>>>>> disabled
>>>>>> icon by applying  RGBImageFilter to the icon.
>>>>>> http://hg.netbeans.org/main/file/97dcf49eb4a7/openide.util/src/org/openide/util/ImageUtilities.java 
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>     3.3 Loading system icons in JDK 1.8
>>>>>>       JDK requests icons from the native system for system L&Fs and
>>>>>> applies filters for them.
>>>>>>       See for example AquaUtils.generateLightenedImage() method:
>>>>>> http://hg.openjdk.java.net/jdk9/client/jdk/file/e6f48c4fad38/src/java.desktop/macosx/classes/com/apple/laf/AquaUtils.java 
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>    4. HiDPI support for Images on different OSes
>>>>>>
>>>>>>      4.1 Mac OS X
>>>>>>        Cocoa API contains NSImage that allows to work with image
>>>>>> representations: add/remove/get all representations.
>>>>>>        It picks up an image with necessary resolution based on the
>>>>>> screen backing store pixel scale factor and applied transforms.
>>>>>> https://developer.apple.com/library/mac/documentation/Cocoa/Reference/ApplicationKit/Classes/NSImage_Class/Reference/Reference.html 
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>      4.2 Linux
>>>>>>        GTK+ 3 API has gtkcssimagescaled lib (it seems that it is not
>>>>>> public/stable)
>>>>>>        that parses the -gtk-scaled css property and draws a 
>>>>>> GtkCssImage
>>>>>> according to the given scale factor.
>>>>>>
>>>>>>        I have not found information about the HiDPI support in Xlib.
>>>>>>
>>>>>>      4.3 Windows
>>>>>>        I have only found the tutorial that suggests to select and 
>>>>>> draw a
>>>>>> bitmap using the queried DPI
>>>>>>        and scale the coordinates for drawing a rectangular frame
>>>>>> http://msdn.microsoft.com/en-us/library/dd464659%28v=vs.85%29.aspx
>>>>>>
>>>>>>        Windows also provides the horizontal and vertical DPI of the
>>>>>> desktop
>>>>>> http://msdn.microsoft.com/en-us/library/windows/apps/dd371316
>>>>>>
>>>>>>    5. Pseudo API
>>>>>>       Below are some ways which illustrates how multi-resolution 
>>>>>> images
>>>>>> can be created and used.
>>>>>>
>>>>>>      5.1 Resolution variants are stored directly in Image class.
>>>>>>      To query a resolution variant it needs to compare the 
>>>>>> resolution
>>>>>> variant width/height
>>>>>>      with the requested high-resolution size.
>>>>>>      ------------
>>>>>>      public abstract class Image {
>>>>>>
>>>>>>          public void addResolutionVariant(Image image) {...}
>>>>>>          public List<Image> getResolutionVariants() {...}
>>>>>>      }
>>>>>>      ------------
>>>>>>      // create a disabled image with resolution variants
>>>>>>
>>>>>>      Image disabledImage = getDisabledImage(image);
>>>>>>
>>>>>>      for (Image rv : image.getResolutionVariants()) {
>>>>>> disabledImage.addResolutionVariant(getDisabledImage(rv));
>>>>>>      }
>>>>>>      ------------
>>>>>>      This approach requires that all resolution variants have been
>>>>>> created even not of them are really used.
>>>>>>
>>>>>>      5.2  Resolution variants are stored in a separate object that
>>>>>> allows to create them by demand.
>>>>>>      To query a resolution variant it needs to compare the 
>>>>>> resolution
>>>>>> variant scale factor
>>>>>>      with the requested scale (that can include both screen DPI 
>>>>>> scale
>>>>>> and applied transforms).
>>>>>>      ------------
>>>>>>      public abstract class Image {
>>>>>>
>>>>>>          public static interface ResolutionVariant {
>>>>>>              Image getImage();
>>>>>>              float getScaleFactor();
>>>>>>          }
>>>>>>
>>>>>>          public void addResolutionVariant(ResolutionVariant
>>>>>> resolutionVariant) {...}
>>>>>>          public List<ResolutionVariant> getResolutionVariants() 
>>>>>> {...}
>>>>>>      }
>>>>>>      ------------
>>>>>>      // create a disabled image with resolution variants
>>>>>>      Image disabledImage = getDisabledImage(image);
>>>>>>
>>>>>>      for (Image.ResolutionVariant rv : 
>>>>>> image.getResolutionVariants()) {
>>>>>>          disabledImage.addResolutionVariant(new
>>>>>> Image.ResolutionVariant() {
>>>>>>
>>>>>>              public Image getImage() {
>>>>>>                  return getDisabledImage(rv.getImage());
>>>>>>              }
>>>>>>
>>>>>>              public float getScaleFactor() {
>>>>>>                  return rv.getScaleFactor();
>>>>>>              }
>>>>>>          });
>>>>>>      }
>>>>>>      ------------
>>>>>>
>>>>>>      It does not have problem if a predefined set of images is 
>>>>>> provided
>>>>>> (like image.png and image at 2x.png on the file system).
>>>>>>      This does not cover cases where a resolution variant can be 
>>>>>> created
>>>>>> using the exact requested size (like loading icons from the native
>>>>>> system).
>>>>>>      A resolution variant can be queried based on a scale factor and
>>>>>> applied transforms.
>>>>>>
>>>>>>      5.3 The provided example allows to create a resolution variant
>>>>>> using the requested high-resolution image size.
>>>>>>      ------------
>>>>>>      public interface MultiResolutionImage {
>>>>>>          Image getResolutionVariant(float width, float height);
>>>>>>      }
>>>>>>      ------------
>>>>>>      // create a multi-resolution image
>>>>>>      Image mrImage = new AbstractMultiResolutionImage() {
>>>>>>
>>>>>>              public Image getResolutionVariant(float width, float
>>>>>> height) {
>>>>>>                  // create and return a resolution variant with 
>>>>>> exact
>>>>>> requested width/height size
>>>>>>              }
>>>>>>
>>>>>>              protected Image getBaseImage() {
>>>>>>                  return baseImage;
>>>>>>              }
>>>>>>          };
>>>>>>      ------------
>>>>>>      // create a disabled image with resolution variants
>>>>>>      Image disabledImage = null;
>>>>>>      if (image instanceof MultiResolutionImage) {
>>>>>>          final MultiResolutionImage mrImage = (MultiResolutionImage)
>>>>>> image;
>>>>>>          disabledImage = new AbstractMultiResolutionImage(){
>>>>>>
>>>>>>              public Image getResolutionVariant(float width, float
>>>>>> height) {
>>>>>>                  return
>>>>>> getDisabledImage(mrImage.getResolutionVariant(width, height));
>>>>>>              }
>>>>>>
>>>>>>              protected Image getBaseImage() {
>>>>>>                  return getDisabledImage(mrImage);
>>>>>>              }
>>>>>>          };
>>>>>>      } else {
>>>>>>          disabledImage = getDisabledImage(image);
>>>>>>      }
>>>>>>      ------------
>>>>>>
>>>>>>    Thanks,
>>>>>>    Alexandr.
>>>>
>>
>



More information about the awt-dev mailing list