Merge #466

466: Refactored swapchain tracking r=sibling a=kvark

This is a sibling of #465 for master.
Fixes #227

Co-authored-by: Dzmitry Malyshau <kvarkus@gmail.com>

ffi/wgpu.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.12.1 */
+/* Generated with cbindgen:0.12.2 */
 
 /* DO NOT MODIFY THIS MANUALLY! This file was generated using cbindgen.
  * To generate this file:
@@ -732,6 +732,13 @@ void wgpu_command_buffer_destroy(WGPUCommandBufferId command_buffer_id);
 WGPURawPass *wgpu_command_encoder_begin_compute_pass(WGPUCommandEncoderId encoder_id,
                                                      const WGPUComputePassDescriptor *_desc);
 
+/**
+ * # Safety
+ *
+ * This function is unsafe as there is no guarantee that the given pointer
+ * (`RenderPassDescriptor::color_attachments`) is valid for
+ * `RenderPassDescriptor::color_attachments_length` elements.
+ */
 WGPURawRenderPass *wgpu_command_encoder_begin_render_pass(WGPUCommandEncoderId encoder_id,
                                                           const WGPURenderPassDescriptor *desc);
 
@@ -771,6 +778,13 @@ void wgpu_compute_pass_dispatch_indirect(WGPURawPass *pass,
                                          WGPUBufferId buffer_id,
                                          WGPUBufferAddress offset);
 
+/**
+ * # Safety
+ *
+ * This function is unsafe because improper use may lead to memory
+ * problems. For example, a double-free may occur if the function is called
+ * twice on the same raw pointer.
+ */
 void wgpu_compute_pass_end_pass(WGPUComputePassId pass_id);
 
 void wgpu_compute_pass_insert_debug_marker(WGPURawPass *_pass, WGPURawString _label);
@@ -779,6 +793,12 @@ void wgpu_compute_pass_pop_debug_group(WGPURawPass *_pass);
 
 void wgpu_compute_pass_push_debug_group(WGPURawPass *_pass, WGPURawString _label);
 
