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

[Alexander Scherbatiy]

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.