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

[Ajit Ghaisas]

On Mon, 5 Jul 2021 15:55:20 GMT, Alexey Ushakov  wrote:

>> Implemented blit via compute kernel
>
> Alexey Ushakov has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   8266079: Lanai: AlphaComposite shows differences on Metal compared to OpenGL
>   
>   Minor cleanup

I verified that the latest version does not re-introduce JDK-8243547.
All automated test run is also green with this version.

-

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 [v3]

[Ajit Ghaisas]

On Fri, 2 Jul 2021 17:49:17 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

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

-

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

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

[Ajit Ghaisas]

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

Marked as reviewed by aghaisas (Committer).

-

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

[Ajit Ghaisas]

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.

I tested this patch with applications such as SwingSet2 and NetBeans IDE with 
graphics card switching. Also, tested multi-monitor scenarios. No regression 
observed.

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.

-

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

[Ajit Ghaisas]

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

> Implemented blit via compute kernel

What I have observed with this patch is - It does not break all shaped or 
translucent windows - but, a manual JCK test does show the black background.

-

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

2d-dev@openjdk.java.net

[Ajit Ghaisas]

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.

This pull request has now been integrated.

Changeset: 89da2021
Author:Ajit Ghaisas 
URL:   
https://git.openjdk.java.net/jdk/commit/89da2021ee194efd70f367f8fec16994335c38aa
Stats: 103 lines in 2 files changed: 103 ins; 0 del; 0 mod

8266159: macOS ARM + Metal pipeline shows artifacts on Swing Menu with Java L&F

Reviewed-by: jdv, prr

-

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&F [v3]

[Ajit Ghaisas]

On Tue, 8 Jun 2021 07:30:00 GMT, Prasanta Sadhukhan  
wrote:

>> Ajit Ghaisas has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   fix review comment on test
>
> test/jdk/java/awt/Graphics/DrawOvalTest.java line 26:
> 
>> 24: /**
>> 25:  * @test
>> 26:  * @key headful
> 
> I feel there's nop need to have headful tag for this test as there is no 
> frame being made visible, all rendering are into volatileimage

There is a call to `getDefaultScreenDevice()` in test that throws 
`java.awt.HeadlessException` if the test is not marked as headful. Request you 
to file a follow-on bug if the test can be modified to make it headless.

-

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&F [v3]

[Ajit Ghaisas]

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

  fix review comment on test

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/4356/files
  - new: https://git.openjdk.java.net/jdk/pull/4356/files/751c85cf..714fe22a

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

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

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&F [v2]

[Ajit Ghaisas]

> 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

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/4356/files
  - new: https://git.openjdk.java.net/jdk/pull/4356/files/b61e512e..751c85cf

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

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

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

2d-dev@openjdk.java.net

[Ajit Ghaisas]

On Sat, 5 Jun 2021 07:46:30 GMT, Sergey Bylokhov  wrote:

> Is it possible to cover this fix with some automated test? Looks non of 
> existed tests found this issue.

Draw Pixel operation is unpredictable on macOS M1 with metal pipeline if 
[[point_size]] is not defined. Due to this unpredictability it is difficult to 
write an automated test.
I have developed a test which successfully detects this failure. It needs to be 
run with "-Dsun.java2d.uiScale=1.0". I will update the PR soon.

-

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

2d-dev@openjdk.java.net

[Ajit Ghaisas]

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.

-

Commit messages:
 - r

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

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

[OpenJDK 2D-Dev] Integrated: 8263486: Clean up MTLSurfaceDataBase.h

[Ajit Ghaisas]

On Thu, 13 May 2021 11:43:17 GMT, Ajit Ghaisas  wrote:

> This PR addresses some cleanup activities :
> - Cleaned up MTLSurfaceDataBase.h & MTLSurfaceData.m
> - Removed OpenGL references from MTLPipelineStatesStorage.m  & 
> MTLRenderQueue.m

This pull request has now been integrated.

Changeset: 72c9567b
Author:Ajit Ghaisas 
URL:   
https://git.openjdk.java.net/jdk/commit/72c9567b4663fc816e4b85b46ea49b20ea78bd72
Stats: 79 lines in 6 files changed: 0 ins; 55 del; 24 mod

8263486: Clean up MTLSurfaceDataBase.h

Reviewed-by: serb

-

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

Re: [OpenJDK 2D-Dev] RFR: 8263486: Clean up MTLSurfaceDataBase.h [v2]

[Ajit Ghaisas]

> This PR addresses some cleanup activities :
> - Cleaned up MTLSurfaceDataBase.h & MTLSurfaceData.m
> - Removed OpenGL references from MTLPipelineStatesStorage.m  & 
> MTLRenderQueue.m

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 three additional commits since 
the last revision:

 - remove x/yOffset fields
 - Merge branch 'master' into cleanup_MTLSurfaceDataBase
 - cleanup unused code

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/4010/files
  - new: https://git.openjdk.java.net/jdk/pull/4010/files/4c167898..5cb3dd45

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

  Stats: 27529 lines in 857 files changed: 12750 ins; 11509 del; 3270 mod
  Patch: https://git.openjdk.java.net/jdk/pull/4010.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/4010/head:pull/4010

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

Re: [OpenJDK 2D-Dev] RFR: 8263486: Clean up MTLSurfaceDataBase.h

[Ajit Ghaisas]

On Thu, 20 May 2021 21:48:18 GMT, Sergey Bylokhov  wrote:

>> This PR addresses some cleanup activities :
>> - Cleaned up MTLSurfaceDataBase.h & MTLSurfaceData.m
>> - Removed OpenGL references from MTLPipelineStatesStorage.m  & 
>> MTLRenderQueue.m
>
> src/java.desktop/macosx/native/libawt_lwawt/java2d/metal/MTLSurfaceData.m 
> line 35:
> 
>> 33: #include "jlong.h"
>> 34: 
>> 35: jboolean MTLSD_InitMTLWindow(JNIEnv *env, BMTLSDOps *bmtlsdo);
> 
> How the MTLSD_WINDOW is used? Do we use it for the layer-based rendering?

It is used in MTLContex.m. It just indicates a surface that we render to. We 
blit from this surface to the CAMetalLayer lateron.
It looks like MTLSD_WINDOW can be replaced with simple MTLSD_TEXTURE. This 
investigation and cleanup has already been identified during lanai code review 
(Bug - JDK-8263463)

> src/java.desktop/macosx/native/libawt_lwawt/java2d/metal/MTLSurfaceDataBase.h 
> line 56:
> 
>> 54:  * jint x/yOffset;
>> 55:  * The offset in pixels of the viewport origin from the lower-left
>> 56:  * corner of the heavyweight drawable.
> 
> Do we use these fields or they are always zero?

Thanks for your review. 
It turned out that - they are always zero. I will remove these fields.

-

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

Re: [OpenJDK 2D-Dev] RFR: 8267116: Lanai: Incorrect AlphaComposite for VolatileImage graphics [v3]

[Ajit Ghaisas]

On Mon, 17 May 2021 07:54:30 GMT, Alexey Ushakov  wrote:

>> Added one more bit for subIndex
>
> 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:
> 
>   8267116: Lanai: Incorrect AlphaComposite for VolatileImage graphics
>   
>   Added one more bit for subIndex

Marked as reviewed by aghaisas (Committer).

-

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

[OpenJDK 2D-Dev] Integrated: 8266520: Revert to OpenGL as the default 2D rendering pipeline for macOS

[Ajit Ghaisas]

On Mon, 17 May 2021 10:18:54 GMT, Ajit Ghaisas  wrote:

> This PR reverts to OpenGL as the default Java2D rendering pipeline for macOS.
> 
> Note : from JBS description :
> In JDK 17 build 19 under bug id 
> [JDK-8265304](https://bugs.openjdk.java.net/browse/JDK-8265304) we switched 
> to Metal as the default rendering pipeline with the plan that it would be 
> reverted to OpenGL after 4 builds.

This pull request has now been integrated.

Changeset: 79b39445
Author:Ajit Ghaisas 
URL:   
https://git.openjdk.java.net/jdk/commit/79b39445f6fcd005744c5de89ba2cd2ed5bc0a54
Stats: 7 lines in 1 file changed: 0 ins; 3 del; 4 mod

8266520: Revert to OpenGL as the default 2D rendering pipeline for macOS

Reviewed-by: azvegint, trebari, kcr, prr

-

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

[OpenJDK 2D-Dev] RFR: 8266520: Revert to OpenGL as the default 2D rendering pipeline for macOS

[Ajit Ghaisas]

This PR reverts to OpenGL as the default Java2D rendering pipeline for macOS.

Note : from JBS description :
In JDK 17 build 19 under bug id 
[JDK-8265304](https://bugs.openjdk.java.net/browse/JDK-8265304) we switched to 
Metal as the default rendering pipeline with the plan that it would be reverted 
to OpenGL after 4 builds.

-

Commit messages:
 - make ogl default on macOS

Changes: https://git.openjdk.java.net/jdk/pull/4052/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=4052&range=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8266520
  Stats: 7 lines in 1 file changed: 0 ins; 3 del; 4 mod
  Patch: https://git.openjdk.java.net/jdk/pull/4052.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/4052/head:pull/4052

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

Re: [OpenJDK 2D-Dev] Heads up : JDK 17 b19 through b22 will use Metal instead of OpenGL for Java 2D rendering on macOS.

[Ajit Ghaisas]

Hi,

   I have filed - https://bugs.openjdk.java.net/browse/JDK-8267226 
 for the reported observation.
   We will need a test program to see the issue.

Regards,
Ajit

> On 07-May-2021, at 3:34 AM, Philip Race  wrote:
> 
> > I’ve only tried a couple of apps and only this test program has shown the 
> > problem.
> 
> Did you try to attach something ? the lists strip attachments.
> Sent it directly to me and I'll see if I can add it to a bug report.
> 
> -phil.
> 
> On 5/6/21 2:50 PM, Alan Snyder wrote:
>> 
>>> On May 6, 2021, at 1:45 PM, Philip Race  wrote:
>>> 
>>> Alan,
>>> 
>>> I am not sure this is a known issue. We'll need a lot more details.
>> I figured you would. :-)
>> 
>> 
>>> What is your h+w and OS update ?
>> iMac 27 inch 2020 Radeon Pro 5500 XT 8 GB
>> 11.3.1
>> 
>> 
>>> Is this all windows in an app or just the first one ?
>> Definitely not just the first one, but not all of them, either.
>> 
>> 
>>> Does it matter what the window content is ?
>> It might. The app is a test program that can create any of 30 different 
>> kinds of windows on demand.
>> 
>> So far, I’ve seen the problem in 11 kinds of windows but not in the others. 
>> No obvious pattern in the content.
>> 
>> The problem is most likely to happen the first time a given window is shown, 
>> but it can also happen on later instances of the same kind of window.
>> I just tried a new kind of window and it happened the first 3 times, but not 
>> the 4th, then about 50% of the time.
>> I tried creating a different window about 25 times, and it happened on #1, 
>> #4, and #25.
>> 
>>> Any app or some specific app ?
>> I’ve only tried a couple of apps and only this test program has shown the 
>> problem.
>> 
>> 
>> 
>>> -phil.
>>> 
>>> 
>>> On 4/29/21 7:18 PM, Alan Snyder wrote:
 I am seeing some unusual behavior (in b20) that I do not see using OpenGL 
 (or using JDK 16).
 
 Sometimes when I open a new window, the window appears blank (except for 
 the title bar) for about two seconds before the content appears.
 
 This behavior is not consistent. Opening another instance of the same 
 window might be fast or slow. It happens with a variety of window classes.
 
 In JDK16 and using OpenGL, the content always appears immediately.
 
 
 
 
 
> On Apr 23, 2021, at 1:13 PM, Philip Race  wrote:
> 
> FYI to the wider community that may not subscribe to the client mailing 
> lists, nor appreciate too much cross-posting.
> 
> -phil.
> 
> 
>  Forwarded Message 
> Subject:  Heads up : JDK 17 b19 through b22 will use Metal instead of 
> OpenGL for Java 2D rendering on macOS.
> Date: Fri, 23 Apr 2021 13:10:46 -0700
> From: Philip Race 
> To:   2d-dev@openjdk.java.net <2d-dev@openjdk.java.net>
> CC:   lanai-...@openjdk.java.net, swing-...@openjdk.java.net 
> , awt-...@openjdk.java.net 
> 
> 
> 
> 
> 
> Heads up to anyone who is testing JDK 17 for running apps on macOS.
> Starting with build 19 [1], JDK 17 for macOS is *temporarily* switched 
> from using OpenGL
> to using Apple's Metal API for Java 2D rendering. This should be 
> invisible to applications.
> We expect to revert this temporary switch in JDK 17 build 23,meaning b22 
> will be the last build with Metal as default.
> 
> See JEP 382 [2] for more information about how Metal is used by JDK.
> 
> If you are running any kind of 2D / Swing/ AWT UI application on macOS, 
> and see any rendering related problems
> starting with JDK 17 b19, please do report them to us at either the usual 
> bug submission channel [3],
> or on the 2d-dev@openjdk.java.net OpenJDK mailing list [4]
> Please be ready to provide us with a test case and screen shots.
> 
> You may also set "-Dsun.java2d.opengl=true" to re-enable OpenGL - which  
> implicitly disables Metal -
> to confirm that any problem you see is a Metal related rendering glitch.
> 
> I will also forward this email to jdk-...@openjdk.java.net
> 
> -Phil.
> 
> [1] https://jdk.java.net/17/
> [2] https://openjdk.java.net/jeps/382 
> [3] https://bugreport.java.com/bugreport/
> [4] https://mail.openjdk.java.net/mailman/listinfo/2d-dev
> 
> 

[OpenJDK 2D-Dev] RFR: 8263486: Clean up MTLSurfaceDataBase.h

[Ajit Ghaisas]

This PR addresses some cleanup activities :
- Cleaned up MTLSurfaceDataBase.h & MTLSurfaceData.m
- Removed OpenGL references from MTLPipelineStatesStorage.m  & MTLRenderQueue.m

-

Commit messages:
 - cleanup unused code

Changes: https://git.openjdk.java.net/jdk/pull/4010/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=4010&range=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8263486
  Stats: 68 lines in 5 files changed: 0 ins; 41 del; 27 mod
  Patch: https://git.openjdk.java.net/jdk/pull/4010.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/4010/head:pull/4010

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

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

[Ajit Ghaisas]

On Tue, 11 May 2021 16:06:09 GMT, Alexey Ushakov  wrote:

>> src/java.desktop/macosx/native/libawt_lwawt/java2d/metal/MTLClip.h line 52:
>> 
>>> 50: @property (readonly) id stencilTextureRef;
>>> 51: @property (readonly) BOOL stencilMaskGenerationInProgress;
>>> 52: @property (readwrite ) BOOL stencilMaskGenerationStarted;
>> 
>> Is 'stencilMaskGenerationStarted' property needed?
>> I don't see it being used anywhere to compare.
>
> It is used here 
> src/java.desktop/macosx/native/libawt_lwawt/java2d/metal/EncoderManager.m 
> (430)
> 
> The main goal of the property is to continue clip generation. Sometimes we're 
> unable to produce all the scanlines at once

Ah.. I wonder- how did I miss it? Thanks for pointing out.

-

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

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

[Ajit Ghaisas]

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

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

I tested on macBook pro 16" with discrete GPU. No regressions were observed in 
J2DDemo, SwingSet2 and Netbeans IDE with this fix. I also tested switching 
between discrete and integrated GPUs.

-

Marked as reviewed by aghaisas (Committer).

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

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

[Ajit Ghaisas]

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

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

src/java.desktop/macosx/native/libawt_lwawt/java2d/metal/MTLClip.h line 52:

> 50: @property (readonly) id stencilTextureRef;
> 51: @property (readonly) BOOL stencilMaskGenerationInProgress;
> 52: @property (readwrite ) BOOL stencilMaskGenerationStarted;

Is 'stencilMaskGenerationStarted' property needed?
I don't see it being used anywhere to compare.

-

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

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

[Ajit Ghaisas]

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.

Marked as reviewed by aghaisas (Committer).

-

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

[OpenJDK 2D-Dev] Integrated: 8263363: Minor cleanup of Lanai code - unused code removal and comments correction

[Ajit Ghaisas]

On Tue, 6 Apr 2021 14:12:55 GMT, Ajit Ghaisas  wrote:

> Refer JBS for 3 issues that this PR addresses.
> In addition, I have corrected an erroneous free() call in the same method I 
> was cleaning up.

This pull request has now been integrated.

Changeset: 8fa50ebd
Author:Ajit Ghaisas 
URL:   
https://git.openjdk.java.net/jdk/commit/8fa50ebdf2c0e44316d8e4b64b3e8ef90a2cc1bb
Stats: 193 lines in 5 files changed: 20 ins; 145 del; 28 mod

8263363: Minor cleanup of Lanai code - unused code removal and comments 
correction

Reviewed-by: serb

-

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

Re: [OpenJDK 2D-Dev] RFR: 8263363: Minor cleanup of Lanai code - unused code removal and comments correction [v5]

[Ajit Ghaisas]

> Refer JBS for 3 issues that this PR addresses.
> In addition, I have corrected an erroneous free() call in the same method I 
> was cleaning up.

Ajit Ghaisas has updated the pull request incrementally with one additional 
commit since the last revision:

  cleanup unused import

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/3357/files
  - new: https://git.openjdk.java.net/jdk/pull/3357/files/746d791d..1cf7d1c2

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

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

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

Re: [OpenJDK 2D-Dev] RFR: 8263363: Minor cleanup of Lanai code - unused code removal and comments correction [v4]

[Ajit Ghaisas]

> Refer JBS for 3 issues that this PR addresses.
> In addition, I have corrected an erroneous free() call in the same method I 
> was cleaning up.

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 five additional commits since 
the last revision:

 - cleanup MTLContext.invalidateCurrentContext()
 - Merge branch 'master' into cleanup_fix
 - log message correction
 - Review fixes
 - 8263363 - cleanup

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/3357/files
  - new: https://git.openjdk.java.net/jdk/pull/3357/files/45e7aec0..746d791d

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

  Stats: 130385 lines in 2601 files changed: 69070 ins; 53454 del; 7861 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3357.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3357/head:pull/3357

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

Re: [OpenJDK 2D-Dev] RFR: 8263363: Minor cleanup of Lanai code - unused code removal and comments correction [v3]

[Ajit Ghaisas]

On Fri, 9 Apr 2021 07:21:57 GMT, Sergey Bylokhov  wrote:

>> This is the only place where we use MTLContext.invalidateCurrentContext() - 
>> which when processed in MTLRenderQueue - clears some native stuff and nulls 
>> out both mtlc and dstOps pointers maintained in MTLRenderQueue.m. I think, 
>> this will be important once we get rid of SET_SCRATCH_SURFACE under 
>> JDK-8263309.
>
> But why you need to invalidate context here? Why do you need "clears some 
> native stuff and nulls out both mtlc and dstOps pointers maintained in 
> MTLRenderQueue.m"?
> 
> In OGL the getCGLConfigInfo() change the state of the OGL state due to 
> "makeCurrentContext", this is why we need to update the lava level state to 
> "invalid", otherwise we will get a mismatch between the state in the native 
> and java state.

I see that MTLContext.invalidateCurrentContext() is not needed here. Also, as 
this is the only place it is used, we can remove the method altogether.

-

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

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

[Ajit Ghaisas]

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).

This pull request has now been integrated.

Changeset: cb8394a8
Author:Ajit Ghaisas 
URL:   https://git.openjdk.java.net/jdk/commit/cb8394a8
Stats: 7 lines in 1 file changed: 3 ins; 0 del; 4 mod

8265304: Temporarily make Metal the default 2D rendering pipeline for macOS

Reviewed-by: jdv, kcr, azvegint, prr

-

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

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

[Ajit Ghaisas]

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).

-

Commit messages:
 - 8265304 - make metal pipeline default

Changes: https://git.openjdk.java.net/jdk/pull/3534/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=3534&range=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8265304
  Stats: 7 lines in 1 file changed: 3 ins; 0 del; 4 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3534.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3534/head:pull/3534

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

Re: [OpenJDK 2D-Dev] RFR: 8263363: Minor cleanup of Lanai code - unused code removal and comments correction [v3]

[Ajit Ghaisas]

> Refer JBS for 3 issues that this PR addresses.
> In addition, I have corrected an erroneous free() call in the same method I 
> was cleaning up.

Ajit Ghaisas has updated the pull request incrementally with one additional 
commit since the last revision:

  log message correction

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/3357/files
  - new: https://git.openjdk.java.net/jdk/pull/3357/files/36ed087b..45e7aec0

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

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

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

Re: [OpenJDK 2D-Dev] RFR: 8263363: Minor cleanup of Lanai code - unused code removal and comments correction [v2]

[Ajit Ghaisas]

> Refer JBS for 3 issues that this PR addresses.
> In addition, I have corrected an erroneous free() call in the same method I 
> was cleaning up.

Ajit Ghaisas has updated the pull request incrementally with one additional 
commit since the last revision:

  Review fixes

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/3357/files
  - new: https://git.openjdk.java.net/jdk/pull/3357/files/ade2381d..36ed087b

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

  Stats: 128 lines in 1 file changed: 20 ins; 81 del; 27 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3357.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3357/head:pull/3357

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

Re: [OpenJDK 2D-Dev] RFR: 8263363: Minor cleanup of Lanai code - unused code removal and comments correction [v2]

[Ajit Ghaisas]

On Wed, 7 Apr 2021 02:50:03 GMT, Sergey Bylokhov  wrote:

>> Ajit Ghaisas has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Review fixes
>
> src/java.desktop/macosx/classes/sun/java2d/metal/MTLGraphicsConfig.java line 
> 149:
> 
>> 147: try {
>> 148: // getMTLConfigInfo() creates new MTLContext, so we should 
>> first
>> 149: // invalidate the current Java-level context and flush the 
>> queue...
> 
> The old discussion was related not only to the comment but to the 
> invalidateCurrentContext, do we need to do it?

This is the only place where we use MTLContext.invalidateCurrentContext() - 
which when processed in MTLRenderQueue - clears some native stuff and nulls out 
both mtlc and dstOps pointers maintained in MTLRenderQueue.m. I think, this 
will be important once we get rid of SET_SCRATCH_SURFACE under JDK-8263309.

> src/java.desktop/macosx/native/libawt_lwawt/java2d/metal/MTLGraphicsConfig.m 
> line 152:
> 
>> 150: NSAutoreleasePool* pool = [[NSAutoreleasePool alloc] init];
>> 151: 
>> 152: 
> 
> Please also check how this function is called, looks like previously it was 
> called as a selector+an array as a parameter, and then reworked as a 
> performOnMainThreadWaiting+block, but it still use an array as a parameter. I 
> think it is similar to JDK-8238075.

Excellent point! Thanks for the pointer to the bug. 
A lot of code in this file can be cleaned up. I will update the PR soon.

-

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

[OpenJDK 2D-Dev] RFR: 8263363: Minor cleanup of Lanai code - unused code removal and comments correction

[Ajit Ghaisas]

Refer JBS for 3 issues that this PR addresses.
In addition, I have corrected an erroneous free() call in the same method I was 
cleaning up.

-

Commit messages:
 - 8263363 - cleanup

Changes: https://git.openjdk.java.net/jdk/pull/3357/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=3357&range=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8263363
  Stats: 21 lines in 3 files changed: 1 ins; 16 del; 4 mod
  Patch: https://git.openjdk.java.net/jdk/pull/3357.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/3357/head:pull/3357

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

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

[Ajit Ghaisas]

On Thu, 1 Apr 2021 05:49:51 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.
>
> Jayathirth D V has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Comment update

Marked as reviewed by aghaisas (Committer).

-

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

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

[Ajit Ghaisas]

On Wed, 31 Mar 2021 15:03:54 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.
>
> Jayathirth D V has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Add comment on usage of MTLRenderCommandEncoder

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

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

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

-

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

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

[Ajit Ghaisas]

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.

I tested this fix. No regressions were observed.

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

-

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

[OpenJDK 2D-Dev] Integrated: 8263488: Verify CWarningWindow works with metal rendering pipeline

[Ajit Ghaisas]

On Fri, 19 Mar 2021 10:17:57 GMT, Ajit Ghaisas  wrote:

> Root cause : 
> CWarningWindow creates a MTLLayer with null peer.
> In MTLLayer.replaceSurfaceData() method, insets should be set only if peer is 
> not null.
> 
> Fix : 
> Added the required null check.

This pull request has now been integrated.

Changeset: 42104e55
Author:Ajit Ghaisas 
URL:   https://git.openjdk.java.net/jdk/commit/42104e55
Stats: 4 lines in 1 file changed: 2 ins; 0 del; 2 mod

8263488: Verify CWarningWindow works with metal rendering pipeline

Reviewed-by: serb, pbansal, avu, kizune

-

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

[OpenJDK 2D-Dev] RFR: 8263488: Verify CWarningWindow works with metal rendering pipeline

[Ajit Ghaisas]

Root cause : 
CWarningWindow creates a MTLLayer with null peer.
In MTLLayer.replaceSurfaceData() method, insets should be set only if peer is 
not null.

Fix : 
Added the required null check.

-

Commit messages:
 - set native insets only if peer is not null

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

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

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

[Ajit Ghaisas]

On Thu, 4 Feb 2021 10:35:02 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

This pull request has now been integrated.

Changeset: 8afec70c
Author:Ajit Ghaisas 
URL:   https://git.openjdk.java.net/jdk/commit/8afec70c
Stats: 17612 lines in 87 files changed: 17573 ins; 2 del; 37 mod

8260931: Implement JEP 382: New macOS Rendering Pipeline

Co-authored-by: Jayathirth D V 
Co-authored-by: Alexey Ushakov 
Co-authored-by: Artem Bochkarev 
Co-authored-by: Prasanta Sadhukhan 
Co-authored-by: Denis Konoplev 
Co-authored-by: Phil Race 
Co-authored-by: Kevin Rushforth 
Co-authored-by: Magnus Ihse Bursie 
Co-authored-by: Ajit Ghaisas 
Reviewed-by: ihse, avu, kcr, gziemski, prr, kizune, jdv, psadhukhan, serb

-

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

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

[Ajit Ghaisas]

On Fri, 12 Mar 2021 10:11:27 GMT, Prasanta Sadhukhan  
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/f5d78efb...369c3d21
>
> src/java.desktop/macosx/classes/sun/awt/CGraphicsConfig.java line 82:
> 
>> 80:  * Creates a new SurfaceData that will be associated with the given
>> 81:  * CGLLayer.
>> 82:  */
> 
> I guess we need to specify CGLLayer/MTLLayer as now we are implementing this 
> method in both pipeline.

