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

[Ajit Ghaisas]

On Fri, 8 Jan 2021 19:26:14 GMT, Phil Race <prr at openjdk.org> wrote:

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

The whole idea of having this method was to simplify the constructor - Creating the GraphicsConfig is a single call - the checks and fallback is the main thing done in this method. I see your point of hiding "side-effect". We can move the code itself to the constructor in that case. I will move it.

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

Opps. Anyway, removing this method due to the other comment.

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

I will rename this method as usingMetalPipeline() - to indicate whether the device is using metal pipeline.
This PR is about implementing the fallback. Once that is in place, we can handle the runtime pipeline selection pattern correction under JDK-8226384.

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

True. A review always helps. Refined now.

> 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() ?

Yes. They are  only called from 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

It is only called from doPrivileged block in initJavaFlags().

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

MacOSFlags is mere indication of pipeline requested and framework availability. I am using enabled/disabled naming to be consistent with WindowsFlags class that uses this convention.

Here is the detailed explanation : 
Rendering pipeline initialization can fail due to 2 checks-
Check-1) Framework not available on the system
Check-2) GraphcisConfiguration creation fails - involves - creation of context etc.

A render pipeline initialization is deemed to be successful only if - User requests for it (either explicitly or implicitly due to default for the platform) AND both above checks pass.


For OpenGL,
Check-1 is done by simply loading the library - hence, it can be done in MacOSFlags class
Check-2 is done in CGraphicsDevice class

For Metal,
Check-1 involves using system profiler to check framework availability
Check-2 is done in CGraphicsDevice class - involves creating metal library object AND other initialization checks

Metal library object creation needs DisplayID hence, it cannot be moved to Check-1.

I have introduced boolean flags metalPipelineEnabled & oglPipelineEnabled in CGraphicsDevice class as fallback pipeline selection can happen after Check-2. These flags correctly indicate which pipeline is enabled.

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

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