Re: [OpenJDK 2D-Dev] RFR: 8273375: Remove redundant 'new String' calls after concatenation in java.desktop

[Jayathirth D V]

On Fri, 3 Sep 2021 07:53:21 GMT, Andrey Turbanov 
 wrote:

> Result of string concatenation is a newly created `String` object. There is 
> no need it wrap it in another `new String` call.
> Such calls are confusing and produce warnings in IDE. Without them code is 
> easier to read.

Link this PR to bug in JBS. Otherwise change looks fine.

-

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

Re: [OpenJDK 2D-Dev] RFR: 8272878: JEP 381 cleanup: Remove unused Solaris code in sun.font.TrueTypeGlyphMapper

[Jayathirth D V]

On Mon, 23 Aug 2021 22:05:13 GMT, Daniel Gredler 
 wrote:

> During the recent JEP 381 removal of Solaris code, a few Solaris-specific 
> constants and private methods were left behind in 
> sun.font.TrueTypeGlyphMapper. This PR removes these unused odds and ends.

Marked as reviewed by jdv (Reviewer).

-

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

Re: [OpenJDK 2D-Dev] RFR: 8262751: RenderPipelineState assertion error in J2DDemo [v2]

[Jayathirth D V]

On Wed, 25 Aug 2021 18:10:54 GMT, Alexey Ushakov  wrote:

>> Provide correct pipeline state for MTLPaint after reset
>
> Alexey Ushakov has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   8262751: RenderPipelineState assertion error in J2DDemo
>   
>   Minor correction in the comments

All test run is fine. Change looks good to me.

-

Marked as reviewed by jdv (Reviewer).

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

Re: [OpenJDK 2D-Dev] RFR: 8262751: RenderPipelineState assertion error in J2DDemo

[Jayathirth D V]

On Wed, 25 Aug 2021 05:41:03 GMT, Jayathirth D V  wrote:

