<Swing Dev> <AWT Dev> [9] Review Request: JDK-8029455 JLightweightFrame: support scaled painting

[Anthony Petrov]

Hi Anton,

I'm not an expert in HiDPI rendering, so I'll defer to Jim and Sergey to 
comment on the core changes. I still have a few comments regarding the fix:

1. src/macosx/classes/sun/lwawt/macosx/CPlatformWindow.java
>  265         contentView.initialize(peer, null);

Are you sure calling view.initialize() from CPW.initializeBase() doesn't 
introduce any unwanted side-effects? Interestingly, the contentView 
field gets re-written and re-initialized in the CPW.initialize() method 
after calling the initializeBase(). Seems very strange to me.


2. I'm not sure if adding the scale field to the BI is a good idea. I 
think that the image shouldn't be aware of any scale. After all, it's 
just an image, a bitmap. It has its real dimensions corresponding to the 
actual size of the image stored in RAM. Whether this image is going to 
be represented as a scaled image is something that a code that uses the 
image should be concerned with, not the image itself.


3. src/share/classes/java/awt/peer/FramePeer.java
>  139     default void notifyScaleFactorChanged() {}

I think this deserves to be declared in WindowPeer so that we could use 
it w/o additional modifications in the future if we add support for 
public notifications about the scale factor changes.


4. I'm CC'ing swing-dev@ and Alexander Scherbatiy to review changes in 
the JViewport class and other Swing classes.

--
best regards,
Anthony

On 12/10/2013 06:22 PM, 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.
>