Details

Reviewers

Clément Foucault (fclem)
Brecht Van Lommel (brecht)

Commits

rBS99de160340c7: macOS: fix viewport lagging, by using CAMetalLayer instead of NSOpenGLView
rBS1418eee5f5c8: macOS: increase minimum required version to 10.11
rBS62fe7e9a9d0e: GPU: support default framebuffer with ID not equal to 0
rBS2a5dc454f63a: Cleanup: minor comment cleanups in GHOST
rBS593eab66b5ca: Cleanup: remove use of deprecated macOS API
rBS01a5e087aa25: Cleanup: move CocoaOpenGLView into own header
rB99de160340c7: macOS: fix viewport lagging, by using CAMetalLayer instead of NSOpenGLView
rB1418eee5f5c8: macOS: increase minimum required version to 10.11
rB62fe7e9a9d0e: GPU: support default framebuffer with ID not equal to 0
rB2a5dc454f63a: Cleanup: minor comment cleanups in GHOST
rB01a5e087aa25: Cleanup: move CocoaOpenGLView into own header
rB593eab66b5ca: Cleanup: remove use of deprecated macOS API

Summary

This patch updates GHOST_ContextCGL and GHOST_WindowCocoa to present OpenGL contents via CAMetalLayer. This has been found to resolve the frame skipping issue described in T60043.

I think this patch may be rather incomplete. Specifically:

It assumes the system has a Metal-capable GPU. It will likely crash with "nil MTLDevice" on other systems. The old code should be preserved as a fall back if old systems need to be supported.
~~The virtual default framebuffer only has a non-multisample color buffer - no depth, stencil or MSAA. I haven't encountered an issue with this so far.~~ Handling of them in GHOST was removed in eda7e84aac661f19478b2e5fe94d8899c9782cc8.
It always uses the system default GPU, so it'll be slightly inefficient for external displays connected to eGPUs, but this saves us handling eGPU notifications.

I'd like a feedback on which of these should be addressed in this patch.

Diff Detail

Repository

rB Blender

Branch

patch-metal-present (branched from master)

Build Status

Buildable 3506
Build 3506: arc lint + arc unit

Event Timeline

Tomoaki Kawada (yvt) created this revision.Mar 30 2019, 8:24 AM

Harbormaster completed remote builds in B3258: Diff 14494.Mar 30 2019, 8:24 AM

Tomoaki Kawada (yvt) mentioned this in T60043: macOS viewport lagging.Mar 30 2019, 9:07 AM

Clément Foucault (fclem) added reviewers: Clément Foucault (fclem), Brecht Van Lommel (brecht).Mar 30 2019, 1:28 PM

I found two minor issues with this patch and I don't have a solution at this moment:

When working in a full-screen mode, the window contents sometimes revert to an old state. The problem seems to be triggered by macOS's notification banner and persists until the window is updated.
When working in a full-screen mode, the display brightness varies depending on whether the window is occluded by something (e.g., the system menubar). This does not occur when the screen is on the maximum brightness.
Screen tearing-like effect is observed while doing lasso select. Perhaps the window contents are presented before the lasso shape is fully rendered.

For the first two issues, I suspect that CAMetalLayer has some kind of exclusive full-screen mode which is automatically activated when the window is in a full-screen mode and not occluded by anything and I think there's a problem transitioning from and to this mode.

Merge remote-tracking branch 'origin/master' into patch-metal-present
Convert the indentation of Metal source code to spaces

Harbormaster completed remote builds in B3400: Diff 14866.Apr 23 2019, 5:13 AM

Revert accidental mix-up with D4580

Harbormaster completed remote builds in B3401: Diff 14867.Apr 23 2019, 5:18 AM

Overall the patch looks clean.

I cannot give any feedback on the Metal implementation and/nor test it because I don't own a mac.

So for my part it's mostly OK.

intern/ghost/GHOST_C-api.h
726	For API consistency (and be more future proof) I would prefer to have a GHOST_FramebufferHandle as type. Even if it is cast into uint after in the case of OpenGL.
intern/ghost/intern/GHOST_Window.h
255	Follow style and make a description.
source/blender/windowmanager/intern/wm_playanim.c
1290–1291	Code Style: Avoid nested function calls.
source/blender/windowmanager/intern/wm_window.c
731–732	Same here

Merge remote-tracking branch 'origin/master' into patch-metal-present
Merge remote-tracking branch 'origin/master' into patch-metal-present
Fix accidental breakage introduced in merge commits
Add documentation comments for getDefaultFramebuffer
Conform with the coding style: Avoid nested function calls

Harbormaster completed remote builds in B3506: Diff 15119.May 4 2019, 2:47 PM

Tomoaki Kawada (yvt) marked 3 inline comments as done.May 4 2019, 2:48 PM

Tomoaki Kawada (yvt) added inline comments.

