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

[Jim Graham]

Hi Anton,

Could the upside-down nature of the pixel readback be solved by reading 
the entire frame in a single operation and then swapping the pixels 
around in our own code?  The memory movement may be faster than the 
overhead of H calls to glReadPixels.  (In the long run, we might want to 
teach the rest of our code how to deal with upside-down image data as 
well and then we don't have to swap it...)

			...jim

On 1/21/14 5:29 AM, Anton V. Tarasov wrote:
> Hi all,
>
> Let me please resume the review process.
>
> With the new webrev:
>
> http://cr.openjdk.java.net/~ant/JDK-8029455/webrev.3
>
> I'm addressing the last concern which was about "leaking" the internal
> OffscreenHiDPIImage to the public via RepaintManager.getOffscreenBuffer.
>
> The explanation will follow, but before that I'd like to share the info
> related to the volatile buffer performance issue (which I was talking
> about before). I did some investigations of the native side and figured
> out the real source of the performance drop. It's the code where
> glReadPixels is called to read _every_ scanline of the image. This is so
> because of the nature of the OGL coordinate space which is upside down
> comparing to the j2d space. Please find more details here:
> https://javafx-jira.kenai.com/browse/RT-30035?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=380146.
> If I'm not mistaken, we can do nothing about it.
>
> So, Swing/Interop can't use a volatile image as a back buffer, and it
> should use a buffered image or no back buffer at all. (Otherwise,
> performance is not acceptable).
>
> Now, to the fix. What I did is I added a "hidpiEnabled" property to the
> OffscreenHiDPIImage class. When it's "true" (by default) the image
> returns its size in layout space (just like in the previous version),
> when it's "false" the image returns its size in physical space (just
> like an ordinary BufferedImage). In RepaintManager I set the image
> "hidpi disabled", thus hiding its layout size from the developer . The
> property is taken into account in SunGraphics2D.isHiDPIImage(). Because
> an OffscreenHiDPIImage with hidpiEnabled==false is drawn as an ordinary
> image, I draw it via drawImage(img, x, y, width, height) in RepaintManager.
>
> Why I still use the OffscreenHiDPIImage class instead of a BufferedImage
> is because otherwise I'd have to do pretty the same coding, but it would
> be scattered. The class basically incapsulates the following logic:
>
> - Keeps layout size of the image. (Used in drawImage.)
> - Keeps the scale factor. (Used by
> SurfaceData/VolatileImage/GraphicsConfig.)
> - Overrides SurfaceData.getDefaultScale. The point is that I can't
> simply call Graphics2D.scale(s, s) as this won't work when Swing draws
> into the image. (SunGraphics2D asks SurfaceData for the scale).
> - Overrides VolatileImage to make it return a scaled BI as its backup.
> (Used by Nimbus.)
> - Overrides GraphicsConfiguration to let it access the BI and its scale
> factor. (Used by Nimbus. This could be implemented otherwise, but not
> much better).
>
> No more changes in the current version. I know there're some concerns
> related to detecting a device change, but let me address it after we
> come to agreement about the approach discussed above.
>
> Thanks,
> Anton.
>
>
> On 18.12.2013 5:03, Jim Graham wrote:
>> Hi Anton,
>>
>> javax.swing.RepaintManager.getOffscreenBuffer is a public method that
>> can now return one of the new HiDPI offscreen images which is a
>> subclass of BufferedImage.  This was what I was worried about in terms
>> of one of these internal double buffers leaking to developer code.  If
>> they test the image using instanceof they will see that it is a
>> BufferdImage and they may try to dig out the raster and get confused...
>>
>>             ...jim
>>
>> On 12/17/13 10:21 AM, Anton V. Tarasov wrote:
>>> Hi all,
>>>
>>> Please look at the new version:
>>>
>>> http://cr.openjdk.java.net/~ant/JDK-8029455/webrev.2
>>>
>>> It contains the following changes:
>>>
>>> - All the scale related stuff is moved to the new class:
>>> OffScreenHiDPIImage.java
>>>
>>> - JViewport and RepaintManager no longer cache buffers.
>>>
>>> - JLightweightFrame has new method: createHiDPIImage(w, h).
>>>
>>> - JViewport, RepaintManager and AbstractRegionPainter goes the new path
>>> to create a HiDPI buffered image.
>>>
>>> - A new internal property is added: "swing.jlf.hidpiImageEnabled". False
>>> by default. It makes JLF.createImage(w, h) forward the call to
>>> JLF.createHiDPIImage(w, h). This can be used by a third party code in
>>> case it creates a buffered image via Component.createImage(w, h) and
>>> uses the image so that it can benefit from being a HiDPI image on a
>>> Retina display.
>>>
>>> For instance, SwingSet2 has an animating Bezier curve demo. Switching
>>> the property on makes the curve auto scale smoothly. Please, look at the
>>> screenshots:
>>>
>>> -- http://cr.openjdk.java.net/~ant/JDK-8029455/RoughtCurve.png
>>> -- http://cr.openjdk.java.net/~ant/JDK-8029455/SmoothCurve.png
>>>
>>> - SunGraphics2D now draws a HiDPI buffered image the same way it draws a
>>> VolatileImage.
>>>
>>> - I've removed the copyArea() method from the BufImgSurfaceData, and
>>> modified the original version. The only question I have is: do I need to
>>> check for "instanceof OffScreenHiDPIImage.SurfaceData" in case when
>>> "transformState == TRANSFORM_TRANSLATESCALE"? If this method is invoked
>>> with some other SD, and the transform is SCALE, will it do the job with
>>> the coordinates conversion done?
>>>
>>> - I've left the new methods in FramePeer default... May yet we implement
>>> them in other peers when we really need it?
>>>
>>> - CPlatformLWWindow.getGraphicsDevice() checks for an intersection +
>>> scale. This heuristic actually may fail when a Window is moved b/w three
>>> or four displays so that it intersects them all at some time. JFX will
>>> set a new scale factor in between and AWT may pick up a wrong device. I
>>> don't know any simple solution for that. For two monitors this will
>>> work.
>>>
>>> Thanks,
>>> Anton.
>