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

Tue Mar 1 23:59:06 UTC 2016

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.