On Thu, 4 Feb 2021 10:35:02 GMT, Ajit Ghaisas <aghai...@openjdk.org> 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) I left a few additional comments. Overall this looks good to me. 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? src/java.desktop/macosx/native/libawt_lwawt/java2d/metal/MTLGraphicsConfig.m line 83: > 81: { > 82: FILE *f = popen("/usr/sbin/system_profiler SPDisplaysDataType", "r"); > 83: bool metalSupported = JNI_FALSE; This code is mixing types; it should be `jboolean metalSupported` src/java.desktop/macosx/native/libawt_lwawt/java2d/metal/MTLMaskFill.m line 26: > 24: */ > 25: > 26: #ifndef HEADLESS I see a few occurrences of `#ifndef HEADLESS` in the metal pipeline. Is this needed? I don't see any of the other native macos files in Java2D (e.g., the OpenGL pipeline) doing the same. Will this file ever be compiled with HEADLESS being undefined? src/java.desktop/macosx/native/libawt_lwawt/java2d/metal/MTLPaints.h line 41: > 39: /** > 40: * The MTLPaint class represents paint mode (color, gradient, e.t.c.) > 41: * */ Minor: `* */` --> `*/`; also typo: `e.t.c.` should be `etc.` src/java.desktop/macosx/native/libawt_lwawt/java2d/metal/MTLSurfaceDataBase.h line 37: > 35: > 36: /** > 37: * The MTLSDOps structure describes a native OpenGL surface and contains > all Should that be "Metal" and not "OpenGL" ? src/java.desktop/macosx/native/libawt_lwawt/java2d/metal/MTLTextRenderer.m line 77: > 75: * be safe to use this one glyph cache for all screens in a multimon > 76: * environment, since the glyph cache texture is shared between all > contexts, > 77: * and (in theory) OpenGL drivers should be smart enough to manage that `Metal` drivers? src/java.desktop/macosx/native/libawt_lwawt/java2d/metal/MTLTexurePool.m line 29: > 27: #import "Trace.h" > 28: > 29: #define SCREEN_MEMORY_SIZE_4K (4096*2160*4) //~33,7 mb This means that a 4k display with a narrower aspect ratio wouldn't fit (assuming there ever were to be such a thing). What would happen if you encountered a screen that was, say, 4k * 2.5K? ------------- PR: https://git.openjdk.java.net/jdk/pull/2403
