Merge #465

465: [0.4] Refactor the swap chain frame tracking r=kvark a=kvark

On the surface, it gets us from "key not present" to "Used swap chain frame has already presented" in the case of an application presenting before submitting a command buffer. But the worst thing is that we used to lock up the driver on that presentation, so now the problem is much less mysterious.
cc @snuk182 

Fixes #458
Works around #227 

Co-authored-by: Dzmitry Malyshau <kvarkus@gmail.com>
Co-authored-by: Korijn van Golen <k.vangolen@clinicalgraphics.com>

.travis.yml

@@ -54,6 +54,8 @@ branches:
 
 before_install:
 - if [[ $TRAVIS_OS_NAME == "windows" ]]; then choco install make; fi
+- if [[ $TRAVIS_RUST_VERSION != "nightly" ]] && [[ $TRAVIS_OS_NAME == "windows" ]]; then rustup default stable-msvc; fi
+- if [[ $TRAVIS_RUST_VERSION == "nightly" ]] && [[ $TRAVIS_OS_NAME == "windows" ]]; then rustup default nightly-msvc; fi
 
 script:
   - cargo test

CHANGELOG.md

@@ -1,5 +1,8 @@
 # Change Log
 
+## v0.4.3 (20-01-2020)
+  - improved swap chain error handling
+
 ## v0.4.2 (15-12-2019)
   - fixed render pass transitions
 

Cargo.lock

@@ -620,7 +620,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index"
 
 [[package]]
 name = "wgpu-native"
