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

[Jim Graham]

Note that I still have outstanding comments in my other review email on 
webrev.01 - the comments below were only about the indicated changes you 
did for Sergey...

			...jim

On 10/22/15 5:34 PM, Jim Graham wrote:
> The changes look fine to me.  I'm OK with deferring on the rounding of
> the scaleDown as long as we don't let it fall between the cracks.  For
> one thing, it will probably require some additional testing to gauge its
> effects and determine the best strategy for rounding down...
>
>              ...jim
>
> On 10/22/15 8:28 AM, Alexander Scherbatiy wrote:
>>
>>   Could you review the updated fix:
>>      http://cr.openjdk.java.net/~alexsch/8073320/webrev.02/
>>
>>
>> On 10/21/2015 5:40 PM, Sergey Bylokhov wrote:
>>> 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 have moved the uiScale properties initialization to the
>> SunGraphicsEnvironment and Win32GraphicsEnvironment classes.
>>>  - I am worried about a rounding in
>>> awt_Win32GraphicsDevice.ScaleDownXY/ScaleUpXY when these methods are
>>> applied to the x/y and width/height.
>>
>>      Could I move this update to a separate fix?
>>
>>     Thanks,
>>     Alexandr.
>>
>>>
>>> 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.
>>>>>>
>>>>
>>>
>>>
>>