Merge #2401

2401: [mtl] Filament compatibility fixes r=grovesNL a=kvark

![gfx-filament-pbr-small](https://user-images.githubusercontent.com/107301/45447569-ea30fd80-b69d-11e8-8ffd-50aa4cd947ec.png)

There are still occasional asserts on frame acquisition, so leaving #2400 open for now.

PR checklist:
- [ ] `make` succeeds (on *nix)
- [ ] `make reftests` succeeds
- [ ] tested examples with the following backends:
- [ ] `rustfmt` run on changed code


Co-authored-by: Dzmitry Malyshau <[email protected]>

Author: bors[bot]

src/backend/metal/src/command.rs

@@ -588,8 +588,11 @@ impl StageResources {
                     pool_offsets.textures += 1;
                 }
                 if layout.content.contains(native::DescriptorContent::BUFFER) {
-                    let (buffer, offset) = data.buffers[pool_offsets.buffers as usize].unwrap();
-                    self.buffers[res_offset.buffers as usize] = Some(buffer);
+                    let (buffer, offset) = match data.buffers[pool_offsets.buffers as usize] {
+                        Some((buffer, offset)) => (Some(buffer), offset),
+                        None => (None, 0),
+                    };
+                    self.buffers[res_offset.buffers as usize] = buffer;
                     self.buffer_offsets[res_offset.buffers as usize] = offset;
                     res_offset.buffers += 1;
                     pool_offsets.buffers += 1;
@@ -1782,8 +1785,9 @@ impl RawCommandQueue<Backend> for CommandQueue {
 
         for (swapchain, index) in swapchains {
             debug!("presenting frame {}", index);
-            let drawable = swapchain.borrow().take_drawable(index);
-            command_buffer.present_drawable(&drawable);
+            if let Ok(drawable) = swapchain.borrow().take_drawable(index) {
+                command_buffer.present_drawable(&drawable);
+            }
         }
 
         command_buffer.commit();

src/backend/metal/src/lib.rs

@@ -48,7 +48,6 @@ use core_graphics::base::CGFloat;
 use core_graphics::geometry::CGRect;
 use foreign_types::ForeignTypeRef;
 use objc::runtime::{Class, Object};
-#[cfg(target_os = "ios")]
 use objc::runtime::{BOOL, YES};
 use parking_lot::{Condvar, Mutex};
 
@@ -194,23 +193,33 @@ impl Instance {
                 panic!("window does not have a valid contentView");
             }
 
+            let existing: *mut Object = msg_send![view, layer];
             let class = Class::get("CAMetalLayer").unwrap();
-            let render_layer: *mut Object = msg_send![class, new];
-            msg_send![view, setLayer: render_layer];
-            msg_send![view, retain];
-            let bounds: CGRect = msg_send![view, bounds];
-            msg_send![render_layer, setBounds: bounds];
-
-            let window: *mut Object = msg_send![view, window];
-            if !window.is_null() {
-                let scale_factor: CGFloat = msg_send![window, backingScaleFactor];
-                msg_send![render_layer, setContentsScale: scale_factor];
-            }
+            let use_current = if existing.is_null() {
+                false
+            } else {
+                let result: BOOL = msg_send![existing, isKindOfClass:class];
+                result == YES
+            };
+
+            let render_layer: *mut Object = if use_current {
+                existing
+            } else {
+                let layer: *mut Object = msg_send![class, new];
+                msg_send![view, setLayer: layer];
+                msg_send![view, retain];
+                let bounds: CGRect = msg_send![view, bounds];
+                msg_send![layer, setBounds: bounds];
+
+                let window: *mut Object = msg_send![view, window];
+                if !window.is_null() {
+                    let scale_factor: CGFloat = msg_send![window, backingScaleFactor];
+                    msg_send![layer, setContentsScale: scale_factor];
+                }
+                layer
+            };
 
-            window::SurfaceInner {
-                view,
-                render_layer: Mutex::new(render_layer),
-            }
+            window::SurfaceInner::new(view, render_layer)
         }
     }
 
@@ -259,10 +268,7 @@ impl Instance {
             msg_send![view, setContentScaleFactor: scale_factor];
 
             msg_send![view, retain];
-            window::SurfaceInner {
-                view,
-                render_layer: Mutex::new(render_layer),
-            }
+            window::SurfaceInner::new(view, render_layer)
         }
     }
 

src/backend/metal/src/window.rs

@@ -18,6 +18,9 @@ use objc::rc::autoreleasepool;
 use objc::runtime::Object;
 
 