I have updated JDK-8263363 to include this.

-

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

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

[Ajit Ghaisas]

On Fri, 12 Mar 2021 01:01:09 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/774f3477...369c3d21
>
> src/java.desktop/macosx/classes/sun/java2d/metal/MTLGraphicsConfig.java line 
> 148:
> 
>> 146: rq.lock();
>> 147: try {
>> 148: // getMTLConfigInfo() creates and destroys temporary
> 
> Does it really create and destroy temporary surfaces/contexts?

No, it doesn't. I have updated JDK-8263363 to include this correction.

-

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

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

[Ajit Ghaisas]

On Fri, 12 Mar 2021 01:04:22 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/23f9642d...369c3d21
>
> src/java.desktop/macosx/native/libawt_lwawt/java2d/metal/MTLPaints.m line 813:
> 
>> 811: initTemplatePipelineDescriptors();
>> 812: // This block is not reached in current implementation.
>> 813: // Texture paint XOR mode rendering uses a tile based rendering 
>> using a SW pipe (similar to OGL)
> 
> Do we have a bugid to implement this later?

This is not exactly a TODO which we intend to implement later. Hence, there is 
no bugid.
Metal does not support XOR mode rendering. I have kept this code as to know the 
code path to take in case we need to implement it in future.

> src/java.desktop/macosx/native/libawt_lwawt/java2d/metal/MTLSurfaceDataBase.h 
> line 59:
> 
>> 57:  * The offset in pixels of the Metal viewport origin from the lower-left
>> 58:  * corner of the heavyweight drawable.  For example, a top-level frame on
>> 59:  * Windows XP has lower-left insets of (4,4).  The Metal viewport origin
> 
> Do we really use these fields? "Windows XP"??

This will be fixed as a follow-on issue - JDK-8263486.

-

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

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

[Ajit Ghaisas]

On Thu, 11 Mar 2021 07:40:47 GMT, Alexey Ushakov  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 45 additional 
>> commits since the last revision:
>> 
>>  - 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
>>  - Merge branch 'master' into 8260931_lanai_JEP_branch
>>  - Lanai PR#206 - 8262729 - aghaisas
>>  - ... and 35 more: 
>> https://git.openjdk.java.net/jdk/compare/d1e5ff8f...3980ecbd
>
> Marked as reviewed by avu (no project role).

We have addressed the review comments (except for a few minor follow-on issues)
This version of the PR is the one which we are planning to integrate into the 
JDK. Please review and approve.

-

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

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

[Ajit Ghaisas]

> **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/1c8fee73...369c3d21

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2403/files
  - new: https://git.openjdk.java.net/jdk/pull/2403/files/3980ecbd..369c3d21

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2403&range=12
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2403&range=11-12

  Stats: 2420 lines in 128 files changed: 1246 ins; 368 del; 806 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2403.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2403/head:pull/2403

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

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

[Ajit Ghaisas]

On Mon, 8 Feb 2021 18:47:09 GMT, Sergey Bylokhov  wrote:

>> src/java.desktop/macosx/native/libawt_lwawt/java2d/metal/MTLGraphicsConfig.m 
>> line 82:
>> 
>>> 80: (JNIEnv *env, jclass mtlgc)
>>> 81: {
>>> 82: FILE *f = popen("/usr/sbin/system_profiler SPDisplaysDataType", 
>>> "r");
>> 
>> How robust is this? It seems like the contents of this could be an 
>> implementation detail and subject to change. Is it documented by Apple?
>
> I suggest fixing this before the integration.

Fixed with JDK-8259825.

-

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

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

[Ajit Ghaisas]

On Tue, 9 Mar 2021 20:18:08 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 41 additional 
>> commits since the last revision:
>> 
>>  - Lanai PR#210 - 8263159 - jdv
>>  - Lanai PR#209 - 8262936 - jdv
>>  - Lanai PR#208 - 8262928 - jdv
>>  - Lanai PR#207 - 8262750 - jdv
>>  - Merge branch 'master' into 8260931_lanai_JEP_branch
>>  - Lanai PR#206 - 8262729 - aghaisas
>>  - Lanai PR#205 - 8262496 - avu
>>  - Lanai PR#203 - 8262313 - jdv
>>  - Lanai PR#202 - 8262293 - avu
>>  - Lanai PR#201 - 8261891 - avu
>>  - ... and 31 more: 
>> https://git.openjdk.java.net/jdk/compare/d65116dd...de456939
>
> src/java.desktop/macosx/native/libawt_lwawt/java2d/metal/MTLGraphicsConfig.m 
> line 185:
> 
>> 183: 
>> 184: NSRect contentRect = NSMakeRect(0, 0, 64, 64);
>> 185: NSWindow *window =
> 
> Some remnant from the scratch surface initialization?

This will be removed under - JDK-8263363

-

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

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

[Ajit Ghaisas]

On Sun, 7 Mar 2021 22:40:53 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 36 additional 
>> commits since the last revision:
>> 
>>  - Lanai PR#206 - 8262729 - aghaisas
>>  - Lanai PR#205 - 8262496 - avu
>>  - Lanai PR#203 - 8262313 - jdv
>>  - Lanai PR#202 - 8262293 - avu
>>  - Lanai PR#201 - 8261891 - avu
>>  - Lanai PR#200 - 8262115 - aghaisas
>>  - Merge branch 'master' into 8260931_lanai_JEP_branch
>>  - Lanai PR#199 - 8262091 - aghaisas
>>  - Lanai PR#198 - 8261646 - avu
>>  - Lanai PR#197 - 8261960 - jdv
>>  - ... and 26 more: 
>> https://git.openjdk.java.net/jdk/compare/33534417...5cb1fd91
>
> src/java.desktop/macosx/classes/sun/lwawt/macosx/CWarningWindow.java line 304:
> 
>> 302: };
>> 303: }
>> 304: public MTLLayer createMTLLayer() {
> 
> TODO to test that this functionality works

Testing was done under JDK-8261141.

-

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

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

[Ajit Ghaisas]

> **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 45 additional commits since the 
last revision:

 - 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
 - Merge branch 'master' into 8260931_lanai_JEP_branch
 - Lanai PR#206 - 8262729 - aghaisas
 - ... and 35 more: https://git.openjdk.java.net/jdk/compare/dfe037de...3980ecbd

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2403/files
  - new: https://git.openjdk.java.net/jdk/pull/2403/files/de456939..3980ecbd

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2403&range=11
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2403&range=10-11

  Stats: 4881 lines in 177 files changed: 1767 ins; 2327 del; 787 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2403.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2403/head:pull/2403

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

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

[Ajit Ghaisas]

> **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 41 additional commits since the 
last revision:

 - Lanai PR#210 - 8263159 - jdv
 - Lanai PR#209 - 8262936 - jdv
 - Lanai PR#208 - 8262928 - jdv
 - Lanai PR#207 - 8262750 - jdv
 - Merge branch 'master' into 8260931_lanai_JEP_branch
 - Lanai PR#206 - 8262729 - aghaisas
 - Lanai PR#205 - 8262496 - avu
 - Lanai PR#203 - 8262313 - jdv
 - Lanai PR#202 - 8262293 - avu
 - Lanai PR#201 - 8261891 - avu
 - ... and 31 more: https://git.openjdk.java.net/jdk/compare/582d72a8...de456939

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2403/files
  - new: https://git.openjdk.java.net/jdk/pull/2403/files/5cb1fd91..de456939

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2403&range=10
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2403&range=09-10

  Stats: 8128 lines in 291 files changed: 5147 ins; 1823 del; 1158 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2403.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2403/head:pull/2403

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

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

[Ajit Ghaisas]

> **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 36 additional commits since the 
last revision:

 - Lanai PR#206 - 8262729 - aghaisas
 - Lanai PR#205 - 8262496 - avu
 - Lanai PR#203 - 8262313 - jdv
 - Lanai PR#202 - 8262293 - avu
 - Lanai PR#201 - 8261891 - avu
 - Lanai PR#200 - 8262115 - aghaisas
 - Merge branch 'master' into 8260931_lanai_JEP_branch
 - Lanai PR#199 - 8262091 - aghaisas
 - Lanai PR#198 - 8261646 - avu
 - Lanai PR#197 - 8261960 - jdv
 - ... and 26 more: https://git.openjdk.java.net/jdk/compare/dd205a58...5cb1fd91

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2403/files
  - new: https://git.openjdk.java.net/jdk/pull/2403/files/614be056..5cb1fd91

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2403&range=09
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2403&range=08-09

  Stats: 14064 lines in 640 files changed: 7890 ins; 3094 del; 3080 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2403.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2403/head:pull/2403

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

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

[Ajit Ghaisas]

> **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 29 additional commits since the 
last revision:

 - Lanai PR#199 - 8262091 - aghaisas
 - Lanai PR#198 - 8261646 - avu
 - Lanai PR#197 - 8261960 - jdv
 - Lanai PR#196 - 8260715 - avu
 - Lanai PR#195 - 8261908 - jdv
 - Lanai PR#194 - 8261703 - jdv
 - Lanai PR#193 - 8261734 - avu
 - Lanai PR#192 - 8261789 - aghaisas
 - Merge branch 'master' into 8260931_lanai_JEP_branch
 - Lanai PR#191 - 8261705 - jdv
 - ... and 19 more: https://git.openjdk.java.net/jdk/compare/b17c91cb...614be056

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2403/files
  - new: https://git.openjdk.java.net/jdk/pull/2403/files/7b0b0dc4..614be056

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2403&range=08
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2403&range=07-08

  Stats: 11833 lines in 292 files changed: 8413 ins; 2170 del; 1250 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2403.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2403/head:pull/2403

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

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

[Ajit Ghaisas]

On Mon, 15 Feb 2021 18:30:51 GMT, Gerard Ziemski  wrote:

>> Changes requested by gziemski (Committer).
>
> I took a look at https://bugs.openjdk.java.net/browse/JDK-8261408 and noticed 
> a startup time regression with the Metal rendering pipeline, so I dug a bit 
> and here is what I found using Xcode startup profiler:
> 
> Here is the OpenGL pipeline:
>  src="https://user-images.githubusercontent.com/63425797/107981859-8efb4780-6f88-11eb-849a-385c29f3ff6f.png";>
> 
> 
> Here is the Metal pipeline:
>  src="https://user-images.githubusercontent.com/63425797/107981882-9ae70980-6f88-11eb-9b73-d1bd19dfa07f.png";>
> 
> If I read the results correctly, there is a weird 330ms time gap in the case 
> of the Metal pipeline where nothing is done and it looks like the culprit is 
> `Java_sun_java2d_metal_MTLGraphicsConfig_isMetalFrameworkAvailable`
> 
> The OpenGL pipeline gets blocked only for 83ms in 
> `Java_sun_java2d_opengl_CGLGraphicsConfig_getCGLConfigInfo`
> 
> I would advice that we take a closer look at why 
> 'Java_sun_java2d_metal_MTLGraphicsConfig_isMetalFrameworkAvailable' takes so 
> long, and optimize it, or cache the results, so the next VM instance can skip 
> it if needed (assuming the call doesn't end up initializing the native Metal 
> or something like that), still worth taking a look.
> 
> I would also recommend that you adopt Xcode and its Instruments profiler 
> tools for future work. Please let me know if you need help in this area.
> 
> I only scratched the surface here and I think that there is plenty of 
> profiling that can be done to investigate the startup time regression further.

Thanks @gerard-ziemski for taking a detailed look at this.
We do have an open bug to address this. Please refer 
[JDK-8259825](https://bugs.openjdk.java.net/browse/JDK-8259825).

-

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

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

[Ajit Ghaisas]

> **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 20 additional commits since the 
last revision:

 - Lanai PR#191 - 8261705 - jdv
 - Lanai PR#190 - 8261706 - jdv
 - Lanai PR#189 - 8261712 - avu
 - Lanai PR#187 - 8261704 - jdv
 - Lanai PR#186 - 8261638 - avu
 - Lanai PR#185 - 8261632 - jdv
 - Lanai PR#184 - 8261620 - aghaisas
 - Lanai PR#182 - 8261547 - psadhukhan
 - Merge branch 'master' into 8260931_lanai_JEP_branch
 - Lanai PR#181 - 8261143 - aghaisas
 - ... and 10 more: https://git.openjdk.java.net/jdk/compare/89c20e04...7b0b0dc4

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2403/files
  - new: https://git.openjdk.java.net/jdk/pull/2403/files/7340d067..7b0b0dc4

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2403&range=07
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2403&range=06-07

  Stats: 6664 lines in 263 files changed: 3414 ins; 1659 del; 1591 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2403.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2403/head:pull/2403

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

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

[Ajit Ghaisas]

On Thu, 11 Feb 2021 21:04:16 GMT, Gerard Ziemski  wrote:

>> I guess you will only see this if `Metal API Validation Enabled`
>
> Which actually begs a question of whether we tested with `Metal API 
> Validation Enabled` ?

Thanks @gerard-ziemski for bringing out this important observation. It clearly 
indicates that there are some overflows which are undetected.

We have tested without Metal API validation enabled till now.
 
Now, I have fixed the JDK-8261620 bug that reported the crash on app launch 
with Metal API validation enabled.
More testing with “Metal API validation enabled” to follow..

-

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

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

[Ajit Ghaisas]

> **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 incrementally with two additional 
commits since the last revision:

 - Lanai PR#181 - 8261143 - aghaisas
 - Lanai PR#180 - 8261546 - jdv

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2403/files
  - new: https://git.openjdk.java.net/jdk/pull/2403/files/6a3f96ef..7340d067

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2403&range=06
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2403&range=05-06

  Stats: 82 lines in 2 files changed: 0 ins; 54 del; 28 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2403.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2403/head:pull/2403

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

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

[Ajit Ghaisas]

On Wed, 10 Feb 2021 22:31:12 GMT, Gerard Ziemski  wrote:

>> Marked as reviewed by gziemski (Committer).
>
> 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!

-

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

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

[Ajit Ghaisas]

> **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 incrementally with two additional 
commits since the last revision:

 - Lanai PR#179 - 8261402 - avu
 - Lanai PR#178 - 8261273 - avu

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2403/files
  - new: https://git.openjdk.java.net/jdk/pull/2403/files/9a72538a..6a3f96ef

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2403&range=05
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2403&range=04-05

  Stats: 73 lines in 6 files changed: 9 ins; 39 del; 25 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2403.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2403/head:pull/2403

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

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

[Ajit Ghaisas]

> **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 seven additional commits since 
the last revision:

 - Merge branch 'master' into 8260931_lanai_JEP_branch
   Merge
 - Lanai PR#177 - 8261430 - aghaisas
 - Lanai PR#176 - 8261399 - jdv
 - Lanai PR#175 - 8261304 - aghaisas
 - Lanai PR#174 - 8261234 - kcr
 - Merge branch 'master' into 8260931_lanai_JEP_branch
   Merge from openjdk/jdk
 - Project Lanai Patch

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2403/files
  - new: https://git.openjdk.java.net/jdk/pull/2403/files/64173289..9a72538a

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

  Stats: 6660 lines in 320 files changed: 3899 ins; 1099 del; 1662 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2403.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2403/head:pull/2403

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

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

[Ajit Ghaisas]

On Mon, 8 Feb 2021 16:53:16 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/awt/CGraphicsDevice.java line 113:
> 
>> 111: // This indicates fallback to other rendering pipeline also 
>> failed.
>> 112: // Should never reach here
>> 113: throw new InternalError("Error - unable to initialize any 
>> rendering pipeline.");
> 
> There is no software based renderer to fall back here?

OpenGL was the sole rendering pipeline on macOS. Now we are adding Metal 
rendering pipeline. There is no software based fall back renderer.

> src/java.desktop/macosx/classes/sun/java2d/MacOSFlags.java line 66:
> 
>> 64:propString.equals("f") ||
>> 65:propString.equals("False") ||
>> 66:propString.equals("F"))
> 
> Shouldn't `1` and `0` be also allowed here?

No.
0 and 1 are not supported for existing VM options for rendering pipelines. We 
won't be adding it for metal pipeline either.

> src/java.desktop/macosx/classes/sun/java2d/MacOSFlags.java line 100:
> 
>> 98: oglState = PropertyState.ENABLED; // Enable 
>> default pipeline
>> 99: metalState = PropertyState.DISABLED; // Disable 
>> non-default pipeline
>> 100: }
> 
> This matches JEP 382 specification, but even when both GL is `false` and 
> Metal is `false` we still get GL? There is no software based pipeline anymore?

OpenGL was the sole rendering pipeline on macOS. Now we are adding Metal 
rendering pipeline. There is no software based fall back renderer.

If user requests both - GL and Metal - as false - we shall use 'the default' 
pipeline.
Current default render pipeline on macOS is OpenGL.
Changing default render pipeline on macOS is not in the scope of this JEP.

> src/java.desktop/macosx/classes/sun/java2d/metal/MTLSurfaceData.java line 61:
> 
>> 59: 
>> 60: public abstract class MTLSurfaceData extends SurfaceData
>> 61: implements AccelSurface {
> 
> `MTLSurfaceData` and `OGLSurfaceData` seem to share lots of the same code, 
> isn't there a way to refactor the common code out?
> 
> There are other files that structually look identical, except for the names 
> of classes they use, ex `MTLMaskBlit.java` and `OGLMaskBlit.java`.

Although it is a good suggestion, I do not recommend this refactoring - 
- We do not wish to destabilize proven OpenGL pipeline by refactoring - 
additional testing on OpenGL pipeline also would be needed.
- OpenGL pipeline on macOS might be removed once Apple removes OpenGL support 
from macOS - this will render this refactoring pointless.

-

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

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

[Ajit Ghaisas]

> **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)

Ajit Ghaisas has updated the pull request incrementally with two additional 
commits since the last revision:

 - Lanai PR#177 - 8261430 - aghaisas
 - Lanai PR#176 - 8261399 - jdv

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2403/files
  - new: https://git.openjdk.java.net/jdk/pull/2403/files/6044adc0..64173289

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

  Stats: 49 lines in 14 files changed: 0 ins; 43 del; 6 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2403.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2403/head:pull/2403

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

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

[Ajit Ghaisas]

On Mon, 8 Feb 2021 14:22:27 GMT, Kevin Rushforth  wrote:

>> I think, a generic name is OK as the path of shader file already has both 
>> awt (libawt_lwawt) and java2d in it.
>
> In the source tree, yes, but not in the jdk image where it ends up in 
> `$JAVA_HOME/lib/shaders.metallib`. I don't have a problem with this, as long 
> as it is a deliberate decision.

OK. I get your point. Keeping the name unchanged for now as there won't be 
another .metallib in JDK. The name can be changed in the future if need arises.

-

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

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

[Ajit Ghaisas]

On Fri, 5 Feb 2021 18:42:02 GMT, Kevin Rushforth  wrote:

>> Ajit Ghaisas has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Lanai PR#175 - 8261304 - aghaisas
>
> make/modules/java.desktop/lib/Awt2dLibraries.gmk line 894:
> 
>> 892:   SHADERS_SUPPORT_DIR := 
>> $(SUPPORT_OUTPUTDIR)/native/java.desktop/libosxui
>> 893:   SHADERS_AIR := $(SHADERS_SUPPORT_DIR)/shaders.air
>> 894:   SHADERS_LIB := $(INSTALL_LIBRARIES_HERE)/shaders.metallib
> 
> Q: Should 2d (or awt) be in the name of this file, or is a generic name OK?

I think, a generic name is OK as the path of shader file already has both awt 
(libawt_lwawt) and java2d in it.

-

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

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

[Ajit Ghaisas]

> **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)

