Re: [9] Review Request: JDK-8029455 JLightweightFrame: support scaled painting

[Anton V. Tarasov]

Hi Jim,

On 11.12.2013 13:14, Jim Graham wrote:
The "inCreateCI" field is not MT-safe.  BufferedImage objects don't have any threading 
restrictions so a solution that is MT-safe is required there in the BufferedImageGraphicsConfig 
object.  I also think that any device-associated GraphicsConfig objects should be MT-safe in those 
methods as well - I'm not sure if we've ever documented otherwise, but those methods have tended 
to be harmless to call on any thread and fairly far removed from the more platform/screen 
intensive parts of the AWT that might assume thread safety.

Right, I didn't take into account that a GraphicsConfig don't have threading restrictions. Will have 
to fix that.



In BufferedImage, the private fields and methods will require an accessor method for the inner 
class to access them.  Is there a reason they can't be left package-accessible?

Sorry, I don't clearly understand what you mean... The "scale" private field and the new methods are 
accessed from other packages, so I can't make them package-private. I've added the accessor which I 
put into OffscreenImage (an internal class).



In JViewport, is there a reason why a map of various scaled backing stores are kept rather than 
just validating the existing backing store against the new desired scale and replacing it when it 
changes?  Does the scale on the backing store ping-pong back and forth between scales much?

This doesn't ping-pong much, indeed. So you want to say using a cache here is not justified? Ok, why 
I did that is probably to mimic the logic used by RepaintManager which does use a map b/w graphic 
configs and volatile images (the volatileMap field), whereas changes of the GraphicsConfig shouldn't 
occur often.

Anyway, this indeed doesn't increase performance but increases a footprint, so I'm ok not to use 
cache here.



BufImgSurfaceData - you have to validate the transform as not flipping or rotating (even quadrant 
rotation will violate your conditions.  I believe that testing the transform type with a mask 
consisting of the TRANSLATE and SCALE_MASK flags should be fine. Also, do we wan to set a 
precedent of allowing copyArea under an arbitrary scale, or should we test it to make sure it is 
specifically the same scale as the device scale factor?

Ok, I'll check the transform for TRANSLATE/SCALE only. The same scale as the device works for me, so 
I'll limit it to that case.


SG2D - I'm worried about the performance impliciations of adding a new method in that creates and 
returns an array on every drawImage call. Did you do a performance test with J2DBench to verify 
the impact of these operations?

Well, this was done just to make the code more clear. I can remove that method (and do the 
instanceof check in every place I called it), or as an alternative I can cache the array so that not 
to create it (out of the stack) every time. Does this make sense to you? (I didn't run J2DBench yet).



With the new support for scaled BufferedImages we now have a situation where some images return 
their logical dimensions from getWidth/Height(null) and some return physical dimensions.  That's a 
little dicey as the method now has two meanings depending on how the images were created.  We're 
sort of between a rock and a hard place in that BufferedImage objects have a historical context 
for how their values relate to the pixels you can access, but we also have a relationship between 
how the return values of getWidth/Height(observer) relate to the size the image is drawn on the 
screen.  Is this only ever done for the backing store objects?  Do we ever expose them to developers?

Well, this is the question I was waiting for... The methods getWidth/Height will continue to return 
the physical size of an image, regardless of was it scaled or not. However, for a scaled image the 
physical size, and so the returned values will be auto-scaled. This is applicable to the following 
"factory" methods I've overriden:

1) BufferedImageGraphicsConfig.createCompatibleImage(...)
2) CGLGraphicsConfig.createCompatibleImage(...)
3) LWLightweightFramePeer.createImage(...)

The 1st depend on the scale factor of the GC instance the method is called on. The scale factor of 
BufferedImageGraphicsConfig is taken from the BI instance passed to its ctor. So, this won't affect 
any third party code, as the BI.scale field has a private access.

The 2nd depend on the scale factor of the device, which is set either by the platform or by me (in 
case when I set it to the default device in CPlatformLWView.java). So, theoretically it is possible 
for a user to call this method on a Retina display and get a scaled image.

