[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-2d-dev
Subject: Re: [OpenJDK 2D-Dev] Review request for 8029339 Custom MultiResolution image support on HiDPI displa
From: Alexander Scherbatiy <alexandr.scherbatiy () oracle ! com>
Date: 2015-04-15 14:04:54
Message-ID: 552E7006.70101 () oracle ! com
[Download RAW message or body]
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@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.
> > > >
> >
>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic