RFR: 8264475: CopyArea ignores clip state in metal rendering pipeline

[Jayathirth D V-2]

In MTLBlitLoops.copyArea() we use standalone encoder which has no clip state information because of which we ignore clip parameters set in rect clip and shape clip. We need to query and use encoders from EncoderManager to honour clip states in copyArea.

-------------

Commit messages:
 - 8264475: CopyArea ignores clip state in metal rendering pipeline

Changes: https://git.openjdk.java.net/jdk/pull/3283/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=3283&range=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8264475
  Stats: 56 lines in 2 files changed: 39 ins; 0 del; 17 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3283.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3283/head:pull/3283

PR: https://git.openjdk.java.net/jdk/pull/3283

[Jayathirth D V-2]

On Wed, 31 Mar 2021 07:27:45 GMT, Jayathirth D V <[hidden email]> wrote:

> In MTLBlitLoops.copyArea() we use standalone encoder which has no clip state information because of which we ignore clip parameters set in rect clip and shape clip. We need to query and use encoders from EncoderManager to honour clip states in copyArea.

src/java.desktop/macosx/native/libawt_lwawt/java2d/metal/MTLTexurePool.m line 206:

> 204:                                                            mipmapped:NO];
> 205:     textureDescriptor.usage = MTLTextureUsageRenderTarget |
> 206:         MTLTextureUsageShaderRead;

