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 AM To: 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 blit Hi, 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 or >> PF_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 two >>>> places: >>>> 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 with >>>> type 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.
