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

[Anton V. Tarasov]

[cc'ing to j2d]

On 11.12.2013 23:14, Anthony Petrov wrote:
> Hi Anton,
>
> On 12/11/2013 02:40 PM, Anton V. Tarasov wrote:
>>> 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...
>
> Thanks for the clarification. The idea sounds reasonable indeed.
>
> I've also read your reply to Jim and I'm now concerned about the fact that scaled images can 
> report larger dimensions than a Graphics created for them would allow one to draw to them. This 
> may be a problem for some apps that perform rendering based on the dimensions of a BI object 
> passed to them.
>
> Shouldn't the BI.getWidth/Height() methods always report the logical size of the image, which is 
> equal to the physical one for scale == 1?

That's probably worth discussing. Let me move this discussion to another thread.

Thanks,
Anton.

>
> -- 
> best regards,
> Anthony
>
>>
>>>
>>>
>>>
>>> 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.
>>>>
>>