Re: RFR: 8260931: Implement JEP 382: New macOS Rendering Pipeline [v3]

2021-02-09 Thread Jayathirth D V
On Mon, 8 Feb 2021 18:05:02 GMT, Gerard Ziemski  wrote:

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

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

-

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


Re: RFR: 8260931: Implement JEP 382: New macOS Rendering Pipeline [v3]

2021-02-09 Thread 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: RFR: 8260931: Implement JEP 382: New macOS Rendering Pipeline [v4]

2021-02-09 Thread 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=2403=03
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=2403=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