From a062cad337c47dbb3b59c777ff79c45dfa361aa8 Mon Sep 17 00:00:00 2001 From: DatCaptainHorse Date: Wed, 23 Sep 2026 08:08:32 +0300 Subject: [PATCH 01/16] 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, and a pipeline flag that was never set (the undefined behaviour hgaiser/pixelforge#40 reports). 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 | 158 ++++++++------------------------------ src/encoder/resources.rs | 6 +- src/video.rs | 60 --------------- src/vulkan.rs | 93 +++++++--------------- 5 files changed, 86 insertions(+), 344 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 24ce2812..29d8248e 100644 --- a/src/converter/pipeline.rs +++ b/src/converter/pipeline.rs @@ -8,7 +8,6 @@ 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"); @@ -36,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 = [ @@ -62,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) } @@ -126,111 +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. + // 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::BufferUsageFlags::SHADER_DEVICE_ADDRESS, + | vk::BufferUsageFlags::TRANSFER_DST, 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; + 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() @@ -265,23 +174,10 @@ 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, }) } @@ -303,27 +199,37 @@ fn create_buffer( let mem_requirements = unsafe { device.get_buffer_memory_requirements(buffer) }; - let memory_type_index = find_memory_type( + let Some(memory_type_index) = find_memory_type( memory_properties, mem_requirements.memory_type_bits, properties, - ) - .ok_or_else(|| { - PixelForgeError::MemoryAllocation(format!( + ) 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 = unsafe { device.allocate_memory(&alloc_info, None) } - .map_err(|e| PixelForgeError::MemoryAllocation(e.to_string()))?; + 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())); + } + }; - unsafe { device.bind_buffer_memory(buffer, memory, 0) } - .map_err(|e| 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 a993d0b7..fe0a9753 100644 --- a/src/vulkan.rs +++ b/src/vulkan.rs @@ -411,7 +411,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 @@ -520,9 +520,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. @@ -662,7 +663,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 { @@ -762,10 +762,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) @@ -949,11 +945,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. @@ -1018,6 +1017,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); @@ -1042,41 +1047,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 { @@ -1098,20 +1068,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) } @@ -1154,7 +1119,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, @@ -1231,7 +1196,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 b292ed4bae0748ea7dfade5274c5671a0243e43f Mon Sep 17 00:00:00 2001 From: DatCaptainHorse Date: Wed, 23 Sep 2026 08:10:46 +0300 Subject: [PATCH 02/16] feat(vulkan): let an adopted context use queues the caller chooses A context adopted from a caller's device took queue 0 of every family it needed. On most hardware queue 0 of the transfer and compute families is the caller's own graphics queue, and a VkQueue may not be submitted to from two threads at once. A renderer that kept drawing while pixelforge decoded on another thread was racing it on its own queue, with no way to avoid it short of a lock around every submission on both sides. The builder now takes a DeviceQueue (family and index) per role, so the caller can create spare queues and hand those over. Each choice is checked against what the role needs and against how many queues the family offers; whether the caller actually created that many is not something Vulkan reports, so that part stays a documented requirement. DeviceRequirements now also says which family serves which role, so the caller knows where a spare queue is worth creating, and whether the device supports VK_KHR_internally_synchronized_queues. That extension is the other way out, on hardware with no queue to spare: a queue created with its flag may be shared, the driver doing the locking. Pixelforge needs no part in it, so it is reported rather than required. decode_adopted now keeps queue 0 to itself wherever a family has a second queue, which also puts the non-zero-index path under test. Co-Authored-By: Claude Opus 5.5 --- examples/decode_adopted.rs | 25 ++++-- src/vulkan.rs | 152 ++++++++++++++++++++++++++++------ src/vulkan/queues.rs | 164 +++++++++++++++++++++++++++++++++++++ 3 files changed, 313 insertions(+), 28 deletions(-) create mode 100644 src/vulkan/queues.rs diff --git a/examples/decode_adopted.rs b/examples/decode_adopted.rs index 6e6d077f..e5bc6c8a 100644 --- a/examples/decode_adopted.rs +++ b/examples/decode_adopted.rs @@ -20,7 +20,7 @@ use common::{Readback, decode_stream, write_nv12}; use pixelforge::decoder::{DecodeConfig, Decoder}; use pixelforge::encoder::Codec; -use pixelforge::vulkan::VideoContextBuilder; +use pixelforge::vulkan::{DeviceQueue, VideoContextBuilder}; fn main() -> Result<(), Box> { let mut args = std::env::args().skip(1); @@ -67,18 +67,28 @@ fn main() -> Result<(), Box> { reqs.extensions.len() ); - // The app merges pixelforge's requirements with its own. Here there is - // nothing else, so we use them directly. - let priorities = [1.0f32]; + // The app merges pixelforge's requirements with its own. A real app keeps + // queue 0 of each family for itself, so where a family has a second queue + // this one creates it and hands that to pixelforge instead: a `VkQueue` + // may not be submitted to from two threads at once, and this is how the + // app keeps its own queue to itself. + let family_props = + unsafe { instance.get_physical_device_queue_family_properties(physical_device) }; + let priorities = [1.0f32; 2]; let queue_infos: Vec = reqs .queue_families .iter() .map(|&f| { + let count = family_props[f as usize].queue_count.min(2) as usize; vk::DeviceQueueCreateInfo::default() .queue_family_index(f) - .queue_priorities(&priorities) + .queue_priorities(&priorities[..count]) }) .collect(); + let spare = |family: u32| { + let index = family_props[family as usize].queue_count.min(2) - 1; + DeviceQueue::new(family, index) + }; let ext_ptrs: Vec<*const std::os::raw::c_char> = reqs.extensions.iter().map(|e| e.as_ptr()).collect(); @@ -106,6 +116,11 @@ fn main() -> Result<(), Box> { let device = unsafe { instance.create_device(physical_device, &device_info, None)? }; // --- Hand the app's device to pixelforge --- + let (transfer, compute) = (spare(reqs.queues.transfer), spare(reqs.queues.compute)); + println!("pixelforge gets transfer queue {transfer:?} and compute queue {compute:?}"); + let builder = builder + .with_transfer_queue(transfer) + .with_compute_queue(compute); let builder = if reqs.unified_image_layouts { builder.declare_unified_image_layouts() } else { diff --git a/src/vulkan.rs b/src/vulkan.rs index fe0a9753..46d4e9c9 100644 --- a/src/vulkan.rs +++ b/src/vulkan.rs @@ -7,6 +7,10 @@ use std::ffi::{CStr, CString}; use std::os::raw::c_char; use tracing::{debug, info, warn}; +mod queues; +use queues::QueueOverrides; +pub use queues::{DeviceQueue, QueueRoles}; + /// Route validation-layer messages into `tracing`. /// /// Without a messenger the validation layer has nowhere to report to and its @@ -60,6 +64,7 @@ pub struct VideoContextBuilder { /// enables unified image layouts when the device supports them, and one /// that adopts a caller's device assumes they were not enabled. unified_image_layouts: Option, + queues: QueueOverrides, } impl Default for VideoContextBuilder { @@ -78,6 +83,7 @@ impl VideoContextBuilder { required_encode_codecs: Vec::new(), required_decode_codecs: Vec::new(), unified_image_layouts: None, + queues: QueueOverrides::default(), } } @@ -111,6 +117,42 @@ impl VideoContextBuilder { self } + /// Submit video decode work to `queue` on an adopted device. + /// + /// See [`Self::with_transfer_queue`] for why this matters. Ignored by + /// [`Self::build`], which creates its own device and owns every queue on + /// it. + pub fn with_decode_queue(mut self, queue: DeviceQueue) -> Self { + self.queues.decode = Some(queue); + self + } + + /// Submit uploads and readback copies to `queue` on an adopted device. + /// + /// Without this, an adopted context takes queue 0 of each family it needs, + /// and on most hardware queue 0 of the transfer and compute families is the + /// caller's own graphics queue. A `VkQueue` must not be submitted to from + /// two threads at once, so a caller that keeps submitting to its queue + /// while pixelforge works on another thread has to either create a spare + /// queue and give it here, or create its queue with + /// `VK_DEVICE_QUEUE_CREATE_INTERNALLY_SYNCHRONIZED_BIT_KHR` (see + /// [`DeviceRequirements::internally_synchronized_queues`]). + /// + /// The queue's family must support transfer, and `queue.index` must be + /// below the number of queues the caller created in that family. The + /// second part cannot be checked. Ignored by [`Self::build`]. + pub fn with_transfer_queue(mut self, queue: DeviceQueue) -> Self { + self.queues.transfer = Some(queue); + self + } + + /// Submit the colour converter's dispatches to `queue` on an adopted + /// device. See [`Self::with_transfer_queue`]. Ignored by [`Self::build`]. + pub fn with_compute_queue(mut self, queue: DeviceQueue) -> Self { + self.queues.compute = Some(queue); + self + } + /// Build the VideoContext. pub fn build(self) -> Result { VideoContext::new(self) @@ -138,10 +180,21 @@ impl VideoContextBuilder { if unified_image_layouts { extensions.push(ash::khr::unified_image_layouts::NAME); } + let queues = QueueRoles { + encode: None, + decode: Some(families.decode), + transfer: families.transfer, + compute: families.compute, + }; Ok(DeviceRequirements { - queue_families: families.unique(), + queue_families: queues.unique_families(), + queues, extensions, unified_image_layouts, + internally_synchronized_queues: supports_internally_synchronized_queues( + instance, + physical_device, + ), }) } @@ -200,6 +253,7 @@ impl VideoContextBuilder { VideoContext::from_existing_decode( self.required_decode_codecs, self.unified_image_layouts.unwrap_or(false), + self.queues, entry, instance, physical_device, @@ -219,6 +273,12 @@ pub struct DeviceRequirements { /// Queue families pixelforge needs a queue created for. Merge these with /// your own (deduplicated) when building the device. pub queue_families: Vec, + /// The family behind each role in [`Self::queue_families`]. An adopted + /// context takes queue 0 of each unless told otherwise with + /// [`VideoContextBuilder::with_transfer_queue`] and friends, which is + /// where a caller that needs its own queue to itself points pixelforge at + /// a spare one. + pub queues: QueueRoles, /// Device extensions pixelforge needs enabled. Merge with your own. pub extensions: Vec<&'static std::ffi::CStr>, /// Whether this device can support unified image layouts for video, which @@ -234,6 +294,17 @@ pub struct DeviceRequirements { /// into a private image, which is correct but costs a full-frame GPU copy /// per frame. pub unified_image_layouts: bool, + /// Whether this device supports `VK_KHR_internally_synchronized_queues`. + /// + /// A queue created with `VK_DEVICE_QUEUE_CREATE_INTERNALLY_SYNCHRONIZED_BIT_KHR` + /// (and the `internallySynchronizedQueues` feature enabled) may be + /// submitted to from several threads at once, the driver doing the + /// locking. That makes it safe to give pixelforge the same queue the + /// caller uses, which is the way out on hardware with too few queues to + /// spare one. Pixelforge needs to be told nothing: it is the caller's + /// queue that has to be created this way. The extension is not in + /// [`Self::extensions`], since only a caller sharing its queue needs it. + pub internally_synchronized_queues: bool, } /// The queue families pixelforge selects for decoding on a given device. @@ -243,19 +314,6 @@ struct DecodeQueueFamilies { compute: u32, } -impl DecodeQueueFamilies { - /// The distinct families, in a stable order. - fn unique(&self) -> Vec { - let mut out = vec![self.decode]; - for f in [self.transfer, self.compute] { - if !out.contains(&f) { - out.push(f); - } - } - out - } -} - /// Select the decode / transfer / compute queue families on `physical_device`, /// failing if it cannot decode the required codecs. /// @@ -367,6 +425,29 @@ fn supports_unified_image_layouts( features.unified_image_layouts != 0 && features.unified_image_layouts_video != 0 } +/// Whether `physical_device` supports `VK_KHR_internally_synchronized_queues`, +/// extension and feature both. +fn supports_internally_synchronized_queues( + instance: &ash::Instance, + physical_device: vk::PhysicalDevice, +) -> bool { + let exts = match unsafe { instance.enumerate_device_extension_properties(physical_device) } { + Ok(exts) => exts, + Err(_) => return false, + }; + let present = exts.iter().any(|ext| { + let name = unsafe { std::ffi::CStr::from_ptr(ext.extension_name.as_ptr()) }; + name == ash::khr::internally_synchronized_queues::NAME + }); + if !present { + return false; + } + let mut features = vk::PhysicalDeviceInternallySynchronizedQueuesFeaturesKHR::default(); + let mut query = vk::PhysicalDeviceFeatures2::default().push(&mut features); + unsafe { instance.get_physical_device_features2(physical_device, &mut query) }; + features.internally_synchronized_queues != 0 +} + fn decode_extension_names(decode_codecs: &[Codec]) -> Vec<&'static std::ffi::CStr> { let mut names = vec![ ash::khr::video_queue::NAME, @@ -1135,6 +1216,7 @@ impl VideoContext { fn from_existing_decode( required_decode_codecs: Vec, declared_unified_image_layouts: bool, + queue_overrides: QueueOverrides, entry: ash::Entry, instance: ash::Instance, physical_device: vk::PhysicalDevice, @@ -1151,10 +1233,34 @@ impl VideoContext { let memory_properties = unsafe { instance.get_physical_device_memory_properties(physical_device) }; - // The caller created a queue for each family (from device_requirements). - let video_decode_queue = unsafe { device.get_device_queue(families.decode, 0) }; - let transfer_queue = unsafe { device.get_device_queue(families.transfer, 0) }; - let compute_queue = unsafe { device.get_device_queue(families.compute, 0) }; + let decode = queues::resolve( + &instance, + physical_device, + "decode", + queue_overrides.decode, + families.decode, + vk::QueueFlags::VIDEO_DECODE_KHR, + )?; + let transfer = queues::resolve( + &instance, + physical_device, + "transfer", + queue_overrides.transfer, + families.transfer, + vk::QueueFlags::TRANSFER, + )?; + let compute = queues::resolve( + &instance, + physical_device, + "compute", + queue_overrides.compute, + families.compute, + vk::QueueFlags::COMPUTE, + )?; + + let video_decode_queue = unsafe { device.get_device_queue(decode.family, decode.index) }; + let transfer_queue = unsafe { device.get_device_queue(transfer.family, transfer.index) }; + let compute_queue = unsafe { device.get_device_queue(compute.family, compute.index) }; let supported_decode_codecs = query_decode_codecs(&entry, &instance, physical_device); @@ -1173,8 +1279,8 @@ impl VideoContext { } info!( - "Adopted caller device for decode: decode family {}, transfer family {}, compute family {}", - families.decode, families.transfer, families.compute + "Adopted caller device for decode: decode queue {:?}, transfer queue {:?}, compute queue {:?}", + decode, transfer, compute ); Ok(VideoContext { @@ -1186,11 +1292,11 @@ impl VideoContext { video_encode_queue_family: None, video_encode_timestamp_valid_bits: 0u32, video_encode_queue: None, - video_decode_queue_family: Some(families.decode), + video_decode_queue_family: Some(decode.family), video_decode_queue: Some(video_decode_queue), - transfer_queue_family: families.transfer, + transfer_queue_family: transfer.family, transfer_queue, - compute_queue_family: families.compute, + compute_queue_family: compute.family, compute_queue, memory_properties, device_properties, diff --git a/src/vulkan/queues.rs b/src/vulkan/queues.rs new file mode 100644 index 00000000..0e8f2480 --- /dev/null +++ b/src/vulkan/queues.rs @@ -0,0 +1,164 @@ +//! Which queues pixelforge submits to on a device it did not create. +//! +//! A `VkQueue` must be externally synchronized: two threads may not submit to +//! it at once. A context that creates its own device owns every queue on it, +//! so the question never comes up. A context adopted from a caller's device +//! shares that device with the caller, and by default takes queue 0 of each +//! family it needs, which is very often the queue the caller renders and +//! presents on. Submitting there from another thread is a data race. +//! +//! [`DeviceQueue`] lets the caller say exactly which queue each role uses, so it +//! can create spare queues for pixelforge and keep its own to itself. + +use crate::error::{PixelForgeError, Result}; +use ash::vk; + +/// One queue on a device: a queue family and an index within it, as passed to +/// `vkGetDeviceQueue`. +#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)] +pub struct DeviceQueue { + /// Queue family index. + pub family: u32, + /// Index of the queue within its family. The caller must have created at + /// least `index + 1` queues in `family`. + pub index: u32, +} + +impl DeviceQueue { + /// Queue `index` of `family`. + pub fn new(family: u32, index: u32) -> Self { + Self { family, index } + } +} + +/// The queue family pixelforge would pick for each role it may use. +/// +/// Returned as part of [`DeviceRequirements`](super::DeviceRequirements). A +/// role that the requested work does not need is `None`. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub struct QueueRoles { + /// Video encode submissions. + pub encode: Option, + /// Video decode submissions. + pub decode: Option, + /// Uploads and readback copies. + pub transfer: u32, + /// The colour converter's compute dispatches. + pub compute: u32, +} + +impl QueueRoles { + /// The distinct families, in a stable order. + pub fn unique_families(&self) -> Vec { + let mut out = Vec::new(); + for f in [ + self.encode, + self.decode, + Some(self.transfer), + Some(self.compute), + ] + .into_iter() + .flatten() + { + if !out.contains(&f) { + out.push(f); + } + } + out + } +} + +/// Queues the caller chose for an adopted context, one per role. `None` means +/// queue 0 of the family pixelforge selects. +#[derive(Debug, Clone, Copy, Default)] +pub(crate) struct QueueOverrides { + pub decode: Option, + pub transfer: Option, + pub compute: Option, +} + +/// Resolve the queue for one role: the caller's choice if there is one, +/// checked against what the role needs, otherwise queue 0 of `default_family`. +/// +/// The index cannot be checked against how many queues the caller actually +/// created, since Vulkan does not report that; it is checked against how many +/// the family offers, which catches the mistakes that can be caught. +pub(crate) fn resolve( + instance: &ash::Instance, + physical_device: vk::PhysicalDevice, + role: &str, + chosen: Option, + default_family: u32, + needs: vk::QueueFlags, +) -> Result { + let Some(queue) = chosen else { + return Ok(DeviceQueue::new(default_family, 0)); + }; + let families = unsafe { instance.get_physical_device_queue_family_properties(physical_device) }; + let Some(props) = families.get(queue.family as usize) else { + return Err(PixelForgeError::InvalidInput(format!( + "{role} queue: family {} does not exist (the device has {})", + queue.family, + families.len() + ))); + }; + if !family_can(props.queue_flags, needs) { + return Err(PixelForgeError::InvalidInput(format!( + "{role} queue: family {} has {:?}, which does not include {:?}", + queue.family, props.queue_flags, needs + ))); + } + if queue.index >= props.queue_count { + return Err(PixelForgeError::InvalidInput(format!( + "{role} queue: index {} is out of range, family {} has {} queue(s)", + queue.index, queue.family, props.queue_count + ))); + } + Ok(queue) +} + +/// Whether a family with `flags` can do `needs`. Graphics and compute families +/// support transfer operations whether or not they advertise it. +fn family_can(flags: vk::QueueFlags, needs: vk::QueueFlags) -> bool { + let mut effective = flags; + if flags.intersects(vk::QueueFlags::GRAPHICS | vk::QueueFlags::COMPUTE) { + effective |= vk::QueueFlags::TRANSFER; + } + effective.contains(needs) +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn graphics_and_compute_imply_transfer() { + assert!(family_can( + vk::QueueFlags::GRAPHICS, + vk::QueueFlags::TRANSFER + )); + assert!(family_can( + vk::QueueFlags::COMPUTE, + vk::QueueFlags::TRANSFER + )); + assert!(!family_can( + vk::QueueFlags::VIDEO_ENCODE_KHR, + vk::QueueFlags::TRANSFER + )); + assert!(!family_can( + vk::QueueFlags::TRANSFER, + vk::QueueFlags::COMPUTE + )); + } + + #[test] + fn unique_families_keeps_first_occurrence_order() { + let roles = QueueRoles { + encode: Some(4), + decode: None, + transfer: 1, + compute: 4, + }; + assert_eq!(roles.unique_families(), vec![4, 1]); + } +} From 0af47b89e4d9d873d3e15613218733f84dcc98db Mon Sep 17 00:00:00 2001 From: DatCaptainHorse Date: Wed, 23 Sep 2026 08:15:06 +0300 Subject: [PATCH 03/16] fix(vulkan): work on adopted devices created below Vulkan 1.3 Pixelforge called vkQueueSubmit2, vkCmdPipelineBarrier2 and vkCmdCopyImage2 through their core entry points, which exist only when the instance asked for Vulkan 1.3. A device adopted from an application that asked for 1.1 or 1.2 has them null, and ash turns the first call into a panic. Nothing in the adopt API said 1.3 was required, and an application sharing its device has no reason to raise its API version for a library. The sync2 commands now go through VK_KHR_synchronization2's own table, which is valid on any device with that extension enabled; pixelforge required the extension already. The one image copy uses vkCmdCopyImage, since it needs nothing the newer entry point adds. The decode requirements now list VK_KHR_timeline_semaphore as well, for the same reason: on a 1.1 device the timeline semaphores the decoder relies on do not exist without it. The new adopted-device test builds a Vulkan 1.1 instance and device the way such an application would, hands pixelforge spare queues, and checks every decoded frame against a context pixelforge created itself on the same GPU. It needs no ffmpeg. Without this change it panics on the first barrier. Co-Authored-By: Claude Opus 5.5 --- src/decoder/common.rs | 7 +- src/decoder/pipeline.rs | 6 +- src/decoder/transfer.rs | 32 ++++--- src/encoder/codec.rs | 8 +- src/encoder/pipeline.rs | 2 + src/encoder/resources.rs | 3 +- src/vulkan.rs | 23 +++++ tests/adopted.rs | 185 +++++++++++++++++++++++++++++++++++++++ tests/common/mod.rs | 31 ++++--- 9 files changed, 262 insertions(+), 35 deletions(-) create mode 100644 tests/adopted.rs diff --git a/src/decoder/common.rs b/src/decoder/common.rs index d7379520..e55db5c5 100644 --- a/src/decoder/common.rs +++ b/src/decoder/common.rs @@ -581,7 +581,7 @@ impl DecoderCommon { /// (if distinct), into the layouts `vkCmdDecodeVideo` requires. pub fn record_barriers(&self, slot: u8) { let session = self.session.as_ref().expect("active session"); - let device = self.context.device(); + let sync2 = self.context.sync2(); let mut barriers: Vec = Vec::new(); let subresource = |layer: u32| vk::ImageSubresourceRange { @@ -633,7 +633,7 @@ impl DecoderCommon { } let dependency = vk::DependencyInfo::default().image_memory_barriers(&barriers); - unsafe { device.cmd_pipeline_barrier2(self.decode_command_buffer(), &dependency) }; + unsafe { sync2.cmd_pipeline_barrier2(self.decode_command_buffer(), &dependency) }; } /// The decode command buffer of the picture currently being recorded. @@ -674,7 +674,8 @@ impl DecoderCommon { pub fn submit_decode(&mut self) -> Result<()> { let device = self.context.device().clone(); let queue = self.decode_queue; - self.pipeline.submit_decode(&device, queue) + self.pipeline + .submit_decode(&device, self.context.sync2(), queue) } /// Close off the current picture and hand `frames` to the completion thread. diff --git a/src/decoder/pipeline.rs b/src/decoder/pipeline.rs index ab710e5f..398f3f1e 100644 --- a/src/decoder/pipeline.rs +++ b/src/decoder/pipeline.rs @@ -236,6 +236,7 @@ impl DecodePipeline { pub(crate) fn submit_decode( &mut self, device: &ash::Device, + sync2: &ash::khr::synchronization2::Device, decode_queue: vk::Queue, ) -> Result<()> { let slot = &self.slots[self.current_slot]; @@ -280,7 +281,7 @@ impl DecodePipeline { .signal_semaphore_infos(&signals); unsafe { - device + sync2 .queue_submit2(decode_queue, &[submit], slot.decode_fence) .map_err(|e| PixelForgeError::Synchronization(e.to_string()))?; } @@ -297,6 +298,7 @@ impl DecodePipeline { pub(crate) fn submit_copy( &mut self, device: &ash::Device, + sync2: &ash::khr::synchronization2::Device, transfer_queue: vk::Queue, ) -> Result<()> { let slot = &self.slots[self.current_slot]; @@ -335,7 +337,7 @@ impl DecodePipeline { .signal_semaphore_infos(&signals); unsafe { - device + sync2 .queue_submit2(transfer_queue, &[submit], slot.transfer_fence) .map_err(|e| PixelForgeError::Synchronization(e.to_string()))?; } diff --git a/src/decoder/transfer.rs b/src/decoder/transfer.rs index 03f5f53b..86752bd2 100644 --- a/src/decoder/transfer.rs +++ b/src/decoder/transfer.rs @@ -36,6 +36,7 @@ impl DecoderCommon { let command_buffer = self.pipeline.current().transfer_command_buffer; let base_layer = frame.array_layer; let device = self.context.device().clone(); + let sync2 = self.context.sync2(); let aspects = vk::ImageAspectFlags::PLANE_0 | vk::ImageAspectFlags::PLANE_1; let range = |image: vk::Image, layer: u32| vk::ImageMemoryBarrier2 { image, @@ -85,7 +86,7 @@ impl DecoderCommon { }, ]; let dep = vk::DependencyInfo::default().image_memory_barriers(&to_transfer); - unsafe { device.cmd_pipeline_barrier2(command_buffer, &dep) }; + unsafe { sync2.cmd_pipeline_barrier2(command_buffer, &dep) }; let plane = |aspect: vk::ImageAspectFlags| vk::ImageSubresourceLayers { aspect_mask: aspect, @@ -95,7 +96,7 @@ impl DecoderCommon { }; let (cdiv_hor, cdiv_vert) = frame.pixel_format.chroma_div(); let regions = [ - vk::ImageCopy2::default() + vk::ImageCopy::default() .src_subresource(vk::ImageSubresourceLayers { base_array_layer: base_layer, ..plane(vk::ImageAspectFlags::PLANE_0) @@ -106,7 +107,7 @@ impl DecoderCommon { height: frame.coded_height, depth: 1, }), - vk::ImageCopy2::default() + vk::ImageCopy::default() .src_subresource(vk::ImageSubresourceLayers { base_array_layer: base_layer, ..plane(vk::ImageAspectFlags::PLANE_1) @@ -118,13 +119,19 @@ impl DecoderCommon { depth: 1, }), ]; - let copy = vk::CopyImageInfo2::default() - .src_image(frame.image) - .src_image_layout(vk::ImageLayout::TRANSFER_SRC_OPTIMAL) - .dst_image(dst_image) - .dst_image_layout(vk::ImageLayout::TRANSFER_DST_OPTIMAL) - .regions(®ions); - unsafe { device.cmd_copy_image2(command_buffer, ©) }; + // `vkCmdCopyImage` rather than its `2` form: the copy needs nothing the + // newer entry point adds, and this one exists on every device, where + // `vkCmdCopyImage2` needs Vulkan 1.3 or `VK_KHR_copy_commands2`. + unsafe { + device.cmd_copy_image( + command_buffer, + frame.image, + vk::ImageLayout::TRANSFER_SRC_OPTIMAL, + dst_image, + vk::ImageLayout::TRANSFER_DST_OPTIMAL, + ®ions, + ) + }; // Restore the source layout so a DPB image stays a valid reference. let restore = [vk::ImageMemoryBarrier2 { @@ -137,7 +144,7 @@ impl DecoderCommon { ..range(frame.image, base_layer) }]; let dep = vk::DependencyInfo::default().image_memory_barriers(&restore); - unsafe { device.cmd_pipeline_barrier2(command_buffer, &dep) }; + unsafe { sync2.cmd_pipeline_barrier2(command_buffer, &dep) }; Ok(()) } @@ -146,6 +153,7 @@ impl DecoderCommon { pub fn submit_copy(&mut self) -> Result<()> { let device = self.context.device().clone(); let queue = self.transfer_queue; - self.pipeline.submit_copy(&device, queue) + self.pipeline + .submit_copy(&device, self.context.sync2(), queue) } } diff --git a/src/encoder/codec.rs b/src/encoder/codec.rs index b0f69b01..847701f9 100644 --- a/src/encoder/codec.rs +++ b/src/encoder/codec.rs @@ -128,9 +128,11 @@ impl EncoderCommon { let encode_queue = self.context.video_encode_queue().ok_or_else(|| { PixelForgeError::NoSuitableDevice("No video encode queue available".to_string()) })?; - let future = self - .pipeline - .submit_current(self.context.device(), encode_queue)?; + let future = self.pipeline.submit_current( + self.context.device(), + self.context.sync2(), + encode_queue, + )?; self.dpb_slot_active[self.current_dpb_slot as usize] = true; Ok(future) } diff --git a/src/encoder/pipeline.rs b/src/encoder/pipeline.rs index 9cb58b6e..117045ba 100644 --- a/src/encoder/pipeline.rs +++ b/src/encoder/pipeline.rs @@ -357,6 +357,7 @@ impl EncodePipeline { pub(crate) fn submit_current( &mut self, device: &ash::Device, + sync2: &ash::khr::synchronization2::Device, encode_queue: vk::Queue, ) -> Result { let wait = self.timeline.wait(); @@ -396,6 +397,7 @@ impl EncodePipeline { unsafe { submit_encode_only( device, + sync2, command_buffer, fence, encode_queue, diff --git a/src/encoder/resources.rs b/src/encoder/resources.rs index ae4c5867..7230ff12 100644 --- a/src/encoder/resources.rs +++ b/src/encoder/resources.rs @@ -806,6 +806,7 @@ pub(crate) unsafe fn record_post_encode_dpb_barrier( /// The command buffer must have been ended. pub(crate) unsafe fn submit_encode_only( device: &ash::Device, + sync2: &ash::khr::synchronization2::Device, command_buffer: vk::CommandBuffer, fence: vk::Fence, encode_queue: vk::Queue, @@ -846,7 +847,7 @@ pub(crate) unsafe fn submit_encode_only( } unsafe { - device + sync2 .queue_submit2(encode_queue, &[submit_info], fence) .map_err(|e| PixelForgeError::CommandBuffer(e.to_string()))?; } diff --git a/src/vulkan.rs b/src/vulkan.rs index 46d4e9c9..b905fb08 100644 --- a/src/vulkan.rs +++ b/src/vulkan.rs @@ -268,6 +268,10 @@ impl VideoContextBuilder { /// features must be enabled as well, and are not listed here because they are /// feature-struct fields rather than extensions: `synchronization2` and /// `timelineSemaphore`. +/// +/// The instance may ask for any Vulkan version from 1.1 up. Pixelforge reaches +/// everything newer through the extensions listed here, so an application that +/// created its instance for 1.1 or 1.2 does not have to raise it. #[derive(Debug, Clone)] pub struct DeviceRequirements { /// Queue families pixelforge needs a queue created for. Merge these with @@ -449,10 +453,15 @@ fn supports_internally_synchronized_queues( } fn decode_extension_names(decode_codecs: &[Codec]) -> Vec<&'static std::ffi::CStr> { + // Synchronization2 and timeline semaphores are core in 1.3 and 1.2, but + // are listed anyway: enabling a promoted extension is harmless, and on a + // device created from an instance asking for less it is what makes them + // available at all. let mut names = vec![ ash::khr::video_queue::NAME, ash::khr::video_decode_queue::NAME, ash::khr::synchronization2::NAME, + ash::khr::timeline_semaphore::NAME, ]; if decode_codecs.contains(&Codec::H264) { names.push(ash::khr::video_decode_h264::NAME); @@ -479,6 +488,14 @@ struct VideoContextInner { instance: ash::Instance, physical_device: vk::PhysicalDevice, device: ash::Device, + /// `vkQueueSubmit2` and `vkCmdPipelineBarrier2` through their KHR names. + /// + /// The core entry points exist only when the device was created from an + /// instance asking for Vulkan 1.3. An adopted device may come from an + /// application that asked for less, where the core pointers are null and + /// calling them crashes. The KHR names are valid on any device with + /// `VK_KHR_synchronization2` enabled, which pixelforge requires either way. + sync2: ash::khr::synchronization2::Device, video_encode_queue_family: Option, video_encode_timestamp_valid_bits: u32, video_encode_queue: Option, @@ -539,6 +556,10 @@ impl VideoContext { &self.inner.device } + pub(crate) fn sync2(&self) -> &ash::khr::synchronization2::Device { + &self.inner.sync2 + } + pub(crate) fn video_encode_queue_family(&self) -> Option { self.inner.video_encode_queue_family } @@ -1184,6 +1205,7 @@ impl VideoContext { Ok(Self { inner: std::sync::Arc::new(VideoContextInner { entry, + sync2: ash::khr::synchronization2::Device::load(&instance, &device), instance, physical_device, device, @@ -1286,6 +1308,7 @@ impl VideoContext { Ok(VideoContext { inner: std::sync::Arc::new(VideoContextInner { entry, + sync2: ash::khr::synchronization2::Device::load(&instance, &device), instance, physical_device, device, diff --git a/tests/adopted.rs b/tests/adopted.rs new file mode 100644 index 00000000..748d01a2 --- /dev/null +++ b/tests/adopted.rs @@ -0,0 +1,185 @@ +//! Pixelforge on a device the caller created, from an instance that asked for +//! Vulkan 1.1. +//! +//! An application that shares its device with pixelforge chose that device's +//! API version for its own reasons, and plenty choose less than 1.3. On such a +//! device the core 1.2 and 1.3 entry points are null, so anything pixelforge +//! reaches that way crashes. These tests build exactly that device, hand +//! pixelforge a spare queue where the hardware has one, and check the output +//! against a context pixelforge created itself on the same GPU. + +use ash::vk; +use ash::vk::TaggedStructure; +#[allow(dead_code)] +mod common; +use common::{Readback, decode_stream}; + +use pixelforge::decoder::{DecodeConfig, Decoder}; +use pixelforge::encoder::Codec; +use pixelforge::vulkan::{DeviceQueue, DeviceRequirements, VideoContext, VideoContextBuilder}; + +/// An instance and device the test owns, destroyed on drop after everything +/// built on them. +struct AppDevice { + entry: ash::Entry, + instance: ash::Instance, + physical_device: vk::PhysicalDevice, + device: ash::Device, +} + +impl Drop for AppDevice { + fn drop(&mut self) { + unsafe { + self.device.destroy_device(None); + self.instance.destroy_instance(None); + } + } +} + +fn validation_requested() -> bool { + std::env::var("PIXELFORGE_VALIDATION").is_ok() +} + +/// Create a Vulkan 1.1 instance, with validation when asked for. +fn instance_1_1() -> Result<(ash::Entry, ash::Instance), Box> { + let entry = unsafe { ash::Entry::load()? }; + let app_info = vk::ApplicationInfo::default() + .application_name(c"pixelforge-adopted-test") + .api_version(vk::API_VERSION_1_1); + let validation = c"VK_LAYER_KHRONOS_validation"; + let has_validation = unsafe { entry.enumerate_instance_layer_properties()? } + .iter() + .any(|l| unsafe { std::ffi::CStr::from_ptr(l.layer_name.as_ptr()) } == validation); + let layers = if validation_requested() && has_validation { + vec![validation.as_ptr()] + } else { + Vec::new() + }; + let info = vk::InstanceCreateInfo::default() + .application_info(&app_info) + .enabled_layer_names(&layers); + let instance = unsafe { entry.create_instance(&info, None)? }; + Ok((entry, instance)) +} + +/// Create the application's device from `reqs`, keeping queue 0 of every +/// family for itself wherever the family has a second queue. Returns the +/// queue pixelforge should use for each of `roles`. +fn create_app_device( + entry: ash::Entry, + instance: ash::Instance, + physical_device: vk::PhysicalDevice, + reqs: &DeviceRequirements, +) -> Result> { + let family_props = + unsafe { instance.get_physical_device_queue_family_properties(physical_device) }; + let priorities = [1.0f32; 2]; + let queue_infos: Vec = reqs + .queue_families + .iter() + .map(|&f| { + let count = family_props[f as usize].queue_count.min(2) as usize; + vk::DeviceQueueCreateInfo::default() + .queue_family_index(f) + .queue_priorities(&priorities[..count]) + }) + .collect(); + let ext_ptrs: Vec<_> = reqs.extensions.iter().map(|e| e.as_ptr()).collect(); + + // The KHR feature structs, since this device is not 1.3: the promoted + // struct types are the same types under their extension names. + let mut sync2 = vk::PhysicalDeviceSynchronization2Features::default().synchronization2(true); + let mut timeline = + vk::PhysicalDeviceTimelineSemaphoreFeatures::default().timeline_semaphore(true); + let mut unified = vk::PhysicalDeviceUnifiedImageLayoutsFeaturesKHR::default() + .unified_image_layouts(true) + .unified_image_layouts_video(true); + let mut info = vk::DeviceCreateInfo::default() + .queue_create_infos(&queue_infos) + .enabled_extension_names(&ext_ptrs) + .push(&mut sync2) + .push(&mut timeline); + if reqs.unified_image_layouts { + info = info.push(&mut unified); + } + let device = unsafe { instance.create_device(physical_device, &info, None)? }; + Ok(AppDevice { + entry, + instance, + physical_device, + device, + }) +} + +/// The last queue this test created in `family`: a spare where there are two. +fn spare(app: &AppDevice, family: u32) -> DeviceQueue { + let props = unsafe { + app.instance + .get_physical_device_queue_family_properties(app.physical_device) + }; + DeviceQueue::new(family, props[family as usize].queue_count.min(2) - 1) +} + +/// Decode `stream` on `context` and read every frame back. +fn decode_all( + context: &VideoContext, + stream: &[u8], +) -> Result>, Box> { + let mut readback = Readback::new(context)?; + let mut decoder = Decoder::new(context.clone(), DecodeConfig::h264().with_byte_stream())?; + let mut frames = Vec::new(); + decode_stream(&mut decoder, stream, |frame| { + let data = readback.read(&frame)?; + frames.push([data.y, data.uv].concat()); + Ok(()) + })?; + drop(decoder); + drop(readback); + Ok(frames) +} + +#[test] +#[ignore = "requires a Vulkan Video device"] +fn decode_on_a_vulkan_1_1_device() -> Result<(), Box> { + let stream = std::fs::read("tests/data/bframes.264")?; + + let own = VideoContextBuilder::new() + .enable_validation(validation_requested()) + .require_decode(Codec::H264) + .build()?; + let expected = decode_all(&own, &stream)?; + let own_name = own.device_properties().device_name; + drop(own); + + let (entry, instance) = instance_1_1()?; + let builder = VideoContextBuilder::new().require_decode(Codec::H264); + // The same GPU the own context picked, so the frames are comparable. + let physical_device = unsafe { instance.enumerate_physical_devices()? } + .into_iter() + .find(|&pd| unsafe { instance.get_physical_device_properties(pd) }.device_name == own_name) + .expect("the GPU the own context used"); + let reqs = builder.decode_device_requirements(&entry, &instance, physical_device)?; + let app = create_app_device(entry, instance, physical_device, &reqs)?; + + let mut builder = builder + .with_decode_queue(spare(&app, reqs.queues.decode.unwrap())) + .with_transfer_queue(spare(&app, reqs.queues.transfer)) + .with_compute_queue(spare(&app, reqs.queues.compute)); + if reqs.unified_image_layouts { + builder = builder.declare_unified_image_layouts(); + } + let context = builder.build_from_existing_decode( + app.entry.clone(), + app.instance.clone(), + app.physical_device, + app.device.clone(), + )?; + let actual = decode_all(&context, &stream)?; + drop(context); + + assert_eq!(actual.len(), expected.len(), "frame count"); + for (i, (a, e)) in actual.iter().zip(&expected).enumerate() { + assert!(a == e, "frame {i} differs from the own-context decode"); + } + Ok(()) +} diff --git a/tests/common/mod.rs b/tests/common/mod.rs index 39b421b4..56dcabe6 100644 --- a/tests/common/mod.rs +++ b/tests/common/mod.rs @@ -109,6 +109,9 @@ pub struct Readback { /// the caller has to get right. _context: VideoContext, device: ash::Device, + /// Barriers through the KHR entry point, which a device created below + /// Vulkan 1.3 still has; the adopted-device tests create one. + sync2: ash::khr::synchronization2::Device, queue: vk::Queue, pool: vk::CommandPool, command_buffer: vk::CommandBuffer, @@ -146,6 +149,7 @@ impl Readback { }; Ok(Self { _context: context.clone(), + sync2: ash::khr::synchronization2::Device::load(context.instance(), &device), device, queue: context.transfer_queue(), pool, @@ -178,7 +182,7 @@ impl Readback { layer_count: 1, }; let regions = [ - vk::BufferImageCopy2::default() + vk::BufferImageCopy::default() .buffer_offset(0) .buffer_row_length(frame.coded_width) .buffer_image_height(frame.coded_height) @@ -188,7 +192,7 @@ impl Readback { height: frame.coded_height, depth: 1, }), - vk::BufferImageCopy2::default() + vk::BufferImageCopy::default() .buffer_offset(y_size as u64) .buffer_row_length(frame.coded_width / 2) .buffer_image_height(frame.coded_height / 2) @@ -234,23 +238,22 @@ impl Readback { .dst_queue_family_index(vk::QUEUE_FAMILY_IGNORED) .image(frame.image) .subresource_range(range)]; - self.device.cmd_pipeline_barrier2( + self.sync2.cmd_pipeline_barrier2( self.command_buffer, &vk::DependencyInfo::default().image_memory_barriers(&to_src), ); } - self.device.cmd_copy_image_to_buffer2( + self.device.cmd_copy_image_to_buffer( self.command_buffer, - &vk::CopyImageToBufferInfo2::default() - .src_image(frame.image) - .src_image_layout(if needs_transition { - vk::ImageLayout::TRANSFER_SRC_OPTIMAL - } else { - frame.layout - }) - .dst_buffer(buffer) - .regions(®ions), + frame.image, + if needs_transition { + vk::ImageLayout::TRANSFER_SRC_OPTIMAL + } else { + frame.layout + }, + buffer, + ®ions, ); if needs_transition { @@ -265,7 +268,7 @@ impl Readback { .dst_queue_family_index(vk::QUEUE_FAMILY_IGNORED) .image(frame.image) .subresource_range(range)]; - self.device.cmd_pipeline_barrier2( + self.sync2.cmd_pipeline_barrier2( self.command_buffer, &vk::DependencyInfo::default().image_memory_barriers(&restore), ); From 09ba9b6a4fd6a462ecef105d4659ac7d268dfe53 Mon Sep 17 00:00:00 2001 From: DatCaptainHorse Date: Wed, 23 Sep 2026 08:17:51 +0300 Subject: [PATCH 04/16] 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. The adopted-device encode test needs the same image as converter input, 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 56dcabe6..300b583f 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 02a3e72582d11ff3205e15ec1afddbfb42595c3a Mon Sep 17 00:00:00 2001 From: DatCaptainHorse Date: Wed, 23 Sep 2026 08:19:49 +0300 Subject: [PATCH 05/16] feat(vulkan): encode on a caller-created device Decoding could already run on a device the caller created; encoding could not, so an application that renders on its own device and wants to encode the result had to give pixelforge a device of its own and move every frame across. That means exporting each image as external memory, importing it on the other device, and waiting on the CPU between the two, since two devices share no timeline a semaphore could use. encode_device_requirements() and build_from_existing_encode() are the encode counterparts of the decode pair. The requirements cover the encoder and the colour converter: the video encode extensions for the codecs asked for (or every codec the device encodes, if none were), push descriptors for the converter, and the 2-plane 4:4:4 formats where the device has them. Features are now reported too, in DeviceRequirements::features, as booleans rather than a pNext chain. A caller merging them into its own device may already chain VkPhysicalDeviceVulkan13Features, and Vulkan forbids chaining that alongside the per-feature structs it contains, so pixelforge names the bits and leaves the structs to the caller. The queue-family scan the decode path used is shared with encode rather than copied, and the adopted-device test gains an encode case: a Vulkan 1.1 device, spare queues, RGB frames through the converter into the encoder, and the stream checked against one encoded by a context pixelforge created itself. encode_adopted is the example counterpart of decode_adopted. Co-Authored-By: Claude Opus 5.5 --- examples/encode_adopted.rs | 148 ++++++++++++++++ src/vulkan.rs | 134 ++++++++++++--- src/vulkan/encode_device.rs | 324 ++++++++++++++++++++++++++++++++++++ src/vulkan/queues.rs | 1 + tests/adopted.rs | 184 ++++++++++++++++++-- 5 files changed, 752 insertions(+), 39 deletions(-) create mode 100644 examples/encode_adopted.rs create mode 100644 src/vulkan/encode_device.rs diff --git a/examples/encode_adopted.rs b/examples/encode_adopted.rs new file mode 100644 index 00000000..be7bfef0 --- /dev/null +++ b/examples/encode_adopted.rs @@ -0,0 +1,148 @@ +//! Encode on a caller-created device. +//! +//! Mirrors what an application with its own Vulkan device does, such as a +//! renderer that wants to stream what it draws: create the instance and device +//! itself, then hand them to pixelforge via `build_from_existing_encode`, so +//! the encoder works on the application's device instead of a second one. +//! +//! Usage: +//! encode_adopted [input.yuv] [output.h264] +//! +//! The input is raw 320x240 YUV420, `testdata/test_frames.yuv` by default. + +use std::fs::File; +use std::io::Write; + +use ash::vk; +use ash::vk::TaggedStructure; +use pixelforge::vulkan::{DeviceQueue, VideoContextBuilder}; +use pixelforge::{ + Codec, EncodeBitDepth, EncodeConfig, Encoder, InputImage, PixelFormat, RateControlMode, +}; + +const WIDTH: u32 = 320; +const HEIGHT: u32 = 240; + +fn main() -> Result<(), Box> { + let mut args = std::env::args().skip(1); + let input_path = args + .next() + .unwrap_or_else(|| "testdata/test_frames.yuv".to_string()); + let output_path = args.next().unwrap_or_else(|| "output.h264".to_string()); + + // --- The application creates its own instance and device --- + let entry = unsafe { ash::Entry::load()? }; + let app_info = vk::ApplicationInfo::default() + .application_name(c"encode-adopted") + .api_version(vk::API_VERSION_1_3); + let instance_info = vk::InstanceCreateInfo::default().application_info(&app_info); + let instance = unsafe { entry.create_instance(&instance_info, None)? }; + + // Ask pixelforge what an encode device needs, and pick a physical device + // that satisfies it. + let builder = VideoContextBuilder::new().require_encode(Codec::H264); + let (physical_device, reqs) = unsafe { instance.enumerate_physical_devices()? } + .into_iter() + .find_map(|pd| { + builder + .encode_device_requirements(&entry, &instance, pd) + .ok() + .map(|r| (pd, r)) + }) + .expect("no physical device can encode H.264"); + + // Keep queue 0 of each family for the application, and give pixelforge a + // second queue wherever the family has one. A `VkQueue` may not be + // submitted to from two threads at once, so this is what lets the + // application keep rendering while pixelforge encodes on another thread. + let family_props = + unsafe { instance.get_physical_device_queue_family_properties(physical_device) }; + let priorities = [1.0f32; 2]; + let queue_infos: Vec = reqs + .queue_families + .iter() + .map(|&f| { + let count = family_props[f as usize].queue_count.min(2) as usize; + vk::DeviceQueueCreateInfo::default() + .queue_family_index(f) + .queue_priorities(&priorities[..count]) + }) + .collect(); + let spare = |family: u32| { + DeviceQueue::new(family, family_props[family as usize].queue_count.min(2) - 1) + }; + let ext_ptrs: Vec<_> = reqs.extensions.iter().map(|e| e.as_ptr()).collect(); + + // `reqs.features` says which feature bits to turn on. An application that + // already chains `VkPhysicalDeviceVulkan13Features` would set them there; + // this one has nothing else, so it uses the individual structs. + let features = reqs.features; + let mut sync2 = vk::PhysicalDeviceSynchronization2Features::default() + .synchronization2(features.synchronization2); + let mut timeline = vk::PhysicalDeviceTimelineSemaphoreFeatures::default() + .timeline_semaphore(features.timeline_semaphore); + let mut ycbcr = vk::PhysicalDeviceSamplerYcbcrConversionFeatures::default() + .sampler_ycbcr_conversion(features.sampler_ycbcr_conversion); + let mut ycbcr_444 = + vk::PhysicalDeviceYcbcr2Plane444FormatsFeaturesEXT::default().ycbcr2plane444_formats(true); + let mut device_info = vk::DeviceCreateInfo::default() + .queue_create_infos(&queue_infos) + .enabled_extension_names(&ext_ptrs) + .push(&mut sync2) + .push(&mut timeline) + .push(&mut ycbcr); + if features.ycbcr_2plane_444_formats { + device_info = device_info.push(&mut ycbcr_444); + } + let device = unsafe { instance.create_device(physical_device, &device_info, None)? }; + + // --- Hand the application's device to pixelforge --- + let context = builder + .with_encode_queue(spare(reqs.queues.encode.unwrap())) + .with_transfer_queue(spare(reqs.queues.transfer)) + .with_compute_queue(spare(reqs.queues.compute)) + .build_from_existing_encode( + entry.clone(), + instance.clone(), + physical_device, + device.clone(), + )?; + + let yuv = std::fs::read(&input_path)?; + let frame_size = (WIDTH * HEIGHT * 3 / 2) as usize; + let mut input = InputImage::new( + context.clone(), + Codec::H264, + WIDTH, + HEIGHT, + EncodeBitDepth::Eight, + PixelFormat::Yuv420, + )?; + let mut encoder = Encoder::new( + context, + EncodeConfig::h264(WIDTH, HEIGHT) + .with_rate_control(RateControlMode::Cqp) + .with_quality_level(26) + .with_frame_rate(30, 1) + .with_b_frames(0), + )?; + let mut output = File::create(&output_path)?; + let mut count = 0usize; + for frame in yuv.chunks_exact(frame_size) { + input.upload_yuv420(frame)?; + let packet = pollster::block_on(encoder.encode(input.image())?)?; + output.write_all(&packet.data)?; + count += 1; + } + println!("Encoded {count} frames on the adopted device into {output_path}"); + + // The context borrowed the device, so everything holding Vulkan objects + // has to go before the device does. + drop(encoder); + drop(input); + unsafe { + device.destroy_device(None); + instance.destroy_instance(None); + } + Ok(()) +} diff --git a/src/vulkan.rs b/src/vulkan.rs index b905fb08..5244a213 100644 --- a/src/vulkan.rs +++ b/src/vulkan.rs @@ -7,6 +7,7 @@ use std::ffi::{CStr, CString}; use std::os::raw::c_char; use tracing::{debug, info, warn}; +mod encode_device; mod queues; use queues::QueueOverrides; pub use queues::{DeviceQueue, QueueRoles}; @@ -117,6 +118,16 @@ impl VideoContextBuilder { self } + /// Submit video encode work to `queue` on an adopted device. + /// + /// See [`Self::with_transfer_queue`] for why this matters. Ignored by + /// [`Self::build`], which creates its own device and owns every queue on + /// it. + pub fn with_encode_queue(mut self, queue: DeviceQueue) -> Self { + self.queues.encode = Some(queue); + self + } + /// Submit video decode work to `queue` on an adopted device. /// /// See [`Self::with_transfer_queue`] for why this matters. Ignored by @@ -190,6 +201,11 @@ impl VideoContextBuilder { queue_families: queues.unique_families(), queues, extensions, + features: DeviceFeatures { + synchronization2: true, + timeline_semaphore: true, + ..DeviceFeatures::default() + }, unified_image_layouts, internally_synchronized_queues: supports_internally_synchronized_queues( instance, @@ -262,12 +278,14 @@ impl VideoContextBuilder { } } -/// Queue families and extensions a caller's device must provide to decode. +/// Queue families, extensions and features a caller's device must provide for +/// pixelforge to work on it. /// -/// Returned by [`VideoContextBuilder::decode_device_requirements`]. Two device -/// features must be enabled as well, and are not listed here because they are -/// feature-struct fields rather than extensions: `synchronization2` and -/// `timelineSemaphore`. +/// Returned by [`VideoContextBuilder::decode_device_requirements`] and +/// [`VideoContextBuilder::encode_device_requirements`]. Enable every extension +/// in [`Self::extensions`] and every feature set in [`Self::features`]: +/// pixelforge assumes all of them are on, since Vulkan cannot be asked +/// afterwards which were. /// /// The instance may ask for any Vulkan version from 1.1 up. Pixelforge reaches /// everything newer through the extensions listed here, so an application that @@ -285,6 +303,8 @@ pub struct DeviceRequirements { pub queues: QueueRoles, /// Device extensions pixelforge needs enabled. Merge with your own. pub extensions: Vec<&'static std::ffi::CStr>, + /// Device features pixelforge needs enabled. Merge with your own. + pub features: DeviceFeatures, /// Whether this device can support unified image layouts for video, which /// is what lets decoded frames be sampled with no copy and no layout /// transition. When true, `VK_KHR_unified_image_layouts` is included in @@ -311,6 +331,30 @@ pub struct DeviceRequirements { pub internally_synchronized_queues: bool, } +/// Device features, as booleans rather than a `pNext` chain. +/// +/// A caller merging these into its own device creation may already chain +/// `VkPhysicalDeviceVulkan12Features` or `VkPhysicalDeviceVulkan13Features`, +/// and Vulkan forbids chaining those alongside the per-feature structs they +/// contain. So pixelforge says which bits it needs and leaves it to the caller +/// to set them in whichever structs it uses. The field names match the Vulkan +/// feature names. +#[derive(Debug, Clone, Copy, Default, PartialEq, Eq)] +pub struct DeviceFeatures { + /// `synchronization2`, from `VkPhysicalDeviceSynchronization2Features`. + pub synchronization2: bool, + /// `timelineSemaphore`, from `VkPhysicalDeviceTimelineSemaphoreFeatures`. + pub timeline_semaphore: bool, + /// `samplerYcbcrConversion`, from + /// `VkPhysicalDeviceSamplerYcbcrConversionFeatures`. + pub sampler_ycbcr_conversion: bool, + /// `ycbcr2plane444Formats`, from + /// `VkPhysicalDeviceYcbcr2Plane444FormatsFeaturesEXT`. + pub ycbcr_2plane_444_formats: bool, + /// `videoEncodeAV1`, from `VkPhysicalDeviceVideoEncodeAV1FeaturesKHR`. + pub video_encode_av1: bool, +} + /// The queue families pixelforge selects for decoding on a given device. struct DecodeQueueFamilies { decode: u32, @@ -318,23 +362,34 @@ struct DecodeQueueFamilies { compute: u32, } -/// Select the decode / transfer / compute queue families on `physical_device`, -/// failing if it cannot decode the required codecs. +/// The families [`scan_queue_families`] found. +struct ScannedFamilies { + /// The last family with the requested video flag, if any. + video: Option, + /// `timestampValidBits` of [`Self::video`]'s family. + video_timestamp_valid_bits: u32, + transfer: u32, + compute: u32, +} + +/// Find a family with the `video` flag, a transfer family and a compute family +/// on `physical_device`, the same way [`VideoContext::new`] does. /// -/// Mirrors the selection [`VideoContext::new`] does inline, but scoped to the -/// decode path: a video-decode family, a transfer family (preferring a -/// dedicated engine over one that also does video — see the scoring), and any -/// compute family. -fn find_decode_queue_families( - entry: &ash::Entry, +/// The transfer family is picked by preference rather than last-wins: a family +/// that also does video is the worst choice, since it contends with the +/// encode or decode work itself, then the graphics family, which the caller +/// most likely renders on, then compute, and best of all a dedicated transfer +/// engine. +fn scan_queue_families( instance: &ash::Instance, physical_device: vk::PhysicalDevice, - decode_codecs: &[Codec], -) -> Result { + video_flag: vk::QueueFlags, +) -> Result { let queue_families = unsafe { instance.get_physical_device_queue_family_properties(physical_device) }; - let mut decode = None; + let mut video = None; + let mut video_timestamp_valid_bits = 0; let mut transfer = u32::MAX; let mut transfer_score = -1i32; let mut compute = u32::MAX; @@ -343,8 +398,9 @@ fn find_decode_queue_families( let idx = idx as u32; let flags = props.queue_flags; - if flags.contains(vk::QueueFlags::VIDEO_DECODE_KHR) { - decode = Some(idx); + if flags.contains(video_flag) { + video = Some(idx); + video_timestamp_valid_bits = props.timestamp_valid_bits; } if flags.contains(vk::QueueFlags::TRANSFER) { let is_video = flags @@ -368,11 +424,6 @@ fn find_decode_queue_families( } } - let decode = decode.ok_or_else(|| { - PixelForgeError::NoSuitableDevice( - "Physical device has no video decode queue family".to_string(), - ) - })?; if transfer == u32::MAX { return Err(PixelForgeError::NoSuitableDevice( "Physical device has no transfer queue family".to_string(), @@ -383,6 +434,39 @@ fn find_decode_queue_families( "Physical device has no compute queue family".to_string(), )); } + Ok(ScannedFamilies { + video, + video_timestamp_valid_bits, + transfer, + compute, + }) +} + +/// Select the decode / transfer / compute queue families on `physical_device`, +/// failing if it cannot decode the required codecs. +/// +/// Mirrors the selection [`VideoContext::new`] does inline, but scoped to the +/// decode path: a video-decode family, a transfer family (preferring a +/// dedicated engine over one that also does video — see the scoring), and any +/// compute family. +fn find_decode_queue_families( + entry: &ash::Entry, + instance: &ash::Instance, + physical_device: vk::PhysicalDevice, + decode_codecs: &[Codec], +) -> Result { + let ScannedFamilies { + video: decode, + transfer, + compute, + .. + } = scan_queue_families(instance, physical_device, vk::QueueFlags::VIDEO_DECODE_KHR)?; + + let decode = decode.ok_or_else(|| { + PixelForgeError::NoSuitableDevice( + "Physical device has no video decode queue family".to_string(), + ) + })?; // Confirm the device actually decodes the codecs asked for. let available = query_decode_codecs(entry, instance, physical_device); @@ -512,8 +596,8 @@ struct VideoContextInner { 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 - /// [`VideoContext::from_existing_decode`] borrows them and destroys neither. + /// instance. A context adopted from a caller's device borrows them and + /// destroys neither. owns_device: bool, /// Validation message sink, present only when validation is enabled and the /// instance is ours. Destroyed before the instance it belongs to. diff --git a/src/vulkan/encode_device.rs b/src/vulkan/encode_device.rs new file mode 100644 index 00000000..00dba30f --- /dev/null +++ b/src/vulkan/encode_device.rs @@ -0,0 +1,324 @@ +//! Encoding on a device the caller created. +//! +//! The encode counterpart of [`VideoContextBuilder::build_from_existing_decode`]. +//! An application that already renders on its own device, and wants to encode +//! what it renders, can hand that device to pixelforge. The encoder and the +//! colour converter then work on the application's images directly: no +//! external memory, no export and import, and a semaphore instead of a CPU +//! wait where one side hands a frame to the other. + +use super::queues::{self, QueueOverrides}; +use super::{ + DeviceFeatures, DeviceRequirements, QueueRoles, VideoContext, VideoContextBuilder, + VideoContextInner, scan_queue_families, supports_internally_synchronized_queues, +}; +use crate::encoder::Codec; +use crate::error::{PixelForgeError, Result}; +use ash::vk; +use ash::vk::TaggedStructure; +use tracing::info; + +/// The queue families pixelforge selects for encoding on a given device. +struct EncodeQueueFamilies { + encode: u32, + encode_timestamp_valid_bits: u32, + transfer: u32, + compute: u32, +} + +/// What pixelforge will use on `physical_device` to encode `required`. +/// +/// When `required` is empty every codec the device encodes is included, so a +/// caller that has not decided yet gets a device that can do any of them. +struct EncodePlan { + families: EncodeQueueFamilies, + codecs: Vec, + push_descriptor: bool, + ycbcr_2plane_444: bool, + sampler_ycbcr_conversion: bool, +} + +impl EncodePlan { + fn new( + entry: &ash::Entry, + instance: &ash::Instance, + physical_device: vk::PhysicalDevice, + required: &[Codec], + ) -> Result { + let scanned = + scan_queue_families(instance, physical_device, vk::QueueFlags::VIDEO_ENCODE_KHR)?; + let encode = scanned.video.ok_or_else(|| { + PixelForgeError::NoSuitableDevice( + "Physical device has no video encode queue family".to_string(), + ) + })?; + + let extensions = unsafe { instance.enumerate_device_extension_properties(physical_device) } + .map_err(|e| PixelForgeError::NoSuitableDevice(e.to_string()))?; + let has = |name: &std::ffi::CStr| { + extensions + .iter() + .any(|ext| unsafe { std::ffi::CStr::from_ptr(ext.extension_name.as_ptr()) } == name) + }; + + let mut available = Vec::new(); + if has(ash::khr::video_encode_h264::NAME) + && VideoContext::check_h264_encode_support(entry, instance, physical_device, encode) + { + available.push(Codec::H264); + } + if has(ash::khr::video_encode_h265::NAME) + && VideoContext::check_h265_encode_support(entry, instance, physical_device, encode) + { + available.push(Codec::H265); + } + if has(ash::khr::video_encode_av1::NAME) + && VideoContext::check_av1_encode_support(entry, instance, physical_device, encode) + { + available.push(Codec::AV1); + } + let codecs = if required.is_empty() { + available + } else { + for codec in required { + if !available.contains(codec) { + return Err(PixelForgeError::CodecNotSupported(format!( + "Physical device does not support encoding {:?}", + codec + ))); + } + } + required.to_vec() + }; + if codecs.is_empty() { + return Err(PixelForgeError::NoSuitableDevice( + "Physical device encodes none of H.264, H.265 or AV1".to_string(), + )); + } + + let mut ycbcr = vk::PhysicalDeviceSamplerYcbcrConversionFeatures::default(); + let mut query = vk::PhysicalDeviceFeatures2::default().push(&mut ycbcr); + unsafe { instance.get_physical_device_features2(physical_device, &mut query) }; + + Ok(Self { + families: EncodeQueueFamilies { + encode, + encode_timestamp_valid_bits: scanned.video_timestamp_valid_bits, + transfer: scanned.transfer, + compute: scanned.compute, + }, + codecs, + push_descriptor: has(ash::khr::push_descriptor::NAME), + ycbcr_2plane_444: has(ash::ext::ycbcr_2plane_444_formats::NAME), + sampler_ycbcr_conversion: ycbcr.sampler_ycbcr_conversion != 0, + }) + } + + fn extensions(&self) -> Vec<&'static std::ffi::CStr> { + // Synchronization2 and timeline semaphores are core in 1.3 and 1.2, and + // listed anyway so a device created from an instance asking for less + // still has them. + let mut names = vec![ + ash::khr::video_queue::NAME, + ash::khr::video_encode_queue::NAME, + ash::khr::synchronization2::NAME, + ash::khr::timeline_semaphore::NAME, + ]; + for codec in &self.codecs { + names.push(match codec { + Codec::H264 => ash::khr::video_encode_h264::NAME, + Codec::H265 => ash::khr::video_encode_h265::NAME, + Codec::AV1 => ash::khr::video_encode_av1::NAME, + }); + } + if self.push_descriptor { + names.push(ash::khr::push_descriptor::NAME); + } + if self.ycbcr_2plane_444 { + names.push(ash::ext::ycbcr_2plane_444_formats::NAME); + } + names + } + + fn features(&self) -> DeviceFeatures { + DeviceFeatures { + synchronization2: true, + timeline_semaphore: true, + sampler_ycbcr_conversion: self.sampler_ycbcr_conversion, + ycbcr_2plane_444_formats: self.ycbcr_2plane_444, + video_encode_av1: self.codecs.contains(&Codec::AV1), + } + } + + fn roles(&self) -> QueueRoles { + QueueRoles { + encode: Some(self.families.encode), + decode: None, + transfer: self.families.transfer, + compute: self.families.compute, + } + } +} + +impl VideoContextBuilder { + /// What a caller-created device must provide for pixelforge to encode on + /// it, and to run the colour converter. + /// + /// Use this when you already have your own Vulkan device (a renderer's, or + /// one a game created) and want to encode images that live on it, without + /// exporting them to a second device. The codecs asked for with + /// [`Self::require_encode`] must all be supported; with none asked for, + /// every codec the device encodes is included. + /// + /// The colour converter needs `VK_KHR_push_descriptor`, which is included + /// whenever the device has it. On a device without it, encoding from YUV + /// input still works and only the converter is unavailable. + pub fn encode_device_requirements( + &self, + entry: &ash::Entry, + instance: &ash::Instance, + physical_device: vk::PhysicalDevice, + ) -> Result { + let plan = EncodePlan::new( + entry, + instance, + physical_device, + &self.required_encode_codecs, + )?; + let queues = plan.roles(); + Ok(DeviceRequirements { + queue_families: queues.unique_families(), + queues, + extensions: plan.extensions(), + features: plan.features(), + unified_image_layouts: false, + internally_synchronized_queues: supports_internally_synchronized_queues( + instance, + physical_device, + ), + }) + } + + /// Adopt a caller-created device for encoding, rather than creating one. + /// + /// The device must have been created with the queue families, extensions + /// and features reported by [`Self::encode_device_requirements`], called + /// with the same codecs for the same `physical_device`. The instance may + /// ask for any Vulkan version from 1.1 up. + /// + /// The resulting context **borrows** `instance` and `device`: dropping it + /// frees neither, so the caller must keep both alive for at least as long + /// as the context and every encoder and converter made from it. + /// + /// Pixelforge submits to one queue per role, queue 0 of each family unless + /// told otherwise. A caller that keeps submitting to its own queues while + /// pixelforge works on another thread should give it spare ones with + /// [`Self::with_encode_queue`], [`Self::with_transfer_queue`] and + /// [`Self::with_compute_queue`]. + pub fn build_from_existing_encode( + self, + entry: ash::Entry, + instance: ash::Instance, + physical_device: vk::PhysicalDevice, + device: ash::Device, + ) -> Result { + VideoContext::from_existing_encode( + &self.required_encode_codecs, + self.queues, + entry, + instance, + physical_device, + device, + ) + } +} + +impl VideoContext { + fn from_existing_encode( + required_encode_codecs: &[Codec], + queue_overrides: QueueOverrides, + entry: ash::Entry, + instance: ash::Instance, + physical_device: vk::PhysicalDevice, + device: ash::Device, + ) -> Result { + let plan = EncodePlan::new(&entry, &instance, physical_device, required_encode_codecs)?; + let families = &plan.families; + + let encode = queues::resolve( + &instance, + physical_device, + "encode", + queue_overrides.encode, + families.encode, + vk::QueueFlags::VIDEO_ENCODE_KHR, + )?; + let transfer = queues::resolve( + &instance, + physical_device, + "transfer", + queue_overrides.transfer, + families.transfer, + vk::QueueFlags::TRANSFER, + )?; + let compute = queues::resolve( + &instance, + physical_device, + "compute", + queue_overrides.compute, + families.compute, + vk::QueueFlags::COMPUTE, + )?; + + // The caller may have picked another encode family than the scan did, + // so the timestamp support is that family's, not the scanned one's. + let encode_timestamp_valid_bits = if encode.family == families.encode { + families.encode_timestamp_valid_bits + } else { + let props = + unsafe { instance.get_physical_device_queue_family_properties(physical_device) }; + props[encode.family as usize].timestamp_valid_bits + }; + + let device_properties = unsafe { instance.get_physical_device_properties(physical_device) }; + let memory_properties = + unsafe { instance.get_physical_device_memory_properties(physical_device) }; + + let video_encode_queue = unsafe { device.get_device_queue(encode.family, encode.index) }; + let transfer_queue = unsafe { device.get_device_queue(transfer.family, transfer.index) }; + let compute_queue = unsafe { device.get_device_queue(compute.family, compute.index) }; + + info!( + "Adopted caller device for encode: {:?}, encode queue {:?}, transfer queue {:?}, compute queue {:?}", + plan.codecs, encode, transfer, compute + ); + + Ok(VideoContext { + inner: std::sync::Arc::new(VideoContextInner { + sync2: ash::khr::synchronization2::Device::load(&instance, &device), + entry, + instance, + physical_device, + device, + video_encode_queue_family: Some(encode.family), + video_encode_timestamp_valid_bits: encode_timestamp_valid_bits, + video_encode_queue: Some(video_encode_queue), + video_decode_queue_family: None, + video_decode_queue: None, + transfer_queue_family: transfer.family, + transfer_queue, + compute_queue_family: compute.family, + compute_queue, + memory_properties, + device_properties, + supported_encode_codecs: plan.codecs.clone(), + supported_decode_codecs: Vec::new(), + has_push_descriptor: plan.push_descriptor, + has_unified_image_layouts: false, + owns_device: false, + // The caller owns the instance; reporting is theirs to set up. + debug_messenger: None, + }), + }) + } +} diff --git a/src/vulkan/queues.rs b/src/vulkan/queues.rs index 0e8f2480..1d6200ca 100644 --- a/src/vulkan/queues.rs +++ b/src/vulkan/queues.rs @@ -72,6 +72,7 @@ impl QueueRoles { /// queue 0 of the family pixelforge selects. #[derive(Debug, Clone, Copy, Default)] pub(crate) struct QueueOverrides { + pub encode: Option, pub decode: Option, pub transfer: Option, pub compute: Option, diff --git a/tests/adopted.rs b/tests/adopted.rs index 748d01a2..a2b8b3f6 100644 --- a/tests/adopted.rs +++ b/tests/adopted.rs @@ -12,11 +12,16 @@ use ash::vk; use ash::vk::TaggedStructure; #[allow(dead_code)] mod common; +use common::source::create_src_image; use common::{Readback, decode_stream}; use pixelforge::decoder::{DecodeConfig, Decoder}; use pixelforge::encoder::Codec; use pixelforge::vulkan::{DeviceQueue, DeviceRequirements, VideoContext, VideoContextBuilder}; +use pixelforge::{ + ColorConverter, ColorConverterConfig, ColorRange, ColorSpec, EncodeConfig, Encoder, + InputFormat, OutputFormat, RateControlMode, +}; /// An instance and device the test owns, destroyed on drop after everything /// built on them. @@ -63,8 +68,7 @@ fn instance_1_1() -> Result<(ash::Entry, ash::Instance), Box = reqs.extensions.iter().map(|e| e.as_ptr()).collect(); - // The KHR feature structs, since this device is not 1.3: the promoted + // The per-feature structs, since this device is not 1.3: the promoted // struct types are the same types under their extension names. - let mut sync2 = vk::PhysicalDeviceSynchronization2Features::default().synchronization2(true); - let mut timeline = - vk::PhysicalDeviceTimelineSemaphoreFeatures::default().timeline_semaphore(true); + let features = reqs.features; + let mut sync2 = vk::PhysicalDeviceSynchronization2Features::default() + .synchronization2(features.synchronization2); + let mut timeline = vk::PhysicalDeviceTimelineSemaphoreFeatures::default() + .timeline_semaphore(features.timeline_semaphore); + let mut ycbcr = vk::PhysicalDeviceSamplerYcbcrConversionFeatures::default() + .sampler_ycbcr_conversion(features.sampler_ycbcr_conversion); + let mut ycbcr_444 = + vk::PhysicalDeviceYcbcr2Plane444FormatsFeaturesEXT::default().ycbcr2plane444_formats(true); + let mut av1 = vk::PhysicalDeviceVideoEncodeAV1FeaturesKHR::default().video_encode_av1(true); let mut unified = vk::PhysicalDeviceUnifiedImageLayoutsFeaturesKHR::default() .unified_image_layouts(true) .unified_image_layouts_video(true); @@ -98,7 +109,14 @@ fn create_app_device( .queue_create_infos(&queue_infos) .enabled_extension_names(&ext_ptrs) .push(&mut sync2) - .push(&mut timeline); + .push(&mut timeline) + .push(&mut ycbcr); + if features.ycbcr_2plane_444_formats { + info = info.push(&mut ycbcr_444); + } + if features.video_encode_av1 { + info = info.push(&mut av1); + } if reqs.unified_image_layouts { info = info.push(&mut unified); } @@ -138,6 +156,17 @@ fn decode_all( Ok(frames) } +/// The GPU a context pixelforge created for itself would pick, found on +/// `instance` by name so an adopted context can be compared against it. +fn same_gpu(instance: &ash::Instance, own: &VideoContext) -> vk::PhysicalDevice { + let name = own.device_properties().device_name; + unsafe { instance.enumerate_physical_devices() } + .expect("physical devices") + .into_iter() + .find(|&pd| unsafe { instance.get_physical_device_properties(pd) }.device_name == name) + .expect("the GPU the own context used") +} + #[test] #[ignore = "requires a Vulkan Video device"] fn decode_on_a_vulkan_1_1_device() -> Result<(), Box> { @@ -148,16 +177,11 @@ fn decode_on_a_vulkan_1_1_device() -> Result<(), Box> { .require_decode(Codec::H264) .build()?; let expected = decode_all(&own, &stream)?; - let own_name = own.device_properties().device_name; - drop(own); let (entry, instance) = instance_1_1()?; let builder = VideoContextBuilder::new().require_decode(Codec::H264); - // The same GPU the own context picked, so the frames are comparable. - let physical_device = unsafe { instance.enumerate_physical_devices()? } - .into_iter() - .find(|&pd| unsafe { instance.get_physical_device_properties(pd) }.device_name == own_name) - .expect("the GPU the own context used"); + let physical_device = same_gpu(&instance, &own); + drop(own); let reqs = builder.decode_device_requirements(&entry, &instance, physical_device)?; let app = create_app_device(entry, instance, physical_device, &reqs)?; @@ -183,3 +207,135 @@ fn decode_on_a_vulkan_1_1_device() -> Result<(), Box> { } Ok(()) } + +const ENCODE_WIDTH: u32 = 320; +const ENCODE_HEIGHT: u32 = 240; +const ENCODE_FRAMES: u32 = 8; + +/// A BGRA frame that moves with `n`, so the encoder has motion to code. +fn moving_frame(n: u32) -> Vec { + let mut data = Vec::with_capacity((ENCODE_WIDTH * ENCODE_HEIGHT * 4) as usize); + for y in 0..ENCODE_HEIGHT { + for x in 0..ENCODE_WIDTH { + let (sx, sy) = (x + n * 3, y + n); + let r = (sx ^ sy) as u8; + let g = sx.wrapping_mul(2).wrapping_add(sy) as u8; + let b = (sy * 3) as u8; + data.extend_from_slice(&[b, g, r, 255]); + } + } + data +} + +/// Convert and encode `ENCODE_FRAMES` frames as H.264 on `context`, the way a +/// capture pipeline would: an RGB image goes through the colour converter +/// straight into the encoder's input. +fn encode_all(context: &VideoContext) -> Result, Box> { + let mut encoder = Encoder::new( + context.clone(), + EncodeConfig::h264(ENCODE_WIDTH, ENCODE_HEIGHT) + .with_rate_control(RateControlMode::Cqp) + .with_quality_level(22) + .with_frame_rate(30, 1) + .with_b_frames(0), + )?; + let mut converter = ColorConverter::new( + context.clone(), + ColorConverterConfig::new( + ENCODE_WIDTH, + ENCODE_HEIGHT, + InputFormat::BGRA, + OutputFormat::NV12, + ColorSpec::Srgb, + ColorSpec::Srgb, + ColorRange::Limited, + ), + )?; + let mut stream = Vec::new(); + for n in 0..ENCODE_FRAMES { + let src = + unsafe { create_src_image(context, ENCODE_WIDTH, ENCODE_HEIGHT, &moving_frame(n))? }; + converter.convert( + src.image, + vk::ImageLayout::SHADER_READ_ONLY_OPTIMAL, + encoder.input_image(), + )?; + let packet = pollster::block_on(encoder.encode(encoder.input_image())?)?; + stream.extend_from_slice(&packet.data); + } + Ok(stream) +} + +/// Luma PSNR between two NV12 frames of the encode size. +fn luma_psnr(a: &[u8], b: &[u8]) -> f64 { + let n = (ENCODE_WIDTH * ENCODE_HEIGHT) as usize; + let mse = a[..n] + .iter() + .zip(&b[..n]) + .map(|(&x, &y)| (x as f64 - y as f64).powi(2)) + .sum::() + / n as f64; + if mse == 0.0 { + f64::INFINITY + } else { + 10.0 * (255.0f64 * 255.0 / mse).log10() + } +} + +#[test] +#[ignore = "requires a Vulkan Video device"] +fn encode_on_a_vulkan_1_1_device() -> Result<(), Box> { + let own = VideoContextBuilder::new() + .enable_validation(validation_requested()) + .require_encode(Codec::H264) + .require_decode(Codec::H264) + .build()?; + let expected = encode_all(&own)?; + + let (entry, instance) = instance_1_1()?; + let builder = VideoContextBuilder::new().require_encode(Codec::H264); + let physical_device = same_gpu(&instance, &own); + let reqs = builder.encode_device_requirements(&entry, &instance, physical_device)?; + assert!( + reqs.extensions.contains(&ash::khr::push_descriptor::NAME), + "the converter needs push descriptors, which every driver this runs on has" + ); + let app = create_app_device(entry, instance, physical_device, &reqs)?; + let context = builder + .with_encode_queue(spare(&app, reqs.queues.encode.unwrap())) + .with_transfer_queue(spare(&app, reqs.queues.transfer)) + .with_compute_queue(spare(&app, reqs.queues.compute)) + .build_from_existing_encode( + app.entry.clone(), + app.instance.clone(), + app.physical_device, + app.device.clone(), + )?; + let actual = encode_all(&context)?; + drop(context); + + // Both streams decoded by the same decoder, so any difference is the + // encode's. Hardware encoders are not always bit-exact run to run, so the + // comparison is a quality floor rather than equality. + let expected_frames = decode_all(&own, &expected)?; + let actual_frames = decode_all(&own, &actual)?; + assert_eq!(actual_frames.len(), ENCODE_FRAMES as usize, "frame count"); + assert_eq!(expected_frames.len(), actual_frames.len(), "frame count"); + let psnrs: Vec = actual_frames + .iter() + .zip(&expected_frames) + .map(|(a, e)| luma_psnr(a, e)) + .collect(); + println!( + "adopted vs own encode, luma PSNR per frame: {:?}; streams bit-identical: {}", + psnrs, + actual == expected + ); + for (i, psnr) in psnrs.iter().enumerate() { + assert!( + *psnr >= 45.0, + "frame {i}: {psnr:.2} dB against the own-context encode" + ); + } + Ok(()) +} From 76431b33bd47e0b374fc7b77bf2741f9d8d79dab Mon Sep 17 00:00:00 2001 From: DatCaptainHorse Date: Wed, 23 Sep 2026 08:24:11 +0300 Subject: [PATCH 06/16] feat: order convert and encode against caller work on the GPU ColorConverter::convert submitted its dispatch and then waited on a fence, and Encoder::encode had no way to wait for anything the caller submitted. That was harmless when every frame crossed from another device anyway, since that crossing needed a CPU wait of its own. On a shared device it is the last CPU round trip left in the frame: the caller's copy, the conversion and the encode can all be ordered by semaphores instead. TimelinePoint names a timeline semaphore value. convert_async() waits for the points it is given, submits, and returns the point its own timeline reaches when the conversion is done; encode_after() makes the frame's first submission, the upload copy if there is one and otherwise the encode, wait for the points it is given. So a frame goes caller -> convert -> encode with each step waiting on the previous on the GPU. convert() and encode() are now those with nothing to wait for, and convert() still blocks until done, so existing callers see no difference. External waits hold back ALL_COMMANDS rather than a narrower stage: pixelforge's submissions open with layout transitions the caller cannot see, and a wait that let those run early would race the caller's writes. The converter used to wait for its fence right after submitting; it now waits at the start of the next conversion instead, before the command buffer, output buffer or cached source view are reused. The adopted-device encode test now takes this path: each conversion waits on a point the host signals only after submitting it, and each encode on the conversion. On RADV the result is bit-identical to the synchronous path on a context pixelforge created itself. Co-Authored-By: Claude Opus 5.5 --- src/converter/mod.rs | 94 ++++++++++++++++++++++++++++++++------ src/converter/pipeline.rs | 3 ++ src/encoder/codec.rs | 20 ++++++++- src/encoder/mod.rs | 32 +++++++++++-- src/encoder/pipeline.rs | 16 ++++++- src/encoder/resources.rs | 37 ++++++++------- src/lib.rs | 2 + src/sync.rs | 43 ++++++++++++++++++ tests/adopted.rs | 95 ++++++++++++++++++++++++++++++++------- 9 files changed, 290 insertions(+), 52 deletions(-) create mode 100644 src/sync.rs diff --git a/src/converter/mod.rs b/src/converter/mod.rs index b0eb6649..beec0ed1 100644 --- a/src/converter/mod.rs +++ b/src/converter/mod.rs @@ -10,6 +10,8 @@ mod pipeline; use crate::encoder::ColorDescription; use crate::error::{PixelForgeError, Result}; +use crate::sync::TimelinePoint; +use crate::video::TimelineChain; use crate::vulkan::VideoContext; use ash::vk; use tracing::debug; @@ -384,6 +386,12 @@ pub struct ColorConverter { command_pool: vk::CommandPool, command_buffer: vk::CommandBuffer, fence: vk::Fence, + /// Whether a conversion was submitted and `fence` not yet waited on. The + /// command buffer, output buffer and cached source view are all reused + /// by the next conversion, so it waits for this one first. + in_flight: bool, + /// Signalled by each conversion, for [`Self::convert_async`] to hand out. + timeline: TimelineChain, } impl ColorConverter { @@ -757,6 +765,43 @@ impl ColorConverter { target_image: vk::Image, ) -> Result<()> { let start = std::time::Instant::now(); + self.convert_async(src_image, src_layout, target_image, &[])?; + self.wait_idle()?; + debug!("ColorConverter::convert() took {:?}", start.elapsed()); + Ok(()) + } + + /// Convert like [`Self::convert`], but without waiting on the CPU. + /// + /// The conversion waits on the GPU for every point in `wait` before + /// reading `src_image`, and returns the point it signals when done. Pass + /// that to [`Encoder::encode_after`](crate::Encoder::encode_after) to + /// encode the result, and wait on it before writing `src_image` again. + /// + /// On a device shared with the caller, where the caller renders or copies + /// into `src_image` itself, this is the whole handover: the caller's + /// submission signals a [`TimelinePoint`], the conversion waits on it, the + /// encode waits on the conversion. If the caller's queue is in another + /// family than [`VideoContext::compute_queue_family`], create `src_image` + /// with `VK_SHARING_MODE_CONCURRENT` across both, and pass the layout it + /// is actually in as `src_layout`: `UNDEFINED` is taken to mean a + /// first-time external-memory import and acquires ownership from + /// `VK_QUEUE_FAMILY_EXTERNAL`, which an image that never left the device + /// does not have to give. + /// + /// The next call to either method waits for this conversion to finish + /// before recording, since both reuse the same command buffer. + pub fn convert_async( + &mut self, + src_image: vk::Image, + src_layout: vk::ImageLayout, + target_image: vk::Image, + wait: &[TimelinePoint], + ) -> Result { + // Before anything is reused, the view below included: switching source + // images destroys the cached view, which the previous conversion may + // still be reading. + self.wait_idle()?; // Get or create ImageView for the source image (must happen before // borrowing device immutably, since this takes &mut self). @@ -1021,27 +1066,47 @@ impl ColorConverter { .map_err(|e| PixelForgeError::CommandBuffer(e.to_string()))?; } - // Submit and wait. + let waits: Vec = + wait.iter().map(TimelinePoint::wait_info).collect(); + let (semaphore, value) = self.timeline.pending_signal(); + let signals = [vk::SemaphoreSubmitInfo::default() + .semaphore(semaphore) + .value(value) + .stage_mask(vk::PipelineStageFlags2::ALL_COMMANDS)]; + let command_buffers = + [vk::CommandBufferSubmitInfo::default().command_buffer(self.command_buffer)]; + let submit_info = vk::SubmitInfo2::default() + .wait_semaphore_infos(&waits) + .command_buffer_infos(&command_buffers) + .signal_semaphore_infos(&signals); + unsafe { device .reset_fences(&[self.fence]) .map_err(|e| PixelForgeError::CommandBuffer(e.to_string()))?; - - let command_buffers = [self.command_buffer]; - let submit_info = vk::SubmitInfo::default().command_buffers(&command_buffers); - - device - .queue_submit(self.context.compute_queue(), &[submit_info], self.fence) - .map_err(|e| PixelForgeError::CommandBuffer(e.to_string()))?; - - device - .wait_for_fences(&[self.fence], true, u64::MAX) + self.context + .sync2() + .queue_submit2(self.context.compute_queue(), &[submit_info], self.fence) .map_err(|e| PixelForgeError::CommandBuffer(e.to_string()))?; } + self.timeline.commit(); + self.in_flight = true; - let elapsed = start.elapsed(); - debug!("ColorConverter::convert() took {:?}", elapsed); + Ok(TimelinePoint::new(semaphore, value)) + } + /// Wait on the CPU for the last conversion submitted, if it is still + /// running. + fn wait_idle(&mut self) -> Result<()> { + if self.in_flight { + unsafe { + self.context + .device() + .wait_for_fences(&[self.fence], true, u64::MAX) + } + .map_err(|e| PixelForgeError::CommandBuffer(e.to_string()))?; + self.in_flight = false; + } Ok(()) } @@ -1081,8 +1146,11 @@ 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(); unsafe { let device = self.context.device(); + self.timeline.destroy(device); // Destroy cached source image view. if let Some((_, view)) = self.cached_src_view.take() { diff --git a/src/converter/pipeline.rs b/src/converter/pipeline.rs index 29d8248e..b3de35d2 100644 --- a/src/converter/pipeline.rs +++ b/src/converter/pipeline.rs @@ -140,6 +140,7 @@ pub fn create_converter( let push_descriptor = ash::khr::push_descriptor::Device::load(context.instance(), context.device()); + let timeline = crate::video::TimelineChain::new(&context)?; // Create command pool for compute queue. let pool_info = vk::CommandPoolCreateInfo::default() @@ -177,6 +178,8 @@ pub fn create_converter( command_pool, command_buffer, fence, + in_flight: false, + timeline, push_descriptor, }) } diff --git a/src/encoder/codec.rs b/src/encoder/codec.rs index 847701f9..74edee7e 100644 --- a/src/encoder/codec.rs +++ b/src/encoder/codec.rs @@ -24,6 +24,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; /// Per-encoder state shared by every codec. @@ -72,6 +73,9 @@ pub(crate) struct EncoderCommon { pub upload_command_pool: vk::CommandPool, pub upload_command_buffer: vk::CommandBuffer, pub upload_fence: vk::Fence, + /// Caller work this frame's first submission must wait for. The upload + /// takes them when it copies, otherwise the encode submission does. + pub pending_waits: Vec, } impl EncoderCommon { @@ -100,6 +104,7 @@ impl EncoderCommon { if src_image == dst_image { return Ok(()); } + let waits = std::mem::take(&mut self.pending_waits); let params = UploadParams { upload_command_buffer: self.upload_command_buffer, @@ -111,6 +116,7 @@ impl EncoderCommon { pixel_format: self.config.pixel_format, input_image_layout, upload_queue: self.context.transfer_queue(), + waits: &waits, }; upload_image_to_input(&self.context, ¶ms)?; self.pipeline.current_mut().input_image_layout = vk::ImageLayout::VIDEO_ENCODE_SRC_KHR; @@ -128,10 +134,12 @@ impl EncoderCommon { let encode_queue = self.context.video_encode_queue().ok_or_else(|| { PixelForgeError::NoSuitableDevice("No video encode queue available".to_string()) })?; + let waits = std::mem::take(&mut self.pending_waits); let future = self.pipeline.submit_current( self.context.device(), self.context.sync2(), encode_queue, + &waits, )?; self.dpb_slot_active[self.current_dpb_slot as usize] = true; Ok(future) @@ -314,9 +322,16 @@ impl CodecEncoder { self.common.pipeline.input_image() } - /// Encode one frame, returning a future for its packet. See [`crate::Encoder::encode`]. - pub fn encode(&mut self, src_image: vk::Image) -> Result { + /// Encode one frame once `wait` is reached, returning a future for its + /// packet. See [`crate::Encoder::encode_after`]. + pub fn encode_after( + &mut self, + src_image: vk::Image, + wait: &[TimelinePoint], + ) -> Result { let plan = self.common.begin_frame(); + self.common.pending_waits.clear(); + self.common.pending_waits.extend_from_slice(wait); self.common.upload(src_image)?; let setup = self.codec.begin_picture(&mut self.common, &plan)?; @@ -715,6 +730,7 @@ pub(crate) fn build_encoder_common(req: &CommonInitRequest) -> Result vk::Image; - fn encode(&mut self, src_image: vk::Image) -> Result; + fn encode_after( + &mut self, + src_image: vk::Image, + wait: &[TimelinePoint], + ) -> Result; fn flush(&mut self) -> Result<()>; fn request_idr(&mut self); fn invalidate_reference_frames(&mut self, first_lost_display_order: u64); @@ -582,8 +587,12 @@ impl EncoderApi for codec::CodecEncoder { fn input_image(&self) -> vk::Image { codec::CodecEncoder::input_image(self) } - fn encode(&mut self, src_image: vk::Image) -> Result { - codec::CodecEncoder::encode(self, src_image) + fn encode_after( + &mut self, + src_image: vk::Image, + wait: &[TimelinePoint], + ) -> Result { + codec::CodecEncoder::encode_after(self, src_image, wait) } fn flush(&mut self) -> Result<()> { codec::CodecEncoder::flush(self) @@ -663,7 +672,22 @@ impl Encoder { /// # } /// ``` pub fn encode(&mut self, src_image: vk::Image) -> Result { - self.0.encode(src_image) + self.0.encode_after(src_image, &[]) + } + + /// Encode a frame once GPU work the caller submitted has finished. + /// + /// Like [`Self::encode`], but the frame's first GPU submission waits for + /// every point in `wait` before touching `src_image`. Pass the point + /// returned by [`ColorConverter::convert_async`](crate::ColorConverter::convert_async) + /// to encode what it converted without waiting for the conversion on the + /// CPU, or a point the caller's own rendering signals. + pub fn encode_after( + &mut self, + src_image: vk::Image, + wait: &[TimelinePoint], + ) -> Result { + self.0.encode_after(src_image, wait) } /// Wait for all in-flight frames to finish encoding (end-of-stream barrier). diff --git a/src/encoder/pipeline.rs b/src/encoder/pipeline.rs index 117045ba..d46ef585 100644 --- a/src/encoder/pipeline.rs +++ b/src/encoder/pipeline.rs @@ -47,6 +47,7 @@ use crate::encoder::resources::{ }; use crate::encoder::{BitDepth, EncodedPacket, FrameType, PixelFormat}; use crate::error::{PixelForgeError, Result}; +use crate::sync::TimelinePoint; use crate::video::{SlotSync, TimelineChain}; use crate::vulkan::VideoContext; @@ -359,6 +360,7 @@ impl EncodePipeline { device: &ash::Device, sync2: &ash::khr::synchronization2::Device, encode_queue: vk::Queue, + external_waits: &[TimelinePoint], ) -> Result { let wait = self.timeline.wait(); let signal = self.timeline.pending_signal(); @@ -394,6 +396,18 @@ impl EncodePipeline { ) }; + // The previous encode, which shares the DPB, then whatever the caller + // asked this frame to wait for. + let waits: Vec = wait + .into_iter() + .map(|(semaphore, value)| { + vk::SemaphoreSubmitInfo::default() + .semaphore(semaphore) + .value(value) + .stage_mask(vk::PipelineStageFlags2::VIDEO_ENCODE_KHR) + }) + .chain(external_waits.iter().map(TimelinePoint::wait_info)) + .collect(); unsafe { submit_encode_only( device, @@ -401,7 +415,7 @@ impl EncodePipeline { command_buffer, fence, encode_queue, - wait, + &waits, Some(signal), )?; } diff --git a/src/encoder/resources.rs b/src/encoder/resources.rs index 7230ff12..fc69b847 100644 --- a/src/encoder/resources.rs +++ b/src/encoder/resources.rs @@ -1,5 +1,6 @@ use crate::encoder::{BitDepth, PixelFormat}; use crate::error::{PixelForgeError, Result}; +use crate::sync::TimelinePoint; use crate::vulkan::VideoContext; use ash::vk::TaggedStructure; use ash::vk::{self, Handle}; @@ -390,7 +391,7 @@ pub(crate) fn clear_input_image(context: &VideoContext, params: &ClearImageParam } /// Parameters for uploading an image to the encoder's input image. -pub(crate) struct UploadParams { +pub(crate) struct UploadParams<'a> { /// The command buffer to use for the upload. pub upload_command_buffer: vk::CommandBuffer, /// The fence to use for synchronization. @@ -409,6 +410,8 @@ pub(crate) struct UploadParams { pub input_image_layout: vk::ImageLayout, /// The queue to submit transfer operations to. pub upload_queue: vk::Queue, + /// Caller work the copy must wait for. + pub waits: &'a [TimelinePoint], } /// Upload an image to the encoder's input image via GPU-to-GPU copy. @@ -425,7 +428,7 @@ pub(crate) struct UploadParams { /// Returns Ok(()) on success, or an error if any Vulkan operation fails. pub(crate) fn upload_image_to_input( context: &crate::vulkan::VideoContext, - params: &UploadParams, + params: &UploadParams<'_>, ) -> Result<()> { let device = context.device(); @@ -601,11 +604,20 @@ pub(crate) fn upload_image_to_input( unsafe { device.end_command_buffer(params.upload_command_buffer) } .map_err(|e| PixelForgeError::CommandBuffer(e.to_string()))?; - let submit_info = vk::SubmitInfo::default() - .command_buffers(std::slice::from_ref(¶ms.upload_command_buffer)); + let command_buffers = + [vk::CommandBufferSubmitInfo::default().command_buffer(params.upload_command_buffer)]; + let waits: Vec = + params.waits.iter().map(TimelinePoint::wait_info).collect(); + let submit_info = vk::SubmitInfo2::default() + .wait_semaphore_infos(&waits) + .command_buffer_infos(&command_buffers); - unsafe { device.queue_submit(params.upload_queue, &[submit_info], params.upload_fence) } - .map_err(|e| PixelForgeError::CommandBuffer(e.to_string()))?; + unsafe { + context + .sync2() + .queue_submit2(params.upload_queue, &[submit_info], params.upload_fence) + } + .map_err(|e| PixelForgeError::CommandBuffer(e.to_string()))?; unsafe { device.wait_for_fences(&[params.upload_fence], true, u64::MAX) } .map_err(|e| PixelForgeError::CommandBuffer(e.to_string()))?; @@ -810,21 +822,12 @@ pub(crate) unsafe fn submit_encode_only( command_buffer: vk::CommandBuffer, fence: vk::Fence, encode_queue: vk::Queue, - wait_timeline: Option<(vk::Semaphore, u64)>, + wait_infos: &[vk::SemaphoreSubmitInfo<'_>], signal_timeline: Option<(vk::Semaphore, u64)>, ) -> Result<()> { let command_buffer_info = vk::CommandBufferSubmitInfo::default().command_buffer(command_buffer); let command_buffer_infos = [command_buffer_info]; - let wait_infos: Vec = wait_timeline - .into_iter() - .map(|(semaphore, value)| { - vk::SemaphoreSubmitInfo::default() - .semaphore(semaphore) - .value(value) - .stage_mask(vk::PipelineStageFlags2::VIDEO_ENCODE_KHR) - }) - .collect(); let signal_infos: Vec = signal_timeline .into_iter() .map(|(semaphore, value)| { @@ -836,7 +839,7 @@ pub(crate) unsafe fn submit_encode_only( .collect(); let submit_info = vk::SubmitInfo2::default() - .wait_semaphore_infos(&wait_infos) + .wait_semaphore_infos(wait_infos) .command_buffer_infos(&command_buffer_infos) .signal_semaphore_infos(&signal_infos); diff --git a/src/lib.rs b/src/lib.rs index c6598ee0..690a43a3 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -307,6 +307,7 @@ pub mod decoder; pub mod encoder; pub mod error; pub mod image; +pub mod sync; pub(crate) mod video; pub mod vulkan; @@ -332,6 +333,7 @@ pub use encoder::{ }; pub use error::PixelForgeError; pub use image::InputImage; +pub use sync::TimelinePoint; pub use vulkan::VideoContextBuilder; /// Re-export VideoContext for convenience. diff --git a/src/sync.rs b/src/sync.rs new file mode 100644 index 00000000..597be876 --- /dev/null +++ b/src/sync.rs @@ -0,0 +1,43 @@ +//! Ordering pixelforge's GPU work against the caller's, without the CPU. + +use ash::vk; + +/// A point on a timeline semaphore: reached once the semaphore's value is at +/// least `value`. +/// +/// Returned by [`ColorConverter::convert_async`](crate::ColorConverter::convert_async) +/// for the work it submitted, and accepted by it and by +/// [`Encoder::encode_after`](crate::Encoder::encode_after) as work to wait +/// for. On a device shared with the caller this is how a frame moves from the +/// caller's rendering to the converter to the encoder with every step ordered +/// on the GPU: each submission waits on the point the previous one signals, +/// and nothing waits on the CPU. +/// +/// The semaphore must be a timeline semaphore on the same device as the +/// context. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub struct TimelinePoint { + /// A timeline semaphore. + pub semaphore: vk::Semaphore, + /// The value to wait for. + pub value: u64, +} + +impl TimelinePoint { + /// The point where `semaphore` reaches `value`. + pub fn new(semaphore: vk::Semaphore, value: u64) -> Self { + Self { semaphore, value } + } + + /// A wait on this point that holds back every stage of the submission. + /// + /// Pixelforge's submissions start with layout transitions whose stages the + /// caller cannot know, so a narrower stage mask could let one of them run + /// before the caller's work is done. + pub(crate) fn wait_info(&self) -> vk::SemaphoreSubmitInfo<'static> { + vk::SemaphoreSubmitInfo::default() + .semaphore(self.semaphore) + .value(self.value) + .stage_mask(vk::PipelineStageFlags2::ALL_COMMANDS) + } +} diff --git a/tests/adopted.rs b/tests/adopted.rs index a2b8b3f6..ef4e1d96 100644 --- a/tests/adopted.rs +++ b/tests/adopted.rs @@ -20,7 +20,7 @@ use pixelforge::encoder::Codec; use pixelforge::vulkan::{DeviceQueue, DeviceRequirements, VideoContext, VideoContextBuilder}; use pixelforge::{ ColorConverter, ColorConverterConfig, ColorRange, ColorSpec, EncodeConfig, Encoder, - InputFormat, OutputFormat, RateControlMode, + InputFormat, OutputFormat, RateControlMode, TimelinePoint, }; /// An instance and device the test owns, destroyed on drop after everything @@ -227,10 +227,25 @@ fn moving_frame(n: u32) -> Vec { data } +/// How [`encode_all`] hands each frame from the converter to the encoder. +#[derive(Clone, Copy, PartialEq)] +enum Handoff { + /// `convert`, which waits on the CPU, then `encode`. + Cpu, + /// `convert_async` waiting on a point the caller signals, then + /// `encode_after` waiting on the conversion: ordered on the GPU, with + /// nothing waited for on the CPU until the packets are collected. + Gpu, +} + /// Convert and encode `ENCODE_FRAMES` frames as H.264 on `context`, the way a /// capture pipeline would: an RGB image goes through the colour converter /// straight into the encoder's input. -fn encode_all(context: &VideoContext) -> Result, Box> { +fn encode_all( + context: &VideoContext, + handoff: Handoff, +) -> Result, Box> { + let device = context.device(); let mut encoder = Encoder::new( context.clone(), EncodeConfig::h264(ENCODE_WIDTH, ENCODE_HEIGHT) @@ -251,18 +266,65 @@ fn encode_all(context: &VideoContext) -> Result, Box { + converter.convert(src.image, vk::ImageLayout::SHADER_READ_ONLY_OPTIMAL, target)?; + encoder.encode(target)? + } + Handoff::Gpu => { + let ready = TimelinePoint::new(rendered, u64::from(n) + 1); + let converted = converter.convert_async( + src.image, + vk::ImageLayout::SHADER_READ_ONLY_OPTIMAL, + target, + &[ready], + )?; + // Signalled only after the conversion was submitted, so it + // really has to wait. + unsafe { + timeline_fns.signal_semaphore( + &vk::SemaphoreSignalInfo::default() + .semaphore(rendered) + .value(ready.value), + )? + }; + encoder.encode_after(target, &[converted])? + } + }; + sources.push(src); + pending.push(future); + } + let mut stream = Vec::new(); + for future in pending { + stream.extend_from_slice(&pollster::block_on(future)?.data); } + drop(converter); + drop(encoder); + drop(sources); + unsafe { device.destroy_semaphore(rendered, None) }; Ok(stream) } @@ -290,7 +352,7 @@ fn encode_on_a_vulkan_1_1_device() -> Result<(), Box> { .require_encode(Codec::H264) .require_decode(Codec::H264) .build()?; - let expected = encode_all(&own)?; + let expected = encode_all(&own, Handoff::Cpu)?; let (entry, instance) = instance_1_1()?; let builder = VideoContextBuilder::new().require_encode(Codec::H264); @@ -311,12 +373,15 @@ fn encode_on_a_vulkan_1_1_device() -> Result<(), Box> { app.physical_device, app.device.clone(), )?; - let actual = encode_all(&context)?; + let actual = encode_all(&context, Handoff::Gpu)?; drop(context); // Both streams decoded by the same decoder, so any difference is the - // encode's. Hardware encoders are not always bit-exact run to run, so the - // comparison is a quality floor rather than equality. + // encode's, or the handoff's: an encode that did not wait for its + // conversion would pick up the previous frame or a half-written one, far + // below the floor here. The floor is not equality because not every + // encoder is reproducible: RADV gives bit-identical streams, while ANV has + // been seen as low as 47 dB between two runs of the same synchronous path. let expected_frames = decode_all(&own, &expected)?; let actual_frames = decode_all(&own, &actual)?; assert_eq!(actual_frames.len(), ENCODE_FRAMES as usize, "frame count"); @@ -333,7 +398,7 @@ fn encode_on_a_vulkan_1_1_device() -> Result<(), Box> { ); for (i, psnr) in psnrs.iter().enumerate() { assert!( - *psnr >= 45.0, + *psnr >= 40.0, "frame {i}: {psnr:.2} dB against the own-context encode" ); } From f91ee0898e159d66dca10ec21f56aac069888b57 Mon Sep 17 00:00:00 2001 From: DatCaptainHorse Date: Wed, 23 Sep 2026 08:32:53 +0300 Subject: [PATCH 07/16] 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, 72 insertions(+), 31 deletions(-) diff --git a/src/converter/mod.rs b/src/converter/mod.rs index beec0ed1..a2884978 100644 --- a/src/converter/mod.rs +++ b/src/converter/mod.rs @@ -371,8 +371,12 @@ 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 last conversion read its source through, destroyed once + // that conversion is done. 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, @@ -798,14 +802,9 @@ impl ColorConverter { target_image: vk::Image, wait: &[TimelinePoint], ) -> Result { - // Before anything is reused, the view below included: switching source - // images destroys the cached view, which the previous conversion may - // still be reading. + // Before anything is reused, and before the previous view goes. self.wait_idle()?; - - // 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)?; + let src_view = self.create_src_view(src_image)?; let device = self.context.device(); @@ -1096,7 +1095,7 @@ impl ColorConverter { } /// Wait on the CPU for the last conversion submitted, if it is still - /// running. + /// running, and destroy the view it read its source through. fn wait_idle(&mut self) -> Result<()> { if self.in_flight { unsafe { @@ -1107,23 +1106,15 @@ impl ColorConverter { .map_err(|e| PixelForgeError::CommandBuffer(e.to_string()))?; self.in_flight = false; } + if let Some(view) = self.src_view.take() { + unsafe { self.context.device().destroy_image_view(view, None) }; + } 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::wait_idle`]. + 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) @@ -1139,7 +1130,7 @@ 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) } } @@ -1152,11 +1143,6 @@ impl Drop for ColorConverter { let device = self.context.device(); self.timeline.destroy(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 b3de35d2..3eb332f9 100644 --- a/src/converter/pipeline.rs +++ b/src/converter/pipeline.rs @@ -171,7 +171,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 515da8dc87d179477d025e2f71565232cacaeea4 Mon Sep 17 00:00:00 2001 From: DatCaptainHorse Date: Wed, 23 Sep 2026 08:34:34 +0300 Subject: [PATCH 08/16] feat(encoder): convert RGB input in the encoder where the device can VK_VALVE_video_encode_rgb_conversion lets an encoder take an RGB image and apply the YUV matrix itself. Where the colour conversion is nothing but that matrix, which covers ordinary SDR capture, the converter's compute dispatch and its buffer-to-image copy disappear from the frame entirely. EncodeConfig::with_rgb_input(format) turns it on. The one profile each codec builds gains the RGB conversion struct, so the capability and format queries, the input and DPB images, the query pools and the session all name the same profile, as the extension requires. The input image becomes a single-plane RGB image while the DPB stays YUV, and Encoder::encode copies an RGB source into it. The matrix and range come from the configured colour description, checked against what the driver reports it can do. The hardware applies only the matrix, so ColorConverterConfig gains rgb_encode_input(), which says whether a given conversion can be handed to the encoder instead: the device has the extension, source and target are the same space, and the formats line up. Anything involving a transfer function or a gamut change still goes through the shader. Chroma siting prefers the midpoint on both axes, matching the converter's 2x2 average. RADV only offers co-sited chroma horizontally, so there it falls back to that, which is also what H.264 and H.265 players assume when a stream does not say. On an RX 9060 XT, H.264 lands at 46 dB in luma against the shader path on the same frames; H.265 and AV1, scored against an ffmpeg BT.709 conversion of those frames, at 46 and 48 dB. The new test compares the two paths for H.264, checks the other codecs produce a stream, and is skipped on devices without the extension. Based on the approach in hgaiser/pixelforge#14, reworked for the current encoder structure. Co-authored-by: Ryan Bateman Co-Authored-By: Claude Opus 5.5 --- src/converter/mod.rs | 31 +++++ src/encoder/av1/init.rs | 15 ++- src/encoder/codec.rs | 134 ++++++++++++++++++++- src/encoder/h264/init.rs | 15 ++- src/encoder/h265/init.rs | 15 ++- src/encoder/mod.rs | 27 +++++ src/encoder/pipeline.rs | 36 +++--- src/encoder/resources.rs | 117 ++++++++++++++++++- src/vulkan.rs | 43 +++++++ src/vulkan/encode_device.rs | 11 ++ tests/rgb_encode.rs | 227 ++++++++++++++++++++++++++++++++++++ 11 files changed, 642 insertions(+), 29 deletions(-) create mode 100644 tests/rgb_encode.rs diff --git a/src/converter/mod.rs b/src/converter/mod.rs index a2884978..6959a9a0 100644 --- a/src/converter/mod.rs +++ b/src/converter/mod.rs @@ -325,6 +325,37 @@ impl ColorConverterConfig { ) } + /// The RGB input to give the encoder instead of running this conversion, + /// when the encoder can do it itself. + /// + /// `Some` when `context` has `VK_VALVE_video_encode_rgb_conversion` and + /// the conversion is nothing but the YUV matrix: source and target are the + /// same space, so no transfer function or gamut change is involved, the + /// output is 4:2:0, and the input is 8-bit for an 8-bit output or + /// `ABGR2101010` for a 10-bit one. Build the encoder with + /// [`EncodeConfig::with_rgb_input`](crate::EncodeConfig::with_rgb_input) + /// and this format, and with [`Self::color_description`], then hand it the + /// RGB images directly and skip the converter. + /// + /// Whether the driver accepts the format for a particular codec and + /// profile is only known when the encoder is created, so if + /// [`Encoder::new`](crate::Encoder::new) refuses it, fall back to the + /// converter. + pub fn rgb_encode_input(&self, context: &VideoContext) -> Option { + if !context.has_video_encode_rgb_conversion() || self.source != self.target { + return None; + } + let matches = match self.output_format { + OutputFormat::NV12 => matches!( + self.input_format, + InputFormat::BGRx | InputFormat::BGRA | InputFormat::RGBx | InputFormat::RGBA + ), + OutputFormat::P010 => self.input_format == InputFormat::ABGR2101010, + _ => false, + }; + matches.then_some(self.input_format) + } + /// Whether the shader can get from this source to this target. /// /// Anything that would need tone mapping or a forward gamma encode is not diff --git a/src/encoder/av1/init.rs b/src/encoder/av1/init.rs index b4726636..6b732319 100644 --- a/src/encoder/av1/init.rs +++ b/src/encoder/av1/init.rs @@ -1,7 +1,8 @@ use super::{Av1, MIN_BITSTREAM_BUFFER_SIZE, SUPERBLOCK_SIZE}; use crate::encoder::codec::{ - CodecEncoder, CommonInitRequest, build_encoder_common, query_video_caps, + CodecEncoder, CommonInitRequest, RgbConversionCaps, build_encoder_common, query_video_caps, + rgb_conversion_profile, }; use crate::encoder::{ColorDescription, EncodeConfig, PixelFormat}; use crate::error::{PixelForgeError, Result}; @@ -47,13 +48,17 @@ impl Av1 { .tuning_mode(encoder_tuning_mode); let mut av1_profile_info = vk::VideoEncodeAV1ProfileInfoKHR::default().std_profile(profile); - let profile_info = vk::VideoProfileInfoKHR::default() + let mut rgb_profile = rgb_conversion_profile(); + let mut profile_info = vk::VideoProfileInfoKHR::default() .video_codec_operation(vk::VideoCodecOperationFlagsKHR::ENCODE_AV1) .chroma_subsampling(chroma_subsampling) .luma_bit_depth(bit_depth) .chroma_bit_depth(bit_depth) .push(&mut av1_profile_info) .push(&mut video_encode_usage_info); + if config.rgb_input.is_some() { + profile_info = profile_info.push(&mut rgb_profile); + } let bitstream_buffer_size = MIN_BITSTREAM_BUFFER_SIZE .max(config.dimensions.width as usize * config.dimensions.height as usize); @@ -65,7 +70,12 @@ impl Av1 { let mut capabilities = vk::VideoCapabilitiesKHR::default() .push(&mut encode_caps) .push(&mut av1_caps); + let mut rgb_caps = vk::VideoEncodeRgbConversionCapabilitiesVALVE::default(); + if config.rgb_input.is_some() { + capabilities = capabilities.push(&mut rgb_caps); + } let caps = query_video_caps(&context, &profile_info, &mut capabilities)?; + let rgb_caps = config.rgb_input.map(|_| RgbConversionCaps::from(&rgb_caps)); let init = build_encoder_common(&CommonInitRequest { context: &context, @@ -84,6 +94,7 @@ impl Av1 { // when the driver does not support separate reference images // (required for AMD RADV). allow_layered_dpb: true, + rgb_caps, })?; let active_reference_count = init.active_reference_count; let common = init.common; diff --git a/src/encoder/codec.rs b/src/encoder/codec.rs index 74edee7e..fce375be 100644 --- a/src/encoder/codec.rs +++ b/src/encoder/codec.rs @@ -13,6 +13,7 @@ //! scaffolding is here. use ash::vk; +use ash::vk::TaggedStructure; use crate::encoder::dpb::MAX_DPB_SLOTS; use crate::encoder::gop::{GopFrameType, GopPosition, GopStructure}; @@ -115,6 +116,7 @@ impl EncoderCommon { height: self.config.dimensions.height, pixel_format: self.config.pixel_format, input_image_layout, + rgb: self.config.rgb_input.is_some(), upload_queue: self.context.transfer_queue(), waits: &waits, }; @@ -447,6 +449,92 @@ pub(crate) struct CommonInitRequest<'a> { /// Whether the codec can use a layered DPB image when the driver lacks /// `SEPARATE_REFERENCE_IMAGES` (H.264/H.265 yes; AV1 no). pub allow_layered_dpb: bool, + /// What the hardware RGB conversion supports for this profile, queried + /// alongside the codec capabilities when [`EncodeConfig::rgb_input`] is + /// set. + pub rgb_caps: Option, +} + +/// The profile struct that turns on RGB input. When +/// [`EncodeConfig::rgb_input`] is set it goes on the one profile the codec +/// builds, which every capability query, image, query pool and the session +/// then share: they must all name the same profile. +pub(crate) fn rgb_conversion_profile() -> vk::VideoEncodeProfileRgbConversionInfoVALVE<'static> { + vk::VideoEncodeProfileRgbConversionInfoVALVE::default().perform_encode_rgb_conversion(true) +} + +/// `VkVideoEncodeRgbConversionCapabilitiesVALVE`, without its `pNext`. +#[derive(Debug, Clone, Copy)] +pub(crate) struct RgbConversionCaps { + models: vk::VideoEncodeRgbModelConversionFlagsVALVE, + ranges: vk::VideoEncodeRgbRangeCompressionFlagsVALVE, + x_chroma_offsets: vk::VideoEncodeRgbChromaOffsetFlagsVALVE, + y_chroma_offsets: vk::VideoEncodeRgbChromaOffsetFlagsVALVE, +} + +impl From<&vk::VideoEncodeRgbConversionCapabilitiesVALVE<'_>> for RgbConversionCaps { + fn from(caps: &vk::VideoEncodeRgbConversionCapabilitiesVALVE<'_>) -> Self { + Self { + models: caps.rgb_models, + ranges: caps.rgb_ranges, + x_chroma_offsets: caps.x_chroma_offsets, + y_chroma_offsets: caps.y_chroma_offsets, + } + } +} + +impl RgbConversionCaps { + /// The session settings that make the hardware produce what `desc` + /// describes, or why it cannot. + fn session_info( + &self, + desc: &ColorDescription, + ) -> Result> { + use vk::VideoEncodeRgbChromaOffsetFlagsVALVE as Offset; + use vk::VideoEncodeRgbModelConversionFlagsVALVE as Model; + use vk::VideoEncodeRgbRangeCompressionFlagsVALVE as Range; + + // H.273 matrix coefficients. + let model = match desc.matrix_coefficients { + 1 => Model::YCBCR_709, + 5 | 6 => Model::YCBCR_601, + 9 => Model::YCBCR_2020, + other => { + return Err(PixelForgeError::InvalidInput(format!( + "RGB input: no hardware conversion for matrix coefficients {other}" + ))); + } + }; + let range = if desc.full_range { + Range::FULL_RANGE + } else { + Range::NARROW_RANGE + }; + if !self.models.contains(model) || !self.ranges.contains(range) { + return Err(PixelForgeError::NoSuitableDevice(format!( + "RGB input: the driver converts {:?} in {:?}, not {:?} in {:?}", + self.models, self.ranges, model, range + ))); + } + // Chroma sited midway between luma samples on both axes, which is what + // the colour converter's 2x2 average produces, so a stream looks the + // same whichever path made it. Where the driver cannot, it gets + // whichever siting it can do. + let pick = |supported: Offset| { + if supported.contains(Offset::MIDPOINT) { + Offset::MIDPOINT + } else { + Offset::COSITED_EVEN + } + }; + Ok( + vk::VideoEncodeSessionRgbConversionCreateInfoVALVE::default() + .rgb_model(model) + .rgb_range(range) + .x_chroma_offset(pick(self.x_chroma_offsets)) + .y_chroma_offset(pick(self.y_chroma_offsets)), + ) + } } /// Result of [`build_encoder_common`]: the assembled common state plus the @@ -565,6 +653,33 @@ pub(crate) fn build_encoder_common(req: &CommonInitRequest) -> Result { + return Err(PixelForgeError::NoSuitableDevice( + "RGB input needs VK_VALVE_video_encode_rgb_conversion, which this device \ + does not have enabled" + .to_string(), + )); + } + Some(_) => { + let caps = req.rgb_caps.ok_or_else(|| { + PixelForgeError::NoSuitableDevice( + "RGB input: the driver reported no conversion capabilities".to_string(), + ) + })?; + Some( + caps.session_info( + &config + .color_description + .unwrap_or_else(ColorDescription::bt709), + )?, + ) + } + None => None, + }; + // Pick input (SRC) and reference (DPB) formats. let preferred_src_format = get_video_format(config.pixel_format, config.bit_depth); let supported_src_formats = query_supported_video_formats( @@ -587,20 +702,22 @@ pub(crate) fn build_encoder_common(req: &CommonInitRequest) -> Result Result Result Result, + /// Take RGB input and let the encoder convert it to YUV itself, through + /// `VK_VALVE_video_encode_rgb_conversion`. See [`Self::with_rgb_input`]. + pub rgb_input: Option, /// Usage hint for encoding. pub encode_usage_hint: EncodeUsageHint, /// Content hint for encoding. @@ -373,6 +376,7 @@ impl EncodeConfig { virtual_buffer_size_ms: 1000, initial_virtual_buffer_size_ms: 1000, color_description: None, + rgb_input: None, encode_usage_hint: EncodeUsageHint::Default, encode_content_hint: EncodeContentHint::Default, encoder_tuning_mode: EncoderTuningMode::Default, @@ -401,6 +405,7 @@ impl EncodeConfig { virtual_buffer_size_ms: 1000, initial_virtual_buffer_size_ms: 1000, color_description: None, + rgb_input: None, encode_usage_hint: EncodeUsageHint::Default, encode_content_hint: EncodeContentHint::Default, encoder_tuning_mode: EncoderTuningMode::Default, @@ -429,6 +434,7 @@ impl EncodeConfig { virtual_buffer_size_ms: 1000, initial_virtual_buffer_size_ms: 1000, color_description: None, + rgb_input: None, encode_usage_hint: EncodeUsageHint::Default, encode_content_hint: EncodeContentHint::Default, encoder_tuning_mode: EncoderTuningMode::Default, @@ -517,6 +523,27 @@ impl EncodeConfig { self } + /// Take `format` RGB images as input and convert them to YUV in the + /// encoder itself, with no colour conversion shader. + /// + /// Needs `VK_VALVE_video_encode_rgb_conversion` (see + /// [`VideoContext::has_video_encode_rgb_conversion`]), and a driver that + /// accepts `format` as encode input for this codec, profile and bit depth; + /// [`Encoder::new`] fails otherwise. The YUV matrix and range come from + /// [`Self::color_description`], so set that to what the stream should + /// carry. The hardware applies only the matrix: a source that needs a + /// transfer function or gamut change still has to go through + /// [`ColorConverter`](crate::ColorConverter), which + /// [`ColorConverterConfig::rgb_encode_input`](crate::ColorConverterConfig::rgb_encode_input) + /// decides for you. + /// + /// With this set, [`Encoder::input_image`] is an RGB image, and + /// [`Encoder::encode`] copies an RGB source into it. + pub fn with_rgb_input(mut self, format: crate::converter::InputFormat) -> Self { + self.rgb_input = Some(format); + self + } + /// Set the usage hint for encoding. pub fn with_encode_usage_hint(mut self, hint: EncodeUsageHint) -> Self { self.encode_usage_hint = hint; diff --git a/src/encoder/pipeline.rs b/src/encoder/pipeline.rs index d46ef585..12883105 100644 --- a/src/encoder/pipeline.rs +++ b/src/encoder/pipeline.rs @@ -40,9 +40,9 @@ use ash::vk::{self, Handle}; use futures_channel::oneshot; use crate::encoder::resources::{ - ClearImageParams, allocate_command_buffers, clear_input_image, create_bitstream_buffer, - create_encode_feedback_query_pool, create_encode_timestamp_query_pool, create_fence, - create_image, map_bitstream_buffer, query_timestamp_diff, submit_encode_only, + ClearImageParams, allocate_command_buffers, clear_input_image, clear_rgb_input_image, + create_bitstream_buffer, create_encode_feedback_query_pool, create_encode_timestamp_query_pool, + create_fence, create_image, map_bitstream_buffer, query_timestamp_diff, submit_encode_only, wait_and_read_bitstream, }; use crate::encoder::{BitDepth, EncodedPacket, FrameType, PixelFormat}; @@ -165,6 +165,8 @@ pub(crate) struct PipelineConfig<'a> { pub aligned_width: u32, pub aligned_height: u32, pub picture_format: vk::Format, + /// Whether the input image is RGB, for the encoder to convert. + pub rgb_input: bool, pub pixel_format: PixelFormat, pub bit_depth: BitDepth, pub bitstream_buffer_size: usize, @@ -245,19 +247,21 @@ impl EncodePipeline { // Zero the padding between the user dimensions and the aligned coded // extent so the first frame has no undefined samples. - clear_input_image( - context, - &ClearImageParams { - command_buffer: config.upload_command_buffer, - fence: config.upload_fence, - queue: context.transfer_queue(), - image: input_image, - width: config.aligned_width, - height: config.aligned_height, - pixel_format: config.pixel_format, - bit_depth: config.bit_depth, - }, - )?; + let clear = ClearImageParams { + command_buffer: config.upload_command_buffer, + fence: config.upload_fence, + queue: context.transfer_queue(), + image: input_image, + width: config.aligned_width, + height: config.aligned_height, + pixel_format: config.pixel_format, + bit_depth: config.bit_depth, + }; + if config.rgb_input { + clear_rgb_input_image(context, &clear)?; + } else { + clear_input_image(context, &clear)?; + } // Created signaled so it is safe to wait on before the first encode; // `submit_encode_only` resets it before each submit. diff --git a/src/encoder/resources.rs b/src/encoder/resources.rs index fc69b847..339fef54 100644 --- a/src/encoder/resources.rs +++ b/src/encoder/resources.rs @@ -171,6 +171,97 @@ pub(crate) fn create_command_resources( }) } +/// Clear an RGB input image to opaque black and leave it in +/// `VIDEO_ENCODE_SRC_KHR`. +/// +/// The YUV path fills each plane from a staging buffer; an RGB image has a +/// single colour plane, so a clear does it. +pub(crate) fn clear_rgb_input_image( + context: &VideoContext, + params: &ClearImageParams, +) -> Result<()> { + let device = context.device(); + let cb = params.command_buffer; + let range = vk::ImageSubresourceRange { + aspect_mask: vk::ImageAspectFlags::COLOR, + base_mip_level: 0, + level_count: 1, + base_array_layer: 0, + layer_count: 1, + }; + let err = |e: vk::Result| PixelForgeError::CommandBuffer(e.to_string()); + unsafe { + device + .reset_command_buffer(cb, vk::CommandBufferResetFlags::empty()) + .map_err(err)?; + device + .begin_command_buffer( + cb, + &vk::CommandBufferBeginInfo::default() + .flags(vk::CommandBufferUsageFlags::ONE_TIME_SUBMIT), + ) + .map_err(err)?; + let to_transfer = 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(params.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_transfer], + ); + device.cmd_clear_color_image( + cb, + params.image, + vk::ImageLayout::TRANSFER_DST_OPTIMAL, + &vk::ClearColorValue { + float32: [0.0, 0.0, 0.0, 1.0], + }, + &[range], + ); + let to_encode = vk::ImageMemoryBarrier::default() + .old_layout(vk::ImageLayout::TRANSFER_DST_OPTIMAL) + .new_layout(vk::ImageLayout::VIDEO_ENCODE_SRC_KHR) + .src_queue_family_index(vk::QUEUE_FAMILY_IGNORED) + .dst_queue_family_index(vk::QUEUE_FAMILY_IGNORED) + .image(params.image) + .subresource_range(range) + .src_access_mask(vk::AccessFlags::TRANSFER_WRITE); + device.cmd_pipeline_barrier( + cb, + vk::PipelineStageFlags::TRANSFER, + vk::PipelineStageFlags::BOTTOM_OF_PIPE, + vk::DependencyFlags::empty(), + &[], + &[], + &[to_encode], + ); + device.end_command_buffer(cb).map_err(err)?; + let cbs = [cb]; + device.reset_fences(&[params.fence]).map_err(err)?; + device + .queue_submit( + params.queue, + &[vk::SubmitInfo::default().command_buffers(&cbs)], + params.fence, + ) + .map_err(err)?; + device + .wait_for_fences(&[params.fence], true, u64::MAX) + .map_err(err)?; + device.reset_fences(&[params.fence]).map_err(err)?; + } + Ok(()) +} + /// Create DPB images for video encoding. /// /// When `use_layered` is true (required when the driver does not support @@ -408,6 +499,8 @@ pub(crate) struct UploadParams<'a> { pub pixel_format: PixelFormat, /// The current layout of the input image. pub input_image_layout: vk::ImageLayout, + /// Whether both images are single-plane RGB rather than YUV. + pub rgb: bool, /// The queue to submit transfer operations to. pub upload_queue: vk::Queue, /// Caller work the copy must wait for. @@ -544,6 +637,28 @@ pub(crate) fn upload_image_to_input( }, }; + // An RGB input image has one colour plane, copied whole. + let rgb_copy_region = vk::ImageCopy { + src_subresource: vk::ImageSubresourceLayers { + aspect_mask: vk::ImageAspectFlags::COLOR, + mip_level: 0, + base_array_layer: 0, + layer_count: 1, + }, + dst_subresource: vk::ImageSubresourceLayers { + aspect_mask: vk::ImageAspectFlags::COLOR, + mip_level: 0, + base_array_layer: 0, + layer_count: 1, + }, + ..y_copy_region + }; + let regions: &[vk::ImageCopy] = if params.rgb { + &[rgb_copy_region] + } else { + &[y_copy_region, uv_copy_region] + }; + unsafe { device.cmd_copy_image( params.upload_command_buffer, @@ -551,7 +666,7 @@ pub(crate) fn upload_image_to_input( vk::ImageLayout::TRANSFER_SRC_OPTIMAL, params.dst_image, vk::ImageLayout::TRANSFER_DST_OPTIMAL, - &[y_copy_region, uv_copy_region], + regions, ); } diff --git a/src/vulkan.rs b/src/vulkan.rs index 5244a213..25867c6f 100644 --- a/src/vulkan.rs +++ b/src/vulkan.rs @@ -353,6 +353,9 @@ pub struct DeviceFeatures { pub ycbcr_2plane_444_formats: bool, /// `videoEncodeAV1`, from `VkPhysicalDeviceVideoEncodeAV1FeaturesKHR`. pub video_encode_av1: bool, + /// `videoEncodeRgbConversion`, from + /// `VkPhysicalDeviceVideoEncodeRgbConversionFeaturesVALVE`. + pub video_encode_rgb_conversion: bool, } /// The queue families pixelforge selects for decoding on a given device. @@ -536,6 +539,15 @@ fn supports_internally_synchronized_queues( features.internally_synchronized_queues != 0 } +/// Whether `physical_device` has the `videoEncodeRgbConversion` feature. The +/// caller checks for the extension first. +fn supports_rgb_conversion(instance: &ash::Instance, physical_device: vk::PhysicalDevice) -> bool { + let mut features = vk::PhysicalDeviceVideoEncodeRgbConversionFeaturesVALVE::default(); + let mut query = vk::PhysicalDeviceFeatures2::default().push(&mut features); + unsafe { instance.get_physical_device_features2(physical_device, &mut query) }; + features.video_encode_rgb_conversion != 0 +} + fn decode_extension_names(decode_codecs: &[Codec]) -> Vec<&'static std::ffi::CStr> { // Synchronization2 and timeline semaphores are core in 1.3 and 1.2, but // are listed anyway: enabling a promoted extension is harmless, and on a @@ -594,6 +606,7 @@ struct VideoContextInner { supported_encode_codecs: Vec, supported_decode_codecs: Vec, has_push_descriptor: bool, + has_video_encode_rgb_conversion: 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 borrows them and @@ -712,6 +725,14 @@ impl VideoContext { self.inner.has_push_descriptor } + /// Whether `VK_VALVE_video_encode_rgb_conversion` is enabled, extension + /// and feature both: encoders can then take RGB input and convert it + /// themselves, see + /// [`EncodeConfig::with_rgb_input`](crate::EncodeConfig::with_rgb_input). + pub fn has_video_encode_rgb_conversion(&self) -> bool { + self.inner.has_video_encode_rgb_conversion + } + /// Whether `VK_KHR_unified_image_layouts` is enabled, with its video bit. /// /// When it is, `VK_IMAGE_LAYOUT_GENERAL` is valid for every use including @@ -1141,6 +1162,23 @@ impl VideoContext { warn!("VK_KHR_push_descriptor not available; the colour converter will be unavailable"); } + // Lets an encoder take RGB input and convert it to YUV itself, so a + // plain matrix conversion needs no shader at all. Extension and feature + // both, or the profile struct is parsed and ignored. + let has_rgb_conversion_ext = video_encode_queue_family.is_some() + && selected_device_exts.as_ref().is_some_and(|exts| { + has_extension(exts, ash::valve::video_encode_rgb_conversion::NAME) + }); + let has_rgb_conversion = + has_rgb_conversion_ext && supports_rgb_conversion(&instance, physical_device); + let mut rgb_conversion_features = + vk::PhysicalDeviceVideoEncodeRgbConversionFeaturesVALVE::default() + .video_encode_rgb_conversion(true); + if has_rgb_conversion { + push_ext(ash::valve::video_encode_rgb_conversion::NAME.as_ptr()); + debug!("Video encode RGB conversion available"); + } + // Enable synchronization2 feature. let mut sync2_features = vk::PhysicalDeviceSynchronization2Features::default().synchronization2(true); @@ -1257,6 +1295,9 @@ impl VideoContext { if has_ycbcr_conversion { device_create_info = device_create_info.push(&mut ycbcr_features); } + if has_rgb_conversion { + device_create_info = device_create_info.push(&mut rgb_conversion_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 @@ -1307,6 +1348,7 @@ impl VideoContext { supported_encode_codecs, supported_decode_codecs, has_push_descriptor, + has_video_encode_rgb_conversion: has_rgb_conversion, has_unified_image_layouts: has_unified_layouts, owns_device: true, debug_messenger, @@ -1410,6 +1452,7 @@ impl VideoContext { supported_encode_codecs: Vec::new(), supported_decode_codecs, has_push_descriptor: false, + has_video_encode_rgb_conversion: false, has_unified_image_layouts: declared_unified_image_layouts, owns_device: false, // The caller owns the instance; reporting is theirs to set up. diff --git a/src/vulkan/encode_device.rs b/src/vulkan/encode_device.rs index 00dba30f..4c5aa88e 100644 --- a/src/vulkan/encode_device.rs +++ b/src/vulkan/encode_device.rs @@ -11,6 +11,7 @@ use super::queues::{self, QueueOverrides}; use super::{ DeviceFeatures, DeviceRequirements, QueueRoles, VideoContext, VideoContextBuilder, VideoContextInner, scan_queue_families, supports_internally_synchronized_queues, + supports_rgb_conversion, }; use crate::encoder::Codec; use crate::error::{PixelForgeError, Result}; @@ -36,6 +37,7 @@ struct EncodePlan { push_descriptor: bool, ycbcr_2plane_444: bool, sampler_ycbcr_conversion: bool, + rgb_conversion: bool, } impl EncodePlan { @@ -111,6 +113,8 @@ impl EncodePlan { push_descriptor: has(ash::khr::push_descriptor::NAME), ycbcr_2plane_444: has(ash::ext::ycbcr_2plane_444_formats::NAME), sampler_ycbcr_conversion: ycbcr.sampler_ycbcr_conversion != 0, + rgb_conversion: has(ash::valve::video_encode_rgb_conversion::NAME) + && supports_rgb_conversion(instance, physical_device), }) } @@ -137,6 +141,9 @@ impl EncodePlan { if self.ycbcr_2plane_444 { names.push(ash::ext::ycbcr_2plane_444_formats::NAME); } + if self.rgb_conversion { + names.push(ash::valve::video_encode_rgb_conversion::NAME); + } names } @@ -147,6 +154,7 @@ impl EncodePlan { sampler_ycbcr_conversion: self.sampler_ycbcr_conversion, ycbcr_2plane_444_formats: self.ycbcr_2plane_444, video_encode_av1: self.codecs.contains(&Codec::AV1), + video_encode_rgb_conversion: self.rgb_conversion, } } @@ -173,6 +181,8 @@ impl VideoContextBuilder { /// The colour converter needs `VK_KHR_push_descriptor`, which is included /// whenever the device has it. On a device without it, encoding from YUV /// input still works and only the converter is unavailable. + /// `VK_VALVE_video_encode_rgb_conversion` is included the same way, for + /// [`EncodeConfig::with_rgb_input`](crate::EncodeConfig::with_rgb_input). pub fn encode_device_requirements( &self, entry: &ash::Entry, @@ -314,6 +324,7 @@ impl VideoContext { supported_encode_codecs: plan.codecs.clone(), supported_decode_codecs: Vec::new(), has_push_descriptor: plan.push_descriptor, + has_video_encode_rgb_conversion: plan.rgb_conversion, has_unified_image_layouts: false, owns_device: false, // The caller owns the instance; reporting is theirs to set up. diff --git a/tests/rgb_encode.rs b/tests/rgb_encode.rs new file mode 100644 index 00000000..1905f7dd --- /dev/null +++ b/tests/rgb_encode.rs @@ -0,0 +1,227 @@ +//! Encoding RGB input with the encoder's own colour conversion, through +//! `VK_VALVE_video_encode_rgb_conversion`, against the colour converter. +//! +//! When the conversion is nothing but the YUV matrix, the encoder can take the +//! RGB frame as it is and the shader does not run at all. These tests encode +//! the same frames both ways and check the hardware path lands where the +//! shader does. +//! +//! Skipped on devices without the extension, which today is everything but +//! AMD under RADV. + +#[allow(dead_code)] +mod common; +use common::source::{SrcImage, create_src_image, one_shot}; +use common::{Readback, decode_stream}; + +use ash::vk; +use pixelforge::decoder::{DecodeConfig, Decoder}; +use pixelforge::{ + Codec, ColorConverter, ColorConverterConfig, ColorRange, ColorSpec, EncodeConfig, Encoder, + InputFormat, OutputFormat, RateControlMode, VideoContext, VideoContextBuilder, +}; + +const WIDTH: u32 = 320; +const HEIGHT: u32 = 240; +const FRAMES: u32 = 8; + +fn context(codec: Codec) -> Result> { + let mut builder = VideoContextBuilder::new() + .enable_validation(std::env::var("PIXELFORGE_VALIDATION").is_ok()) + .require_encode(codec); + if codec == Codec::H264 { + builder = builder.require_decode(Codec::H264); + } + Ok(builder.build()?) +} + +fn converter_config() -> ColorConverterConfig { + ColorConverterConfig::new( + WIDTH, + HEIGHT, + InputFormat::BGRA, + OutputFormat::NV12, + ColorSpec::Srgb, + ColorSpec::Srgb, + ColorRange::Limited, + ) +} + +/// A BGRA frame that moves with `n`. +fn moving_frame(n: u32) -> Vec { + let mut data = Vec::with_capacity((WIDTH * HEIGHT * 4) as usize); + for y in 0..HEIGHT { + for x in 0..WIDTH { + let (sx, sy) = (x + n * 3, y + n); + data.extend_from_slice(&[(sy * 3) as u8, (sx * 2 + sy) as u8, (sx ^ sy) as u8, 255]); + } + } + data +} + +/// Upload frame `n` and leave it in `GENERAL`, which is where +/// [`Encoder::encode`] expects a source it copies from. +fn source(context: &VideoContext, n: u32) -> Result> { + let src = unsafe { create_src_image(context, WIDTH, HEIGHT, &moving_frame(n))? }; + unsafe { + one_shot(context, |cb| { + let barrier = vk::ImageMemoryBarrier::default() + .old_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(src.image) + .subresource_range(vk::ImageSubresourceRange { + aspect_mask: vk::ImageAspectFlags::COLOR, + base_mip_level: 0, + level_count: 1, + base_array_layer: 0, + layer_count: 1, + }) + .src_access_mask(vk::AccessFlags::SHADER_READ) + .dst_access_mask(vk::AccessFlags::TRANSFER_READ); + context.device().cmd_pipeline_barrier( + cb, + vk::PipelineStageFlags::ALL_COMMANDS, + vk::PipelineStageFlags::ALL_COMMANDS, + vk::DependencyFlags::empty(), + &[], + &[], + &[barrier], + ); + })?; + } + Ok(src) +} + +fn encode_config(codec: Codec) -> EncodeConfig { + let config = match codec { + Codec::H264 => EncodeConfig::h264(WIDTH, HEIGHT), + Codec::H265 => EncodeConfig::h265(WIDTH, HEIGHT), + Codec::AV1 => EncodeConfig::av1(WIDTH, HEIGHT), + }; + config + .with_rate_control(RateControlMode::Cqp) + .with_quality_level(22) + .with_frame_rate(30, 1) + .with_b_frames(0) + .with_color_description(converter_config().color_description().unwrap()) +} + +/// Encode the frames with the colour converter doing the conversion. +fn encode_with_shader( + context: &VideoContext, + codec: Codec, +) -> Result, Box> { + let mut encoder = Encoder::new(context.clone(), encode_config(codec))?; + let mut converter = ColorConverter::new(context.clone(), converter_config())?; + let mut stream = Vec::new(); + for n in 0..FRAMES { + let src = source(context, n)?; + converter.convert(src.image, vk::ImageLayout::GENERAL, encoder.input_image())?; + let packet = pollster::block_on(encoder.encode(encoder.input_image())?)?; + stream.extend_from_slice(&packet.data); + } + Ok(stream) +} + +/// Encode the frames with the encoder doing the conversion. +fn encode_rgb( + context: &VideoContext, + codec: Codec, + format: InputFormat, +) -> Result, Box> { + let mut encoder = Encoder::new(context.clone(), encode_config(codec).with_rgb_input(format))?; + let mut stream = Vec::new(); + for n in 0..FRAMES { + let src = source(context, n)?; + let packet = pollster::block_on(encoder.encode(src.image)?)?; + stream.extend_from_slice(&packet.data); + } + Ok(stream) +} + +fn decode_all( + context: &VideoContext, + stream: &[u8], +) -> Result>, Box> { + let mut readback = Readback::new(context)?; + let mut decoder = Decoder::new(context.clone(), DecodeConfig::h264().with_byte_stream())?; + let mut frames = Vec::new(); + decode_stream(&mut decoder, stream, |frame| { + frames.push(readback.read(&frame)?.y); + Ok(()) + })?; + Ok(frames) +} + +fn psnr(a: &[u8], b: &[u8]) -> f64 { + let mse = a + .iter() + .zip(b) + .map(|(&x, &y)| (x as f64 - y as f64).powi(2)) + .sum::() + / a.len() as f64; + if mse == 0.0 { + f64::INFINITY + } else { + 10.0 * (255.0f64 * 255.0 / mse).log10() + } +} + +/// The format to hand the encoder, or `None` to skip on this device. +fn rgb_format(context: &VideoContext, test: &str) -> Option { + let format = converter_config().rgb_encode_input(context); + if format.is_none() { + eprintln!("skipping {test}: no VK_VALVE_video_encode_rgb_conversion on this device"); + } + format +} + +#[test] +#[ignore = "requires a Vulkan Video device with VK_VALVE_video_encode_rgb_conversion"] +fn rgb_input_matches_the_converter() -> Result<(), Box> { + let context = context(Codec::H264)?; + let Some(format) = rgb_format(&context, "rgb_input_matches_the_converter") else { + return Ok(()); + }; + let shader = decode_all(&context, &encode_with_shader(&context, Codec::H264)?)?; + let hardware = decode_all(&context, &encode_rgb(&context, Codec::H264, format)?)?; + assert_eq!(hardware.len(), FRAMES as usize, "frame count"); + let psnrs: Vec = hardware + .iter() + .zip(&shader) + .map(|(h, s)| psnr(h, s)) + .collect(); + println!("hardware vs shader conversion, luma PSNR per frame: {psnrs:?}"); + // Both apply the same BT.709 matrix to the same frames, then the same + // encoder settings; what is left is rounding in the two conversions and + // what the encoder makes of it. + for (i, p) in psnrs.iter().enumerate() { + assert!(*p >= 40.0, "frame {i}: {p:.2} dB against the shader path"); + } + Ok(()) +} + +#[test] +#[ignore = "requires a Vulkan Video device with VK_VALVE_video_encode_rgb_conversion"] +fn rgb_input_encodes_every_codec() -> Result<(), Box> { + for codec in [Codec::H264, Codec::H265, Codec::AV1] { + let context = match context(codec) { + Ok(context) => context, + Err(e) => { + eprintln!("{codec:?}: no encoder on this device ({e})"); + continue; + } + }; + let Some(format) = rgb_format(&context, "rgb_input_encodes_every_codec") else { + return Ok(()); + }; + let stream = encode_rgb(&context, codec, format)?; + assert!(!stream.is_empty(), "{codec:?}: empty stream"); + if let Ok(dir) = std::env::var("PIXELFORGE_RGB_STREAM_DIR") { + std::fs::write(format!("{dir}/rgb.{codec:?}").to_lowercase(), &stream)?; + } + } + Ok(()) +} From 7f07aad07eec48a0fdbded6a30784aa7c8553bc2 Mon Sep 17 00:00:00 2001 From: DatCaptainHorse Date: Wed, 23 Sep 2026 08:35:53 +0300 Subject: [PATCH 09/16] docs: document shared devices, GPU ordering and RGB input The crate docs gain a section on encoding on the caller's own device: what to create it with, why pixelforge should get queues of its own, and how a frame is ordered from the caller's rendering through conversion to encode. The colour conversion section points at the hardware RGB path, and the examples list gains encode_adopted. README.md is regenerated from them. CHANGELOG covers the additions, the move to push descriptors, the lower Vulkan version an adopted device may use, and the two fixes. AGENTS.md says how to exercise the adopted-device and RGB input tests, 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 | 18 +++++++++++++++++ CHANGELOG.md | 39 +++++++++++++++++++++++++++++++++++ README.md | 46 +++++++++++++++++++++++++++++++++++++++++- src/lib.rs | 57 +++++++++++++++++++++++++++++++++++++++++++++++++++- 4 files changed, 158 insertions(+), 2 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 5ba7a8ec..a76b63a7 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -110,6 +110,18 @@ rebuilt, and getting that wrong is silent rather than loud. Build the reference resolution-changing stream to its first size and a whole-stream reference will not compare. +Pixelforge can also run on a device the caller created, for decode and for +encode. `tests/adopted.rs` builds one from a Vulkan 1.1 instance, as an +application that never asked for 1.3 would, hands pixelforge spare queues, and +checks the output against a context pixelforge created itself. It needs no +ffmpeg. Anything that reaches a core 1.2 or 1.3 entry point panics there, so +run it after touching submission or barrier code. + +The RGB input path (`EncodeConfig::with_rgb_input`) needs +`VK_VALVE_video_encode_rgb_conversion`, which today only RADV has. +`tests/rgb_encode.rs` compares it against the colour converter and skips on +other devices, so a green run elsewhere says nothing about it. + 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 @@ -117,6 +129,12 @@ 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. +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 - Run `cargo fmt` before committing diff --git a/CHANGELOG.md b/CHANGELOG.md index fe0b4de0..5179257d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -102,12 +102,51 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Validation layer messages are now routed into `tracing` through a `VK_EXT_debug_utils` messenger. Previously the layer was loaded with nowhere to report, so enabling validation verified nothing. +- Encoding on a caller-created device: `encode_device_requirements` and + `build_from_existing_encode`, the counterparts of the decode pair. An + application that renders on its own device can encode its images there + directly, with no external memory and no second device. +- `DeviceRequirements::features` names the device features pixelforge needs as + booleans, so a caller can set them in whichever feature structs it already + chains. +- `VideoContextBuilder::with_encode_queue`, `with_decode_queue`, + `with_transfer_queue` and `with_compute_queue` choose exactly which queue an + adopted context submits to, so the caller can keep its own queues to itself. + `DeviceRequirements::queues` says which family serves which role, and + `DeviceRequirements::internally_synchronized_queues` whether the device can + share a queue safely instead. +- `ColorConverter::convert_async` and `Encoder::encode_after` wait for, and in + the converter's case signal, a `TimelinePoint`, so a frame can go from the + caller's rendering through conversion to encode ordered on the GPU, with no + CPU wait in between. +- `EncodeConfig::with_rgb_input` hands RGB frames to the encoder and lets it + apply the YUV matrix itself, through `VK_VALVE_video_encode_rgb_conversion`. + `ColorConverterConfig::rgb_encode_input` says when a conversion can be done + that way instead of by the shader. Based on the approach in #14. ### 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`. +- Adopted devices may come from an instance asking for any Vulkan version from + 1.1 up. Pixelforge reaches synchronization2 through its extension rather than + core 1.3, and the requirements list `VK_KHR_timeline_semaphore`. + - 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. + ## [0.9.1] - 01-09-2026 ### Fixed diff --git a/README.md b/README.md index c4b2e23a..f40040b4 100644 --- a/README.md +++ b/README.md @@ -14,7 +14,11 @@ H.265 and AV1 encode, and H.264 decode. - **Asynchronous pipelines**: both directions submit without waiting. Encoding hands back an [`EncodeFuture`]; decoding delivers frames through a [`DecodeSource`] as the GPU finishes with them. -- **GPU color conversion**: RGB/BGR → YUV via Vulkan compute shaders (BT.709, BT.2020, sRGB→BT.2020+PQ, scRGB-linear→BT.2020+PQ). +- **GPU color conversion**: RGB/BGR → YUV via Vulkan compute shaders (BT.709, BT.2020, sRGB→BT.2020+PQ, scRGB-linear→BT.2020+PQ), + or in the encoder itself where `VK_VALVE_video_encode_rgb_conversion` is available. +- **Shared devices**: encode and decode on a Vulkan device your application + created, from Vulkan 1.1 up, on queues you choose, ordered against your own + work with timeline semaphores. - **HDR support**: 10-bit encoding (P010, YUV444P10), PQ transfer function, BT.2020 color space. - **GPU-native API**: Encode directly from Vulkan images (`vk::Image`). - **Flexible configuration**: Rate control (CBR, VBR, CQP), quality levels, GOP settings. @@ -247,6 +251,43 @@ let mut encoder = Encoder::new(context, encode_config)?; // converter.convert(input_image, layout, encoder.input_image())?; ``` +Where the conversion is nothing but the YUV matrix, the encoder may be able +to do it with no shader at all: +[`ColorConverterConfig::rgb_encode_input`] says when, and +[`EncodeConfig::with_rgb_input`] hands it the RGB frames. + +### Encoding on your own device + +An application that already renders with Vulkan can give pixelforge its own +device instead of letting it create a second one. Frames then never leave +the device: no external memory, no import, and no CPU wait between the +application's work and pixelforge's. + +[`VideoContextBuilder::encode_device_requirements`] lists the queue +families, extensions and features to create the device with, and +[`VideoContextBuilder::build_from_existing_encode`] adopts it. A `VkQueue` +may not be submitted to from two threads at once, so give pixelforge queues +the application does not use, with +[`VideoContextBuilder::with_encode_queue`] and friends. + +[`ColorConverter::convert_async`] and [`Encoder::encode_after`] then order +each frame on the GPU: the application's submission signals a +[`TimelinePoint`], the conversion waits for it and signals its own, and the +encode waits for that. + +```rust +// `rendered` reaches `frame_number` once the application has drawn `image`. +let converted = converter.convert_async( + image, + vk::ImageLayout::GENERAL, + encoder.input_image(), + &[TimelinePoint::new(rendered, frame_number)], +)?; +let packet = encoder.encode_after(encoder.input_image(), &[converted])?; +``` + +`examples/encode_adopted.rs` builds such a device from scratch. + ## Benchmarking Run the encode latency benchmark with: @@ -269,6 +310,9 @@ cargo run --example decode -- input.264 output.yuv # Decode on a caller-created Vulkan device cargo run --example decode_adopted -- input.264 output.yuv +# Encode on a caller-created Vulkan device +cargo run --example encode_adopted -- input.yuv output.h264 + # Encode, choosing the codec (h264, h265 or av1) cargo run --example encode -- h265 diff --git a/src/lib.rs b/src/lib.rs index 690a43a3..7014b94c 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -8,7 +8,11 @@ //! - **Asynchronous pipelines**: both directions submit without waiting. //! Encoding hands back an [`EncodeFuture`]; decoding delivers frames through a //! [`DecodeSource`] as the GPU finishes with them. -//! - **GPU color conversion**: RGB/BGR → YUV via Vulkan compute shaders (BT.709, BT.2020, sRGB→BT.2020+PQ, scRGB-linear→BT.2020+PQ). +//! - **GPU color conversion**: RGB/BGR → YUV via Vulkan compute shaders (BT.709, BT.2020, sRGB→BT.2020+PQ, scRGB-linear→BT.2020+PQ), +//! or in the encoder itself where `VK_VALVE_video_encode_rgb_conversion` is available. +//! - **Shared devices**: encode and decode on a Vulkan device your application +//! created, from Vulkan 1.1 up, on queues you choose, ordered against your own +//! work with timeline semaphores. //! - **HDR support**: 10-bit encoding (P010, YUV444P10), PQ transfer function, BT.2020 color space. //! - **GPU-native API**: Encode directly from Vulkan images (`vk::Image`). //! - **Flexible configuration**: Rate control (CBR, VBR, CQP), quality levels, GOP settings. @@ -244,6 +248,54 @@ //! # } //! ``` //! +//! Where the conversion is nothing but the YUV matrix, the encoder may be able +//! to do it with no shader at all: +//! [`ColorConverterConfig::rgb_encode_input`] says when, and +//! [`EncodeConfig::with_rgb_input`] hands it the RGB frames. +//! +//! ## Encoding on your own device +//! +//! An application that already renders with Vulkan can give pixelforge its own +//! device instead of letting it create a second one. Frames then never leave +//! the device: no external memory, no import, and no CPU wait between the +//! application's work and pixelforge's. +//! +//! [`VideoContextBuilder::encode_device_requirements`] lists the queue +//! families, extensions and features to create the device with, and +//! [`VideoContextBuilder::build_from_existing_encode`] adopts it. A `VkQueue` +//! may not be submitted to from two threads at once, so give pixelforge queues +//! the application does not use, with +//! [`VideoContextBuilder::with_encode_queue`] and friends. +//! +//! [`ColorConverter::convert_async`] and [`Encoder::encode_after`] then order +//! each frame on the GPU: the application's submission signals a +//! [`TimelinePoint`], the conversion waits for it and signals its own, and the +//! encode waits for that. +//! +//! ```rust,no_run +//! # use ash::vk; +//! # use pixelforge::{ColorConverter, Encoder, TimelinePoint}; +//! # fn frame( +//! # converter: &mut ColorConverter, +//! # encoder: &mut Encoder, +//! # rendered: vk::Semaphore, +//! # frame_number: u64, +//! # image: vk::Image, +//! # ) -> Result<(), Box> { +//! // `rendered` reaches `frame_number` once the application has drawn `image`. +//! let converted = converter.convert_async( +//! image, +//! vk::ImageLayout::GENERAL, +//! encoder.input_image(), +//! &[TimelinePoint::new(rendered, frame_number)], +//! )?; +//! let packet = encoder.encode_after(encoder.input_image(), &[converted])?; +//! # Ok(()) +//! # } +//! ``` +//! +//! `examples/encode_adopted.rs` builds such a device from scratch. +//! //! # Benchmarking //! //! Run the encode latency benchmark with: @@ -266,6 +318,9 @@ //! # Decode on a caller-created Vulkan device //! cargo run --example decode_adopted -- input.264 output.yuv //! +//! # Encode on a caller-created Vulkan device +//! cargo run --example encode_adopted -- input.yuv output.h264 +//! //! # Encode, choosing the codec (h264, h265 or av1) //! cargo run --example encode -- h265 //! From a0f965d74550fcf09fa4b237e21a5d245cc46132 Mon Sep 17 00:00:00 2001 From: DatCaptainHorse Date: Wed, 23 Sep 2026 08:42:37 +0300 Subject: [PATCH 10/16] fix(examples): enable every feature the encode requirements list An adopted context takes every feature DeviceRequirements::features sets as enabled, since Vulkan cannot be asked afterwards which were. The adopted test and the encode_adopted example built their devices by hand and left out videoEncodeRgbConversion, and the example also videoEncodeAV1. On RADV the requirements set the first, so the context claimed RGB conversion on a device that never turned it on: the kind of mismatch where the driver parses the structs and ignores them, which surfaces as wrong output rather than an error. Neither used it, which is why nothing failed. Both now set every feature the requirements ask for, and the example says why that matters. Co-Authored-By: Claude Opus 5.5 --- examples/encode_adopted.rs | 17 ++++++++++++++--- tests/adopted.rs | 5 +++++ 2 files changed, 19 insertions(+), 3 deletions(-) diff --git a/examples/encode_adopted.rs b/examples/encode_adopted.rs index be7bfef0..ac2d403e 100644 --- a/examples/encode_adopted.rs +++ b/examples/encode_adopted.rs @@ -73,9 +73,11 @@ fn main() -> Result<(), Box> { }; let ext_ptrs: Vec<_> = reqs.extensions.iter().map(|e| e.as_ptr()).collect(); - // `reqs.features` says which feature bits to turn on. An application that - // already chains `VkPhysicalDeviceVulkan13Features` would set them there; - // this one has nothing else, so it uses the individual structs. + // `reqs.features` says which feature bits to turn on, and every one it + // sets has to be: pixelforge takes what the requirements list as enabled, + // since Vulkan cannot be asked afterwards. An application that already + // chains `VkPhysicalDeviceVulkan13Features` would set them there; this one + // has nothing else, so it uses the individual structs. let features = reqs.features; let mut sync2 = vk::PhysicalDeviceSynchronization2Features::default() .synchronization2(features.synchronization2); @@ -94,6 +96,15 @@ fn main() -> Result<(), Box> { if features.ycbcr_2plane_444_formats { device_info = device_info.push(&mut ycbcr_444); } + let mut av1 = vk::PhysicalDeviceVideoEncodeAV1FeaturesKHR::default().video_encode_av1(true); + if features.video_encode_av1 { + device_info = device_info.push(&mut av1); + } + let mut rgb = vk::PhysicalDeviceVideoEncodeRgbConversionFeaturesVALVE::default() + .video_encode_rgb_conversion(true); + if features.video_encode_rgb_conversion { + device_info = device_info.push(&mut rgb); + } let device = unsafe { instance.create_device(physical_device, &device_info, None)? }; // --- Hand the application's device to pixelforge --- diff --git a/tests/adopted.rs b/tests/adopted.rs index ef4e1d96..ec4342d8 100644 --- a/tests/adopted.rs +++ b/tests/adopted.rs @@ -102,6 +102,8 @@ fn create_app_device( let mut ycbcr_444 = vk::PhysicalDeviceYcbcr2Plane444FormatsFeaturesEXT::default().ycbcr2plane444_formats(true); let mut av1 = vk::PhysicalDeviceVideoEncodeAV1FeaturesKHR::default().video_encode_av1(true); + let mut rgb = vk::PhysicalDeviceVideoEncodeRgbConversionFeaturesVALVE::default() + .video_encode_rgb_conversion(true); let mut unified = vk::PhysicalDeviceUnifiedImageLayoutsFeaturesKHR::default() .unified_image_layouts(true) .unified_image_layouts_video(true); @@ -117,6 +119,9 @@ fn create_app_device( if features.video_encode_av1 { info = info.push(&mut av1); } + if features.video_encode_rgb_conversion { + info = info.push(&mut rgb); + } if reqs.unified_image_layouts { info = info.push(&mut unified); } From ebfa0166fce25b82590c5f450bdf522811329b88 Mon Sep 17 00:00:00 2001 From: DatCaptainHorse Date: Wed, 23 Sep 2026 09:41:01 +0300 Subject: [PATCH 11/16] test: show validation messages from every integration test pixelforge sends the validation layer's findings to tracing. roundtrip, rfi and verify_all install a subscriber; color_conversion, rgb_encode and adopted did not, so with validation on their messages went nowhere, and a run of them 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 those three tests call 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: color_conversion and rgb_encode both draw messages, fixed in the commits that follow. Co-Authored-By: Claude Opus 5.5 --- tests/adopted.rs | 2 ++ tests/color_conversion.rs | 1 + tests/common/mod.rs | 20 ++++++++++++++++++++ tests/rgb_encode.rs | 1 + 4 files changed, 24 insertions(+) diff --git a/tests/adopted.rs b/tests/adopted.rs index ec4342d8..10f05dcb 100644 --- a/tests/adopted.rs +++ b/tests/adopted.rs @@ -175,6 +175,7 @@ fn same_gpu(instance: &ash::Instance, own: &VideoContext) -> vk::PhysicalDevice #[test] #[ignore = "requires a Vulkan Video device"] fn decode_on_a_vulkan_1_1_device() -> Result<(), Box> { + common::init_logging(); let stream = std::fs::read("tests/data/bframes.264")?; let own = VideoContextBuilder::new() @@ -352,6 +353,7 @@ fn luma_psnr(a: &[u8], b: &[u8]) -> f64 { #[test] #[ignore = "requires a Vulkan Video device"] fn encode_on_a_vulkan_1_1_device() -> Result<(), Box> { + common::init_logging(); let own = VideoContextBuilder::new() .enable_validation(validation_requested()) .require_encode(Codec::H264) 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 300b583f..62dbbc98 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; diff --git a/tests/rgb_encode.rs b/tests/rgb_encode.rs index 1905f7dd..37c81b6b 100644 --- a/tests/rgb_encode.rs +++ b/tests/rgb_encode.rs @@ -26,6 +26,7 @@ const HEIGHT: u32 = 240; const FRAMES: u32 = 8; fn context(codec: Codec) -> Result> { + common::init_logging(); let mut builder = VideoContextBuilder::new() .enable_validation(std::env::var("PIXELFORGE_VALIDATION").is_ok()) .require_encode(codec); From 7ca4ad0760208332652546e164cdd5f6f5cc3ac8 Mon Sep 17 00:00:00 2001 From: DatCaptainHorse Date: Wed, 23 Sep 2026 09:39:01 +0300 Subject: [PATCH 12/16] 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. It also gains TRANSFER_SRC usage: an encoder taking RGB input copies from it, which the RGB tests did without the flag, 72 messages' worth. What rgb_encode still draws is the validation layer not modelling VK_VALVE_video_encode_rgb_conversion: it rejects the profile struct in a query pool's chain, which the spec requires to match the session's, and checks the input view against the plain format features rather than the profile's. Co-Authored-By: Claude Opus 5.5 --- tests/adopted.rs | 4 ++-- tests/color_conversion.rs | 12 ++---------- tests/common/source.rs | 19 +++++++++++++++---- tests/rgb_encode.rs | 37 ++++--------------------------------- 4 files changed, 23 insertions(+), 49 deletions(-) diff --git a/tests/adopted.rs b/tests/adopted.rs index 10f05dcb..264d90ae 100644 --- a/tests/adopted.rs +++ b/tests/adopted.rs @@ -297,14 +297,14 @@ fn encode_all( let target = encoder.input_image(); let future = match handoff { Handoff::Cpu => { - converter.convert(src.image, vk::ImageLayout::SHADER_READ_ONLY_OPTIMAL, target)?; + converter.convert(src.image, vk::ImageLayout::GENERAL, target)?; encoder.encode(target)? } Handoff::Gpu => { let ready = TimelinePoint::new(rendered, u64::from(n) + 1); let converted = converter.convert_async( src.image, - vk::ImageLayout::SHADER_READ_ONLY_OPTIMAL, + vk::ImageLayout::GENERAL, target, &[ready], )?; 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..dfb7731c 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, and the one the encoder's own copy expects. Starting it +/// anywhere else means a second conversion told the wrong layout. pub unsafe fn create_src_image( context: &VideoContext, width: u32, @@ -100,7 +105,13 @@ pub unsafe fn create_src_image( .array_layers(1) .samples(vk::SampleCountFlags::TYPE_1) .tiling(vk::ImageTiling::OPTIMAL) - .usage(vk::ImageUsageFlags::SAMPLED | vk::ImageUsageFlags::TRANSFER_DST) + // Sampled by the converter, and copied from by an encoder taking RGB + // input, which reads its source with a transfer. + .usage( + vk::ImageUsageFlags::SAMPLED + | vk::ImageUsageFlags::TRANSFER_DST + | vk::ImageUsageFlags::TRANSFER_SRC, + ) .sharing_mode(vk::SharingMode::EXCLUSIVE) .initial_layout(vk::ImageLayout::UNDEFINED); let image = unsafe { device.create_image(&info, None) }?; @@ -179,13 +190,13 @@ 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) .subresource_range(range) .src_access_mask(vk::AccessFlags::TRANSFER_WRITE) - .dst_access_mask(vk::AccessFlags::SHADER_READ); + .dst_access_mask(vk::AccessFlags::SHADER_READ | vk::AccessFlags::TRANSFER_READ); device.cmd_pipeline_barrier( cb, vk::PipelineStageFlags::TRANSFER, diff --git a/tests/rgb_encode.rs b/tests/rgb_encode.rs index 37c81b6b..49318576 100644 --- a/tests/rgb_encode.rs +++ b/tests/rgb_encode.rs @@ -11,7 +11,7 @@ #[allow(dead_code)] mod common; -use common::source::{SrcImage, create_src_image, one_shot}; +use common::source::{SrcImage, create_src_image}; use common::{Readback, decode_stream}; use ash::vk; @@ -60,39 +60,10 @@ fn moving_frame(n: u32) -> Vec { data } -/// Upload frame `n` and leave it in `GENERAL`, which is where -/// [`Encoder::encode`] expects a source it copies from. +/// Upload frame `n`. It is left in `GENERAL`, which is where [`Encoder::encode`] +/// expects a source it copies from. fn source(context: &VideoContext, n: u32) -> Result> { - let src = unsafe { create_src_image(context, WIDTH, HEIGHT, &moving_frame(n))? }; - unsafe { - one_shot(context, |cb| { - let barrier = vk::ImageMemoryBarrier::default() - .old_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(src.image) - .subresource_range(vk::ImageSubresourceRange { - aspect_mask: vk::ImageAspectFlags::COLOR, - base_mip_level: 0, - level_count: 1, - base_array_layer: 0, - layer_count: 1, - }) - .src_access_mask(vk::AccessFlags::SHADER_READ) - .dst_access_mask(vk::AccessFlags::TRANSFER_READ); - context.device().cmd_pipeline_barrier( - cb, - vk::PipelineStageFlags::ALL_COMMANDS, - vk::PipelineStageFlags::ALL_COMMANDS, - vk::DependencyFlags::empty(), - &[], - &[], - &[barrier], - ); - })?; - } - Ok(src) + Ok(unsafe { create_src_image(context, WIDTH, HEIGHT, &moving_frame(n))? }) } fn encode_config(codec: Codec) -> EncodeConfig { From e22c3f297180364dd9a3516e8279577c972237e2 Mon Sep 17 00:00:00 2001 From: DatCaptainHorse Date: Wed, 23 Sep 2026 09:39:44 +0300 Subject: [PATCH 13/16] fix(encoder): refuse full-range RGB input, which RADV writes as limited RADV reports both full and limited range as supported for VK_VALVE_video_encode_rgb_conversion, and writes limited range whichever it is asked for. On an RX 9060 XT with Mesa 26.3, a flat grey of 51 decodes to 59 under either setting: 16 + 51 * 219 / 255, limited range. A full-range stream through this path therefore carries limited-range samples under a full-range label, and every player shows it washed out. Mesa passes the setting to the firmware as given, so the loss is below it. The RGB test compared the hardware path with the shader, both at limited range, which is why it could not see this. It now also measures the levels absolutely, on a flat grey, and checks that full range is never offered. ColorConverterConfig::rgb_encode_input no longer offers a full-range conversion, and Encoder::new refuses RGB input with a full-range description, rather than produce a stream whose label and samples disagree. Lifting that is one condition, once a driver honours it. Co-Authored-By: Claude Opus 5.5 --- src/converter/mod.rs | 11 ++++++++- src/encoder/codec.rs | 7 ++++++ src/encoder/mod.rs | 5 ++++ tests/rgb_encode.rs | 56 ++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 78 insertions(+), 1 deletion(-) diff --git a/src/converter/mod.rs b/src/converter/mod.rs index 6959a9a0..8e4cf019 100644 --- a/src/converter/mod.rs +++ b/src/converter/mod.rs @@ -337,12 +337,21 @@ impl ColorConverterConfig { /// and this format, and with [`Self::color_description`], then hand it the /// RGB images directly and skip the converter. /// + /// Limited range only. RADV, the one driver with the extension so far, + /// reports full range as supported and writes limited range regardless, + /// so a full-range stream would carry limited-range samples and every + /// player would show it washed out. See + /// [`EncodeConfig::with_rgb_input`](crate::EncodeConfig::with_rgb_input). + /// /// Whether the driver accepts the format for a particular codec and /// profile is only known when the encoder is created, so if /// [`Encoder::new`](crate::Encoder::new) refuses it, fall back to the /// converter. pub fn rgb_encode_input(&self, context: &VideoContext) -> Option { - if !context.has_video_encode_rgb_conversion() || self.source != self.target { + if !context.has_video_encode_rgb_conversion() + || self.source != self.target + || self.range.is_full() + { return None; } let matches = match self.output_format { diff --git a/src/encoder/codec.rs b/src/encoder/codec.rs index fce375be..15aeaf81 100644 --- a/src/encoder/codec.rs +++ b/src/encoder/codec.rs @@ -663,6 +663,13 @@ pub(crate) fn build_encoder_common(req: &CommonInitRequest) -> Result { + return Err(PixelForgeError::InvalidInput( + "RGB input is limited range only: the driver writes limited range whatever \ + it is asked, so a full-range stream would be mislabelled" + .to_string(), + )); + } Some(_) => { let caps = req.rgb_caps.ok_or_else(|| { PixelForgeError::NoSuitableDevice( diff --git a/src/encoder/mod.rs b/src/encoder/mod.rs index 9add9c65..4316abba 100644 --- a/src/encoder/mod.rs +++ b/src/encoder/mod.rs @@ -537,6 +537,11 @@ impl EncodeConfig { /// [`ColorConverterConfig::rgb_encode_input`](crate::ColorConverterConfig::rgb_encode_input) /// decides for you. /// + /// Limited range only: [`Encoder::new`] refuses a full-range description. + /// RADV reports full range as supported and writes limited range anyway, + /// measured on an RX 9060 XT with Mesa 26.3, so a full-range stream would + /// carry limited-range samples under a full-range label. + /// /// With this set, [`Encoder::input_image`] is an RGB image, and /// [`Encoder::encode`] copies an RGB source into it. pub fn with_rgb_input(mut self, format: crate::converter::InputFormat) -> Self { diff --git a/tests/rgb_encode.rs b/tests/rgb_encode.rs index 49318576..94aa9eaf 100644 --- a/tests/rgb_encode.rs +++ b/tests/rgb_encode.rs @@ -197,3 +197,59 @@ fn rgb_input_encodes_every_codec() -> Result<(), Box> { } Ok(()) } + +/// The hardware conversion's levels, measured absolutely rather than against +/// the shader. +/// +/// Comparing the two paths cannot catch a range error they share, and a range +/// error is exactly what a driver ignoring its range setting produces: a flat +/// grey that should decode to one code value decodes to another, and the +/// picture as a whole comes out washed out or crushed. +#[test] +#[ignore = "requires a Vulkan Video device with VK_VALVE_video_encode_rgb_conversion"] +fn rgb_input_writes_limited_range_levels() -> Result<(), Box> { + let context = context(Codec::H264)?; + let Some(format) = rgb_format(&context, "rgb_input_writes_limited_range_levels") else { + return Ok(()); + }; + let mut encoder = Encoder::new( + context.clone(), + encode_config(Codec::H264).with_rgb_input(format), + )?; + let grey: Vec = (0..WIDTH * HEIGHT) + .flat_map(|_| [51u8, 51, 51, 255]) + .collect(); + let src = unsafe { create_src_image(&context, WIDTH, HEIGHT, &grey)? }; + let mut stream = Vec::new(); + for _ in 0..FRAMES { + stream.extend_from_slice(&pollster::block_on(encoder.encode(src.image)?)?.data); + } + let frames = decode_all(&context, &stream)?; + let last = frames.last().ok_or("no frames decoded")?; + let mean = last.iter().map(|&v| f64::from(v)).sum::() / last.len() as f64; + // Limited range maps 0..255 onto 16..235: 16 + 51 * 219 / 255. + let expected = 16.0 + 51.0 * 219.0 / 255.0; + assert!( + (mean - expected).abs() <= 1.0, + "flat grey 51 decoded to luma {mean:.1}, limited range puts it at {expected:.1}" + ); + Ok(()) +} + +/// A full-range conversion is never handed to the hardware, since the one +/// driver that offers the path writes limited range regardless. +#[test] +#[ignore = "requires a Vulkan Video device"] +fn full_range_is_never_offered_to_the_hardware() -> Result<(), Box> { + let context = context(Codec::H264)?; + let mut config = converter_config(); + config.range = ColorRange::Full; + assert_eq!(config.rgb_encode_input(&context), None); + let full = encode_config(Codec::H264) + .with_color_description(config.color_description().unwrap()) + .with_rgb_input(InputFormat::BGRA); + if context.has_video_encode_rgb_conversion() { + assert!(Encoder::new(context.clone(), full).is_err()); + } + Ok(()) +} From aca4748eb287cb25c23a2dc0c8b7a676f47b24c9 Mon Sep 17 00:00:00 2001 From: DatCaptainHorse Date: Wed, 23 Sep 2026 09:39:59 +0300 Subject: [PATCH 14/16] docs: say how the tests show validation, and what the RGB path draws AGENTS.md now says that integration tests need common::init_logging() for their validation messages to go anywhere, and which two messages the RGB input path draws because the validation layer does not model its extension. The CHANGELOG entry for RGB input says it is limited range only, and why. Co-Authored-By: Claude Opus 5.5 --- AGENTS.md | 20 +++++++++++++++----- CHANGELOG.md | 4 +++- 2 files changed, 18 insertions(+), 6 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index a76b63a7..f570947b 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -119,15 +119,25 @@ run it after touching submission or barrier code. The RGB input path (`EncodeConfig::with_rgb_input`) needs `VK_VALVE_video_encode_rgb_conversion`, which today only RADV has. -`tests/rgb_encode.rs` compares it against the colour converter and skips on -other devices, so a green run elsewhere says nothing about it. +`tests/rgb_encode.rs` compares it against the colour converter, checks its +levels absolutely, and skips on other devices, so a green run elsewhere says +nothing about it. Under validation it draws two messages that are the layer's, +not pixelforge's: the layer does not model that extension, so it rejects the +RGB profile struct in a query pool's profile chain (the spec requires that +chain to match the session's) and checks the RGB input view against the plain +format features rather than the video profile's. 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 diff --git a/CHANGELOG.md b/CHANGELOG.md index 5179257d..1cb4656e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -122,7 +122,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - `EncodeConfig::with_rgb_input` hands RGB frames to the encoder and lets it apply the YUV matrix itself, through `VK_VALVE_video_encode_rgb_conversion`. `ColorConverterConfig::rgb_encode_input` says when a conversion can be done - that way instead of by the shader. Based on the approach in #14. + that way instead of by the shader. Limited range only: RADV, the one driver + with the extension so far, writes limited range even when asked for full. + Based on the approach in #14. ### Changed From 6f824aff117d6d238a9e3d9db767009014e9237b Mon Sep 17 00:00:00 2001 From: DatCaptainHorse Date: Wed, 23 Sep 2026 23:24:29 +0300 Subject: [PATCH 15/16] 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 destination now includes MEMORY_READ | MEMORY_WRITE. The barrier uses the original barrier API, which has no video-specific access flags, and those two are what cover the encode's accesses. Under synchronization validation no encode hazard remains, on either card. Co-Authored-By: Claude Opus 5.5 --- src/encoder/resources.rs | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/encoder/resources.rs b/src/encoder/resources.rs index 339fef54..545d7109 100644 --- a/src/encoder/resources.rs +++ b/src/encoder/resources.rs @@ -794,7 +794,11 @@ pub(crate) unsafe fn record_dpb_barriers( layer_count: 1, }) .src_access_mask(vk::AccessFlags::empty()) - .dst_access_mask(vk::AccessFlags::empty()); + // The encode writes the reconstructed picture into this slot, so it has + // to be in the barrier's second scope, or the layout transition and that + // write are unordered. The original barrier API has no video access + // flags; MEMORY_* covers the encode's reads and writes. + .dst_access_mask(vk::AccessFlags::MEMORY_READ | vk::AccessFlags::MEMORY_WRITE); let mut all_barriers = vec![dpb_barrier]; From f4b78395e3c5b76ab50bcb0dea6e5c2192c013da Mon Sep 17 00:00:00 2001 From: DatCaptainHorse Date: Wed, 23 Sep 2026 23:27:46 +0300 Subject: [PATCH 16/16] revert: allow full-range RGB input again This reverts e22c3f2, which refused full-range RGB input on the grounds that RADV writes limited range regardless of what it is asked. That conclusion came from one flat-grey measurement and does not match what full-range streams look like in practice, so the refusal goes until the behaviour is pinned down properly. The CHANGELOG and AGENTS.md lines that described the restriction and the level test go with it. Co-Authored-By: Claude Opus 5.5 --- AGENTS.md | 5 ++-- CHANGELOG.md | 4 +--- src/converter/mod.rs | 11 +-------- src/encoder/codec.rs | 7 ------ src/encoder/mod.rs | 5 ---- tests/rgb_encode.rs | 56 -------------------------------------------- 6 files changed, 4 insertions(+), 84 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index f570947b..976e96d7 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -119,9 +119,8 @@ run it after touching submission or barrier code. The RGB input path (`EncodeConfig::with_rgb_input`) needs `VK_VALVE_video_encode_rgb_conversion`, which today only RADV has. -`tests/rgb_encode.rs` compares it against the colour converter, checks its -levels absolutely, and skips on other devices, so a green run elsewhere says -nothing about it. Under validation it draws two messages that are the layer's, +`tests/rgb_encode.rs` compares it against the colour converter and skips on +other devices, so a green run elsewhere says nothing about it. Under validation it draws two messages that are the layer's, not pixelforge's: the layer does not model that extension, so it rejects the RGB profile struct in a query pool's profile chain (the spec requires that chain to match the session's) and checks the RGB input view against the plain diff --git a/CHANGELOG.md b/CHANGELOG.md index 1cb4656e..5179257d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -122,9 +122,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - `EncodeConfig::with_rgb_input` hands RGB frames to the encoder and lets it apply the YUV matrix itself, through `VK_VALVE_video_encode_rgb_conversion`. `ColorConverterConfig::rgb_encode_input` says when a conversion can be done - that way instead of by the shader. Limited range only: RADV, the one driver - with the extension so far, writes limited range even when asked for full. - Based on the approach in #14. + that way instead of by the shader. Based on the approach in #14. ### Changed diff --git a/src/converter/mod.rs b/src/converter/mod.rs index 8e4cf019..6959a9a0 100644 --- a/src/converter/mod.rs +++ b/src/converter/mod.rs @@ -337,21 +337,12 @@ impl ColorConverterConfig { /// and this format, and with [`Self::color_description`], then hand it the /// RGB images directly and skip the converter. /// - /// Limited range only. RADV, the one driver with the extension so far, - /// reports full range as supported and writes limited range regardless, - /// so a full-range stream would carry limited-range samples and every - /// player would show it washed out. See - /// [`EncodeConfig::with_rgb_input`](crate::EncodeConfig::with_rgb_input). - /// /// Whether the driver accepts the format for a particular codec and /// profile is only known when the encoder is created, so if /// [`Encoder::new`](crate::Encoder::new) refuses it, fall back to the /// converter. pub fn rgb_encode_input(&self, context: &VideoContext) -> Option { - if !context.has_video_encode_rgb_conversion() - || self.source != self.target - || self.range.is_full() - { + if !context.has_video_encode_rgb_conversion() || self.source != self.target { return None; } let matches = match self.output_format { diff --git a/src/encoder/codec.rs b/src/encoder/codec.rs index 15aeaf81..fce375be 100644 --- a/src/encoder/codec.rs +++ b/src/encoder/codec.rs @@ -663,13 +663,6 @@ pub(crate) fn build_encoder_common(req: &CommonInitRequest) -> Result { - return Err(PixelForgeError::InvalidInput( - "RGB input is limited range only: the driver writes limited range whatever \ - it is asked, so a full-range stream would be mislabelled" - .to_string(), - )); - } Some(_) => { let caps = req.rgb_caps.ok_or_else(|| { PixelForgeError::NoSuitableDevice( diff --git a/src/encoder/mod.rs b/src/encoder/mod.rs index 4316abba..9add9c65 100644 --- a/src/encoder/mod.rs +++ b/src/encoder/mod.rs @@ -537,11 +537,6 @@ impl EncodeConfig { /// [`ColorConverterConfig::rgb_encode_input`](crate::ColorConverterConfig::rgb_encode_input) /// decides for you. /// - /// Limited range only: [`Encoder::new`] refuses a full-range description. - /// RADV reports full range as supported and writes limited range anyway, - /// measured on an RX 9060 XT with Mesa 26.3, so a full-range stream would - /// carry limited-range samples under a full-range label. - /// /// With this set, [`Encoder::input_image`] is an RGB image, and /// [`Encoder::encode`] copies an RGB source into it. pub fn with_rgb_input(mut self, format: crate::converter::InputFormat) -> Self { diff --git a/tests/rgb_encode.rs b/tests/rgb_encode.rs index 94aa9eaf..49318576 100644 --- a/tests/rgb_encode.rs +++ b/tests/rgb_encode.rs @@ -197,59 +197,3 @@ fn rgb_input_encodes_every_codec() -> Result<(), Box> { } Ok(()) } - -/// The hardware conversion's levels, measured absolutely rather than against -/// the shader. -/// -/// Comparing the two paths cannot catch a range error they share, and a range -/// error is exactly what a driver ignoring its range setting produces: a flat -/// grey that should decode to one code value decodes to another, and the -/// picture as a whole comes out washed out or crushed. -#[test] -#[ignore = "requires a Vulkan Video device with VK_VALVE_video_encode_rgb_conversion"] -fn rgb_input_writes_limited_range_levels() -> Result<(), Box> { - let context = context(Codec::H264)?; - let Some(format) = rgb_format(&context, "rgb_input_writes_limited_range_levels") else { - return Ok(()); - }; - let mut encoder = Encoder::new( - context.clone(), - encode_config(Codec::H264).with_rgb_input(format), - )?; - let grey: Vec = (0..WIDTH * HEIGHT) - .flat_map(|_| [51u8, 51, 51, 255]) - .collect(); - let src = unsafe { create_src_image(&context, WIDTH, HEIGHT, &grey)? }; - let mut stream = Vec::new(); - for _ in 0..FRAMES { - stream.extend_from_slice(&pollster::block_on(encoder.encode(src.image)?)?.data); - } - let frames = decode_all(&context, &stream)?; - let last = frames.last().ok_or("no frames decoded")?; - let mean = last.iter().map(|&v| f64::from(v)).sum::() / last.len() as f64; - // Limited range maps 0..255 onto 16..235: 16 + 51 * 219 / 255. - let expected = 16.0 + 51.0 * 219.0 / 255.0; - assert!( - (mean - expected).abs() <= 1.0, - "flat grey 51 decoded to luma {mean:.1}, limited range puts it at {expected:.1}" - ); - Ok(()) -} - -/// A full-range conversion is never handed to the hardware, since the one -/// driver that offers the path writes limited range regardless. -#[test] -#[ignore = "requires a Vulkan Video device"] -fn full_range_is_never_offered_to_the_hardware() -> Result<(), Box> { - let context = context(Codec::H264)?; - let mut config = converter_config(); - config.range = ColorRange::Full; - assert_eq!(config.rgb_encode_input(&context), None); - let full = encode_config(Codec::H264) - .with_color_description(config.color_description().unwrap()) - .with_rgb_input(InputFormat::BGRA); - if context.has_video_encode_rgb_conversion() { - assert!(Encoder::new(context.clone(), full).is_err()); - } - Ok(()) -}