From c299a41a67364ed6361e79a88f13da2dacce5d03 Mon Sep 17 00:00:00 2001 From: enricobuehler Date: Tue, 7 Jul 2026 22:20:15 +0200 Subject: [PATCH] =?UTF-8?q?fix(presenter):=20resize=20crash=20=E2=80=94=20?= =?UTF-8?q?never=20vkDeviceWaitIdle=20while=20the=20pump=20decodes?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit vkDeviceWaitIdle's external-sync rule claims EVERY queue on the device; with Vulkan Video the session pump concurrently submits FFmpeg decode work to the shared device's video queue, so the resize path's wait-idle (and the video-image/staging rebuilds') raced it — observed as a crash on window resize mid-stream (software/VAAPI never touched the device from the pump, which is why this only appeared now). Mid-session quiescing is now fence-only ( waits the single in-flight fence, which covers every command buffer WE submitted), and the replaced swapchain + its per-image semaphores/overlay targets are parked in a retire list — the presentation engine may still hold the old swapchain's final semaphore wait, which no fence covers — and destroyed after the next present on the successor completes a fence cycle. The one legitimate device-wide idle left is teardown, and the run loop now JOINS the pump thread first (SessionHandle carries the JoinHandle; quick — the pump notices stop within its 20 ms receive timeout). Co-Authored-By: Claude Fable 5 --- crates/pf-client-core/src/session.rs | 7 +- crates/pf-presenter/src/run.rs | 25 +++++-- crates/pf-presenter/src/vk.rs | 101 +++++++++++++++++++++++---- 3 files changed, 113 insertions(+), 20 deletions(-) diff --git a/crates/pf-client-core/src/session.rs b/crates/pf-client-core/src/session.rs index 663a6f9d..543797ca 100644 --- a/crates/pf-client-core/src/session.rs +++ b/crates/pf-client-core/src/session.rs @@ -131,6 +131,10 @@ pub struct SessionHandle { pub events: async_channel::Receiver, pub frames: async_channel::Receiver, pub stop: Arc, + /// The pump thread. A Vulkan-Video pump SUBMITS to the shared device's decode + /// queue — the presenter must join this before any `vkDeviceWaitIdle`/teardown + /// (external-sync rule over every device queue). + pub thread: Option>, } pub fn start(params: SessionParams) -> SessionHandle { @@ -139,7 +143,7 @@ pub fn start(params: SessionParams) -> SessionHandle { let (frame_tx, frame_rx) = async_channel::bounded(2); let stop = Arc::new(AtomicBool::new(false)); let stop_w = stop.clone(); - std::thread::Builder::new() + let thread = std::thread::Builder::new() .name("punktfunk-session".into()) .spawn(move || pump(params, ev_tx, frame_tx, stop_w)) .expect("spawn session thread"); @@ -147,6 +151,7 @@ pub fn start(params: SessionParams) -> SessionHandle { events: ev_rx, frames: frame_rx, stop, + thread: Some(thread), } } diff --git a/crates/pf-presenter/src/run.rs b/crates/pf-presenter/src/run.rs index 89986349..98ccb669 100644 --- a/crates/pf-presenter/src/run.rs +++ b/crates/pf-presenter/src/run.rs @@ -170,6 +170,17 @@ impl StreamState { } } + /// Stop the pump and JOIN its thread — required before any device-wide idle or + /// teardown (the pump submits decode work to the shared device). Quick: the pump + /// notices `stop` within its 20 ms receive timeout, and on a normal end it's + /// already returning. + fn shutdown(mut self) { + self.handle.stop.store(true, Ordering::SeqCst); + if let Some(t) = self.handle.thread.take() { + let _ = t.join(); + } + } + /// Deliberate user exit (chord / window close): release capture, close with /// QUIT_CLOSE_CODE so the host tears down instead of lingering, stop the pump. /// The pump then emits `Ended(None)` — the loop's normal end path picks it up. @@ -530,7 +541,9 @@ fn run_inner(mut opts: SessionOpts, mut mode: ModeCtl) -> Result } ModeCtl::Browse(_) => { tracing::warn!(%msg, "connect failed — back to the library"); - stream = None; + if let Some(st) = stream.take() { + st.shutdown(); + } mouse.set_relative_mouse_mode(&window, false); mouse.show_cursor(true); if let Some(o) = overlay.as_mut() { @@ -550,7 +563,9 @@ fn run_inner(mut opts: SessionOpts, mut mode: ModeCtl) -> Result ModeCtl::Single(_) => break 'main Some(Outcome::Ended(reason)), ModeCtl::Browse(_) => { window.set_title(&opts.window_title).ok(); - stream = None; + if let Some(st) = stream.take() { + st.shutdown(); + } if let Some(o) = overlay.as_mut() { o.session_phase(SessionPhase::Ended(reason.as_deref())); } @@ -736,8 +751,10 @@ fn run_inner(mut opts: SessionOpts, mut mode: ModeCtl) -> Result } }; - if let Some(st) = &stream { - st.handle.stop.store(true, Ordering::SeqCst); + // Join the pump BEFORE the device-wide idle: its decode submissions on the shared + // device would race vkDeviceWaitIdle otherwise. + if let Some(st) = stream.take() { + st.shutdown(); } // Overlay resources live on the presenter's device: quiesce the queue first, drop // the overlay (its Drop destroys the Skia surfaces), THEN the presenter tears down. diff --git a/crates/pf-presenter/src/vk.rs b/crates/pf-presenter/src/vk.rs index 120fda5b..66b0aacd 100644 --- a/crates/pf-presenter/src/vk.rs +++ b/crates/pf-presenter/src/vk.rs @@ -65,6 +65,37 @@ impl Retired { } } +/// A replaced swapchain and its per-image objects, parked until a present + fence on +/// the NEW swapchain proves the presentation engine is past them (its semaphore waits +/// are outside fence visibility). This is what lets resize avoid `vkDeviceWaitIdle`, +/// which would race FFmpeg's decode submissions from the pump thread (external-sync +/// rule: wait-idle claims EVERY queue on the device). +struct DisplayGarbage { + swapchain: vk::SwapchainKHR, + render_sems: Vec, + overlay_views: Vec, + overlay_framebuffers: Vec, +} + +impl DisplayGarbage { + fn destroy(self, device: &ash::Device, swap_d: &ash::khr::swapchain::Device) { + unsafe { + for fb in self.overlay_framebuffers { + device.destroy_framebuffer(fb, None); + } + for v in self.overlay_views { + device.destroy_image_view(v, None); + } + for s in self.render_sems { + device.destroy_semaphore(s, None); + } + if self.swapchain != vk::SwapchainKHR::null() { + swap_d.destroy_swapchain(self.swapchain, None); + } + } + } +} + /// The overlay composite: one premultiplied-alpha quad blended over the swapchain image /// after the video blit (the §6.1 contract's presenter half). Always built — it has no /// Skia dependency and costs nothing while no overlay frame arrives (the render pass @@ -188,7 +219,16 @@ impl OverlayPipe { }) } + /// Detach the current per-swapchain-image targets (for deferred destruction). + fn take_targets(&mut self) -> (Vec, Vec) { + ( + std::mem::take(&mut self.views), + std::mem::take(&mut self.framebuffers), + ) + } + /// Rebuild the per-swapchain-image views + framebuffers (swapchain recreation). + /// The caller has already taken the old targets for deferred destruction. fn rebuild_targets( &mut self, device: &ash::Device, @@ -196,7 +236,7 @@ impl OverlayPipe { format: vk::Format, extent: vk::Extent2D, ) -> Result<()> { - self.destroy_targets(device); + self.destroy_targets(device); // no-op after take_targets; safety net otherwise for &image in images { let view = unsafe { device.create_image_view( @@ -292,6 +332,9 @@ pub struct Presenter { /// frame + our plane views): its GPU reads end with the in-flight fence, so it's /// destroyed right after the next fence wait. retired_hw: Option, + /// Replaced swapchains (resize), destroyed once a present on the successor has + /// gone through a fence cycle. + retired_display: Vec, format: vk::SurfaceFormatKHR, present_mode: vk::PresentModeKHR, swapchain: vk::SwapchainKHR, @@ -590,6 +633,7 @@ impl Presenter { video_export, overlay_pipe, retired_hw: None, + retired_display: Vec::new(), format, present_mode, swapchain: vk::SwapchainKHR::null(), @@ -608,10 +652,23 @@ impl Presenter { Ok(p) } + /// Wait the in-flight fence: OUR command buffers are done (staging, video image, + /// old-swapchain images). Deliberately NOT `vkDeviceWaitIdle` — the pump thread + /// submits FFmpeg's Vulkan decode work concurrently, and wait-idle's external-sync + /// rule over every device queue would race it (observed as a resize crash). + fn quiesce_own(&mut self) -> Result<()> { + unsafe { + if self.submitted { + self.device.wait_for_fences(&[self.fence], true, u64::MAX)?; + self.submitted = false; + } + } + Ok(()) + } + /// (Re)build the swapchain for the window's current pixel size. Also the resize path. pub fn recreate_swapchain(&mut self, window: &sdl3::video::Window) -> Result<()> { - unsafe { self.device.device_wait_idle() }.ok(); - self.submitted = false; + self.quiesce_own()?; let caps = unsafe { self.surface_i @@ -655,18 +712,23 @@ impl Presenter { .old_swapchain(old); let swapchain = unsafe { self.swap_d.create_swapchain(&info, None) } .context("vkCreateSwapchainKHR")?; - if old != vk::SwapchainKHR::null() { - unsafe { self.swap_d.destroy_swapchain(old, None) }; - } + // The old swapchain (and everything tied to its images) is parked, not + // destroyed: the presentation engine may still hold its final present's + // semaphore wait, which no fence covers. It dies after the next present on + // the NEW swapchain has gone through a fence cycle. + let (overlay_views, overlay_framebuffers) = self.overlay_pipe.take_targets(); + self.retired_display.push(DisplayGarbage { + swapchain: old, + render_sems: std::mem::take(&mut self.render_sems), + overlay_views, + overlay_framebuffers, + }); self.swapchain = swapchain; self.images = unsafe { self.swap_d.get_swapchain_images(swapchain) }?; self.extent = extent; self.overlay_pipe .rebuild_targets(&self.device, &self.images, self.format.format, extent)?; - for s in self.render_sems.drain(..) { - unsafe { self.device.destroy_semaphore(s, None) }; - } for _ in 0..self.images.len() { self.render_sems.push(unsafe { self.device @@ -695,8 +757,9 @@ impl Presenter { self.video_export.clone() } - /// Quiesce the queue — the run loop calls this before dropping the overlay so - /// nothing in flight still references its images. + /// Full device idle — TEARDOWN ONLY, and only after the session pump thread has + /// been joined (it submits FFmpeg decode work; wait-idle's external-sync rule + /// covers every queue on the device). Mid-session code uses the fence quiesce. pub fn wait_idle(&self) { unsafe { self.device.device_wait_idle() }.ok(); } @@ -763,6 +826,11 @@ impl Presenter { if let Some(old) = self.retired_hw.take() { old.destroy(&self.device); } + // A fence cycle has completed since the swapchain swap — the old display + // objects are past every wait now. + for g in self.retired_display.drain(..) { + g.destroy(&self.device, &self.swap_d); + } if let Some(f) = cpu_frame { self.stage_frame(f)?; @@ -1231,8 +1299,8 @@ impl Presenter { } fn rebuild_video_image(&mut self, width: u32, height: u32) -> Result<()> { - unsafe { self.device.device_wait_idle() }.ok(); - self.submitted = false; + // Fence-quiesce: the old image is only ever referenced by OUR command buffers. + self.quiesce_own()?; if let Some(v) = self.video.take() { unsafe { if v.framebuffer != vk::Framebuffer::null() { @@ -1308,8 +1376,7 @@ impl Presenter { } fn rebuild_staging(&mut self, capacity: usize) -> Result<()> { - unsafe { self.device.device_wait_idle() }.ok(); - self.submitted = false; + self.quiesce_own()?; if let Some(s) = self.staging.take() { unsafe { self.device.unmap_memory(s.memory); @@ -1392,6 +1459,10 @@ impl Drop for Presenter { self.device.destroy_image(v.image, None); self.device.free_memory(v.memory, None); } + for g in self.retired_display.drain(..) { + let device = self.device.clone(); + g.destroy(&device, &self.swap_d); + } self.hw.take(); self.csc.destroy(&self.device); self.overlay_pipe.destroy(&self.device);