diff --git a/CHANGELOG.md b/CHANGELOG.md index 3493e84495..c9560fc4e3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -111,6 +111,7 @@ By @cwfitzgerald in [#8162](https://github.com/gfx-rs/wgpu/pull/8162). - Make a compacted hal acceleration structure inherit a label from the base BLAS. By @Vecvec in [#8103](https://github.com/gfx-rs/wgpu/pull/8103). - The limits requested for a device must now satisfy `min_subgroup_size <= max_subgroup_size`. By @andyleiserson in [#8085](https://github.com/gfx-rs/wgpu/pull/8085). - Require new `F16_IN_F32` downlevel flag for `quantizeToF16`, `pack2x16float`, and `unpack2x16float` in WGSL input. By @aleiserson in [#8130](https://github.com/gfx-rs/wgpu/pull/8130). +- The error message for non-copyable depth/stencil formats no longer mentions the aspect when it is not relevant. By @reima in [#8156](https://github.com/gfx-rs/wgpu/pull/8156). #### naga diff --git a/tests/tests/wgpu-validation/api/texture.rs b/tests/tests/wgpu-validation/api/texture.rs index 4b1e93ebb1..01c88c7bdf 100644 --- a/tests/tests/wgpu-validation/api/texture.rs +++ b/tests/tests/wgpu-validation/api/texture.rs @@ -298,3 +298,213 @@ fn planar_texture_bad_size() { ); } } + +/// Creates a texture and a buffer, and encodes a copy from the texture to the +/// buffer. +fn encode_copy_texture_to_buffer( + device: &wgpu::Device, + format: wgpu::TextureFormat, + aspect: wgpu::TextureAspect, + bytes_per_texel: u32, +) -> wgpu::CommandEncoder { + let size = wgpu::Extent3d { + width: 256, + height: 256, + depth_or_array_layers: 1, + }; + + let texture = device.create_texture(&wgpu::TextureDescriptor { + label: None, + size, + mip_level_count: 1, + sample_count: 1, + dimension: wgpu::TextureDimension::D2, + format, + usage: wgpu::TextureUsages::COPY_SRC, + view_formats: &[], + }); + + let buffer = device.create_buffer(&wgpu::BufferDescriptor { + label: None, + size: (size.width * size.height * bytes_per_texel) as u64, + usage: wgpu::BufferUsages::COPY_DST, + mapped_at_creation: false, + }); + + let mut encoder = + device.create_command_encoder(&wgpu::CommandEncoderDescriptor { label: None }); + + encoder.copy_texture_to_buffer( + wgpu::TexelCopyTextureInfo { + texture: &texture, + mip_level: 0, + origin: wgpu::Origin3d::ZERO, + aspect, + }, + wgpu::TexelCopyBufferInfo { + buffer: &buffer, + layout: wgpu::TexelCopyBufferLayout { + offset: 0, + bytes_per_row: Some(size.width * bytes_per_texel), + rows_per_image: None, + }, + }, + size, + ); + + encoder +} + +/// Ensures that attempting to copy a texture with a forbidden source format to +/// a buffer fails validation. +#[test] +fn copy_texture_to_buffer_forbidden_format() { + let (device, _queue) = wgpu::Device::noop(&wgpu::DeviceDescriptor::default()); + + let format = wgpu::TextureFormat::Depth24Plus; + + let encoder = encode_copy_texture_to_buffer(&device, format, wgpu::TextureAspect::All, 4); + + fail( + &device, + || { + encoder.finish(); + }, + Some(&format!( + "Copying from textures with format {format:?} is forbidden" + )), + ); +} + +/// Ensures that attempting ta copy a texture with a forbidden source +/// format/aspect combination to a buffer fails validation. +#[test] +fn copy_texture_to_buffer_forbidden_format_aspect() { + let (device, _queue) = wgpu::Device::noop(&wgpu::DeviceDescriptor::default()); + + let format = wgpu::TextureFormat::Depth24PlusStencil8; + let aspect = wgpu::TextureAspect::DepthOnly; + + let encoder = encode_copy_texture_to_buffer(&device, format, aspect, 4); + + fail( + &device, + || { + encoder.finish(); + }, + Some(&format!( + "Copying from textures with format {format:?} and aspect {aspect:?} is forbidden" + )), + ); +} + +/// Creates a texture and a buffer, and encodes a copy from the buffer to the +/// texture. +fn encode_copy_buffer_to_texture( + device: &wgpu::Device, + format: wgpu::TextureFormat, + aspect: wgpu::TextureAspect, + bytes_per_texel: u32, +) -> wgpu::CommandEncoder { + let size = wgpu::Extent3d { + width: 256, + height: 256, + depth_or_array_layers: 1, + }; + + let texture = device.create_texture(&wgpu::TextureDescriptor { + label: None, + size, + mip_level_count: 1, + sample_count: 1, + dimension: wgpu::TextureDimension::D2, + format, + usage: wgpu::TextureUsages::COPY_DST, + view_formats: &[], + }); + + let buffer = device.create_buffer(&wgpu::BufferDescriptor { + label: None, + size: (size.width * size.height * bytes_per_texel) as u64, + usage: wgpu::BufferUsages::COPY_SRC, + mapped_at_creation: false, + }); + + let mut encoder = + device.create_command_encoder(&wgpu::CommandEncoderDescriptor { label: None }); + + encoder.copy_buffer_to_texture( + wgpu::TexelCopyBufferInfo { + buffer: &buffer, + layout: wgpu::TexelCopyBufferLayout { + offset: 0, + bytes_per_row: Some(size.width * bytes_per_texel), + rows_per_image: None, + }, + }, + wgpu::TexelCopyTextureInfo { + texture: &texture, + mip_level: 0, + origin: wgpu::Origin3d::ZERO, + aspect, + }, + size, + ); + + encoder +} + +/// Ensures that attempting to copy a buffer to a texture with a forbidden +/// destination format fails validation. +#[test] +fn copy_buffer_to_texture_forbidden_format() { + let (device, _queue) = wgpu::Device::noop(&wgpu::DeviceDescriptor::default()); + + for format in [ + wgpu::TextureFormat::Depth24Plus, + wgpu::TextureFormat::Depth32Float, + ] { + let encoder = encode_copy_buffer_to_texture(&device, format, wgpu::TextureAspect::All, 4); + + fail( + &device, + || { + encoder.finish(); + }, + Some(&format!( + "Copying to textures with format {format:?} is forbidden" + )), + ); + } +} + +/// Ensures that attempting to copy a buffer to a texture with a forbidden +/// destination format/aspect combination fails validation. +#[test] +fn copy_buffer_to_texture_forbidden_format_aspect() { + let required_features = wgpu::Features::DEPTH32FLOAT_STENCIL8; + let device_desc = wgpu::DeviceDescriptor { + required_features, + ..Default::default() + }; + let (device, _queue) = wgpu::Device::noop(&device_desc); + + let aspect = wgpu::TextureAspect::DepthOnly; + + for (format, bytes_per_texel) in [ + (wgpu::TextureFormat::Depth24PlusStencil8, 4), + (wgpu::TextureFormat::Depth32FloatStencil8, 8), + ] { + let encoder = encode_copy_buffer_to_texture(&device, format, aspect, bytes_per_texel); + + fail( + &device, + || { + encoder.finish(); + }, + Some(&format!( + "Copying to textures with format {format:?} and aspect {aspect:?} is forbidden" + )), + ); + } +} diff --git a/wgpu-core/src/command/transfer.rs b/wgpu-core/src/command/transfer.rs index 3a818e148c..2115b23b75 100644 --- a/wgpu-core/src/command/transfer.rs +++ b/wgpu-core/src/command/transfer.rs @@ -13,7 +13,6 @@ use crate::device::trace::Command as TraceCommand; use crate::{ api_log, command::{clear_texture, CommandEncoderError, EncoderStateError}, - conv, device::{Device, MissingDownlevelFlags}, global::Global, id::{BufferId, CommandEncoderId, TextureId}, @@ -133,13 +132,17 @@ pub enum TransferError { CopyDstMissingAspects, #[error("Copy aspect must refer to a single aspect of texture format")] CopyAspectNotOne, + #[error("Copying from textures with format {0:?} is forbidden")] + CopyFromForbiddenTextureFormat(wgt::TextureFormat), #[error("Copying from textures with format {format:?} and aspect {aspect:?} is forbidden")] - CopyFromForbiddenTextureFormat { + CopyFromForbiddenTextureFormatAspect { format: wgt::TextureFormat, aspect: wgt::TextureAspect, }, + #[error("Copying to textures with format {0:?} is forbidden")] + CopyToForbiddenTextureFormat(wgt::TextureFormat), #[error("Copying to textures with format {format:?} and aspect {aspect:?} is forbidden")] - CopyToForbiddenTextureFormat { + CopyToForbiddenTextureFormatAspect { format: wgt::TextureFormat, aspect: wgt::TextureAspect, }, @@ -200,8 +203,10 @@ impl WebGpuError for TransferError { | Self::CopySrcMissingAspects | Self::CopyDstMissingAspects | Self::CopyAspectNotOne - | Self::CopyFromForbiddenTextureFormat { .. } - | Self::CopyToForbiddenTextureFormat { .. } + | Self::CopyFromForbiddenTextureFormat(..) + | Self::CopyFromForbiddenTextureFormatAspect { .. } + | Self::CopyToForbiddenTextureFormat(..) + | Self::CopyToForbiddenTextureFormatAspect { .. } | Self::ExternalCopyToForbiddenTextureFormat(..) | Self::TextureFormatsNotCopyCompatible { .. } | Self::MissingDownlevelFlags(..) @@ -364,6 +369,54 @@ pub(crate) fn validate_linear_texture_data( Ok((bytes_in_copy, image_stride_bytes)) } +/// Validate the source format of a texture copy. +/// +/// This performs the check from WebGPU's [validating texture buffer copy][vtbc] +/// algorithm that ensures that the format and aspect form a valid texel copy source +/// as defined in the [depth-stencil formats][dsf]. +/// +/// [vtbc]: https://gpuweb.github.io/gpuweb/#abstract-opdef-validating-texture-buffer-copy +/// [dsf]: https://gpuweb.github.io/gpuweb/#depth-formats +pub(crate) fn validate_texture_copy_src_format( + format: wgt::TextureFormat, + aspect: wgt::TextureAspect, +) -> Result<(), TransferError> { + use wgt::TextureAspect as Ta; + use wgt::TextureFormat as Tf; + match (format, aspect) { + (Tf::Depth24Plus, _) => Err(TransferError::CopyFromForbiddenTextureFormat(format)), + (Tf::Depth24PlusStencil8, Ta::DepthOnly) => { + Err(TransferError::CopyFromForbiddenTextureFormatAspect { format, aspect }) + } + _ => Ok(()), + } +} + +/// Validate the destination format of a texture copy. +/// +/// This performs the check from WebGPU's [validating texture buffer copy][vtbc] +/// algorithm that ensures that the format and aspect form a valid texel copy destination +/// as defined in the [depth-stencil formats][dsf]. +/// +/// [vtbc]: https://gpuweb.github.io/gpuweb/#abstract-opdef-validating-texture-buffer-copy +/// [dsf]: https://gpuweb.github.io/gpuweb/#depth-formats +pub(crate) fn validate_texture_copy_dst_format( + format: wgt::TextureFormat, + aspect: wgt::TextureAspect, +) -> Result<(), TransferError> { + use wgt::TextureAspect as Ta; + use wgt::TextureFormat as Tf; + match (format, aspect) { + (Tf::Depth24Plus | Tf::Depth32Float, _) => { + Err(TransferError::CopyToForbiddenTextureFormat(format)) + } + (Tf::Depth24PlusStencil8 | Tf::Depth32FloatStencil8, Ta::DepthOnly) => { + Err(TransferError::CopyToForbiddenTextureFormatAspect { format, aspect }) + } + _ => Ok(()), + } +} + /// Validation for texture/buffer copies. /// /// This implements the following checks from WebGPU's [validating texture buffer copy][vtbc] @@ -376,7 +429,7 @@ pub(crate) fn validate_linear_texture_data( /// * Invocation of other validation algorithms. /// * The texture usage (COPY_DST / COPY_SRC) check. /// * The check for non-copyable depth/stencil formats. The caller must perform -/// this check using `is_valid_copy_src_format` / `is_valid_copy_dst_format` +/// this check using `validate_texture_copy_src_format` / `validate_texture_copy_dst_format` /// before calling this function. This function will panic if /// [`wgt::TextureFormat::block_copy_size`] returns `None` due to a /// non-copyable format. @@ -945,14 +998,7 @@ impl Global { .map(|pending| pending.into_hal(dst_raw)) .collect::>(); - if !conv::is_valid_copy_dst_texture_format(dst_texture.desc.format, destination.aspect) - { - return Err(TransferError::CopyToForbiddenTextureFormat { - format: dst_texture.desc.format, - aspect: destination.aspect, - } - .into()); - } + validate_texture_copy_dst_format(dst_texture.desc.format, destination.aspect)?; validate_texture_buffer_copy( destination, @@ -1073,13 +1119,7 @@ impl Global { .into()); } - if !conv::is_valid_copy_src_texture_format(src_texture.desc.format, source.aspect) { - return Err(TransferError::CopyFromForbiddenTextureFormat { - format: src_texture.desc.format, - aspect: source.aspect, - } - .into()); - } + validate_texture_copy_src_format(src_texture.desc.format, source.aspect)?; validate_texture_buffer_copy( source, diff --git a/wgpu-core/src/conv.rs b/wgpu-core/src/conv.rs index aa6ceb046d..3aca805659 100644 --- a/wgpu-core/src/conv.rs +++ b/wgpu-core/src/conv.rs @@ -5,31 +5,6 @@ use crate::resource::{self, TextureDescriptor}; // Some core-only texture format helpers. The helper methods on `TextureFormat` // defined in wgpu-types may need to be modified along with the ones here. -pub fn is_valid_copy_src_texture_format( - format: wgt::TextureFormat, - aspect: wgt::TextureAspect, -) -> bool { - use wgt::TextureAspect as Ta; - use wgt::TextureFormat as Tf; - match (format, aspect) { - (Tf::Depth24Plus, _) | (Tf::Depth24PlusStencil8, Ta::DepthOnly) => false, - _ => true, - } -} - -pub fn is_valid_copy_dst_texture_format( - format: wgt::TextureFormat, - aspect: wgt::TextureAspect, -) -> bool { - use wgt::TextureAspect as Ta; - use wgt::TextureFormat as Tf; - match (format, aspect) { - (Tf::Depth24Plus | Tf::Depth32Float, _) - | (Tf::Depth24PlusStencil8 | Tf::Depth32FloatStencil8, Ta::DepthOnly) => false, - _ => true, - } -} - #[cfg_attr(any(not(webgl)), expect(unused))] pub fn is_valid_external_image_copy_dst_texture_format(format: wgt::TextureFormat) -> bool { use wgt::TextureFormat as Tf; diff --git a/wgpu-core/src/device/queue.rs b/wgpu-core/src/device/queue.rs index 63f9130b61..d9f338cf7c 100644 --- a/wgpu-core/src/device/queue.rs +++ b/wgpu-core/src/device/queue.rs @@ -20,10 +20,10 @@ use crate::{ api_log, command::{ extract_texture_selector, validate_linear_texture_data, validate_texture_buffer_copy, - validate_texture_copy_range, ClearError, CommandAllocator, CommandBuffer, CommandEncoder, - CommandEncoderError, CopySide, TexelCopyTextureInfo, TransferError, + validate_texture_copy_dst_format, validate_texture_copy_range, ClearError, + CommandAllocator, CommandBuffer, CommandEncoder, CommandEncoderError, CopySide, + TexelCopyTextureInfo, TransferError, }, - conv, device::{DeviceError, WaitIdleError}, get_lowest_common_denom, global::Global, @@ -757,13 +757,7 @@ impl Queue { let (selector, dst_base) = extract_texture_selector(&destination, size, &dst)?; - if !conv::is_valid_copy_dst_texture_format(dst.desc.format, destination.aspect) { - return Err(TransferError::CopyToForbiddenTextureFormat { - format: dst.desc.format, - aspect: destination.aspect, - } - .into()); - } + validate_texture_copy_dst_format(dst.desc.format, destination.aspect)?; validate_texture_buffer_copy( &destination, @@ -961,6 +955,8 @@ impl Queue { destination: wgt::CopyExternalImageDestInfo>, size: wgt::Extent3d, ) -> Result<(), QueueWriteError> { + use crate::conv; + profiling::scope!("Queue::copy_external_image_to_texture"); self.device.check_is_valid()?;