[OpenJDK 2D-Dev] <AWT Dev> [8] Review request for 8011059 [macosx] Make JDK demos look perfect on retina displays
Alexander Scherbatiy
alexandr.scherbatiy at oracle.com
Tue Dec 3 03:48:43 PST 2013
Hello,
Could you review the updated fix:
http://cr.openjdk.java.net/~alexsch/8011059/webrev.13/
- observer cache is returned back
- resolution variant x/y/width/height are divided by 2 in the
observer wrapper
On 12/3/2013 1:16 AM, Jim Graham wrote:
> Hi Alexander,
>
> There must have been a miscommunication. While I think the cache has
> some issues that worry me, it is absolutely needed if you are going to
> wrap the observer. You can't just delete it and re-wrap the observer
> on every call because that will create a number of problems. If you
> are going to use a wrapper solution then we will need to live with the
> potential issues of a cache.
>
> Note that the underlying ImageReps store lists of observers to be
> notified. You will be growing that list every time a call is made on
> an image because each new wrapper looks like a new observer. For an
> animated GIF the number of drawImage calls and associated wrappers
> could be infinite (since they are never fully loaded). Also, for each
> new wrapper, an additional callback is performed. This will drive the
> performance of an animated GIF into the ground after a time when
> thousands of growing callbacks are issued for each frame.
>
> If you are going to use observer wrappers then you must cache the
> wrapper so you reuse the same wrapper on every new call.
>
> On 12/2/13 4:55 AM, Alexander Scherbatiy wrote:
>>
>> Could you review the updated fix:
>> http://cr.openjdk.java.net/~alexsch/8011059/webrev.12/
>> - Image observers are not cached now for the resolution variants
>> - base image width and height are checked in the wrapped image
>> observer
>>
>> On 11/30/2013 3:27 AM, Jim Graham wrote:
>>> Hi Alexander,
>>>
>>> I suppose the wrapping solution works for ImageObservers, but I think
>>> the suggestion I gave in recent emails was to simply modify the
>>> newInfo method (and a couple of other methods) to deliver the same
>>> information with no caches, no hashmaps/lookups, no wrapping the
>>> observer, and no wrapping with soft references. Keep in mind that
>>> observers are typically references to Component objects so any delay
>>> in processing the soft references could keep relatively large
>>> component hierarchies in play (if they are parents). It should work
>>> well for a first draft, though.
>> It seems that just updating the newInfo method is not enough.
>
> There were 5 or 6 places that called imageUpdate when I did a quick
> search and most of the calls went through newInfo. They'd all have to
> be updated. Other than that, I'm not sure why it would not be enough?
Consider the following scenario. There are image.png and image at 2x.png
files on the disk.
Image image1 = Toolkit.getImage("image.png"); // load as
multi-resolution image
Image image2 = Toolkit.getImage("image at 2x.png"); // load the image
from cache
toolkit.prepareImage(image2,.., imageObserver2);
The image2 has image1 as the base image so it rescale its
coordinates/dimension and passes the base instead of self to the
imageObserver2 which does not look correct.
>
>> The resolution variant can have a reference to the base image.
>> Loading the base image it still should compare its result with the
>> resolution variant loading.
>> So the image loading status is just moved from the SunToolkit to
>> the ToolkitImage.
>
> If you are referring to comparing the new and old dimensions then the
> newInfo method would have access to that information if it could ask a
> resolution variant what its base image is. That should be a very
> simple change to ToolkitImage (add a set/getBaseImage() method). Then
> it can do the associated comparisons and parameter adjustments right
> before calling back to the observer.
>
> Perhaps I haven't explained my vision of modifying newInfo very well.
> I'll look into sketching it out in another email since I think the
> cache/wrappers are OK for now.
>
>> I think that the current fix for the checkImage/prepareImage in the
>> SunToolkit is the simplest for the current implementation.
>> The separate fix for the ToolkitImage/ImageRepresentation can get
>> all things right.
>
> I didn't really follow you here, but I said above that the cache
> should be fine for now. The cache is definitely needed if you are
> wrapping observers as I said above in the first couple of paragraphs.
>
>>> Also, why does ObserverCache exist? Couldn't the cache just be a
>>> static field on MRToolkitImage?
>> MRToolkitImage can be used in drawImage(Image,..,ImageObserver)
>> method always with null observer. So the is no need to create the
>> observer cache or use a synchronization during the cache initialization.
>
> Maybe I'm missing something about your answer here, but I think you
> may have misunderstood my question. You placed the field that holds
> the reference to the cache inside an inner class. I didn't see why
> the reference could not be stored in the base class. Why is there an
> empty inner class to wrap a single field? In other words, why was
> this used:
>
> 56
> 57 private static class ObserverCache {
> 58
> 59 static final SoftCache INSTANCE = new SoftCache();
> 60 }
> 61
>
> Instead of just:
>
> 56
> 59 static final SoftCache INSTANCE = new SoftCache();
> 61
Just to not create the cache in case if MRToolkitImageis used but
image observers are always null. See the comment above.
Thanks,
Alexandr,.
>
>>> The current way of baking in the image dimensions assumes that they
>>> are known. If the image is loaded asynchronously, then the calls to
>>> getWidth/Height(null) may return -1 and the cached observer wrapper
>>> has no way to get them later. I would think this would fail in the
>>> typical default scenario where the user gets an image and the first
>>> even that triggers loading it is to render to the screen which would
>>> bake the -1's into the observer wrapper in all of those cases. This
>>> should probably be addressed sooner rather than later.
>> I added the base image width/height check to the observer wrapper.
>> However, there is no way to rescale image representation width and
>> height in case if the base image width and height are not known.
>
> That is true if we are basing the scaling solely on the difference in
> dimensions. In the case of @2x we also could assume that the scaling
> would be 2.0. I'm not sure if that kind of assumption can be made for
> all future resolution variant mechanisms, but I'd imagine that many of
> them come with a standard for determining the scaling of the variants
> up front so that the correct media can be chosen without having to
> load all of it first just to compare dimensions.
>
> If the image dimensions weren't known when your observer was created
> then you never attempt to recover, though. You should be querying
> "image.getWidth(this)" to try to grab the dimensions on the fly when
> they come in rather than always punting. In the current case where
> nothing is cached, you end up with multiple wrappers so some of them
> will have been created before the dimensions were known and some will
> be created after the dimensions are known and the multiple callbacks
> will all have conflicting information. You'll need to go back to a
> cache and that will mean a single wrapper for any given observer and,
> at that point, inserting calls to "image.getWidth(this /* wrapper */)"
> will be needed to make sure you get the proper dimensions at some point.
>
>>> If an @2x image gets an error we silently start using just the regular
>>> version of the image. In addition MediaTracker should not reflect
>>> that error in its answers, but I think it currently does since you add
>>> it as an implicit extra media with the same ID. I think this is OK
>>> for the first pass, but we need to track it as an issue.
>>>
>>> I saw that you fixed the toolkit versions of check/prepare image.
>>> Component also has the same operations (via its peers). The Component
>>> versions do back off to the toolkit versions, but only if they fail to
>>> find an actual peer to delegate to. Note that MediaTracker uses the
>>> Component versions so it is affected by this, but I don't think it
>>> will cause functional problems that I can think of. Eventually we'd
>>> want MT to only load the version that is appropriate for the indicated
>>> Component - and at that point then this might be an issue, but I don't
>>> think it is an issue for this first draft if it tracks both variations.
>> I checked all peers like W/LW/XComponentPeer and toolkits. They all
>> delegate check/prepareImage calls to the default toolkit.
>
> Sounds good. Eventually we might want to have them prepare just the
> default resolution for that particular device if they are already
> associated with a screen, but this should be good to go for now...
>
> The outstanding issues for me are:
>
> - The cache was necessary to eliminate the overhead of duplicated
> wrappers so it should go back (unless you want to investigate an
> alternate solution by modifying all places that call imageUpdate
> instead, which are hopefully few)
>
> - Retrying on the base image dimensions if they weren't known when a
> wrapper was created (or switching to a pre-determined scale derived
> from the resolution mechanism which benefits @2x and keeping the
> "relative dimension" scaling for the general case)
>
> ...jim
>
>> Thanks,
>> Alexandr.
>>
>>> I think it is OK to send multiple notifications to an observer since
>>> we've always been loose on the exact sequencing and the operations are
>>> all asynchronous. There could potentially be several notifications in
>>> flight at the time that the observer indicates a lack of interest and
>>> there is no way to stop them. This could be considered "another case
>>> of that". Eventually we could consider addressing this with a tighter
>>> integration between the wrapper mechanism and the code that calls
>>> imageUpdate() and receives the answer that the observer is no longer
>>> interested, but I think this is all OK for now.
>>>
>>> The only thing I think we should worry about now for this first draft
>>> is the issue of getting the right dimensions for the observer
>>> wrappers. They need to be more asynchronous about that. It already has
>>> a handle on the original image anyway, so I think it just needs to get
>>> the dimensions from there instead of passing them into the
>>> constructor, with appropriate checks for -1's...
>>>
>>> ...jim
>>>
>>> On 11/28/13 6:45 AM, Alexander Scherbatiy wrote:
>>>>
>>>> Could you review the updated fix:
>>>> http://cr.openjdk.java.net/~alexsch/8011059/webrev.11/
>>>>
>>>> I have moved new API supporting to the separate issue:
>>>> JDK-8029339 Custom MultiResolution image support on HiDPI
>>>> displays
>>>> https://bugs.openjdk.java.net/browse/JDK-8029339
>>>>
>>>> - SunToolkit.checkImage/updateImage are updated to handle
>>>> multi-resolution image
>>>> Both base image and the resolution variant are loaded now.
>>>> However, in future, it would be useful to have an ability to load, for
>>>> example, only @2x image
>>>> if the system has only Retina display on Mac OS X. So there are
>>>> can be problems with backward compatibility.
>>>>
>>>> - MediaTracker is updated to handle multi-resolution image
>>>> MediaTracker uses notifications both form the
>>>> Toolkit.prepareImage() method and image decoders. So only updating
>>>> Toolkit.prepareImage() is not enough.
>>>>
>>>> - ImageObserver is wrapped both in drawImage() and
>>>> SunToolkit.prepareImage() methods. Size and coordinates are
>>>> converted to
>>>> the base image.
>>>> A user can receive several notifications about the complete image
>>>> loading from the original image and from the resolution variant.
>>>> It can breaks his code if the code relies that only one
>>>> notification should be received.
>>>>
>>>> - Resolution variant is ignored in case of errors in the
>>>> drawImage()
>>>> It needs to be decided on which step a user should be notified
>>>> that a resolution variant has error or this notification should be
>>>> just
>>>> ignored.
>>>>
>>>> - Test is updated
>>>>
>>>> Icons and CustomCursor issues are handled by 8028212 and 8024926
>>>> bugs
>>>> and depend on the current bug implementation.
>>>>
>>>> Thanks,
>>>> Alexandr.
>>>>
>>>> On 11/28/2013 3:21 AM, Jim Graham wrote:
>>>>> The things I was talking about in the quoted email were mostly future
>>>>> directions, but I did point out some issues in another email that
>>>>> should be addressed before FCS. In particular:
>>>>>
>>>>> We do need to get the callbacks working whether they draw hi res or
>>>>> not. I think the current fix delivers no notifications at all
>>>>> because
>>>>> the observer is not registered anywhere during the course of a
>>>>> drawImage(). That's a big bug. I don't think we need to wrap
>>>>> observers to do that, there are only a few helper functions that
>>>>> deliver the notifications and they can be taught how to translate a
>>>>> resolution variant into the base image easily enough.
>>>>>
>>>>> You point out that there was no error checking for the hidpi
>>>>> image. I
>>>>> agree. Also, if the base image errors out, should we draw the @2x
>>>>> anyway? Or should that base image be required? Minimally I think we
>>>>> need to not attempt the @2x for FCS if it errored and we can worry
>>>>> about how to respond to errors in the base image later.
>>>>>
>>>>> I think that com.sun would require CCC approval for a new interface
>>>>> and the bar is really high for API in 8.0 right now. Unless a
>>>>> developer really needs to use it it might be best to move the API to
>>>>> sun.*. I didn't see any developers clamoring for it in the
>>>>> discussions I read, but let me know if I missed something. If we get
>>>>> automatic use of @2x images then I think that satisfies the
>>>>> discussions I saw.
>>>>>
>>>>> MediaTracker defers to Component.prepareImage() for which variants
>>>>> should be loaded. I think we need to teach prepareImage() to trigger
>>>>> the variant appropriate for the screen that the Component is on
>>>>> (perhaps the default screen if it isn't displayed yet).
>>>>>
>>>>> Also, there should probably be some MediaTracker, ImageIcon, and
>>>>> Cursor test cases that show that those mechanisms all work on images
>>>>> that have @2x variants. Minimally they shouldn't fail entirely, and
>>>>> very desireably they should use the @2x version when appropriate, but
>>>>> that could be a follow on bug fix as long as they don't fail for the
>>>>> first fix.
>>>>>
>>>>> I've included below the quote from my last "review" email about the
>>>>> specific issues I found. The main "blocking" issue right now is that
>>>>> it looks to me that no imageUpdate notifications at all happen for a
>>>>> hidpi image due to the ImageObserver getting dropped on the floor.
>>>>> Can
>>>>> we at least get imageUpdate() called with the base image, even if the
>>>>> xywh are wrong before we push the initial fix? I hope my info below
>>>>> helps make that an easy task.
>>>>>
>>>>>> If they only ever use the image on a retina display (or some scaled
>>>>>> context) then I don't think we ever send any notifications the way
>>>>>> the current fix is written. Also, if you don't send notifications
>>>>>> for
>>>>>> the scaled variant, but load the original and expect its
>>>>>> notifications to suffice, then we have a race condition - if the
>>>>>> original variant is fully constructed before the scaled version is
>>>>>> done then the final "OK to draw everything" notice would not happen
>>>>>> and they'd be left with a partial image.
>>>>>>
>>>>>> I think it should be easy to pass along the observer and simply have
>>>>>> ImageRep translate the image variant into the base image in its
>>>>>> newInfo method (and there are a couple of other locations that
>>>>>> imageUpdate is called that could do the same). All it takes is
>>>>>> adding "ToolkitImage.set/getBaseImage()" to ToolkitImage, defaulting
>>>>>> to "this", but the Multi-Res image will set the base image on
>>>>>> both of
>>>>>> its variants to the Multi-Res instance.
>>>>>>
>>>>>> Then we get full notices of all resolution variants.
>>>>>>
>>>>>> It would be best to scale the xywh supplied there as well. That
>>>>>> should be fairly easy, but if it is a big problem then that is lower
>>>>>> priority than getting the "ALLBITS" notification right, as that is
>>>>>> the main trigger for so many uses.
>>>>>>
>>>>>> (In the future we can add ImageObserver2 (or MultiResObserver?)
>>>>>> which
>>>>>> has support for receiving notifications of variants without
>>>>>> translation.)
>>>>>
>>>>>
>>>>> ...jim
>>>>>
>>>>> On 11/27/13 6:28 AM, Sergey Bylokhov wrote:
>>>>>> On 27.11.2013 15:12, Sergey Bylokhov wrote:
>>>>>>> Hello.
>>>>>>> Probably we are in a point when it is necessary to stop and move
>>>>>>> out
>>>>>>> all extensions to the separate bugs.
>>>>>>> We should give a time to our sqe to test these changes.
>>>>>>> Actually current version looks good to me.
>>>>>> Small additional notes, It would be good:
>>>>>> - to wrap initial observer in the drawImage and replace
>>>>>> img/x,y,....
>>>>>> in the observer.
>>>>>> - don't draw hidpi image, if it was loaded with errors.
>>>>>> - sun.com package should be used?
>>>>>> - If MediaTracker was requested to load MultiResolutionImage,it
>>>>>> should
>>>>>> load all versions?
>>>>>>>
>>>>>>> On 26.11.2013 2:31, Jim Graham wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>> On 11/25/13 5:51 AM, Alexander Scherbatiy wrote:
>>>>>>>>> On 11/23/2013 5:11 AM, Jim Graham wrote:
>>>>>>>>>> Hi Alexander,
>>>>>>>>>>
>>>>>>>>>> If we are going to be advertising it as something that
>>>>>>>>>> developers
>>>>>>>>>> use
>>>>>>>>>> in production code then we would need to file this via CCC.
>>>>>>>>>> With the
>>>>>>>>>> current implementation, any user that has their own
>>>>>>>>>> ImageObservers
>>>>>>>>>> must use this API and so it becomes not only public, at a time
>>>>>>>>>> when
>>>>>>>>>> there is a lockdown on new public API in 8.0, but it also means
>>>>>>>>>> that
>>>>>>>>>> we are tied to its implementation and we can't rely on "it was
>>>>>>>>>> experimental and so we can change it at any point" - you
>>>>>>>>>> can't say
>>>>>>>>>> that about an API that is necessary to correctly use a touted
>>>>>>>>>> feature.
>>>>>>>>>
>>>>>>>>> This issue has its own history. It has been already fixed
>>>>>>>>> for the
>>>>>>>>> JDK 7u for the Java2D demo. It was decided to not bring new API
>>>>>>>>> for the
>>>>>>>>> HiDPI support in the JDK 7.
>>>>>>>>> It was suggested to using code like below to create a
>>>>>>>>> ScalableImage.
>>>>>>>>> ------------------------------
>>>>>>>>> /**
>>>>>>>>> * Class that loads and returns images according to
>>>>>>>>> * scale factor of graphic device
>>>>>>>>> *
>>>>>>>>> * <pre> {@code
>>>>>>>>> * Image image = new ScalableImage() {
>>>>>>>>> *
>>>>>>>>> * public Image createScaledImage(float scaleFactor) {
>>>>>>>>> * return (scaleFactor <= 1) ? loadImage("image.png")
>>>>>>>>> * : loadImage("image at 2x.png");
>>>>>>>>> * }
>>>>>>>>> *
>>>>>>>>> * Image loadImage(String name){
>>>>>>>>> * // load image
>>>>>>>>> * }
>>>>>>>>> * };}</pre>
>>>>>>>>> */
>>>>>>>>> ------------------------------
>>>>>>>>>
>>>>>>>>> It was based on the display scale factor. The scale factor
>>>>>>>>> should be
>>>>>>>>> manually retrieved from the Graphics2D and was not a part of the
>>>>>>>>> public
>>>>>>>>> API:
>>>>>>>>> g2d.drawImage(scalbleImage.getScaledImage(scaleFactor),..).
>>>>>>>>>
>>>>>>>>> From that time it was clear that there should be 2 basic
>>>>>>>>> operations
>>>>>>>>> for the HiDPI images support:
>>>>>>>>> - retrieve an image with necessary resolution
>>>>>>>>> - retrieve images with all resolutions
>>>>>>>>>
>>>>>>>>> The second one is useful when it is necessary to create one
>>>>>>>>> set of
>>>>>>>>> images using the given one (for example, to draw a shape on all
>>>>>>>>> images).
>>>>>>>>
>>>>>>>> For now, these are just ToolkitImages and you can't draw on
>>>>>>>> them, so
>>>>>>>> this is moot for the case of @2x support in getImage().
>>>>>>>>
>>>>>>>>> The JDK 7 solution has been revisited for the JDK 8 and the
>>>>>>>>> neccesary
>>>>>>>>> CCC request 8011059 has been created and approved.
>>>>>>>>>
>>>>>>>>> Both the JDK-8011059 issue description and the approved CCC
>>>>>>>>> request
>>>>>>>>> says:
>>>>>>>>> -----------------------
>>>>>>>>> A user should have a unified way to provide high resolution
>>>>>>>>> images
>>>>>>>>> that can be drawn on HIDPI displays according to the user
>>>>>>>>> provided
>>>>>>>>> algorithm.
>>>>>>>>> -----------------------
>>>>>>>>
>>>>>>>> We've implemented the Hint part of that solution, but the
>>>>>>>> mechanism
>>>>>>>> for creating custom multi-res images was not workable. I no longer
>>>>>>>> sit as the client rep on the CCC or I would have pointed that
>>>>>>>> out, my
>>>>>>>> apologies.
>>>>>>>>
>>>>>>>>> The 8011059 fix consists of two parts:
>>>>>>>>> - Provide a way for a custom image creation that holds images
>>>>>>>>> with
>>>>>>>>> different resolutions
>>>>>>>>> - Load @2x images on Mac OS X
>>>>>>>>>
>>>>>>>>> The first one of the fix can be used without the second. it
>>>>>>>>> is not
>>>>>>>>> difficult to crate just a class which loads @2x images using the
>>>>>>>>> given
>>>>>>>>> file path/url.
>>>>>>>>
>>>>>>>> If we support @2x transparently behind the scenes as we are doing
>>>>>>>> now, who is the requester for the way to create a custom set of
>>>>>>>> resolution variants? I think the most important customer-driven
>>>>>>>> request was to support @2x for the Mac developers, wasn't it?
>>>>>>>>
>>>>>>>>> Now it is suggested to implement the given
>>>>>>>>> MultiResolutionImage
>>>>>>>>> interface and override two methods:
>>>>>>>>> - Image getResolutionVariant(int width, int height)
>>>>>>>>> - List<Image> getResolutionVariants()
>>>>>>>>>
>>>>>>>>> An image with necessary resolution should be drawn
>>>>>>>>> automatically in
>>>>>>>>> SunGraphics2D.
>>>>>>>>>
>>>>>>>>> What we need is to focus on the first part of the fix.
>>>>>>>>> From this point of view it up to the user' code to load all or
>>>>>>>>> some
>>>>>>>>> resolution variants by MediaTracker and using
>>>>>>>>> Component.prepareImage/checkImage methods.
>>>>>>>>
>>>>>>>> This is all an interesting goal, but I'm not sure it is as high a
>>>>>>>> priority if we support a convention (based on platform
>>>>>>>> conventions)
>>>>>>>> for high resolution variants behind the scenes. I do agree
>>>>>>>> that we
>>>>>>>> need something like this before too long, but I don't think it was
>>>>>>>> workable to target the getScaledInstance() method as the
>>>>>>>> mechanism to
>>>>>>>> implement it. That was the only convention that was approved in
>>>>>>>> that
>>>>>>>> CCC request, so a more complete API based on another mechanism is
>>>>>>>> not
>>>>>>>> covered there.
>>>>>>>>
>>>>>>>> Also, Win8 has its own conventions as well which come into play on
>>>>>>>> the new high resolution Surface tablets and we should consider
>>>>>>>> those
>>>>>>>> to guide the API we develop.
>>>>>>>>
>>>>>>>> All, in all, though, I think if we get @2x support in with no code
>>>>>>>> changes needed for apps then we have taken care of the primary use
>>>>>>>> case. We can probably even handle the Win8 conventions behind the
>>>>>>>> scenes in an 8u release...
>>>>>>>>
>>>>>>>> ...jim
>>>>>>>
>>>>>>>
>>>>>>
>>>>>>
>>>>
>>
More information about the macosx-port-dev
mailing list