Re: [OpenJDK 2D-Dev] Review request for 8069348 SunGraphics2D.copyArea() does not properly work for scaled graphics in D3D

[Sergey Bylokhov]

+1

On 02.02.16 4:38, Jim Graham wrote:

Hi Alexandr,

This looks great.  It's good to go, but you might want to fix one
indenting issue before you push:

X11SD and XRSD both had the same edit, but you made a different decision
on whether to get rid of some extra parentheses or not and XRSD was left
with the indentation off by 1.  It would probably make sense to get rid
of the extra parens in X11SD and then make sure the resulting
indentation is correct in both.  But, that won't affect the correctness
of the fix.

I don't feel the need to do a review for these indentation changes if
you make them...

                 ...jim

On 1/28/16 9:24 AM, Alexandr Scherbatiy wrote:

  Hello,

  Could you review the updated fix:
   http://cr.openjdk.java.net/~alexsch/8069348/webrev.04/

  - TRANSFORM_TRANSLATESCALE check is restored in the
OSXOffScreenSurfaceData
  - TRANSFORM_TRANSLATESCALE check is removed from X11SurfaceData and
GDIWindowSurfaceData

   Thanks,
   Alexandr.

On 12/15/2015 11:22 AM, Jim Graham wrote:

Also, X11SD and GDISD still reject TRANSLATESCALE.

I think it may make sense for OSXOffscreen to return false for
TRANSLATESCALE and punt back to the general implementation?  But, X11
and GDI should be able to allow that condition...

            ...jim

On 12/14/15 4:57 PM, Jim Graham wrote:

Will OSXOffScreenSurfaceData.copyArea work with a scaled context?  It
calls the drawImage on the original sg2d, but it is using transformed
coordinates already.  On the other hand, I installed the patch and
built
on my retina MBP and didn't see any errors in SwingSet2 - is that
because I was using a different SurfaceData by default (CGL?)?

The rest looks fine...

             ...jim

On 12/11/15 12:09 PM, Alexander Scherbatiy wrote:

Hello,

Could you review the updated fix:
http://cr.openjdk.java.net/~alexsch/8069348/webrev.03

On 28/11/15 02:43, Jim Graham wrote:

Hi Alexandr,

On 11/27/15 2:06 AM, Alexander Scherbatiy wrote:

OSXOffscreenSD.java (and all *SD.java), line 481 - should we just
make
this part of the SD.copyArea contract, that the coordinates are in
device space and the SD method should not concern itself with the
SG2D
transform?

     I updated the SurfaceData.copyArea() x,y,width, and height
description.

And yet most of the implementations still check the transformState.
Why do they do that if they are no longer concerned with transforming
the inputs?

   Updated.

   XRSurfaceData didn't handled translate scale transform state. I
removed it and checked that on Linux scaled internal frames are
properly
moved and scroll works correctly.
   GDIWindowSurfaceData handles only translate state. For scale state
support it is necessary also to scale destination coordinates. It is
also requires some additional testing. I left it unchanged.

CopyAreaTest.java, line 61 - rounding is not the same operation
that
SG2D uses, but it works anyway?

The rounding still isn't the same as SG2D.  Floor() != ceil(v - 0.5).

On second thought, it's probably best not to worry about the exact
rounding in the test case, but just test 1 pixel inset from the
coordinates that are needed.  In other words, check:

scale(X + (N+1) * DX) + 1
scale(Y + (N+1) * DY) + 1,
scale(W) - 2
scale(H) - 2

and go back to just rounding...

    Updated.

CopyAreaTest.java, lines 143,144 - why subtract 2DX and 2DY here?
Ah,
this may mask the error in line 94 above...?

I notice that it used to check the rectangle at X+(N+1)*DX,
Y+(N+1)*DY, but now it only checks X+N*DX,Y+N*DY. Why not continue to
check the N+1 copy? That should be the location of the destination of
the last copy, right?

   I believe my initial assumption was incorrect.
   For example, let's take N = 1. The loops below has only one
iteration:
     -----------
         for (int i = 0; i < N; i++) {
             g.copyArea(X + i * DX, Y + i * DY, W, H, DX, DY);
         }
     -----------

  Which is just g.copyArea(X, Y, W, H, DX, DY). So the destination
rectangle is (X+DX, Y+DY, W, H)
  which corresponds to the N = 1.

  I also updated the test to check different scaleX and scaleY.

  Thanks,
  Alexandr.

            ...jim

--
Best regards, Sergey.