The 3rd depend on the scale factor taken from the JLightweightFrame instance. This affects only the 
code run inside the JLF, however a user is able to call this method (on any of a component from the 
JLF's hierarchy) and get a scaled image.

So, we have situations when a user might create a BufferedImage with w*h size but get back an image 
with w*h*4 size (on a Retina display). And this might be unexpected. However, this image will 
produce a "context" with the same scale factor. A BufferedImageGraphicsConfig will be created with 
the same scale (see its ctor), a BufImgSurfaceData will be created with the same scale (see its 
getDefaultScale() method), and a SunGraphics2D will be created with the same scale ("devScale = 
sd.getDefaultScale();" in its ctor). In this case, all drawings into the image will take the scale 
into account, and should be correct. The only concern is the size of the image itself. At least, 
this is not documented...

>   Could this be implemented by having the code that renders the backing store state its explicit 
on-screen size so that we don't need to add any computations to the use of getWidth/Height(null) to 
SG2D?

The computations we do in SG2D, namely the getLogicalSize(Image) method, is the way to get the 
"logical" size of a BufferedImage (the size it would have not being scaled) which returns its 
physical size via its getWidth/Height methods. So, the problem is that the image doesn't keep its 
logical size, but its physical (prescaled) width/height values and the scale factor.

Thanks for the review!

Anton.


I'm going to take a break here and hit send to get these concepts out into the discussion before I 
look too much further...

            ...jim

On 12/10/13 6:22 AM, Anton V. Tarasov wrote:
Hi Jim, Sergey and All,

Please review the fix that adds support of Retina displays to
JLightweightFrame (which javafx SwingNode is based on).

webrev: http://cr.openjdk.java.net/~ant/JDK-8029455/webrev.1
jira: https://bugs.openjdk.java.net/browse/JDK-8029455

(After the fix goes into jdk9 it should be ported to 8u20 as well,
because the functionality is essential for SwingNode.)

The general idea of the fix is as follows.

A BufferedImage instance, being created in the context in which the
scale factor is determined and is different from one, is automatically
created with appropriately extended size. The image itself becomes a
scaled image (a "scale" private field is set on it). By the "context" I
mean the circumstances where the BufferedImage is related to a
JLightweightFrame, a GraphicsConfiguration, a SurfaceData, or a
GraphicsDevice which determine the scale factor.

Here are the related changes:

-
http://cr.openjdk.java.net/~ant/JDK-8029455/webrev.1/src/share/classes/java/awt/image/BufferedImage.java.udiff.html 


-
http://cr.openjdk.java.net/~ant/JDK-8029455/webrev.1/src/share/classes/sun/awt/image/OffScreenImage.java.udiff.html 


-
http://cr.openjdk.java.net/~ant/JDK-8029455/webrev.1/src/share/classes/sun/swing/JLightweightFrame.java.udiff.html 

(the resizeBuffer method)
-
http://cr.openjdk.java.net/~ant/JDK-8029455/webrev.1/src/macosx/classes/sun/lwawt/LWLightweightFramePeer.java.udiff.html 


-
http://cr.openjdk.java.net/~ant/JDK-8029455/webrev.1/src/share/classes/sun/awt/image/BufferedImageGraphicsConfig.java.udiff.html 


-
http://cr.openjdk.java.net/~ant/JDK-8029455/webrev.1/src/macosx/classes/sun/java2d/opengl/CGLGraphicsConfig.java.udiff.html 



The "scale" value of a BufferedImage is used when 1)
BufferedImageGraphicsConfig is created 2)
BufImgSurfaceData.getDefaultScale() is called:

-
http://cr.openjdk.java.net/~ant/JDK-8029455/webrev.1/src/share/classes/sun/awt/image/BufferedImageGraphicsConfig.java.udiff.html 


-
http://cr.openjdk.java.net/~ant/JDK-8029455/webrev.1/src/share/classes/sun/awt/image/BufImgSurfaceData.java.udiff.html 



The former is used in the GraphicsConfiguration.createCompatibleImage()
calls, and the latter is used in SurfaceManager.getImageScale(Image):

-
http://cr.openjdk.java.net/~ant/JDK-8029455/webrev.1/src/share/classes/sun/awt/image/SurfaceManager.java.udiff.html 



A scaled BufferedImage is supported by the SunGraphics2D.drawImage()
primitives. Here's the pattern of how the image may be created and drawn:

int scale = <get the scale factor from the context>;
BufferedImage img = new BufferedImage(width * scale, height * scale, ...);
img.setScale(scale); // an accessor is currently used instead
<...>
g2d.drawImage(img, x, y, ...); // 1) draw the image with auto-scale
g2d.drawImage(img, x, y, dw, dh, ...) // 2) draw the image into a
specified rect

In the first case, if the BufferedImage is created with an extended
size, the "scale" value of the image matters, it should be drawn as a
HiDPI image.
In the second case, if the BufferedImage is created with an extended
size, the "scale" value of the image doesn't matter (it may not be
evidently set) as the image will anyway be scaled from its physical
bounds into provided logical bounds. This all should (as I suppose)
provide backward compatibility for buffered images that were created in
their logical bounds or without setting the "scale" field. For instance,
the AquaPainter.paintFromSingleCachedImage(...) method creates & draws
an image as follows:

