From 0833da33ba0aaa3c4b18532b85d8064be101cdc6 Mon Sep 17 00:00:00 2001 From: John Wells Date: Thu, 20 Oct 2022 08:33:30 -0400 Subject: [PATCH] Fix stuttering; add configurable frames-in-flight --- CHANGELOG.md | 2 + Cargo.toml | 2 +- src/display.rs | 130 ++++++++++------------------------- src/driver/descriptor_set.rs | 4 +- src/driver/image.rs | 11 ++- src/event_loop.rs | 17 ++++- 6 files changed, 64 insertions(+), 102 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 879663f6..8492e1b5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,10 +10,12 @@ adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). ### Fixed - Crash/device lost while resizing the window +- Inconsistent frame timing on certain drivers ### Added - Fullscreen demostration in `vsm_omni` example using F11 and F12 keys +- Configurable frames-in-flight setting ## [0.6.1] - 2022-10-16 diff --git a/Cargo.toml b/Cargo.toml index b013deea..ef122fac 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "screen-13" -version = "0.6.1" +version = "0.6.2" authors = ["John Wells "] edition = "2021" license = "MIT OR Apache-2.0" diff --git a/src/display.rs b/src/display.rs index b3cfbd45..d24223ea 100644 --- a/src/display.rs +++ b/src/display.rs @@ -19,20 +19,28 @@ use { /// A physical display interface. pub struct Display { - cmd_bufs: Vec<[CommandBuffer; 3]>, - device: Arc, + cmd_buf_idx: usize, + cmd_bufs: Vec, pool: Box, swapchain: Swapchain, } impl Display { /// Constructs a new `Display` object. - pub fn new(device: &Arc, pool: Box, swapchain: Swapchain) -> Self { - let device = Arc::clone(device); + pub fn new( + device: &Arc, + pool: Box, + swapchain: Swapchain, + cmd_buf_count: usize, + ) -> Self { + let mut cmd_bufs = Vec::with_capacity(cmd_buf_count); + for _ in 0..cmd_buf_count { + cmd_bufs.push(CommandBuffer::create(device, device.queue.family).unwrap()); + } Self { - device, - cmd_bufs: Default::default(), + cmd_buf_idx: 0, + cmd_bufs, pool, swapchain, } @@ -76,20 +84,11 @@ impl Display { .expect("uninitialized swapchain image: write something each frame!"); let mut resolver = render_graph.resolve(); let wait_dst_stage_mask = resolver.node_pipeline_stages(swapchain_image); - let swapchain_node = swapchain_image; - let swapchain_image = resolver.unbind_node(swapchain_node); - let swapchain_image_idx = swapchain_image.idx as usize; - - while self.cmd_bufs.len() <= swapchain_image_idx { - self.cmd_bufs.push([ - CommandBuffer::create(&self.device, self.device.queue.family)?, - CommandBuffer::create(&self.device, self.device.queue.family)?, - CommandBuffer::create(&self.device, self.device.queue.family)?, - ]); - } - let cmd_bufs = &mut self.cmd_bufs[swapchain_image_idx]; - let cmd_buf = &mut cmd_bufs[0]; + self.cmd_buf_idx += 1; + self.cmd_buf_idx %= self.cmd_bufs.len(); + + let cmd_buf = &mut self.cmd_bufs[self.cmd_buf_idx]; let started = Instant::now(); @@ -97,41 +96,14 @@ impl Display { Self::wait_for_fence(cmd_buf)?; } - let mut wait_elapsed = Instant::now() - started; - unsafe { Self::begin(cmd_buf)?; } - resolver.record_node_dependencies(&mut *self.pool, cmd_buf, swapchain_node)?; - - unsafe { - trace!("submitting swapchain dependencies"); - - // Record up to but not including the swapchain work - Self::submit( - cmd_buf, - vk::SubmitInfo::builder().command_buffers(from_ref(cmd_buf)), - )?; - } - - // Switch commnd buffers because we're going to be submitting with a wait semaphore on the - // swapchain image before we get access to record commands that use it - let cmd_buf = &mut cmd_bufs[1]; - - let wait_started = Instant::now(); - - unsafe { - Self::wait_for_fence(cmd_buf)?; - } - - wait_elapsed += Instant::now() - wait_started; - - unsafe { - Self::begin(cmd_buf)?; - } + // resolver.record_node_dependencies(&mut *self.pool, cmd_buf, swapchain_image)?; + resolver.record_node(&mut *self.pool, cmd_buf, swapchain_image)?; - resolver.record_node(&mut *self.pool, cmd_buf, swapchain_node)?; + let swapchain_image = resolver.unbind_node(swapchain_image); pipeline_barrier( &cmd_buf.device, @@ -157,9 +129,13 @@ impl Display { }), ); - unsafe { - trace!("submitting swapchain passes"); + // We may have unresolved nodes; things like copies that happen after present or operations + // before present which use nodes that are unused in the remainder of the graph. + // These operations are still important, but they don't need to wait for any of the above + // things so we do them last + resolver.record_unscheduled_passes(&mut *self.pool, cmd_buf)?; + unsafe { Self::submit( cmd_buf, vk::SubmitInfo::builder() @@ -170,49 +146,14 @@ impl Display { )?; } - let cmd_buf = &mut cmd_bufs[2]; - - let wait_started = Instant::now(); - - unsafe { - Self::wait_for_fence(cmd_buf)?; - } - - wait_elapsed += Instant::now() - wait_started; - - // We may have unresolved nodes; things like copies that happen after present or operations - // before present which use nodes that are unused in the remainder of the graph. - // These operations are still important, but they don't need to wait for any of the above - // things so we do them last - if !resolver.is_resolved() { - unsafe { - Self::begin(cmd_buf)?; - } - - resolver.record_unscheduled_passes(&mut *self.pool, cmd_buf)?; - - unsafe { - trace!("submitting unscheduled passes"); - - Self::submit( - cmd_buf, - vk::SubmitInfo::builder().command_buffers(from_ref(cmd_buf)), - ) - }?; - } - - let elapsed = Instant::now() - started - wait_elapsed; - trace!( - "🔜🔜🔜 vkQueueSubmit took {} Ξs (delay {} Ξs)", - elapsed.as_micros(), - wait_elapsed.as_micros() - ); + let elapsed = Instant::now() - started; + trace!("🔜🔜🔜 vkQueueSubmit took {} Ξs", elapsed.as_micros(),); self.swapchain.present_image(swapchain_image); // Store the resolved graph because it contains bindings, leases, and other shared resources // that need to be kept alive until the fence is waited upon. - CommandBuffer::push_fenced_drop(&mut self.cmd_bufs[swapchain_image_idx][2], resolver); + CommandBuffer::push_fenced_drop(cmd_buf, resolver); Ok(()) } @@ -227,10 +168,6 @@ impl Display { .device .end_command_buffer(**cmd_buf) .map_err(|_| ())?; - cmd_buf - .device - .reset_fences(from_ref(&cmd_buf.fence)) - .map_err(|_| ())?; cmd_buf .device .queue_submit( @@ -242,10 +179,15 @@ impl Display { } unsafe fn wait_for_fence(cmd_buf: &mut CommandBuffer) -> Result<(), ()> { + use std::slice::from_ref; + Device::wait_for_fence(&cmd_buf.device, &cmd_buf.fence).map_err(|_| ())?; CommandBuffer::drop_fenced(cmd_buf); - Ok(()) + cmd_buf + .device + .reset_fences(from_ref(&cmd_buf.fence)) + .map_err(|_| ()) } } diff --git a/src/driver/descriptor_set.rs b/src/driver/descriptor_set.rs index e0881155..9a06ef8c 100644 --- a/src/driver/descriptor_set.rs +++ b/src/driver/descriptor_set.rs @@ -1,7 +1,7 @@ use { super::{DescriptorSetLayout, Device, DriverError}, ash::vk, - log::{trace, warn}, + log::warn, std::{ops::Deref, sync::Arc, thread::panicking}, }; @@ -157,8 +157,6 @@ impl DescriptorPool { .set_layouts(from_ref(layout)); create_info.descriptor_set_count = count; - trace!("allocate_descriptor_sets"); - Ok(unsafe { this.device .allocate_descriptor_sets(&create_info) diff --git a/src/driver/image.rs b/src/driver/image.rs index 5fcea9ab..49cefe9c 100644 --- a/src/driver/image.rs +++ b/src/driver/image.rs @@ -13,6 +13,7 @@ use { std::{ collections::{hash_map::Entry, HashMap}, fmt::{Debug, Formatter}, + mem::take, ops::Deref, ptr::null, sync::{ @@ -244,11 +245,15 @@ impl Image { } pub(super) fn clone_raw(this: &Self) -> Self { + // Moves the image view cache from the current instance to the clone! + let mut image_view_cache = this.image_view_cache.lock(); + let image_view_cache = take(&mut *image_view_cache); + Self { allocation: None, device: Arc::clone(&this.device), image: this.image, - image_view_cache: Mutex::new(Default::default()), + image_view_cache: Mutex::new(image_view_cache), info: this.info, name: this.name.clone(), prev_access: AtomicU8::new(access_type_into_u8(AccessType::Nothing)), @@ -307,11 +312,11 @@ impl Drop for Image { return; } - self.image_view_cache.lock().clear(); - // When our allocation is some we allocated ourself; otherwise somebody // else owns this image and we should not destroy it. Usually it's the swapchain... if let Some(allocation) = self.allocation.take() { + self.image_view_cache.lock().clear(); + unsafe { self.device.destroy_image(self.image, None); } diff --git a/src/event_loop.rs b/src/event_loop.rs index 6b00c182..94461681 100644 --- a/src/event_loop.rs +++ b/src/event_loop.rs @@ -187,6 +187,7 @@ impl AsRef> for EventLoop { /// Builder for `EventLoop`. pub struct EventLoopBuilder { + cmd_buf_count: usize, driver_cfg: DriverConfigBuilder, event_loop: winit::event_loop::EventLoop<()>, resolver_pool: Option>, @@ -202,6 +203,7 @@ impl Debug for EventLoopBuilder { impl Default for EventLoopBuilder { fn default() -> Self { Self { + cmd_buf_count: 5, driver_cfg: DriverConfigBuilder::default(), event_loop: winit::event_loop::EventLoop::new(), resolver_pool: None, @@ -216,6 +218,19 @@ impl EventLoopBuilder { self.event_loop.available_monitors() } + /// Specifies the number of in-flight command buffers, which should be greater + /// than or equal to the desired swapchain image count. + /// + /// More command buffers mean less time waiting for previously submitted frames to complete, but + /// more memory in use. + /// + /// Generally a value of one or two greater than desired image count produces the smoothest + /// animation. + pub fn command_buffer_count(mut self, cmd_buf_count: usize) -> Self { + self.cmd_buf_count = cmd_buf_count; + self + } + /// Provides a closure which configures the `DriverConfig` instance. pub fn configure(mut self, configure_fn: ConfigureFn) -> Self where @@ -336,7 +351,7 @@ impl EventLoopBuilder { let pool = self .resolver_pool .unwrap_or_else(|| Box::new(HashPool::new(&driver.device))); - let display = Display::new(&driver.device, pool, driver.swapchain); + let display = Display::new(&driver.device, pool, driver.swapchain, self.cmd_buf_count); info!( "display resolution: {}x{} ({}x scale)",