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

Support Metal on macOS #109

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Conversation

samizdatco
Copy link

@samizdatco samizdatco commented Jul 24, 2022

These changes allow skulpin to run on macOS systems without relying on MoltenVK. The skia_support module in skulpin-renderer has been split to create Vulkan- and Metal-specific versions of the SkiaContext and SkiaSurface structs. Context-creation is quite different between the two backends, but Surface-creation has a lot of overlap and could potentially share more of the implementation. The repetition that currently exists is in the interest of being able to keep the metal and vulkan crate dependencies separated.

The Renderer itself only required minimal changes to the creation of its RafxApi struct and an additional switch between the two versions of the cooked shaders (which now live in shaders/out/vk and shaders/out/mtl).

Unrelated changes involve adding support for version 0.26 of winit and updating skia-safe to 0.52.

Current Status

The examples run successfully without UI interaction. However, I've been able to trigger a segmentation violation by rapidly resizing the window. After a second or two of resizing the window, it crashes reliably (seemingly during the reallocation of the swapchain frame buffers?). The logging leading up to the crash looks like:

[2022-07-24T23:20:39Z INFO  rafx_api::extra::swapchain_helper] Rebuild Swapchain
[2022-07-24T23:20:39Z DEBUG rafx_api::extra::swapchain_helper] Force swapchain rebuild due to changed window size
[2022-07-24T23:20:39Z INFO  rafx_api::extra::swapchain_helper] Rebuild Swapchain
[2022-07-24T23:20:39Z DEBUG rafx_api::extra::swapchain_helper] Force swapchain rebuild due to changed window size
[2022-07-24T23:20:39Z INFO  rafx_api::extra::swapchain_helper] Rebuild Swapchain
[2022-07-24T23:20:39Z DEBUG rafx_api::extra::swapchain_helper] Force swapchain rebuild due to changed window size
[2022-07-24T23:20:39Z INFO  rafx_api::extra::swapchain_helper] Rebuild Swapchain
[2022-07-24T23:20:39Z DEBUG rafx_api::extra::swapchain_helper] Force swapchain rebuild due to changed window size
[2022-07-24T23:20:39Z INFO  rafx_api::extra::swapchain_helper] Rebuild Swapchain
fish: Job 1, 'cargo run --example hello_skulp…' terminated by signal SIGSEGV (Address boundary error)

And here's the backtrace, which points to a crash here. Changing the surface creation parameters to use skia_safe::Budgeted::No increases the number of resizings before the crash but doesn't seem to prevent it altogether, so leaving it set to Yes may make it easier to track down the root cause.

On the other hand, it's gratifying to have gotten this far at least:
image
image

@aclysma
Copy link
Owner

aclysma commented Jul 25, 2022

Nice work, while I’m not actively maintaining/extending this library any more (because I don’t use it for anything now), I can test/upstream this.

One thing I did notice in the logs earlier when using moltenvk was that moltenvk was claiming to be vulkan 1.3.x. Using an older moltenvk might fix the problem (moltenvk is not really a “compliant” implementation and I’ve had to fix interactions between skia/newer moltenvk before.)

I need to look in more detail but one thing that could be simplified is using the same shader package for vulkan and metal (pass both the package-vk and package-metal args). The way it’s done here isn’t wrong but being a single file for both backends does make it harder to update shaders for one backend and not the other.

As for the crash, may need to look at the skia implementation to see what pointer they’re dereferencing without checking first and see if it’s possible to defer the call until the pointer won’t be null. (That’s my first guess anyways.)

@samizdatco
Copy link
Author

As for the crash, may need to look at the skia implementation to see what pointer they’re dereferencing without checking first and see if it’s possible to defer the call until the pointer won’t be null. (That’s my first guess anyways.)

Another place where it's currently running into problems is deallocating renderers. When I close a window, it crashes with this backtrace and I see this in the logs:

Window WindowId(Id(5409772624)) has received the signal to close
[2022-07-25T14:20:15Z DEBUG skulpin_renderer::renderer] destroying Renderer
[2022-07-25T14:20:15Z DEBUG skulpin_renderer::renderer] destroyed Renderer
[2022-07-25T14:20:15Z DEBUG rafx_api::extra::swapchain_helper] Destroying swapchain helper
[2022-07-25T14:20:15Z DEBUG rafx_api::extra::swapchain_helper] wait for all fences to complete
[2022-07-25T14:20:15Z INFO  rafx_framework::resources::resource_manager] Cleaning up resource manager
fish: Job 1, 'cargo run' terminated by signal SIGSEGV (Address boundary error)

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

Successfully merging this pull request may close these issues.

2 participants