[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:       Jim Graham <james.graham () oracle ! com>
Date:       2015-12-15 19:33:15
Message-ID: 56706AFB.7060407 () oracle ! com
[Download RAW message or body]

A safer approach may be to punt for that implementation if there is no 
strong argument for having an optimized copyArea there.  How sure or not 
sure are you that this is still used?

			...jim

On 12/15/15 11:30 AM, Sergey Bylokhov wrote:
> On 15/12/15 03:57, 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?)?
>
> OSXOffScreenSurfaceData and other classes are a part of the old quartz
> based pipeline. These classes were added to the jdk as part of the
> printing support on the osx. As far as I know the printing support was
> copied as is from jdk6 and I am not sure that all of these files are
> used right now.
>
>>
>> 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
>>>
>
>
[prev in list] [next in list] [prev in thread] [next in thread]