<AWT Dev> [OpenJDK 2D-Dev] [8] Review request for 8011059 [macosx] Make JDK demos look perfect on retina displays
Jim Graham
james.graham at oracle.com
Mon Dec 2 13:16:14 PST 2013
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?
> 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
>> 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 awt-dev
mailing list