In copyArea we use intermediate texture queried from TexturePool as render target. If we dont set any MTLTextureUsage, we are getting assertion errors when Metal API validation is enabled because by default we can only do texture shader read(https://developer.apple.com/documentation/metal/mtltexturedescriptor/1515783-usage?language=objc). To avoid these assertion errors in Metal API validation we need to set appropriate MTLTextureUsage parameters.

-------------

PR: https://git.openjdk.java.net/jdk/pull/3283

[Ajit Ghaisas-2]

On Wed, 31 Mar 2021 07:27:45 GMT, Jayathirth D V <[hidden email]> wrote:

> In MTLBlitLoops.copyArea() we use standalone encoder which has no clip state information because of which we ignore clip parameters set in rect clip and shape clip. We need to query and use encoders from EncoderManager to honour clip states in copyArea.

I tested this fix. No regressions were observed.

Using MTLBlitCommandEncoder is intuitive when metal resource copying needs to be done.
I suggest to add a comment (may be as a method description or in code where we do the actual copy) to mention why we use MTLRenderCommandEncoder and not MTLBlitCommandEncoder.

-------------

PR: https://git.openjdk.java.net/jdk/pull/3283

[Jayathirth D V-2]

> In MTLBlitLoops.copyArea() we use standalone encoder which has no clip state information because of which we ignore clip parameters set in rect clip and shape clip. We need to query and use encoders from EncoderManager to honour clip states in copyArea.

Jayathirth D V has updated the pull request incrementally with one additional commit since the last revision:

  Add comment on usage of MTLRenderCommandEncoder

-------------

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/3283/files
  - new: https://git.openjdk.java.net/jdk/pull/3283/files/d72f9490..3e1ba4c9

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=3283&range=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=3283&range=00-01

  Stats: 7 lines in 1 file changed: 7 ins; 0 del; 0 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3283.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3283/head:pull/3283

PR: https://git.openjdk.java.net/jdk/pull/3283

[Jayathirth D V-2]

On Wed, 31 Mar 2021 12:39:49 GMT, Ajit Ghaisas <[hidden email]> wrote:

> I tested this fix. No regressions were observed.
>
> Using MTLBlitCommandEncoder is intuitive when metal resource copying needs to be done.
> I suggest to add a comment (may be as a method description or in code where we do the actual copy) to mention why we use MTLRenderCommandEncoder and not MTLBlitCommandEncoder.

Thanks Ajit for testing the fix. I have added comment mentioning why we use MTLRenderCommandEncoder and not MTLBlitCommandEncoder.

-------------

PR: https://git.openjdk.java.net/jdk/pull/3283

[Ajit Ghaisas-2]

On Wed, 31 Mar 2021 15:03:54 GMT, Jayathirth D V <[hidden email]> wrote:

>> In MTLBlitLoops.copyArea() we use standalone encoder which has no clip state information because of which we ignore clip parameters set in rect clip and shape clip. We need to query and use encoders from EncoderManager to honour clip states in copyArea.
>
> Jayathirth D V has updated the pull request incrementally with one additional commit since the last revision:
>
>   Add comment on usage of MTLRenderCommandEncoder

src/java.desktop/macosx/native/libawt_lwawt/java2d/metal/MTLBlitLoops.m line 821:

> 819:              * performing copyArea, thats why we need to query encoder with
> 820:              * appropriate state from EncoderManager and not use
> 821:              * direct MTLBlitCommandEncoder for texture mapping.

Minor : "texture mapping" should be "texture copy" as MTLBlitCommandEncoder cannot be used for texture mapping anyway.

-------------

PR: https://git.openjdk.java.net/jdk/pull/3283

[Jayathirth D V-2]

> In MTLBlitLoops.copyArea() we use standalone encoder which has no clip state information because of which we ignore clip parameters set in rect clip and shape clip. We need to query and use encoders from EncoderManager to honour clip states in copyArea.

Jayathirth D V has updated the pull request incrementally with one additional commit since the last revision:

  Comment update

-------------

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/3283/files
  - new: https://git.openjdk.java.net/jdk/pull/3283/files/3e1ba4c9..9b4a90ab

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=3283&range=02
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=3283&range=01-02

  Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3283.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3283/head:pull/3283

PR: https://git.openjdk.java.net/jdk/pull/3283

[Ajit Ghaisas-2]

On Thu, 1 Apr 2021 05:49:51 GMT, Jayathirth D V <[hidden email]> wrote:

>> In MTLBlitLoops.copyArea() we use standalone encoder which has no clip state information because of which we ignore clip parameters set in rect clip and shape clip. We need to query and use encoders from EncoderManager to honour clip states in copyArea.
>
> Jayathirth D V has updated the pull request incrementally with one additional commit since the last revision:
>
>   Comment update

Marked as reviewed by aghaisas (Committer).

-------------

PR: https://git.openjdk.java.net/jdk/pull/3283

[Prasanta Sadhukhan-2]

On Thu, 1 Apr 2021 05:49:51 GMT, Jayathirth D V <[hidden email]> wrote:

>> In MTLBlitLoops.copyArea() we use standalone encoder which has no clip state information because of which we ignore clip parameters set in rect clip and shape clip. We need to query and use encoders from EncoderManager to honour clip states in copyArea.
>
> Jayathirth D V has updated the pull request incrementally with one additional commit since the last revision:
>
>   Comment update

src/java.desktop/macosx/native/libawt_lwawt/java2d/metal/MTLBlitLoops.m line 826:

> 824:             id<MTLRenderCommandEncoder> interEncoder =
> 825:                 [mtlc.encoderManager getTextureEncoder:interTexture
> 826:                                            isSrcOpaque:dstOps->isOpaque

Should it not be srcOps->isOpaque?

src/java.desktop/macosx/native/libawt_lwawt/java2d/metal/MTLBlitLoops.m line 854:

> 852:                             atIndex:MeshVertexBuffer];
> 853:             [finalEncoder setFragmentTexture:interTexture atIndex: 0];
> 854:             [finalEncoder drawPrimitives:MTLPrimitiveTypeTriangle vertexStart:0

Can't we reuse drawTex2Tex() for this snippet?

-------------

PR: https://git.openjdk.java.net/jdk/pull/3283

[Jayathirth D V-2]

> In MTLBlitLoops.copyArea() we use standalone encoder which has no clip state information because of which we ignore clip parameters set in rect clip and shape clip. We need to query and use encoders from EncoderManager to honour clip states in copyArea.

Jayathirth D V has updated the pull request incrementally with one additional commit since the last revision:

  Refactor code to use drawTex2Tex

-------------

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/3283/files
  - new: https://git.openjdk.java.net/jdk/pull/3283/files/9b4a90ab..91e068b9

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=3283&range=03
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=3283&range=02-03

  Stats: 35 lines in 1 file changed: 0 ins; 22 del; 13 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3283.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3283/head:pull/3283

PR: https://git.openjdk.java.net/jdk/pull/3283

[Jayathirth D V-2]

On Thu, 1 Apr 2021 06:58:27 GMT, Prasanta Sadhukhan <[hidden email]> wrote:

>> Jayathirth D V has updated the pull request incrementally with one additional commit since the last revision:
>>
>>   Comment update
>
> src/java.desktop/macosx/native/libawt_lwawt/java2d/metal/MTLBlitLoops.m line 826:
>
>> 824:             id<MTLRenderCommandEncoder> interEncoder =
>> 825:                 [mtlc.encoderManager getTextureEncoder:interTexture
>> 826:                                            isSrcOpaque:dstOps->isOpaque
>
> Should it not be srcOps->isOpaque?

Both source and destination are same in copyArea. So isOpaque property will be same.
But we use 32 bit non-opaque intermediate texture, so for intermediate texture we should pass JNI_FALSE for isOpaque property.

> src/java.desktop/macosx/native/libawt_lwawt/java2d/metal/MTLBlitLoops.m line 854:
>
>> 852:                             atIndex:MeshVertexBuffer];
>> 853:             [finalEncoder setFragmentTexture:interTexture atIndex: 0];
>> 854:             [finalEncoder drawPrimitives:MTLPrimitiveTypeTriangle vertexStart:0
>
> Can't we reuse drawTex2Tex() for this snippet?

Thanks for the suggestion Prasanta, yes we should refactor code to use drawTex2Tex. I have updated the PR.

-------------

PR: https://git.openjdk.java.net/jdk/pull/3283

[Prasanta Sadhukhan-2]

On Thu, 1 Apr 2021 14:15:37 GMT, Jayathirth D V <[hidden email]> wrote:

>> In MTLBlitLoops.copyArea() we use standalone encoder which has no clip state information because of which we ignore clip parameters set in rect clip and shape clip. We need to query and use encoders from EncoderManager to honour clip states in copyArea.
>
> Jayathirth D V has updated the pull request incrementally with one additional commit since the last revision:
>
>   Refactor code to use drawTex2Tex

Marked as reviewed by psadhukhan (Reviewer).

-------------

PR: https://git.openjdk.java.net/jdk/pull/3283

[Sergey Bylokhov-2]

On Thu, 1 Apr 2021 15:50:50 GMT, Prasanta Sadhukhan <[hidden email]> wrote:

>> Jayathirth D V has updated the pull request incrementally with one additional commit since the last revision:
>>
>>   Refactor code to use drawTex2Tex
>
> Marked as reviewed by psadhukhan (Reviewer).

Just curious how this new "round trip" will affect the performance when the clip is set and when not? is it cheap?

-------------

PR: https://git.openjdk.java.net/jdk/pull/3283

[Jayathirth D V-2]

On Fri, 2 Apr 2021 22:43:36 GMT, Sergey Bylokhov <[hidden email]> wrote:

> Just curious how this new "round trip" will affect the performance when the clip is set and when not? is it cheap?

I am not getting what do you mean by 'new "round trip" '. Please elaborate.
Regarding performance metrics i have captured the details in the bug and there is no degradation.

-------------

PR: https://git.openjdk.java.net/jdk/pull/3283

[Sergey Bylokhov-2]

On Sat, 3 Apr 2021 02:56:48 GMT, Jayathirth D V <[hidden email]> wrote:

> I am not getting what do you mean by 'new "round trip" '. Please elaborate.
> Regarding performance metrics i have captured the details in the bug and there is no degradation.

The new code path which takes care of the clip, if there is no degradation means that we get it for free?

-------------

PR: https://git.openjdk.java.net/jdk/pull/3283

[Jayathirth D V-2]

On Sat, 3 Apr 2021 03:51:29 GMT, Sergey Bylokhov <[hidden email]> wrote:

> > I am not getting what do you mean by 'new "round trip" '. Please elaborate.
> > Regarding performance metrics i have captured the details in the bug and there is no degradation.
>
> The new code path which takes care of the clip, if there is no degradation means that we get it for free?

Before this change we used to get blitEncoder from same commandbuffer(and same CommandQueue) as we are doing now. And then commit the commandbuffer, so from computational perspective we are not holding or doing anything extra in new implementation. We need to use appropriate encoder(where scissor is set) to honour clip in copyArea.

Since we are not seeing any difference in performance numbers (especially in swingmark where we do lot of scrolling/copyArea) we are basically getting clipping in copyArea without any degradation. Also in Netbeans i have done good amount scrolling test and i dont see any degradation.

-------------

PR: https://git.openjdk.java.net/jdk/pull/3283

[Sergey Bylokhov-2]

On Sat, 3 Apr 2021 05:02:24 GMT, Jayathirth D V <[hidden email]> wrote:

> We need to use appropriate encoder(where scissor is set) to honour clip in copyArea.

It is quite interesting that blitting with or without clipping does not have any difference. thank you for confirmation. but probably our perf tests are not good enough.

-------------

PR: https://git.openjdk.java.net/jdk/pull/3283

[Jayathirth D V-2]

On Sat, 3 Apr 2021 05:30:12 GMT, Sergey Bylokhov <[hidden email]> wrote:

> > We need to use appropriate encoder(where scissor is set) to honour clip in copyArea.
>
> It is quite interesting that blitting with or without clipping does not have any difference. thank you for confirmation. but probably our perf tests are not good enough.

Exactly i suspect we are not hitting threshold of GPU computing in our tests. If we hit GPU threshold with larger clip bounds i expect performance to be on higher side after the fix.

-------------

PR: https://git.openjdk.java.net/jdk/pull/3283

[Jayathirth D V-2]

On Sat, 3 Apr 2021 05:30:12 GMT, Sergey Bylokhov <[hidden email]> wrote:

>>> > I am not getting what do you mean by 'new "round trip" '. Please elaborate.
>>> > Regarding performance metrics i have captured the details in the bug and there is no degradation.
>>>
>>> The new code path which takes care of the clip, if there is no degradation means that we get it for free?
>>
>> Before this change we used to get blitEncoder from same commandbuffer(and same CommandQueue) as we are doing now. And then commit the commandbuffer, so from computational perspective we are not holding or doing anything extra in new implementation. We need to use appropriate encoder(where scissor is set) to honour clip in copyArea.
>>
>> Since we are not seeing any difference in performance numbers (especially in swingmark where we do lot of scrolling/copyArea) we are basically getting clipping in copyArea without any degradation. Also in Netbeans i have done good amount scrolling test and i dont see any degradation. Also i expected improvement in performance (we are seeing little improvement in swingmark numbers) because we are actually doing less amount of copy operation now.
>
>> We need to use appropriate encoder(where scissor is set) to honour clip in copyArea.
>
> It is quite interesting that blitting with or without clipping does not have any difference. thank you for confirmation. but probably our perf tests are not good enough.

@mrserb Are there any more inputs/review comments?

-------------

PR: https://git.openjdk.java.net/jdk/pull/3283

[Sergey Bylokhov-2]

On Sun, 4 Apr 2021 10:00:16 GMT, Jayathirth D V <[hidden email]> wrote:

>>> We need to use appropriate encoder(where scissor is set) to honour clip in copyArea.
>>
>> It is quite interesting that blitting with or without clipping does not have any difference. thank you for confirmation. but probably our perf tests are not good enough.
>
> @mrserb Are there any more inputs/review comments?

nop, it is fine.

-------------

PR: https://git.openjdk.java.net/jdk/pull/3283