int scale = ((SunGraphics2D) g).surfaceData.getDefaultScale();
int imgW = bounds.width * scale;
int imgH = bounds.height * scale;
BufferedImage img = new BufferedImage(imgW, imgH, ...);
<paint into the img>
g.drawImage(img, bounds.x, bounds.y, bounds.width, bounds.height, null);

Here, the img.scale value is not set (I didn't modify this code), and
SunGraphics2D doesn't treat the image as a HiDPI image, however it is
drawn as expected. An alternative way to draw the image would be:

int scale = ((SunGraphics2D) g).surfaceData.getDefaultScale();
int imgW = bounds.width * scale;
int imgH = bounds.height * scale;
BufferedImage img = new BufferedImage(imgW, imgH, ...);
img.setScale(scale);
<paint into the img>
g.drawImage(img, bounds.x, bounds.y, ...);

The result would be the same.

-
http://cr.openjdk.java.net/~ant/JDK-8029455/webrev.1/src/share/classes/sun/java2d/SunGraphics2D.java.sdiff.html 



The following changes:

-
http://cr.openjdk.java.net/~ant/JDK-8029455/webrev.1/src/macosx/classes/sun/lwawt/macosx/CPlatformLWView.java.udiff.html 



are defined by this logic. Running Swing via JLightweightFrame (JLF)
makes it "display agnostic". Swing is painted to an off-screen buffer
and it's the host (e.g. SwingNode) that renders the buffer on a
particular device. So, the host should detect the scale of the current
display and set it on JLF.

However, AWT in order to paint to a volatile image requires
CGraphicsDevice and CGLSurfaceData to be created. By default AWT creates
CGraphicsDevice instances matching all the detected display devices
(CGraphicsEnvironment.initDevices()). But, as JLF doesn't have any
platform window behind it, AWT can't match JLF to the exact device it's
currently displayed on. So, on the one hand, AWT doesn't know which
device is current and what is the current scale (the host passes this
value), but from the other hand, AWT has a list of all the
CGraphicsDevice instances.

I tried to leverage from that fact. The
CPlatformLWView.getGraphicsDevice() method takes the current scale from
the JLF instance, and then tries to match it to an existent device from
the list. In case it can't find a device with the specified scale (which
should not actually happen, unless the host passes an arbitrary scale
value, which is not the case for SwingNode) it takes a default device
and changes its scale forcedly. I'm not sure if I should create a new
dummy device instance instead. The scale factor of the device (which is
then propagated to CGLSurfaceData on its creation) is the only info that
JLF will take from the device to create a scaled volatile image.

The following changes:

-
http://cr.openjdk.java.net/~ant/JDK-8029455/webrev.1/src/share/classes/javax/swing/JViewport.java.udiff.html 


-
http://cr.openjdk.java.net/~ant/JDK-8029455/webrev.1/src/share/classes/javax/swing/RepaintManager.java.udiff.html 



were made to map a backing store image to a scale factor.

The JViewPort.paint(...) method calls SunGraphics2D.copyArea(...) on
scrolling. The method was not implemented for a graphics with a scale
transform and a BufImgSurfaceData (it threw exceptions). I took that
code, copied it to the BufImgSurfaceData.copyArea(...) and added a
general translation for the coords:

-
http://cr.openjdk.java.net/~ant/JDK-8029455/webrev.1/src/share/classes/sun/awt/image/BufImgSurfaceData.java.udiff.html 



It works, but I'm not sure the implementation is eligible (I don't know
the details of the Blit class, at least it warns not to use the same
source and dest).

The rest of the changes (not covered here) should be clear.

Testing:

- Using jfc/SwingSet2 and jfc/Java2D demos (in a standalone mode &
embedded into SwingNode [1]).
- Testing both Nimbus and Aqua L&F.
- Setting swing.volatileImageBufferEnabled=false/true for all combinations.

Currently, I see no regressions and no visual issues comparing a
standalone mode and a SwingSet mode.

At the end, I suspect there may be some intersection b/w this fix and
the fix which introduced MultiResolutionToolkitImage. Unfortunately, I
didn't yet read that review saga... Please tell me if I should
incorporate anything from that fix.

Thanks,
Anton.

[1] There's a SwingSet part of the fix which I'm going to post to the
jfx alias separately.