intern/ghost/GHOST_C-api.h
726	Isn't `GHOST_*Handle` reserved for opaque pointers backed by actual C++ classes?

Tomoaki Kawada (yvt) edited the summary of this revision. (Show Details)May 4 2019, 2:52 PM

Tomoaki Kawada (yvt) edited the summary of this revision. (Show Details)

William Reynish (billreynish) awarded a token.May 4 2019, 3:06 PM

Eugene (cgeugene) awarded a token.May 9 2019, 10:28 PM

Rebase for latest master
Keep draw manager GPU flush since it's now used for all platforms
Bump minimum SDK version for building Blender itself, not just libraries

Harbormaster completed remote builds in B3729: Diff 15596.May 24 2019, 6:56 PM

Sorry this took so long. I'd like try to commit this next week.

We still need to support running Blender without this Metal layer. Metal seems to require AMD GCN, and so far Blender has still been running on older AMD GPUs. If you want to update it to make that work go ahead, otherwise I can work on it.

The code seems generally fine.

Error handling is not ideal, abort() only works with debug builds and otherwise it will just crash on null pointers. Slightly better would be to exit(1) I think, even better would be to show a dialog with the error and then exit.

Blitting seems to happen using a shader, I wonder if there isn't a more efficient way to do using something similar to glBlitNamedFramebuffer()?

Should GPU_offscreen_draw_to_screen() use GPU_framebuffer_default() instead of 0 as well?

Brecht Van Lommel (brecht) requested changes to this revision.May 24 2019, 6:57 PM

This revision now requires changes to proceed.May 24 2019, 6:57 PM

Fallback to NSOpenGLView if Metal is not supported.
Show error dialog and quit on errors

Harbormaster completed remote builds in B3795: Diff 15726.Jun 2 2019, 1:05 PM

Brecht Van Lommel (brecht) accepted this revision.Jun 2 2019, 1:06 PM

This revision is now accepted and ready to land.Jun 2 2019, 1:06 PM

Closed by commit rB593eab66b5ca: Cleanup: remove use of deprecated macOS API (authored by Brecht Van Lommel (brecht)). · Explain WhyJun 2 2019, 1:07 PM

This revision was automatically updated to reflect the committed changes.

Use `CAMetalLayer` in place of `NSOpenGLView` for presentation
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 15119

build_files/build_environment/cmake/options.cmake

build_files/cmake/platform/platform_apple.cmake

build_files/cmake/platform/platform_apple_xcode.cmake

intern/ghost/GHOST_C-api.h

intern/ghost/GHOST_IWindow.h

intern/ghost/intern/GHOST_C-api.cpp

intern/ghost/intern/GHOST_ContextCGL.h

intern/ghost/intern/GHOST_ContextCGL.mm

intern/ghost/intern/GHOST_Window.h

intern/ghost/intern/GHOST_WindowCocoa.h

intern/ghost/intern/GHOST_WindowCocoa.mm

source/blender/draw/engines/eevee/eevee_lightcache.c

source/blender/draw/intern/draw_manager.c

source/blender/gpu/GPU_context.h

source/blender/gpu/intern/gpu_context.cpp

source/blender/gpu/intern/gpu_context_private.h

source/blender/gpu/intern/gpu_extensions.c

source/blender/gpu/intern/gpu_framebuffer.c

source/blender/render/intern/source/pipeline.c

source/blender/windowmanager/intern/wm_playanim.c

source/blender/windowmanager/intern/wm_window.c

Use `CAMetalLayer` in place of `NSOpenGLView` for presentationClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 15119

build_files/build_environment/cmake/options.cmake

build_files/cmake/platform/platform_apple.cmake

build_files/cmake/platform/platform_apple_xcode.cmake

intern/ghost/GHOST_C-api.h

intern/ghost/GHOST_IWindow.h

intern/ghost/intern/GHOST_C-api.cpp

intern/ghost/intern/GHOST_ContextCGL.h

intern/ghost/intern/GHOST_ContextCGL.mm

intern/ghost/intern/GHOST_Window.h

intern/ghost/intern/GHOST_WindowCocoa.h

intern/ghost/intern/GHOST_WindowCocoa.mm

source/blender/draw/engines/eevee/eevee_lightcache.c

source/blender/draw/intern/draw_manager.c

source/blender/gpu/GPU_context.h

source/blender/gpu/intern/gpu_context.cpp

source/blender/gpu/intern/gpu_context_private.h

source/blender/gpu/intern/gpu_extensions.c

source/blender/gpu/intern/gpu_framebuffer.c

source/blender/render/intern/source/pipeline.c

source/blender/windowmanager/intern/wm_playanim.c

source/blender/windowmanager/intern/wm_window.c

Use `CAMetalLayer` in place of `NSOpenGLView` for presentation
ClosedPublic
Actions

Revision Contents
Changeset List