[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:20:58 UTC 2016
+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