+1>The fix looks good. But you should remove the "-Dsun.java2d.d3d" option in th > test(don't change it to true) otherwise it will fail on non-windows platform, because the "=false" is
> ignored if pipeline is not available.
Removing it was the right thing to do but it would not have failed on non-windows
platforms .. they simply would not recognise the system property name. No more harm than -Dfoo.bar=xyz -phil. On 3/2/16, 6:01 AM, Sergey Bylokhov wrote:
Looks fine. On 02.03.16 10:16, Prahalad Kumar Narayanan wrote:Dear Sergey, Phil & Java 2D-Dev Group Good day to you. A Quick Follow-up to Bug: JDK-8044788 Bug Link: https://bugs.openjdk.java.net/browse/JDK-8044788 Review Fixes:The following fixes have been incorporated collating all of Sergey's valuable feedback.1. File: D3DBlitLoops.java. Method: complexClipBlit.Removed if(...) condition pertaining to setting of BufferedImage's pixel format type. Proper type has been set with appropriate comment indicating why the setting has been done.2. File: IncorrectClipSurface2SW.java.The option -Dsun.java2d.d3d=true has been removed from the test file. This will be required inorder to run the test across all platforms (incl. non-windows platforms)The webrev incorporating the above review fixes is available under: http://cr.openjdk.java.net/~psadhukhan/prahlad/webrev.02/ Kindly review the fix and provide your opinion. Thank you for your time in review Have a good day Prahalad N. -----Original Message----- From: Sergey Bylokhov Sent: Wednesday, March 02, 2016 5:29 AMTo: Prahalad kumar Narayanan; Philip Race; prasanta sadhukhan; [email protected] Subject: Re: [OpenJDK 2D-Dev] Review Request: (JDK-8044788) [D3D] clip is ignored during surface->sw blitHi, Prahalad.The fix looks good. But you should remove the "-Dsun.java2d.d3d" option in the test(don't change it to true) otherwise it will fail on non-windows platform, because the "=false" is ignored if pipeline is not available.On 01.03.16 8:10, Prahalad kumar Narayanan wrote:Hello Everyone, 2D-Dev Group. Good day to you. First, thanks to Sergey for his accurate review & suggestions. I.Review Fixes: 1. Upon inspection into the code, I found that native D3D interfaces support on-the-fly conversion of pixels in D3DSurface to Sysmem based BufferedImage of type "IntARGB only". 2. Hence, the if(...) condition present in complexClipBlit method is not valid. This change has been incorporated in the code now. 3. The webrev.01 is available for review under: http://cr.openjdk.java.net/~psadhukhan/prahlad/webrev.01/ II.Next Steps: 1. Further discussions with Sergey revealed the fact that,D3DBlitLoops.java registers a primitive for SurfaceToSw blit as follows:static void register() { ... new D3DSurfaceToSwBlit(SurfaceType.IntArgb, D3DSurfaceData.ST_INT_ARGB); ... }; 2. The above code implies that there is no single/ direct blit method if the destination is of SurfaceType.IntArgbPre. 3. When no registered primitive exists for the source and destination type, the conversion might be implemented through GeneralBlits causing many CPU cycles. 4. Sergey suggested that, we could enhance the native D3D interface to support on the fly conversion to destination of type IntArgbPre. This will help improve the performance. But this can be taken up after the current fix. Kindly review the fix for current issue from the link - http://cr.openjdk.java.net/~psadhukhan/prahlad/webrev.01/ Thank you for your time in review Have a good day Prahalad N. On 2/24/2016 5:15 PM, Sergey Bylokhov wrote:Hi, Prahalad. The fix looks good in general. I have only one question: In the complexClipBlit() you have this code: 552 // Type- indicates the pixel format of Sysmem based BufferedImage. 553 // Native side D3D interface supports copying surface contents to 554 // ST_INT_ARGB, ST_INT_ARGB_PRE and other pixel formats 555 final int type = typeval == D3DSurfaceData.ST_INT_ARGB_PRE ? 556 BufferedImage.TYPE_INT_ARGB_PRE : 557 BufferedImage.TYPE_INT_ARGB; This code was copied from the OGL version of the fix and it have two assumptions: - The type of the native texture can be ST_INT_ARGB_PRE, or can contains "other"(???) value. Please take a look to the OGLBlitLoops.java code. In OGL we register two different blits: OGLSurfaceToSwBlit blitSurfaceToIntArgbPre = new OGLSurfaceToSwBlit(SurfaceType.IntArgbPre,OGLSurfaceData.PF_INT_ARGB_ PRE); // surface->sw ops new OGLSurfaceToSwBlit(SurfaceType.IntArgb,OGLSurfaceData.PF_INT_ARGB), blitSurfaceToIntArgbPre, So in OGL when the blit is called the type can be PF_INT_ARGB_PRE orPF_INT_ARGB. This is why we have "if" statement in the complexClipBlit.- The second assumption is that the native code is able to copy the d3d texture to the system memory and do conversion on the fly. The ogl code have such conversion(argb_pre is converted to argb in the OGLBlitLoops.c in the flip() method). Do you know our native d3d part is able to do the same? If yes, then we should register separate blits in D3DBlitLoops to cover different source/destination and change the "if". On 22.02.16 11:57, Prahalad kumar Narayanan wrote:Hello Everyone, 2D-Dev Group I continued to inspect the bug : JDK-8044788: [D3D] Clip is ignored during Surface > Sw Blit issue. Link : https://bugs.openjdk.java.net/browse/JDK-8044788 Here is a quick brief on root cause, solutions experimented and the webrev link for review. Kindly go through each of these items and provide your review so that we could check-in the bug fix. Root Cause: 1. D3DSurfaceToSw Blit does not pass clip value to the native cpp interface 2. Even if the clip were to be passed through JNI, the native interface ignores the clip because it uses d3d's StretRect API. 3. Henceforth, in the current implementation, clipping in SurfaceToSw usecase will work ' only ' for Rect bounds provided proper src(x, y), dst(x, y), width and height are provided. 4. It can be noted that : the proper src(x, y), dst(x, y) are also not calculated considering the intersection of these rectangles with the clip bounds. Different Solutions & Results 1. Three solutions are feasible to fix this issue: a. Solution 1: Convert the input Surface to Sysmem image, followed by regular Blit. To optimize this further, the conversion can be taken up only for Shape clips. b. Solution 2: Using existing classes in D3DBlitLoops, execute SwToSurface (copy dest to accel Surface), SurfaceToSurface (supports clip), SurfaceToSw (full copy) in order. c. Solution 3: Modify the native side interfaces to accept clip and execute Solution2 's logic in native code. 2. Upon various experiments, Solution 1 has been found to be faster than the rest. a. This can be attributed to fewer draw /or copy iterations compared to the other two solutions. b. In addition to the above fact, the code retains the same approach as used by the OGL pipeline making it easier to understand. Webrev Link: http://cr.openjdk.java.net/~psadhukhan/prahlad/webrev.00/ Sufficient comments have been added so that developers could easily get understanding of the issue and solution checked in. Thank you Have a great day Prahalad N. Subject: Re: [OpenJDK 2D-Dev] Review Request: (JDK-8044788) [D3D] clip is ignored during surface->sw blit Date: Wed, 15 Apr 2015 16:17:37 +0300 From: Sergey Bylokhov <[email protected]> To: prasanta sadhukhan <[email protected]>, [email protected] Hi, Prasanta. The same approach was used in OGL pipeline for the reason. Because we use glReadPixels and there is no way to apply the clip. So we read pixels to the temporary buffer and apply the clip later. Can you additionaly investigate is it possible to apply the clip in d3d directly or not? Note that this [1] comment is not correct in d3d, because d3d have only one direct blit D3d surface -> SW which is D3DSurfaceToSwBlit(IntArgb, ST_INT_ARGB), all other blits will be done via temporary buffer in D3DGeneralBlit. [1]523 // We can convert argb_pre data from D3d surface in twoplaces: 524 // - During D3d surface -> SW blit 525 // - During SW -> SW blit 526 // The first one is faster when we use opaque D3d surface, because in 527 // this case we simply skip conversion and use color components as is.528 // Because of this we align intermediate buffer type withtype of 529 // destination not source.On 15.04.15 11:57, prasanta sadhukhan wrote:Hi, I would like a review for a solution of this bug in jdk9. The clip was ignored during surface->sw blit in d3d pipeline. The fix is to use the clip parameter to calculate the blit coordinates correctly. Bug:https://bugs.openjdk.java.net/browse/JDK-8044788 webrev:http://cr.openjdk.java.net/~psadhukhan/8044788/webrev.00/ Regards Prasanta-- Best regards, Sergey.-- Best regards, Sergey.