+/**
+ * # Safety
+ *
+ * This function is unsafe as there is no guarantee that the given pointer is
+ * valid for `offset_length` elements.
+ */
 void wgpu_compute_pass_set_bind_group(WGPURawPass *pass,
                                       uint32_t index,
                                       WGPUBindGroupId bind_group_id,
@@ -803,6 +823,12 @@ WGPUBindGroupLayoutId wgpu_device_create_bind_group_layout(WGPUDeviceId device_i
 
 WGPUBufferId wgpu_device_create_buffer(WGPUDeviceId device_id, const WGPUBufferDescriptor *desc);
 
+/**
+ * # Safety
+ *
+ * This function is unsafe as there is no guarantee that the given pointer
+ * dereferenced in this function is valid.
+ */
 WGPUBufferId wgpu_device_create_buffer_mapped(WGPUDeviceId device_id,
                                               const WGPUBufferDescriptor *desc,
                                               uint8_t **mapped_ptr_out);
@@ -838,6 +864,12 @@ WGPUQueueId wgpu_device_get_queue(WGPUDeviceId device_id);
 
 void wgpu_device_poll(WGPUDeviceId device_id, bool force_wait);
 
+/**
+ * # Safety
+ *
+ * This function is unsafe as there is no guarantee that the given pointer is
+ * valid for `command_buffers_length` elements.
+ */
 void wgpu_queue_submit(WGPUQueueId queue_id,
                        const WGPUCommandBufferId *command_buffers,
                        uintptr_t command_buffers_length);
@@ -863,6 +895,13 @@ void wgpu_render_pass_draw_indirect(WGPURawRenderPass *pass,
                                     WGPUBufferId buffer_id,
                                     WGPUBufferAddress offset);
 
+/**
+ * # Safety
+ *
+ * This function is unsafe because improper use may lead to memory
+ * problems. For example, a double-free may occur if the function is called
+ * twice on the same raw pointer.
+ */
 void wgpu_render_pass_end_pass(WGPURenderPassId pass_id);
 
 void wgpu_render_pass_execute_bundles(WGPURawRenderPass *_pass,
@@ -875,6 +914,12 @@ void wgpu_render_pass_pop_debug_group(WGPURawRenderPass *_pass);
 
 void wgpu_render_pass_push_debug_group(WGPURawRenderPass *_pass, WGPURawString _label);
 
+/**
+ * # Safety
+ *
+ * This function is unsafe as there is no guarantee that the given pointer is
+ * valid for `offset_length` elements.
+ */
 void wgpu_render_pass_set_bind_group(WGPURawRenderPass *pass,
                                      uint32_t index,
                                      WGPUBindGroupId bind_group_id,
@@ -897,6 +942,12 @@ void wgpu_render_pass_set_scissor_rect(WGPURawRenderPass *pass,
 
 void wgpu_render_pass_set_stencil_reference(WGPURawRenderPass *pass, uint32_t value);
 
+/**
+ * # Safety
+ *
+ * This function is unsafe as there is no guarantee that the given pointers
+ * (`buffer_ids` and `offsets`) are valid for `length` elements.
+ */
 void wgpu_render_pass_set_vertex_buffers(WGPURawRenderPass *pass,
                                          uint32_t start_slot,
                                          const WGPUBufferId *buffer_ids,
@@ -911,6 +962,11 @@ void wgpu_render_pass_set_viewport(WGPURawRenderPass *pass,
                                    float depth_min,
                                    float depth_max);
 
+/**
+ * # Safety
+ *
+ * This function is unsafe as it calls an unsafe extern callback.
+ */
 void wgpu_request_adapter_async(const WGPURequestAdapterOptions *desc,
                                 WGPUBackendBit mask,
                                 WGPURequestAdapterCallback callback,

wgpu-core/src/command/mod.rs

@@ -136,7 +136,7 @@ pub struct CommandBuffer<B: hal::Backend> {
     pub(crate) device_id: Stored<id::DeviceId>,
     pub(crate) life_guard: LifeGuard,
     pub(crate) trackers: TrackerSet,
-    pub(crate) used_swap_chain: Option<(Stored<id::TextureViewId>, B::Framebuffer)>,
+    pub(crate) used_swap_chain: Option<(Stored<id::SwapChainId>, B::Framebuffer)>,
     pub(crate) features: Features,
 }
 
@@ -263,13 +263,17 @@ impl<F> Global<F> {
     ) -> id::CommandBufferId {
         let hub = B::hub(self);
         let mut token = Token::root();
+        let (swap_chain_guard, mut token) = hub.swap_chains.read(&mut token);
         //TODO: actually close the last recorded command buffer
         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 view_id, _)) = comb.used_swap_chain {
+        if let Some((ref sc_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 {:?} {:#?}", encoder_id, comb.trackers);

wgpu-core/src/command/render.rs

@@ -325,7 +325,7 @@ impl<F> Global<F> {
             let base_trackers = &cmb.trackers;
 
             let mut extent = None;
-            let mut used_swap_chain_image = None::<Stored<id::TextureViewId>>;
+            let mut used_swap_chain = None::<Stored<id::SwapChainId>>;
 
             let sample_count = color_attachments
                 .get(0)
@@ -430,15 +430,12 @@ impl<F> Global<F> {
                             };
                             old_layout .. hal::image::Layout::ColorAttachmentOptimal
                         }
-                        TextureViewInner::SwapChain { .. } => {
-                            if let Some((ref view_id, _)) = cmb.used_swap_chain {
-                                assert_eq!(view_id.value, at.attachment);
+                        TextureViewInner::SwapChain { ref source_id, .. } => {
+                            if let Some((ref sc_id, _)) = cmb.used_swap_chain {
+                                assert_eq!(source_id.value, sc_id.value);
                             } else {
-                                assert!(used_swap_chain_image.is_none());
-                                used_swap_chain_image = Some(Stored {
-                                    value: at.attachment,
-                                    ref_count: view.life_guard.add_ref(),
-                                });
+                                assert!(used_swap_chain.is_none());
+                                used_swap_chain = Some(source_id.clone());
                             }
 
                             let end = hal::image::Layout::Present;
@@ -490,15 +487,12 @@ impl<F> Global<F> {
                             };
                             old_layout .. hal::image::Layout::ColorAttachmentOptimal
                         }
-                        TextureViewInner::SwapChain { .. } => {
-                            if let Some((ref view_id, _)) = cmb.used_swap_chain {
-                                assert_eq!(view_id.value, resolve_target);
+                        TextureViewInner::SwapChain { ref source_id, .. } => {
+                            if let Some((ref sc_id, _)) = cmb.used_swap_chain {
+                                assert_eq!(source_id.value, sc_id.value);
                             } else {
-                                assert!(used_swap_chain_image.is_none());
-                                used_swap_chain_image = Some(Stored {
-                                    value: resolve_target,
-                                    ref_count: view.life_guard.add_ref(),
-                                });
+                                assert!(used_swap_chain.is_none());
+                                used_swap_chain = Some(source_id.clone());
                             }
 
                             let end = hal::image::Layout::Present;
@@ -624,8 +618,8 @@ impl<F> Global<F> {
                 depth_stencil: depth_stencil_attachment.map(|at| at.attachment),
             };
 
-            let framebuffer = match used_swap_chain_image.take() {
-                Some(view_id) => {
+            let framebuffer = match used_swap_chain.take() {
+                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 {
@@ -638,7 +632,7 @@ impl<F> Global<F> {
                             .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-core/src/conv.rs

@@ -516,6 +516,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;
     }

wgpu-core/src/device/mod.rs

@@ -31,6 +31,7 @@ use hal::{
 use parking_lot::{Mutex, MutexGuard};
 use rendy_descriptor::{DescriptorAllocator, DescriptorRanges};
 use rendy_memory::{Block, Heaps};
+use smallvec::SmallVec;
 
 use std::{
     collections::hash_map::Entry,
@@ -668,7 +669,7 @@ impl<F: IdentityFilter<id::TextureId>> Global<F> {
             texture.life_guard.ref_count.take();
             texture.device_id.value
         };
-        
+
         let (device_guard, mut token) = hub.devices.read(&mut token);
         device_guard[device_id]
             .lock_life(&mut token)
@@ -1309,16 +1310,16 @@ impl<F: AllIdentityFilter + IdentityFilter<id::CommandBufferId>> Global<F> {
                 .submission_index
                 .fetch_add(1, Ordering::Relaxed);
 
-            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 (mut command_buffer_guard, mut token) = hub.command_buffers.write(&mut token);
             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);
 
             //Note: locking the trackers has to be done after the storages
-            let mut signal_semaphores = Vec::new();
+            let mut signal_swapchain_semaphores = SmallVec::<[_; 1]>::new();
             let mut trackers = device.trackers.lock();
 
             //TODO: if multiple command buffers are submitted, we can re-use the last
@@ -1329,21 +1330,12 @@ impl<F: AllIdentityFilter + IdentityFilter<id::CommandBufferId>> Global<F> {
             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() {
-                    match texture_view_guard[view_id.value].inner {
-                        resource::TextureViewInner::Native { .. } => unreachable!(),
-                        resource::TextureViewInner::SwapChain {
-                            ref source_id,
-                            ref mut framebuffers,
-                            ..
-                        } => {
-                            if framebuffers.is_empty() {
-                                let sem = &swap_chain_guard[source_id.value].semaphore;
-                                signal_semaphores.push(sem);
-                            }
-                            framebuffers.push(fbo);
-                        }
-                    };
+                if let Some((sc_id, fbo)) = comb.used_swap_chain.take() {
+                    let sc = &mut swap_chain_guard[sc_id.value];
+                    if sc.acquired_framebuffers.is_empty() {
+                        signal_swapchain_semaphores.push(sc_id.value);
+                    }
+                    sc.acquired_framebuffers.push(fbo);
                 }
 
                 // optimize the tracked states
@@ -1405,12 +1397,15 @@ impl<F: AllIdentityFilter + IdentityFilter<id::CommandBufferId>> Global<F> {
 
             // 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 {
@@ -1853,6 +1848,7 @@ impl<F: IdentityFilter<id::SwapChainId>> Global<F> {
             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-core/src/hub.rs

@@ -165,6 +165,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-core/src/instance.rs

@@ -379,6 +379,9 @@ impl<F: IdentityFilter<DeviceId>> Global<F> {
         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
@@ -388,7 +391,10 @@ impl<F: IdentityFilter<DeviceId>> Global<F> {
             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-core/src/resource.rs

@@ -14,7 +14,6 @@ use crate::{
 
 use hal;
 use rendy_memory::MemoryBlock;
-use smallvec::SmallVec;
 #[cfg(feature = "serde")]
 use serde::{Deserialize, Serialize};
 
@@ -307,7 +306,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-core/src/swap_chain.rs

@@ -45,8 +45,6 @@ use crate::{
 
 use hal::{self, device::Device as _, queue::CommandQueue as _, window::PresentationSurface as _};
 
-use smallvec::SmallVec;
-
 
 const FRAME_TIMEOUT_MS: u64 = 1000;
 pub const DESIRED_NUM_FRAMES: u32 = 3;
@@ -59,6 +57,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)]
@@ -169,7 +168,6 @@ impl<F: IdentityFilter<TextureViewId>> Global<F> {
                     value: swap_chain_id,
                     ref_count: sc.life_guard.add_ref(),
                 },
-                framebuffers: SmallVec::new(),
             },
             format: sc.desc.format,
             extent: hal::image::Extent {
@@ -218,13 +216,9 @@ impl<F: IdentityFilter<TextureViewId>> Global<F> {
             .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 {
-                image,
-                framebuffers,
-                ..
-            } => (image, framebuffers),
+            resource::TextureViewInner::SwapChain { image, .. } => image,
         };
 
         let err = unsafe {
@@ -235,7 +229,7 @@ impl<F: IdentityFilter<TextureViewId>> Global<F> {
             log::warn!("present failed: {:?}", e);
         }
 
-        for fbo in framebuffers {
+        for fbo in sc.acquired_framebuffers.drain(..) {
             unsafe {
                 device.raw.destroy_framebuffer(fbo);
             }

wgpu-native/Cargo.toml

@@ -32,5 +32,5 @@ parking_lot = "0.9"
 raw-window-handle = "0.3"
 libc = {version="0.2", features=[]}
 
-[target.'cfg(target_os = "macos")'.dependencies.objc]
+[target.'cfg(target_os = "macos")'.dependencies]
 objc = "0.2.7"