Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

macOS: fix initializing Vulkan surface when using gfx-portability #8724

Conversation

kemenaran
Copy link
Contributor

Since #8666, when running Dolphin using gfx-portability instead of MoltenVK, it crashes with an exception:

$ LIBVULKAN_PATH=/usr/local/lib/libportability.dylib /Applications/Dolphin.app/Contents/MacOS/Dolphin
2020-04-05 21:05:53.416 Dolphin[73210:890862] -[CAMetalLayer layer]: unrecognized selector sent to instance 0x7fa0be67a710
2020-04-05 21:05:53.436 Dolphin[73210:890862] *** Terminating app due to uncaught exception 'NSInvalidArgumentException', reason: '-[CAMetalLayer layer]: unrecognized selector sent to instance 0x7fa0be67a710'
*** First throw call stack:
(
	0   CoreFoundation                      0x00007fff30a16acd __exceptionPreprocess + 256
	1   libobjc.A.dylib                     0x00007fff5b11aa17 objc_exception_throw + 48
	2   CoreFoundation                      0x00007fff30a908d6 -[NSObject(NSObject) __retain_OA] + 0
	3   CoreFoundation                      0x00007fff309b893f ___forwarding___ + 1485
	4   CoreFoundation                      0x00007fff309b82e8 _CF_forwarding_prep_0 + 120
	5   libportability.dylib                0x000000010c315758 _ZN17gfx_backend_metal8Instance26create_surface_from_nsview17h127760c1704de68dE + 72
	6   libportability.dylib                0x000000010c284cff _ZN15portability_gfx5impls24gfxCreateMacOSSurfaceMVK17h9f931ad376c6842eE + 207
	7   Dolphin                             0x00000001009c8fd1 _ZN6Vulkan9SwapChain19CreateVulkanSurfaceEP12VkInstance_TRK16WindowSystemInfo + 65
)
libc++abi.dylib: terminating with uncaught exception of type NSException

This seems to be because vkCreateWin32SurfaceKHR expects an NSView (per the documentation), but after #8666 it receives a CAMetalLayer.

This PR fixes the crash by passing the CAMetalLayer-backed NSView instead. Running Dolphin with gfx-portability runs fine after this.

Note of gfx-portability

On a 2013 Macbook / Iris 5100, gfx-portability gives a substantially better framerate than MoltenVK (around ~10 FPS). At some point I remember it also had better frame pacing. So I'm quite eager to keep Dophin working with gfx-portability :)

Since dolphin-emu#8666, when running Dolphin using gfx-portability instead of
MoltenVK, it crashes with an exception:

```
$ LIBVULKAN_PATH=/usr/local/lib/libportability.dylib /Applications/Dolphin.app/Contents/MacOS/Dolphin
2020-04-05 21:05:53.416 Dolphin[73210:890862] -[CAMetalLayer layer]: unrecognized selector sent to instance 0x7fa0be67a710
2020-04-05 21:05:53.436 Dolphin[73210:890862] *** Terminating app due to uncaught exception 'NSInvalidArgumentException', reason: '-[CAMetalLayer layer]: unrecognized selector sent to instance 0x7fa0be67a710'
*** First throw call stack:
(
	0   CoreFoundation                      0x00007fff30a16acd __exceptionPreprocess + 256
	1   libobjc.A.dylib                     0x00007fff5b11aa17 objc_exception_throw + 48
	2   CoreFoundation                      0x00007fff30a908d6 -[NSObject(NSObject) __retain_OA] + 0
	3   CoreFoundation                      0x00007fff309b893f ___forwarding___ + 1485
	4   CoreFoundation                      0x00007fff309b82e8 _CF_forwarding_prep_0 + 120
	5   libportability.dylib                0x000000010c315758 _ZN17gfx_backend_metal8Instance26create_surface_from_nsview17h127760c1704de68dE + 72
	6   libportability.dylib                0x000000010c284cff _ZN15portability_gfx5impls24gfxCreateMacOSSurfaceMVK17h9f931ad376c6842eE + 207
	7   Dolphin                             0x00000001009c8fd1 _ZN6Vulkan9SwapChain19CreateVulkanSurfaceEP12VkInstance_TRK16WindowSystemInfo + 65
)
libc++abi.dylib: terminating with uncaught exception of type NSException
```

This seems to be because `vkCreateWin32SurfaceKHR` expects an NSView
(per the documentation), but after dolphin-emu#8666 it receives a CAMetalLayer.

This PR fixes the crash by passing the CAMetalLayer-backed NSView
instead.
@gilcel
Copy link

gilcel commented Apr 6, 2020

Interesting! But how do you add gfx-portability to Dolphin.app ?
Can it be bundled with the macOS app ?

@kemenaran
Copy link
Contributor Author

For now gfx-portability dynamically replaces MoltenVK at runtime. You have to download a compiled release of gfx-portability, then run Dolphin with LIBVULKAN_PATH=/path/to/libportability.dylib /Applications/Dolphin.app/Contents/MacOS/Dolphin. The Vulkan backend will then use gfx-portability instead of MoltenVK.

@gilcel
Copy link

gilcel commented Apr 6, 2020

Wow! Amazing! Thanks!
I recompiled Dolphin again with your commit and installed the gfx-portability library and followed your instructions.
I now get 60fps constant with Vulkan/gfx-portabilty for SMBWii on a Mac mini 2012 with an Intel HD 4000 wannabe GPU. DKCR is a bit faster too....

@stenzek
Copy link
Contributor

stenzek commented Apr 7, 2020

