<AWT Dev> [OpenJDK 2D-Dev] [9] Review request for 8073320 Windows HiDPI Graphics support
Sergey Bylokhov
Sergey.Bylokhov at oracle.com
Wed Nov 11 16:48:01 UTC 2015
The latest version looks fine to me.
A few notes.
- SG2D.drawHiDPIImage does not take into account that
MultiResolutionImage can contain VolatileImages.
- Probably it will be better to move all code related to up/down scale
from SG2D to the DrawImagePipe, which sometimes uses scaled coordinates
and sometimes not.
On 11.11.15 14:08, Alexander Scherbatiy wrote:
> On 11/10/2015 10:41 PM, Jim Graham wrote:
>> 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?
> The only case where xform is not null is the drawImage(img, xform,
> observer) method which always use zero x, y and the same src and dst
> width/height.
> Could we move the full support for a resolution variant
> transformation to a separate fix?
>
>>
>> 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?
>
> It is updated in the latest webrev:
> http://cr.openjdk.java.net/~alexsch/8073320/webrev.03
> which has been sent to the review:
> http://mail.openjdk.java.net/pipermail/2d-dev/2015-November/005814.html
>
> For some reasons the link to the latest webrev and comments are not
> mentioned in this email.
>
> The BIGC:73 line has been corrected to:
> 73 ret = new BufferedImageGraphicsConfig(bImg, null,
> scaleX, scaleY);
>
>>
>> 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"?
>
> It should also be updated in the latest webrev.
>
> Thanks,
> Alexandr.
>
>>
>> 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.
>>>>>>>
>>>>>
>>>>
>>>>
>>>
>
--
Best regards, Sergey.
More information about the awt-dev
mailing list