[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 2d-dev
mailing list