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

[Anthony Petrov]

On 12/12/2013 07:18 PM, Anton V. Tarasov wrote:
> [cc'ing to j2d alias]
>
> On 11.12.2013 21:29, Sergey Bylokhov wrote:
>> 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?
>
> JLightweightFrame is a toplevel that paints its content to an off-screen
> buffer, so it is conceptually not associated with any screen. The host
> (SwingNode) application communicates with JLF on an API level.
> Introducing a notion of a "screen" to the API doesn't correlate with the
> JLF's concept, imho.

Why? IMO, this would simplify the code significantly as Swing is already 
HiDPI-aware. It only needs to be able to determine the scale factor of a 
screen device its top-level is on. Of course, the code in the JLF 
implementation needs to know that too, so that it's able to create a BI 
of appropriate dimensions. So making JLF tracking its current 
GraphicsConfiguration looks like a reasonable idea to me. As Sergey 
suggested, this could be done easily by checking intersections between 
JLF's bounds in screen coordinates and the bounds of all the available 
GraphicsDevices.

--
best regards,
Anthony



>
> Why I'm still picking the device is because this seemed to me an
> acceptable approach that integrates with LWAWT smoothly. But I agree
> with you that matching the device via a scale factor is not a good idea.
> Theoretically I can pickup wrong device, but even then it won't change
> anything for me. I just need a device with the requested scale.
>
> What do you think then if we always use default device, for which we
> will change the scale?
>
>>
>>>>>
>>>>> - 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.
>
> Ok, but as this is a separate task I'd like to know what we're fighting
> for. Is the goal to avoid creating BufferedImage's at all?
>
>>> 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?
>
> Right.
>
> Thanks,
> Anton.
>
>>>
>>>
>>> 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.
>>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>
>>>>
>>>
>>
>>
>