Ajit Ghaisas has updated the pull request incrementally with one additional 
commit since the last revision:

  Lanai PR#175 - 8261304 - aghaisas

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2403/files
  - new: https://git.openjdk.java.net/jdk/pull/2403/files/8ed7b5f5..6044adc0

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

  Stats: 32 lines in 10 files changed: 0 ins; 12 del; 20 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2403.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2403/head:pull/2403

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

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

[Ajit Ghaisas]

> **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)

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 three additional commits since 
the last revision:

 - Lanai PR#174 - 8261234 - kcr
 - Merge branch 'master' into 8260931_lanai_JEP_branch
   Merge from openjdk/jdk
 - Project Lanai Patch

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2403/files
  - new: https://git.openjdk.java.net/jdk/pull/2403/files/0b8d96b2..8ed7b5f5

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

  Stats: 12194 lines in 245 files changed: 7103 ins; 4454 del; 637 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2403.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2403/head:pull/2403

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

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

[Ajit Ghaisas]

This is the implementation of JEP 382 : New macOS Rendering Pipeline (Metal 
rendering pipeline)

JEP Link - https://bugs.openjdk.java.net/browse/JDK-8238361

-

Commit messages:
 - Project Lanai Patch

Changes: https://git.openjdk.java.net/jdk/pull/2403/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=2403&range=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8260931
  Stats: 17445 lines in 83 files changed: 17395 ins; 1 del; 49 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2403.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2403/head:pull/2403

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

Re: [OpenJDK 2D-Dev] RFR: 8171303 sun/java2d/pipe/InterpolationQualityTest.java fails on Windows & Linux

[Ajit Ghaisas]

> On 02-Sep-2020, at 10:46 AM, Sergey Bylokhov  
> wrote:
> 
> On 01.09.2020 03:42, Ajit Ghaisas wrote:
>>> 
>>> In the HiDPI mode the VolatileImage internally have twice more pixels than 
>>> BufferedImage, so when we draw the data to
>>> the VolatileImage and then scale it back to the size of the BufferedImage 
>>> for comparison, the test fails.
>> This looks good. I checked with OGL and Metal (project Lanai) on macOS.
>> One question is - do we need to support this test in HiDPI mode?
> 
> The difference between HiDPI and non-HiDPI mode is that  VolatileImage has 
> more
> pixels and every draw operation use scaled blits. The test in question scales
> the graphics ourselves. To eliminate the "sun.java2d.uiScale=1" we will need 
> to
> apply the scale to the BufferedImage size, then we should skip the usage of
> "getSnapshot", and then we will need to scale all rendering to the
> BufferedImage(simulating the HiDPI in VolatileImage), but in the end, the same
> scaled blit will be tested.

OK. Thanks for the explanation.
Test fix looks good. +1. (not a ‘R’ reviewer)

Regards,
Ajit

> -- 
> Best regards, Sergey.

Re: [OpenJDK 2D-Dev] RFR: 8171303 sun/java2d/pipe/InterpolationQualityTest.java fails on Windows & Linux

[Ajit Ghaisas]

> On 26-Aug-2020, at 5:05 AM, Sergey Bylokhov  
> wrote:
> 
> On 25.08.2020 14:59, Philip Race wrote:
>> This is fine but
>> 1) I like to see the bug under which you fixed it in the @bug list.
> 
> It is not a product bug and the test isn't reworked, so it is not necessary 
> to have this test fix in the list of bugs
> 
>> 2) I am not sure I see how all the various reasons for this test failing can 
>> be explained by this.
> 
> In the HiDPI mode the VolatileImage internally have twice more pixels than 
> BufferedImage, so when we draw the data to
> the VolatileImage and then scale it back to the size of the BufferedImage for 
> comparison, the test fails.

This looks good. I checked with OGL and Metal (project Lanai) on macOS.

One question is - do we need to support this test in HiDPI mode?

> 
>> -phil.
>> On 8/25/20, 1:37 PM, Sergey Bylokhov wrote:
>>> Hello.
>>> Please review the fix for jdk/client.
>>> 
>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8171303
>>> Fix: http://cr.openjdk.java.net/~serb/8171303/webrev.00
>>> 
>>> This the only test which was created to check different types of 
>>> interpolation.
>>> It checks the rendering to the VolatileImage and uses BufferedImage as a 
>>> gold image.
>>> But it does not take into account that rendering to the VolatileImage might 
>>> be affected
>>> by the HiDPI support.
>>> 
> 
> 
> -- 
> Best regards, Sergey.

Re: [OpenJDK 2D-Dev] JDK-8220154 Improve java2d rendering performance on macOS by using Metal framework

[Ajit Ghaisas]

Hi,

   I have pushed a minor correction to the metal-prototype-branch of 
