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

[Jim Graham]

Hi Alexandr,

OSXOffScreenSD.java - what are these "rdar:" references?

OSXOffscreenSD.java - since this method just refers back to the SG2D, 
how is it any better than just returning false and letting SG2D's 
general copyArea deal with the issue?  Also, SG2D.drawImage() is not 
guaranteed to handle overlapping copyareas so arguably it could fail - 
but I suppose the assertion is that the drawImage to such destinations 
happens to deal with overlapping areas?  Is that asserted or tested 
anywhere?  Also, this method claims that the Windows version ignores the 
"light clip", but it doesn't.  In fact, if there is a clip then 
GDIcopyarea punts to the general version.  I find a lot to worry about 
in this implementation and I wonder how well it was tested...?

(Note that other than the two issues I point out below with this file, 
those concerns were not raised by this fix - it looks like a fairly 
faithful translation of the questionable implementation to the new 
scheme (ignoring the x/y bug below))

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?

OSXOffscreenSD.java, line 511 - double check your x/y's

OSXSD.java, line 1097 - typos: heavyweight, clipped

X11SD.java & XRSD.java - (not an issue with this fix, but a question for 
future work) needExposures?  Do any of the other copyarea 
implementations send repaint events?  Shouldn't this be hard-coded to 
false?  More investigation is likely needed here.

CopyAreaTest.java, line 52 - typo "BACKGROUND_COLOR"

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

CopyAreaTest.java, line 86 - perhaps move the scale to after you fill 
the background?

CopyAreaTest.java, line 94 - "Y + i * DX" => DY  (that's odd, how did 
the test pass?)

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

			...jim

On 11/24/15 5:07 AM, Alexander Scherbatiy wrote:
>
>   Hello,
>
>   Could you review the updated fix:
>     http://cr.openjdk.java.net/~alexsch/8069348/webrev.01/
>
>   - The given image sizes are transformed in the SG2D.doCopyArea method.
> The copyArea() methods are updated accordingly.
>   - OGLSurfaceData.copyArea() method is updated to process scaled
> transforms.
>   - The test which uses copyArea() method for VolatileImage is added. I
> left the test which moves internal frames for the Swing testing purposes.
>
>   Thanks,
>   Alexandr.
>
>
> On 11/20/2015 6:59 PM, Jim Graham wrote:
>> In terms of consolidating the code...
>>
>> Since SD.copyArea is an internal API, we could simply specify that it
>> takes pixel coordinates and assumes COPY semantics so it is up to SG2D
>> to verify the transform and compState and perform appropriate
>> coordinate transformations before asking the SD to do the dirty work.
>> I'm not sure why we left so much veto power up to the SD class there...
>>
>>             ...jim
>>
>> On 11/20/15 7:38 AM, Sergey Bylokhov wrote:
>>> On 20.11.15 14:49, Alexander Scherbatiy wrote:
>>>>
>>>> Hello,
>>>>
>>>> Could you review the fix:
>>>>    bug: https://bugs.openjdk.java.net/browse/JDK-8069348
>>>>    webrev: http://cr.openjdk.java.net/~alexsch/8069348/webrev.00
>>>>
>>>>    For some reasons Blit operation does not properly work in the
>>>> SunGraphics2D.copyArea() method
>>>>    with scaled transform neither on Windows nor on Mac OS X.
>>>>    I have filled an issue on it: JDK-8143392 SunGraphics2D.copyArea()
>>>> does not properly handle Blit operation
>>>
>>> Interesting.
>>>
>>>>
>>>>    The current solution updates D3DSurfaceData.copyArea() to handle
>>>> scaled graphics in the same way
>>>>    as it has been already done for the CGLSurfaceData (see JDK-8000629
>>>> [macosx] Blurry rendering with Java 7 on Retina display).
>>>
>>> Note that this fix does not help ogl on windows right? because on
>>> windows the usual ogl blit should be used since the fix for retina was
>>> osx specific. Or it works properly?
>>>
>>>>
>>>>    May be there is a way to avoid the code duplication: put it to some
>>>> SurfaceDataUtils class (it also
>>>>    requires adding parent copyArea() method to BufferedRenderPipe) or
>>>> something else. I am not sure about the best option.
>>>
>>> This is up to the 2d team, but I think yes, it will be better to move it
>>> somewhere because after the current fix the different pipelines will
>>> behave differently on different platforms for a different
>>> transformations =(
>>>
>>>>
>>>>
>>>>    Thanks,
>>>>    Alexandr.
>>>>
>>>
>>>
>