Page MenuHome

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

Authored by Tomoaki Kawada (yvt) on Mar 30 2019, 8:24 AM.

Details

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

Event Timeline

Tomoaki Kawada (yvt) added a comment.EditedApr 16 2019, 6:27 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
  • Revert accidental mix-up with D4580

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
730 ↗(On Diff #14867)

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
261 ↗(On Diff #14867)

Follow style and make a description.

source/blender/windowmanager/intern/wm_playanim.c
1290 ↗(On Diff #14867)

Code Style: Avoid nested function calls.

source/blender/windowmanager/intern/wm_window.c
730 ↗(On Diff #14867)

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
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
730 ↗(On Diff #14867)

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

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
This revision is now accepted and ready to land.Jun 2 2019, 1:06 PM
This revision was automatically updated to reflect the committed changes.