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

[Sergey Bylokhov]

On 11.12.2013 20:23, Anton V. Tarasov wrote:
>
>>> - CGraphicsDevice
>>>
>>> This setter is only called from CPlatformLWView.getGraphicsDevice(). 
>>> I've explained it in my previous message. It's needed to change the 
>>> scale factor of the default device when no device in the list fits. 
>>> The case is impossible with the current implementation of SwingNode 
>>> (which only passes JLF a scale factor matching one of a real 
>>> display), however, as JLF provides a generic lw embedding API, I 
>>> should cover that case as well.
Not sure that matching fx to awt devices via scale is not a good idea. 
Note that it is expected that fields in the CGraphicsDevice chenges only 
if the screen is changed/added/removed.
Probably Instead of notifyScaleFactorChanged you can notify about 
screens changes?
>>>
>>> - OffScreenImage
>>>
>>> I've put a BufferedImage accessor there, nothing else. I didn't find 
>>> a better place... (I'd appreciate showing it).
>>>
>>> - JViewport, RepaintManager
>>>
>>> These classes create a double buffer. In case the buffer is backed 
>>> by a BufferedImage, it will be created with the current scale factor 
>>> set. The buffer won't be changed when a user moves the host window 
>>> across multiple screens with different scales. I see two options. 1) 
>>> Drop the double buffer reference every time the scale changes (in 
>>> that case, the buffer will be recreated every time, I cross a 
>>> screen) 2) Create a map which will cache the buffers (say, for 1 and 
>>> 2 scale factors for double screen env). I think the second approach 
>>> is better.
>>>
>>> > Probably it will be better to disable doublebuffering and 
>>> SwingPaintEventDispatcher completely(see swing.showFromDoubleBuffer)?
>>>
>>> Why? If we can manage it for JLF/SwingNode, why should we downgrade 
>>> performance?
>> You have 1 buffere on fx side, buffer in SwingNode, buffer in 
>> jviewport, and swing itself use double buffering.
>
> Ok, this is a good point. But still I can't simply switch off double 
> buffering w/o doing any benchmarking. SwingNode perf analisys & 
> improvement is in plans...
It would be good to know results of the benchmarks.
> So far, unless it requires lots of coding (but it doesn't) I'd prefer 
> to keep that option working.
>
>>>
>>>> Actually I still do not understand why JViewport works in the 
>>>> standalone application.
>>>
>>> Could you please clarify, I don't understand this question...
>> I see that JViewport use Offscreen image as a double buffer, is that 
>> true that it use it in the standalone swing application? If yes why 
>> it works.
>
> JViewport.paint() is not called with its default blit mode, and so it 
> doesn't actually use an OffscreenBuffer. For JLF, the mode is set to 
> backing store. If I set the backing store mode in standalone swing, 
> JViewport stops scaling on Retina. So, it didn't work before.
Is this related to the JDK-8023966?
>
>
> Thanks,
> Anton.
>
>>
>>>
>>> Thanks for the review!
>>>
>>> Anton.
>>>
>>>>
>>>> On 10.12.2013 18:22, 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.
>>>> Does it mean that all methods related to the 
>>>> Component.getLocationOnScreen() does not work?
>>>>>
>>>>> 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.
>>>> Why? You can try to check it youseft via 
>>>> CGLGraphicsConfig.getBounds()+Peer.getBounds();
>>>>> 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.
>>>>>
>>>>
>>>>
>>>
>>
>>
>


-- 
Best regards, Sergey.