[wgpu-core] Improve errors for forbidden texture copy formats (#8156)

TransferError now has separate variants for texture copy formats that
are only forbidden in combination with specific aspects
(CopyFrom/ToForbiddenTextureFormatAspect), and texture copy formats that
are always forbidden, irrespective of the aspect
(CopyFrom/ToForbiddenTextureFormat).

This produces a less confusing error message by not mentioning the
aspect it is not relevant.

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
 

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"
+            )),
+        );
+    }
+}

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::<Vec<_>>();
 
-            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,

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;

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<Fallible<Texture>>,
         size: wgt::Extent3d,
     ) -> Result<(), QueueWriteError> {
+        use crate::conv;
+
         profiling::scope!("Queue::copy_external_image_to_texture");
 
         self.device.check_is_valid()?;