[OpenJDK 2D-Dev] [9] Review request for 8073320 Windows HiDPI Graphics support

[Jim Graham]

I only read the code portions I commented on below and the ones you 
indicated you had changed.  I'm assuming there are no other code changes 
in this webrev...?

This is getting really close to final which is good since I'll be 
talking about this in my JavaOne talk next week... ;)

In SG2D.scaleImage - the branch that uses transformImage uses the raw 
transform which is expecting to transform an unscaled image.  You'll 
need to modify the transform accordingly so the result doesn't end up 
double-sized.

BIGC, line 73 - shouldn't you pass in scaleX,scaleY to the constructor?

Win32GD.c, lines 90,91 - I don't think I was clear below - is there a 
need to have these initialized inline here with "= 1.0f" when they are 
initialized in the constructor anyway?  Are we worried that they'll be 
accessed before the initScaleFactors() call can set them to appropriate 
values (in which case we should then be worried that they'll be the 
hard-coded 1.0 value instead of the "real" value)?

Win32GD.c - I think I'm going to add an optional "x" to the scale 
factors in JavaFX for completeness.  Thus:
     scale=Ndpi means scale factor of N/96
     scale=N%   means scale factor of N/100
     scale=N.Mx means scale factor of N.M
     scale=N.M  also means scale factor of N.M (x is implicitly assumed)
The reason for this is both to have an explicit token for the regular 
scale factor and also so that this matches with the default image suffix 
of "@2x" so that we can add more ways to provide alternate media such as 
"@120dpi" which are consistent with the values we use here.

BTW, we need to discuss ways to automate loading alternate media beyond 
@2x at some point...

			...jim

On 10/21/15 6:34 AM, Alexander Scherbatiy wrote:
>
>   Could you review the updated fix:
>     http://cr.openjdk.java.net/~alexsch/8073320/webrev.01/
>
>   - xform is concatenated with transform in SG2D.getResolutionVariant()
> method
>   - imagepipe.transformImage() is used for drawImage(Image,
> AffineTransform,...) case
>   - scale factors are made final in BufImgSurfaceData
>   - two arrays are used for configs caching in BIGC
>   - scale factors intialized from properties are moved to the static
> block in Win32GD
>
> On 10/10/2015 3:14 AM, Jim Graham wrote:
>> Hi Alexandr,
>>
>> Just 2 potential issues I found:
>>
>> SG2D.java - getResolutionVariant() doesn't take a transform so it
>> can't take into account the scaling in the transform handed to
>> drawImage(img, xform). We can leave that as a follow-on bug fix (it
>> won't do bad things, but it may not pick the best resolution source
>> image).
>>
>> WGLOffscreenSD (in WGLSD.java) - does peer.getBounds() cache the bounds?
>
>     No. peer.getBounds() directly calls targetComponent.getBounds()
> which returns a copy of original bounds.
>
>    Thanks,
>    Alexandr.
>> If so, then every time we call get bounds on the WGLWSD we will
>> permanently modify the peer's bounds. Would it be better to not make
>> the assumption and just make a protected copy here (perhaps with scale
>> != 1?).
>> WGLWindowSD - same comment
>> D3DWindowSD - same comment
>> GDIWindowSD - same comment
>>
>> The rest of these are tweaks and optimizations.
>>
>> BISD.java - why do you have default initializers for the scale fields?
>> (There are only 2 constructors to cover.)
>>
>> BIGC.java - [2][NUMTYPES] would involve a lot fewer objects than
>> [NUMTYPES][2]...
>> BIGC.java - Or...  If we only have 2 variations of each type, why not
>> just have 2 static arrays instead of a double-indexed array?
>>     int index = (scaleX && scaleY) ? 0 : 1;
>> becomes:
>>     BIGC configarray = (scaleX && scaleY) ? standardArray : scaledArray;
>>
>> SG2D.java - Another way of dealing with transform in scaleImage would
>> be to make a copy of the incoming transform and adjust it to match the
>> request, as in:
>>     // You can probably assert that dxywh == 0,0,imgW,imgH, or:
>>     AffineTransform renderTX;
>>     if (dxywh == 0,0,imgW,imgH) {
>>         renderTX = xform;
>>     } else {
>>         // Should never happen in practice...
>>         AffineTransform renderTX = new AffineTransform(xform);
>>         renderTX.translate(dx1, dy1);
>>         renderTX.scale((dx2 - dx1) / imgW, (dy2 - dy1) / imgH);
>>     }
>>     double-nested-try {
>>         imagepipe.transformImage(..., renderTX);
>>     } catch (InvalidPipe...) {...}
>> It would be more code, though, since you'd have to have 2 sets of
>> "double-nested-try-catch(InvalidPipe)" blocks.
>>
>> Win32GD.java - is there a need to have static initializers for the
>> scale fields?
>>
>> I'll leave the native code review to someone more familiar with that
>> code base...
>>
>>             ...jim
>>
>> On 9/22/15 2:33 AM, Alexander Scherbatiy wrote:
>>>
>>> Hello,
>>>
>>> Could you review the fix:
>>>    bug: https://bugs.openjdk.java.net/browse/JDK-8073320
>>>    webrev: http://cr.openjdk.java.net/~alexsch/8073320/webrev.00
>>>
>>>    This is an initial part of the HiDPI Graphics support on Windows for
>>> the JEP 263: HiDPI Graphics on Windows and Linux
>>>      http://openjdk.java.net/jeps/263
>>>
>>>    - scale factors are added to surface dates
>>>    - window size, events coordinates and font are scaled on native side
>>>    - backup buffered image is scaled in SunVolatileImage
>>>    - AwtRobot MouseMove() and GetRGBPixels() methods are updated
>>>    - GetDpiForMonitor function is used to query the specified monitor
>>> for the horizontal and vertical DPI values.
>>>      If it is not available ID2D1Factory::GetDesktopDpi method is used
>>> instead.
>>>    - "sun.java2d.uiScale.enabled", "sun.java2d.win.uiScale",
>>> "sun.java2d.win.uiScaleX", and "sun.java2d.win.uiScaleY" options are
>>> added for the testing purposes.
>>>
>>> Thanks,
>>> Alexandr.
>>>
>