jdk-sandbox (http://hg.openjdk.java.net/jdk/sandbox 
<http://hg.openjdk.java.net/jdk/sandbox>)
   
   Now, we can build and launch J2DDemo, J2DBenchmark and SwingSet2 with Metal 
pipeline.
   Of course, rendering performance is slow and still a lot of functionality 
needs to be implemented.

Regards,
Ajit


> On 20-Jun-2019, at 3:50 AM, Alexey Ushakov  
> wrote:
> 
> Hi Ajit,
> 
>> Thanks for suggesting how to run basic perf test.
>> I did run it with and without Metal. The test run with Metal takes ~2X 
>> time to run as compared with OpenGL.
>> What is your observation?
> 
> Yes, in some cases we have similar performance drop at the moment 
> (testFlatBubbles, testFlatOvalRotBubbles, testWiredBubbles, testFlatQuad, 
> testWiredQuad)
> Here is the results from Mac mini 2018, macOS 10.14.4
> 
> metalEnabled=0
> testFlatBubbles 70
> testFlatBoxBubbles 78
> testImgBubbles 73
> testFlatBoxRotBubbles 78
> testFlatOvalRotBubbles 74
> testLinGradOvalRotBubbles 46
> testWiredBubbles 54
> testWiredBoxBubbles 82
> testLines 84
> testFlatQuad 62
> testWiredQuad 58
> 
> metalEnabled=1
> testFlatBubbles 31
> testFlatBoxBubbles 91
> testImgBubbles 88
> testFlatBoxRotBubbles 92
> testFlatOvalRotBubbles 36
> testLinGradOvalRotBubbles 37
> testWiredBubbles 17
> testWiredBoxBubbles -
> testLines 89
> testFlatQuad 21
> testWiredQuad 19
> 
>> Although, the java side code changes are good, I see an issue with 
>> native Metal renderer implementation. 
>> For rendering primitives, I see that the metal commands are being 
>> encoded for each drawXXX call in MTLRenderer.m
>>  
>> Why is MetalContext.createRenderEncoderForDest is being created for each 
>> draw call? I think, it is an overhead.
>> Have you considered managing a VertexBuffer and encoding ’N’ set of draw 
>> calls in a render pass?
> 
> Yes, I thought about such optimizations. They definetely should help with 
> this performance drop.
> 
> Best Regards,
> Alexey  
> 
> 
>> On 11 Jun 2019, at 12:01, Ajit Ghaisas > <mailto:ajit.ghai...@oracle.com>> wrote:
>> 
>> Hi Alexey,
>> 
>> Thanks for suggesting how to run basic perf test.
>> I did run it with and without Metal. The test run with Metal takes ~2X 
>> time to run as compared with OpenGL.
>> What is your observation?
>> 
>> Although, the java side code changes are good, I see an issue with 
>> native Metal renderer implementation. 
>> For rendering primitives, I see that the metal commands are being 
>> encoded for each drawXXX call in MTLRenderer.m
>>  
>> Why is MetalContext.createRenderEncoderForDest is being created for each 
>> draw call? I think, it is an overhead.
>> Have you considered managing a VertexBuffer and encoding ’N’ set of draw 
>> calls in a render pass?
>> 
>> Regards,
>> Ajit
>> 
>> 
>>> On 17-May-2019, at 12:20 PM, Jayathirth Rao >> <mailto:jayathirth@oracle.com>> wrote:
>>> 
>>> Hi Alexey,
>>> 
>>> Thanks for the clarifications.
>>> Build failures we are getting are mainly because of tighter compiler 
>>> restrictions, I have attached error log for the same.
>>> Basically we have 2 log errors and one conditional error.
>>> 
>>> Yes in the latest build there is refactoring of LWComponentPeer.java but I 
>>> have taken care of it and build error was not because of that.
>>> 
>>> And thanks for sharing test and Graphics2D supported method details.
>>> 
>>> Regards,
>>> Jay
>>> 
>>> 
>>> 
>>>> On 16-May-2019, at 10:39 PM, Alexey Ushakov >>> <mailto:alexey.usha...@jetbrains.com>> wrote:
>>>> 
>>>> Hi Jay,
>>>> 
>>>>>> I can see that now you have added the MTLTexturePool.m file. It only 
>>>>>> partially solves the build failures.
>>>>>> Jay & myself tried building it separately and we do still see the build 
>>>>>> failures.
>>>> 
>>>> It’s strange. There wasn’t any copy/paste errors. The only reason that I 
>>>> can imagine that some chunks of the patch were rejected (as I actually 
>>>> experienced recently because of some refactoring of LWComponentPeer.java 
>>>> happened couple of days ago).
>>>> Anyway, I regenerated webrev once more 
>>>> (http://cr.openjdk.java.net/~avu/JDK-8220154/webre

Re: [OpenJDK 2D-Dev] JDK-8220154 Improve java2d rendering performance on macOS by using Metal framework

[Ajit Ghaisas]

Hi Alexey,

Thanks for suggesting how to run basic perf test.
I did run it with and without Metal. The test run with Metal takes ~2X time 
to run as compared with OpenGL.
What is your observation?

Although, the java side code changes are good, I see an issue with native 
Metal renderer implementation. 
For rendering primitives, I see that the metal commands are being encoded 
for each drawXXX call in MTLRenderer.m
 
Why is MetalContext.createRenderEncoderForDest is being created for each 
draw call? I think, it is an overhead.
Have you considered managing a VertexBuffer and encoding ’N’ set of draw 
calls in a render pass?

Regards,
Ajit


> On 17-May-2019, at 12:20 PM, Jayathirth Rao  wrote:
> 
> Hi Alexey,
> 
> Thanks for the clarifications.
> Build failures we are getting are mainly because of tighter compiler 
> restrictions, I have attached error log for the same.
> Basically we have 2 log errors and one conditional error.
> 
> Yes in the latest build there is refactoring of LWComponentPeer.java but I 
> have taken care of it and build error was not because of that.
> 
> And thanks for sharing test and Graphics2D supported method details.
> 
> Regards,
> Jay
> 
> 
> 
>> On 16-May-2019, at 10:39 PM, Alexey Ushakov > <mailto:alexey.usha...@jetbrains.com>> wrote:
>> 
>> Hi Jay,
>> 
>>>> I can see that now you have added the MTLTexturePool.m file. It only 
>>>> partially solves the build failures.
>>>> Jay & myself tried building it separately and we do still see the build 
>>>> failures.
>> 
>> It’s strange. There wasn’t any copy/paste errors. The only reason that I can 
>> imagine that some chunks of the patch were rejected (as I actually 
>> experienced recently because of some refactoring of LWComponentPeer.java 
>> happened couple of days ago).
>> Anyway, I regenerated webrev once more 
>> (http://cr.openjdk.java.net/~avu/JDK-8220154/webrev.01 
>> <http://cr.openjdk.java.net/~avu/JDK-8220154/webrev.01>) and verified it 
>> with the openjdk workspace hg.openjdk.java.net/jdk/client 
>> <http://hg.openjdk.java.net/jdk/client> . 
>> 
>> 
>>> So with current JB changes what parts of primitive and image drawing are 
>>> expected to work?
>>> Please provide your inputs.
>> 
>> I’ve included junit4 performance test that you can try to see what is 
>> currently supported.
>> Here is some steps to run the test provided with the webrev
>> 
>> #cd openjdk_ws 
>> #mkdir -p test/jdk/jbu/testdata/perf/metal/
>> #cp webrev/raw_files/new/test/jdk/jbu/testdata/perf/metal/duke.png 
>> test/jdk/jbu/testdata/perf/metal/
>> 
>> #./build/macosx-x86_64-server-release/images/jdk-bundle/jdk-13.jdk/Contents/Home/bin/javac
>>   -cp .:/Users/avu/tools/junit-4.10.jar -d . 
>> test/jdk/jbu/perf/metal/MetalPerfTest.java
>> 
>> #./build/macosx-x86_64-server-release/images/jdk-bundle/jdk-13.jdk/Contents/Home/bin/java
>>   -cp .:/Users/avu/tools/junit-4.10.jar 
>> -Dtestdata=/Users/avu/ws/openjdk/test/jdk/jbu/testdata 
>> -Djb.java2d.metal=true org.junit.runner.JUnitCore  perf.metal.MetalPerfTest
>> 
>> Here is the list of supported methods from Graphics2D: 
>> setColor
>> fillOval
>> translate
>> setTransform
>> rotate
>> setPaint(new LinearGradientPaint())
>> drawImage
>> fillRect
>> draw/fill (Shape)
>> 
>> Best Regards,
>> Alexey 
>> 
>>> On 16 May 2019, at 15:08, Jayathirth Rao >> <mailto:jayathirth@oracle.com>> wrote:
>>> 
>>> Hi Alexey,
>>> 
>>> We are trying to test basic calls of Graphics2D as mentioned in 
>>> https://docs.oracle.com/javase/tutorial/2d/TOC.html 
>>> <https://docs.oracle.com/javase/tutorial/2d/TOC.html> 
>>> 
>>> I see that Graphics2D.drawImage() with BufferedImage as input works. Also I 
>>> tried other operations like fillRect() and fillOval() and they work. I am 
>>> testing with simple JFrame and adding draw calls inside overriden paint() 
>>> method of a panel.
>>> 
>>> So with current JB changes what parts of primitive and image drawing are 
>>> expected to work?
>>> Please provide your inputs.
>>> 
>>> Thanks,
>>> Jay
>>> 
>>>> On 16-May-2019, at 12:47 PM, Ajit Ghaisas >>> <mailto:ajit.ghai...@oracle.com>> wrote:
>>>> 
>>>> Thanks Alexey.
>>>> 
>>>> I can see that now you have added the MTLTexturePool.m file. It only 
>>>> partially

Re: [OpenJDK 2D-Dev] JDK-8220154 Improve java2d rendering performance on macOS by using Metal framework

[Ajit Ghaisas]

Thanks Alexey.

I can see that now you have added the MTLTexturePool.m file. It only partially 
solves the build failures.
Jay & myself tried building it separately and we do still see the build 
failures.

Some statements need bracketing & some copy-paste errors need correction.
We could fix these errors and have got a build. We will continue our test 
experiments with it.

Meanwhile, you may want to update the patch to fix the build errors. 

Regards,
Ajit


> On 08-May-2019, at 8:56 PM, Alexey Ushakov  
> wrote:
> 
> FYI, I’ve rebased our work on top of the latest state of 
> http://hg.openjdk.java.net/jdk/jdk <http://hg.openjdk.java.net/jdk/jdk> 
> (http://cr.openjdk.java.net/~avu/JDK-8220154/webrev.01 
> <http://cr.openjdk.java.net/~avu/JDK-8220154/webrev.01>)
> 
> Best Regards,
> Alexey
> 
>> On 8 May 2019, at 16:19, Alexey Ushakov > <mailto:alexey.usha...@jetbrains.com>> wrote:
>> 
>> Thanks for catching it, Ajit!
>> 
>> Looks like it was a problem with webrev script applied to multiple git 
>> commits. I’ve updated the webrev.
>> Also, we didn’t rebase yet on the latest state of 
>> http://hg.openjdk.java.net/jdk/jdk <http://hg.openjdk.java.net/jdk/jdk> 
>> (this work is in progress).
>> I’ll let you know when the rebase is ready.
>> 
>> Best Regards,
>> Alexey
>> 
>>> On 7 May 2019, at 21:02, Ajit Ghaisas >> <mailto:ajit.ghai...@oracle.com>> wrote:
>>> 
>>> Hi Alexey,
>>> 
>>>I tried building this patch with latest 
>>> http://hg.openjdk.java.net/jdk/jdk/ <http://hg.openjdk.java.net/jdk/jdk/>
>>>  
>>>1. Some basic copy paste errors are resulting in build failures
>>>2. MTLTexturePool.m file is missing from the patch
>>> 
>>>Can you please check & update?
>>> 
>>> Regards,
>>> Ajit
>>> 
>>>> On 30-Apr-2019, at 2:52 PM, Alexey Ushakov >>> <mailto:alexey.usha...@jetbrains.com>> wrote:
>>>> 
>>>> Hello,
>>>> 
>>>> Here  is an update on our effort to use Metal framework for java2d 
>>>> rendering.
>>>> 
>>>> We’ve added image rendering support and some support for LinearGradient. 
>>>> Also, the code has been refactored.
>>>> 
>>>> Please have a look:
>>>> 
>>>> http://cr.openjdk.java.net/~avu/JDK-8220154/webrev.01 
>>>> <http://cr.openjdk.java.net/~avu/JDK-8220154/webrev.01>
>>>> 
>>>> Best Regards,
>>>> Alexey
>>>> 
>>>> 
>>>>> Hello,
>>>>> 
>>>>> As far as we know Apple has deprecated OpenGL on MacOS platform 
>>>>> (https://developer.apple.com/macos/whats-new/ 
>>>>> <https://developer.apple.com/macos/whats-new/>).
>>>>> 
>>>>> Unfortunately, this decision greatly affects our products that based on 
>>>>> Java Client technologies. So, we (here at JetBrains) decided to start a 
>>>>> project to replace OpenGL rendering on MacOS platform with a new one 
>>>>> based on Metal. This is a huge task, so we  decided to leverage current 
>>>>> rendering architecture that is used in OpenGL rendering pipeline on Mac.  
>>>>> 
>>>>> That’s why we didn’t use MTKView for representing AWT windows (that 
>>>>> probably would be a better approach in the long term). Currently we're 
>>>>> using CAMetalLayer within AWTView. We’ve implemented flat color 
>>>>> shape/curve rendering so far and there is a lot of work to do. So, we’re 
>>>>> looking forward to any collaboration.
>>>>> 
>>>>> In the mean time I’d like to share our current work to discuss metal 
>>>>> pipeline architecture at the early stage of work.
>>>>> 
>>>>> Here is the webrev with our on going development:
>>>>> 
>>>>> http://cr.openjdk.java.net/~avu/JDK-8220154/webrev.00 
>>>>> <http://cr.openjdk.java.net/~avu/JDK-8220154/webrev.00>
>>>>> 
>>>>> Best Regards,
>>>>> Alexey
>>> 
>> 
> 

Re: [OpenJDK 2D-Dev] JDK-8220154 Improve java2d rendering performance on macOS by using Metal framework

[Ajit Ghaisas]

Hi Alexey,

   I tried building this patch with latest http://hg.openjdk.java.net/jdk/jdk/
 
   1. Some basic copy paste errors are resulting in build failures
   2. MTLTexturePool.m file is missing from the patch

   Can you please check & update?

Regards,
Ajit

> On 30-Apr-2019, at 2:52 PM, Alexey Ushakov  
> wrote:
> 
> Hello,
> 
> Here  is an update on our effort to use Metal framework for java2d rendering.
> 
> We’ve added image rendering support and some support for LinearGradient. 
> Also, the code has been refactored.
> 
> Please have a look:
> 
> http://cr.openjdk.java.net/~avu/JDK-8220154/webrev.01 
> 
> 
> Best Regards,
> Alexey
> 
> 
>> Hello,
>> 
>> As far as we know Apple has deprecated OpenGL on MacOS platform 
>> (https://developer.apple.com/macos/whats-new/ 
>> ).
>> 
>> Unfortunately, this decision greatly affects our products that based on Java 
>> Client technologies. So, we (here at JetBrains) decided to start a project 
>> to replace OpenGL rendering on MacOS platform with a new one based on Metal. 
>> This is a huge task, so we  decided to leverage current rendering 
>> architecture that is used in OpenGL rendering pipeline on Mac.  
>> 
>> That’s why we didn’t use MTKView for representing AWT windows (that probably 
>> would be a better approach in the long term). Currently we're using 
>> CAMetalLayer within AWTView. We’ve implemented flat color shape/curve 
>> rendering so far and there is a lot of work to do. So, we’re looking forward 
>> to any collaboration.
>> 
>> In the mean time I’d like to share our current work to discuss metal 
>> pipeline architecture at the early stage of work.
>> 
>> Here is the webrev with our on going development:
>> 
>> http://cr.openjdk.java.net/~avu/JDK-8220154/webrev.00 
>> 
>> 
>> Best Regards,
>> Alexey

Re: [OpenJDK 2D-Dev] [10]JDK-8183341:Better cleanup for javax/imageio/AllowSearch.java

[Ajit Ghaisas]

A nit… 

There should be a space between if and ( -- on lines 65 and 70.

You can make this change before pushing. No need to have a new webrev just for 
this.

 

Regards,

Ajit

 

From: Shashidhara Veerabhadraiah 
Sent: Friday, July 14, 2017 10:01 AM
To: 2d-dev@openjdk.java.net
Subject: Re: [OpenJDK 2D-Dev] [10]JDK-8183341:Better cleanup for 
javax/imageio/AllowSearch.java

 

Hi All, Anything more comments on this? Can I close this now?

 

Thanks and regards,

Shashi

 

From: Jayathirth D V 
Sent: Wednesday, July 12, 2017 3:14 PM
To: Shashidhara Veerabhadraiah mailto:shashidhara.veerabhadra...@oracle.com"shashidhara.veerabhadra...@oracle.com>
Cc: HYPERLINK "mailto:2d-dev@openjdk.java.net"2d-dev@openjdk.java.net
Subject: RE: [OpenJDK 2D-Dev] [10]JDK-8183341:Better cleanup for 
javax/imageio/AllowSearch.java

 

Hello Shashi,

 

Changes are fine.

 

Thanks,

Jay

 

From: Shashidhara Veerabhadraiah 
Sent: Wednesday, July 12, 2017 2:50 PM
To: Jayathirth D V; Sergey Bylokhov; Prahalad Kumar Narayanan
Cc: HYPERLINK "mailto:2d-dev@openjdk.java.net"2d-dev@openjdk.java.net
Subject: RE: [OpenJDK 2D-Dev] [10]JDK-8183341:Better cleanup for 
javax/imageio/AllowSearch.java

 

Thanks for that suggestion Jay. I have modified with some changes so that we 
don’t run into a null pointer exception!! And here is the new Webrev for the 
same:

 

http://cr.openjdk.java.net/~pkbalakr/shashi/8183341/webrev_02/

 

Thanks and regards,

Shashi

 

From: Jayathirth D V 
Sent: Wednesday, July 12, 2017 11:53 AM
To: Shashidhara Veerabhadraiah mailto:shashidhara.veerabhadra...@oracle.com"shashidhara.veerabhadra...@oracle.com>
Cc: HYPERLINK "mailto:2d-dev@openjdk.java.net"2d-dev@openjdk.java.net
Subject: RE: [OpenJDK 2D-Dev] [10]JDK-8183341:Better cleanup for 
javax/imageio/AllowSearch.java

 

Hello Shashi,

 

The lines before the try{} block in test() function in the test case can throw 
IOException, like createImageInputStream() call present at line no 50.
In that case we will not reach finally block and temporary file will not be 
deleted.
Including complete code of test() function in try{} and deleting resources in 
finally block will be a better option.
 

Thanks,

Jay

 

From: Shashidhara Veerabhadraiah 
Sent: Wednesday, July 12, 2017 11:03 AM
To: Sergey Bylokhov; Prahalad Kumar Narayanan
Cc: HYPERLINK "mailto:2d-dev@openjdk.java.net"2d-dev@openjdk.java.net
Subject: Re: [OpenJDK 2D-Dev] [10]JDK-8183341:Better cleanup for 
javax/imageio/AllowSearch.java

 

Sergey, Thank you for the excellent suggestion. I have updated the Webrev 
accordingly.

 

Prahalad, With the use of Sergey’s suggestion of adding an finally block solves 
the problem of not deleting files on a fail case. 

  . Was the bug reproducible at your end ?

Yes, it was reproducible. We can see the temp files hanging at %temp% folder.

  . If yes, was it reproducible when the test succeeded /or when the test 
failed ?

Yes, it was reproducible on success for sure. The fail case was not tested as 
that would require me to do an undo of an earlier changeset change. But I think 
the output would remain the same as the pass case.

  . Is there a difference in the VM's termination based on the outcome of 
the test case.

Not sure since the fail case was not tested. But per the code, it is sure that 
it will have differences. The finally block should fix this problem.

  . How many such test cases exist that need similar clean up ?

    . grep on 'deleteOnExit' within jdk/test/javax/imageio/ gives me 
atleast 10 instances.

    . grep on 'File.createTempFile' within same directory gives me 29 
instances.

Am not sure what to do with respect to them. Should I raise new bugs if there 
is problem with them(if not on the bug db) and fix it?

 

Updated Webrev:

http://cr.openjdk.java.net/~pkbalakr/shashi/8183341/webrev_01/

 

Thanks and regards,

Shashi

 

From: Sergey Bylokhov 
Sent: Wednesday, July 12, 2017 12:13 AM
To: Shashidhara Veerabhadraiah mailto:shashidhara.veerabhadra...@oracle.com"shashidhara.veerabhadra...@oracle.com>
Cc: Prasanta Sadhukhan mailto:prasanta.sadhuk...@oracle.com"prasanta.sadhuk...@oracle.com>; HYPERLINK 
"mailto:2d-dev@openjdk.java.net"2d-dev@openjdk.java.net; Philip Race mailto:philip.r...@oracle.com"philip.r...@oracle.com>
Subject: Re: [OpenJDK 2D-Dev] [10]JDK-8183341:Better cleanup for 
javax/imageio/AllowSearch.java

 

Hi Shashi.
I suggest to add a finally to the try block in the test() method and delete the 
file, close the stream and close the reader there.

- HYPERLINK 
"mailto:shashidhara.veerabhadra...@oracle.com"shashidhara.veerabhadra...@oracle.com
 wrote: 
> 

Hi All,

Please review a fix for a test bug which was not cleaning up the temporary test 
files that it used to create.

The issue with this test was that the test used to create temporary files but 
not deleting them.

The updated test file does the deletion of the temporary files that the test is 
creating.

Bug:

Re: [OpenJDK 2D-Dev] Review Request JDK-8172559 [PIT][TEST_BUG] Move @test to be 1st annotation in java/awt/image/Raster/TestChildRasterOp.java

[Ajit Ghaisas]

Looks fine.

 

Regards,

Ajit

 

From: Prem Balakrishnan 
Sent: Thursday, January 12, 2017 11:34 AM
To: Ajit Ghaisas; Prasanta Sadhukhan; 2d-dev@openjdk.java.net
Subject: Review Request JDK-8172559 [PIT][TEST_BUG] Move @test to be 1st 
annotation in java/awt/image/Raster/TestChildRasterOp.java

 

Hi,

Please review fix for JDK 9,

 

Bug: https://bugs.openjdk.java.net/browse/JDK-8172559 

Webrev: http://cr.openjdk.java.net/~pkbalakr/8172559/webrev.00/ 

 

Jtreg Error: Not a test or directory containing tests: 
java/awt/image/Raster/TestChildRasterOp.java

Cause: Wrong order of tag tokens , @test tag token should be the topmost tag.

 

Regards,

Prem

Re: [OpenJDK 2D-Dev] [9] RFR JDK-8025439: [TEST BUG] [macosx] PrintServiceLookup.lookupPrintServices doesn't work properly since jdk8b105

[Ajit Ghaisas]

The test should not worry about linux/solaris future change. If there is any 
change, we will get to know with test failure.

 

Change is OK.

Please update the year in banner & add this bug id in jtreg @bug tag before 
pushing.

 

Regards,

Ajit

 

From: Prasanta Sadhukhan 
Sent: Wednesday, November 16, 2016 11:55 AM
To: Ajit Ghaisas; Philip Race; 2d-dev
Subject: Re: [OpenJDK 2D-Dev] [9] RFR JDK-8025439: [TEST BUG] [macosx] 
PrintServiceLookup.lookupPrintServices doesn't work properly since jdk8b105

 

It can be done that way and it will solve this specific problem but I did 
!Windows because even though linux/solaris GUI does not support spaces as of 
now, GUI is easy to change and can accomodate spaces in near future
but CUPS library will not accomodate the spaces so it might fail in 
linux/solaris then.
Anyways, I am ok with making this mac specific
http://cr.openjdk.java.net/~psadhukhan/8025439/webrev.01/

Regards
Prasanta

On 11/15/2016 4:11 PM, Ajit Ghaisas wrote:

If we know that exceptional behavior (service name containing spaces) is only 
limited to Mac, then the check in test should be only Mac specific and not 
(!windows).

 

Regards,

Ajit

 

From: Prasanta Sadhukhan 
Sent: Tuesday, November 15, 2016 12:32 PM
To: Phil Race; 2d-dev
Subject: Re: [OpenJDK 2D-Dev] [9] RFR JDK-8025439: [TEST BUG] [macosx] 
PrintServiceLookup.lookupPrintServices doesn't work properly since jdk8b105

 

 

 

On 11/15/2016 4:39 AM, Phil Race wrote:

This evaluation needs to go in the bug report, not (just) here.

mac. shows spaces in the name in its GUI but "_" in the names reported by lpstat
so it may be that the replacement is right but I'd still like to dig a bit here.
Can you point to the code that does the " "->"_" replacement. I can't see it
in CUPSPrinter.getAllPrinters().

As per 
http://hg.openjdk.java.net/jdk9/client/jdk/file/449518f6a468/src/java.desktop/unix/classes/sun/print/CUPSPrinter.java#l425
it gets the response from CUPS server and the printer names obtained from 
server is stored in jdk. In my case, even though I specified "Ricoh Aficio..." 
in GUI
the "nameStr" obtained from CUPS responseMap is "Ricoh_Aficio..."




And you saying that 

PrintServiceLookup.lookupPrintServices(null, null)

will return an array with a "null" element ?

No, sorry to "eat-up words". What I meant to say, lookupPrintServices(null, 
attributes) returns array with 0 elements as checkPrinterName() return false 
[as user is asking to find "Ricoh Aficio" and not "Ricoh_Aficio"]
resulting in getServiceByName() returning null
which in turn causes lookByName() in the testcase to return null

Regards
Prasanta

That would be a bug.

-phil.

On 11/14/2016 02:18 AM, Prasanta Sadhukhan wrote:

Hi All,

Please review a small bugfix whereby it is seen that if we specify printer with 
space in its name, then 
javax/print/PrintServiceLookup/GetPrintServices.java fails citing NPE.

Bug: https://bugs.openjdk.java.net/browse/JDK-8025439
webrev: HYPERLINK 
"http://cr.openjdk.java.net/%7Epsadhukhan/8025439/webrev.00/"http://cr.openjdk.java.net/~psadhukhan/8025439/webrev.00/

The NPE happens because
http://hg.openjdk.java.net/jdk9/client/jdk/file/b1543c5eb8af/src/java.desktop/unix/classes/sun/print/PrintServiceLookupProvider.java#l460
calls checkPrinterName() which checks it name contains letter or digit
http://hg.openjdk.java.net/jdk9/client/jdk/file/b1543c5eb8af/src/java.desktop/unix/classes/sun/print/PrintServiceLookupProvider.java#l433
and returns null if has spaces
so lookupPrintServices() gets null

Now, if we remove this  check then also, it will not work as
In system running with CUPS, refreshServices calls CUPSPrinter#getAllPrinters() 
which returns a set of printers. It seems it replaces " "  with "_" when 
populating the list
for e.g Ricoh Aficio MP 5002 printer name is sent as Ricoh_Aficio_MP_5002 and 
stored in the list so we cannot have  in printer name.

In Mac, it takes  in printer name when we add printers but in linux, 
solaris it does not allow spaces in printer name during addition
so in the proposed fix, a check for  is added to make it automatic pass for 
non-windows (CUPS) system.

Regards
Prasanta





 

 

 

Re: [OpenJDK 2D-Dev] [9] RFR JDK-8025439: [TEST BUG] [macosx] PrintServiceLookup.lookupPrintServices doesn't work properly since jdk8b105

[Ajit Ghaisas]

If we know that exceptional behavior (service name containing spaces) is only 
limited to Mac, then the check in test should be only Mac specific and not 
(!windows).

 

Regards,

Ajit

 

From: Prasanta Sadhukhan 
Sent: Tuesday, November 15, 2016 12:32 PM
To: Phil Race; 2d-dev
Subject: Re: [OpenJDK 2D-Dev] [9] RFR JDK-8025439: [TEST BUG] [macosx] 
PrintServiceLookup.lookupPrintServices doesn't work properly since jdk8b105

 

 

 

On 11/15/2016 4:39 AM, Phil Race wrote:

This evaluation needs to go in the bug report, not (just) here.

mac. shows spaces in the name in its GUI but "_" in the names reported by lpstat
so it may be that the replacement is right but I'd still like to dig a bit here.
Can you point to the code that does the " "->"_" replacement. I can't see it
in CUPSPrinter.getAllPrinters().

As per 
http://hg.openjdk.java.net/jdk9/client/jdk/file/449518f6a468/src/java.desktop/unix/classes/sun/print/CUPSPrinter.java#l425
it gets the response from CUPS server and the printer names obtained from 
server is stored in jdk. In my case, even though I specified "Ricoh Aficio..." 
in GUI
the "nameStr" obtained from CUPS responseMap is "Ricoh_Aficio..."



And you saying that 

PrintServiceLookup.lookupPrintServices(null, null)

will return an array with a "null" element ?

No, sorry to "eat-up words". What I meant to say, lookupPrintServices(null, 
attributes) returns array with 0 elements as checkPrinterName() return false 
[as user is asking to find "Ricoh Aficio" and not "Ricoh_Aficio"]
resulting in getServiceByName() returning null
which in turn causes lookByName() in the testcase to return null

Regards
Prasanta

That would be a bug.

-phil.

On 11/14/2016 02:18 AM, Prasanta Sadhukhan wrote:

Hi All,

Please review a small bugfix whereby it is seen that if we specify printer with 
space in its name, then 
javax/print/PrintServiceLookup/GetPrintServices.java fails citing NPE.

Bug: https://bugs.openjdk.java.net/browse/JDK-8025439
webrev: HYPERLINK 
"http://cr.openjdk.java.net/%7Epsadhukhan/8025439/webrev.00/"http://cr.openjdk.java.net/~psadhukhan/8025439/webrev.00/

The NPE happens because
http://hg.openjdk.java.net/jdk9/client/jdk/file/b1543c5eb8af/src/java.desktop/unix/classes/sun/print/PrintServiceLookupProvider.java#l460
calls checkPrinterName() which checks it name contains letter or digit
http://hg.openjdk.java.net/jdk9/client/jdk/file/b1543c5eb8af/src/java.desktop/unix/classes/sun/print/PrintServiceLookupProvider.java#l433
and returns null if has spaces
so lookupPrintServices() gets null

Now, if we remove this  check then also, it will not work as
In system running with CUPS, refreshServices calls CUPSPrinter#getAllPrinters() 
which returns a set of printers. It seems it replaces " "  with "_" when 
populating the list
for e.g Ricoh Aficio MP 5002 printer name is sent as Ricoh_Aficio_MP_5002 and 
stored in the list so we cannot have  in printer name.

In Mac, it takes  in printer name when we add printers but in linux, 
solaris it does not allow spaces in printer name during addition
so in the proposed fix, a check for  is added to make it automatic pass for 
non-windows (CUPS) system.

Regards
Prasanta




 

 

Re: [OpenJDK 2D-Dev] [8u] RFR JDK-8162461: Hang due to JNI up-call made whilst holding JNI critical lock.

[Ajit Ghaisas]

Looks OK to me.

 

Regards,

Ajit

 

From: Philip Race 
Sent: Wednesday, October 05, 2016 9:49 PM
To: Jayathirth D V
Cc: 2d-dev
Subject: Re: [OpenJDK 2D-Dev] [8u] RFR JDK-8162461: Hang due to JNI up-call 
made whilst holding JNI critical lock.

 

+1

-phil.

On 9/29/16, 1:49 AM, Jayathirth D V wrote: 

Hi Phil,

 

New changes are submitted in JDK9 under JDK-8166685.

 

Please find updated webrev for review in JDK8u:

HYPERLINK 
"http://cr.openjdk.java.net/%7Ejdv/8162461.8u/webrev.01/"http://cr.openjdk.java.net/~jdv/8162461.8u/webrev.01/
 

 

I have verified JCK, jtreg and JPRT for updated changes in JDK8u.

 

Thanks,

Jay

 

From: Philip Race 
Sent: Friday, September 23, 2016 10:10 PM
To: Jayathirth D V
Cc: 2d-dev
Subject: Re: [OpenJDK 2D-Dev] [8u] RFR JDK-8162461: Hang due to JNI up-call 
made whilst holding JNI critical lock.

 

You can't re-use the 9 bug ID in 9. So you will need to create a new bug to fix
that in 9.

But for 8u it can be part of this fix .. it would be weird to knowingly check in
a problem just so you could fix it separately.


-phil.

On 9/23/16, 12:13 AM, Jayathirth D V wrote: 

Hi Phil,

 

I assumed setjmp/longjmp functions to be internal error handling mechanism 
rather than C standard library functions which provide non-local jumps.

I verified setjmp/longjmp usage in ImageIO JPEG context and we need that 
RELEASE_ARRAYS() call in writeImage() which I have removed.

Does I have to create a new Bug and add the RELEASE_ARRAY() call in 
writeImage() or I have to check-in this change under JDK-8162461 only? 

 

After check-in of this modification I will raise new webrev for 8u backport.

 

Thanks,

Jay

From: Philip Race 
Sent: Thursday, September 22, 2016 9:51 PM
To: Jayathirth D V
Cc: 2d-dev
Subject: Re: [OpenJDK 2D-Dev] [8u] RFR JDK-8162461: Hang due to JNI up-call 
made whilst holding JNI critical lock.

 

I see this is mostly what I approved for JDK9 but I noticed you made a change
after I approved it and I did not see or approve the updated version.
I am concerned about this comment you posted for the JDK 9 webrev :

>Regarding  "2856 RELEASE_ARRAYS(env, data, (const JOCTET 
>*)(dest->next_output_byte));". 
>We don't need this RELEASE_ARRAY() call at this place as we have not yet 
>pinned any buffer.
> So I have removed it.
>Please find updated webrev for review :
>HYPERLINK 
>"http://cr.openjdk.java.net/%7Ejdv/8162461/webrev.02/"http://cr.openjdk.java.net/~jdv/8162461/webrev.02/
> 

What makes you so sure we have not pinned a buffer by then ?
setjmp is special. It may return from any point in the writing process.

In other words that removal looks wrong to me.

-phil.

On 9/15/16, 1:29 AM, Jayathirth D V wrote: 

Hi,

 

Please review the following backport to JDK-8u from JDK-9 at your convenience:

 

JDK-9 review thread : 
http://mail.openjdk.java.net/pipermail/2d-dev/2016-September/007601.html 

 

Bug : https://bugs.openjdk.java.net/browse/JDK-8162461 

 

JDK-8u Webrev : HYPERLINK 
"http://cr.openjdk.java.net/%7Ejdv/8162461.8u/webrev.00/"http://cr.openjdk.java.net/~jdv/8162461.8u/webrev.00/
 

 

Issue : If we try to perform operations like reader.abort()/reader.dispose()/ 
reader.reset() in any of the IIOReadUpdateListener callbacks, JVM will throw 
deadlock error.

 

Root cause : We are making callbacks from native side to Java by holding some 
resources in JNI critical lock.

 

Solution : We have to release the JNI critical lock on the resources before we 
call Java function from native side. If we have JNI critical lock and we throw 
an Exception in that case also we should release the lock.

 

I have verified jtreg, JCK and JPRT in JDK8u-dev also.

 

Thanks,

Jay

 

Re: [OpenJDK 2D-Dev] [8u Backport] Fix for JDK-8158356 : SIGSEGV when attempting to rotate BufferedImage using AffineTransform by NaN degrees

[Ajit Ghaisas]

Hi Jim,

I agree upon aborting the operation in case the non-finite slope is not due 
to divide by 0 case. 
As this is the 8u backport of the fix done in 9, I will address the review 
comment in JDK-8166009.

The original backport webrev is still the same. 
http://cr.openjdk.java.net/~aghaisas/8158356/8u_backport/webrev.00/

Request you to approve this 8u backport.

Regards,
Ajit

-Original Message-
From: Jim Graham 
Sent: Friday, September 02, 2016 1:16 AM
To: Ajit Ghaisas; Philip Race; 2d-dev
Subject: Re: [8u Backport] Fix for JDK-8158356 : SIGSEGV when attempting to 
rotate BufferedImage using AffineTransform by NaN degrees

Hi Ajit,

In the cases where you "continue" on a non-finite slope, doesn't that mean that 
the edges will be mismatched?  If you can't determine the bounding polygon, 
perhaps the entire operation should be aborted instead...?

It's different from the case of dy1==dy2 which also results in a "continue" 
because in that case there are no edges to insert in the array because that 
edge of the bounding box doesn't occupy any vertical space.  In the case of an 
infinite slope, there could be edges expected to be produced (in fact there 
should be because we've already tested that dy1 != dy2), but the loop doesn't 
produce any edge values and stale data is left in the array.

        ...jim

On 9/1/16 2:14 AM, Ajit Ghaisas wrote:
> Hi,
>
>
>
>This is a review request for 8u back-port of the fix done in JDK-9.
>
>
>
> Bug :
>
>https://bugs.openjdk.java.net/browse/JDK-8158356
>
>
>
> Root cause :
>
>The root cause of the crash is - NaN is converted to an integer and used 
> as array index in mlib_ImageScanPoly.c.
>
>
>
> Fix :
>
>The native method previously did not check the validity of the 
> input double argument. Now, I have added a check for finite double values.
>
>
>
> Webrev :
>
> 
> http://cr.openjdk.java.net/~aghaisas/8158356/8u_backport/webrev.00/
>
>
>
> Regards,
>
> Ajit
>

[OpenJDK 2D-Dev] [8u Backport] Fix for JDK-8158356 : SIGSEGV when attempting to rotate BufferedImage using AffineTransform by NaN degrees

[Ajit Ghaisas]

Hi,

 

   This is a review request for 8u back-port of the fix done in JDK-9.

   

Bug : 

   https://bugs.openjdk.java.net/browse/JDK-8158356

 

Root cause :

   The root cause of the crash is - NaN is converted to an integer and used as 
array index in mlib_ImageScanPoly.c.

 

Fix :

   The native method previously did not check the validity of the input double 
argument. Now, I have added a check for finite double values.

 

Webrev :

http://cr.openjdk.java.net/~aghaisas/8158356/8u_backport/webrev.00/

 

Regards,

Ajit

Re: [OpenJDK 2D-Dev] [9]Fix for JDK-8158356 : SIGSEGV when attempting to rotate BufferedImage using AffineTransform by NaN degrees

[Ajit Ghaisas]

Hi,

   The root cause of the crash is - NaN is converted to an integer and used as 
array index in mlib_ImageScanPoly.c.
   The native method previously did not check the validity of the input double 
argument. Now, I have added a check for finite double values.
   
   If NaN or INF arguments are present in affine transform, the changes done in 
native code result in ImagingOpException ("Unable to transform src image") in 
AffineTransformOp.filter() methods.
   The constructors in AffineTransformOp.java are left unchanged. 
   Please note that, this fix fixes the crash. The behavioral change in 
AffineTransformOp for such inputs will be fixed under JDK-8164729.

   Please review the updated webrev.
   http://cr.openjdk.java.net/~aghaisas/8158356/webrev.01/

Regards,
Ajit

-Original Message-
From: Philip Race 
Sent: Thursday, August 11, 2016 3:22 AM
To: Jim Graham
Cc: Ajit Ghaisas; 2d-dev
Subject: Re: [OpenJDK 2D-Dev] [9]Fix for JDK-8158356 : SIGSEGV when attempting 
to rotate BufferedImage using AffineTransform by NaN degrees

Agreed, I had previously asked for that too (off-line).
ie. root cause why a NaN would cause a crash ..

-phil.

On 8/10/16, 2:47 PM, Jim Graham wrote:
> This does address the specific test case directly, but I'd be happier 
> if we dug down and figured out where it went wrong in trying to 
> transform the image and put in a fix that addressed the root problem 
> whether it comes from the inputs being NaN or from some other similar 
> condition that could also trigger the same poorly written transform 
> code...
>
> ...jim
>
> On 8/10/16 2:22 PM, Phil Race wrote:
>> 1) The spec for the constructors needs to be updated to include this 
>> reason for throwing ImagingOpException. A CCC request will be needed.
>>
>> 2) The C usage of "isnan()" may be problematic in some compilation 
>> environments.
>> For example I believe this will not compile with VS2010, and many 
>> folks still use that.
>> Instead you could use matrix[j] != matrix[j] as the two values should 
>> not compare equal if it is NaN.
>>
>> -phil.
>>
>> On 08/10/2016 04:15 AM, Ajit Ghaisas wrote:
>>> Hi,
>>>
>>>  Bug : https://bugs.openjdk.java.net/browse/JDK-8158356
>>>
>>>  Issue : AffineTransform using NaN value as input parameter 
>>> results in SIGSEGV.
>>>
>>>  Fix : Transformation matrix is checked for NaN values in 
>>> AffineTransformOp.validateTransform().
>>>   Also, at native level a separate check is made to 
>>> return error in case of NaN values.
>>>
>>>  Webrev : 
>>> http://cr.openjdk.java.net/~aghaisas/8158356/webrev.00/
>>>
>>>  Request you to review.
>>>
>>> Regards,
>>> Ajit
>>>
>>

[OpenJDK 2D-Dev] [9]Fix for JDK-8158356 : SIGSEGV when attempting to rotate BufferedImage using AffineTransform by NaN degrees

[Ajit Ghaisas]

Hi,

Bug : https://bugs.openjdk.java.net/browse/JDK-8158356

Issue : AffineTransform using NaN value as input parameter results in 
SIGSEGV.

Fix : Transformation matrix is checked for NaN values in 
AffineTransformOp.validateTransform().
 Also, at native level a separate check is made to return error in 
case of NaN values.

Webrev : http://cr.openjdk.java.net/~aghaisas/8158356/webrev.00/

Request you to review.

Regards,
Ajit

Re: [OpenJDK 2D-Dev] [9] Fix for JDK-6427331 : NullPointerException in LookupOp.filter(Raster, WritableRaster)

[Ajit Ghaisas]

Thanks Prasanta.

I checked filter() methods of following classes -

AffineTransformOp, ColorConvertOp, ConvolveOp, LookupOp, RescaleOp and 
BandCombineOp.

They do not have this issue of accessing null destination buffer/raster.

 

From: Prasanta Sadhukhan 
Sent: Tuesday, August 02, 2016 2:59 PM
To: Ajit Ghaisas; 2d-dev
Subject: Re: [OpenJDK 2D-Dev] [9] Fix for JDK-6427331 : NullPointerException in 
LookupOp.filter(Raster, WritableRaster)

 

+1. Just check other op file like Rescale, Convolve if similar problem exists 
there too.

Regards
Prasanta

On 8/2/2016 2:31 PM, Ajit Ghaisas wrote:

Hi,

 

   Bug : https://bugs.openjdk.java.net/browse/JDK-6427331

   Issue : If destination raster is provided as null to  
java.awt.image.LookupOp.filter(Raster src, WritableRaster dest),

according to Javadoc, a new raster should be created. Instead, 
it results in NullPointerException.

 

   Fix : HYPERLINK 
"http://cr.openjdk.java.net/%7Eaghaisas/6427331/webrev.00/"http://cr.openjdk.java.net/~aghaisas/6427331/webrev.00/

 

   Request you to review.

 

Regards,

Ajit

 

[OpenJDK 2D-Dev] [9] Fix for JDK-6427331 : NullPointerException in LookupOp.filter(Raster, WritableRaster)

[Ajit Ghaisas]

Hi,

 

   Bug : https://bugs.openjdk.java.net/browse/JDK-6427331

   Issue : If destination raster is provided as null to  
java.awt.image.LookupOp.filter(Raster src, WritableRaster dest),

according to Javadoc, a new raster should be created. Instead, 
it results in NullPointerException.

 

   Fix : http://cr.openjdk.java.net/~aghaisas/6427331/webrev.00/

 

   Request you to review.

 

Regards,

Ajit

Re: [OpenJDK 2D-Dev] [9] RFR JDK-4882305: StreamPrintServ.getSupportedAttributeValues returns null for Orientation attr

[Ajit Ghaisas]

Hi Prasanta,

 

1.   The test contains commented code – that needs to be removed

2.   The test only tests for image/jpeg docFlavor – what about image/gif 
and image/png docflavors testing?

3.   The year in banner at the top of PSStreamPrintService.java can be 
updated

 

Regards,

Ajit

 

From: Philip Race 
Sent: Wednesday, July 20, 2016 8:44 PM
To: Prasanta Sadhukhan
Cc: 2d-dev
Subject: Re: [OpenJDK 2D-Dev] [9] RFR JDK-4882305: 
StreamPrintServ.getSupportedAttributeValues returns null for Orientation attr

 

+1

-phil.

On 7/14/16, 2:09 AM, Prasanta Sadhukhan wrote: 

Hi All,

Please review a fix for an issue where it is seen that 
even though StreamPrintService returns OrientationRequested category as 
supported, when actually querying the supported attribute value with respect to 
the supported DocFlavors, 
null values are returned for the Orientation attributes when the DocFlavor is 
not either Pageable or Printable (SERVICE_FORMATTED)
BUT we can print a jpeg iamge using StreamPrintService in LANDSCAPE mode and it 
worked fine so StreamPrintService should not return the supported values as 
null for supported DocFlavor.

Bug: https://bugs.openjdk.java.net/browse/JDK-4882305
webrev: HYPERLINK 
"http://cr.openjdk.java.net/%7Epsadhukhan/4882305/webrev.00/"http://cr.openjdk.java.net/~psadhukhan/4882305/webrev.00/

Proposed fix is to add the image/jpeg, image/gif, image/png docflavor in 
addition to Pageable/Printable for Orientation attribute.

Regards
Prasanta

Re: [OpenJDK 2D-Dev] [9] Fix for JDK-8074824: Resolve disabled warnings for libawt_xawt

[Ajit Ghaisas]

Thanks Phil for the review.
Please find my answers below.

Semyon, can you please comment on Phil's question below?

Regards,
Ajit

-Original Message-
From: Phil Race 
Sent: Wednesday, July 06, 2016 2:02 AM
To: Ajit Ghaisas
Cc: Sergey Bylokhov; Erik Joelsson; 2d-dev; awt-...@openjdk.java.net; 
build-...@openjdk.java.net
Subject: Re: [9] Fix for JDK-8074824: Resolve disabled warnings for libawt_xawt

It is not always clear to me what warning is being suppressed and why you have 
chosen a particular solution/action


this next one looks like it might introduce an unused variable warning.
What was it solving ? That the code was not checking a return value ?

size_t bytesWritten = write ( AWT_WRITEPIPE, &wakeUp_char, 1 ); //bytesWritten 
is unused

Isn't the compiler's point here that you *should* be doing something with the 
result?
Not just ignoring it differently ...

--
[Ajit] : there was a warning of type 'unused-result' for write() method. Now, I 
have just assigned that to a variable which fixes the warning.
I think, we should have some code to do error check on bytesWritten and return 
it - but it is out of purview of this fix as it may introduce behavioral change.
--

-

and this one ? I want Semyon to comment on what this code is trying to do in 
its original form since it was added for GTK3.

@@ -1989,11 +2029,7 @@
  static guint8 recode_color(gdouble channel)
  {
  guint16 result = (guint16)(channel * 65535);
-if (result < 0) {
-result = 0;
-} else if (result > 65535) {
-result = 65535;
-}
+
  return (guint8)( result >> 8);
  }

-
[Ajit] : there was a warning about guint16 will not be less than 0 and larger 
than 65535. Hence I have removed code checking this range.
-

-----


-phil.

On 06/23/2016 12:09 AM, Ajit Ghaisas wrote:
> Hi,
>
> Bug :
>  https://bugs.openjdk.java.net/browse/JDK-8074824
>  (Resolve disabled warnings for libawt_xawt)
>
> As part of fixing this bug, I have -
>
>  1. Fixed warnings in source code after removing blanket warning 
> suppressions from makefile.
>
>  2. In case the warning fix is not possible, converted blanket warning 
> suppression for this library to suppression of warnings for individual files.
>
>  3. Added comments in makefile for the warning suppression that cannot be 
> fixed.
>
> One type of gcc warning 'deprecated-declarations' will be fixed 
> separately (as part of JDK-8160146)
>
>
> I have built the changes successfully on all supported platforms.
>
>
> Webrev :
>  http://cr.openjdk.java.net/~aghaisas/8074824/webrev.00/
>
> Request you to review.
>
> Regards,
> Ajit

Re: [OpenJDK 2D-Dev] [9] Fix for JDK-8160421 : Regression: JDK-8139192 causes NPE in java.awt.Toolkit.createCustomCursor()

[Ajit Ghaisas]

Thanks for the review.

I have corrected the test case name as per suggestion.
Here is the updated webrev.
http://cr.openjdk.java.net/~aghaisas/8160421/webrev.02/

Yes, the test captures the redirected contents of stderr correctly when run 
using jtreg - both with or without othervm specified.

Regards,
Ajit

-Original Message-
From: Phil Race 
Sent: Wednesday, July 06, 2016 12:48 AM
To: Ajit Ghaisas
Cc: Jim Graham; Sergey Bylokhov; 2d-dev
Subject: Re: [OpenJDK 2D-Dev] [9] Fix for JDK-8160421 : Regression: JDK-8139192 
causes NPE in java.awt.Toolkit.createCustomCursor()

I prefer meaningful test names .. the bug id is already in the @bug tag and a 
test can later be updated to test > 1 bug.
Did you verify that you are in fact capturing stderr - even in othervm mode - 
when running the test under jtreg ?
Even when setting up a new VM jtreg may capture this for you.

-phil.


On 07/05/2016 04:42 AM, Ajit Ghaisas wrote:
> Thanks Jim for suggesting test should have its own VM. I agree with it.
>
> Here is the updated webrev :
> http://cr.openjdk.java.net/~aghaisas/8160421/webrev.01/
>
> Phil, can you please review it?
>
> Regards,
> Ajit
>
>
> -Original Message-
> From: Jim Graham
> Sent: Friday, July 01, 2016 5:13 AM
> To: Ajit Ghaisas; Philip Race; Sergey Bylokhov; 2d-dev
> Subject: Re: [OpenJDK 2D-Dev] [9] Fix for JDK-8160421 : Regression: 
> JDK-8139192 causes NPE in java.awt.Toolkit.createCustomCursor()
>
> The solution looks fine to me.
>
> I didn't run the test case, but about the only issue I can see with it is 
> whether it should have a flag to run it in its own VM because of its 
> interaction with System.err.  Phil?
>
>   ...jim
>
> On 06/30/2016 06:41 AM, Ajit Ghaisas wrote:
>> Hi,
>>
>> Bug :
>>   This is regarding a NPE getting printed as stacktrace in 
>> java.awt.Toolkit.createCustomCursor() method.
>>   The bug is described at :
>> https://bugs.openjdk.java.net/browse/JDK-8160421
>>
>> Root Cause :
>>   Fix of bug JDK-8139192 : Custom ImageFilters return blank images in 
>> Java 8(.45) while working in 7.
>>   The above fix added a RuntimeException catch & log mechanism to 
>> support a ImageFilter which was broken due to the second imageComplete() 
>> call in OffScreenImageSource.produce() method.
>>
>> Analysis:
>>   Without the fix of JDK-8139192, java.awt.Toolkit.createCustomCursor() 
>> call results in
>>   NullPointerException when imageComplete(ImageConsumer.STATICIMAGEDONE) 
>> call is made - but, it gets consumed silently.
>>   Cathing RuntimeException() out of 
>> imageComplete(ImageConsumer.STATICIMAGEDONE) was added to fix JDK-8160421.
>>   This started catching & logging the exception in case 'theConsumer' 
>> has unregistered itself as a result of call
>>  
>> theConsumer.imageComplete(ImageConsumer.SINGLEFRAMEDONE);
>>This log is undesirable as this mechanism is used in 
>> java.awt.Toolkit.createCustomCursor() and may be in other places.
>>
>> Fix :
>>   Make the call to imageComplete(ImageConsumer.STATICIMAGEDONE) only if 
>> 'theConsumer' has not been unregistered.
>>
>> Webrev:
>>   http://cr.openjdk.java.net/~aghaisas/8160421/webrev.00/
>>
>> Request you to review.
>>
>> Regards,
>> Ajit
>>

Re: [OpenJDK 2D-Dev] [9] Fix for JDK-8160421 : Regression: JDK-8139192 causes NPE in java.awt.Toolkit.createCustomCursor()

[Ajit Ghaisas]

Thanks Jim for suggesting test should have its own VM. I agree with it.

Here is the updated webrev :
http://cr.openjdk.java.net/~aghaisas/8160421/webrev.01/

Phil, can you please review it?

Regards,
Ajit


-Original Message-
From: Jim Graham 
Sent: Friday, July 01, 2016 5:13 AM
To: Ajit Ghaisas; Philip Race; Sergey Bylokhov; 2d-dev
Subject: Re: [OpenJDK 2D-Dev] [9] Fix for JDK-8160421 : Regression: JDK-8139192 
causes NPE in java.awt.Toolkit.createCustomCursor()

The solution looks fine to me.

I didn't run the test case, but about the only issue I can see with it is 
whether it should have a flag to run it in its own VM because of its 
interaction with System.err.  Phil?

...jim

On 06/30/2016 06:41 AM, Ajit Ghaisas wrote:
> Hi,
>
> Bug :
>  This is regarding a NPE getting printed as stacktrace in 
> java.awt.Toolkit.createCustomCursor() method.
>  The bug is described at : 
> https://bugs.openjdk.java.net/browse/JDK-8160421
>
> Root Cause :
>  Fix of bug JDK-8139192 : Custom ImageFilters return blank images in Java 
> 8(.45) while working in 7.
>  The above fix added a RuntimeException catch & log mechanism to support 
> a ImageFilter which was broken due to the second imageComplete() call in 
> OffScreenImageSource.produce() method.
>
> Analysis:
>  Without the fix of JDK-8139192, java.awt.Toolkit.createCustomCursor() 
> call results in
>  NullPointerException when imageComplete(ImageConsumer.STATICIMAGEDONE) 
> call is made - but, it gets consumed silently.
>  Cathing RuntimeException() out of 
> imageComplete(ImageConsumer.STATICIMAGEDONE) was added to fix JDK-8160421.
>  This started catching & logging the exception in case 'theConsumer' has 
> unregistered itself as a result of call
> theConsumer.imageComplete(ImageConsumer.SINGLEFRAMEDONE);
>   This log is undesirable as this mechanism is used in 
> java.awt.Toolkit.createCustomCursor() and may be in other places.
>
> Fix :
>  Make the call to imageComplete(ImageConsumer.STATICIMAGEDONE) only if 
> 'theConsumer' has not been unregistered.
>
> Webrev:
>  http://cr.openjdk.java.net/~aghaisas/8160421/webrev.00/
>
> Request you to review.
>
> Regards,
> Ajit
>

[OpenJDK 2D-Dev] [9] Fix for JDK-8160421 : Regression: JDK-8139192 causes NPE in java.awt.Toolkit.createCustomCursor()

[Ajit Ghaisas]

Hi,

Bug :
This is regarding a NPE getting printed as stacktrace in 
java.awt.Toolkit.createCustomCursor() method.
The bug is described at : https://bugs.openjdk.java.net/browse/JDK-8160421

Root Cause :
Fix of bug JDK-8139192 : Custom ImageFilters return blank images in Java 
8(.45) while working in 7. 
The above fix added a RuntimeException catch & log mechanism to support a 
ImageFilter which was broken due to the second imageComplete() call in 
OffScreenImageSource.produce() method.
   
Analysis:
Without the fix of JDK-8139192, java.awt.Toolkit.createCustomCursor() call 
results in 
NullPointerException when imageComplete(ImageConsumer.STATICIMAGEDONE) call 
is made - but, it gets consumed silently.
Cathing RuntimeException() out of 
imageComplete(ImageConsumer.STATICIMAGEDONE) was added to fix JDK-8160421.
This started catching & logging the exception in case 'theConsumer' has 
unregistered itself as a result of call
   theConsumer.imageComplete(ImageConsumer.SINGLEFRAMEDONE);
 This log is undesirable as this mechanism is used in 
java.awt.Toolkit.createCustomCursor() and may be in other places.

Fix :
Make the call to imageComplete(ImageConsumer.STATICIMAGEDONE) only if 
'theConsumer' has not been unregistered.

Webrev:
http://cr.openjdk.java.net/~aghaisas/8160421/webrev.00/

Request you to review.

Regards,
Ajit

[OpenJDK 2D-Dev] [9] Fix for JDK-8074824: Resolve disabled warnings for libawt_xawt

[Ajit Ghaisas]

Hi,

Bug :
https://bugs.openjdk.java.net/browse/JDK-8074824
(Resolve disabled warnings for libawt_xawt)

As part of fixing this bug, I have -

1. Fixed warnings in source code after removing blanket warning 
suppressions from makefile.

2. In case the warning fix is not possible, converted blanket warning 
suppression for this library to suppression of warnings for individual files. 

3. Added comments in makefile for the warning suppression that cannot be 
fixed. 

   One type of gcc warning 'deprecated-declarations' will be fixed separately 
(as part of JDK-8160146)


I have built the changes successfully on all supported platforms.


Webrev : 
http://cr.openjdk.java.net/~aghaisas/8074824/webrev.00/

Request you to review.

Regards,
Ajit

Re: [OpenJDK 2D-Dev] Fix for JDK-8074829 : Resolve disabled warnings for libawt_headless

[Ajit Ghaisas]

Hi,

I faced merge issues while getting the webrev.00 committed.

I have taken the latest code and merged webrev.00 changes on top of it to 
generate webrev.01.

Here is the updated webrev.
http://cr.openjdk.java.net/~aghaisas/8074829/webrev.01/

The changes are exactly the same between webrev.00 & webrev.01.
Request you to kindly review and approve again. 

Regards,
Ajit

-Original Message-
From: Phil Race 
Sent: Tuesday, May 31, 2016 10:05 PM
To: Ajit Ghaisas
Cc: Sergey Bylokhov; 2d-dev; build-...@openjdk.java.net
Subject: Re:  Fix for JDK-8074829 : Resolve disabled warnings for 
libawt_headless

+1 given your off-list confirmation that JPRT built all combinations of 
platforms it can ..

-phil.

On 04/28/2016 05:44 AM, Ajit Ghaisas wrote:
> Hi,
>
>   I tried excluding files under directory : 
> jdk/src/java.desktop/share/native/common/java2d/opengl from libawt_headless.
>   It resulted in compilation errors - as the headers in this directory 
> (and under sub-directory J2D_GL) are used in other places.
>   
>   To Phil's question on - why I mentioned only OGLBlitLoops.c file? - 
> this is the file where warning is reported and build stopped.
>   
>   Hence, I propose not to remove the suppression of warning 
> E_EMPTY_TRANSLATION_UNIT  in make file for Solaris.
>   There is no change in original webrev : 
> http://cr.openjdk.java.net/~aghaisas/8074829/webrev.00/
>
>   Based on Erik's suggestion, I have built it for arm and arm64 with no 
> errors.
>
> Regards,
> Ajit
>
> -Original Message-
> From: Phil Race
> Sent: Friday, April 22, 2016 1:43 AM
> To: Ajit Ghaisas
> Cc: Sergey Bylokhov; 2d-dev; build-...@openjdk.java.net
> Subject: Re:  Fix for JDK-8074829 : Resolve disabled warnings for 
> libawt_headless
>
>   >  Another solution is to exclude this file from HEADLESS compilation.
>   >  I am not sure how to achieve it. Any suggestion?
>
> I suppose that is possible and I expect we can do that See in the make file, 
> where I think you just need to add entries to
>   LIBAWT_HEADLESS_EXCLUDES := medialib
>
> although I have not tried it.
> Hmm .. I wonder why medialib needs to be explicitly excluded from headless ? 
> .. but that is for another day.
>
> I have another question: why do you mention only OGLBlitLoops.c ?
> I've flicked through a number of the C files in the same location and all 
> look to have the same issue.
>
> -phil.
>
> On 04/21/2016 06:33 AM, Ajit Ghaisas wrote:
>>>> On 04/20/2016 12:27 PM, Sergey Bylokhov wrote:
>>>> 2d-dev added.
>>> In fact all these are 2D. No AWT warnings here.
>>>> I am not sure but why "declaration in the code" is a bad thing and
>>>> we should fix it?
>>>> - DISABLED_WARNINGS_solstudio := E_DECLARATION_IN_CODE
>>>>
>>>> I cannot find the documentation in solaris studio for this warning.
>>> I don't mind fixing it if it is still an issue but does the current 
>>> compiler actually complain about it ?
>>> The SS11 -> SS12 upgrade might have got a more modern C compiler ..
>> [Ajit ] Yes. The Solaris compiler still complains about this declaration in 
>> code. Hence, I have fixed the reported warnings after removing the 
>> suppression from makefile.
>>
>>>> On 20.04.16 11:57, Ajit Ghaisas wrote:
>>>>> Hi,
>>>>>
>>>>>Bug : https://bugs.openjdk.java.net/browse/JDK-8074829
>>>>>This bug is to remove warning suppressions from makefile and
>>>>> fix the warnings for libawt_headless library.
>>>>>
>>>>>I have removed following warning suppressions & fixed the
>>>>> warnings for libawt_headless library.
>>>>>DISABLED_WARNINGS_gcc := maybe-uninitialized
>>>>> int-to-pointer-cast
>>> What made that one go away ??
>> [Ajit]  :  I fixed warnings reported for 'maybe-uninitialized' and 
>> 'E_DECLARATION_IN_CODE' warning types in two .c files in webrev.
>> There was no warning after removal of 'int-to-pointer-cast' suppression from 
>> makefile. No code change was made for this type of warning.
>>
>>
>>>>>DISABLED_WARNINGS_solstudio := E_DECLARATION_IN_CODE
>>>>>
>>>>>Warning suppression that cannot be removed :
>>>>>DISABLED_WARNINGS_solstudio := E_EMPTY_TRANSLATION_UNIT
>>>>>This is due to the fact that -
>>>>> jdk/src/java.desktop/share/native/common/java2d/opengl/OGLBlitL

Re: [OpenJDK 2D-Dev] Fix for JDK-8139192 : Custom ImageFilters return blank images in Java 8(.45) while working in 7

[Ajit Ghaisas]

Thanks Jim and Phil for reviewing this.

Adding a comment is a good suggestion. I have added it and requested a push of 
following webrev-
http://cr.openjdk.java.net/~aghaisas/8139192/webrev.04/
(Sent here for a reference)

Regards,
Ajit



-Original Message-
From: Jim Graham 
Sent: Thursday, June 02, 2016 2:54 AM
To: Ajit Ghaisas; Philip Race; Sergey Bylokhov; 2d-dev
Subject: Re: [OpenJDK 2D-Dev] Fix for JDK-8139192 : Custom ImageFilters return 
blank images in Java 8(.45) while working in 7

+1.

If you wanted to add a comment on the new catch, you could say:

} catch (RuntimeException e) {
 // We did not previously call this method here and some filters
 // were not prepared for it to be called at this time so we
 // allow them to have runtime issues for this one call only
 // without triggering the ERROR condition below.
 ... (printstack)
}

Your call, I don't need to approve a webrev for that comment addition...

...jim

On 6/1/16 3:28 AM, Ajit Ghaisas wrote:
> Thanks Jim and Phil for valuable feedback and suggestions.
>
> The detailed discussion helped me to understand why catching RuntimeException 
> is a better option for a call to imageComplete(STATICIMAGEDONE).
> I have modified the code on line 192 accordingly.
>
> I have not modified the code catching NullPointerException (on line 196) as 
> doing so would mask a wider variety of legitimate exceptions. So, line 196 
> remains unchanged.
>
> I have also modified the test to throw NullPointerException instead of 
> calling intValue() on a null.
>
> Please review updated webrev :
> http://cr.openjdk.java.net/~aghaisas/8139192/webrev.03/
>
> Regards,
> Ajit
>
> -Original Message-
> From: Phil Race
> Sent: Wednesday, June 01, 2016 1:41 AM
> To: Jim Graham; Ajit Ghaisas
> Cc: Sergey Bylokhov; 2d-dev
> Subject: Re: [OpenJDK 2D-Dev] Fix for JDK-8139192 : Custom 
> ImageFilters return blank images in Java 8(.45) while working in 7
>
> I agree with the suggestion of RuntimeException as likely sufficient but not 
> too much ...
>
> -phil.
>
>
> On 5/31/2016 12:29 PM, Jim Graham wrote:
>> I agree with this.  It would be nice if we didn't spit out print 
>> statements by default, but I'm not sure what a good flag would be to 
>> use to diagnose this that balances how likely developers will think 
>> to use it.
>>
>> The point about catching a wider variety of exceptions is not that it 
>> is good practice in general, but we've added a new call to a 
>> long-standing algorithm which has decades (really?  plural? pretty 
>> much, I think) of past code that isn't expecting the new call, and 
>> that call is basically "informative" not something that they really 
>> need to successfully handle, so just as someone might accidentally 
>> process a null pointer in a case like that, someone else might run 
>> off the end of an array (AIOOB) or any other "I wasn't expecting that"
>> sort of issue.
>>
>> Throwable might be a bit much, though.  Phil?  I was thinking 
>> Exception because those tend to focus on "your code made a mistake"
>> whereas Error is something like "you are mix/matching incompatible 
>> code that was compiled to contain conflicting information" (kind of 
>> like calling a method that doesn't exist in this release, etc.).  
>> It's hard to say what the proper level of forgiveness should be here.
>> Another thought is "RuntimeException", since any other exception 
>> would need to be declared to be thrown and we don't declare it for 
>> that method, so their implementation shouldn't be allowed to declare 
>> it either...
>>
>> ...jim
>>
>> On 05/31/2016 10:34 AM, Phil Race wrote:
>>> Based on what Ajit wrote in the bug report, he at least found a jar 
>>> file that contains this code, but I have so far failed to locate the 
>>> source code as all the versions I find on the net use the Java 2D 
>>> JDK 1.2 BufferedImageOp APIs so I suspect this bug is in a very old 
>>> version and it may not be open source.
>>> Therefore I am not sure how much e.printStackTrace() will help them 
>>> if they don't own that source except to encourage them to upgrade.
>>> I'd be inclined to stick it behind a debugging property if we have 
>>> one that we could re-use except that in such a case they probably 
>>> won't  know enough to set the property.
>>> So on balance it is probably best to do as it has been presented 
>>> here except that catch (Throwable t) probably makes sense as we 
>>> d

Re: [OpenJDK 2D-Dev] Fix for JDK-8139192 : Custom ImageFilters return blank images in Java 8(.45) while working in 7

[Ajit Ghaisas]

Thanks Jim and Phil for valuable feedback and suggestions.

The detailed discussion helped me to understand why catching RuntimeException 
is a better option for a call to imageComplete(STATICIMAGEDONE).
I have modified the code on line 192 accordingly.

I have not modified the code catching NullPointerException (on line 196) as 
doing so would mask a wider variety of legitimate exceptions. So, line 196 
remains unchanged.

I have also modified the test to throw NullPointerException instead of calling 
intValue() on a null.

Please review updated webrev :
http://cr.openjdk.java.net/~aghaisas/8139192/webrev.03/

Regards,
Ajit

-Original Message-
From: Phil Race 
Sent: Wednesday, June 01, 2016 1:41 AM
To: Jim Graham; Ajit Ghaisas
Cc: Sergey Bylokhov; 2d-dev
Subject: Re: [OpenJDK 2D-Dev] Fix for JDK-8139192 : Custom ImageFilters return 
blank images in Java 8(.45) while working in 7

I agree with the suggestion of RuntimeException as likely sufficient but not 
too much ...

-phil.


On 5/31/2016 12:29 PM, Jim Graham wrote:
> I agree with this.  It would be nice if we didn't spit out print 
> statements by default, but I'm not sure what a good flag would be to 
> use to diagnose this that balances how likely developers will think to 
> use it.
>
> The point about catching a wider variety of exceptions is not that it 
> is good practice in general, but we've added a new call to a 
> long-standing algorithm which has decades (really?  plural? pretty 
> much, I think) of past code that isn't expecting the new call, and 
> that call is basically "informative" not something that they really 
> need to successfully handle, so just as someone might accidentally 
> process a null pointer in a case like that, someone else might run off 
> the end of an array (AIOOB) or any other "I wasn't expecting that"
> sort of issue.
>
> Throwable might be a bit much, though.  Phil?  I was thinking 
> Exception because those tend to focus on "your code made a mistake"
> whereas Error is something like "you are mix/matching incompatible 
> code that was compiled to contain conflicting information" (kind of 
> like calling a method that doesn't exist in this release, etc.).  It's 
> hard to say what the proper level of forgiveness should be here.
> Another thought is "RuntimeException", since any other exception would 
> need to be declared to be thrown and we don't declare it for that 
> method, so their implementation shouldn't be allowed to declare it 
> either...
>
> ...jim
>
> On 05/31/2016 10:34 AM, Phil Race wrote:
>> Based on what Ajit wrote in the bug report, he at least found a jar 
>> file that contains this code, but I have so far failed to locate the 
>> source code as all the versions I find on the net use the Java 2D  
>> JDK 1.2 BufferedImageOp APIs so I suspect this bug is in a very old 
>> version and it may not be open source.
>> Therefore I am not sure how much e.printStackTrace() will help them 
>> if they don't own that source except to encourage them to upgrade.
>> I'd be inclined to stick it behind a debugging property if we have 
>> one that we could re-use except that in such a case they probably 
>> won't  know enough to set the property.
>> So on balance it is probably best to do as it has been presented here 
>> except that catch (Throwable t) probably makes sense as we don't want 
>> to revisit this for a different exception.
>>
>> Minor nit about the test: instead of calling intValue() on a null 
>> Integer, why not just  "throw new NullPointerException("reason ..") ?
>>
>>
>> -phil.
>>
>>
>>
>>
>>
>> On 05/30/2016 01:22 AM, Ajit Ghaisas wrote:
>>> I did contemplate catching 'Exception' instead of
>>> 'NullPointerException' while raising the webrev, but decided not to do
>>> it as-
>>> 1. Complaint in current image filter was due to NPE only
>>> 2. Catching & consuming generic exception does not seem quite correct
>>> here as we are just printing stack trace and not taking any concrete
>>> action.
>>> 3. There is no reported issue of any other type of Exception out of
>>> this method.
>>>
>>> Regards,
>>> Ajit
>>>
>>>
>>> -Original Message-
>>> From: Jim Graham
>>> Sent: Saturday, May 28, 2016 4:41 AM
>>> To: Ajit Ghaisas; Sergey Bylokhov; 2d-dev; Philip Race
>>> Subject: Re: [OpenJDK 2D-Dev] Fix for JDK-8139192 : Custom
>>> ImageFilters return blank images in Java 8(.45) while working in 7
>>>
>&g

Re: [OpenJDK 2D-Dev] Fix for JDK-8139192 : Custom ImageFilters return blank images in Java 8(.45) while working in 7

[Ajit Ghaisas]

I did contemplate catching 'Exception' instead of 'NullPointerException' while 
raising the webrev, but decided not to do it as-
1. Complaint in current image filter was due to NPE only
2. Catching & consuming generic exception does not seem quite correct here as 
we are just printing stack trace and not taking any concrete action.
3. There is no reported issue of any other type of Exception out of this method.

Regards,
Ajit


-Original Message-
From: Jim Graham 
Sent: Saturday, May 28, 2016 4:41 AM
To: Ajit Ghaisas; Sergey Bylokhov; 2d-dev; Philip Race
Subject: Re: [OpenJDK 2D-Dev] Fix for JDK-8139192 : Custom ImageFilters return 
blank images in Java 8(.45) while working in 7

That looks good, but I wonder if it should catch all exceptions instead of just 
NPE.  Historically we were only catching NPE before we added the call to 
STATICIMAGEDONE, and the current filter in question turns out to throw an NPE, 
but if a filter that was commonly used with offscreen images was not expecting 
the STATICIMAGEDONE as was this WaterFilter, then they could potentially throw 
a wider variety of exceptions.

I'm good with NPE since that is the only case we've seen, but I'd also be good 
with changing line 192 to catch all exceptions.  Phil?

    ...jim

On 5/27/16 1:32 AM, Ajit Ghaisas wrote:
> Hi Jim,
>
> Thanks for in-depth analysis and detailed review.
> I appreciate your concern for filtered image being blank in Java 8, while 
> it used to work in Java 7.
>
> Yes. I totally agree with your suggestion to do both -
> 1. Not send IMAGEERROR if there is exception from ImageFilter while 
> processing STATICIMAGE done
> 2. At the same time, as we are consuming the exception, show the 
> exception stacktrace.
>
> With this fix the functionality of com.jhlabs.image.WaterFilter is 
> restored. It does not return blank images anymore.
> Also, as additional information, exception details are shown as a 
> stacktrace.
>
> Please review the updated webrev :
> http://cr.openjdk.java.net/~aghaisas/8139192/webrev.02/
>
> Regards,
> Ajit
>
>
> -Original Message-
> From: Jim Graham
> Sent: Thursday, May 26, 2016 4:57 AM
> To: Sergey Bylokhov; Ajit Ghaisas; 2d-dev; Philip Race
> Subject: Re: [OpenJDK 2D-Dev] Fix for JDK-8139192 : Custom 
> ImageFilters return blank images in Java 8(.45) while working in 7
>
> Their complaint is that the resulting image is empty - most likely because 
> the error gets passed through and clears the image buffer.  It appears that 
> the sequence of events is:
>
> We send SINGLEFRAMEDONE.
> - the image is displayable
> We send STATICIMAGEDONE.
> - filter throws NPE
> - image is probably still displayable
> We send ERROR
> - if it gets passed through to the toolkit image consumer then we'll 
> clear the image buffer
> - image is no longer displayable
>
> It's hard to say for sure without having an instance of their filter in-house 
> for testing, but previously we weren't sending the STATICDONE notice and the 
> program was working just fine.  Since the NPE comes from their code when we 
> send it, it shouldn't have affected any down-stream consumers, so we should 
> be OK with just continuing on and the image should still be displayable just 
> as if we hadn't sent the STATICDONE notice in the first place.
>
> Now, *during debugging*, they were thwarted by the fact that we ate the 
> exception, true, but the original issue is that the image wasn't displayable 
> at all.  We might want to do both:
>
> - not send ERROR for STATICDONE when we used to not send STATICDONE at 
> all
> - show the exception so that someone realizes that there is a problem, even 
> though we've made it not hurt their functionality.
>
> Does that make sense?
>
>   ...jim
>
> On 5/25/2016 1:36 PM, Sergey Bylokhov wrote:
>> On 25.05.16 23:33, Jim Graham wrote:
>>> How about option 3 -
>>>
>>> NPE before imageComplete sends an ERROR as it does now.
>>>
>>> NPE during imageComplete is ignored, both for backwards 
>>> compatibility and because it is more of a "hint" than an operation 
>>> that requires action from the consumer.
>>
>> I guess the case is that when we ignore this NPE(w/o any 
>> notification) the users complains that the bug is in jdk.
>>
>>>
>>> ...jim
>>>
>>> On 5/25/2016 1:31 AM, Ajit Ghaisas wrote:
>>>> Hi
>>>>
>>>>
>>>>
>>>> Bug :
>>>>
>>>> https://bugs.openjdk.java.net/browse/JDK-8139192
>>>>
>>>> 

Re: [OpenJDK 2D-Dev] Fix for JDK-8139192 : Custom ImageFilters return blank images in Java 8(.45) while working in 7

[Ajit Ghaisas]

Hi Jim,

Thanks for in-depth analysis and detailed review.
I appreciate your concern for filtered image being blank in Java 8, while 
it used to work in Java 7.

Yes. I totally agree with your suggestion to do both -
1. Not send IMAGEERROR if there is exception from ImageFilter while 
processing STATICIMAGE done
2. At the same time, as we are consuming the exception, show the exception 
stacktrace. 

With this fix the functionality of com.jhlabs.image.WaterFilter is 
restored. It does not return blank images anymore. 
Also, as additional information, exception details are shown as a 
stacktrace.

Please review the updated webrev : 
http://cr.openjdk.java.net/~aghaisas/8139192/webrev.02/

Regards,
Ajit


-Original Message-
From: Jim Graham 
Sent: Thursday, May 26, 2016 4:57 AM
To: Sergey Bylokhov; Ajit Ghaisas; 2d-dev; Philip Race
Subject: Re: [OpenJDK 2D-Dev] Fix for JDK-8139192 : Custom ImageFilters return 
blank images in Java 8(.45) while working in 7

Their complaint is that the resulting image is empty - most likely because the 
error gets passed through and clears the image buffer.  It appears that the 
sequence of events is:

We send SINGLEFRAMEDONE.
- the image is displayable
We send STATICIMAGEDONE.
- filter throws NPE
- image is probably still displayable
We send ERROR
- if it gets passed through to the toolkit image consumer then we'll clear the 
image buffer
- image is no longer displayable

It's hard to say for sure without having an instance of their filter in-house 
for testing, but previously we weren't sending the STATICDONE notice and the 
program was working just fine.  Since the NPE comes from their code when we 
send it, it shouldn't have affected any down-stream consumers, so we should be 
OK with just continuing on and the image should still be displayable just as if 
we hadn't sent the STATICDONE notice in the first place.

Now, *during debugging*, they were thwarted by the fact that we ate the 
exception, true, but the original issue is that the image wasn't displayable at 
all.  We might want to do both:

- not send ERROR for STATICDONE when we used to not send STATICDONE at all
- show the exception so that someone realizes that there is a problem, even 
though we've made it not hurt their functionality.

Does that make sense?

...jim

On 5/25/2016 1:36 PM, Sergey Bylokhov wrote:
> On 25.05.16 23:33, Jim Graham wrote:
>> How about option 3 -
>>
>> NPE before imageComplete sends an ERROR as it does now.
>>
>> NPE during imageComplete is ignored, both for backwards compatibility 
>> and because it is more of a "hint" than an operation that requires 
>> action from the consumer.
>
> I guess the case is that when we ignore this NPE(w/o any notification) 
> the users complains that the bug is in jdk.
>
>>
>> ...jim
>>
>> On 5/25/2016 1:31 AM, Ajit Ghaisas wrote:
>>> Hi
>>>
>>>
>>>
>>> Bug :
>>>
>>> https://bugs.openjdk.java.net/browse/JDK-8139192
>>>
>>> Custom ImageFilters return blank images in Java 8(.45) while working 
>>> in 7
>>>
>>>
>>>
>>>
>>>
>>> Root cause :
>>>
>>> private method produce() in OffScreenImageSource.java consumes a 
>>> NullPointerException that originates from a custom ImageConsumer (a 
>>> 3^rd party image library class - com.jhlabs.image.WaterFilter)
>>>
>>>
>>>
>>>
>>>
>>> Analysis:
>>>
>>> 1. How the behavior changed between JDK7 and JK8 :
>>>
>>> A call to imageComplete(ImageConsumer.SINGLEFRAMEDONE) was added in 
>>> addition to imageComplete(ImageConsumer. STATICIMAGEDONE)  as a fix 
>>> for JDK-7143612.
>>>
>>>
>>>
>>> 2. What debugging revealed:
>>>
>>> A NullPointerException is being thrown from the library during the 
>>> call to imageComplete(ImageConsumer.STATICIMAGEDONE)
>>>
>>>
>>>
>>> 3. It looks like the fix of JDK-7143612 is valid and successive 
>>> calls to
>>> imageComplete() are allowed.
>>>
>>>
>>>
>>> 4. The 3rd party library behavior appears incorrect (if it can not 
>>> handle subsequent calls to imageComplete(), it should de-register 
>>> itself).
>>>
>>> The 3rd-party library code should be fixed.
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>> Possible modifications in JDK  :
>>>
>>>
>>>
>>> Currently, OffScreenImageSource::produce() consumes the 
>>> NullPo

[OpenJDK 2D-Dev] Fix for JDK-8139192 : Custom ImageFilters return blank images in Java 8(.45) while working in 7

[Ajit Ghaisas]

Hi

 

Bug :

https://bugs.openjdk.java.net/browse/JDK-8139192

Custom ImageFilters return blank images in Java 8(.45) while working in 7

 

 

Root cause : 

private method produce() in OffScreenImageSource.java consumes a 
NullPointerException that originates from a custom ImageConsumer (a 3rd party 
image library class - com.jhlabs.image.WaterFilter)

 

 

Analysis:

1. How the behavior changed between JDK7 and JK8 :

A call to imageComplete(ImageConsumer.SINGLEFRAMEDONE) was added in addition to 
imageComplete(ImageConsumer. STATICIMAGEDONE)  as a fix for JDK-7143612.

 

2. What debugging revealed:

A NullPointerException is being thrown from the library during the call to 
imageComplete(ImageConsumer.STATICIMAGEDONE)

 

3. It looks like the fix of JDK-7143612 is valid and successive calls to 
imageComplete() are allowed.

 

4. The 3rd party library behavior appears incorrect (if it can not handle 
subsequent calls to imageComplete(), it should de-register itself). 

The 3rd-party library code should be fixed.

 

 

 

Possible modifications in JDK  :

 

Currently, OffScreenImageSource::produce() consumes the NullPointerException - 
this is incorrect and results in silent failure of this particular image filter.

We need to let the image filter library know that exception has occurred in its 
code and not in JDK. We have two options -

 

Option 1 : Rethrow the NullPointerException   --- It is the clearest way to let 
3rd party library know that their code is erroneous with latest JDK. This will 
change the 3rd party image filter behavior that generates blank image.

Option 2 : Add a stack trace where the exception is being consumed --- Adding 
stack trace provides more information regarding failure of 3rd party image 
filter with retaining the current behavior that generates blank image.

 

I have implemented both the options:

Option 1: http://cr.openjdk.java.net/~aghaisas/8139192/webrev.00/

Option 2:  http://cr.openjdk.java.net/~aghaisas/8139192/webrev.01/

 

Request you to review both the webrevs and provide your preference.

I will add a test to the selected webrev.

 

Regards,

Ajit

 

 

 

 

Re: [OpenJDK 2D-Dev] Fix for JDK-8074829 : Resolve disabled warnings for libawt_headless

[Ajit Ghaisas]

Hi,

 I tried excluding files under directory : 
jdk/src/java.desktop/share/native/common/java2d/opengl from libawt_headless.
 It resulted in compilation errors - as the headers in this directory (and 
under sub-directory J2D_GL) are used in other places.
 
 To Phil's question on - why I mentioned only OGLBlitLoops.c file? - this 
is the file where warning is reported and build stopped.
 
 Hence, I propose not to remove the suppression of warning 
E_EMPTY_TRANSLATION_UNIT  in make file for Solaris. 
 There is no change in original webrev : 
http://cr.openjdk.java.net/~aghaisas/8074829/webrev.00/

 Based on Erik's suggestion, I have built it for arm and arm64 with no 
errors.

Regards,
Ajit

-Original Message-
From: Phil Race 
Sent: Friday, April 22, 2016 1:43 AM
To: Ajit Ghaisas
Cc: Sergey Bylokhov; 2d-dev; build-...@openjdk.java.net
Subject: Re:  Fix for JDK-8074829 : Resolve disabled warnings for 
libawt_headless

 >  Another solution is to exclude this file from HEADLESS compilation.
 >  I am not sure how to achieve it. Any suggestion?

I suppose that is possible and I expect we can do that See in the make file, 
where I think you just need to add entries to
 LIBAWT_HEADLESS_EXCLUDES := medialib

although I have not tried it.
Hmm .. I wonder why medialib needs to be explicitly excluded from headless ? .. 
but that is for another day.

I have another question: why do you mention only OGLBlitLoops.c ?
I've flicked through a number of the C files in the same location and all look 
to have the same issue.

-phil.

On 04/21/2016 06:33 AM, Ajit Ghaisas wrote:
>>> On 04/20/2016 12:27 PM, Sergey Bylokhov wrote:
>>> 2d-dev added.
>> In fact all these are 2D. No AWT warnings here.
>>> I am not sure but why "declaration in the code" is a bad thing and 
>>> we should fix it?
>>> - DISABLED_WARNINGS_solstudio := E_DECLARATION_IN_CODE
>>>
>>> I cannot find the documentation in solaris studio for this warning.
>> I don't mind fixing it if it is still an issue but does the current compiler 
>> actually complain about it ?
>> The SS11 -> SS12 upgrade might have got a more modern C compiler ..
> [Ajit ] Yes. The Solaris compiler still complains about this declaration in 
> code. Hence, I have fixed the reported warnings after removing the 
> suppression from makefile.
>
>>> On 20.04.16 11:57, Ajit Ghaisas wrote:
>>>> Hi,
>>>>
>>>>   Bug : https://bugs.openjdk.java.net/browse/JDK-8074829
>>>>   This bug is to remove warning suppressions from makefile and 
>>>> fix the warnings for libawt_headless library.
>>>>
>>>>   I have removed following warning suppressions & fixed the 
>>>> warnings for libawt_headless library.
>>>>   DISABLED_WARNINGS_gcc := maybe-uninitialized 
>>>> int-to-pointer-cast
>> What made that one go away ??
> [Ajit]  :  I fixed warnings reported for 'maybe-uninitialized' and 
> 'E_DECLARATION_IN_CODE' warning types in two .c files in webrev.
> There was no warning after removal of 'int-to-pointer-cast' suppression from 
> makefile. No code change was made for this type of warning.
>
>
>>>>   DISABLED_WARNINGS_solstudio := E_DECLARATION_IN_CODE
>>>>
>>>>   Warning suppression that cannot be removed :
>>>>   DISABLED_WARNINGS_solstudio := E_EMPTY_TRANSLATION_UNIT
>>>>   This is due to the fact that - 
>>>> jdk/src/java.desktop/share/native/common/java2d/opengl/OGLBlitLoops
>>>> .c file becomes empty file in case of HEADLESS mode compilation.
>> Sigh .. there ought to be "informational" warnings as well as "risky 
>> practice" warnings and this should be in the former category.
>> You could move something like the jni.h and jlong.h imports outside to see 
>> if that shuts it up.
>> Not saying that is what we want to do but it would be interesting to check.
> [Ajit] : Nope. Moving jni.h or jlong.h inclusions outside #ifndef HEADLESS 
> did not help. We still get E_EMPTY_TRANSLATION_UNIT warning.
> To get rid of this warning, there are suggestions to make a typedef - and not 
> use it anywhere - but, I would rather keep the suppression in makefile than 
> defining a typedef without actual usage.
> Another solution is to exclude this file from HEADLESS compilation. I am not 
> sure how to achieve it. Any suggestion?
>
>
>> -phil.
>>>>   Request you to review following webrev :
>>>>   http://cr.openjdk.java.net/~aghaisas/8074829/webrev.00/
>>>>
>>>> Regards,
>>>> Ajit

Re: [OpenJDK 2D-Dev] Fix for JDK-8074829 : Resolve disabled warnings for libawt_headless

[Ajit Ghaisas]

>>On 04/20/2016 12:27 PM, Sergey Bylokhov wrote:
>> 2d-dev added.

>In fact all these are 2D. No AWT warnings here.

>>
>> I am not sure but why "declaration in the code" is a bad thing and we 
>> should fix it?
>> - DISABLED_WARNINGS_solstudio := E_DECLARATION_IN_CODE
>>
>> I cannot find the documentation in solaris studio for this warning.

>I don't mind fixing it if it is still an issue but does the current compiler 
>actually complain about it ?
>The SS11 -> SS12 upgrade might have got a more modern C compiler ..

[Ajit ] Yes. The Solaris compiler still complains about this declaration in 
code. Hence, I have fixed the reported warnings after removing the suppression 
from makefile.

>>
>> On 20.04.16 11:57, Ajit Ghaisas wrote:
>>> Hi,
>>>
>>>  Bug : https://bugs.openjdk.java.net/browse/JDK-8074829
>>>  This bug is to remove warning suppressions from makefile and fix 
>>> the warnings for libawt_headless library.
>>>
>>>  I have removed following warning suppressions & fixed the 
>>> warnings for libawt_headless library.
>>>  DISABLED_WARNINGS_gcc := maybe-uninitialized int-to-pointer-cast

>What made that one go away ??

[Ajit]  :  I fixed warnings reported for 'maybe-uninitialized' and 
'E_DECLARATION_IN_CODE' warning types in two .c files in webrev.
There was no warning after removal of 'int-to-pointer-cast' suppression from 
makefile. No code change was made for this type of warning.


>>>  DISABLED_WARNINGS_solstudio := E_DECLARATION_IN_CODE
>>>
>>>  Warning suppression that cannot be removed :
>>>  DISABLED_WARNINGS_solstudio := E_EMPTY_TRANSLATION_UNIT
>>>  This is due to the fact that -
>>> jdk/src/java.desktop/share/native/common/java2d/opengl/OGLBlitLoops.c
>>> file becomes empty file in case of HEADLESS mode compilation.

>Sigh .. there ought to be "informational" warnings as well as "risky practice" 
>warnings and this should be in the former category.
>You could move something like the jni.h and jlong.h imports outside to see if 
>that shuts it up.
>Not saying that is what we want to do but it would be interesting to check.

[Ajit] : Nope. Moving jni.h or jlong.h inclusions outside #ifndef HEADLESS did 
not help. We still get E_EMPTY_TRANSLATION_UNIT warning.
To get rid of this warning, there are suggestions to make a typedef - and not 
use it anywhere - but, I would rather keep the suppression in makefile than 
defining a typedef without actual usage.
Another solution is to exclude this file from HEADLESS compilation. I am not 
sure how to achieve it. Any suggestion?


>-phil.
>>
>>>  Request you to review following webrev :
>>>  http://cr.openjdk.java.net/~aghaisas/8074829/webrev.00/
>>>
>>> Regards,
>>> Ajit

Re: [OpenJDK 2D-Dev] [9] Review-request for 6353518: Creation of a WritableRaster with a custom DataBuffer causes erroneous Exception

[Ajit Ghaisas]

Hi,

 Thanks Sergey and Phil for answering some of Jim's concerns regarding this 
fix.

 I have answered rest of them below: 


Raster.java, line 742: This branch of the switch statement instantiates a SWR 
which doesn't have a specific buffer type.  It could be replaced with just 
"break;" and let the code after the switch create the raster (it can't be 
deleted from the switch statement because the default would throw an exception, 
but it could just "break;" on that dataType).  At a minimum, though, it didn't 
need the instanceof or cast that was added since it will just fall through to 
exactly the same code anyway.

>>> [Ajit] : Good catch. As suggested, I have replaced this with just a 'break' 
>>> statement. It's much cleaner now. Thanks.


Raster.java, line 894: Why was the test for dataType removed from this if 
statement?

>>> [Ajit] : It was an extra check. It was removed after Phil's review comment. 
The check for 'dataType' is removed as it is populated from 'dataBuffer'  (and 
not from SampleModel as in other methods). 
The newly added check (dataBuffer instanceof DataBufferByte) that uses 
'dataBuffer' is sufficient to decide whether to create BytePackedRaster or 
SunWritableRaster.


Raster.java: line 980: Something to file as a potential bug for future work.  
The test for getSampleSize(0) in this method is much more permissive than the 
test for bitsPerPixel in createPackedRaster just above this method.  Both 
determine whether to return a BytePackedRaster so they should probably agree.

>>> [Ajit] : I agree.  The code here can be made in line with test for 
>>> bitsPerPixel as in createPackedRaster method. I will create a bug for this.


Raster.java, line 986: Something to file as a potential bug for future work 
since the fix would have to be verified that it doesn't disrupt the other parts 
of this method, but... The set of if statements in this method never checked 
for a BandedSampleModel to try to create a BandedRaster as in 
createBandedRaster.  On the other hand, no developer has complained so far 
(that I know of, but maybe we have an old bug filed on it that I'm not aware 
of).

>>>[Ajit] :  Without my fix, it was easier to get RasterFormatException as 
>>>explained in Bug Description. This fix enhances the code by making type 
>>>specific Raster class constructors type-safe.
Now, the bigger question is: Whether methods in Raster.java support all 
possible combinations of Raster creations?
What I can say is - this fix does not break anything than what is already 
present in Raster.java.
If there is specific user complaint or any existing bug - we can look into it 
separately.

Raster.java: the same comments for createRaster above apply for 
createWritableRaster at line 1033.


The following 2 comments are identical for all of the raster types {
 ByteBandedRaster.java, line 76: This code will throw a ClassCastException
 now instead of the RasterFormatException that used to be thrown.
 Do we care?  It would be easy enough to add a test to just this
 method (and the similar versions in other files).
>>> [Ajit] :  Since we have decided to make the type specific Raster class 
>>> constructors type safe, yes, it is possible to get ClassCastException. But, 
>>> as the usage of these classes is guarded now with "instanceof" checks, it 
>>> would be rare. 


 ByteBandedRaster.java, line 668: Extra credit: This cast could be avoided
 if we make SunWritableRaster employ generics for a strongly typed 
DataBuffer.

>>> [Ajit] : the casted member ' dataBuffer' on this line is a member of Raster 
>>> class.
The class hierarchy of ByteBandedRaster is ( added on single line for 
convenience) 
    ByteBandedRaster extends SunWritableRaster extends WritableRaster 
extends Raster.

I am not quite clear on how we can avoid cast on this line. Can you please 
elaborate?

}


ShortInterleavedRaster - I think the import of DataBuffer can be eliminated now?

>>> [Ajit] Good catch - I have eliminated the extra import now. Thanks.


Here is the updated webrev:
http://cr.openjdk.java.net/~arapte/ajit/6353518/webrev.05/

Request you to review it.

Thanks,
Ajit


On 3/28/16 3:18 AM, Ajit Ghaisas wrote:
> Hi,
>
>  Thanks Jim for thorough review.
>
>  Here is the updated webrev :
>  http://cr.openjdk.java.net/~arapte/ajit/6353518/webrev.04/
>
>  In this update :
>  1)  I have corrected missing space after casting
>  2)  I have modified code to adapt suggested indentation for blocks 
> having conditionals & method declaration split on multiple lines.
>
>  Please note that, I have done indentation change only if it is related 
> to

