Re: [OpenJDK 2D-Dev] [8] Review request for 8011059 [macosx] Make JDK demos look perfect on retina displays

[Alexander Scherbatiy]

  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

    - 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("im...@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