From 2e516551594332585033625d1954af7d05c166cc Mon Sep 17 00:00:00 2001 From: DatCaptainHorse Date: Wed, 23 Sep 2026 08:08:32 +0300 Subject: [PATCH 1/7] refactor(converter)!: bind descriptors with push descriptors The converter binds two descriptors, a sampled source image and an output buffer, and rewrites both on every dispatch. It did that through VK_EXT_descriptor_buffer, which is now deprecated in favour of VK_EXT_descriptor_heap, and which was a heavy tool for two descriptors: it needed bufferDeviceAddress, descriptorBufferCaptureReplay, a mapped host buffer, per-binding offset queries, device-address memory, and a pipeline flag that was missing until hgaiser/pixelforge#40 added it. Push descriptors write both straight into the command buffer. There is no descriptor memory, no device addresses and nothing to enable beyond VK_KHR_push_descriptor, which every desktop driver has had for years and which is core in Vulkan 1.4. Descriptor heaps would be the replacement for a large, long-lived descriptor set; this is not one. The shorter feature list matters most for a caller that hands pixelforge its own device, which would otherwise have to turn on descriptor buffers and capture-replay just so the converter can bind two descriptors. Removing the descriptor-buffer block also fixes two features that had ended up inside it by accident: samplerYcbcrConversion and the 2-plane 4:4:4 formats were only enabled on devices that also had descriptor buffers, even though the 4:4:4 extension itself was enabled regardless. BREAKING CHANGE: VideoContext::has_descriptor_buffer is replaced by VideoContext::has_push_descriptor. Co-Authored-By: Claude Opus 5.5 --- src/converter/mod.rs | 113 ++++---------------- src/converter/pipeline.rs | 212 ++++++++++++++------------------------ src/encoder/resources.rs | 6 +- src/video.rs | 60 ----------- src/vulkan.rs | 93 ++++++----------- 5 files changed, 129 insertions(+), 355 deletions(-) diff --git a/src/converter/mod.rs b/src/converter/mod.rs index dbabfeae..b0eb6649 100644 --- a/src/converter/mod.rs +++ b/src/converter/mod.rs @@ -375,26 +375,10 @@ pub struct ColorConverter { // Output buffer (compute shader writes here). output_buffer: vk::Buffer, output_memory: vk::DeviceMemory, - // Output buffer size and device address. The address is fed into - // `VkDescriptorAddressInfoEXT` when populating the binding-1 descriptor. output_buffer_size: usize, - output_buffer_address: vk::DeviceAddress, - - // Descriptor buffer (mapped, descriptors are written here per-frame). - descriptor_buffer: vk::Buffer, - descriptor_buffer_memory: vk::DeviceMemory, - descriptor_buffer_address: vk::DeviceAddress, - descriptor_buffer_usage: vk::BufferUsageFlags, - descriptor_buffer_ptr: *mut u8, - // Descriptor sizes for each binding type, queried from - // `VkPhysicalDeviceDescriptorBufferPropertiesEXT`. - combined_image_sampler_descriptor_size: usize, - storage_buffer_descriptor_size: usize, - // Loaded descriptor-buffer extension device for `vkGetDescriptorEXT`. - ext_device: ash::ext::descriptor_buffer::Device, - // Per-binding offsets within the descriptor buffer. - binding0_offset: u64, - binding1_offset: u64, + + // Binds both descriptors inside the command buffer each frame. + push_descriptor: ash::khr::push_descriptor::Device, // Command resources. command_pool: vk::CommandPool, @@ -877,78 +861,30 @@ impl ColorConverter { self.pipeline, ); - // --- Populate descriptors directly into the descriptor buffer --- - // - // Use `vkGetDescriptorEXT` for runtime descriptor population. (The - // previous implementation mistakenly used the - // `vkGet*OpaqueCaptureDescriptorDataEXT` family — those produce - // opaque payloads for capture/replay tooling and are NOT the - // descriptor data the GPU consumes at binding offsets, which made - // the compute shader read garbage and emit constant-Y output — - // visible as a green-screen stream.) - // - // The descriptor buffer is persistent-mapped HOST_COHERENT, so a - // plain memcpy via the slice handed to `get_descriptor` is enough. - - // Binding 0: COMBINED_IMAGE_SAMPLER (sampler + image view). - let image_info = vk::DescriptorImageInfo::default() + let image_info = [vk::DescriptorImageInfo::default() .sampler(self.sampler) .image_view(src_view) - .image_layout(vk::ImageLayout::SHADER_READ_ONLY_OPTIMAL); - let combined_get_info = vk::DescriptorGetInfoEXT::default() - .ty(vk::DescriptorType::COMBINED_IMAGE_SAMPLER) - .data(vk::DescriptorDataEXT { - p_combined_image_sampler: &image_info, - }); - let combined_dst = std::slice::from_raw_parts_mut( - self.descriptor_buffer_ptr - .add(self.binding0_offset as usize), - self.combined_image_sampler_descriptor_size, - ); - self.ext_device - .get_descriptor(&combined_get_info, combined_dst); - - // Binding 1: STORAGE_BUFFER (output YUV). - let buffer_addr_info = vk::DescriptorAddressInfoEXT::default() - .address(self.output_buffer_address) - .range(self.output_buffer_size as u64) - .format(vk::Format::UNDEFINED); - let storage_get_info = vk::DescriptorGetInfoEXT::default() - .ty(vk::DescriptorType::STORAGE_BUFFER) - .data(vk::DescriptorDataEXT { - p_storage_buffer: &buffer_addr_info, - }); - let storage_dst = std::slice::from_raw_parts_mut( - self.descriptor_buffer_ptr - .add(self.binding1_offset as usize), - self.storage_buffer_descriptor_size, - ); - self.ext_device - .get_descriptor(&storage_get_info, storage_dst); - - // --- Bind descriptor buffers --- - let binding_info = vk::DescriptorBufferBindingInfoEXT::default() - .address(self.descriptor_buffer_address) - .usage(self.descriptor_buffer_usage); - - self.ext_device.cmd_bind_descriptor_buffers( - self.command_buffer, - std::slice::from_ref(&binding_info), - ); - - // Associate set 0 in the pipeline layout with the bound descriptor buffer. - // This is required because descriptor buffers use offset-based binding. - // The base offset is 0 since all payloads are placed at their binding offsets. - let buffer_indices = [0u32]; - let offsets = [0 as vk::DeviceSize]; - - self.ext_device.cmd_set_descriptor_buffer_offsets( + .image_layout(vk::ImageLayout::SHADER_READ_ONLY_OPTIMAL)]; + let buffer_info = [vk::DescriptorBufferInfo::default() + .buffer(self.output_buffer) + .offset(0) + .range(self.output_buffer_size as vk::DeviceSize)]; + let writes = [ + vk::WriteDescriptorSet::default() + .dst_binding(0) + .descriptor_type(vk::DescriptorType::COMBINED_IMAGE_SAMPLER) + .image_info(&image_info), + vk::WriteDescriptorSet::default() + .dst_binding(1) + .descriptor_type(vk::DescriptorType::STORAGE_BUFFER) + .buffer_info(&buffer_info), + ]; + self.push_descriptor.cmd_push_descriptor_set( self.command_buffer, vk::PipelineBindPoint::COMPUTE, self.pipeline_layout, - 0, // first_set - &buffer_indices, - &offsets, + 0, + &writes, ); // Push constants: width, height, input_format, output_format, @@ -1160,11 +1096,6 @@ impl Drop for ColorConverter { device.destroy_buffer(self.output_buffer, None); device.free_memory(self.output_memory, None); - // Destroy descriptor buffer and its memory. - device.unmap_memory(self.descriptor_buffer_memory); - device.destroy_buffer(self.descriptor_buffer, None); - device.free_memory(self.descriptor_buffer_memory, None); - // Destroy pipeline resources. device.destroy_pipeline(self.pipeline, None); device.destroy_pipeline_layout(self.pipeline_layout, None); diff --git a/src/converter/pipeline.rs b/src/converter/pipeline.rs index ab2c547a..29d8248e 100644 --- a/src/converter/pipeline.rs +++ b/src/converter/pipeline.rs @@ -4,10 +4,10 @@ //! including the compute pipeline built from the precompiled SPIR-V shader. use super::{ColorConverter, ColorConverterConfig}; +use crate::encoder::resources::find_memory_type; use crate::error::{PixelForgeError, Result}; use crate::vulkan::VideoContext; use ash::vk; -use ash::vk::TaggedStructure; /// Precompiled SPIR-V bytecode for the color conversion compute shader. const COLOR_CONVERT_SPIRV_BYTES: &[u8] = include_bytes!("../../shader/color_convert.spv"); @@ -35,15 +35,13 @@ pub fn create_converter( context: VideoContext, config: ColorConverterConfig, ) -> Result { - if !context.has_descriptor_buffer() { + if !context.has_push_descriptor() { return Err(PixelForgeError::NoSuitableDevice( - "VK_EXT_descriptor_buffer with capture-replay is required but not available on this device".to_string(), + "VK_KHR_push_descriptor is required but not enabled on this device".to_string(), )); } let device = context.device(); - let instance = context.instance(); - let physical_device = context.physical_device(); // Create descriptor set layout. let bindings = [ @@ -61,8 +59,12 @@ pub fn create_converter( .stage_flags(vk::ShaderStageFlags::COMPUTE), ]; + // Both descriptors change with every frame and there are only two of them, + // so they are pushed into the command buffer rather than kept in a set or + // a descriptor buffer: no pool, no memory to manage, and nothing the device + // has to enable beyond the extension itself. let layout_info = vk::DescriptorSetLayoutCreateInfo::default() - .flags(vk::DescriptorSetLayoutCreateFlags::DESCRIPTOR_BUFFER_EXT) + .flags(vk::DescriptorSetLayoutCreateFlags::PUSH_DESCRIPTOR_KHR) .bindings(&bindings); let descriptor_set_layout = unsafe { device.create_descriptor_set_layout(&layout_info, None) } @@ -95,15 +97,7 @@ pub fn create_converter( .module(shader_module) .name(&entry_point); - // The descriptor set layout above was created with DESCRIPTOR_BUFFER_EXT - // (see `layout_info` above), so the pipeline consuming it must also be - // created with this flag. Without it, binding a descriptor buffer at - // dispatch time is undefined behaviour (VUID-vkCmdDispatch-None-08117 / - // VUID-vkCmdDispatch-None-08600) -- the compute shader runs with - // effectively-unbound descriptor bindings, which is a GPU memory-safety - // hazard (arbitrary reads/writes each dispatch). let pipeline_info = vk::ComputePipelineCreateInfo::default() - .flags(vk::PipelineCreateFlags::DESCRIPTOR_BUFFER_EXT) .stage(stage_info) .layout(pipeline_layout); @@ -133,115 +127,19 @@ pub fn create_converter( .map_err(|e| PixelForgeError::ResourceCreation(format!("sampler creation: {}", e)))?; // Create output buffer (device local for compute shader output, transfer - // source for image copy). Needs SHADER_DEVICE_ADDRESS so we can query a - // device address to feed `VkDescriptorAddressInfoEXT` when populating - // the storage-buffer descriptor at runtime. Memory must be allocated with - // `VK_MEMORY_ALLOCATE_DEVICE_ADDRESS_BIT` for `get_buffer_device_address` - // to return a well-defined address, so use the device-address-aware - // allocator rather than the plain `create_buffer` helper. - let (output_buffer, output_memory) = - crate::encoder::resources::create_buffer_with_device_address( - device, - context.memory_properties(), - output_size as vk::DeviceSize, - vk::BufferUsageFlags::STORAGE_BUFFER - | vk::BufferUsageFlags::TRANSFER_SRC - | vk::BufferUsageFlags::TRANSFER_DST - | vk::BufferUsageFlags::SHADER_DEVICE_ADDRESS, - vk::MemoryPropertyFlags::DEVICE_LOCAL, - )?; - - // Resolve the output buffer's device address. This goes into the - // `VkDescriptorAddressInfoEXT` we pass to `vkGetDescriptorEXT` when - // populating the storage-buffer descriptor each frame. - let output_buffer_address = unsafe { - device.get_buffer_device_address( - &vk::BufferDeviceAddressInfo::default().buffer(output_buffer), - ) - }; - - // Query descriptor buffer properties to determine correct descriptor sizes. - // The bug in the previous implementation was using the *capture-replay* - // sizes (e.g. `sampler_capture_replay_descriptor_data_size`) — those are - // for capture/replay tooling like RenderDoc and have nothing to do with - // runtime descriptor population. The correct sizes for in-buffer - // descriptors are the regular `*_descriptor_size` fields. - let mut db_props = vk::PhysicalDeviceDescriptorBufferPropertiesEXT::default(); - let mut props = vk::PhysicalDeviceProperties2::default().push(&mut db_props); - unsafe { - instance.get_physical_device_properties2(physical_device, &mut props); - } - let combined_image_sampler_size = db_props.combined_image_sampler_descriptor_size; - let storage_buffer_size_descriptor = db_props.storage_buffer_descriptor_size; - - // Query descriptor set layout size and binding offsets for correct buffer sizing. - let ext_device = - ash::ext::descriptor_buffer::Device::load(context.instance(), context.device()); - let vk_device = context.device().handle(); - let mut layout_size = 0u64; - unsafe { - (ext_device.fp().get_descriptor_set_layout_size_ext)( - vk_device, - descriptor_set_layout, - &mut layout_size, - ); - } - let binding0_offset = unsafe { - let mut offset = 0u64; - (ext_device.fp().get_descriptor_set_layout_binding_offset_ext)( - vk_device, - descriptor_set_layout, - 0, - &mut offset, - ); - offset - }; - let binding1_offset = unsafe { - let mut offset = 0u64; - (ext_device.fp().get_descriptor_set_layout_binding_offset_ext)( - vk_device, - descriptor_set_layout, - 1, - &mut offset, - ); - offset - }; - - // Descriptor buffer layout: - // Offset 0: Sampler + image view capture payload (binding 0) - // Offset X: Buffer capture payload (binding 1) - // The total size is the layout size which accounts for alignment. - let descriptor_buffer_size: vk::DeviceSize = layout_size as vk::DeviceSize; - - let (descriptor_buffer, descriptor_buffer_memory) = - crate::encoder::resources::create_buffer_with_device_address( - device, - context.memory_properties(), - descriptor_buffer_size, - vk::BufferUsageFlags::SHADER_DEVICE_ADDRESS - | vk::BufferUsageFlags::RESOURCE_DESCRIPTOR_BUFFER_EXT, - vk::MemoryPropertyFlags::HOST_VISIBLE | vk::MemoryPropertyFlags::HOST_COHERENT, - )?; - - // Get the buffer's device address for binding descriptor buffers. - // cmdBindDescriptorBuffers requires the buffer's device address, not the memory capture address. - let buf_addr_info = vk::BufferDeviceAddressInfo::default().buffer(descriptor_buffer); - let descriptor_buffer_address = unsafe { device.get_buffer_device_address(&buf_addr_info) }; - - // Persistent map the descriptor buffer (HOST_COHERENT, no flush needed). - let descriptor_buffer_ptr = unsafe { - device - .map_memory( - descriptor_buffer_memory, - 0, - vk::WHOLE_SIZE, - vk::MemoryMapFlags::empty(), - ) - .map_err(|e| { - PixelForgeError::ResourceCreation(format!("map descriptor buffer: {}", e)) - })? - }; - let descriptor_buffer_ptr = descriptor_buffer_ptr as *mut u8; + // source for image copy). + let (output_buffer, output_memory) = create_buffer( + device, + context.memory_properties(), + output_size as vk::DeviceSize, + vk::BufferUsageFlags::STORAGE_BUFFER + | vk::BufferUsageFlags::TRANSFER_SRC + | vk::BufferUsageFlags::TRANSFER_DST, + vk::MemoryPropertyFlags::DEVICE_LOCAL, + )?; + + let push_descriptor = + ash::khr::push_descriptor::Device::load(context.instance(), context.device()); // Create command pool for compute queue. let pool_info = vk::CommandPoolCreateInfo::default() @@ -276,22 +174,62 @@ pub fn create_converter( output_buffer, output_memory, output_buffer_size: output_size, - output_buffer_address, command_pool, command_buffer, fence, - // Descriptor buffer fields. - descriptor_buffer, - descriptor_buffer_memory, - descriptor_buffer_address, - descriptor_buffer_usage: vk::BufferUsageFlags::SHADER_DEVICE_ADDRESS - | vk::BufferUsageFlags::RESOURCE_DESCRIPTOR_BUFFER_EXT, - descriptor_buffer_ptr, - combined_image_sampler_descriptor_size: combined_image_sampler_size, - storage_buffer_descriptor_size: storage_buffer_size_descriptor, - ext_device, - // Layout info for correct offset computation. - binding0_offset, - binding1_offset, + push_descriptor, }) } + +/// Create a buffer with associated memory. +fn create_buffer( + device: &ash::Device, + memory_properties: &vk::PhysicalDeviceMemoryProperties, + size: vk::DeviceSize, + usage: vk::BufferUsageFlags, + properties: vk::MemoryPropertyFlags, +) -> Result<(vk::Buffer, vk::DeviceMemory)> { + let buffer_info = vk::BufferCreateInfo::default() + .size(size) + .usage(usage) + .sharing_mode(vk::SharingMode::EXCLUSIVE); + + let buffer = unsafe { device.create_buffer(&buffer_info, None) } + .map_err(|e| PixelForgeError::ResourceCreation(format!("buffer creation: {}", e)))?; + + let mem_requirements = unsafe { device.get_buffer_memory_requirements(buffer) }; + + let Some(memory_type_index) = find_memory_type( + memory_properties, + mem_requirements.memory_type_bits, + properties, + ) else { + unsafe { device.destroy_buffer(buffer, None) }; + return Err(PixelForgeError::MemoryAllocation(format!( + "No suitable memory type for buffer with properties {:?}", + properties + ))); + }; + + let alloc_info = vk::MemoryAllocateInfo::default() + .allocation_size(mem_requirements.size) + .memory_type_index(memory_type_index); + + let memory = match unsafe { device.allocate_memory(&alloc_info, None) } { + Ok(m) => m, + Err(e) => { + unsafe { device.destroy_buffer(buffer, None) }; + return Err(PixelForgeError::MemoryAllocation(e.to_string())); + } + }; + + if let Err(e) = unsafe { device.bind_buffer_memory(buffer, memory, 0) } { + unsafe { + device.destroy_buffer(buffer, None); + device.free_memory(memory, None); + } + return Err(PixelForgeError::MemoryAllocation(e.to_string())); + } + + Ok((buffer, memory)) +} diff --git a/src/encoder/resources.rs b/src/encoder/resources.rs index 503a17fa..ae4c5867 100644 --- a/src/encoder/resources.rs +++ b/src/encoder/resources.rs @@ -11,9 +11,9 @@ use std::ptr; pub(crate) use crate::video::gcd; pub(crate) use crate::video::{ VideoImageParams, align_up, allocate_command_buffers, allocate_session_memory, - create_bitstream_buffer, create_buffer_with_device_address, create_command_pool, - create_dpb_images as create_dpb_images_shared, create_fence, create_video_image, - find_memory_type, get_video_format, lcm, map_bitstream_buffer, query_supported_video_formats, + create_bitstream_buffer, create_command_pool, create_dpb_images as create_dpb_images_shared, + create_fence, create_video_image, find_memory_type, get_video_format, lcm, + map_bitstream_buffer, query_supported_video_formats, }; /// Create the encoder's DPB images. diff --git a/src/video.rs b/src/video.rs index c6cd468c..449de659 100644 --- a/src/video.rs +++ b/src/video.rs @@ -314,66 +314,6 @@ pub(crate) fn get_video_format(pixel_format: PixelFormat, bit_depth: BitDepth) - } } -/// Create a buffer that requires device addresses (SHADER_DEVICE_ADDRESS usage). -/// -/// This allocates memory with `VK_MEMORY_ALLOCATE_DEVICE_ADDRESS_BIT` so that -/// `get_buffer_device_address` returns a valid address. -pub(crate) fn create_buffer_with_device_address( - device: &ash::Device, - memory_properties: &vk::PhysicalDeviceMemoryProperties, - size: vk::DeviceSize, - usage: vk::BufferUsageFlags, - properties: vk::MemoryPropertyFlags, -) -> Result<(vk::Buffer, vk::DeviceMemory)> { - let buffer_info = vk::BufferCreateInfo::default() - .size(size) - .usage(usage) - .sharing_mode(vk::SharingMode::EXCLUSIVE); - - let buffer = unsafe { device.create_buffer(&buffer_info, None) } - .map_err(|e| PixelForgeError::ResourceCreation(format!("buffer creation: {}", e)))?; - - let mem_requirements = unsafe { device.get_buffer_memory_requirements(buffer) }; - - let memory_type_index = find_memory_type( - memory_properties, - mem_requirements.memory_type_bits, - properties, - ) - .ok_or_else(|| { - PixelForgeError::MemoryAllocation(format!( - "No suitable memory type for buffer with properties {:?}", - properties - )) - })?; - - let mut alloc_flags_info = - vk::MemoryAllocateFlagsInfo::default().flags(vk::MemoryAllocateFlags::DEVICE_ADDRESS); - let alloc_info = vk::MemoryAllocateInfo::default() - .allocation_size(mem_requirements.size) - .memory_type_index(memory_type_index) - .push(&mut alloc_flags_info); - - let memory = match unsafe { device.allocate_memory(&alloc_info, None) } { - Ok(m) => m, - Err(e) => { - unsafe { device.destroy_buffer(buffer, None) }; - return Err(PixelForgeError::MemoryAllocation(e.to_string())); - } - }; - - match unsafe { device.bind_buffer_memory(buffer, memory, 0) } { - Ok(()) => Ok((buffer, memory)), - Err(e) => { - unsafe { - device.destroy_buffer(buffer, None); - device.free_memory(memory, None); - } - Err(PixelForgeError::MemoryAllocation(e.to_string())) - } - } -} - pub(crate) fn find_memory_type( memory_props: &vk::PhysicalDeviceMemoryProperties, type_filter: u32, diff --git a/src/vulkan.rs b/src/vulkan.rs index 716b9f6e..8564a767 100644 --- a/src/vulkan.rs +++ b/src/vulkan.rs @@ -421,7 +421,7 @@ struct VideoContextInner { device_properties: vk::PhysicalDeviceProperties, supported_encode_codecs: Vec, supported_decode_codecs: Vec, - has_descriptor_buffer: bool, + has_push_descriptor: bool, has_unified_image_layouts: bool, /// Whether this context created (and therefore must destroy) the device and /// instance. A context adopted from a caller's device via @@ -530,9 +530,10 @@ impl VideoContext { &self.inner.device_properties } - /// Returns true if `VK_EXT_descriptor_buffer` is available and enabled. - pub fn has_descriptor_buffer(&self) -> bool { - self.inner.has_descriptor_buffer + /// Whether `VK_KHR_push_descriptor` is enabled, which the colour converter + /// needs. + pub fn has_push_descriptor(&self) -> bool { + self.inner.has_push_descriptor } /// Whether `VK_KHR_unified_image_layouts` is enabled, with its video bit. @@ -672,7 +673,6 @@ impl VideoContext { let mut compute_queue_family = u32::MAX; let mut supported_encode_codecs = Vec::new(); let mut supported_decode_codecs = Vec::new(); - let mut has_descriptor_buffer_ext = false; let has_extension = |extensions: &[vk::ExtensionProperties], name: &std::ffi::CStr| -> bool { @@ -769,10 +769,6 @@ impl VideoContext { // Check codec support for encoding. let mut encode_codecs = Vec::new(); if let Some(eq) = encode_queue { - // Check if descriptor buffer extension is available. - has_descriptor_buffer_ext = - has_extension(&available_extensions, ash::ext::descriptor_buffer::NAME); - // Only check codec support if the extension exists if has_extension(&available_extensions, ash::khr::video_encode_h264::NAME) && Self::check_h264_encode_support(&entry, &instance, physical_device, eq) @@ -956,11 +952,14 @@ impl VideoContext { } } - // Enable VK_EXT_descriptor_buffer extension (required for descriptor buffer API). - if has_descriptor_buffer_ext { - push_ext(ash::ext::descriptor_buffer::NAME.as_ptr()); + // The colour converter binds its two descriptors with push descriptors. + let has_push_descriptor = selected_device_exts + .as_ref() + .is_some_and(|exts| has_extension(exts, ash::khr::push_descriptor::NAME)); + if has_push_descriptor { + push_ext(ash::khr::push_descriptor::NAME.as_ptr()); } else { - warn!("VK_EXT_descriptor_buffer not available on this device"); + warn!("VK_KHR_push_descriptor not available; the colour converter will be unavailable"); } // Enable synchronization2 feature. @@ -1025,6 +1024,12 @@ impl VideoContext { vk::PhysicalDeviceTimelineSemaphoreFeatures::default().timeline_semaphore(true); // Enable sampler YCbCr conversion feature (required for YUV image views with SAMPLED flag). + let mut supported_ycbcr = vk::PhysicalDeviceSamplerYcbcrConversionFeatures::default(); + let mut ycbcr_query = vk::PhysicalDeviceFeatures2::default().push(&mut supported_ycbcr); + unsafe { + instance.get_physical_device_features2(physical_device, &mut ycbcr_query); + } + let has_ycbcr_conversion = supported_ycbcr.sampler_ycbcr_conversion != 0; let mut ycbcr_features = vk::PhysicalDeviceSamplerYcbcrConversionFeatures::default() .sampler_ycbcr_conversion(true); @@ -1049,41 +1054,6 @@ impl VideoContext { let mut av1_encode_features = vk::PhysicalDeviceVideoEncodeAV1FeaturesKHR::default().video_encode_av1(true); - // Query descriptor buffer and buffer device address feature support. - let mut desc_buf_features = vk::PhysicalDeviceDescriptorBufferFeaturesEXT::default(); - let mut buffer_device_address_features = - vk::PhysicalDeviceBufferDeviceAddressFeatures::default(); - - if has_descriptor_buffer_ext { - let mut feat2 = vk::PhysicalDeviceFeatures2::default().push(&mut desc_buf_features); - unsafe { - instance.get_physical_device_features2(physical_device, &mut feat2); - } - let desc_buf_supported = desc_buf_features.descriptor_buffer != 0 - && desc_buf_features.descriptor_buffer_capture_replay != 0; - - // Query buffer device address support. - let mut feat2_bda = - vk::PhysicalDeviceFeatures2::default().push(&mut buffer_device_address_features); - unsafe { - instance.get_physical_device_features2(physical_device, &mut feat2_bda); - } - - if desc_buf_supported && buffer_device_address_features.buffer_device_address != 0 { - desc_buf_features.descriptor_buffer = 1; - desc_buf_features.descriptor_buffer_capture_replay = 1; - } else if desc_buf_supported { - warn!( - "VK_EXT_descriptor_buffer extension present but bufferDeviceAddress not supported; descriptor buffer will not be enabled" - ); - } - } - - // Store whether descriptor buffer is available for use by callers. - let has_descriptor_buffer = has_descriptor_buffer_ext - && desc_buf_features.descriptor_buffer != 0 - && desc_buf_features.descriptor_buffer_capture_replay != 0; - // Log all extensions being enabled debug!("Enabling {} device extensions:", extension_names.len()); for ext_name_ptr in &extension_names { @@ -1105,20 +1075,15 @@ impl VideoContext { device_create_info = device_create_info.push(&mut av1_encode_features); } - // Attach the chain to device_create_info. - // When descriptor buffer is available, the chain is: - // desc_buf_features -> buffer_device_address_features -> sync2_features -> ... - // When descriptor buffer is not available, only sync2_features is chained. - if has_descriptor_buffer_ext { - device_create_info = device_create_info - .push(&mut desc_buf_features) - .push(&mut buffer_device_address_features) - .push(&mut ycbcr_features); - - // Enable YCbCr 2-plane 444 formats feature (required for YUV444 encoding with NVIDIA). - if has_ycbcr_2plane_444_ext { - device_create_info = device_create_info.push(&mut ycbcr_2plane_444_features); - } + if has_ycbcr_conversion { + device_create_info = device_create_info.push(&mut ycbcr_features); + } + // Enable YCbCr 2-plane 444 formats feature (required for YUV444 encoding with NVIDIA). + // Its extension is enabled above whenever present, so the feature has + // to follow it unconditionally: an extension without its feature bit + // leaves the 4:4:4 formats unusable. + if has_ycbcr_2plane_444_ext { + device_create_info = device_create_info.push(&mut ycbcr_2plane_444_features); } let device = unsafe { instance.create_device(physical_device, &device_create_info, None) } @@ -1161,7 +1126,7 @@ impl VideoContext { device_properties, supported_encode_codecs, supported_decode_codecs, - has_descriptor_buffer, + has_push_descriptor, has_unified_image_layouts: has_unified_layouts, owns_device: true, debug_messenger, @@ -1238,7 +1203,7 @@ impl VideoContext { device_properties, supported_encode_codecs: Vec::new(), supported_decode_codecs, - has_descriptor_buffer: false, + has_push_descriptor: false, has_unified_image_layouts: declared_unified_image_layouts, owns_device: false, // The caller owns the instance; reporting is theirs to set up. From 928f22a33e3f7f706ec68a379b2594ed8f1510a6 Mon Sep 17 00:00:00 2001 From: DatCaptainHorse Date: Wed, 23 Sep 2026 08:17:51 +0300 Subject: [PATCH 2/7] test: share the synthetic source image between tests The colour conversion tests built their BGRA source image, and the one-shot submission and host buffer helpers behind it, inline. A test of the converter replacing its source image needs that image built from pixels of its own choosing, so these move to tests/common, with the frame size and pixels passed in rather than taken from the conversion test's constants. No behaviour change. Co-Authored-By: Claude Opus 5.5 --- tests/color_conversion.rs | 201 +----------------------------------- tests/common/mod.rs | 2 + tests/common/source.rs | 207 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 212 insertions(+), 198 deletions(-) create mode 100644 tests/common/source.rs diff --git a/tests/color_conversion.rs b/tests/color_conversion.rs index 362a1170..41504b1a 100644 --- a/tests/color_conversion.rs +++ b/tests/color_conversion.rs @@ -23,6 +23,7 @@ #[allow(dead_code)] mod common; +use common::source::{SrcImage, create_src_image, host_buffer, one_shot}; use ash::vk; use pixelforge::{ @@ -132,202 +133,6 @@ fn expected_code( } } -/// The synthetic frame, on the GPU, cleaned up when the test drops it. -struct SrcImage { - /// Keeps the device alive until after the image is destroyed. - context: VideoContext, - image: vk::Image, - memory: vk::DeviceMemory, -} - -impl Drop for SrcImage { - fn drop(&mut self) { - let device = self.context.device(); - unsafe { - device.destroy_image(self.image, None); - device.free_memory(self.memory, None); - } - } -} - -unsafe fn one_shot( - context: &VideoContext, - record: F, -) -> Result<(), Box> { - let device = context.device(); - let pool_info = vk::CommandPoolCreateInfo::default() - .queue_family_index(context.transfer_queue_family()) - .flags(vk::CommandPoolCreateFlags::TRANSIENT); - let pool = unsafe { device.create_command_pool(&pool_info, None) }?; - let cb_info = vk::CommandBufferAllocateInfo::default() - .command_pool(pool) - .level(vk::CommandBufferLevel::PRIMARY) - .command_buffer_count(1); - let cb = unsafe { device.allocate_command_buffers(&cb_info) }?[0]; - unsafe { - device.begin_command_buffer( - cb, - &vk::CommandBufferBeginInfo::default() - .flags(vk::CommandBufferUsageFlags::ONE_TIME_SUBMIT), - )?; - record(cb); - device.end_command_buffer(cb)?; - let cbs = [cb]; - let submit = vk::SubmitInfo::default().command_buffers(&cbs); - device.queue_submit(context.transfer_queue(), &[submit], vk::Fence::null())?; - device.queue_wait_idle(context.transfer_queue())?; - device.destroy_command_pool(pool, None); - } - Ok(()) -} - -unsafe fn host_buffer( - context: &VideoContext, - size: u64, - usage: vk::BufferUsageFlags, -) -> Result<(vk::Buffer, vk::DeviceMemory), Box> { - let device = context.device(); - let info = vk::BufferCreateInfo::default() - .size(size) - .usage(usage) - .sharing_mode(vk::SharingMode::EXCLUSIVE); - let buffer = unsafe { device.create_buffer(&info, None) }?; - let reqs = unsafe { device.get_buffer_memory_requirements(buffer) }; - let mem_type = context - .find_memory_type( - reqs.memory_type_bits, - vk::MemoryPropertyFlags::HOST_VISIBLE | vk::MemoryPropertyFlags::HOST_COHERENT, - ) - .ok_or("no host visible memory")?; - let alloc = vk::MemoryAllocateInfo::default() - .allocation_size(reqs.size) - .memory_type_index(mem_type); - let memory = unsafe { device.allocate_memory(&alloc, None) }?; - unsafe { device.bind_buffer_memory(buffer, memory, 0) }?; - Ok((buffer, memory)) -} - -unsafe fn create_src_image(context: &VideoContext) -> Result> { - let device = context.device(); - let info = vk::ImageCreateInfo::default() - .image_type(vk::ImageType::TYPE_2D) - .format(vk::Format::B8G8R8A8_UNORM) - .extent(vk::Extent3D { - width: WIDTH, - height: HEIGHT, - depth: 1, - }) - .mip_levels(1) - .array_layers(1) - .samples(vk::SampleCountFlags::TYPE_1) - .tiling(vk::ImageTiling::OPTIMAL) - .usage(vk::ImageUsageFlags::SAMPLED | vk::ImageUsageFlags::TRANSFER_DST) - .sharing_mode(vk::SharingMode::EXCLUSIVE) - .initial_layout(vk::ImageLayout::UNDEFINED); - let image = unsafe { device.create_image(&info, None) }?; - let reqs = unsafe { device.get_image_memory_requirements(image) }; - let mem_type = context - .find_memory_type(reqs.memory_type_bits, vk::MemoryPropertyFlags::DEVICE_LOCAL) - .ok_or("no device local memory")?; - let alloc = vk::MemoryAllocateInfo::default() - .allocation_size(reqs.size) - .memory_type_index(mem_type); - let memory = unsafe { device.allocate_memory(&alloc, None) }?; - unsafe { device.bind_image_memory(image, memory, 0) }?; - - let pixels = make_frame_bgra(); - let (staging, staging_mem) = unsafe { - host_buffer( - context, - pixels.len() as u64, - vk::BufferUsageFlags::TRANSFER_SRC, - )? - }; - unsafe { - let ptr = device.map_memory( - staging_mem, - 0, - pixels.len() as u64, - vk::MemoryMapFlags::empty(), - )?; - std::ptr::copy_nonoverlapping(pixels.as_ptr(), ptr as *mut u8, pixels.len()); - device.unmap_memory(staging_mem); - } - - let range = vk::ImageSubresourceRange { - aspect_mask: vk::ImageAspectFlags::COLOR, - base_mip_level: 0, - level_count: 1, - base_array_layer: 0, - layer_count: 1, - }; - unsafe { - one_shot(context, |cb| { - let to_dst = vk::ImageMemoryBarrier::default() - .old_layout(vk::ImageLayout::UNDEFINED) - .new_layout(vk::ImageLayout::TRANSFER_DST_OPTIMAL) - .src_queue_family_index(vk::QUEUE_FAMILY_IGNORED) - .dst_queue_family_index(vk::QUEUE_FAMILY_IGNORED) - .image(image) - .subresource_range(range) - .dst_access_mask(vk::AccessFlags::TRANSFER_WRITE); - device.cmd_pipeline_barrier( - cb, - vk::PipelineStageFlags::TOP_OF_PIPE, - vk::PipelineStageFlags::TRANSFER, - vk::DependencyFlags::empty(), - &[], - &[], - &[to_dst], - ); - let copy = vk::BufferImageCopy::default() - .image_subresource(vk::ImageSubresourceLayers { - aspect_mask: vk::ImageAspectFlags::COLOR, - mip_level: 0, - base_array_layer: 0, - layer_count: 1, - }) - .image_extent(vk::Extent3D { - width: WIDTH, - height: HEIGHT, - depth: 1, - }); - device.cmd_copy_buffer_to_image( - cb, - staging, - image, - vk::ImageLayout::TRANSFER_DST_OPTIMAL, - &[copy], - ); - let to_read = vk::ImageMemoryBarrier::default() - .old_layout(vk::ImageLayout::TRANSFER_DST_OPTIMAL) - .new_layout(vk::ImageLayout::SHADER_READ_ONLY_OPTIMAL) - .src_queue_family_index(vk::QUEUE_FAMILY_IGNORED) - .dst_queue_family_index(vk::QUEUE_FAMILY_IGNORED) - .image(image) - .subresource_range(range) - .src_access_mask(vk::AccessFlags::TRANSFER_WRITE) - .dst_access_mask(vk::AccessFlags::SHADER_READ); - device.cmd_pipeline_barrier( - cb, - vk::PipelineStageFlags::TRANSFER, - vk::PipelineStageFlags::ALL_COMMANDS, - vk::DependencyFlags::empty(), - &[], - &[], - &[to_read], - ); - })?; - device.destroy_buffer(staging, None); - device.free_memory(staging_mem, None); - } - Ok(SrcImage { - context: context.clone(), - image, - memory, - }) -} - /// Read the converter's luma plane back as code values. fn read_luma( context: &VideoContext, @@ -435,7 +240,7 @@ fn convert( #[ignore = "requires a Vulkan Video device"] fn quantizers_round_rather_than_truncate() -> Result<(), Box> { let context = context()?; - let src = unsafe { create_src_image(&context)? }; + let src = unsafe { create_src_image(&context, WIDTH, HEIGHT, &make_frame_bgra())? }; let (eight, ten) = encoders(&context)?; for (output_format, encoder) in [ @@ -505,7 +310,7 @@ fn quantizers_round_rather_than_truncate() -> Result<(), Box Result<(), Box> { let context = context()?; - let src = unsafe { create_src_image(&context)? }; + let src = unsafe { create_src_image(&context, WIDTH, HEIGHT, &make_frame_bgra())? }; let (eight, ten) = encoders(&context)?; let supported = [ diff --git a/tests/common/mod.rs b/tests/common/mod.rs index 39b421b4..9f1eeb87 100644 --- a/tests/common/mod.rs +++ b/tests/common/mod.rs @@ -17,6 +17,8 @@ //! once is undefined, so these examples drive the decoder and the readback //! from the same thread. +pub mod source; + use ash::vk; use pixelforge::decoder::{DecodedFrame, Decoder, FramePoll}; use pixelforge::encoder::BitDepth; diff --git a/tests/common/source.rs b/tests/common/source.rs new file mode 100644 index 00000000..b0ffa0a0 --- /dev/null +++ b/tests/common/source.rs @@ -0,0 +1,207 @@ +//! A synthetic RGB frame on the GPU, as a colour converter's input, and the +//! small submission helpers it is built with. + +use ash::vk; +use pixelforge::VideoContext; + +/// The synthetic frame, on the GPU, cleaned up when the test drops it. +pub struct SrcImage { + /// Keeps the device alive until after the image is destroyed. + context: VideoContext, + pub image: vk::Image, + memory: vk::DeviceMemory, +} + +impl Drop for SrcImage { + fn drop(&mut self) { + let device = self.context.device(); + unsafe { + device.destroy_image(self.image, None); + device.free_memory(self.memory, None); + } + } +} + +pub unsafe fn one_shot( + context: &VideoContext, + record: F, +) -> Result<(), Box> { + let device = context.device(); + let pool_info = vk::CommandPoolCreateInfo::default() + .queue_family_index(context.transfer_queue_family()) + .flags(vk::CommandPoolCreateFlags::TRANSIENT); + let pool = unsafe { device.create_command_pool(&pool_info, None) }?; + let cb_info = vk::CommandBufferAllocateInfo::default() + .command_pool(pool) + .level(vk::CommandBufferLevel::PRIMARY) + .command_buffer_count(1); + let cb = unsafe { device.allocate_command_buffers(&cb_info) }?[0]; + unsafe { + device.begin_command_buffer( + cb, + &vk::CommandBufferBeginInfo::default() + .flags(vk::CommandBufferUsageFlags::ONE_TIME_SUBMIT), + )?; + record(cb); + device.end_command_buffer(cb)?; + let cbs = [cb]; + let submit = vk::SubmitInfo::default().command_buffers(&cbs); + device.queue_submit(context.transfer_queue(), &[submit], vk::Fence::null())?; + device.queue_wait_idle(context.transfer_queue())?; + device.destroy_command_pool(pool, None); + } + Ok(()) +} + +pub unsafe fn host_buffer( + context: &VideoContext, + size: u64, + usage: vk::BufferUsageFlags, +) -> Result<(vk::Buffer, vk::DeviceMemory), Box> { + let device = context.device(); + let info = vk::BufferCreateInfo::default() + .size(size) + .usage(usage) + .sharing_mode(vk::SharingMode::EXCLUSIVE); + let buffer = unsafe { device.create_buffer(&info, None) }?; + let reqs = unsafe { device.get_buffer_memory_requirements(buffer) }; + let mem_type = context + .find_memory_type( + reqs.memory_type_bits, + vk::MemoryPropertyFlags::HOST_VISIBLE | vk::MemoryPropertyFlags::HOST_COHERENT, + ) + .ok_or("no host visible memory")?; + let alloc = vk::MemoryAllocateInfo::default() + .allocation_size(reqs.size) + .memory_type_index(mem_type); + let memory = unsafe { device.allocate_memory(&alloc, None) }?; + unsafe { device.bind_buffer_memory(buffer, memory, 0) }?; + Ok((buffer, memory)) +} + +/// Upload `pixels`, tightly packed BGRA8, as a `width` x `height` image left in +/// `SHADER_READ_ONLY_OPTIMAL`, ready for the colour converter. +pub unsafe fn create_src_image( + context: &VideoContext, + width: u32, + height: u32, + pixels: &[u8], +) -> Result> { + let device = context.device(); + let info = vk::ImageCreateInfo::default() + .image_type(vk::ImageType::TYPE_2D) + .format(vk::Format::B8G8R8A8_UNORM) + .extent(vk::Extent3D { + width, + height, + depth: 1, + }) + .mip_levels(1) + .array_layers(1) + .samples(vk::SampleCountFlags::TYPE_1) + .tiling(vk::ImageTiling::OPTIMAL) + .usage(vk::ImageUsageFlags::SAMPLED | vk::ImageUsageFlags::TRANSFER_DST) + .sharing_mode(vk::SharingMode::EXCLUSIVE) + .initial_layout(vk::ImageLayout::UNDEFINED); + let image = unsafe { device.create_image(&info, None) }?; + let reqs = unsafe { device.get_image_memory_requirements(image) }; + let mem_type = context + .find_memory_type(reqs.memory_type_bits, vk::MemoryPropertyFlags::DEVICE_LOCAL) + .ok_or("no device local memory")?; + let alloc = vk::MemoryAllocateInfo::default() + .allocation_size(reqs.size) + .memory_type_index(mem_type); + let memory = unsafe { device.allocate_memory(&alloc, None) }?; + unsafe { device.bind_image_memory(image, memory, 0) }?; + + let (staging, staging_mem) = unsafe { + host_buffer( + context, + pixels.len() as u64, + vk::BufferUsageFlags::TRANSFER_SRC, + )? + }; + unsafe { + let ptr = device.map_memory( + staging_mem, + 0, + pixels.len() as u64, + vk::MemoryMapFlags::empty(), + )?; + std::ptr::copy_nonoverlapping(pixels.as_ptr(), ptr as *mut u8, pixels.len()); + device.unmap_memory(staging_mem); + } + + let range = vk::ImageSubresourceRange { + aspect_mask: vk::ImageAspectFlags::COLOR, + base_mip_level: 0, + level_count: 1, + base_array_layer: 0, + layer_count: 1, + }; + unsafe { + one_shot(context, |cb| { + let to_dst = vk::ImageMemoryBarrier::default() + .old_layout(vk::ImageLayout::UNDEFINED) + .new_layout(vk::ImageLayout::TRANSFER_DST_OPTIMAL) + .src_queue_family_index(vk::QUEUE_FAMILY_IGNORED) + .dst_queue_family_index(vk::QUEUE_FAMILY_IGNORED) + .image(image) + .subresource_range(range) + .dst_access_mask(vk::AccessFlags::TRANSFER_WRITE); + device.cmd_pipeline_barrier( + cb, + vk::PipelineStageFlags::TOP_OF_PIPE, + vk::PipelineStageFlags::TRANSFER, + vk::DependencyFlags::empty(), + &[], + &[], + &[to_dst], + ); + let copy = vk::BufferImageCopy::default() + .image_subresource(vk::ImageSubresourceLayers { + aspect_mask: vk::ImageAspectFlags::COLOR, + mip_level: 0, + base_array_layer: 0, + layer_count: 1, + }) + .image_extent(vk::Extent3D { + width, + height, + depth: 1, + }); + device.cmd_copy_buffer_to_image( + cb, + staging, + image, + vk::ImageLayout::TRANSFER_DST_OPTIMAL, + &[copy], + ); + let to_read = vk::ImageMemoryBarrier::default() + .old_layout(vk::ImageLayout::TRANSFER_DST_OPTIMAL) + .new_layout(vk::ImageLayout::SHADER_READ_ONLY_OPTIMAL) + .src_queue_family_index(vk::QUEUE_FAMILY_IGNORED) + .dst_queue_family_index(vk::QUEUE_FAMILY_IGNORED) + .image(image) + .subresource_range(range) + .src_access_mask(vk::AccessFlags::TRANSFER_WRITE) + .dst_access_mask(vk::AccessFlags::SHADER_READ); + device.cmd_pipeline_barrier( + cb, + vk::PipelineStageFlags::TRANSFER, + vk::PipelineStageFlags::ALL_COMMANDS, + vk::DependencyFlags::empty(), + &[], + &[], + &[to_read], + ); + })?; + device.destroy_buffer(staging, None); + device.free_memory(staging_mem, None); + } + Ok(SrcImage { + context: context.clone(), + image, + memory, + }) +} From d7dbaa34ff3910fef4cb37d8e26a03c1bc546514 Mon Sep 17 00:00:00 2001 From: DatCaptainHorse Date: Sat, 17 Oct 2026 21:51:12 +0300 Subject: [PATCH 3/7] fix(converter): read each source image through a view of its own The converter kept one image view and reused it whenever the next source had the same VkImage handle. A handle only identifies an image while it exists: drivers hand a new image a destroyed one's handle, and RADV does so readily. A caller that destroyed its source after each frame and created the next got every frame after the first converted from a view of the first, freed image. On RADV that meant a garbage picture from the second frame on, with nothing reporting an error. The validation layer gives every object a unique handle, so it hides this entirely, and none of the existing tests replaced a source image. The new conversion test does, and fails on RADV without this change. A view is now made for each conversion and destroyed once that conversion has finished. Creating a view is cheap next to a frame's dispatch and copy, and this way nothing survives the image it was made from. Co-Authored-By: Claude Opus 5.5 --- src/converter/mod.rs | 46 +++++++++++++++----------------- src/converter/pipeline.rs | 2 +- tests/color_conversion.rs | 55 +++++++++++++++++++++++++++++++++++++++ 3 files changed, 77 insertions(+), 26 deletions(-) diff --git a/src/converter/mod.rs b/src/converter/mod.rs index b0eb6649..07f6f20f 100644 --- a/src/converter/mod.rs +++ b/src/converter/mod.rs @@ -369,8 +369,11 @@ pub struct ColorConverter { // Sampler for texelFetch on the source image. sampler: vk::Sampler, - // Cached ImageView for the source image (avoids per-frame recreation). - cached_src_view: Option<(vk::Image, vk::ImageView)>, + // The view the current conversion reads its source through. A view is + // made per conversion rather than cached by image handle: a driver may + // give a new image a destroyed one's handle, and a cache would then read + // the new image through a view of the old one. + src_view: Option, // Output buffer (compute shader writes here). output_buffer: vk::Buffer, @@ -758,9 +761,9 @@ impl ColorConverter { ) -> Result<()> { let start = std::time::Instant::now(); - // Get or create ImageView for the source image (must happen before - // borrowing device immutably, since this takes &mut self). - let src_view = self.get_or_create_src_view(src_image)?; + // A conversion that failed part way may have left its view behind. + self.destroy_src_view(); + let src_view = self.create_src_view(src_image)?; let device = self.context.device(); @@ -1038,6 +1041,7 @@ impl ColorConverter { .wait_for_fences(&[self.fence], true, u64::MAX) .map_err(|e| PixelForgeError::CommandBuffer(e.to_string()))?; } + self.destroy_src_view(); let elapsed = start.elapsed(); debug!("ColorConverter::convert() took {:?}", elapsed); @@ -1045,20 +1049,9 @@ impl ColorConverter { Ok(()) } - /// Get or create an ImageView for the source image. - fn get_or_create_src_view(&mut self, src_image: vk::Image) -> Result { - // Return cached view if it matches the current source image. - if let Some((cached_image, cached_view)) = self.cached_src_view { - if cached_image == src_image { - return Ok(cached_view); - } - // Different image — destroy the old view. - unsafe { - self.context.device().destroy_image_view(cached_view, None); - } - } - - // Create a new ImageView for the source image. + /// Create the view this conversion reads `src_image` through. It lives + /// until the conversion is done, see [`Self::destroy_src_view`]. + fn create_src_view(&mut self, src_image: vk::Image) -> Result { let view_info = vk::ImageViewCreateInfo::default() .image(src_image) .view_type(vk::ImageViewType::TYPE_2D) @@ -1074,21 +1067,24 @@ impl ColorConverter { let view = unsafe { self.context.device().create_image_view(&view_info, None) } .map_err(|e| PixelForgeError::ResourceCreation(format!("source image view: {}", e)))?; - self.cached_src_view = Some((src_image, view)); + self.src_view = Some(view); Ok(view) } + + /// Destroy the source view, once nothing on the GPU reads through it. + fn destroy_src_view(&mut self) { + if let Some(view) = self.src_view.take() { + unsafe { self.context.device().destroy_image_view(view, None) }; + } + } } impl Drop for ColorConverter { fn drop(&mut self) { + self.destroy_src_view(); unsafe { let device = self.context.device(); - // Destroy cached source image view. - if let Some((_, view)) = self.cached_src_view.take() { - device.destroy_image_view(view, None); - } - // Destroy sampler. device.destroy_sampler(self.sampler, None); diff --git a/src/converter/pipeline.rs b/src/converter/pipeline.rs index 29d8248e..65c9aa2e 100644 --- a/src/converter/pipeline.rs +++ b/src/converter/pipeline.rs @@ -170,7 +170,7 @@ pub fn create_converter( pipeline_layout, pipeline, sampler, - cached_src_view: None, + src_view: None, output_buffer, output_memory, output_buffer_size: output_size, diff --git a/tests/color_conversion.rs b/tests/color_conversion.rs index 41504b1a..73bb3d01 100644 --- a/tests/color_conversion.rs +++ b/tests/color_conversion.rs @@ -414,3 +414,58 @@ fn unsupported_conversions_are_refused() -> Result<(), Box Result<(), Box> { + let context = context()?; + let (encoder, _) = encoders(&context)?; + let first = make_frame_bgra(); + let second: Vec = first + .as_chunks::<4>() + .0 + .iter() + .flat_map(|&[b, g, r, a]| [255 - b, 255 - g, 255 - r, a]) + .collect(); + + let config = ColorConverterConfig::new( + WIDTH, + HEIGHT, + InputFormat::BGRA, + OutputFormat::NV12, + ColorSpec::Srgb, + ColorSpec::Srgb, + ColorRange::Limited, + ); + let convert_with = |converter: &mut ColorConverter, pixels: &[u8]| { + let src = unsafe { create_src_image(&context, WIDTH, HEIGHT, pixels)? }; + converter.convert( + src.image, + vk::ImageLayout::SHADER_READ_ONLY_OPTIMAL, + encoder.input_image(), + )?; + let luma = read_luma(&context, converter, OutputFormat::NV12)?; + drop(src); + Ok::<_, Box>(luma) + }; + + let expected = convert_with( + &mut ColorConverter::new(context.clone(), config.clone())?, + &second, + )?; + let mut converter = ColorConverter::new(context.clone(), config)?; + convert_with(&mut converter, &first)?; + let actual = convert_with(&mut converter, &second)?; + assert!( + actual == expected, + "the second frame came out as something other than the second frame" + ); + Ok(()) +} From beab87452326fb02058a89070af286304b27dc3d Mon Sep 17 00:00:00 2001 From: DatCaptainHorse Date: Wed, 23 Sep 2026 23:24:29 +0300 Subject: [PATCH 4/7] fix(encoder): order a DPB slot's layout transition before the encode The barrier moving the setup slot into VIDEO_ENCODE_DPB_KHR had an empty destination access mask. Its execution dependency held, but no memory dependency did: the encode's write of the reconstructed picture was outside the barrier's second scope, so the transition and that write were unordered. Synchronization validation reports it as a write-after-write hazard in vkCmdEncodeVideoKHR, once per DPB slot on first use, on RADV and ANV alike. The DPB barriers now use synchronization2, which has video encode stages and access flags, in place of the original barrier API, which has neither and could only say ALL_COMMANDS and MEMORY_*. Every one is from the encode stage to the encode stage. The setup slot's destination is encode read and write, which is the fix. Its source is now the encode's write as well, where it was empty. When a slot is reused, the earlier encode's write to it has to be ordered before the new one. Today the queue's signal operation between the two submissions already does that, and synchronization validation, with submit-time checks on, reports nothing either way on RADV or ANV. Naming the write keeps the barrier correct without leaning on that. Co-Authored-By: Claude Opus 5.5 --- src/encoder/resources.rs | 46 ++++++++++++++++++++++------------------ 1 file changed, 25 insertions(+), 21 deletions(-) diff --git a/src/encoder/resources.rs b/src/encoder/resources.rs index ae4c5867..31278a60 100644 --- a/src/encoder/resources.rs +++ b/src/encoder/resources.rs @@ -618,8 +618,9 @@ pub(crate) fn upload_image_to_input( /// Record DPB image barriers for encode. /// -/// Transitions the setup DPB slot from UNDEFINED to VIDEO_ENCODE_DPB and -/// adds execution barriers for reference slot images. +/// Brings the setup slot into VIDEO_ENCODE_DPB_KHR, from UNDEFINED on its +/// first use, and orders every slot this encode touches after the encodes +/// that wrote it before. /// /// # Safety /// @@ -653,7 +654,19 @@ pub(crate) unsafe fn record_dpb_barriers( vk::ImageLayout::UNDEFINED }; - let dpb_barrier = vk::ImageMemoryBarrier::default() + // Only encodes touch a DPB slot, so both sides of every barrier are the + // encode stage. The setup slot is about to be written: the transition and + // the write of the reconstructed picture both have to come after any + // earlier encode's write to the same slot, and the write has to come after + // the transition. A reference slot is only read, after the write that put + // the picture there. + let dpb_barrier = vk::ImageMemoryBarrier2::default() + .src_stage_mask(vk::PipelineStageFlags2::VIDEO_ENCODE_KHR) + .src_access_mask(vk::AccessFlags2::VIDEO_ENCODE_WRITE_KHR) + .dst_stage_mask(vk::PipelineStageFlags2::VIDEO_ENCODE_KHR) + .dst_access_mask( + vk::AccessFlags2::VIDEO_ENCODE_READ_KHR | vk::AccessFlags2::VIDEO_ENCODE_WRITE_KHR, + ) .old_layout(setup_old_layout) .new_layout(vk::ImageLayout::VIDEO_ENCODE_DPB_KHR) .src_queue_family_index(vk::QUEUE_FAMILY_IGNORED) @@ -665,9 +678,7 @@ pub(crate) unsafe fn record_dpb_barriers( level_count: 1, base_array_layer: dpb_base_array_layer, layer_count: 1, - }) - .src_access_mask(vk::AccessFlags::empty()) - .dst_access_mask(vk::AccessFlags::empty()); + }); let mut all_barriers = vec![dpb_barrier]; @@ -678,7 +689,11 @@ pub(crate) unsafe fn record_dpb_barriers( (dpb_images[ref_slot as usize], 0u32) }; all_barriers.push( - vk::ImageMemoryBarrier::default() + vk::ImageMemoryBarrier2::default() + .src_stage_mask(vk::PipelineStageFlags2::VIDEO_ENCODE_KHR) + .src_access_mask(vk::AccessFlags2::VIDEO_ENCODE_WRITE_KHR) + .dst_stage_mask(vk::PipelineStageFlags2::VIDEO_ENCODE_KHR) + .dst_access_mask(vk::AccessFlags2::VIDEO_ENCODE_READ_KHR) .old_layout(vk::ImageLayout::VIDEO_ENCODE_DPB_KHR) .new_layout(vk::ImageLayout::VIDEO_ENCODE_DPB_KHR) .src_queue_family_index(vk::QUEUE_FAMILY_IGNORED) @@ -690,23 +705,12 @@ pub(crate) unsafe fn record_dpb_barriers( level_count: 1, base_array_layer: ref_layer, layer_count: 1, - }) - .src_access_mask(vk::AccessFlags::MEMORY_WRITE) - .dst_access_mask(vk::AccessFlags::MEMORY_READ), + }), ); } - unsafe { - device.cmd_pipeline_barrier( - command_buffer, - vk::PipelineStageFlags::ALL_COMMANDS, - vk::PipelineStageFlags::ALL_COMMANDS, - vk::DependencyFlags::empty(), - &[], - &[], - &all_barriers, - ); - } + let dependency = vk::DependencyInfo::default().image_memory_barriers(&all_barriers); + unsafe { device.cmd_pipeline_barrier2(command_buffer, &dependency) }; } /// Prepare an encode command buffer for recording. From a082229d82a8e3b1161a249f2f352e15d17e02ea Mon Sep 17 00:00:00 2001 From: DatCaptainHorse Date: Wed, 23 Sep 2026 09:41:01 +0300 Subject: [PATCH 5/7] test: show validation messages from the colour conversion tests pixelforge sends the validation layer's findings to tracing. roundtrip, rfi and verify_all install a subscriber; color_conversion did not, so with validation on its messages went nowhere, and a run of it printing nothing proved nothing. tests/common gains init_logging(), a tracing subscriber at RUST_LOG's level (warn when unset) writing through the test harness, and color_conversion calls it before building a context. Run with --nocapture to see a passing test's output. The first run with it shows what had been hidden: the source image is described to the converter in a layout it is no longer in, fixed in the next commit. Co-Authored-By: Claude Opus 5.5 --- tests/color_conversion.rs | 1 + tests/common/mod.rs | 20 ++++++++++++++++++++ 2 files changed, 21 insertions(+) diff --git a/tests/color_conversion.rs b/tests/color_conversion.rs index 73bb3d01..ba91ea8b 100644 --- a/tests/color_conversion.rs +++ b/tests/color_conversion.rs @@ -200,6 +200,7 @@ fn encoders( } fn context() -> Result> { + common::init_logging(); Ok(VideoContextBuilder::new() .app_name("pixelforge-color-conversion") .require_encode(Codec::H264) diff --git a/tests/common/mod.rs b/tests/common/mod.rs index 9f1eeb87..4aabcff2 100644 --- a/tests/common/mod.rs +++ b/tests/common/mod.rs @@ -26,6 +26,26 @@ use pixelforge::vulkan::VideoContext; use std::fs::File; use std::io::Write; +/// Show pixelforge's log, validation messages included, on the test's output. +/// +/// pixelforge sends the validation layer's findings to `tracing`, and without +/// a subscriber they go nowhere: a test that enabled validation and printed +/// nothing proved nothing. Call before building a context. The level comes +/// from `RUST_LOG`, `warn` when unset, so validation errors always show. +/// Output is captured like any other test output: run with `--nocapture` to +/// see a passing test's. +pub fn init_logging() { + static ONCE: std::sync::Once = std::sync::Once::new(); + ONCE.call_once(|| { + let filter = tracing_subscriber::EnvFilter::try_from_default_env() + .unwrap_or_else(|_| tracing_subscriber::EnvFilter::new("warn")); + let _ = tracing_subscriber::fmt() + .with_env_filter(filter) + .with_test_writer() + .try_init(); + }); +} + /// Bytes handed to the decoder per call, standing in for a network read. pub const CHUNK_SIZE: usize = 64 * 1024; From 9bc9ad341762c87087f3fb41d254923aa8cbcd31 Mon Sep 17 00:00:00 2001 From: DatCaptainHorse Date: Wed, 23 Sep 2026 09:39:01 +0300 Subject: [PATCH 6/7] test: keep the synthetic source in GENERAL, as the converter leaves it The converter returns its source image to GENERAL after every conversion. The tests created their source in SHADER_READ_ONLY_OPTIMAL and kept saying so on every call, so every conversion after the first read through a descriptor written for a layout the image was no longer in. Descriptor buffers were never tracked for that; push descriptors are, and the validation layer said so 12 times in color_conversion. The source now starts in GENERAL and every call says GENERAL. Co-Authored-By: Claude Opus 5.5 --- tests/color_conversion.rs | 12 ++---------- tests/common/source.rs | 9 +++++++-- 2 files changed, 9 insertions(+), 12 deletions(-) diff --git a/tests/color_conversion.rs b/tests/color_conversion.rs index ba91ea8b..3cea06f6 100644 --- a/tests/color_conversion.rs +++ b/tests/color_conversion.rs @@ -229,11 +229,7 @@ fn convert( ); let mut converter = ColorConverter::new(context.clone(), config)?; let description = converter.color_description(); - converter.convert( - src.image, - vk::ImageLayout::SHADER_READ_ONLY_OPTIMAL, - encoder.input_image(), - )?; + converter.convert(src.image, vk::ImageLayout::GENERAL, encoder.input_image())?; Ok((read_luma(context, &converter, output_format)?, description)) } @@ -447,11 +443,7 @@ fn a_replaced_source_image_is_read_afresh() -> Result<(), Box>(luma) diff --git a/tests/common/source.rs b/tests/common/source.rs index b0ffa0a0..58aec1f2 100644 --- a/tests/common/source.rs +++ b/tests/common/source.rs @@ -80,7 +80,12 @@ pub unsafe fn host_buffer( } /// Upload `pixels`, tightly packed BGRA8, as a `width` x `height` image left in -/// `SHADER_READ_ONLY_OPTIMAL`, ready for the colour converter. +/// `GENERAL`. +/// +/// `GENERAL` because that is where the colour converter leaves its source after +/// every conversion, so it is the layout the image is in for every conversion +/// after the first. Starting it anywhere else means a second conversion told +/// the wrong layout. pub unsafe fn create_src_image( context: &VideoContext, width: u32, @@ -179,7 +184,7 @@ pub unsafe fn create_src_image( ); let to_read = vk::ImageMemoryBarrier::default() .old_layout(vk::ImageLayout::TRANSFER_DST_OPTIMAL) - .new_layout(vk::ImageLayout::SHADER_READ_ONLY_OPTIMAL) + .new_layout(vk::ImageLayout::GENERAL) .src_queue_family_index(vk::QUEUE_FAMILY_IGNORED) .dst_queue_family_index(vk::QUEUE_FAMILY_IGNORED) .image(image) From f2b0a5a9e82e82a83546e058d36b97c93f1ec957 Mon Sep 17 00:00:00 2001 From: DatCaptainHorse Date: Sat, 17 Oct 2026 21:52:40 +0300 Subject: [PATCH 7/7] docs: changelog for push descriptors and the two fixes, test notes CHANGELOG covers the move to push descriptors, the features that had been enabled only alongside descriptor buffers, the converter's stale source view and the DPB barrier. AGENTS.md says that integration tests need common::init_logging() for their validation messages to go anywhere, and why the ignored tests need a run without validation too: the layer gives every object a unique handle, which hid the converter's stale view. Co-Authored-By: Claude Opus 5.5 --- AGENTS.md | 16 +++++++++++++--- CHANGELOG.md | 16 ++++++++++++++++ 2 files changed, 29 insertions(+), 3 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 5ba7a8ec..c69eb37c 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -113,9 +113,19 @@ not compare. Make sure there are no Vulkan validation layer errors during execution. Enable them with `PIXELFORGE_VALIDATION=1`; the layer's messages are routed through `tracing`, so pair it with `RUST_LOG=warn` (or `debug` for the layer's own -chatter). Without `VK_LAYER_KHRONOS_validation` installed, pixelforge logs a -warning and carries on with validation disabled, so absence of errors means -nothing if the layer is missing. +chatter). An integration test needs a subscriber installed for any of it to +show: `common::init_logging()` installs one, and a new test that builds a +context has to call it or install its own, or its validation messages go +nowhere. They are captured like any test output, so run with `--nocapture` to +see a passing test's. Without `VK_LAYER_KHRONOS_validation` installed, +pixelforge logs a warning and carries on with validation disabled, so absence +of errors means nothing if the layer is missing. + +Run the ignored tests once without validation as well. The layer gives every +object a unique handle, while drivers reuse the handles of destroyed objects +freely, so anything that remembers state by handle can pass with validation +on and fail without it. That has happened: the converter once cached an image +view by image handle, and only RADV without validation showed it. ## Code Style - Follow `rustfmt.toml` formatting rules diff --git a/CHANGELOG.md b/CHANGELOG.md index fe0b4de0..46d67387 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -105,9 +105,25 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changed +- The colour converter binds its descriptors with push descriptors instead of + `VK_EXT_descriptor_buffer`, which is deprecated. It now needs only + `VK_KHR_push_descriptor`, and no longer buffer device addresses or + descriptor buffer capture-replay. `VideoContext::has_descriptor_buffer` is + replaced by `VideoContext::has_push_descriptor`. - Adopting a caller-created device for decode (`build_from_existing_decode`) now requires the `timelineSemaphore` feature in addition to `synchronization2`. +### Fixed + +- `samplerYcbcrConversion` and the 2-plane 4:4:4 formats feature were only + enabled on devices that also had descriptor buffers. +- The colour converter reused its source image view whenever the next source + had the same handle. Drivers reuse the handles of destroyed images, so a + caller creating a new source per frame got every frame after the first + converted from a freed image. +- The barrier moving a DPB slot into its encode layout did not order the + transition before the encode's write of the reconstructed picture. + ## [0.9.1] - 01-09-2026 ### Fixed