[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:
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