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

Philip Race philip.race at oracle.com
Wed Mar 2 22:22:20 UTC 2016


Oh sorry, I just realised you did not add the new bug id to the test

it should now say
@bug 8041644 8044788

No need for an updated webrev ..

-phil.

On 3/2/16, 2:20 PM, Philip Race wrote:
> +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 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