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

Wed Mar 2 14:01:43 UTC 2016

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 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.
>

-- 
Best regards, Sergey.