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

[Anton V. Tarasov]

On 12/13/13 2:05 AM, Anthony Petrov wrote:
> 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.

I'm not against the idea of using the right device internally (I just 
don't like the idea to add a notifyScreenChanged method). If this really 
can be implemented by means of the comparing the coordinates, then I'll 
do that.

Thanks,
Anton.
>
> -- 
> 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.
>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>>
>>>>
>>>
>>>
>>