diff --git a/src/converter/mod.rs b/src/converter/mod.rs index 15eadaa7..719beebf 100644 --- a/src/converter/mod.rs +++ b/src/converter/mod.rs @@ -12,7 +12,7 @@ use crate::encoder::ColorDescription; use crate::error::{PixelForgeError, Result}; use crate::sync::TimelinePoint; use crate::video::TimelineChain; -use crate::vulkan::VideoContext; +use crate::vulkan::{VideoContext, gpu_work_completed}; use ash::vk; use tracing::debug; @@ -1143,7 +1143,28 @@ impl ColorConverter { impl Drop for ColorConverter { fn drop(&mut self) { // Nothing below may be destroyed while a conversion still uses it. - let _ = self.wait_idle(); + if let Err(error) = self.wait_idle() { + tracing::error!(?error, "Converter fence wait failed during teardown"); + // A failed fence wait leaves completion unknown. Synchronize + // against the queue that submitted this conversion before cleanup. + let completion = unsafe { + self.context + .device() + .queue_wait_idle(self.context.compute_queue()) + }; + if !gpu_work_completed(&completion) { + tracing::error!( + ?completion, + "Cannot prove converter GPU completion; retaining Vulkan resources" + ); + // Keep the device alive if this is the last converter using it. + // Raw Vulkan handles are deliberately leaked on this path. + std::mem::forget(self.context.clone()); + return; + } + self.in_flight = false; + self.destroy_src_view(); + } unsafe { let device = self.context.device(); // Not destroyed here: an encode may still be waiting on a point diff --git a/src/encoder/codec.rs b/src/encoder/codec.rs index 32444f6e..32fcce06 100644 --- a/src/encoder/codec.rs +++ b/src/encoder/codec.rs @@ -25,7 +25,7 @@ use crate::encoder::resources::{ use crate::encoder::{ColorDescription, EncodeConfig, FrameType, RateControlMode}; use crate::error::{PixelForgeError, Result}; use crate::sync::TimelinePoint; -use crate::vulkan::VideoContext; +use crate::vulkan::{VideoContext, gpu_work_completed}; /// Per-encoder state shared by every codec. /// @@ -402,10 +402,37 @@ impl Drop for CodecEncoder { unsafe { let common = &mut self.common; let device = common.context.device(); - // Wait on just the queues this encoder used, not the whole device. - let _ = device.queue_wait_idle(common.context.transfer_queue()); + // The converter may still be writing this encoder's input image on + // the compute queue, even if no encode submission followed it. + // Wait for compute as well as the encoder's transfer and video queues. + let compute = device.queue_wait_idle(common.context.compute_queue()); + if !gpu_work_completed(&compute) { + tracing::error!( + ?compute, + "Cannot prove encoder input conversion completed; retaining Vulkan resources" + ); + std::mem::forget(common.context.clone()); + return; + } + let transfer = device.queue_wait_idle(common.context.transfer_queue()); + if !gpu_work_completed(&transfer) { + tracing::error!( + ?transfer, + "Cannot prove encoder transfer completion; retaining Vulkan resources" + ); + std::mem::forget(common.context.clone()); + return; + } if let Some(q) = common.context.video_encode_queue() { - let _ = device.queue_wait_idle(q); + let encode = device.queue_wait_idle(q); + if !gpu_work_completed(&encode) { + tracing::error!( + ?encode, + "Cannot prove video encode completion; retaining Vulkan resources" + ); + std::mem::forget(common.context.clone()); + return; + } } common.pipeline.destroy(device); diff --git a/src/vulkan.rs b/src/vulkan.rs index 9d5928f7..0abc3178 100644 --- a/src/vulkan.rs +++ b/src/vulkan.rs @@ -7,6 +7,25 @@ use std::ffi::{CStr, CString}; use std::os::raw::c_char; use tracing::{debug, info, warn}; +/// Successful GPU waits establish completion. Vulkan treats device loss as +/// completion for pending-command-buffer and resource-in-use cleanup rules. +pub(crate) fn gpu_work_completed(result: &std::result::Result<(), vk::Result>) -> bool { + matches!(result, Ok(()) | Err(vk::Result::ERROR_DEVICE_LOST)) +} + +#[cfg(test)] +mod teardown_tests { + use super::*; + + #[test] + fn only_success_or_device_loss_permits_resource_destruction() { + assert!(gpu_work_completed(&Ok(()))); + assert!(gpu_work_completed(&Err(vk::Result::ERROR_DEVICE_LOST))); + assert!(!gpu_work_completed(&Err(vk::Result::ERROR_OUT_OF_HOST_MEMORY))); + assert!(!gpu_work_completed(&Err(vk::Result::ERROR_UNKNOWN))); + } +} + mod encode_device; mod queues; use queues::QueueOverrides; @@ -628,8 +647,49 @@ struct VideoContextInner { debug_messenger: Option<(ash::ext::debug_utils::Instance, vk::DebugUtilsMessengerEXT)>, } +impl VideoContextInner { + /// Make sure retired timeline semaphores are no longer referenced by GPU + /// submissions before freeing them. A context-owned device must idle all + /// its queues before destruction. For a borrowed device, only wait on + /// pixelforge's queues; the caller remains responsible for any other work + /// it submitted using pixelforge's exported TimelinePoints. + fn wait_before_teardown(&self) -> std::result::Result<(), vk::Result> { + if self.owns_device { + return unsafe { self.device.device_wait_idle() }; + } + let mut queues = vec![self.compute_queue]; + if !queues.contains(&self.transfer_queue) { + queues.push(self.transfer_queue); + } + if let Some(queue) = self.video_encode_queue { + if !queues.contains(&queue) { + queues.push(queue); + } + } + if let Some(queue) = self.video_decode_queue { + if !queues.contains(&queue) { + queues.push(queue); + } + } + for queue in queues { + unsafe { self.device.queue_wait_idle(queue)? }; + } + Ok(()) + } +} + impl Drop for VideoContextInner { fn drop(&mut self) { + let completion = self.wait_before_teardown(); + if !gpu_work_completed(&completion) { + tracing::error!( + ?completion, + "Cannot prove GPU completion; retaining Vulkan device and retired semaphores" + ); + // ash handles have no automatic destructor. Retain rather than + // destroy resources with unresolved GPU references. + return; + } // Ours whoever owns the device, and gone before the device can be. let retired = std::mem::take( self.retired_semaphores