[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-2d-dev
Subject:    Re: [OpenJDK 2D-Dev] Review request for 8069348 SunGraphics2D.copyArea() does not properly work for 
From:       Sergey Bylokhov <Sergey.Bylokhov () oracle ! com>
Date:       2016-02-26 10:25:31
Message-ID: 56D0281B.2020503 () oracle ! com
[Download RAW message or body]

+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.
[prev in list] [next in list] [prev in thread] [next in thread]