Re: [OpenJDK 2D-Dev] AWT Dev [8] Review request for 8011059 [macosx] Make JDK demos look perfect on retina displays
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. 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. 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. 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. 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. 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. 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 -
Re: [OpenJDK 2D-Dev] RFR(L) - 2nd round: 8024854: Basic changes and files to build the class library on AIX
On 26 Nov 2013, at 18:08, Iris Clark wrote: So overall it looks good to me and should be pushed to the staging forest once you hear from others that commented previously. I think that means Chris Hegarty, Michael McMahon, and Sergey Bylokhov. Alan, please correct me if I'm wrong. I'm ok with these changes going into the staging forest as are. -Chris. Thanks, iris -Original Message- From: Alan Bateman Sent: Tuesday, November 26, 2013 9:03 AM To: Volker Simonis Cc: Vladimir Kozlov; 2d-dev@openjdk.java.net; serviceability-...@openjdk.java.net; security-dev; ppc-aix-port-...@openjdk.java.net; awt-...@openjdk.java.net; Java Core Libs; net-dev Subject: Re: [OpenJDK 2D-Dev] RFR(L) - 2nd round: 8024854: Basic changes and files to build the class library on AIX On 26/11/2013 16:23, Volker Simonis wrote: Hi, thanks to everybody for the prompt and helpful reviews. Here comes the final webrev which incorporates all the corrections and suggestions from the second review round: http://cr.openjdk.java.net/~simonis/webrevs/8024854.v3/ I've successfully build (and run some smoke tests) with these changes on Linux (x86_32, x86_64, ppc64), Solaris/sparcv9, Windows/x86_64, MacOSX and AIX (5.3, 7.1). I've skimmed over the last webrev with focus on: NetworkingLibraries.gmk where I see this is now fixed for all platforms. net_util.* and the platform specific net_util_md.* where I see you've added platformInit so it's much cleaner. UnixNativeDispatcher.c where the error translation is now removed (and looks fine). So overall it looks good to me and should be pushed to the staging forest once you hear from others that commented previously. -Alan
Re: [OpenJDK 2D-Dev] AWT Dev [8] Review request for 8011059 [macosx] Make JDK demos look perfect on retina displays
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
