From e782be19eb27dcecd88881fcbd6312388c30e63c Mon Sep 17 00:00:00 2001 From: Mads Marquart Date: Mon, 12 Aug 2024 17:47:25 +0200 Subject: [PATCH 1/7] [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 --- wgpu-hal/src/metal/mod.rs | 4 +- wgpu-hal/src/metal/surface.rs | 233 +++++++++++++++++++++++--------- wgpu-hal/src/vulkan/instance.rs | 10 +- 3 files changed, 175 insertions(+), 72 deletions(-) diff --git a/wgpu-hal/src/metal/mod.rs b/wgpu-hal/src/metal/mod.rs index 62d409a8ff..2ec34e56ba 100644 --- a/wgpu-hal/src/metal/mod.rs +++ b/wgpu-hal/src/metal/mod.rs @@ -127,12 +127,12 @@ impl crate::Instance for Instance { #[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(), None) }) } #[cfg(target_os = "macos")] raw_window_handle::RawWindowHandle::AppKit(handle) => Ok(unsafe { Surface::from_view( - handle.ns_view.as_ptr(), + handle.ns_view.cast(), Some(&self.managed_metal_layer_delegate), ) }), diff --git a/wgpu-hal/src/metal/surface.rs b/wgpu-hal/src/metal/surface.rs index 115e4208a5..ed3d338948 100644 --- a/wgpu-hal/src/metal/surface.rs +++ b/wgpu-hal/src/metal/surface.rs @@ -1,11 +1,15 @@ #![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::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, @@ -16,7 +20,6 @@ use objc::{ }; use parking_lot::{Mutex, RwLock}; -#[cfg(target_os = "macos")] #[link(name = "QuartzCore", kind = "framework")] extern "C" { #[allow(non_upper_case_globals)] @@ -46,6 +49,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, @@ -72,26 +76,15 @@ 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, + view: NonNull, 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) + let layer = unsafe { Self::get_metal_layer(view, delegate) }; + // SAFETY: The layer is an initialized instance of `CAMetalLayer`. + let layer = unsafe { metal::MetalLayer::from_ptr(layer.cast()) }; + let view: *mut Object = msg_send![view.as_ptr(), retain]; + let view = NonNull::new(view).expect("retain should return the same object"); + Self::new(Some(view), layer) } pub unsafe fn from_layer(layer: &metal::MetalLayerRef) -> Self { @@ -101,52 +94,155 @@ impl super::Surface { Self::new(None, layer.to_owned()) } - /// If not called on the main thread, this will panic. + /// 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: *mut Object, + view: NonNull, delegate: Option<&HalManagedMetalLayerDelegate>, ) -> *mut Object { - if view.is_null() { - panic!("window does not have a valid contentView"); - } - 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]; + + 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). - if is_valid_layer == YES { - main_layer + 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. + 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]; - } - }; + + let _: () = msg_send![new_layer, setContentsGravity: unsafe { kCAGravityTopLeft }]; + + // 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]; + if let Some(delegate) = delegate { let () = msg_send![new_layer, setDelegate: delegate.0]; } @@ -210,19 +306,28 @@ 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 the 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 here. + // + // 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 scale_factor: CGFloat = msg_send![view.as_ptr(), contentScaleFactor]; + 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 832c74b030..7ba286afa6 100644 --- a/wgpu-hal/src/vulkan/instance.rs +++ b/wgpu-hal/src/vulkan/instance.rs @@ -510,7 +510,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( @@ -518,9 +518,7 @@ 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(), None) }; let surface = { let metal_loader = @@ -870,13 +868,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" From 0170dfe45861a54f1adf0e59df2ac81925fb921f Mon Sep 17 00:00:00 2001 From: Mads Marquart Date: Mon, 12 Aug 2024 23:29:58 +0200 Subject: [PATCH 2/7] [metal]: Fix double-free when re-using layer --- wgpu-hal/src/metal/surface.rs | 13 ++++++++----- wgpu-hal/src/vulkan/instance.rs | 5 ++++- 2 files changed, 12 insertions(+), 6 deletions(-) diff --git a/wgpu-hal/src/metal/surface.rs b/wgpu-hal/src/metal/surface.rs index ed3d338948..74770d9e48 100644 --- a/wgpu-hal/src/metal/surface.rs +++ b/wgpu-hal/src/metal/surface.rs @@ -1,6 +1,7 @@ #![allow(clippy::let_unit_value)] // `let () =` being used to constrain result type use std::ffi::c_uint; +use std::mem::ManuallyDrop; use std::ptr::NonNull; use std::sync::Once; use std::thread; @@ -14,7 +15,7 @@ use objc::{ class, declare::ClassDecl, msg_send, - rc::autoreleasepool, + rc::{autoreleasepool, StrongPtr}, runtime::{Class, Object, Sel, BOOL, NO, YES}, sel, sel_impl, }; @@ -80,7 +81,9 @@ impl super::Surface { delegate: Option<&HalManagedMetalLayerDelegate>, ) -> Self { let layer = unsafe { Self::get_metal_layer(view, delegate) }; - // SAFETY: The layer is an initialized instance of `CAMetalLayer`. + 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()) }; let view: *mut Object = msg_send![view.as_ptr(), retain]; let view = NonNull::new(view).expect("retain should return the same object"); @@ -107,7 +110,7 @@ impl super::Surface { pub(crate) unsafe fn get_metal_layer( view: NonNull, delegate: Option<&HalManagedMetalLayerDelegate>, - ) -> *mut Object { + ) -> 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."); @@ -141,7 +144,7 @@ impl super::Surface { // 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. - root_layer + unsafe { StrongPtr::retain(root_layer) } } else { // The view does not have a `CAMetalLayer` as the root layer (this // is the default for most views). @@ -246,7 +249,7 @@ impl super::Surface { if let Some(delegate) = delegate { let () = msg_send![new_layer, setDelegate: delegate.0]; } - new_layer + unsafe { StrongPtr::new(new_layer) } } } diff --git a/wgpu-hal/src/vulkan/instance.rs b/wgpu-hal/src/vulkan/instance.rs index 7ba286afa6..7759d814d1 100644 --- a/wgpu-hal/src/vulkan/instance.rs +++ b/wgpu-hal/src/vulkan/instance.rs @@ -519,13 +519,16 @@ impl super::Instance { } let layer = unsafe { crate::metal::Surface::get_metal_layer(view.cast(), None) }; + // 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() } }; From 1b153aec9f0c62a9b101effdf46212ff6b8fd776 Mon Sep 17 00:00:00 2001 From: Mads Marquart Date: Fri, 23 Aug 2024 05:04:03 +0200 Subject: [PATCH 3/7] doc: Document the behavior when mis-configuring width/height of Surface --- wgpu-types/src/lib.rs | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/wgpu-types/src/lib.rs b/wgpu-types/src/lib.rs index 4f15e68a17..c9167de4e9 100644 --- a/wgpu-types/src/lib.rs +++ b/wgpu-types/src/lib.rs @@ -5530,8 +5530,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 From bd427b7cecc809ba801927c83fecb092872f326f Mon Sep 17 00:00:00 2001 From: Mads Marquart Date: Mon, 12 Aug 2024 16:31:55 +0200 Subject: [PATCH 4/7] [metal]: Use kCAGravityResize for smoother resizing --- wgpu-hal/src/metal/surface.rs | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/wgpu-hal/src/metal/surface.rs b/wgpu-hal/src/metal/surface.rs index 74770d9e48..a717983c64 100644 --- a/wgpu-hal/src/metal/surface.rs +++ b/wgpu-hal/src/metal/surface.rs @@ -24,7 +24,7 @@ use parking_lot::{Mutex, RwLock}; #[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( @@ -239,7 +239,15 @@ impl super::Surface { let frame: CGRect = msg_send![root_layer, bounds]; let () = msg_send![new_layer, setFrame: frame]; - let _: () = msg_send![new_layer, setContentsGravity: unsafe { kCAGravityTopLeft }]; + // 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). From 9192c908e79f40fa8a20d60244299271bf7eb783 Mon Sep 17 00:00:00 2001 From: Mads Marquart Date: Sun, 25 Aug 2024 17:28:36 +0200 Subject: [PATCH 5/7] [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. --- wgpu-hal/src/metal/mod.rs | 1 - wgpu-hal/src/metal/surface.rs | 29 +++++++++-------------------- 2 files changed, 9 insertions(+), 21 deletions(-) diff --git a/wgpu-hal/src/metal/mod.rs b/wgpu-hal/src/metal/mod.rs index 2ec34e56ba..cd9b824f7f 100644 --- a/wgpu-hal/src/metal/mod.rs +++ b/wgpu-hal/src/metal/mod.rs @@ -367,7 +367,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 a717983c64..5e61084b52 100644 --- a/wgpu-hal/src/metal/surface.rs +++ b/wgpu-hal/src/metal/surface.rs @@ -63,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()), @@ -85,16 +84,14 @@ impl super::Surface { // 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()) }; - let view: *mut Object = msg_send![view.as_ptr(), retain]; - let view = NonNull::new(view).expect("retain should return the same object"); - Self::new(Some(view), layer) + 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()) } /// Get or create a new `CAMetalLayer` associated with the given `NSView` @@ -278,16 +275,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; @@ -319,7 +306,7 @@ impl crate::Surface for super::Surface { // AppKit / UIKit automatically sets the correct scale factor for // layers attached to a view. Our layer, however, may not be directly - // attached to the view; in those cases, we need to set the scale + // 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 @@ -327,13 +314,15 @@ impl crate::Surface for super::Surface { // `true` - this tells the system to automatically update the scale // factor when it changes. // - // For UIKit, we manually update the scale factor here. + // 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 scale_factor: CGFloat = msg_send![view.as_ptr(), contentScaleFactor]; + 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]; } } From d6e3e30c08d54bcdaea6c97c2e5449b2beb937cd Mon Sep 17 00:00:00 2001 From: Mads Marquart Date: Fri, 30 Aug 2024 07:51:49 +0200 Subject: [PATCH 6/7] Always set delegate on layers created by Wgpu --- wgpu-hal/src/metal/mod.rs | 20 ++++++-------------- wgpu-hal/src/metal/surface.rs | 18 ++++++------------ wgpu-hal/src/vulkan/instance.rs | 2 +- 3 files changed, 13 insertions(+), 27 deletions(-) diff --git a/wgpu-hal/src/metal/mod.rs b/wgpu-hal/src/metal/mod.rs index cd9b824f7f..1935e843ec 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.cast(), 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.cast(), - 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" ))), diff --git a/wgpu-hal/src/metal/surface.rs b/wgpu-hal/src/metal/surface.rs index 5e61084b52..ec117f17ce 100644 --- a/wgpu-hal/src/metal/surface.rs +++ b/wgpu-hal/src/metal/surface.rs @@ -75,11 +75,8 @@ 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: NonNull, - delegate: Option<&HalManagedMetalLayerDelegate>, - ) -> Self { - let layer = unsafe { Self::get_metal_layer(view, delegate) }; + 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`. @@ -104,10 +101,7 @@ impl super::Surface { /// # Safety /// /// The `view` must be a valid instance of `NSView` or `UIView`. - pub(crate) unsafe fn get_metal_layer( - view: NonNull, - delegate: Option<&HalManagedMetalLayerDelegate>, - ) -> StrongPtr { + 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."); @@ -251,9 +245,9 @@ impl super::Surface { let scale_factor: CGFloat = msg_send![root_layer, contentsScale]; let () = msg_send![new_layer, setContentsScale: scale_factor]; - if let Some(delegate) = delegate { - let () = msg_send![new_layer, setDelegate: delegate.0]; - } + let delegate = HalManagedMetalLayerDelegate::new(); + let () = msg_send![new_layer, setDelegate: delegate.0]; + unsafe { StrongPtr::new(new_layer) } } } diff --git a/wgpu-hal/src/vulkan/instance.rs b/wgpu-hal/src/vulkan/instance.rs index 7759d814d1..f52a055284 100644 --- a/wgpu-hal/src/vulkan/instance.rs +++ b/wgpu-hal/src/vulkan/instance.rs @@ -518,7 +518,7 @@ 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(); From b43f8bd31a207822c7371053da80a3c04abff1f5 Mon Sep 17 00:00:00 2001 From: Mads Marquart Date: Wed, 4 Sep 2024 12:06:29 +0200 Subject: [PATCH 7/7] More docs on contentsGravity --- wgpu-hal/src/metal/surface.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/wgpu-hal/src/metal/surface.rs b/wgpu-hal/src/metal/surface.rs index ec117f17ce..668b602474 100644 --- a/wgpu-hal/src/metal/surface.rs +++ b/wgpu-hal/src/metal/surface.rs @@ -230,6 +230,9 @@ impl super::Surface { let frame: CGRect = msg_send![root_layer, bounds]; let () = msg_send![new_layer, setFrame: frame]; + // 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