From fb0cb1eb11663f8de023f6dd64128ed1f5342ec7 Mon Sep 17 00:00:00 2001 From: Mads Marquart Date: Sun, 8 Sep 2024 16:28:14 +0200 Subject: [PATCH] [metal] Improve layer initialization and resizing (#6107) * [metal]: Create a new layer instead of overwriting the existing one Overriding the `layer` on `NSView` makes the view "layer-hosting", see [wantsLayer], which disables drawing functionality on the view like `drawRect:`/`updateLayer`. This prevents crates like Winit from providing a robust rendering callback that integrates well with the rest of the system. Instead, if the layer is not CAMetalLayer, we create a new sublayer, and render to that instead. [wantsLayer]: https://developer.apple.com/documentation/appkit/nsview/1483695-wantslayer?language=objc * [metal]: Fix double-free when re-using layer * doc: Document the behavior when mis-configuring width/height of Surface * [metal]: Use kCAGravityResize for smoother resizing * [metal] Do not keep the view around that the surface was created from We do not need to use it, and the layer itself is already retained, so it won't be de-allocated from under our feet. * Always set delegate on layers created by Wgpu * More docs on contentsGravity --- wgpu-hal/src/metal/mod.rs | 21 +-- wgpu-hal/src/metal/surface.rs | 282 ++++++++++++++++++++++---------- wgpu-hal/src/vulkan/instance.rs | 15 +- wgpu-types/src/lib.rs | 10 ++ 4 files changed, 216 insertions(+), 112 deletions(-) diff --git a/wgpu-hal/src/metal/mod.rs b/wgpu-hal/src/metal/mod.rs index 62d409a8f..1935e843e 100644 --- a/wgpu-hal/src/metal/mod.rs +++ b/wgpu-hal/src/metal/mod.rs @@ -96,9 +96,7 @@ crate::impl_dyn_resource!( TextureView ); -pub struct Instance { - managed_metal_layer_delegate: surface::HalManagedMetalLayerDelegate, -} +pub struct Instance {} impl Instance { pub fn create_surface_from_layer(&self, layer: &metal::MetalLayerRef) -> Surface { @@ -113,9 +111,7 @@ impl crate::Instance for Instance { profiling::scope!("Init Metal Backend"); // We do not enable metal validation based on the validation flags as it affects the entire // process. Instead, we enable the validation inside the test harness itself in tests/src/native.rs. - Ok(Instance { - managed_metal_layer_delegate: surface::HalManagedMetalLayerDelegate::new(), - }) + Ok(Instance {}) } unsafe fn create_surface( @@ -126,16 +122,12 @@ impl crate::Instance for Instance { match window_handle { #[cfg(target_os = "ios")] raw_window_handle::RawWindowHandle::UiKit(handle) => { - let _ = &self.managed_metal_layer_delegate; - Ok(unsafe { Surface::from_view(handle.ui_view.as_ptr(), None) }) + Ok(unsafe { Surface::from_view(handle.ui_view.cast()) }) } #[cfg(target_os = "macos")] - raw_window_handle::RawWindowHandle::AppKit(handle) => Ok(unsafe { - Surface::from_view( - handle.ns_view.as_ptr(), - Some(&self.managed_metal_layer_delegate), - ) - }), + raw_window_handle::RawWindowHandle::AppKit(handle) => { + Ok(unsafe { Surface::from_view(handle.ns_view.cast()) }) + } _ => Err(crate::InstanceError::new(format!( "window handle {window_handle:?} is not a Metal-compatible handle" ))), @@ -367,7 +359,6 @@ pub struct Device { } pub struct Surface { - view: Option>, render_layer: Mutex, swapchain_format: RwLock>, extent: RwLock, diff --git a/wgpu-hal/src/metal/surface.rs b/wgpu-hal/src/metal/surface.rs index 115e4208a..668b60247 100644 --- a/wgpu-hal/src/metal/surface.rs +++ b/wgpu-hal/src/metal/surface.rs @@ -1,26 +1,30 @@ #![allow(clippy::let_unit_value)] // `let () =` being used to constrain result type -use std::{os::raw::c_void, ptr::NonNull, sync::Once, thread}; +use std::ffi::c_uint; +use std::mem::ManuallyDrop; +use std::ptr::NonNull; +use std::sync::Once; +use std::thread; use core_graphics_types::{ base::CGFloat, geometry::{CGRect, CGSize}, }; +use metal::foreign_types::ForeignType; use objc::{ class, declare::ClassDecl, msg_send, - rc::autoreleasepool, + rc::{autoreleasepool, StrongPtr}, runtime::{Class, Object, Sel, BOOL, NO, YES}, sel, sel_impl, }; use parking_lot::{Mutex, RwLock}; -#[cfg(target_os = "macos")] #[link(name = "QuartzCore", kind = "framework")] extern "C" { #[allow(non_upper_case_globals)] - static kCAGravityTopLeft: *mut Object; + static kCAGravityResize: *mut Object; } extern "C" fn layer_should_inherit_contents_scale_from_window( @@ -46,6 +50,7 @@ impl HalManagedMetalLayerDelegate { type Fun = extern "C" fn(&Class, Sel, *mut Object, CGFloat, *mut Object) -> BOOL; let mut decl = ClassDecl::new(&class_name, class!(NSObject)).unwrap(); unsafe { + // decl.add_class_method::( sel!(layer:shouldInheritContentsScale:fromWindow:), layer_should_inherit_contents_scale_from_window, @@ -58,9 +63,8 @@ impl HalManagedMetalLayerDelegate { } impl super::Surface { - fn new(view: Option>, layer: metal::MetalLayer) -> Self { + fn new(layer: metal::MetalLayer) -> Self { Self { - view, render_layer: Mutex::new(layer), swapchain_format: RwLock::new(None), extent: RwLock::new(wgt::Extent3d::default()), @@ -71,86 +75,183 @@ impl super::Surface { /// If not called on the main thread, this will panic. #[allow(clippy::transmute_ptr_to_ref)] - pub unsafe fn from_view( - view: *mut c_void, - delegate: Option<&HalManagedMetalLayerDelegate>, - ) -> Self { - let view = view.cast::(); - let render_layer = { - let layer = unsafe { Self::get_metal_layer(view, delegate) }; - let layer = layer.cast::(); - // SAFETY: This pointer… - // - // - …is properly aligned. - // - …is dereferenceable to a `MetalLayerRef` as an invariant of the `metal` - // field. - // - …points to an _initialized_ `MetalLayerRef`. - // - …is only ever aliased via an immutable reference that lives within this - // lexical scope. - unsafe { &*layer } - } - .to_owned(); - let _: *mut c_void = msg_send![view, retain]; - Self::new(NonNull::new(view), render_layer) + pub unsafe fn from_view(view: NonNull) -> Self { + let layer = unsafe { Self::get_metal_layer(view) }; + let layer = ManuallyDrop::new(layer); + // SAFETY: The layer is an initialized instance of `CAMetalLayer`, and + // we transfer the retain count to `MetalLayer` using `ManuallyDrop`. + let layer = unsafe { metal::MetalLayer::from_ptr(layer.cast()) }; + Self::new(layer) } pub unsafe fn from_layer(layer: &metal::MetalLayerRef) -> Self { let class = class!(CAMetalLayer); let proper_kind: BOOL = msg_send![layer, isKindOfClass: class]; assert_eq!(proper_kind, YES); - Self::new(None, layer.to_owned()) + Self::new(layer.to_owned()) } - /// If not called on the main thread, this will panic. - pub(crate) unsafe fn get_metal_layer( - view: *mut Object, - delegate: Option<&HalManagedMetalLayerDelegate>, - ) -> *mut Object { - if view.is_null() { - panic!("window does not have a valid contentView"); - } - + /// Get or create a new `CAMetalLayer` associated with the given `NSView` + /// or `UIView`. + /// + /// # Panics + /// + /// If called from a thread that is not the main thread, this will panic. + /// + /// # Safety + /// + /// The `view` must be a valid instance of `NSView` or `UIView`. + pub(crate) unsafe fn get_metal_layer(view: NonNull) -> StrongPtr { let is_main_thread: BOOL = msg_send![class!(NSThread), isMainThread]; if is_main_thread == NO { panic!("get_metal_layer cannot be called in non-ui thread."); } - let main_layer: *mut Object = msg_send![view, layer]; - let class = class!(CAMetalLayer); - let is_valid_layer: BOOL = msg_send![main_layer, isKindOfClass: class]; + // Ensure that the view is layer-backed. + // Views are always layer-backed in UIKit. + #[cfg(target_os = "macos")] + let () = msg_send![view.as_ptr(), setWantsLayer: YES]; - if is_valid_layer == YES { - main_layer + let root_layer: *mut Object = msg_send![view.as_ptr(), layer]; + // `-[NSView layer]` can return `NULL`, while `-[UIView layer]` should + // always be available. + assert!(!root_layer.is_null(), "failed making the view layer-backed"); + + // NOTE: We explicitly do not touch properties such as + // `layerContentsPlacement`, `needsDisplayOnBoundsChange` and + // `contentsGravity` etc. on the root layer, both since we would like + // to give the user full control over them, and because the default + // values suit us pretty well (especially the contents placement being + // `NSViewLayerContentsRedrawDuringViewResize`, which allows the view + // to receive `drawRect:`/`updateLayer` calls). + + let is_metal_layer: BOOL = msg_send![root_layer, isKindOfClass: class!(CAMetalLayer)]; + if is_metal_layer == YES { + // The view has a `CAMetalLayer` as the root layer, which can + // happen for example if user overwrote `-[NSView layerClass]` or + // the view is `MTKView`. + // + // This is easily handled: We take "ownership" over the layer, and + // render directly into that; after all, the user passed a view + // with an explicit Metal layer to us, so this is very likely what + // they expect us to do. + unsafe { StrongPtr::retain(root_layer) } } else { - // If the main layer is not a CAMetalLayer, we create a CAMetalLayer and use it. - let new_layer: *mut Object = msg_send![class, new]; - let frame: CGRect = msg_send![main_layer, bounds]; + // The view does not have a `CAMetalLayer` as the root layer (this + // is the default for most views). + // + // This case is trickier! We cannot use the existing layer with + // Metal, so we must do something else. There are a few options: + // + // 1. Panic here, and require the user to pass a view with a + // `CAMetalLayer` layer. + // + // While this would "work", it doesn't solve the problem, and + // instead passes the ball onwards to the user and ecosystem to + // figure it out. + // + // 2. Override the existing layer with a newly created layer. + // + // If we overlook that this does not work in UIKit since + // `UIView`'s `layer` is `readonly`, and that as such we will + // need to do something different there anyhow, this is + // actually a fairly good solution, and was what the original + // implementation did. + // + // It has some problems though, due to: + // + // a. `wgpu` in our API design choosing not to register a + // callback with `-[CALayerDelegate displayLayer:]`, but + // instead leaves it up to the user to figure out when to + // redraw. That is, we rely on other libraries' callbacks + // telling us when to render. + // + // (If this were an API only for Metal, we would probably + // make the user provide a `render` closure that we'd call + // in the right situations. But alas, we have to be + // cross-platform here). + // + // b. Overwriting the `layer` on `NSView` makes the view + // "layer-hosting", see [wantsLayer], which disables drawing + // functionality on the view like `drawRect:`/`updateLayer`. + // + // These two in combination makes it basically impossible for + // crates like Winit to provide a robust rendering callback + // that integrates with the system's built-in mechanisms for + // redrawing, exactly because overwriting the layer would be + // implicitly disabling those mechanisms! + // + // [wantsLayer]: https://developer.apple.com/documentation/appkit/nsview/1483695-wantslayer?language=objc + // + // 3. Create a sublayer. + // + // `CALayer` has the concept of "sublayers", which we can use + // instead of overriding the layer. + // + // This is also the recommended solution on UIKit, so it's nice + // that we can use (almost) the same implementation for these. + // + // It _might_, however, perform ever so slightly worse than + // overriding the layer directly. + // + // 4. Create a new `MTKView` (or a custom view), and add it as a + // subview. + // + // Similar to creating a sublayer (see above), but also + // provides a bunch of event handling that we don't need. + // + // Option 3 seems like the most robust solution, so this is what + // we're going to do. + + // Create a new sublayer. + let new_layer: *mut Object = msg_send![class!(CAMetalLayer), new]; + let () = msg_send![root_layer, addSublayer: new_layer]; + + // Automatically resize the sublayer's frame to match the + // superlayer's bounds. + // + // Note that there is a somewhat hidden design decision in this: + // We define the `width` and `height` in `configure` to control + // the `drawableSize` of the layer, while `bounds` and `frame` are + // outside of the user's direct control - instead, though, they + // can control the size of the view (or root layer), and get the + // desired effect that way. + // + // We _could_ also let `configure` set the `bounds` size, however + // that would be inconsistent with using the root layer directly + // (as we may do, see above). + let width_sizable = 1 << 1; // kCALayerWidthSizable + let height_sizable = 1 << 4; // kCALayerHeightSizable + let mask: c_uint = width_sizable | height_sizable; + let () = msg_send![new_layer, setAutoresizingMask: mask]; + + // Specify the relative size that the auto resizing mask above + // will keep (i.e. tell it to fill out its superlayer). + let frame: CGRect = msg_send![root_layer, bounds]; let () = msg_send![new_layer, setFrame: frame]; - #[cfg(target_os = "ios")] - { - // Unlike NSView, UIView does not allow to replace main layer. - let () = msg_send![main_layer, addSublayer: new_layer]; - // On iOS, "from_view" may be called before the application initialization is complete, - // `msg_send![view, window]` and `msg_send![window, screen]` will get null. - let screen: *mut Object = msg_send![class!(UIScreen), mainScreen]; - let scale_factor: CGFloat = msg_send![screen, nativeScale]; - let () = msg_send![view, setContentScaleFactor: scale_factor]; - }; - #[cfg(target_os = "macos")] - { - let () = msg_send![view, setLayer: new_layer]; - let () = msg_send![view, setWantsLayer: YES]; - let () = msg_send![new_layer, setContentsGravity: unsafe { kCAGravityTopLeft }]; - let window: *mut Object = msg_send![view, window]; - if !window.is_null() { - let scale_factor: CGFloat = msg_send![window, backingScaleFactor]; - let () = msg_send![new_layer, setContentsScale: scale_factor]; - } - }; - if let Some(delegate) = delegate { - let () = msg_send![new_layer, setDelegate: delegate.0]; - } - new_layer + + // The gravity to use when the layer's `drawableSize` isn't the + // same as the bounds rectangle. + // + // The desired content gravity is `kCAGravityResize`, because it + // masks / alleviates issues with resizing when + // `present_with_transaction` is disabled, and behaves better when + // moving the window between monitors. + // + // Unfortunately, it also makes it harder to see changes to + // `width` and `height` in `configure`. When debugging resize + // issues, swap this for `kCAGravityTopLeft` instead. + let _: () = msg_send![new_layer, setContentsGravity: unsafe { kCAGravityResize }]; + + // Set initial scale factor of the layer. This is kept in sync by + // `configure` (on UIKit), and the delegate below (on AppKit). + let scale_factor: CGFloat = msg_send![root_layer, contentsScale]; + let () = msg_send![new_layer, setContentsScale: scale_factor]; + + let delegate = HalManagedMetalLayerDelegate::new(); + let () = msg_send![new_layer, setDelegate: delegate.0]; + + unsafe { StrongPtr::new(new_layer) } } } @@ -171,16 +272,6 @@ impl super::Surface { } } -impl Drop for super::Surface { - fn drop(&mut self) { - if let Some(view) = self.view { - unsafe { - let () = msg_send![view.as_ptr(), release]; - } - } - } -} - impl crate::Surface for super::Surface { type A = super::Api; @@ -210,19 +301,30 @@ impl crate::Surface for super::Surface { _ => (), } - let device_raw = device.shared.device.lock(); - // On iOS, unless the user supplies a view with a CAMetalLayer, we - // create one as a sublayer. However, when the view changes size, - // its sublayers are not automatically resized, and we must resize - // it here. The drawable size and the layer size don't correlate - #[cfg(target_os = "ios")] + // AppKit / UIKit automatically sets the correct scale factor for + // layers attached to a view. Our layer, however, may not be directly + // attached to a view; in those cases, we need to set the scale + // factor ourselves. + // + // For AppKit, we do so by adding a delegate on the layer with the + // `layer:shouldInheritContentsScale:fromWindow:` method returning + // `true` - this tells the system to automatically update the scale + // factor when it changes. + // + // For UIKit, we manually update the scale factor from the super layer + // here, if there is one. + // + // TODO: Is there a way that we could listen to such changes instead? + #[cfg(not(target_os = "macos"))] { - if let Some(view) = self.view { - let main_layer: *mut Object = msg_send![view.as_ptr(), layer]; - let bounds: CGRect = msg_send![main_layer, bounds]; - let () = msg_send![*render_layer, setFrame: bounds]; + let superlayer: *mut Object = msg_send![render_layer.as_ptr(), superlayer]; + if !superlayer.is_null() { + let scale_factor: CGFloat = msg_send![superlayer, contentsScale]; + let () = msg_send![render_layer.as_ptr(), setContentsScale: scale_factor]; } } + + let device_raw = device.shared.device.lock(); render_layer.set_device(&device_raw); render_layer.set_pixel_format(caps.map_format(config.format)); render_layer.set_framebuffer_only(framebuffer_only); diff --git a/wgpu-hal/src/vulkan/instance.rs b/wgpu-hal/src/vulkan/instance.rs index f44c4fa30..5673859e4 100644 --- a/wgpu-hal/src/vulkan/instance.rs +++ b/wgpu-hal/src/vulkan/instance.rs @@ -514,7 +514,7 @@ impl super::Instance { #[cfg(metal)] fn create_surface_from_view( &self, - view: *mut c_void, + view: std::ptr::NonNull, ) -> Result { if !self.shared.extensions.contains(&ext::metal_surface::NAME) { return Err(crate::InstanceError::new(String::from( @@ -522,16 +522,17 @@ impl super::Instance { ))); } - let layer = unsafe { - crate::metal::Surface::get_metal_layer(view.cast::(), None) - }; + let layer = unsafe { crate::metal::Surface::get_metal_layer(view.cast()) }; + // NOTE: The layer is retained by Vulkan's `vkCreateMetalSurfaceEXT`, + // so no need to retain it beyond the scope of this function. + let layer_ptr = (*layer).cast(); let surface = { let metal_loader = ext::metal_surface::Instance::new(&self.shared.entry, &self.shared.raw); let vk_info = vk::MetalSurfaceCreateInfoEXT::default() .flags(vk::MetalSurfaceCreateFlagsEXT::empty()) - .layer(layer.cast()); + .layer(layer_ptr); unsafe { metal_loader.create_metal_surface(&vk_info, None).unwrap() } }; @@ -873,13 +874,13 @@ impl crate::Instance for super::Instance { (Rwh::AppKit(handle), _) if self.shared.extensions.contains(&ext::metal_surface::NAME) => { - self.create_surface_from_view(handle.ns_view.as_ptr()) + self.create_surface_from_view(handle.ns_view) } #[cfg(all(target_os = "ios", feature = "metal"))] (Rwh::UiKit(handle), _) if self.shared.extensions.contains(&ext::metal_surface::NAME) => { - self.create_surface_from_view(handle.ui_view.as_ptr()) + self.create_surface_from_view(handle.ui_view) } (_, _) => Err(crate::InstanceError::new(format!( "window handle {window_handle:?} is not a Vulkan-compatible handle" diff --git a/wgpu-types/src/lib.rs b/wgpu-types/src/lib.rs index e44f91665..cc0318eda 100644 --- a/wgpu-types/src/lib.rs +++ b/wgpu-types/src/lib.rs @@ -5547,8 +5547,18 @@ pub struct SurfaceConfiguration { /// `Bgra8Unorm` and `Bgra8UnormSrgb` pub format: TextureFormat, /// Width of the swap chain. Must be the same size as the surface, and nonzero. + /// + /// If this is not the same size as the underlying surface (e.g. if it is + /// set once, and the window is later resized), the behaviour is defined + /// but platform-specific, and may change in the future (currently macOS + /// scales the surface, other platforms may do something else). pub width: u32, /// Height of the swap chain. Must be the same size as the surface, and nonzero. + /// + /// If this is not the same size as the underlying surface (e.g. if it is + /// set once, and the window is later resized), the behaviour is defined + /// but platform-specific, and may change in the future (currently macOS + /// scales the surface, other platforms may do something else). pub height: u32, /// Presentation mode of the swap chain. Fifo is the only mode guaranteed to be supported. /// FifoRelaxed, Immediate, and Mailbox will crash if unsupported, while AutoVsync and