+//TODO: make it a weak pointer, so that we know which
+// frames can be replaced if we receive an unknown
+// texture pointer by an acquired drawable.
 pub type CAMetalLayer = *mut Object;
 
 pub struct Surface {
@@ -27,9 +30,9 @@ pub struct Surface {
 }
 
 #[derive(Debug)]
-pub(crate) struct SurfaceInner {
-    pub(crate) view: *mut Object,
-    pub(crate) render_layer: Mutex<CAMetalLayer>,
+pub struct SurfaceInner {
+    view: *mut Object,
+    render_layer: Mutex<CAMetalLayer>,
 }
 
 unsafe impl Send for SurfaceInner {}
@@ -41,16 +44,30 @@ impl Drop for SurfaceInner {
     }
 }
 
+#[derive(Debug)]
+struct FrameNotFound {
+    drawable: metal::Drawable,
+    texture: metal::Texture,
+}
+
+
 impl SurfaceInner {
-    pub(crate) fn into_surface(self) -> Surface {
+    pub fn new(view: *mut Object, layer: CAMetalLayer) -> Self {
+        SurfaceInner {
+            view,
+            render_layer: Mutex::new(layer),
+        }
+    }
+
+    pub fn into_surface(self) -> Surface {
         Surface {
             inner: Arc::new(self),
             main_thread_id: thread::current().id(),
             has_swapchain: false,
         }
     }
 
-    fn next_frame<'a>(&self, frames: &'a [Frame]) -> Result<(usize, MutexGuard<'a, FrameInner>), ()> {
+    fn next_frame<'a>(&self, frames: &'a [Frame]) -> Result<(usize, MutexGuard<'a, FrameInner>), FrameNotFound> {
         let layer_ref = self.render_layer.lock();
         autoreleasepool(|| { // for the drawable
             let (drawable, texture_temp): (&metal::DrawableRef, &metal::TextureRef) = unsafe {
@@ -68,19 +85,38 @@ impl SurfaceInner {
                     debug!("next is frame[{}]", index);
                     Ok((index, frame))
                 }
-                None => Err(()),
+                None => Err(FrameNotFound {
+                    drawable: drawable.to_owned(),
+                    texture: texture_temp.to_owned(),
+                }),
             }
         })
     }
+
+    fn dimensions(&self) -> Extent2D {
+        unsafe {
+            // NSView/UIView bounds are measured in DIPs
+            let bounds: CGRect = msg_send![self.view, bounds];
+            //let bounds_pixel: NSRect = msg_send![self.nsview, convertRectToBacking:bounds];
+            Extent2D {
+                width: bounds.size.width as _,
+                height: bounds.size.height as _,
+            }
+        }
+    }
 }
 
+
 #[derive(Debug)]
 struct FrameInner {
     drawable: Option<metal::Drawable>,
     /// If there is a `drawable`, availability indicates if it's free for grabs.
     /// If there is `None`, `available == false` means that the frame has already
     /// been acquired and the `drawable` will appear at some point.
     available: bool,
+    /// Stays true for as long as the drawable is circulating through the
+    /// CAMetalLayer's frame queue.
+    linked: bool,
     last_frame: usize,
 }
 
@@ -128,16 +164,23 @@ impl Drop for Swapchain {
 impl Swapchain {
     /// Returns the drawable for the specified swapchain image index,
     /// marks the index as free for future use.
-    pub(crate) fn take_drawable(&self, index: hal::SwapImageIndex) -> metal::Drawable {
+    pub(crate) fn take_drawable(&self, index: hal::SwapImageIndex) -> Result<metal::Drawable, ()> {
         let mut frame = self
             .frames[index as usize]
             .inner
             .lock();
-        assert!(!frame.available);
-        frame.available = true;
-        frame.drawable
-            .take()
-            .expect("Drawable has not been acquired!")
+        assert!(!frame.available && frame.linked);
+
+        match frame.drawable.take() {
+            Some(drawable) => {
+                frame.available = true;
+                Ok(drawable)
+            }
+            None => {
+                frame.linked = false;
+                Err(())
+            }
+        }
     }
 
     fn signal_sync(&self, sync: hal::FrameSync<Backend>) {
@@ -175,10 +218,21 @@ impl SwapchainImage {
         }
         // wait for new frames to come until we meet the chosen one
         let mut count = 1;
-        while self.surface.next_frame(&self.frames).unwrap().0 != self.index as usize {
-            count += 1;
-        }
-        debug!("Swapchain image is ready after {} frames", count);
+        loop {
+            match self.surface.next_frame(&self.frames) {
+                Ok((index, _)) if index == self.index as usize => {
+                    debug!("Swapchain image is ready after {} frames", count);
+                    break
+                }
+                Ok(_) => {
+                    count += 1;
+                }
+                Err(_e) => {
+                    debug!("Swapchain drawables are changed");
+                    break
+                }
+            }
+        }        
         count
     }
 }
@@ -234,20 +288,6 @@ impl hal::Surface<Backend> for Surface {
     }
 }
 
-impl SurfaceInner {
-    fn dimensions(&self) -> Extent2D {
-        unsafe {
-            // NSView/UIView bounds are measured in DIPs
-            let bounds: CGRect = msg_send![self.view, bounds];
-            //let bounds_pixel: NSRect = msg_send![self.nsview, convertRectToBacking:bounds];
-            Extent2D {
-                width: bounds.size.width as _,
-                height: bounds.size.height as _,
-            }
-        }
-    }
-}
-
 impl Device {
     pub(crate) fn build_swapchain(
         &self,
@@ -313,6 +353,7 @@ impl Device {
                     inner: Mutex::new(FrameInner {
                         drawable,
                         available: true,
+                        linked: true,
                         last_frame: 0,
                     }),
                     texture: texture.to_owned(),
@@ -340,10 +381,9 @@ impl Device {
             extent: config.extent,
             last_frame: 0,
             image_ready_callbacks: Vec::new(),
-            acquire_mode: AcquireMode::Wait,
+            acquire_mode: AcquireMode::Oldest,
         };
 
-
         (swapchain, Backbuffer::Images(images))
     }
 }
@@ -404,6 +444,9 @@ impl hal::Swapchain<Backend> for Swapchain {
                 }
 
                 let frame = self.frames[oldest_index].inner.lock();
+                if !frame.linked {
+                    return Err(hal::AcquireError::OutOfDate);
+                }
                 (oldest_index, frame)
             }
         };