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

[Anton V. Tarasov]

Hi Anthony,

On 10.12.2013 23:57, Anthony Petrov wrote:
> 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.

Oh, this is a miss. I should have put it into CPlatformLWWindow. Thanks for noticing!

>
>
> 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.

There are two options. 1) Follow to your logic, that is to fix every place in AWT/Swing where 
BufferedImage is created. In which case in every such place we will have to get a current scale 
factor from the context. 2) Don't touch that code, but solve the task in some generic way, the way I 
tried to implement, when a buffered image is created with an extended size right in the factory methods.

The logic of the first approach is simpler. However, it would require lots of modifications (in the 
L&F code, and not only there). And it would require to take into account the scale factor every time 
a new buffered image case is added to the code. Still, this is possible.

With the scond approach I don't need to bother about that code, I just create scaled images "on the 
fly".

 > I think that the image shouldn't be aware of any scale.

The "scale" field put into the BufferedImage class means that an image instance should (or 
shouldn't) be treated as a HiDPI image by the Graphics.drawImage(). So, this is a kind of a special 
"scale" case, aimed at supporting Retina technology. Probably it deserves a better name, 
"hidpiScale" or something. So, it's not that the image has been scaled by the user to be drawn on a 
lager area, but that the image should (or shouldn't) be scaled just to "look smoothly" on a Retina 
display. That's what I was thinking about...

>
>
>
> 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.

Ok.

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

Thanks for the review!

Anton.

>
> -- 
> 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.
>>