[OpenJDK 2D-Dev] Review request for 8069348 SunGraphics2D.copyArea() does not properly work for scaled graphics in D3D
james.graham at oracle.com
Tue Nov 24 21:30:24 UTC 2015
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
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
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...?
On 11/24/15 5:07 AM, Alexander Scherbatiy wrote:
> Could you review the updated fix:
> - 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
> - The test which uses copyArea() method for VolatileImage is added. I
> left the test which moves internal frames for the Swing testing purposes.
> 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...
>> On 11/20/15 7:38 AM, Sergey Bylokhov wrote:
>>> On 20.11.15 14:49, Alexander Scherbatiy wrote:
>>>> 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
>>>> 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 =(
More information about the 2d-dev