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

[Jim Graham]

In drawHiDPIImage, in the VolatileImage case you ignore the transform, 
but I suppose that is OK because you pass it through to the scaleImage() 
case.  However if it came in with a transform then the asserts at the 
top of scaleImage may fail?  Maybe it would be better to actually handle 
the "non-0,0,iw,ih" case rather than assert that it doesn't exist?

BIGC.getConfig() - I don't see where you put the scale into the BIGC 
when you create it.  Shouldn't the constructor with scales be used at 
line 73?

SGE.getScaleFactor() - suggestion: perhaps ignore an optional final 
suffix "x" in the regular scale case so that they can say "=3.0x" to 
mean the same as "=3"?

Looks almost good to go from my perspective.  What is the plan for Swing 
testing and fixing those bugs?

			...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.
>>>>>
>>>
>>
>>
>