<AWT Dev> [OpenJDK 2D-Dev] [9] Review request for 8073320 Windows HiDPI Graphics support
Alexander Scherbatiy
alexandr.scherbatiy at oracle.com
Thu Nov 5 10:50:06 UTC 2015
Hello,
Could you review the updated fix:
http://cr.openjdk.java.net/~alexsch/8073320/webrev.03/
See my comments inline:
On 10/23/2015 3:27 AM, Jim Graham wrote:
> 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.
I reverted the changes back for the VolatileImage because it
returns width and height of the original image, not the scaled ones.
I added the xform transform rescaling for the MultiresolutionImage.
>
> BIGC, line 73 - shouldn't you pass in scaleX,scaleY to the constructor?
Updated.
>
> 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)?
It requires additional 'else' branch for the case when UI scales
are disabled by user properties.
I have updated this.
>
> 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.
Updated.
Thanks,
Alexandr.
>
> 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.
>>>>
>>
More information about the awt-dev
mailing list