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

[Anton V. Tarasov]

On 11.12.2013 14:38, Sergey Bylokhov wrote:
> On 11.12.2013 13:18, Anton V. Tarasov wrote:
>> Hi Sergey,
>>
>> On 11.12.2013 3:26, Sergey Bylokhov wrote:
>>> Hi, Anton.
>>> My expectation was that everything should work automatically, if you get correct CGraphicsDevice 
>>> for the embedded swing window + some tweaks in the code related to the peer and CGLSurfaceData/
>>>
>>> Why we need all this change in 
>>> CGraphicsDevice,CGLGraphicsConfig,OffScreenImage,CPlatformLWView,JViewport,RepaintManager.
>>
>> With Nimus, at some moment, when the nimbus.AbstractRegionPainter.paint(Graphics2D g, ...) method 
>> is called it is passed the graphics instance created by JLF.createGraphics() which derives it 
>> from the JLF's root buffered image. Then, somewhere up the stack the method calls for 
>> getImage(g.getDeviceConfiguration(),..),
> Yes, correct. But you can created double sized surface for you buffered image(in the same way as 
> it was done for volatile) and provide correct DeviceConfiguration for it.

Ok, I see what you're talking about, let me try it.

>> where the graphics conf is of BufferdeImageGraphicsConfig type. This flows into 
>> GraphicsConfiguration.createCompatibleVolatileImage(int w, int h, ...) and then into 
>> BufImgVolatileSurfaceManager which doesn't support acceleration, and eventually into 
>> BufferedImageGraphicsConfig.createCompatibleImage(int w, int h, ...). The [w, h] values passed 
>> here represent a logical size of a Nimbus ui element. Unless I scale the size of the requested 
>> image here, the ui element is shown stretched. That's why the changes in 
>> BufferedImageGraphicsConfig.
>>
>> With Aqua, I don't observe calls into BufImgVolatileSurfaceManager. I suppose the reason is that 
>> a GraphicsConfiguration instance is always taken from a component (from the JLF's hierarchy) 
>> which is tight to a CGraphicsDevice and has a CGLGraphicsConfig, not BufferdeImageGraphicsConfig. 
>> CGLVolatileSurfaceManager enables acceleration, and so the process of a volatile image creation 
>> never falls back to CLGGraphicsConfig.createCompatibleImage(...). By "never" I mean that I didn't 
>> catch it with the tests I ran. However, "find usages" shows that these methods are directly 
>> called from CTrayIcon, CDragSourceContextPeer, CCustomCursor, at least. I rather should cover 
>> those areas with some tests as well. Anyway, I've made these methods 
>> (CLGGraphicsConfig.createCompatibleImage(...)) consistent with the 
>> BufferedImageGraphicsConfig.createCompatibleImage(...) methods and with the general idea I've 
>> described before.
>>
>> Then, the other files you're referring to:
>>
>> - CPlatformLWView
>>
>> As I explained before, AWT in the lw embedding mode can't match a window to the display it is 
>> showing on, simply because it doesn't have a platform window underneath. 
>> CPlatformView.nativeGetNSViewDisplayID(getAWTView()) returns zero, and so 
>> CPlatformView.getGraphicsDevice() returns default device, not necessarily matching the scale 
>> factor of the current device.
> Why? You can try to check it youseft via CGLGraphicsConfig.getBounds()+Peer.getBounds(); You need 
> to override CPlatformWindow.getGraphicsDevice() and return correct device.  I see that the similar 
> bug exists in applets where CPlatformEmbeddedFrame.getGraphicsDevice always return  default device.

Probably, I could override CPlatformWindow.getGraphicsDevice() instead of 
CPlatformView.getGraphicsDevice() (the former calls the latter). But even then, how can I use the 
CGLGraphicsConfig.getBounds() which uses a display id which I don't have? And why it's better to 
pick the device via bounds than pick the device via a scale factor (the property I only need)?

>
> Same question about Component.getLocationOnScreen() is working in SwingNode?

SwingNode updates JLF location every time it changes on the fx side. So, the location as well as the 
scale is what the host (SwingNode) provides.

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

>>
>>>
>>> One unrelated question. Did you try to use CALayer's embedding mechanics? Probably it is 
>>> possible to add CAlayer which is used by the swing and awt to the FX CAlayer? In this case all 
>>> problems related to the painting goes away and it will be much faster, only events should be 
>>> generated(The same way our plugin works see CPlatformEmbeddedFrame).
>>
>> This is in plans (interop "unified rendering" for d3d, ogl). At least, there are plans to 
>> investigate it....
> I guess it could be simpler than the current solution, because different layers will have 
> different context which will be used by the swing and fx independently.

I can't estimate the complexity without any investigations done... So I can't say if it may be 
simpler than it is now, I'd anticipate pitfalls.

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