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

[Sergey Bylokhov]

I have two small suggestions:
  - Probably it will be better to move the new properties from 
Win32GraphicsDevice.java to SunGraphicsEnvironment.java, at least 
"sun.java2d.uiScale" should be implemented on linux, and can be 
implemented on osx later.
  - I am worried about a rounding in 
awt_Win32GraphicsDevice.ScaleDownXY/ScaleUpXY when these methods are 
applied to the x/y and width/height.

On 21.10.15 16:34, 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.
>>>
>


-- 
Best regards, Sergey.