Re: [OpenJDK 2D-Dev] [9] Review-request for 6353518: Creation of a WritableRaster with a custom DataBuffer causes erroneous Exception

[Ajit Ghaisas]

Hi,

Thanks Jim for thorough review.

Here is the updated webrev :
http://cr.openjdk.java.net/~arapte/ajit/6353518/webrev.04/

In this update : 
1)  I have corrected missing space after casting
2)  I have modified code to adapt suggested indentation for blocks having 
conditionals & method declaration split on multiple lines.

Please note that, I have done indentation change only if it is related to 
the code changes done as part of this fix. 
The files in this review already have indentation issues and fixing all of 
them will result in multiple changes masking the actual code changes that fixes 
the reported issue.

   Request you to review the updated webrev.

Regards,
Ajit

-Original Message-
From: Jim Graham 
Sent: Thursday, March 24, 2016 5:59 AM
To: Phil Race
Cc: 2d-dev
Subject: Re: [OpenJDK 2D-Dev] [9] Review-request for 6353518: Creation of a 
WritableRaster with a custom DataBuffer causes erroneous Exception

I should point out that this form I demonstrate below is only used when there 
are continuation lines on the prefix to the code block (conditionals, method 
declarations, loop controls, etc.), so:

 if (some simple condition) {
 ...
 }

