RFR: 8258754: Gracefully fallback to the OpenGL rendering pipeline if Metal rendering pipeline initialization fails [v5]

[Phil Race]

On Fri, 8 Jan 2021 19:13:19 GMT, Ajit Ghaisas <aghaisas at openjdk.org> wrote:

>> This implements fallback to the OpenGL rendering pipeline if Metal rendering pipeline initialization fails.
>> I have tested this fallback on 10.15.4 by stubbing code. We need real testing on a system where Metal is not available.
>
> Ajit Ghaisas has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Cover all combinations of OGL and Metal pipelines

src/java.desktop/macosx/classes/sun/awt/CGraphicsDevice.java line 325:

> 323:     // if it fails, it tries to create MTLLGraphicsConfig as a fallback
> 324:     private void creatCGLGraphicsConfig(final int displayID) {
> 325:         this.config = CGLGraphicsConfig.getConfig(this);

creat ? should be create.

src/java.desktop/macosx/classes/sun/awt/CGraphicsDevice.java line 313:

> 311:                 System.out.println("Metal rendering pipeline initialization failed. Using OpenGL rendering pipeline.");
> 312:             }
> 313: 

Not sure I like this "side-effect" approach.
I think this method should return false if it fails and that get detected by the calller which then
decides what should happen next.

src/java.desktop/macosx/classes/sun/awt/CGraphicsDevice.java line 72:

> 70:         this.displayID = displayID;
> 71: 
> 72:         if (MacOSFlags.isMetalEnabled()) {

I guess this flag really means "requested". It does not mean "supported and enabled".
And we also have a boolean called isMetalEnabled which is closer to the truth.

src/java.desktop/macosx/classes/sun/java2d/MacosxSurfaceManagerFactory.java line 54:

> 52:                                                         Object context)
> 53:     {
> 54:         return CGraphicsDevice.useMetalPipeline() ? new MTLVolatileSurfaceManager(vImg, context) :

similar to my other naming comments this is more "using" than "use"  - or enabled again.
I'd like consistent naming where it makes sense.
But also this looks like another case where I'd prefer to see us have a pattern that retrieves  the installed pipeline.
So this entire method body is something like 
getInstalledPipeline().createVolatileSurfaceManager(vImg, context);

src/java.desktop/macosx/classes/sun/java2d/macos/MacOSFlags.java line 57:

> 55:     private static PropertyState getBooleanProp(String p, PropertyState defaultVal) {
> 56:         String propString = System.getProperty(p);
> 57:         PropertyState returnVal = defaultVal;

I guess all these get/setProperty calls are used only from that doPrivileged block in initJavaFlags() ?

src/java.desktop/macosx/classes/sun/java2d/macos/MacOSFlags.java line 78:

> 76: 
> 77:     private static boolean isBooleanPropTrueVerbose(String p) {
> 78:         String propString = System.getProperty(p);

doPrivileged

src/java.desktop/macosx/classes/sun/java2d/macos/MacOSFlags.java line 105:

> 103:                             oglEnabled = false;
> 104:                             metalEnabled = true;
> 105:                         } else if (oglState == PropertyState.ENABLED || oglState == PropertyState.UNSPECIFIED) {

There are only 3 states aren't there ? And you don't have another branch, so why do we need
"if (oglState == PropertyState.ENABLED || oglState == PropertyState.UNSPECIFIED"
One of those has to be true if we reach here, doesn't it ?

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

PR: https://git.openjdk.java.net/lanai/pull/147