[OpenJDK 2D-Dev] Review Request: (JDK-8044788) [D3D] clip is ignored during surface->sw blit

Prahalad Kumar Narayanan prahalad.kumar.narayanan at oracle.com
Wed Mar 2 07:16:21 UTC 2016

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:

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; 2d-dev at openjdk.java.net
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 <Sergey.Bylokhov at oracle.com>
>>> To:     prasanta sadhukhan <prasanta.sadhukhan at oracle.com>,
>>> 2d-dev at openjdk.java.net
>>> 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.

More information about the 2d-dev mailing list