and:

 if (some complex condition &&
 some additional condition)
 {
 ...
 }

but, not:

 if (some single line condition)
 {
 // blech
 ...
 }

The reason for this is that the standard indentation would recommend:

 if (some complex condition &&
 some additional condition) {
 // code block
 }
or
 void foomethod(int sx, int sy,
 int dx, int dy) {
 // code block
 }

which may be more compact, but the lack of a breaking line means you have to 
vary the indentation of the declarations/conditionals and as a result they 
don't line up nicely which makes them harder to scan if they are all related 
(and frequently in graphics code you end up with a series of very similar 
arguments or conditionals that are more readable if they line up nicely), and 
the only indication of when the multiple lines of continued 
declaration/conditionals end and the body of the method begins is the number of 
spaces - noting that frequently the indentation on lines in practice is just 
wrong so this form makes it hard to distinguish between "that line is a 
continuation line" and "someone indented that line wrong"...

...jim

On 3/23/16 5:14 PM, Jim Graham wrote:
> For the record, in many places in the 2D code we also adopt a slight
> extension of the indentation rules such that the below might be written as:
>
>  public ByteBandedRaster(SampleModel sampleModel,
>  DataBufferByte dataBuffer,
>  Point origin)
>  {
>  // body lines of the method...
>  }
>
> with the open curly brace on the following line so that it more visually
> points out the beginning of the body block of the method and it's easy
> to find the start/end of the block by sighting down the left margin. The
> official policy is for the "{" to be at the end of the previous line
> after "Point origin) {" and as more new engineers work on the code and
> follow the official rules, the above form becomes less common.  (Sad
> face since I particularly like the above form...)
>
>  ...jim
>
> On 3/22/16 4:10 PM, Phil Race wrote:
>> Ajit,
>>
>> There is also some odd indentation in ByteBandedRaster.java which is not
>> yours but
>>
>>   98 public ByteBandedRaster(SampleModel sampleModel,
>>99DataBufferByte dataBuffer,
>>   100Point origin) {
>>
>>
>> This appears to be the result of someone using tabs a long time ago.
>>
>> Since you are touching the method signature lines anyway may be
>> better fixed so we have these aligned
>>
>>   public ByteBandedRaster(SampleModel sampleModel,
>>   DataBufferByte dataBuffer,
>>   Point origin) {
>>
>> [not sure if that will make it through mail as intended].
>>
>> Here in Raster.java, the first condition now seems to be redundant ..
>> 890 if (dataType == DataBuffer.TYPE_BYTE &&
>>   891 dataBuffer instanceof DataBufferByte &&
>>
>>
>>
>> -phil.
>>
>>
>> On 03/22/2016 03:28 PM, Jim Graham wrote:
>>> Hi Ajit,
>>>
>>> Most of your if statements are spaced wrong.  There should be a space
>>> between "if" and the parentheses.  I'll review more later, but I
>>> noticed that issue i

Re: [OpenJDK 2D-Dev] [9] Review-request for 6353518: Creation of a WritableRaster with a custom DataBuffer causes erroneous Exception

[Ajit Ghaisas]

Thanks Jim and Phil.
Yes. It was a mistake to omit spaces in 'if' conditions. I have corrected the 
spacing issues for my changes now.

I have corrected the indentation of the lines similar to those mentioned by 
Phil.
I have also removed the redundant first condition check on line 890 in 
Raster.java mentioned as a review comment.

Please review updated webrev :
http://cr.openjdk.java.net/~arapte/ajit/6353518/webrev.03/

Regards,
Ajit

-Original Message-
From: Phil Race 
Sent: Wednesday, March 23, 2016 4:40 AM
To: Jim Graham
Cc: Ajit Ghaisas; Sergey Bylokhov; 2d-dev
Subject: Re: [OpenJDK 2D-Dev] [9] Review-request for 6353518: Creation of a 
WritableRaster with a custom DataBuffer causes erroneous Exception

Ajit,

There is also some odd indentation in ByteBandedRaster.java which is not yours 
but

  98 public ByteBandedRaster(SampleModel sampleModel,
   99DataBufferByte dataBuffer,
  100Point origin) {


This appears to be the result of someone using tabs a long time ago.

Since you are touching the method signature lines anyway may be better fixed so 
we have these aligned

  public ByteBandedRaster(SampleModel sampleModel,
  DataBufferByte dataBuffer,
  Point origin) {

[not sure if that will make it through mail as intended].

Here in Raster.java, the first condition now seems to be redundant ..
890 if (dataType == DataBuffer.TYPE_BYTE &&
  891 dataBuffer instanceof DataBufferByte &&



-phil.


On 03/22/2016 03:28 PM, Jim Graham wrote:
> Hi Ajit,
>
> Most of your if statements are spaced wrong.  There should be a space 
> between "if" and the parentheses.  I'll review more later, but I 
> noticed that issue in the first couple of files I looked at...
>
> ...jim
>
> On 3/15/16 7:05 AM, Ajit Ghaisas wrote:
>> Hi,
>>
>> Thanks Sergey and Jim for suggestions.
>>
>>  I have made the data specific Raster constructors type safe now.  
>> Also, I have modified all Raster creations in Raster.java to support 
>> custom DataBuffer classes.
>>
>>  Please review the changes present in updated webrev :
>>  http://cr.openjdk.java.net/~arapte/ajit/6353518/webrev.02/
>>
>> Regards,
>> Ajit
>>
>> -Original Message-
>> From: Jim Graham
>> Sent: Friday, March 11, 2016 2:40 AM
>> To: Sergey Bylokhov; Ajit Ghaisas; 2d-dev
>> Subject: Re: [OpenJDK 2D-Dev] [9] Review-request for 6353518: 
>> Creation of a WritableRaster with a custom DataBuffer causes 
>> erroneous Exception
>>
>> Yes, those constructors should be type-safe.  Perhaps that was done 
>> to save code in the caller, but that is a small price to pay to avoid 
>> errors in the future, and it makes sure there is a backup plan for 
>> different kinds of buffers...
>>
>> ...jim
>>
>> On 3/10/16 4:48 AM, Sergey Bylokhov wrote:
>>> Hi, Ajit.
>>> What about other cases in Raster.java, where the DataBuffer is 
>>> passed as a parameter? Probably we can simplify the code and find 
>>> all such issues if we changes the ByteInterleavedRaster/etc. For example:
>>>
>>> ByteInterleavedRaster(SampleModel sampleModel,DataBuffer
>>> dataBuffer,...) to
>>>
>>> ByteInterleavedRaster(SampleModel sampleModel,DataBufferByte
>>> dataBuffer,...)
>>>
>>> Because we throw an exception in such cases anyway:
>>>
>>> if (!(dataBuffer instanceof DataBufferByte)) {
>>>   throw new RasterFormatException("ByteInterleavedRasters must 
>>> have "
>>> + "byte DataBuffers");
>>> }
>>>
>>> And the compiler will help us, what everybody think about it?
>>>
>>> On 09.03.16 17:38, Ajit Ghaisas wrote:
>>>> Hi,
>>>>
>>>>  Modified the test and added check for 
>>>> MultiPixelPackedSampleModel condition.
>>>>
>>>>  Please review updated webrev :
>>>> http://cr.openjdk.java.net/~arapte/ajit/6353518/webrev.01/
>>>>
>>>> Regards,
>>>> Ajit
>>>>
>>>> -Original Message-
>>>> From: Sergey Bylokhov
>>>> Sent: Wednesday, March 09, 2016 4:06 PM
>>>> To: Ajit Ghaisas; awt-...@openjdk.java.net; Semyon Sadetsky; 
>>>> Ambarish Rapte; 2d-dev
>>>> Subject: Re: [9] Review-request for 6353518: Creation of a 
>>>> WritableRaster with a custom DataBuffer causes erroneous Exception
>>>>
>>>> Changes for 2d 

Re: [OpenJDK 2D-Dev] Review Request for JDK-6185114: Undefined Exception in SampleModel, method createCompatibleSampleModel

[Ajit Ghaisas]

Hi,

 

I have corrected the typo and modified the failure message string. 

 

Please review the updated webrev.

http://cr.openjdk.java.net/~arapte/ajit/6185114/webrev.01/

 

Regards,

Ajit   

 

From: prasanta sadhukhan 
Sent: Wednesday, March 16, 2016 4:25 PM
To: Ajit Ghaisas; 2d-dev; Philip Race
Subject: Re: Review Request for JDK-6185114: Undefined Exception in 
SampleModel, method createCompatibleSampleModel

 

Fix Looks ok. Test failure message does not say what it is failing for. Maybe 
you can mention "Expected IAE count <>, got IAE count <>" . lune 52 has a typo 
heiht

Regards
Prasanta

On 3/16/2016 3:57 PM, Ajit Ghaisas wrote:

Hi,

 

Bug : https://bugs.openjdk.java.net/browse/JDK-6185114

 

Issue : Undefined Exception in SampleModel, method createCompatibleSampleModel

 

Root Cause : 

The SampleModel constructor documentation says -  it throws the 
IllegalArgumentException if the product of width and height parameters is 
greater than Integer.MAX_VALUE.

The condition for this check is incorrectly checks for "greater than equal to" 
where it should be only "greater than".

 

Fix :

Corrected the erroneous condition in below webrev and added a test.

HYPERLINK 
"http://cr.openjdk.java.net/%7Earapte/ajit/6185114/webrev.00/"http://cr.openjdk.java.net/~arapte/ajit/6185114/webrev.00/

 

Regards,

Ajit