>> Provide correct pipeline state for MTLPaint after reset
>
> src/java.desktop/macosx/native/libawt_lwawt/java2d/metal/MTLPaints.m line 971:
> 
>> 969: [encoder setRenderPipelineState:pipelineState];
>> 970: } else {
>> 971:   // Fallback to valid pipeline state
> 
> We should not call it a valid pipeline state. It is more of standard pipeline 
> state with color 0 which we are using  after reset.
> 
> We are hitting default MTLPaint without any texture(which seems to be the 
> main root cause for this issue- state management problem) and we end up not 
> setting any pipeline state. Please reword this comment.

Also is it possible for us to just return at any earlier stage before we hit 
setRenderPipelineState() without valid texture or color?

-

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

Re: [OpenJDK 2D-Dev] RFR: 8262751: RenderPipelineState assertion error in J2DDemo

[Jayathirth D V]

On Mon, 23 Aug 2021 19:30:35 GMT, Alexey Ushakov  wrote:

> Provide correct pipeline state for MTLPaint after reset

I have given all test run on metal pipeline. I will add my observation once it 
is done.

-

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

Re: [OpenJDK 2D-Dev] RFR: 8262751: RenderPipelineState assertion error in J2DDemo

[Jayathirth D V]

On Mon, 23 Aug 2021 19:30:35 GMT, Alexey Ushakov  wrote:

> Provide correct pipeline state for MTLPaint after reset

src/java.desktop/macosx/native/libawt_lwawt/java2d/metal/MTLPaints.m line 971:

> 969: [encoder setRenderPipelineState:pipelineState];
> 970: } else {
> 971:   // Fallback to valid pipeline state

We should not call it a valid pipeline state. It is more of standard pipeline 
state with color 0 which we are using  after reset.

We are hitting default MTLPaint without any texture(which seems to be the main 
root cause for this issue- state management problem) and we end up not setting 
any pipeline state. Please reword this comment.

-

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

[OpenJDK 2D-Dev] Integrated: 8270893: IndexOutOfBoundsException while reading large TIFF file

[Jayathirth D V]

On Tue, 20 Jul 2021 06:25:22 GMT, Jayathirth D V  wrote:

> We are incorrectly passing source offset to ImageInputStream.readFully() 
> which is getting used on destination buffer. streamPos maintained in each 
> implementation of stream maintain's appropriate source offset while reading 
> the data. Since we are completely utilizing destination buffer any offset 
> greater than 0 would cause IOOBE. In our case we should use 0 as offset value.
> 
> Also to hit this code we need stream/file with at-least 1MB of IFD data, 
> that's why there is no regression test. This change can be verified using 
> image attached in JBS. All test run is green.

This pull request has now been integrated.

Changeset: efcdcc7f
Author:Jayathirth D V 
URL:   
https://git.openjdk.java.net/jdk/commit/efcdcc7fb792c77aef1da69b1fcc652b401115f7
Stats: 83 lines in 2 files changed: 76 ins; 0 del; 7 mod

8270893: IndexOutOfBoundsException while reading large TIFF file

Reviewed-by: prr, serb

-

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

Re: [OpenJDK 2D-Dev] RFR: 8270893: IndexOutOfBoundsException while reading large TIFF file [v2]

[Jayathirth D V]

On Tue, 3 Aug 2021 19:03:59 GMT, Sergey Bylokhov  wrote:

> Thank you! look fine.
> BTW Looks like stream.readFully(unit, 0, sz) can be simplified to the 
> stream.readFully(unit)?

Thanks for the review. Yes we can simplify readFully(), looks like we have 
other instances of similar usage of readFully() in this file. I would like to 
cleanup all of them but then i dont want to face issues while backporting it to 
different trains. So i would not refactor readFully() in this PR, if needed i 
will do refactoring as part of another bug.

-

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

Re: [OpenJDK 2D-Dev] RFR: 8271456: Avoid looking up standard charsets in "java.desktop" module

[Jayathirth D V]

On Sun, 1 Aug 2021 07:07:21 GMT, Sergey Bylokhov  wrote:

> This is a request to clean up a desktop module as was done in JDK-8233884 for 
> "java.base" module.
> 
> In many places standard charsets are looked up via their names, for example:
> absolutePath.getBytes("UTF-8");
> 
> This could be done more efficiently(x20 time faster) with use of 
> java.nio.charset.StandardCharsets:
> absolutePath.getBytes(StandardCharsets.UTF_8);
> 
> The later variant also makes the code cleaner, as it is known not to throw 
> UnsupportedEncodingException in contrary to the former variant.
> 
> Tested by the desktop headless/headful tests on linux/windows.

Marked as reviewed by jdv (Reviewer).

-

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

Re: [OpenJDK 2D-Dev] RFR: 8270893: IndexOutOfBoundsException while reading large TIFF file [v2]

[Jayathirth D V]

> We are incorrectly passing source offset to ImageInputStream.readFully() 
> which is getting used on destination buffer. streamPos maintained in each 
> implementation of stream maintain's appropriate source offset while reading 
> the data. Since we are completely utilizing destination buffer any offset 
> greater than 0 would cause IOOBE. In our case we should use 0 as offset value.
> 
> Also to hit this code we need stream/file with at-least 1MB of IFD data, 
> that's why there is no regression test. This change can be verified using 
> image attached in JBS. All test run is green.

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

  Added regression test

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/4836/files
  - new: https://git.openjdk.java.net/jdk/pull/4836/files/81d54f09..9c7e113c

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=4836=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=4836=00-01

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

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

Re: [OpenJDK 2D-Dev] RFR: 8270893: IndexOutOfBoundsException while reading large TIFF file

[Jayathirth D V]

On Sat, 24 Jul 2021 03:17:11 GMT, Sergey Bylokhov  wrote:

>> We are incorrectly passing source offset to ImageInputStream.readFully() 
>> which is getting used on destination buffer. streamPos maintained in each 
>> implementation of stream maintain's appropriate source offset while reading 
>> the data. Since we are completely utilizing destination buffer any offset 
>> greater than 0 would cause IOOBE. In our case we should use 0 as offset 
>> value.
>> 
>> Also to hit this code we need stream/file with at-least 1MB of IFD data, 
>> that's why there is no regression test. This change can be verified using 
>> image attached in JBS. All test run is green.
>
> 1024000 bytes does not look like a big problem especially if it is generated 
> on the fly during the test execution?
> You can try to load the data for our PYCC color profile and set one of its 
> tag data to a big chunk like icSigCopyrightTag, or you can try to just fill 
> it by zeros at the end.

@mrserb added regression test, please review.

-

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

Re: [OpenJDK 2D-Dev] RFR: 8270893: IndexOutOfBoundsException while reading large TIFF file

[Jayathirth D V]

On Tue, 20 Jul 2021 06:25:22 GMT, Jayathirth D V  wrote:

> We are incorrectly passing source offset to ImageInputStream.readFully() 
> which is getting used on destination buffer. streamPos maintained in each 
> implementation of stream maintain's appropriate source offset while reading 
> the data. Since we are completely utilizing destination buffer any offset 
> greater than 0 would cause IOOBE. In our case we should use 0 as offset value.
> 
> Also to hit this code we need stream/file with at-least 1MB of IFD data, 
> that's why there is no regression test. This change can be verified using 
> image attached in JBS. All test run is green.

I went through TIFF spec and image provided in the bug to understand whether we 
can find a way to pass similar data to reproduce the issue.

The image attached in JBS has ICCProfile as one of the TIFFTag(This is 
considered as UNDEFINED tag by our standard reader) and its count is more than 
1024000. And for this ICCProfile tag corresponding data is also present in the 
stream, it is not some corrupt header scenario where we can just write bad data 
in header and hit the issue. We divide the tag data in chunks on 1024000 bytes, 
when we are done reading first chunk of ICCProfile data and start reading the 
second chunk we hit this issue.

So to add regression test for this scenario we need more than 1024000 bytes of 
data in one of the TIFFTag type where the present change is done. We will not 
be able to pass that amount of data in byteArray stream. Also if we want to 
pass raw data as part of a TIFFTag i need relevant TIFFtag data like ICCProfile 
in the image attached in JBS.

 So i am leaving discussion open so that others can give inputs on ways we can 
put relevant data into our TIFFImageWriter to hit this issue.

-

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

Re: [OpenJDK 2D-Dev] RFR: 8270893: IndexOutOfBoundsException while reading large TIFF file

[Jayathirth D V]

On Wed, 21 Jul 2021 22:15:35 GMT, Phil Race  wrote:

> > Is it possible to create a testcase for this fix? Probably this regression 
> > should be fixed in jdk17?
> 
> FWIW the fixer had already written :
> "Also to hit this code we need stream/file with at-least 1MB of IFD data, 
> that's why there is no regression test"
> 
> Maybe 1Mb isn't actually a blocker, and it would be good to learn if we can 
> create a TIFF image of that size and have github accept it.
> 
> But we definitely should run all the relevant existing regression tests.
> 
> As for JDk 17, we are in RDP 2 and this has been out there for exactly (?) 12 
> months until the first report and will need backporting anyway to other 
> release trains.
> So it is safer to put in 18 and backport later to a 17 update

Sure i will take a look at creating regression test case for this issue and see 
whether we can hit this particular path.
All client tier test cases were run and CI job link is present in the JBS and 
it is green with fix.
Yes i am planning to backport it to the trains where the original issue was 
fixed.

-

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

Re: [OpenJDK 2D-Dev] [jdk17] RFR: 8266079: Lanai: AlphaComposite shows differences on Metal compared to OpenGL [v3]

[Jayathirth D V]

On Mon, 5 Jul 2021 15:51:46 GMT, Alexey Ushakov  wrote:

>> The latest version of this PR re-introduces 
>> [JDK-8243547](https://bugs.openjdk.java.net/browse/JDK-8243547)
>
>> The latest version of this PR re-introduces 
>> [JDK-8243547](https://bugs.openjdk.java.net/browse/JDK-8243547)
> 
> Fixed

@avu Since we are past RDP2, lets close this JDK17 PR and open new jdk-dev(18) 
PR to continue the review.

-

PR: https://git.openjdk.java.net/jdk17/pull/62

Re: [OpenJDK 2D-Dev] [jdk17] RFR: 8266079: Lanai: AlphaComposite shows differences on Metal compared to OpenGL [v2]

[Jayathirth D V]

On Wed, 23 Jun 2021 12:15:03 GMT, Alexey Ushakov  wrote:

>> Implemented blit via compute kernel
>
> Alexey Ushakov has refreshed the contents of this pull request, and previous 
> commits have been removed. The incremental views will show differences 
> compared to the previous content of the PR. The pull request contains one new 
> commit since the last revision:
> 
>   8266079: Lanai: AlphaComposite shows differences on Metal compared to OpenGL
>   
>   Keep MTLLayer opacity in sync with window content view

All test run is green with latest commit.

-

PR: https://git.openjdk.java.net/jdk17/pull/62

[OpenJDK 2D-Dev] [jdk17] Integrated: 8267602: [macos] [lanai] java/awt/PrintJob/Text/stringwidth.sh doesn't exit on cancelling print dialog

[Jayathirth D V]

On Tue, 29 Jun 2021 17:34:00 GMT, Jayathirth D V  wrote:

> Final blit operation in MTLLayer.blitTexture() is driven by CVDisplayLink in 
> Metal.
> In this test case we are hitting an invalid condition because of which we 
> exit from MTLLayer.blitTexture(), but we are not stopping the CVDisplayLink. 
> This is causing the CVDisplayLink callback to run in loop. Fix is to stop 
> CVDisplayLink when we return without completing final blit operation in 
> MTLLayer.blitTexture().
> 
> Sanity and performance analysis is green. More details in JBS.

This pull request has now been integrated.

Changeset: f7ffd587
Author:Jayathirth D V 
URL:   
https://git.openjdk.java.net/jdk17/commit/f7ffd5872d69633c89505ce3e4fef9df8293e76b
Stats: 6 lines in 1 file changed: 6 ins; 0 del; 0 mod

8267602: [macos] [lanai] java/awt/PrintJob/Text/stringwidth.sh doesn't exit on 
cancelling print dialog

Reviewed-by: aghaisas, serb

-

PR: https://git.openjdk.java.net/jdk17/pull/175

Re: [OpenJDK 2D-Dev] [jdk17] RFR: 8267602: [macos] [lanai] java/awt/PrintJob/Text/stringwidth.sh doesn't exit on cancelling print dialog [v2]

[Jayathirth D V]

On Wed, 30 Jun 2021 14:37:31 GMT, Jayathirth D V  wrote:

>> Final blit operation in MTLLayer.blitTexture() is driven by CVDisplayLink in 
>> Metal.
>> In this test case we are hitting an invalid condition because of which we 
>> exit from MTLLayer.blitTexture(), but we are not stopping the CVDisplayLink. 
>> This is causing the CVDisplayLink callback to run in loop. Fix is to stop 
>> CVDisplayLink when we return without completing final blit operation in 
>> MTLLayer.blitTexture().
>> 
>> Sanity and performance analysis is green. More details in JBS.
>
> Jayathirth D V has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Remove stopDisplayLink call when nextDrawableCount is not zero

Verified that all test run is green with latest commit also.

-

PR: https://git.openjdk.java.net/jdk17/pull/175

Re: [OpenJDK 2D-Dev] [jdk17] RFR: 8267602: [macos] [lanai] java/awt/PrintJob/Text/stringwidth.sh doesn't exit on cancelling print dialog [v2]

[Jayathirth D V]

> Final blit operation in MTLLayer.blitTexture() is driven by CVDisplayLink in 
> Metal.
> In this test case we are hitting an invalid condition because of which we 
> exit from MTLLayer.blitTexture(), but we are not stopping the CVDisplayLink. 
> This is causing the CVDisplayLink callback to run in loop. Fix is to stop 
> CVDisplayLink when we return without completing final blit operation in 
> MTLLayer.blitTexture().
> 
> Sanity and performance analysis is green. More details in JBS.

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

  Remove stopDisplayLink call when nextDrawableCount is not zero

-

Changes:
  - all: https://git.openjdk.java.net/jdk17/pull/175/files
  - new: https://git.openjdk.java.net/jdk17/pull/175/files/5b5914ab..cca4c900

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk17=175=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk17=175=00-01

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

PR: https://git.openjdk.java.net/jdk17/pull/175

Re: [OpenJDK 2D-Dev] [jdk17] RFR: 8267602: [macos] [lanai] java/awt/PrintJob/Text/stringwidth.sh doesn't exit on cancelling print dialog

[Jayathirth D V]

On Wed, 30 Jun 2021 11:36:18 GMT, Ajit Ghaisas  wrote:

>> Final blit operation in MTLLayer.blitTexture() is driven by CVDisplayLink in 
>> Metal.
>> In this test case we are hitting an invalid condition because of which we 
>> exit from MTLLayer.blitTexture(), but we are not stopping the CVDisplayLink. 
>> This is causing the CVDisplayLink callback to run in loop. Fix is to stop 
>> CVDisplayLink when we return without completing final blit operation in 
>> MTLLayer.blitTexture().
>> 
>> Sanity and performance analysis is green. More details in JBS.
>
> src/java.desktop/macosx/native/libawt_lwawt/java2d/metal/MTLLayer.m line 88:
> 
>> 86: 
>> 87: if (self.nextDrawableCount != 0) {
>> 88: [self stopDisplayLink];
> 
> Please check invoking stopDisplayLink at this place.  If a Drawable is not 
> available, we should return from here but recheck after 16ms. A drawable 
> might be made available on subsequent attempts.
> 
> Stopping DisplayLink at other invalid cases makes sense.

@aghaisas Thanks for the review. Redraw of static content is driven by 
setNeedsDisplay, because we stop CVDisplayLink once final blit is done. Also 
with preliminary testing i see that we are not hitting this condition at all 
after CVDisplayLink is enabled. We still have a verification task under 
https://bugs.openjdk.java.net/browse/JDK-8258583 . Stopping or not stopping 
CVDisplayLink at this place is not causing any change. I will go ahead and 
remove stopDisplayLink at this place, in future if we hit this code because of 
some boundary condition we can enable it and verify.

-

PR: https://git.openjdk.java.net/jdk17/pull/175

Re: [OpenJDK 2D-Dev] [jdk17] RFR: 8267602: [macos] [lanai] java/awt/PrintJob/Text/stringwidth.sh doesn't exit on cancelling print dialog

[Jayathirth D V]

On Tue, 29 Jun 2021 23:28:44 GMT, Sergey Bylokhov  wrote:

> Please confirm that mach5 is green.

@mrserb Yes CI test run is green. Test link in JBS.

-

PR: https://git.openjdk.java.net/jdk17/pull/175

Re: [OpenJDK 2D-Dev] [jdk17] RFR: 8267602: [macos] [lanai] java/awt/PrintJob/Text/stringwidth.sh doesn't exit on cancelling print dialog

[Jayathirth D V]

On Wed, 30 Jun 2021 04:29:56 GMT, Sergey Bylokhov  wrote:

>> At this point if we exit, we just return and unlock MTLRenderQueue for 
>> backbuffer rendering. And when it is done we again start CVDisplayLink in 
>> MTLRenderQueue which in turn calls setNeedsDisplay to get callback to 
>> MTLLayer.display().
>> 
>> This scenario is same as returning from CGLLayer.blitTexture() when 
>> textureID is 0. Appkit thread will be running in the background and we dont 
>> stop it.
>
> No, I meant if we dispose the frame/exist from the app/etc before 
> "Java_sun_java2d_metal_MTLLayer_blitTexture" method is executed but after we 
> call "startDisplayLink", when we will call the stopDisplayLink in this case?

@mrserb Please let me know if i can resolve this conversation.

-

PR: https://git.openjdk.java.net/jdk17/pull/175

Re: [OpenJDK 2D-Dev] [jdk17] RFR: 8267602: [macos] [lanai] java/awt/PrintJob/Text/stringwidth.sh doesn't exit on cancelling print dialog

[Jayathirth D V]

On Wed, 30 Jun 2021 04:29:56 GMT, Sergey Bylokhov  wrote:

>> At this point if we exit, we just return and unlock MTLRenderQueue for 
>> backbuffer rendering. And when it is done we again start CVDisplayLink in 
>> MTLRenderQueue which in turn calls setNeedsDisplay to get callback to 
>> MTLLayer.display().
>> 
>> This scenario is same as returning from CGLLayer.blitTexture() when 
>> textureID is 0. Appkit thread will be running in the background and we dont 
>> stop it.
>
> No, I meant if we dispose the frame/exist from the app/etc before 
> "Java_sun_java2d_metal_MTLLayer_blitTexture" method is executed but after we 
> call "startDisplayLink", when we will call the stopDisplayLink in this case?

In that case it will hit MTLLayer.dealloc() where we stop and release 
CVDisplayLink.

-

PR: https://git.openjdk.java.net/jdk17/pull/175

Re: [OpenJDK 2D-Dev] [jdk17] RFR: 8267602: [macos] [lanai] java/awt/PrintJob/Text/stringwidth.sh doesn't exit on cancelling print dialog

[Jayathirth D V]

On Tue, 29 Jun 2021 23:31:33 GMT, Sergey Bylokhov  wrote:

>> Final blit operation in MTLLayer.blitTexture() is driven by CVDisplayLink in 
>> Metal.
>> In this test case we are hitting an invalid condition because of which we 
>> exit from MTLLayer.blitTexture(), but we are not stopping the CVDisplayLink. 
>> This is causing the CVDisplayLink callback to run in loop. Fix is to stop 
>> CVDisplayLink when we return without completing final blit operation in 
>> MTLLayer.blitTexture().
>> 
>> Sanity and performance analysis is green. More details in JBS.
>
> src/java.desktop/macosx/native/libawt_lwawt/java2d/metal/MTLLayer.m line 286:
> 
>> 284: if (layer == NULL || ctx == NULL) {
>> 285: J2dTraceLn(J2D_TRACE_VERBOSE, "MTLLayer_blit : Layer or Context 
>> is null");
>> 286: [layer stopDisplayLink];
> 
> What happens if we exit before this "blitTexture" is called? Do not we need 
> to stop(w/o possibility to restart it) that thread on toolkit shutdown or 
> something like this?

At this point if we exit, we just return and unlock MTLRenderQueue for 
backbuffer rendering. And when it is done we again start CVDisplayLink in 
MTLRenderQueue which in turn calls setNeedsDisplay to get callback to 
MTLLayer.display().

This scenario is same as returning from CGLLayer.blitTexture() when textureID 
is 0. Appkit thread will be running in the background and we dont stop it.

-

PR: https://git.openjdk.java.net/jdk17/pull/175

[OpenJDK 2D-Dev] [jdk17] RFR: 8267602: [macos] [lanai] java/awt/PrintJob/Text/stringwidth.sh doesn't exit on cancelling print dialog

[Jayathirth D V]

Final blit operation in MTLLayer.blitTexture() is driven by CVDisplayLink in 
Metal.
In this test case we are hitting an invalid condition because of which we exit 
from MTLLayer.blitTexture(), but we are not stopping the CVDisplayLink. This is 
causing the CVDisplayLink callback to run in loop. Fix is to stop CVDisplayLink 
when we return without completing final blit operation in 
MTLLayer.blitTexture().

Sanity and performance analysis is green. More details in JBS.

-

Commit messages:
 - 8267602: [macos] [lanai] java/awt/PrintJob/Text/stringwidth.sh doesn't exit 
on cancelling print dialog

Changes: https://git.openjdk.java.net/jdk17/pull/175/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk17=175=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8267602
  Stats: 7 lines in 1 file changed: 7 ins; 0 del; 0 mod
  Patch: https://git.openjdk.java.net/jdk17/pull/175.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk17 pull/175/head:pull/175

PR: https://git.openjdk.java.net/jdk17/pull/175

Re: [OpenJDK 2D-Dev] [jdk17] RFR: 8266079: Lanai: AlphaComposite shows differences on Metal compared to OpenGL

[Jayathirth D V]

On Tue, 15 Jun 2021 16:57:00 GMT, Alexey Ushakov  wrote:

> Implemented blit via compute kernel

With this patch shaped windows with translucent/color background are showing 
only black background. In manual JCK shaped window test also this difference in 
behaviour is seen.

This behaviour difference can be checked using Ruler.java test case attached in 
https://bugs.openjdk.java.net/browse/JDK-8267963

-

PR: https://git.openjdk.java.net/jdk17/pull/62

Re: [OpenJDK 2D-Dev] [jdk17] RFR: 8266079: Lanai: AlphaComposite shows differences on Metal compared to OpenGL

[Jayathirth D V]

On Thu, 17 Jun 2021 07:06:24 GMT, Jayathirth D V  wrote:

>> Implemented blit via compute kernel
>
> src/java.desktop/macosx/native/libawt_lwawt/java2d/metal/MTLLayer.m line 136:
> 
>> 134: NSUInteger w = computePipelineState.threadExecutionWidth;
>> 135: 
>> 136: // Workaround for some OS/device bug reporting incorrect 
>> maxTotalThreadsPerThreadgroup
> 
> @avu Do we know in which hardware we have this issue? Also do we have any 
> reference to Apple bug?
> Also if we use thread group as 1 what is the performance impact in such 
> hardware?

Forced maxTotalThreadsPerThreadgroup to 1 and tested in Intel SoC 2015 Macbook 
Pro, as expected i see almost ~50% reduction in FPS numbers in many use cases 
in RenderPerfTest. This final blit hits common operation and we would be able 
to make better decision whether this performance reduction is okay or not based 
on hardware in which we are seeing Apple bug.

-

PR: https://git.openjdk.java.net/jdk17/pull/62

Re: [OpenJDK 2D-Dev] [jdk17] RFR: 8266079: Lanai: AlphaComposite shows differences on Metal compared to OpenGL

[Jayathirth D V]

On Wed, 16 Jun 2021 11:57:01 GMT, Jayathirth D V  wrote:

>> @jayathirthrao @aghaisas please let me know if I can push this change into 
>> JDK17 repository
>
> @avu i have given all test run. I will get back to you once it is done.

Automated jtreg/JCK test run is green.

-

PR: https://git.openjdk.java.net/jdk17/pull/62

Re: [OpenJDK 2D-Dev] [jdk17] RFR: 8266079: Lanai: AlphaComposite shows differences on Metal compared to OpenGL

[Jayathirth D V]

On Tue, 15 Jun 2021 16:57:00 GMT, Alexey Ushakov  wrote:

> Implemented blit via compute kernel

src/java.desktop/macosx/native/libawt_lwawt/java2d/metal/MTLLayer.m line 136:

> 134: NSUInteger w = computePipelineState.threadExecutionWidth;
> 135: 
> 136: // Workaround for some OS/device bug reporting incorrect 
> maxTotalThreadsPerThreadgroup

@avu Do we know in which hardware we have this issue? Also do we have any 
reference to Apple bug?
Also if we use thread group as 1 what is the performance impact in such 
hardware?

-

PR: https://git.openjdk.java.net/jdk17/pull/62

Re: [OpenJDK 2D-Dev] [jdk17] RFR: 8266079: Lanai: AlphaComposite shows differences on Metal compared to OpenGL

[Jayathirth D V]

On Wed, 16 Jun 2021 10:34:50 GMT, Alexey Ushakov  wrote:

>> src/java.desktop/macosx/native/libawt_lwawt/java2d/metal/MTLLayer.m line 159:
>> 
>>> 157: [computeEncoder endEncoding];
>>> 158: [cb commit];
>>> 159: #endif
>> 
>> This is better than changing the background color, but should be carefully 
>> checked. @jayathirthrao @aghaisas please take a look.
>> Since this affects every blit to the window I suggest running a full test 
>> run.
>
> @jayathirthrao @aghaisas please let me know if I can push this change into 
> JDK17 repository

@avu i have given all test run. I will get back to you once it is done.

-

PR: https://git.openjdk.java.net/jdk17/pull/62

Re: [OpenJDK 2D-Dev] RFR: 8266159: macOS ARM + Metal pipeline shows artifacts on Swing Menu with Java L [v2]

[Jayathirth D V]

On Mon, 7 Jun 2021 12:03:35 GMT, Ajit Ghaisas  wrote:

>> This PR fixes an issue exclusively seen on Apple M1 systems when SwingSet2 
>> demo is run with uiScale=1.0.
>> 
>> **Issue :**
>> SwingSet2 Demo - As reported in JBS description
>> J2DDemo - As reported in a comment on JBS
>> 
>> **Root Cause :** 
>> DrawPixel path is used only with uiScale=1.0. 
>> MTLPrimitiveTypePoint is used to draw a pixel while encoding a render 
>> command.
>> As mentioned in the documentation -
>> https://developer.apple.com/documentation/metal/mtlprimitivetype/mtlprimitivetypepoint?language=objc
>> 
>> "The vertex shader must provide [[point_size]], or the point size is 
>> undefined."
>> 
>> In our shader functions, we do not define this point size. It is harmless on 
>> x86_64 based mac systems, but causes visual artifacts on M1 mac systems.
>> 
>> **Solution :**
>>  Explicitly define point size in shader functions that draw 
>> MTLPrimitiveTypePoint.
>
> Ajit Ghaisas has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   add automated test

Marked as reviewed by jdv (Reviewer).

-

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

Re: [OpenJDK 2D-Dev] RFR: 8266159: macOS ARM + Metal pipeline shows artifacts on Swing Menu with Java L

[Jayathirth D V]

On Fri, 4 Jun 2021 11:14:46 GMT, Ajit Ghaisas  wrote:

> This PR fixes an issue exclusively seen on Apple M1 systems when SwingSet2 
> demo is run with uiScale=1.0.
> 
> **Issue :**
> SwingSet2 Demo - As reported in JBS description
> J2DDemo - As reported in a comment on JBS
> 
> **Root Cause :** 
> DrawPixel path is used only with uiScale=1.0. 
> MTLPrimitiveTypePoint is used to draw a pixel while encoding a render command.
> As mentioned in the documentation -
> https://developer.apple.com/documentation/metal/mtlprimitivetype/mtlprimitivetypepoint?language=objc
> 
> "The vertex shader must provide [[point_size]], or the point size is 
> undefined."
> 
> In our shader functions, we do not define this point size. It is harmless on 
> x86_64 based mac systems, but causes visual artifacts on M1 mac systems.
> 
> **Solution :**
>  Explicitly define point size in shader functions that draw 
> MTLPrimitiveTypePoint.

LGTM.
Please add CI test run link in JBS, once it is completed.

-

Marked as reviewed by jdv (Reviewer).

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

Re: [OpenJDK 2D-Dev] RFR: 8250658: Performance of ClipFlatOval Renderperf test is very low

[Jayathirth D V]

On Fri, 7 May 2021 22:29:53 GMT, Alexey Ushakov  wrote:

> Implemented indirect rendering (via stencil texture attachment) to stencil 
> texture

Its good that we have removed intermediate stencil blit operations using this 
approach.
LGTM.
jtreg/JCK all test run is fine.
J2DDemo, SwingSet2, Font2DTest is running fine in my Intel SoC Macbook pro.
RenderPerfTest : ClipFlatOval and ClipFlatOvalAA show improvement in FPS from 
~13 to ~24 in my Intel SoC Macbook pro.

@aghaisas  mentioned that he is also testing on discrete GPU Macbook Pro. Lets 
wait for his test output.

-

Marked as reviewed by jdv (Reviewer).

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

[OpenJDK 2D-Dev] Integrated: 8226384: Implement a better logic to switch between OpenGL and Metal pipeline

[Jayathirth D V]

On Tue, 4 May 2021 11:02:40 GMT, Jayathirth D V  wrote:

> We have many if else conditions to select OpenGL/Metal pipeline objects.
> Apart from initialization phase we should not fetch these objects everytime 
> checking whether we are using OpenGL/Metal pipeline.

This pull request has now been integrated.

Changeset: 53db2a0a
Author:Jayathirth D V 
URL:   
https://git.openjdk.java.net/jdk/commit/53db2a0acd4e208fb5cfb7108206ec667c7c4131
Stats: 186 lines in 5 files changed: 83 ins; 91 del; 12 mod

8226384: Implement a better logic to switch between OpenGL and Metal pipeline

Reviewed-by: prr

-

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

Re: [OpenJDK 2D-Dev] RFR: 8226384: Implement a better logic to switch between OpenGL and Metal pipeline [v2]

[Jayathirth D V]

On Mon, 10 May 2021 10:14:40 GMT, Jayathirth D V  wrote:

>> You can add a new abstract method to the parent class: getRenderQueue(), and 
>> override it in subclasses to call MTLRenderQueue/OGL.getInstance(); In this 
>> way, you can push to the parent more methods by calling just this new 
>> getRenderQueue().
>> 
>> I think the only methods which cannot be pushe dto the parent is 
>> replaceSurfaceData() due to insets usage.
>
> I tried making this change.
> Moved all methods except replaceSurfaceData() to CFLayer.java. Moved the jni 
> native declaration also to CFlayer.java.
> For this to link i need to change native JNI signatures also to map to 
> CFLayer. But this will result in duplicate JNI symbols. If i change name of 
> these native JNI calls to something like nativeMTL/CGLSetScale, i again have 
> to use if/else check to verify which signature to call. Introducing a common 
> native interface between MTLLayer.m and CGLLayer.m would not help because 
> their implementation is different(Also it is out of scope of this PR).
> 
> I am not finding ways to move methods accessing native JNI calls to common 
> CFLayer.

I will push this PR. If we find appropriate way to move more methods to 
CFLayer.java i will handle it in a separate bug.

-

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

Re: [OpenJDK 2D-Dev] RFR: 8226384: Implement a better logic to switch between OpenGL and Metal pipeline [v2]

[Jayathirth D V]

On Fri, 7 May 2021 19:44:31 GMT, Sergey Bylokhov  wrote:

>> In dispose() we call validate which internally calls corresponding 
>> RenderQueue's of OpenGL/Metal, thats why it is not moved to CFLayer.
>> For other getters since initialization of peer was happening in individual 
>> subclasses(which in turn calls OpenGL/Metal native initialization) i had 
>> kept getters also in subclasses, but we can move getters to CFLayer. I have 
>> updated the PR.
>
> You can add a new abstract method to the parent class: getRenderQueue(), and 
> override it in subclasses to call MTLRenderQueue/OGL.getInstance(); In this 
> way, you can push to the parent more methods by calling just this new 
> getRenderQueue().
> 
> I think the only methods which cannot be pushe dto the parent is 
> replaceSurfaceData() due to insets usage.

I tried making this change.
Moved all methods except replaceSurfaceData() to CFLayer.java. Moved the jni 
native declaration also to CFlayer.java.
For this to link i need to change native JNI signatures also to map to CFLayer. 
But this will result in duplicate JNI symbols. If i change name of these native 
JNI calls to something like nativeMTL/CGLSetScale, i again have to use if/else 
check to verify which signature to call. Introducing a common native interface 
between MTLLayer.m and CGLLayer.m would not help because their implementation 
is different(Also it is out of scope of this PR).

I am not finding ways to move methods accessing native JNI calls to common 
CFLayer.

-

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

Re: [OpenJDK 2D-Dev] RFR: 8226384: Implement a better logic to switch between OpenGL and Metal pipeline [v2]

[Jayathirth D V]

On Wed, 5 May 2021 07:37:23 GMT, Jayathirth D V  wrote:

>> We have many if else conditions to select OpenGL/Metal pipeline objects.
>> Apart from initialization phase we should not fetch these objects everytime 
>> checking whether we are using OpenGL/Metal pipeline.
>
> Jayathirth D V has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Move peer and getters to CFLayer

@mrserb Please share your inputs.

-

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

[OpenJDK 2D-Dev] Integrated: 8252758: Lanai: Optimize index calculation while copying glyphs

[Jayathirth D V]

On Wed, 5 May 2021 13:50:47 GMT, Jayathirth D V  wrote:

> Loop optimization while copying glyph content.
> J2DDemo, SwingSet2 and Font2DTest are green.

This pull request has now been integrated.

Changeset: 2438498a
Author:Jayathirth D V 
URL:   
https://git.openjdk.java.net/jdk/commit/2438498a3f6dfa53966a0f5b28af28617ca00e6b
Stats: 15 lines in 1 file changed: 0 ins; 0 del; 15 mod

8252758: Lanai: Optimize index calculation while copying glyphs

Reviewed-by: aghaisas, pbansal

-

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

Re: [OpenJDK 2D-Dev] RFR: 8226384: Implement a better logic to switch between OpenGL and Metal pipeline [v2]

[Jayathirth D V]

On Wed, 5 May 2021 02:34:52 GMT, Sergey Bylokhov  wrote:

>> Jayathirth D V has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   Move peer and getters to CFLayer
>
> src/java.desktop/macosx/classes/sun/java2d/opengl/CGLLayer.java line 60:
> 
>> 58: return ptr;
>> 59: }
>> 60: 
> 
> dispose and getBounds(probably some others) seems to have the same 
> implementation in both classes?

In dispose() we call validate which internally calls corresponding 
RenderQueue's of OpenGL/Metal, thats why it is not moved to CFLayer.
For other getters since initialization of peer was happening in individual 
subclasses(which in turn calls OpenGL/Metal native initialization) i had kept 
getters also in subclasses, but we can move getters to CFLayer. I have updated 
the PR.

-

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

Re: [OpenJDK 2D-Dev] RFR: 8262392: Update Mesa 3-D Headers to version 21.0.3

[Jayathirth D V]

On Tue, 4 May 2021 14:24:22 GMT, Phil Race  wrote:

> Upgrades OpenGL header files to the latest Mesa project ones.
> Build+test looks fine.

Marked as reviewed by jdv (Reviewer).

-

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

Re: [OpenJDK 2D-Dev] RFR: 8226384: Implement a better logic to switch between OpenGL and Metal pipeline

[Jayathirth D V]

On Tue, 4 May 2021 17:51:24 GMT, Phil Race  wrote:

> LGTM. Is there anything not covered by this fix ?

Hi Phil,

We still have some if else checks in initialization paths like 
CPlatformView.initialize(), CPlatformEmbeddedFrame and static access of 
RenderQueue methods like LWCToolkit.sync().

Thanks,
Jay

-

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

Re: [OpenJDK 2D-Dev] RFR: 8226384: Implement a better logic to switch between OpenGL and Metal pipeline

[Jayathirth D V]

On Tue, 4 May 2021 11:02:40 GMT, Jayathirth D V  wrote:

> We have many if else conditions to select OpenGL/Metal pipeline objects.
> Apart from initialization phase we should not fetch these objects everytime 
> checking whether we are using OpenGL/Metal pipeline.

jtreg/JCK all test run is green in both OpenGL and Metal.
Also sanity check using J2DDemo and SwingSet2 is fine.

-

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

[OpenJDK 2D-Dev] RFR: 8226384: Implement a better logic to switch between OpenGL and Metal pipeline

[Jayathirth D V]

We have many if else conditions to select OpenGL/Metal pipeline objects.
Apart from initialization phase we should not fetch these objects everytime 
checking whether we are using OpenGL/Metal pipeline.

-

Commit messages:
 - 8226384: Implement a better logic to switch between OpenGL and Metal pipeline

Changes: https://git.openjdk.java.net/jdk/pull/3851/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=3851=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8226384
  Stats: 111 lines in 5 files changed: 57 ins; 42 del; 12 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3851.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3851/head:pull/3851

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

Re: [OpenJDK 2D-Dev] RFR: 8266040: Lanai: Incorrect calculations of clipping boundaries

[Jayathirth D V]

On Wed, 28 Apr 2021 10:55:48 GMT, Alexey Ushakov  wrote:

> Adjust initial clipping values to correctly calculate shape clip boundary

LGTM.
jtreg and JCK all test run is fine.
J2DDemo and SwingSet2 looks good.
Ran RenderPerfTest and saw improvement in ClipFlatOval/ClipFlatOvalAA from 
~10FPS to ~14FPS in my machine.

-

Marked as reviewed by jdv (Reviewer).

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

Re: [OpenJDK 2D-Dev] RFR: 8266284: ProblemList java/awt/Graphics2D/DrawString/DrawRotatedStringUsingRotatedFont.java

[Jayathirth D V]

On Thu, 29 Apr 2021 07:57:17 GMT, Prasanta Sadhukhan  
wrote:

> It seems the newly deproblemlisted test 
> DrawRotatedStringUsingRotatedFont.java which is paasing in windows,linux-x64 
> and macos-x64 is failing in newly added linux-aarch64 systems.
> Problemlisting the test for now as it is font layout issue which might need 
> some upgrade in harfbuzz library in those CI systems or maybe in product.

Marked as reviewed by jdv (Reviewer).

-

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

Re: [OpenJDK 2D-Dev] RFR: 8264143 Lanai: RenderPerfTest.BgrSwBlitImage has artefacts on apple M1 [v3]

[Jayathirth D V]

On Wed, 7 Apr 2021 19:00:17 GMT, Denis Konoplev  wrote:

>> There was no code to check num of work items in compute shader.
>> Also, I've replaced four similar shaders.
>
> Denis Konoplev has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   8264143: Change uint8_t to unsigned char

LGTM.
Ran RenderPerfTest in my Intel Integrated SoC there are no issues.
CI test run of jtreg and JCK tests is fine. 
Basic sanity with J2DDemo and Swingset2 is also fine.

-

Marked as reviewed by jdv (Reviewer).

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

Re: [OpenJDK 2D-Dev] RFR: 8265304: Temporarily make Metal the default 2D rendering pipeline for macOS

[Jayathirth D V]

On Fri, 16 Apr 2021 05:57:13 GMT, Ajit Ghaisas  wrote:

> This PR makes Metal as the default Java2D rendering pipeline for macOS.
> 
> Note : from JBS description :
> The plan of record has always been that for JDK 17 the new Metal pipeline 
> will be OFF by default and must be explicitly enabled by setting a system 
> property.
> We are not changing that plan but to get more testing exposure we intend to 
> make it the default (instead of OpenGL) for approximately one month (from the 
> next build up to the 20th May build of JDK 17).

LGTM. Please make sure CI test run is fine before integration.

-

Marked as reviewed by jdv (Reviewer).

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

Re: [OpenJDK 2D-Dev] RFR: 8264318 Lanai: DrawHugeImageTest.java fails on apple M1

[Jayathirth D V]

On Wed, 7 Apr 2021 05:24:30 GMT, Denis Konoplev  wrote:

> Check if blit sizes are less than MTL_GPU_FAMILY_MAC_TXT_SIZE.
> 
> It's safe since we copy tile from the image with memcpy. 
> 
> // copy src pixels inside src bounds to buff
> for (int row = 0; row < sh; row++) {
> memcpy(buff.contents + (row * sw * srcInfo->pixelStride), raster, sw * 
> srcInfo->pixelStride);
> raster += (NSUInteger)srcInfo->scanStride;
> }

Marked as reviewed by jdv (Reviewer).

I will sponsor this PR, waiting for complete test run to finish.

-

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

Re: [OpenJDK 2D-Dev] RFR: 8264923: PNGImageWriter.write_zTXt throws Exception with a typo

[Jayathirth D V]

On Thu, 8 Apr 2021 13:00:49 GMT, Aleksey Shipilev  wrote:

> Noticed this when backporting JDK-8242557: there is a trivial copy-paste 
> error in exception message. See:
>  https://hg.openjdk.java.net/jdk/jdk/rev/645c71334acd#l1.58

Marked as reviewed by jdv (Reviewer).

-

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

Re: [OpenJDK 2D-Dev] RFR: 8263984: Invalidate printServices when there are no printers [v3]

[Jayathirth D V]

On Tue, 6 Apr 2021 20:38:57 GMT, Alexey Ivanov  wrote:

>> When `getAllPrinterNames()` returns null, the list of `printServices` is 
>> assigned a new empty array without invalidating old services which were in 
>> the array before.
>> 
>> The old print services should be invalidated.
>
> Alexey Ivanov has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Add a blank line to separate condition from body

Marked as reviewed by jdv (Reviewer).

-

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

Re: [OpenJDK 2D-Dev] RFR: 8258788: incorrect response to change in window insets [lanai]

[Jayathirth D V]

On Tue, 6 Apr 2021 12:51:47 GMT, Alexey Ushakov  wrote:

> > @avu Test passes without fix also.
> > @jayathirthrao Could you provide the details about your configuration along 
> > with parameters passed to jtreg ?

@avu I am running test in 13 inch Macbook Early 2015 with integrated Intel Iris 
Graphics 6100.
jtreg command i am using "jtreg -jdk: -Dsun.java2d.metal=true 
"

-

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

Re: [OpenJDK 2D-Dev] RFR: 8258788: incorrect response to change in window insets [lanai]

[Jayathirth D V]

On Tue, 6 Apr 2021 12:03:14 GMT, Jayathirth D V  wrote:

> > > Is it possible to automatically test it?
> > 
> > 
> > Yes, just added the test.
> 
> @avu Test passes without fix also.

@aghaisas Please verify the regression test behavior from your end also.

-

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

Re: [OpenJDK 2D-Dev] RFR: 8258788: incorrect response to change in window insets [lanai]

[Jayathirth D V]

On Fri, 2 Apr 2021 11:41:12 GMT, Alexey Ushakov  wrote:

> > Is it possible to automatically test it?
> 
> Yes, just added the test.

@avu Test passes without fix also.

-

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

Re: [OpenJDK 2D-Dev] RFR: 8258788: incorrect response to change in window insets [lanai]

[Jayathirth D V]

On Tue, 6 Apr 2021 12:03:14 GMT, Jayathirth D V  wrote:

>>> Is it possible to automatically test it?
>> 
>> Yes, just added the test.
>
>> > Is it possible to automatically test it?
>> 
>> Yes, just added the test.
> 
> @avu Test passes without fix also.

Verified test case attached in JBS : 
https://bugs.openjdk.java.net/browse/JDK-8258788 . I see that fix resolves 
identified issue in JBS. Also jtreg and JCK test run is green with and without 
Metal API validation flags.

-

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

[OpenJDK 2D-Dev] Integrated: 8264475: CopyArea ignores clip state in metal rendering pipeline

[Jayathirth D V]

On Wed, 31 Mar 2021 07:27:45 GMT, Jayathirth D V  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.

This pull request has now been integrated.

Changeset: 0039c18e
Author:    Jayathirth D V 
URL:   https://git.openjdk.java.net/jdk/commit/0039c18e
Stats: 41 lines in 2 files changed: 24 ins; 0 del; 17 mod

8264475: CopyArea ignores clip state in metal rendering pipeline
8251036: SwingSet2 - Dragging internal frame inside jframe leaves artifacts 
with MetalLookAndFeel

Reviewed-by: aghaisas, psadhukhan

-

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

Re: [OpenJDK 2D-Dev] RFR: 8264475: CopyArea ignores clip state in metal rendering pipeline [v4]

[Jayathirth D V]

On Sat, 3 Apr 2021 05:30:12 GMT, Sergey Bylokhov  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

Re: [OpenJDK 2D-Dev] RFR: 8264475: CopyArea ignores clip state in metal rendering pipeline [v4]

[Jayathirth D V]

On Sat, 3 Apr 2021 05:30:12 GMT, Sergey Bylokhov  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

Re: [OpenJDK 2D-Dev] RFR: 8264475: CopyArea ignores clip state in metal rendering pipeline [v4]

[Jayathirth D V]

On Sat, 3 Apr 2021 03:51:29 GMT, Sergey Bylokhov  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

Re: [OpenJDK 2D-Dev] RFR: 8264475: CopyArea ignores clip state in metal rendering pipeline [v4]

[Jayathirth D V]

On Fri, 2 Apr 2021 22:43:36 GMT, Sergey Bylokhov  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

Re: [OpenJDK 2D-Dev] RFR: 8264475: CopyArea ignores clip state in metal rendering pipeline [v4]

[Jayathirth D V]

> 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=3283=03
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=3283=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

Re: [OpenJDK 2D-Dev] RFR: 8264475: CopyArea ignores clip state in metal rendering pipeline [v3]

[Jayathirth D V]

On Thu, 1 Apr 2021 06:58:27 GMT, Prasanta Sadhukhan  
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 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

Re: [OpenJDK 2D-Dev] RFR: 8263984: Invalidate printServices when there are no printers [v2]

[Jayathirth D V]

On Thu, 1 Apr 2021 13:28:48 GMT, Alexey Ivanov  wrote:

>> When `getAllPrinterNames()` returns null, the list of `printServices` is 
>> assigned a new empty array without invalidating old services which were in 
>> the array before.
>> 
>> The old print services should be invalidated.
>
> Alexey Ivanov has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Revert indentation of if condition continuation line

Marked as reviewed by jdv (Reviewer).

-

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

Re: [OpenJDK 2D-Dev] RFR: 8263984: Invalidate printServices when there are no printers

[Jayathirth D V]

On Thu, 1 Apr 2021 12:57:44 GMT, Jayathirth D V  wrote:

>> Is it not right?
>> I admit I don't understand what you mean by _map_ here.
>> 
>> When the code is written like it was:
>> if ((printServices[j] instanceof Win32PrintService) &&
>> (!printServices[j].equals(defaultPrintService))) {
>> 
>> ((Win32PrintService)printServices[j]).invalidateService();
>> }
>> it's hard to scan: it's not clear what is part of the condition and what is 
>> the statement inside the if block.
>> 
>> I'd prefer to write it like this:
>> if ((printServices[j] instanceof Win32PrintService)
>> && (!printServices[j].equals(defaultPrintService))) {
>> 
>> 
>> ((Win32PrintService)printServices[j]).invalidateService();
>> }
>> That is moving the operator to the continuation line which makes it obvious 
>> it is a continuation line of the condition and adding a blank line before 
>> the statement in the code. It's still not perfect, however; and it changes 
>> the author in `blame` output.
>> 
>> I indented the continuation line by additional 8 spaces, which is also a 
>> common practice in Java, to visually separate the condition and the 
>> statement. In fact, it's IDE that updated the formatting, I decided to keep 
>> it because it makes the code clearer.
>> 
>> I can revert the change to this line if you like.
>> Or I can just add a blank line between the condition and the statement.
>> 
>> What is your preference?
>
> By mapping i mean same indentation for all conditions in if statement without 
> adding additional indentation for each continuation line like(Basically line 
> without your change of indentation)
> if ((condition1) &&
>  (condition2)) {
> }
> 
> or 
> 
> if ((condition1)
>  &&(condition2)) {
> }
> 
> I have not come across code in java.desktop where we add indentation at each 
> continuation line of 'if' condition.
> I understand difficulty to scan without indentation but then in cases where 
> we have multiple lines on continuation line in if statement we will easily 
> hit 80 characters limit.
> 
> If we want to differentiate between if conditions and actual statement 
> execution to improve readability, we can move the statement block to new line 
> like 
> if ((condition1) &&
>  (condition2))
> {
> }

I would prefer if you revert this line or if we want to put emphasis on 
readability moving '{' to new line also seems fine.

-

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

Re: [OpenJDK 2D-Dev] RFR: 8263984: Invalidate printServices when there are no printers

[Jayathirth D V]

On Thu, 1 Apr 2021 12:05:30 GMT, Alexey Ivanov  wrote:

>> src/java.desktop/windows/classes/sun/print/PrintServiceLookupProvider.java 
>> line 159:
>> 
>>> 157: for (int j=0; j < printServices.length; j++) {
>>> 158: if ((printServices[j] instanceof Win32PrintService) &&
>>> 159: 
>>> (!printServices[j].equals(defaultPrintService))) {
>> 
>> Is this indentation right? I thought we always map newline additional 
>> conditions and not add extra indentation.
>
> Is it not right?
> I admit I don't understand what you mean by _map_ here.
> 
> When the code is written like it was:
> if ((printServices[j] instanceof Win32PrintService) &&
> (!printServices[j].equals(defaultPrintService))) {
> ((Win32PrintService)printServices[j]).invalidateService();
> }
> it's hard to scan: it's not clear what is part of the condition and what is 
> the statement inside the if block.
> 
> I'd prefer to write it like this:
> if ((printServices[j] instanceof Win32PrintService)
> && (!printServices[j].equals(defaultPrintService))) {
> 
> ((Win32PrintService)printServices[j]).invalidateService();
> }
> That is moving the operator to the continuation line which makes it obvious 
> it is a continuation line of the condition and adding a blank line before the 
> statement in the code. It's still not perfect, however; and it changes the 
> author in `blame` output.
> 
> I indented the continuation line by additional 8 spaces, which is also a 
> common practice in Java, to visually separate the condition and the 
> statement. In fact, it's IDE that updated the formatting, I decided to keep 
> it because it makes the code clearer.
> 
> I can revert the change to this line if you like.
> Or I can just add a blank line between the condition and the statement.
> 
> What is your preference?

By mapping i mean same indentation for all conditions in if statement without 
adding additional indentation for each continuation line like(Basically line 
without your change of indentation)
if ((condition1) &&
 (condition2)) {
}

or 

if ((condition1)
 &&(condition2)) {
}

I have not come across code in java.desktop where we add indentation at each 
continuation line of 'if' condition.
I understand difficulty to scan without indentation but then in cases where we 
have multiple lines on continuation line in if statement we will easily hit 80 
characters limit.

If we want to differentiate between if conditions and actual statement 
execution to improve readability, we can move the statement block to new line 
like 
if ((condition1) &&
 (condition2))
{
}

-

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

Re: [OpenJDK 2D-Dev] RFR: 8263984: Invalidate printServices when there are no printers

[Jayathirth D V]

On Tue, 23 Mar 2021 13:45:33 GMT, Alexey Ivanov  wrote:

> When `getAllPrinterNames()` returns null, the list of `printServices` is 
> assigned a new empty array without invalidating old services which were in 
> the array before.
> 
> The old print services should be invalidated.

src/java.desktop/windows/classes/sun/print/PrintServiceLookupProvider.java line 
159:

> 157: for (int j=0; j < printServices.length; j++) {
> 158: if ((printServices[j] instanceof Win32PrintService) &&
> 159: 
> (!printServices[j].equals(defaultPrintService))) {

Is this indentation right? I thought we always map newline additional 
conditions and not add extra indentation.

-

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

Re: [OpenJDK 2D-Dev] RFR: 8263984: Invalidate printServices when there are no printers

[Jayathirth D V]

On Wed, 24 Mar 2021 20:44:26 GMT, Alexey Ivanov  wrote:

>> Since this is windows specific code, I am not sure if system will not have 
>> any printers. I guess by default, Microsoft XPS Document Writer, Microsoft 
>> Print-to-PDF, Fax are present in printers list. That is the reason we do not 
>> have getPrintService() return 0 printer although there may not be any real 
>> printer present in windows system...reason for some failure in jtreg test 
>> which caused us to use `@key printer `tag in those tests to make real 
>> printers are configured.
>
>> Since this is windows specific code, I am not sure if system will not have 
>> any printers. I guess by default, Microsoft XPS Document Writer, Microsoft 
>> Print-to-PDF, Fax are present in printers list.
> 
> Yes, that's correct. But you can remove them. You can also configure the 
> image so that these are not installed by default.
> 
>> That is the reason we do not have getPrintService() return 0 printer 
>> although there may not be any real printer present in windows 
>> system...reason for some failure in jtreg test which caused us to use `@key 
>> printer `tag in those tests to make real printers are configured.
> 
> I agree that getting zero printers in Windows is unlikely. But it's still 
> possible.
> 
> Consider the following scenario: Let's assume there are 10 printers in the 
> system. The user removes 5 of them. In this case, `invalidateService()` is 
> called on the instances of `Win32PrintService` which were removed from the 
> system.
> 
> Then the user removes the remaining 5 printers. In this case, 
> `invalidateService()` is not called at all. If an application has references 
> to any of these instances, they will continue to appear operational. However, 
> the flag `isInvalid` in `Win32PrintService` is used in two methods only: 
> `getPrinterState` and `getPrinterStateReasons`.
> 
> This fix is minor, probably this situation never occurs in the real life.
> 
> The difference in handling the deleted services caught my attention. If 
> everyone agrees it's not problem, I'll withdraw the PR and close the bug as 
> _Not an Issue_.

Change looks like good practice and needed, eventhough it may occur rarely.

-

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

Re: [OpenJDK 2D-Dev] RFR: 8264475: CopyArea ignores clip state in metal rendering pipeline [v3]

[Jayathirth D V]

> 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=3283=02
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=3283=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

Re: [OpenJDK 2D-Dev] RFR: 8264475: CopyArea ignores clip state in metal rendering pipeline

[Jayathirth D V]

On Wed, 31 Mar 2021 12:39:49 GMT, Ajit Ghaisas  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

Re: [OpenJDK 2D-Dev] RFR: 8264475: CopyArea ignores clip state in metal rendering pipeline [v2]

[Jayathirth D V]

> 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=3283=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=3283=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

[OpenJDK 2D-Dev] RFR: 8264475: CopyArea ignores clip state in metal rendering pipeline

[Jayathirth D V]

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=3283=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

Re: [OpenJDK 2D-Dev] RFR: 8264475: CopyArea ignores clip state in metal rendering pipeline

[Jayathirth D V]

On Wed, 31 Mar 2021 07:27:45 GMT, Jayathirth D V  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

Re: [OpenJDK 2D-Dev] RFR: 8260931: Implement JEP 382: New macOS Rendering Pipeline [v13]

[Jayathirth D V]

On Thu, 11 Mar 2021 18:00:15 GMT, Ajit Ghaisas  wrote:

>> **Description :**
>> This is the implementation of [JEP 382 : New macOS Rendering 
>> Pipeline](https://bugs.openjdk.java.net/browse/JDK-8238361)
>> It implements a Java 2D internal rendering pipeline for macOS using the 
>> Apple Metal API.
>> The entire work on this was done under [OpenJDK Project - 
>> Lanai](http://openjdk.java.net/projects/lanai/)
>> 
>> We iterated through several Early Access (EA) builds and have reached a 
>> stage where it is ready to be integrated to openjdk/jdk. The latest EA build 
>> is available at - https://jdk.java.net/lanai/
>> 
>> A new option -Dsun.java2d.metal=true | True needs to be used to use this 
>> pipeline.
>> 
>> **Testing :**
>> This implementation has been tested with the tests present at - [Test Plan 
>> for JEP 382: New macOS Rendering 
>> Pipeline](https://bugs.openjdk.java.net/browse/JDK-8251396)
>> 
>> **Note to reviewers :**
>> 1) Default rendering pipeline on macOS has not been changed by this PR. 
>> OpenGL still stays as the default rendering pipeline and Metal rendering 
>> pipeline is optional to choose.
>> 
>> 2) To apply and test this PR - 
>> To enable the metal pipeline you must specify on command line 
>> -Dsun.java2d.metal=true (No message will be printed in this case) or 
>> -Dsun.java2d.metal=True (A message indicating Metal rendering pipeline is 
>> enabled gets printed)
>> 
>> 3) Review comments (including some preliminary informal review comments) are 
>> tracked with JBS issues - https://bugs.openjdk.java.net/issues/?filter=40598
>
> Ajit Ghaisas has updated the pull request with a new target base due to a 
> merge or a rebase. The incremental webrev excludes the unrelated changes 
> brought in by the merge/rebase. The pull request contains 47 additional 
> commits since the last revision:
> 
>  - Lanai PR#214 - 8263324 - avu
>  - Merge branch 'master' into 8260931_lanai_JEP_branch
>  - Lanai PR#213 - 8263325 - avu
>  - Lanai PR#212 - 8259825 - aghaisas
>  - Lanai PR#211 - 8262882 - aghaisas
>  - Merge branch 'master' into 8260931_lanai_JEP_branch
>  - Lanai PR#210 - 8263159 - jdv
>  - Lanai PR#209 - 8262936 - jdv
>  - Lanai PR#208 - 8262928 - jdv
>  - Lanai PR#207 - 8262750 - jdv
>  - ... and 37 more: 
> https://git.openjdk.java.net/jdk/compare/46a8379b...369c3d21

Marked as reviewed by jdv (Reviewer).

-

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

Re: [OpenJDK 2D-Dev] RFR: 8260931: Implement JEP 382: New macOS Rendering Pipeline [v13]

[Jayathirth D V]

> On 12-Mar-2021, at 10:30 AM, Sergey Bylokhov  wrote:
> 
> On Fri, 12 Mar 2021 00:09:54 GMT, Kevin Rushforth  wrote:
> 
>>> Ajit Ghaisas has updated the pull request with a new target base due to a 
>>> merge or a rebase. The incremental webrev excludes the unrelated changes 
>>> brought in by the merge/rebase. The pull request contains 47 additional 
>>> commits since the last revision:
>>> 
>>> - Lanai PR#214 - 8263324 - avu
>>> - Merge branch 'master' into 8260931_lanai_JEP_branch
>>> - Lanai PR#213 - 8263325 - avu
>>> - Lanai PR#212 - 8259825 - aghaisas
>>> - Lanai PR#211 - 8262882 - aghaisas
>>> - Merge branch 'master' into 8260931_lanai_JEP_branch
>>> - Lanai PR#210 - 8263159 - jdv
>>> - Lanai PR#209 - 8262936 - jdv
>>> - Lanai PR#208 - 8262928 - jdv
>>> - Lanai PR#207 - 8262750 - jdv
>>> - ... and 37 more: 
>>> https://git.openjdk.java.net/jdk/compare/9d82be31...369c3d21
>> 
>> Latest changes look good. I confirmed that this PR has all of the content 
>> from the lanai repo.
> 
>> By default we don?t do LCD anti-aliasing for text, only when we set Text 
>> Rendering Hint to use LCD antialiasing we take sub-pixel rendering path.
> 
> And it actually produces LCD glyphs? not the grayscale?
We get glyphs which take more than a byte to represent each pixel.
Based on which we take LCD sub-pixel rendering path.
> 
> -
> 
> PR: https://git.openjdk.java.net/jdk/pull/2403

Re: [OpenJDK 2D-Dev] RFR: 8260931: Implement JEP 382: New macOS Rendering Pipeline [v13]

[Jayathirth D V]

> On 12-Mar-2021, at 9:29 AM, Scott Palmer  wrote:
> 
> 
>> On Mar 11, 2021, at 9:53 PM, Sergey Bylokhov  wrote:
>> 
>> On Fri, 12 Mar 2021 02:29:04 GMT, Jayathirth D V  wrote:
>> 
>>>> src/java.desktop/macosx/classes/sun/java2d/metal/MTLSurfaceData.java line 
>>>> 323:
>>>> 
>>>>> 321:  * more code just to support a few uncommon cases.
>>>>> 322:  */
>>>>> 323: public boolean canRenderLCDText(SunGraphics2D sg2d) {
>>>> 
>>>> Just curious, can we render LCD on 10.14+ via metal? Does it work fine?
>>> 
>>> Yes Sergey it works fine in 10.14+ systems via metal. Most of the JCK 
>>> manual tests use LCD text on UI Components and it is recently verified in 
>>> 10.14+ systems for EA10.
>> 
>> Ok, for some reason I thought that the new macOS stopped providing LCD 
>> glyphs.
>> 
> 
> Are the glyphs different or just the way they are rasterized?
> 
> Newer versions of macOS don’t do LCD text and have gone back to plain 
> grey-scale anti-aliasing. 
> Java 2D should default to NOT doing LCD anti-aliasing for text on macOS if it 
> wants to fit in with the look of native applications. 
> (I’m not sure if that applies to non-retina displays.)
> 
By default we don’t do LCD anti-aliasing for text, only when we set Text 
Rendering Hint to use LCD antialiasing we take sub-pixel rendering path.
> Scott

Re: [OpenJDK 2D-Dev] RFR: 8260931: Implement JEP 382: New macOS Rendering Pipeline [v13]

[Jayathirth D V]

On Fri, 12 Mar 2021 00:42:35 GMT, Sergey Bylokhov  wrote:

>> Ajit Ghaisas has updated the pull request with a new target base due to a 
>> merge or a rebase. The incremental webrev excludes the unrelated changes 
>> brought in by the merge/rebase. The pull request contains 47 additional 
>> commits since the last revision:
>> 
>>  - Lanai PR#214 - 8263324 - avu
>>  - Merge branch 'master' into 8260931_lanai_JEP_branch
>>  - Lanai PR#213 - 8263325 - avu
>>  - Lanai PR#212 - 8259825 - aghaisas
>>  - Lanai PR#211 - 8262882 - aghaisas
>>  - Merge branch 'master' into 8260931_lanai_JEP_branch
>>  - Lanai PR#210 - 8263159 - jdv
>>  - Lanai PR#209 - 8262936 - jdv
>>  - Lanai PR#208 - 8262928 - jdv
>>  - Lanai PR#207 - 8262750 - jdv
>>  - ... and 37 more: 
>> https://git.openjdk.java.net/jdk/compare/53b0bea8...369c3d21
>
> src/java.desktop/macosx/classes/sun/java2d/metal/MTLSurfaceData.java line 323:
> 
>> 321:  * more code just to support a few uncommon cases.
>> 322:  */
>> 323: public boolean canRenderLCDText(SunGraphics2D sg2d) {
> 
> Just curious, can we render LCD on 10.14+ via metal? Does it work fine?

Yes Sergey it works fine in 10.14+ systems via metal. Most of the JCK manual 
tests use LCD text on UI Components and it is recently verified in 10.14+ 
systems for EA10.

-

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

Re: [OpenJDK 2D-Dev] RFR: 8261170: Upgrade to freetype 2.10.4

[Jayathirth D V]

On Wed, 24 Feb 2021 18:34:31 GMT, Phil Race  wrote:

> This upgrades JDK (Java2D) from using freetype 2.10.2 to 2.10.4.
> A minor upgrade but there's been some refactoring of include files so most 
> files are touched.
> Testing looks good.

Marked as reviewed by jdv (Reviewer).

-

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

Re: [OpenJDK 2D-Dev] RFR: 8260931: Implement JEP 382: New macOS Rendering Pipeline [v4]

[Jayathirth D V]

On Thu, 11 Feb 2021 05:41:15 GMT, Ajit Ghaisas  wrote:

>> According to Xcode Instruments leak profile, there are 2 minor memory leaks 
>> in the Metal rendering pipeline:
>> 
>> `#1 Malloc 80 Bytes  1   0x7fde0d4247b0  80 Byteslibjava.dylib   
>> getStringUTF8`
>>0 libsystem_malloc.dylib malloc_zone_malloc
>>1 libsystem_malloc.dylib malloc
>>2 libjava.dylib getStringUTF8 
>> /Volumes/Work/review/2403/jdk/src/java.base/share/native/libjava/jni_util.c:888
>>3 libjava.dylib JNU_GetStringPlatformChars 
>> /Volumes/Work/review/2403/jdk/src/java.base/share/native/libjava/jni_util.c:917
>>4 libawt_lwawt.dylib 
>> Java_sun_java2d_metal_MTLGraphicsConfig_getMTLConfigInfo 
>> /Volumes/Work/review/2403/jdk/src/java.desktop/macosx/native/libawt_lwawt/java2d/metal/MTLGraphicsConfig.m:151
>>5  0x11ab08d48
>>6  0x11ab0250d
>> 
>> `#2 Malloc 80 Bytes  1   0x7fde0d4325a0  80 Byteslibjava.dylib   
>> getStringUTF8`
>>0 libsystem_malloc.dylib malloc_zone_malloc
>>1 libsystem_malloc.dylib malloc
>>2 libjava.dylib getStringUTF8 
>> /Volumes/Work/review/2403/jdk/src/java.base/share/native/libjava/jni_util.c:888
>>3 libjava.dylib JNU_GetStringPlatformChars 
>> /Volumes/Work/review/2403/jdk/src/java.base/share/native/libjava/jni_util.c:917
>>4 libawt_lwawt.dylib 
>> Java_sun_java2d_metal_MTLGraphicsConfig_tryLoadMetalLibrary 
>> /Volumes/Work/review/2403/jdk/src/java.desktop/macosx/native/libawt_lwawt/java2d/metal/MTLGraphicsConfig.m:120
>>5  0x11ab08d48
>>6  0x11ab024c8
>> 
>> Those can be handled as a followup issues though if you like, it's only 160 
>> bytes total.
>
>> I tried to code review the native implementation files, but Metal APIs is 
>> brand new to me and it's been a long while since I worked with graphics API, 
>> so I can't be of much help really.
>> 
>> The code I've looked at looked clean and nothing caught my eye. But it's a 
>> partial code review at best.
>> 
>> Good job!
> 
> Every bit helps. Thanks for your review effort!

> A small thing caught my eye when I was comparing OpenGL and Metal pipelines 
> running J2Demo.jar
> 
> It looks to me like OpenGL rendering pipeline uses brighter colors. I prefer 
> the more subdued colors that Metal is rendering, but not sure if this is 
> something you want to investigate further.
> 
> I tried to capture what I see below:
> 
> OpenGL:
> 
>  src="https://user-images.githubusercontent.com/63425797/107575744-dc0ca180-6bb5-11eb-9eb3-5cff415eb8a3.png;>
> 
> Metal (the green color has a yellowish tint):
> 
>  src="https://user-images.githubusercontent.com/63425797/107575716-cf884900-6bb5-11eb-8ae8-14212ec94e87.png;>
> 
> Performance wise I did not see much difference either way.

@gerard-ziemski Thanks for verifying the behavior.
Text present in Memory monitor/Performance in J2DDemo is not drawn using Metal 
rendering pipeline(drawGlyphList or any other rendering opcodes, I have 
verified it again in latest code). This is drawn using software renderloops and 
only thing happens from Metal pipeline is this content is used in blit opcode. 
Slight difference in color that we are seeing is related to how Metal 
internally handles blending of colors.

-

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

Re: [OpenJDK 2D-Dev] RFR: 8260931: Implement JEP 382: New macOS Rendering Pipeline [v3]

[Jayathirth D V]

On Mon, 8 Feb 2021 18:05:02 GMT, Gerard Ziemski  wrote:

>> Ajit Ghaisas has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Lanai PR#175 - 8261304 - aghaisas
>
> src/java.desktop/macosx/classes/sun/java2d/metal/MTLRenderQueue.java line 97:
> 
>> 95: public static void disposeGraphicsConfig(long pConfigInfo) {
>> 96: MTLRenderQueue rq = getInstance();
>> 97: rq.lock();
> 
> Is it allowed to have multiple `MTLRenderQueue` instances?
> 
> If not, then I see this pattern everywhere:
> 
> MTLRenderQueue rq = getInstance();
> rq.lock();
> {
>   ...
> }
> rq.unlock();
> why not just do:
> 
> MTLRenderQueue.lock();
> {
>   ...
> }
> MTLRenderQueue.unlock();
> 
> and have `MTLRenderQueue.lock()` and `MTLRenderQueue.unlock()` implement 
> getting the actual lock instance and locking/unlocking it?

Thanks for the recommendation. This is a common behavior among different 
pipelines :  D3D, OpenGL and Metal. Mentioned lock/unlock should be implemented 
in parent RenderQueue class after refactoring and it will hit other pipelines. 
Its better to not touch other pipelines as part of this JEP.

-

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

Re: [OpenJDK 2D-Dev] RFR: 8260380: Upgrade to LittleCMS 2.12

[Jayathirth D V]

On Tue, 9 Feb 2021 22:52:04 GMT, Phil Race  wrote:

> Minor upgrade from LCMS 2.11 -> 2.12
> Automated tests pass, 2D demo also looks good.

Marked as reviewed by jdv (Reviewer).

-

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

Re: [OpenJDK 2D-Dev] RFR: 6206189: Graphics2D.clip specifies incorrectly that a 'null' is a valid value for this method

[Jayathirth D V]

On Tue, 9 Feb 2021 12:54:26 GMT, Prasanta Sadhukhan  
wrote:

>> The spec says "s - the Shape to be intersected with the current Clip" so I 
>> assume it means there should be a current clip set, so that is why I have 
>> used setClip to "set" a clip. So, setClip() should be there as far I see.
>
> Also, if there is no clip set, then the spec statement " If s is null, this 
> method clears the current Clip" does not carry any meaning, so in both 
> regard, setClip() should be there, I presume.

@prsadhuk At first glance i also thought clip() should be called after calling 
setClip() but that is not the case.

If we see SunGraphics2D implementation of clip() we dont exit if there is no 
clip(usrClip object) is set using setClip(). So clip() doesnt depend on whether 
setClip() is used or not.

-

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

Re: [OpenJDK 2D-Dev] RFR: 8198343: Test java/awt/print/PrinterJob/TestPgfmtSetMPA.java may fail w/o printer

[Jayathirth D V]

On Wed, 3 Feb 2021 05:41:23 GMT, Prasanta Sadhukhan  
wrote:

> This test require a printer to test its objective, so printer existence check 
> is added.

Marked as reviewed by jdv (Reviewer).

-

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

Re: [OpenJDK 2D-Dev] RFR: 8255043: Incorrectly styled copyright text

[Jayathirth D V]

On Tue, 20 Oct 2020 08:17:27 GMT, Sergey Bylokhov  wrote:

> In some files, the copyright text is styled as a JavaDoc comment.
> Most of the affected files are tests, only one product file is affected:
> src/java.sql/share/classes/javax/sql/package-info.java
> 
> The chenge is trivial:
>  - /**
>  + /*
> * Copyright (c)

Marked as reviewed by jdv (Reviewer).

-

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

Re: [OpenJDK 2D-Dev] RFR: 8251123: doclint warnings about missing javadoc tags and comments

[Jayathirth D V]

On Fri, 25 Sep 2020 21:45:39 GMT, Sergey Bylokhov  wrote:

> We have a number of missing javadoc tags and comments in the desktop module.
> Most of the missing comments are related to the serialized form.
> 
> The fix:
>   - Adds missing comments to the non-static/non-transient fields(even 
> private) of the "serializable" classes
>   - Adds comments to the "serializable" classes even if those classes are 
> non-public
>   - Fixes references/adds missing tags to the special methods(like 
> readObject/writeObject)
>   - Delete the java.awt.PeerFixer class.
> 
> I need advice about what exact change should be reviewed in  the CSR(except 
> PeerFixer removal)
> 
> Note that in some cases I added the comments to the "implementation details", 
> so I did not specify it fully.
> 
> The old review request:
> https://mail.openjdk.java.net/pipermail/beans-dev/2020-August/000423.html

Marked as reviewed by jdv (Reviewer).

-

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

Re: [OpenJDK 2D-Dev] RFR: 8251123: doclint warnings about missing javadoc tags and comments

[Jayathirth D V]

On Fri, 25 Sep 2020 21:45:39 GMT, Sergey Bylokhov  wrote:

> We have a number of missing javadoc tags and comments in the desktop module.
> Most of the missing comments are related to the serialized form.
> 
> The fix:
>   - Adds missing comments to the non-static/non-transient fields(even 
> private) of the "serializable" classes
>   - Adds comments to the "serializable" classes even if those classes are 
> non-public
>   - Fixes references/adds missing tags to the special methods(like 
> readObject/writeObject)
>   - Delete the java.awt.PeerFixer class.
> 
> I need advice about what exact change should be reviewed in  the CSR(except 
> PeerFixer removal)
> 
> Note that in some cases I added the comments to the "implementation details", 
> so I did not specify it fully.
> 
> The old review request:
> https://mail.openjdk.java.net/pipermail/beans-dev/2020-August/000423.html

src/java.desktop/share/classes/java/awt/Window.java line 248:

> 246:  * Focus transfers for the lightweight components request should be 
> made
> 247:  * synchronously.
> 248:  */

More simpler way "Focus transfers should be synchronous for lightweight 
component requests"

-

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

Re: [OpenJDK 2D-Dev] RFR: 8251123: doclint warnings about missing javadoc tags and comments

[Jayathirth D V]

On Fri, 25 Sep 2020 21:45:39 GMT, Sergey Bylokhov  wrote:

> We have a number of missing javadoc tags and comments in the desktop module.
> Most of the missing comments are related to the serialized form.
> 
> The fix:
>   - Adds missing comments to the non-static/non-transient fields(even 
> private) of the "serializable" classes
>   - Adds comments to the "serializable" classes even if those classes are 
> non-public
>   - Fixes references/adds missing tags to the special methods(like 
> readObject/writeObject)
>   - Delete the java.awt.PeerFixer class.
> 
> I need advice about what exact change should be reviewed in  the CSR(except 
> PeerFixer removal)
> 
> Note that in some cases I added the comments to the "implementation details", 
> so I did not specify it fully.
> 
> The old review request:
> https://mail.openjdk.java.net/pipermail/beans-dev/2020-August/000423.html

Inputs from 
https://mail.openjdk.java.net/pipermail/beans-dev/2020-August/000424.html are 
incorporated or is this fresh
git review?

-

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

Re: [OpenJDK 2D-Dev] RFR: 8252199: Reimplement support of Type 1 fonts without MappedByteBuffer

[Jayathirth D V]

On Fri, 18 Sep 2020 20:23:25 GMT, Phil Race  wrote:

> I  changed it to read the file into a ByteBuffer to avoid the problem that we 
> can't control when NIO free's the mmaped
> buffer. This is a problem for leaking tmp Type1 font files when using 
> createFont and I thought maybe we could just
> change that case, but Type 1 fonts are getting very rare. Neither Mac nor 
> Windows ship them and Mac at least does not
> support them at all, and current Linuxes seem to be almost all TTF and OTF 
> fonts. Also T1 fonts are small, so any
> minimal peformance benefit is not worth it. There isn't a specific regression 
> test. I verified this on Ubuntu 20.04
> which has at least a (very) few Type 1 fonts.

Marked as reviewed by jdv (Reviewer).

-

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

[OpenJDK 2D-Dev] Integrated: 8252100: NumberOverflow in class MemoryCache

[Jayathirth D V]

On Tue, 15 Sep 2020 13:49:19 GMT, Jayathirth D V  wrote:

> Number overflow in javax/imageio/stream/MemoryCache.java
> I tried to reproduce the issue but was not able to create stream or 
> BufferedImage of such a large size because of array
> size limitations. Submitter has mentioned simple fix for overflow which has 
> solved his issue. I have verified the fix
> by running jtreg and compatibility tests and there are no failures.

This pull request has now been integrated.

Changeset: b87a1599
Author:Jayathirth D V 
URL:   https://git.openjdk.java.net/jdk/commit/b87a1599
Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod

8252100: NumberOverflow in class MemoryCache

Reviewed-by: prr, serb

-

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

Re: [OpenJDK 2D-Dev] RFR: 8253206: Enforce whitespace checking for additional source files

[Jayathirth D V]

On Tue, 15 Sep 2020 22:18:09 GMT, Kevin Rushforth  wrote:

> This adds the following extensions to the list of source files that git 
> jcheck will check for whitespace errors:
> 
>  .cc, .hh, .m, .mm
> 
> All files with the above extensions are now white-space clean after the fix 
> for
> [JDK-8240487](https://bugs.openjdk.java.net/browse/JDK-8240487). This will 
> help them stay that way.
> I just integrated a similar fix to `jfx` (with a few more source extensions 
> that aren't relevant for the JDK) in
> [JDK-8240499](https://bugs.openjdk.java.net/browse/JDK-8240499).

Marked as reviewed by jdv (Reviewer).

-

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

Re: [OpenJDK 2D-Dev] RFR: 8252070: Some platform-specific BLIT optimizations are not effective

[Jayathirth D V]

On Thu, 10 Sep 2020 22:47:53 GMT, Sergey Bylokhov  wrote:

> Some of our code assumes that the native system(XRender, D3D, OGL) makes some
> effective optimizations, but in some cases, we can do better.
> 
> One of the areas for improvement is direct blitting. If the source is much
> bigger than the destination we should not try to copy to the non-existent area
> and could cut coordinates accordingly.
> 
> The actual change is:
>   951 Rectangle dst =
>   952 new Rectangle(dx, dy, w, 
> h).intersection(dstData.getBounds());
>   953 if (dst.isEmpty()) {
>   972 // return
>   975 }
>   979 sx += dst.x - dx;
>   980 sy += dst.y - dy;
> 
> See performance data and some additional comments:
> https://bugs.openjdk.java.net/browse/JDK-8252070?focusedCommentId=14365864=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-14365864
> 
> The old review request:
> https://mail.openjdk.java.net/pipermail/2d-dev/2020-August/011007.html

Marked as reviewed by jdv (Reviewer).

-

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

Re: [OpenJDK 2D-Dev] RFR: 8252100: NumberOverflow in class MemoryCache

[Jayathirth D V]

On Wed, 16 Sep 2020 01:18:00 GMT, Sergey Bylokhov  wrote:

>> If pos is big enough you could still theoretically have overflow, but this 
>> is still better.
>
> You can try to create such images using some separate tools, BTW I have found 
> one here(32768 × 32768):
> https://forums.bohemia.net/forums/topic/202016-high-quality-maps-for-altis-and-stratis/

Hi Sergey,

I tried reading images from 
https://forums.bohemia.net/forums/topic/202016-high-quality-maps-for-altis-and-stratis/,
 we
are trying to create destination BufferedImage of huge size and again we will 
hit:

Exception in thread "main" java.lang.IllegalArgumentException: Invalid scanline 
stride
at 
java.desktop/java.awt.image.ComponentSampleModel.getBufferSize(ComponentSampleModel.java:268)

where we check scanline with Integer.MAX_VALUE. From the description in JBS i 
can see that submitter has his own plugin
for TIFF and that is the only reason that he is able to read such a large 
images. With default plugin we will hit this
issue whenever we try to read large images as it is not supported.

Thanks,
Jay

-

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

[OpenJDK 2D-Dev] RFR: 8252100: NumberOverflow in class MemoryCache

[Jayathirth D V]

Number overflow in javax/imageio/stream/MemoryCache.java
I tried to reproduce the issue but was not able to create stream or 
BufferedImage of such a large size because of array
size limitations. Submitter has mentioned simple fix for overflow which has 
solved his issue. I have verified the fix
by running jtreg and compatibility tests and there are no failures.

-

Commit messages:
 - 8252100: NumberOverflow in class MemoryCache

Changes: https://git.openjdk.java.net/jdk/pull/182/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=182=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8252100
  Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod
  Patch: https://git.openjdk.java.net/jdk/pull/182.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/182/head:pull/182

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

Re: [OpenJDK 2D-Dev] RFR: 8252817: Cleanup the classes in the java.awt.color package

[Jayathirth D V]

On Sat, 5 Sep 2020 21:32:42 GMT, Sergey Bylokhov  wrote:

> - Class declarations reformat
> - Docs cleanup

Looks good to me.

-

Marked as reviewed by jdv (Reviewer).

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

Re: [OpenJDK 2D-Dev] RFR: 8245400: Upgrade to LittleCMS 2.11

[Jayathirth D v]

+1.

Thanks,
Jay

> On 30-Aug-2020, at 6:09 AM, Sergey Bylokhov  
> wrote:
> 
> Looks fine.
> 
> On 28.08.2020 07:42, Philip Race wrote:
>> Bug : https://bugs.openjdk.java.net/browse/JDK-8245400
>> Webrev : http://cr.openjdk.java.net/~prr/8245400/
>> A rotuine 3rd party library upgrade.
>> All platforms build  + all tests pass.
>> -phil.
> 
> 
> -- 
> Best regards, Sergey.

Re: [OpenJDK 2D-Dev] [16] RFR JDK-8243674: Remove language tag length limit for iTXt chunk in PNGImageReader

[Jayathirth D v]

Thanks for the review Sergey.
The extra check for length will hit only in case of corrupted chunk. Since we 
don’t know the exact length of “languageTag” or “Translated keyword” or “Text” 
we use chunkLength to calculate remaining length.
This fix is to comply with spec. Testcase throws appropriate “Found non null 
terminated string” IIOException without this fix(because languageTag can be 
more than 80) and works properly with fix.

Regards,
Jay

> On 31-Jul-2020, at 4:25 AM, Sergey Bylokhov  
> wrote:
> 
> Hi, Jay.
> 
> The fix looks fine, but maybe we can to trigger added 
> IIOException(remainingLen < 0) by the test?
> 
> On 29.07.2020 02:41, Jayathirth D v wrote:
>> Hello All,
>> Please review the following fix for JDK 16:
>> Bug : https://bugs.openjdk.java.net/browse/JDK-8243674
>> Webrev : http://cr.openjdk.java.net/~jdv/8243674/webrev.00/
>> Issue : We have language tag length limit of 80 for iTXt chunk in 
>> PNGImageReader which is not spec 
>> compliant(http://www.libpng.org/pub/png/spec/1.2/PNG-Chunks.html#C.iTXt). 
>> There should not be limit on length of language tag.
>> Solution : Remove language tag restriction of 80. In PNGImageWriter we don’t 
>> enforce any limit on language tag length.
>> Thanks,
>> Jay
> 
> 
> -- 
> Best regards, Sergey.

Re: [OpenJDK 2D-Dev] RFR: 8166038 BufferedImage methods getTileGridXOffset() and getTileGridYOffset() return a non 0 value for sub images

[Jayathirth D v]

+1.

Thanks,
Jay

> On 28-Jul-2020, at 9:56 AM, Sergey Bylokhov  
> wrote:
> 
> The new version of the fix:
> 
> Bug: https://bugs.openjdk.java.net/browse/JDK-8166038
> Fix: http://cr.openjdk.java.net/~serb/8166038/webrev.02
> 
> In the new version, the test was updated based on the feedback.
> 
> On 01.04.2020 19:51, Sergey Bylokhov wrote:
>> Hello.
>> Please review the fix for jdk/client.
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8166038
>> Fix: http://cr.openjdk.java.net/~serb/8166038/webrev.00
>> The fix contributed by Martin Desruisseaux.
>> Initial discussion about the bug:
>> https://mail.openjdk.java.net/pipermail/2d-dev/2020-February/010576.html
>> Implementation of getTileGridXOffset() and getTileGridXOffset() in
>> BufferedImage seems in contradiction with specification. The
>> RenderedImage specification said:
>> Returns the X offset of the tile grid relative to the origin, i.e.,
>> the X coordinate of the upper-left pixel of tile (0, 0). (Note that
>> tile (0, 0) may not actually exist.)
>> Since BufferedImage has only one tile, always at index (0,0), the (x,y)
>> coordinates of the upper-left pixel of that tile should be the image
>> (minX, minY), which is always (0,0) in a BufferedImage. Indeed
>> BufferedImage.getTileGridXOffset() javadoc adds the following sentence:
>> This is always zero.
>> But the BufferedImage implementation is:
>> public int getTileGridXOffset() {
>>  return raster.getSampleModelTranslateX();
>> }
>> Which does not always returns zero.
> 
> 
> -- 
> Best regards, Sergey.
> 

[OpenJDK 2D-Dev] [16] RFR JDK-8243674: Remove language tag length limit for iTXt chunk in PNGImageReader

[Jayathirth D v]

Hello All,

Please review the following fix for JDK 16:

Bug : https://bugs.openjdk.java.net/browse/JDK-8243674 
 
Webrev : http://cr.openjdk.java.net/~jdv/8243674/webrev.00/ 
 

Issue : We have language tag length limit of 80 for iTXt chunk in 
PNGImageReader which is not spec 
compliant(http://www.libpng.org/pub/png/spec/1.2/PNG-Chunks.html#C.iTXt 
). There should 
not be limit on length of language tag.
Solution : Remove language tag restriction of 80. In PNGImageWriter we don’t 
enforce any limit on language tag length.

Thanks,
Jay

Re: [OpenJDK 2D-Dev] RFR: 8250755 Better cleanup for jdk/test/javax/imageio/plugins/shared/CanWriteSequence.java

[Jayathirth D v]

Thanks for the cleanup. +1.

Thanks,
Jay

> On 29-Jul-2020, at 2:28 PM, Sergey Bylokhov  
> wrote:
> 
> Hello.
> Please review the fix for jdk/client.
> 
> Bug: https://bugs.openjdk.java.net/browse/JDK-8250755
> Fix: http://cr.openjdk.java.net/~serb/8250755/webrev.00
> 
> The test in question creates two temp files. One of them properly cleaned 
> after
> http://cr.openjdk.java.net/~jdv/8183349/webrev.02/test/javax/imageio/plugins/shared/CanWriteSequence.java.sdiff.html
> 
> But there is another one created by the ImageOutputStream, so the 
> ImageOutputStream should be closed
> to delete this temp cache file.
> 
> -- 
> Best regards, Sergey.

Re: [OpenJDK 2D-Dev] PING: RFR: 8249215: JFrame::setVisible crashed with -Dfile.encoding=UTF-8

[Jayathirth D V]

Hi Yasumasa,

I tried after changing the locale to Japanese but I don’t see the issue.

Also tried to reproduce the issue by enabling/disabling setting "Beta:Use 
Unicode UTF-8 for worldwide language support" in my locale setting.

@Others : Can somebody else try to reproduce this issue?

Thanks,
Jay

-Original Message-
From: Yasumasa Suenaga  
Sent: Thursday, July 23, 2020 5:41 PM
To: Jayathirth D v 
Cc: 2d-dev <2d-dev@openjdk.java.net>; awt-...@openjdk.java.net
Subject: Re: [OpenJDK 2D-Dev] PING: RFR: 8249215: JFrame::setVisible crashed 
with -Dfile.encoding=UTF-8

Hi Jay,

On 2020/07/23 19:09, Jayathirth D v wrote:
> Hi,
> 
> I tried reproducing the issue in my Windows 10 machine with UTF-8 encoding 
> and test file mentioned in the bug, I don’t see any crash.
> Am I missing something?

OS locale may be affecting.

My laptop has been set Japanese (CP932 / Windows-31J), so WFontConfiguration 
attempt to find Japanese font by default.
However WFontConfiguration cannot find out the font of "DEFAULT_CHARSET" when 
-Dfile.encoding=UTF-8 is passed.


Thanks,

Yasumasa


> Also I think this should be in awt-dev so adding the mailing list.
> 
> Thanks,
> Jay
> 
>> On 20-Jul-2020, at 12:59 PM, Yasumasa Suenaga  
>> wrote:
>>
>> PING: could you review it?
>>
>>>JBS: https://bugs.openjdk.java.net/browse/JDK-8249215
>>>webrev: http://cr.openjdk.java.net/~ysuenaga/JDK-8249215/webrev.00/
>>
>> Yasumasa
>>
>> On 2020/07/11 17:39, Yasumasa Suenaga wrote:
>>> Hi all,
>>> Please review this change:
>>>JBS: https://bugs.openjdk.java.net/browse/JDK-8249215
>>>webrev: http://cr.openjdk.java.net/~ysuenaga/JDK-8249215/webrev.00/
>>> I tried to run Sample.java in JDK-8236161 with -Dfile.encoding=UTF-8, but 
>>> JVM crashed due to internal error on fastdebug VM. I saw same call stack 
>>> with JDK-8236161 in hs_err log.
>>> I investigated it, then I found out current implementation cannot handle 
>>> default charset.
>>> If charset is set to UTF-8, it would be handled as "DEFAULT_CHARSET" in 
>>> WFontConfiguration::initTables. However it does not affect native font 
>>> name, so we cannot find valid font.
>>> This change has passed all tests on submit repo 
>>> (mach5-one-ysuenaga-JDK-8249215-20200711-0655-12566039)
>>> Thanks,
>>> Yasumasa
> 

Re: [OpenJDK 2D-Dev] PING: RFR: 8249215: JFrame::setVisible crashed with -Dfile.encoding=UTF-8

[Jayathirth D v]

Hi,

I tried reproducing the issue in my Windows 10 machine with UTF-8 encoding and 
test file mentioned in the bug, I don’t see any crash.
Am I missing something?

Also I think this should be in awt-dev so adding the mailing list.

Thanks,
Jay

> On 20-Jul-2020, at 12:59 PM, Yasumasa Suenaga  wrote:
> 
> PING: could you review it?
> 
>>   JBS: https://bugs.openjdk.java.net/browse/JDK-8249215
>>   webrev: http://cr.openjdk.java.net/~ysuenaga/JDK-8249215/webrev.00/
> 
> Yasumasa
> 
> On 2020/07/11 17:39, Yasumasa Suenaga wrote:
>> Hi all,
>> Please review this change:
>>   JBS: https://bugs.openjdk.java.net/browse/JDK-8249215
>>   webrev: http://cr.openjdk.java.net/~ysuenaga/JDK-8249215/webrev.00/
>> I tried to run Sample.java in JDK-8236161 with -Dfile.encoding=UTF-8, but 
>> JVM crashed due to internal error on fastdebug VM. I saw same call stack 
>> with JDK-8236161 in hs_err log.
>> I investigated it, then I found out current implementation cannot handle 
>> default charset.
>> If charset is set to UTF-8, it would be handled as "DEFAULT_CHARSET" in 
>> WFontConfiguration::initTables. However it does not affect native font name, 
>> so we cannot find valid font.
>> This change has passed all tests on submit repo 
>> (mach5-one-ysuenaga-JDK-8249215-20200711-0655-12566039)
>> Thanks,
>> Yasumasa

Re: [OpenJDK 2D-Dev] RFR JDK-8246742: ServiceUI.printDialog does not support properties dialog

[Jayathirth D v]

Hi Prasanta,

I again tested updated webrev in Windows and Linux and it works as expected.

webrev.1 looks good to me.

Thanks,
Jay

> On 20-Jul-2020, at 8:45 PM, Prasanta Sadhukhan 
>  wrote:
> 
> Actually, I was a bit wrong in construing that the button should just be 
> disabled for ServiceUI.printDialog if ServiceUIFactory is null & 
> getUI(MAIN_ROLE...) is null. 
> 
> The "properties" dialog is used for another use-case which is for 
> cross-platform dialog also ie, when we have a printer job created using (in 
> which case also getUI() will be null for windows)
> 
> PrintRequestAttributeSet pSet =  new HashPrintRequestAttributeSet();
> pSet.add(DialogTypeSelection.COMMON);
> pj.printDialog(pSet);
> And, as per JDK-4673406, it  is likely that supporting this "properties" 
> sheet will only be possible where there is already a job with which
> to make the association. ie using 
> java.awt.PrinterJob.printDialog(AttributeSet) and not for direct uses with 
> javax.print.ServiceUI.printDialog(..)"
> 
> So, ideally, we should be checking if we have a printer job in addition to 
> ServiceUIFactory being not null to allow creation of association between 
> printer properties and the printer job 
> so if a PrinterJob cross-platform printer-dialog is created, then we should 
> support "Properties" dialog. 
> If we directly use javax.print.ServiceUI.printDialog(..), then in that case 
> no printer job will be created, so properties dialog can be disabled for that 
> use-case.
> 
> Modified webrev: http://cr.openjdk.java.net/~psadhukhan/8246742/webrev.1/ 
> <http://cr.openjdk.java.net/~psadhukhan/8246742/webrev.1/>
> 
> Regards
> Prasanta
> On 20-Jul-20 3:53 PM, Jayathirth D v wrote:
>> Hi Prasanta,
>> 
>> Is just checking for dialog is null fine or we also need to verify 
>> DocumentProptiesUI in our case? As done in else case when dialog is null at 
>> http://hg.openjdk.java.net/jdk/client/file/fabf11c3a8ca/src/java.desktop/share/classes/sun/print/ServiceDialog.java#l801
>>  
>> <http://hg.openjdk.java.net/jdk/client/file/fabf11c3a8ca/src/java.desktop/share/classes/sun/print/ServiceDialog.java#l801>
>>  ?
>> 
>> I also see that Win32ServiceUIFactory
>> .getUI() doesnt return null in some specific DocumentsPropertyUI at 
>> http://hg.openjdk.java.net/jdk/client/file/754ec520eb4a/src/java.desktop/windows/classes/sun/print/Win32PrintService.java#l1692
>>  
>> <http://hg.openjdk.java.net/jdk/client/file/754ec520eb4a/src/java.desktop/windows/classes/sun/print/Win32PrintService.java#l1692>
>>  
>> 
>> Thanks,
>> Jay
>> 
>>> On 17-Jul-2020, at 11:22 AM, Prasanta Sadhukhan 
>>> mailto:prasanta.sadhuk...@oracle.com>> 
>>> wrote:
>>> 
>>> Hi Jay,
>>> 
>>> I am using the same methodology used to determine whether to show the 
>>> properties dialog when button-clicked on it (which is not to show if dialog 
>>> is null) as per
>>> 
>>> http://hg.openjdk.java.net/jdk/client/file/fabf11c3a8ca/src/java.desktop/share/classes/sun/print/ServiceDialog.java#l793
>>>  
>>> <http://hg.openjdk.java.net/jdk/client/file/fabf11c3a8ca/src/java.desktop/share/classes/sun/print/ServiceDialog.java#l793>
>>> So, if we cannot show the dialog using that mechanism, we can 
>>> enable/disable the button itself beforehand using that check.
>>> 
>>> Regards
>>> Prasanta.
>>> On 16-Jul-20 9:26 PM, Jayathirth D v wrote:
>>>> Hi Prasanta,
>>>> 
>>>> I tested the fix in Mac and Windows and it works as mentioned.
>>>> 
>>>> In 
>>>> http://hg.openjdk.java.net/jdk/client/file/754ec520eb4a/src/java.desktop/windows/classes/sun/print/Win32PrintService.java#l1688
>>>>  
>>>> <http://hg.openjdk.java.net/jdk/client/file/754ec520eb4a/src/java.desktop/windows/classes/sun/print/Win32PrintService.java#l1688>
>>>>  we are returning null when “role <= ServiceUIFactory.MAIN_UIROLE”. So it 
>>>> covers 3 roles “MAIN_UIROLE”, “ADMIN_UIROLE” and “ABOUT_UIROLE”.
>>>> 
>>>> In your webrev we are checking only for “MAIN_UIROLE”. Is creation of 
>>>> “JDIALOG_UI” restricted only to “MAIN_UIROLE”?
>>>> 
>>>> Thanks,
>>>> Jay
>>>> 
>>>>> On 15-Jul-2020, at 6:27 PM, Prasanta Sadhukhan 
>>>>> mailto:prasanta.sadhuk...@oracle.com>> 
>>>>> wrote:
>>>>> 
>>>>> Any reviewer for this?
>>>>> 
>>>>> On 09-Jul-20 

Re: [OpenJDK 2D-Dev] RFR JDK-8246742: ServiceUI.printDialog does not support properties dialog

[Jayathirth D v]

Hi Prasanta,

Is just checking for dialog is null fine or we also need to verify 
DocumentProptiesUI in our case? As done in else case when dialog is null at 
http://hg.openjdk.java.net/jdk/client/file/fabf11c3a8ca/src/java.desktop/share/classes/sun/print/ServiceDialog.java#l801
 
<http://hg.openjdk.java.net/jdk/client/file/fabf11c3a8ca/src/java.desktop/share/classes/sun/print/ServiceDialog.java#l801>
 ?

I also see that Win32ServiceUIFactory
.getUI() doesnt return null in some specific DocumentsPropertyUI at 
http://hg.openjdk.java.net/jdk/client/file/754ec520eb4a/src/java.desktop/windows/classes/sun/print/Win32PrintService.java#l1692
 
<http://hg.openjdk.java.net/jdk/client/file/754ec520eb4a/src/java.desktop/windows/classes/sun/print/Win32PrintService.java#l1692>
 

Thanks,
Jay

> On 17-Jul-2020, at 11:22 AM, Prasanta Sadhukhan 
>  wrote:
> 
> Hi Jay,
> 
> I am using the same methodology used to determine whether to show the 
> properties dialog when button-clicked on it (which is not to show if dialog 
> is null) as per
> 
> http://hg.openjdk.java.net/jdk/client/file/fabf11c3a8ca/src/java.desktop/share/classes/sun/print/ServiceDialog.java#l793
>  
> <http://hg.openjdk.java.net/jdk/client/file/fabf11c3a8ca/src/java.desktop/share/classes/sun/print/ServiceDialog.java#l793>
> So, if we cannot show the dialog using that mechanism, we can enable/disable 
> the button itself beforehand using that check.
> 
> Regards
> Prasanta.
> On 16-Jul-20 9:26 PM, Jayathirth D v wrote:
>> Hi Prasanta,
>> 
>> I tested the fix in Mac and Windows and it works as mentioned.
>> 
>> In 
>> http://hg.openjdk.java.net/jdk/client/file/754ec520eb4a/src/java.desktop/windows/classes/sun/print/Win32PrintService.java#l1688
>>  
>> <http://hg.openjdk.java.net/jdk/client/file/754ec520eb4a/src/java.desktop/windows/classes/sun/print/Win32PrintService.java#l1688>
>>  we are returning null when “role <= ServiceUIFactory.MAIN_UIROLE”. So it 
>> covers 3 roles “MAIN_UIROLE”, “ADMIN_UIROLE” and “ABOUT_UIROLE”.
>> 
>> In your webrev we are checking only for “MAIN_UIROLE”. Is creation of 
>> “JDIALOG_UI” restricted only to “MAIN_UIROLE”?
>> 
>> Thanks,
>> Jay
>> 
>>> On 15-Jul-2020, at 6:27 PM, Prasanta Sadhukhan 
>>> mailto:prasanta.sadhuk...@oracle.com>> 
>>> wrote:
>>> 
>>> Any reviewer for this?
>>> 
>>> On 09-Jul-20 1:10 PM, Prasanta Sadhukhan wrote:
>>>> Hi All,
>>>> 
>>>> Please review a fix for an issue where "Properties" button in 
>>>> ServiceUI.printDialog is enabled in windows but clicking it doesn't show 
>>>> any dialog.
>>>> 
>>>> According to JDK-4673406 
>>>> <https://bugs.openjdk.java.net/browse/JDK-4673406>, the properties dialog 
>>>> isn't supported for direct uses with javax.print.ServiceUI.printDialog, so 
>>>> it makes sense to disable this properies button for this usecase.
>>>> 
>>>> This button is disabled in linux,mac already. This is because, as per
>>>> 
>>>> http://hg.openjdk.java.net/jdk/client/annotate/754ec520eb4a/src/java.desktop/share/classes/sun/print/ServiceDialog.java#l964
>>>>  
>>>> <http://hg.openjdk.java.net/jdk/client/annotate/754ec520eb4a/src/java.desktop/share/classes/sun/print/ServiceDialog.java#l964>
>>>> the button is disabled if ServiceUIFactory is null and for linux/mac, it 
>>>> is null
>>>> 
>>>> http://hg.openjdk.java.net/jdk/client/file/754ec520eb4a/src/java.desktop/unix/classes/sun/print/IPPPrintService.java#l1637
>>>>  
>>>> <http://hg.openjdk.java.net/jdk/client/file/754ec520eb4a/src/java.desktop/unix/classes/sun/print/IPPPrintService.java#l1637>
>>>> http://hg.openjdk.java.net/jdk/client/file/754ec520eb4a/src/java.desktop/share/classes/sun/print/PSStreamPrintService.java#l490
>>>>  
>>>> <http://hg.openjdk.java.net/jdk/client/file/754ec520eb4a/src/java.desktop/share/classes/sun/print/PSStreamPrintService.java#l490>
>>>> but for windows, it created "Win32ServiceUIFactory" but it does not handle 
>>>> the properties dialog if "role" requested to be performed by ServiceUI is 
>>>> <= ServiceUIFactory.MAIN_UIROLE
>>>> 
>>>> [http://hg.openjdk.java.net/jdk/client/file/754ec520eb4a/src/java.desktop/windows/classes/sun/print/Win32PrintService.java#l1688
>>>>  
>>>> <http://hg.openjdk.java.net/jdk/client/file/754ec520eb4a/src/java.desktop/windows/classes/sun/print/Win32PrintService.java#l1688>]
>>>> 
>>>> Proposed fix is to make sure this role is accounted for in the 
>>>> buttonProperties enabling check.
>>>> 
>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8246742 
>>>> <https://bugs.openjdk.java.net/browse/JDK-8246742>
>>>> webrev: http://cr.openjdk.java.net/~psadhukhan/8246742/webrev.0/ 
>>>> <http://cr.openjdk.java.net/~psadhukhan/8246742/webrev.0/>Regards
>>>> Prasanta
>>