-version = "0.4.1"
+version = "0.4.3"
 dependencies = [
  "arrayvec 0.5.0 (registry+https://github.com/rust-lang/crates.io-index)",
  "bitflags 1.0.4 (registry+https://github.com/rust-lang/crates.io-index)",
@@ -649,7 +649,7 @@ version = "0.1.0"
 dependencies = [
  "log 0.4.6 (registry+https://github.com/rust-lang/crates.io-index)",
  "parking_lot 0.9.0 (registry+https://github.com/rust-lang/crates.io-index)",
- "wgpu-native 0.4.1",
+ "wgpu-native 0.4.3",
 ]
 
 [[package]]

ffi/wgpu-remote.h

@@ -2,7 +2,7 @@
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
-/* Generated with cbindgen:0.9.1 */
+/* Generated with cbindgen:0.12.2 */
 
 /* DO NOT MODIFY THIS MANUALLY! This file was generated using cbindgen.
  * To generate this file:

ffi/wgpu.h

@@ -1,7 +1,7 @@
 #define WGPU_LOCAL
 
 
-/* Generated with cbindgen:0.9.1 */
+/* Generated with cbindgen:0.12.2 */
 
 #include <stdarg.h>
 #include <stdbool.h>

wgpu-native/Cargo.toml

@@ -1,6 +1,6 @@
 [package]
 name = "wgpu-native"
-version = "0.4.2"
+version = "0.4.3"
 authors = [
 	"Dzmitry Malyshau <kvark@mozilla.com>",
 	"Joshua Groves <josh@joshgroves.com>",

wgpu-native/src/command/mod.rs

@@ -37,6 +37,7 @@ use crate::{
     LifeGuard,
     RenderPassId,
     Stored,
+    SwapChainId,
     Texture,
     TextureId,
     TextureUsage,
@@ -115,7 +116,7 @@ pub struct CommandBuffer<B: hal::Backend> {
     pub(crate) device_id: Stored<DeviceId>,
     pub(crate) life_guard: LifeGuard,
     pub(crate) trackers: TrackerSet,
-    pub(crate) used_swap_chain: Option<(Stored<TextureViewId>, B::Framebuffer)>,
+    pub(crate) used_swap_chain: Option<(Stored<SwapChainId>, B::Framebuffer)>,
     pub(crate) features: Features,
 }
 
@@ -193,12 +194,15 @@ pub fn command_encoder_finish<B: GfxBackend>(
     let hub = B::hub(global);
     let mut token = Token::root();
     //TODO: actually close the last recorded command buffer
+    let (swap_chain_guard, mut token) = hub.swap_chains.read(&mut token);
     let (mut comb_guard, _) = hub.command_buffers.write(&mut token);
     let comb = &mut comb_guard[encoder_id];
     assert!(comb.is_recording);
     comb.is_recording = false;
-    // stop tracking the swapchain image, if used
+    if let Some((ref sc_id, _)) = comb.used_swap_chain {
-    if let Some((ref view_id, _)) = comb.used_swap_chain {
+        let view_id = swap_chain_guard[sc_id.value].acquired_view_id
+            .as_ref()
+            .expect("Used swap chain frame has already presented");
         comb.trackers.views.remove(view_id.value);
     }
     log::debug!("Command buffer {:?} tracker: {:#?}", encoder_id, comb.trackers);
@@ -251,7 +255,7 @@ pub fn command_encoder_begin_render_pass<B: GfxBackend>(
 
         let mut extent = None;
         let mut barriers = Vec::new();
-        let mut used_swap_chain_image = None::<Stored<TextureViewId>>;
+        let mut used_swap_chain = None::<Stored<SwapChainId>>;
 
         let color_attachments =
             unsafe { slice::from_raw_parts(desc.color_attachments, desc.color_attachments_length) };
@@ -388,15 +392,12 @@ pub fn command_encoder_begin_render_pass<B: GfxBackend>(
                         };
                         old_layout .. hal::image::Layout::ColorAttachmentOptimal
                     }
-                    TextureViewInner::SwapChain { .. } => {
+                    TextureViewInner::SwapChain { ref source_id, .. } => {
-                        if let Some((ref view_id, _)) = cmb.used_swap_chain {
+                        if let Some((ref sc_id, _)) = cmb.used_swap_chain {
-                            assert_eq!(view_id.value, at.attachment);
+                            assert_eq!(source_id.value, sc_id.value);
                         } else {
-                            assert!(used_swap_chain_image.is_none());
+                            assert!(used_swap_chain.is_none());
-                            used_swap_chain_image = Some(Stored {
+                            used_swap_chain = Some(source_id.clone());
-                                value: at.attachment,
-                                ref_count: view.life_guard.ref_count.clone(),
-                            });
                         }
 
                         let end = hal::image::Layout::Present;
@@ -468,15 +469,12 @@ pub fn command_encoder_begin_render_pass<B: GfxBackend>(
                         };
                         old_layout .. hal::image::Layout::ColorAttachmentOptimal
                     }
-                    TextureViewInner::SwapChain { .. } => {
+                    TextureViewInner::SwapChain { ref source_id, .. } => {
-                        if let Some((ref view_id, _)) = cmb.used_swap_chain {
+                        if let Some((ref sc_id, _)) = cmb.used_swap_chain {
-                            assert_eq!(view_id.value, resolve_target);
+                            assert_eq!(sc_id.value, source_id.value);
                         } else {
-                            assert!(used_swap_chain_image.is_none());
+                            assert!(used_swap_chain.is_none());
-                            used_swap_chain_image = Some(Stored {
+                            used_swap_chain = Some(source_id.clone());
-                                value: resolve_target,
-                                ref_count: view.life_guard.ref_count.clone(),
-                            });
                         }
 
                         let end = hal::image::Layout::Present;
@@ -587,8 +585,8 @@ pub fn command_encoder_begin_render_pass<B: GfxBackend>(
             depth_stencil: depth_stencil_attachment.map(|at| at.attachment),
         };
 
-        let framebuffer = match used_swap_chain_image.take() {
+        let framebuffer = match used_swap_chain.take() {
-            Some(view_id) => {
+            Some(sc_id) => {
                 assert!(cmb.used_swap_chain.is_none());
                 // Always create a new framebuffer and delete it after presentation.
                 let attachments = fb_key.all().map(|&id| match view_guard[id].inner {
@@ -601,7 +599,7 @@ pub fn command_encoder_begin_render_pass<B: GfxBackend>(
                         .create_framebuffer(&render_pass, attachments, extent.unwrap())
                         .unwrap()
                 };
-                cmb.used_swap_chain = Some((view_id, framebuffer));
+                cmb.used_swap_chain = Some((sc_id, framebuffer));
                 &mut cmb.used_swap_chain.as_mut().unwrap().1
             }
             None => {

wgpu-native/src/conv.rs

@@ -35,7 +35,7 @@ pub fn map_buffer_usage(
     if usage.contains(W::UNIFORM) {
         hal_usage |= U::UNIFORM;
     }
-    if usage.contains(W::STORAGE) {
+    if usage.intersects(W::STORAGE | W::STORAGE_READ) {
         hal_usage |= U::STORAGE;
     }
     if usage.contains(W::INDIRECT) {
@@ -514,6 +514,9 @@ pub fn map_buffer_state(usage: resource::BufferUsage) -> hal::buffer::State {
     if usage.contains(W::UNIFORM) {
         access |= A::UNIFORM_READ | A::SHADER_READ;
     }
+    if usage.contains(W::STORAGE_READ) {
+        access |= A::SHADER_READ;
+    }
     if usage.contains(W::STORAGE) {
         access |= A::SHADER_WRITE;
     }
@@ -550,7 +553,7 @@ pub fn map_texture_state(
         access |= A::SHADER_READ;
     }
     if usage.contains(W::STORAGE) {
-        access |= A::SHADER_WRITE;
+        access |= A::SHADER_READ | A::SHADER_WRITE;
     }
     if usage.contains(W::OUTPUT_ATTACHMENT) {
         //TODO: read-only attachments

wgpu-native/src/device.rs

@@ -59,6 +59,7 @@ use hal::{
 use parking_lot::Mutex;
 use rendy_descriptor::{DescriptorAllocator, DescriptorRanges, DescriptorSet};
 use rendy_memory::{Block, Heaps, MemoryBlock};
+use smallvec::SmallVec;
 
 #[cfg(feature = "local")]
 use std::marker::PhantomData;
@@ -1546,11 +1547,11 @@ pub fn queue_submit<B: GfxBackend>(
     let (submit_index, fence) = {
         let mut token = Token::root();
         let (mut device_guard, mut token) = hub.devices.write(&mut token);
-        let (swap_chain_guard, mut token) = hub.swap_chains.read(&mut token);
+        let (mut swap_chain_guard, mut token) = hub.swap_chains.write(&mut token);
         let device = &mut device_guard[queue_id];
 
         let mut trackers = device.trackers.lock();
-        let mut signal_semaphores = Vec::new();
+        let mut signal_swapchain_semaphores = SmallVec::<[_; 1]>::new();
 
         let submit_index = 1 + device
             .life_guard
@@ -1561,7 +1562,7 @@ pub fn queue_submit<B: GfxBackend>(
         let (bind_group_guard, mut token) = hub.bind_groups.read(&mut token);
         let (buffer_guard, mut token) = hub.buffers.read(&mut token);
         let (texture_guard, mut token) = hub.textures.read(&mut token);
-        let (mut texture_view_guard, mut token) = hub.texture_views.write(&mut token);
+        let (texture_view_guard, mut token) = hub.texture_views.read(&mut token);
         let (sampler_guard, _) = hub.samplers.read(&mut token);
 
         //TODO: if multiple command buffers are submitted, we can re-use the last
@@ -1572,21 +1573,12 @@ pub fn queue_submit<B: GfxBackend>(
         for &cmb_id in command_buffer_ids {
             let comb = &mut command_buffer_guard[cmb_id];
 
-            if let Some((view_id, fbo)) = comb.used_swap_chain.take() {
+            if let Some((sc_id, fbo)) = comb.used_swap_chain.take() {
-                match texture_view_guard[view_id.value].inner {
+                let sc = &mut swap_chain_guard[sc_id.value];
-                    resource::TextureViewInner::Native { .. } => unreachable!(),
+                if sc.acquired_framebuffers.is_empty() {
-                    resource::TextureViewInner::SwapChain {
+                    signal_swapchain_semaphores.push(sc_id.value);
-                        ref source_id,
+                }
-                        ref mut framebuffers,
+                sc.acquired_framebuffers.push(fbo);
-                        ..
-                    } => {
-                        if framebuffers.is_empty() {
-                            let sem = &swap_chain_guard[source_id.value].semaphore;
-                            signal_semaphores.push(sem);
-                        }
-                        framebuffers.push(fbo);
-                    }
-                };
             }
 
             // optimize the tracked states
@@ -1656,12 +1648,14 @@ pub fn queue_submit<B: GfxBackend>(
 
         // now prepare the GPU submission
         let fence = device.raw.create_fence(false).unwrap();
-        let submission = hal::queue::Submission::<_, _, Vec<&B::Semaphore>> {
+        let submission = hal::queue::Submission {
             command_buffers: command_buffer_ids
                 .iter()
                 .flat_map(|&cmb_id| &command_buffer_guard[cmb_id].raw),
             wait_semaphores: Vec::new(),
-            signal_semaphores,
+            signal_semaphores: signal_swapchain_semaphores
+                .into_iter()
+                .map(|sc_id| &swap_chain_guard[sc_id].semaphore),
         };
 
         unsafe {
@@ -2115,6 +2109,7 @@ pub fn device_create_swap_chain<B: GfxBackend>(
         num_frames,
         semaphore: device.raw.create_semaphore().unwrap(),
         acquired_view_id: None,
+        acquired_framebuffers: Vec::new(),
     };
     swap_chain_guard.insert(sc_id, swap_chain);
     sc_id

wgpu-native/src/hub.rs

@@ -179,6 +179,7 @@ impl<B: hal::Backend> Access<Adapter<B>> for Surface {}
 impl<B: hal::Backend> Access<Device<B>> for Root {}
 impl<B: hal::Backend> Access<Device<B>> for Surface {}
 impl<B: hal::Backend> Access<Device<B>> for Adapter<B> {}
+impl<B: hal::Backend> Access<SwapChain<B>> for Root {}
 impl<B: hal::Backend> Access<SwapChain<B>> for Device<B> {}
 impl<B: hal::Backend> Access<PipelineLayout<B>> for Root {}
 impl<B: hal::Backend> Access<PipelineLayout<B>> for Device<B> {}

wgpu-native/src/instance.rs

@@ -22,7 +22,7 @@ use bitflags::bitflags;
 #[cfg(feature = "serde")]
 use serde::{Deserialize, Serialize};
 
-use hal::{self, adapter::PhysicalDevice as _, queue::QueueFamily as _, Instance as _};
+use hal::{adapter::PhysicalDevice as _, queue::QueueFamily as _, Instance as _};
 #[cfg(feature = "local")]
 use std::marker::PhantomData;
 
@@ -485,6 +485,9 @@ pub fn adapter_request_device<B: GfxBackend>(
     let device = {
         let (adapter_guard, _) = hub.adapters.read(&mut token);
         let adapter = &adapter_guard[adapter_id].raw;
+        let wishful_features =
+            hal::Features::VERTEX_STORES_AND_ATOMICS |
+            hal::Features::FRAGMENT_STORES_AND_ATOMICS;
 
         let family = adapter
             .queue_families
@@ -494,7 +497,10 @@ pub fn adapter_request_device<B: GfxBackend>(
         let mut gpu = unsafe {
             adapter
                 .physical_device
-                .open(&[(family, &[1.0])], hal::Features::empty())
+                .open(
+                    &[(family, &[1.0])],
+                    adapter.physical_device.features() & wishful_features,
+                )
                 .unwrap()
         };
 

wgpu-native/src/resource.rs

@@ -18,7 +18,6 @@ use crate::{
 use bitflags::bitflags;
 use hal;
 use rendy_memory::MemoryBlock;
-use smallvec::SmallVec;
 
 use std::borrow::Borrow;
 
@@ -275,7 +274,6 @@ pub(crate) enum TextureViewInner<B: hal::Backend> {
     SwapChain {
         image: <B::Surface as hal::window::PresentationSurface<B>>::SwapchainImage,
         source_id: Stored<SwapChainId>,
-        framebuffers: SmallVec<[B::Framebuffer; 1]>,
     },
 }
 

wgpu-native/src/swap_chain.rs

@@ -50,8 +50,6 @@ use crate::{gfx_select, hub::GLOBAL};
 
 use hal::{self, device::Device as _, queue::CommandQueue as _, window::PresentationSurface as _};
 
-use smallvec::SmallVec;
-
 #[cfg(feature = "local")]
 use std::marker::PhantomData;
 
@@ -67,6 +65,7 @@ pub struct SwapChain<B: hal::Backend> {
     pub(crate) num_frames: hal::window::SwapImageIndex,
     pub(crate) semaphore: B::Semaphore,
     pub(crate) acquired_view_id: Option<Stored<TextureViewId>>,
+    pub(crate) acquired_framebuffers: Vec<B::Framebuffer>,
 }
 
 #[repr(C)]
@@ -176,7 +175,6 @@ pub fn swap_chain_get_next_texture<B: GfxBackend>(
                 value: swap_chain_id,
                 ref_count: sc.life_guard.ref_count.clone(),
             },
-            framebuffers: SmallVec::new(),
         },
         format: sc.desc.format,
         extent: hal::image::Extent {
@@ -232,11 +230,9 @@ pub fn swap_chain_present<B: GfxBackend>(global: &Global, swap_chain_id: SwapCha
         .take()
         .expect("Swap chain image is not acquired");
     let (view, _) = hub.texture_views.unregister(view_id.value, &mut token);
-    let (image, framebuffers) = match view.inner {
+    let image = match view.inner {
         resource::TextureViewInner::Native { .. } => unreachable!(),
-        resource::TextureViewInner::SwapChain {
+        resource::TextureViewInner::SwapChain { image, .. } => image,
-            image, framebuffers, ..
-        } => (image, framebuffers),
     };
 
     let err = unsafe {
@@ -247,7 +243,7 @@ pub fn swap_chain_present<B: GfxBackend>(global: &Global, swap_chain_id: SwapCha
         log::warn!("present failed: {:?}", e);
     }
 
-    for fbo in framebuffers {
+    for fbo in sc.acquired_framebuffers.drain(..) {
         unsafe {
             device.raw.destroy_framebuffer(fbo);
         }