-1 for this. This was a deliberate choice, portability is in the wrong here. The swap chain is created on the video thread, which is not the UI thread. Calling any methods on the NSView object off the UI thread is invalid, and you'll notice if you run under XCode that you get an error. Hence why we create the layer on the UI thread, and directly pass it to MVK so it doesn't touch the NSView.

Portability should accept a layer pointer unless they are only interested in people creating swap chains on the UI thread.

@galad87
Copy link

galad87 commented Apr 7, 2020

There is a new non MoltenVK specific extension to pass the CALayer: VK_EXT_metal_surface. I think the right fix would be to switch to that (and update Dolphin built-in MoltenVK).

@stenzek
Copy link
Contributor

stenzek commented Apr 7, 2020

@galad87 I feel like that doesn't help the underlying issue, which is altering the view off the UI thread. You're still going to need to create a layer at some point, so unless portability is queusing it on the main/UI thread, it's not safe.

While it might work in the current version, Apple has a history of restricting these sorts of things with OS updates, so what works now might not work in 6 months time. I'd rather just "do it properly".

@galad87
Copy link

galad87 commented Apr 7, 2020

Of course, NSView is main-thread only, but the new VK_EXT_metal_surface extension defines a common way to pass a CALayer to Vulkan, and hopefully both MoltenVK and gfx-portability implements this extension correctly.
The issue here is that gfx-portability requires a NSView when using the VK_MVK_macos_surface extension, while MoltenVK works with a CALayer too. So switching to VK_EXT_metal_surface that requires a CALayer on both should fix this.

Altering the NSView will continue to be a main-thread only operation anyway.

@stenzek
Copy link
Contributor

stenzek commented Apr 7, 2020

Okay, great, we're on the same page then.

I'll switch dolphin over to the extension then, it's due to update mvk anyway.

@Zirro
Copy link

Zirro commented Apr 7, 2020

If the performance improves significantly when gfx-portability is used instead of MoltenVK as the comments above suggest, perhaps the former should be included in Dolphin by default instead?

@stenzek
Copy link
Contributor

stenzek commented Apr 7, 2020

@Zirro I'd rather use the Khronos-recommended/supported translation layer. It's not just about performance, compatibility matters too. But users should have the choice.

@Zirro
Copy link

Zirro commented Apr 7, 2020

I see, though I have a feeling that @kvark would be quite interested in sorting out any compatibility issues which may be uncovered.

@gilcel
Copy link

gilcel commented Apr 7, 2020

Well I just tested 2 games. My mac mini has a (crappy) Intel HD 4000 integrated GPU so not sure if everyone get the same results...it should be thorougly tested by other Mac users as well.

@galad87
Copy link

galad87 commented Apr 7, 2020

You should test the latest version of MoltenVK too.

@stenzek
Copy link
Contributor

stenzek commented Apr 7, 2020

Of course - and Dolphin has helped fix issues for both projects in the past. That's why the command-line override exists ;)

I'd just prefer to stick with what is supported by the vendor as it's the lowest-risk option, considering none of our team actively develops for macOS and Dolphin's support for it is basically on life support at the moment. Unless someone wants to take on the platform maintainer role :)

@stenzek
Copy link
Contributor

stenzek commented Apr 7, 2020

See #8728

@kvark
Copy link
Contributor

kvark commented Apr 7, 2020

We can and will work around this in gfx-portability-0.8 release.

@kvark
Copy link
Contributor

kvark commented Apr 7, 2020

(a more detailed answer as I caught up to the discussion)

Passing CAMetalLayer instead of NSView to VK_MVK_macos_surface is dirty hack. The extension wasn't specified this way, and the change in behavior and expectations isn't a part of it. gfx-portability implements it as it was specified, and rightfully crashes in this case.

We made VK_EXT_metal_surface specifically to allow applications to target Vulkan on macOS in a standard way. It's the preferred future-proof solution to the problem. We hope to remove support for MVK extensions in gfx-portability entirely in the future. There is a wait for Vulkan validation layers to catch up and understand this extension though, and for now they will complain, but otherwise everything will work with both gfx-portability and MoltenVK.

In the meantime, we are changing our implementation of VK_MVK_macos_surface to support this hack. Hopefully will release gfx-portability-0.8 with this soon.

@stenzek
Copy link
Contributor

stenzek commented Apr 7, 2020

@kvark are there other programs which have this problem as well? Because I've already changed Dolphin to use the correct extension (#8728), so please don't feel the need to add hacks just for us.

@kvark
Copy link
Contributor

kvark commented Apr 7, 2020

No, I've been testing a lot of stuff recently in gfx-rs/portability#210 (comment) and didn't find any other case that would need that. It's still a relatively harmless hack, and I'm ok to have it given that the whole VK_MVK_macos_surface will eventually be scraped :)
Thank you for #8728 !

@stenzek
Copy link
Contributor

stenzek commented Apr 7, 2020

Okay, fair enough. I wish someone told me about the extension earlier, since I'm not following any MVK/Portability changes, and I would have already switched it over with the fixes for Qt 5.14 last month! :)

@gilcel
Copy link

gilcel commented Apr 7, 2020

Could yuzu's recent macOS pull request benefit from your code ? Would be great ;-)
https://github.com/yuzu-emu/yuzu/pull/3593

@kemenaran
Copy link
Contributor Author

@stenzek thanks for investigating this, and for the new PR – it's much better than my attempt.

I guess we can close this PR as it is superseded by #8728 then?

@kemenaran
Copy link
Contributor Author

Superseded by #8728.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

6 participants