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

[Alexander Scherbatiy]

   Hello,

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

On 11/25/2015 12:30 AM, Jim Graham wrote:
> Hi Alexandr,
>
> OSXOffScreenSD.java - what are these "rdar:" references?
     These are references to the internal Apple bug system. They have 
been contributed with the Mac OS X port:
http://hg.openjdk.java.net/macosx-port/macosx-port/jdk/rev/ee9c8c9b5c2e

>
> 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...?

     I create a separate issue on it: JDK-8144181 
OSXOffScreenSurfaceData.copyArea() method improvement
>
> (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?
     I updated the SurfaceData.copyArea() x,y,width, and height description.
>
> OSXOffscreenSD.java, line 511 - double check your x/y's
     Updated.
>
> OSXSD.java, line 1097 - typos: heavyweight, clipped
    Updated.
>
> 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.
     I moved it to the separate issue:  JDK-8144179 GraphicsExposure 
event generation investigation in XCopyArea
>
> 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...?

     The test is updated according to the comments.

   Thanks,
   Alexandr.
>
>             ...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.
>>>>>
>>>>
>>>>
>>