Skip to content

Commit 961d818

Browse files
reimareima
committed
[wgpu-core] Improve errors for forbidden texture copy formats
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.
1 parent 9a8dbfb commit 961d818

File tree

5 files changed

+276
-56
lines changed

5 files changed

+276
-56
lines changed

‎CHANGELOG.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -100,6 +100,7 @@ This allows using precompiled shaders without manually checking which backend's
100100
By @kpreid in [#8011](https://github.com/gfx-rs/wgpu/pull/8011).
101101
- 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).
102102
- 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).
103+
- The error message for non-copyable depth/stencil formats no longer mentions the aspect when it is not relevant. By @reima in [#XXXX](https://github.com/gfx-rs/wgpu/pull/XXXX).
103104

104105
#### Naga
105106

‎tests/tests/wgpu-validation/api/texture.rs

Lines changed: 210 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -298,3 +298,213 @@ fn planar_texture_bad_size() {
298298
);
299299
}
300300
}
301+
302+
/// Creates a texture and a buffer, and encodes a copy from the texture to the
303+
/// buffer.
304+
fn encode_copy_texture_to_buffer(
305+
device: &wgpu::Device,
306+
format: wgpu::TextureFormat,
307+
aspect: wgpu::TextureAspect,
308+
bytes_per_texel: u32,
309+
) -> wgpu::CommandEncoder {
310+
let size = wgpu::Extent3d {
311+
width: 256,
312+
height: 256,
313+
depth_or_array_layers: 1,
314+
};
315+
316+
let texture = device.create_texture(&wgpu::TextureDescriptor {
317+
label: None,
318+
size,
319+
mip_level_count: 1,
320+
sample_count: 1,
321+
dimension: wgpu::TextureDimension::D2,
322+
format,
323+
usage: wgpu::TextureUsages::COPY_SRC,
324+
view_formats: &[],
325+
});
326+
327+
let buffer = device.create_buffer(&wgpu::BufferDescriptor {
328+
label: None,
329+
size: (size.width * size.height * bytes_per_texel) as u64,
330+
usage: wgpu::BufferUsages::COPY_DST,
331+
mapped_at_creation: false,
332+
});
333+
334+
let mut encoder =
335+
device.create_command_encoder(&wgpu::CommandEncoderDescriptor { label: None });
336+
337+
encoder.copy_texture_to_buffer(
338+
wgpu::TexelCopyTextureInfo {
339+
texture: &texture,
340+
mip_level: 0,
341+
origin: wgpu::Origin3d::ZERO,
342+
aspect,
343+
},
344+
wgpu::TexelCopyBufferInfo {
345+
buffer: &buffer,
346+
layout: wgpu::TexelCopyBufferLayout {
347+
offset: 0,
348+
bytes_per_row: Some(size.width * bytes_per_texel),
349+
rows_per_image: None,
350+
},
351+
},
352+
size,
353+
);
354+
355+
encoder
356+
}
357+
358+
/// Ensures that attempting to copy a texture with a forbidden source format to
359+
/// a buffer fails validation.
360+
#[test]
361+
fn copy_texture_to_buffer_forbidden_format() {
362+
let (device, _queue) = wgpu::Device::noop(&wgpu::DeviceDescriptor::default());
363+
364+
let format = wgpu::TextureFormat::Depth24Plus;
365+
366+
let encoder = encode_copy_texture_to_buffer(&device, format, wgpu::TextureAspect::All, 4);
367+
368+
fail(
369+
&device,
370+
|| {
371+
encoder.finish();
372+
},
373+
Some(&format!(
374+
"Copying from textures with format {format:?} is forbidden"
375+
)),
376+
);
377+
}
378+
379+
/// Ensures that attempting ta copy a texture with a forbidden source
380+
/// format/aspect combination to a buffer fails validation.
381+
#[test]
382+
fn copy_texture_to_buffer_forbidden_format_aspect() {
383+
let (device, _queue) = wgpu::Device::noop(&wgpu::DeviceDescriptor::default());
384+
385+
let format = wgpu::TextureFormat::Depth24PlusStencil8;
386+
let aspect = wgpu::TextureAspect::DepthOnly;
387+
388+
let encoder = encode_copy_texture_to_buffer(&device, format, aspect, 4);
389+
390+
fail(
391+
&device,
392+
|| {
393+
encoder.finish();
394+
},
395+
Some(&format!(
396+
"Copying from textures with format {format:?} and aspect {aspect:?} is forbidden"
397+
)),
398+
);
399+
}
400+
401+
/// Creates a texture and a buffer, and encodes a copy from the buffer to the
402+
/// texture.
403+
fn encode_copy_buffer_to_texture(
404+
device: &wgpu::Device,
405+
format: wgpu::TextureFormat,
406+
aspect: wgpu::TextureAspect,
407+
bytes_per_texel: u32,
408+
) -> wgpu::CommandEncoder {
409+
let size = wgpu::Extent3d {
410+
width: 256,
411+
height: 256,
412+
depth_or_array_layers: 1,
413+
};
414+
415+
let texture = device.create_texture(&wgpu::TextureDescriptor {
416+
label: None,
417+
size,
418+
mip_level_count: 1,
419+
sample_count: 1,
420+
dimension: wgpu::TextureDimension::D2,
421+
format,
422+
usage: wgpu::TextureUsages::COPY_DST,
423+
view_formats: &[],
424+
});
425+
426+
let buffer = device.create_buffer(&wgpu::BufferDescriptor {
427+
label: None,
428+
size: (size.width * size.height * bytes_per_texel) as u64,
429+
usage: wgpu::BufferUsages::COPY_SRC,
430+
mapped_at_creation: false,
431+
});
432+
433+
let mut encoder =
434+
device.create_command_encoder(&wgpu::CommandEncoderDescriptor { label: None });
435+
436+
encoder.copy_buffer_to_texture(
437+
wgpu::TexelCopyBufferInfo {
438+
buffer: &buffer,
439+
layout: wgpu::TexelCopyBufferLayout {
440+
offset: 0,
441+
bytes_per_row: Some(size.width * bytes_per_texel),
442+
rows_per_image: None,
443+
},
444+
},
445+
wgpu::TexelCopyTextureInfo {
446+
texture: &texture,
447+
mip_level: 0,
448+
origin: wgpu::Origin3d::ZERO,
449+
aspect,
450+
},
451+
size,
452+
);
453+
454+
encoder
455+
}
456+
457+
/// Ensures that attempting to copy a buffer to a texture with a forbidden
458+
/// destination format fails validation.
459+
#[test]
460+
fn copy_buffer_to_texture_forbidden_format() {
461+
let (device, _queue) = wgpu::Device::noop(&wgpu::DeviceDescriptor::default());
462+
463+
for format in [
464+
wgpu::TextureFormat::Depth24Plus,
465+
wgpu::TextureFormat::Depth32Float,
466+
] {
467+
let encoder = encode_copy_buffer_to_texture(&device, format, wgpu::TextureAspect::All, 4);
468+
469+
fail(
470+
&device,
471+
|| {
472+
encoder.finish();
473+
},
474+
Some(&format!(
475+
"Copying to textures with format {format:?} is forbidden"
476+
)),
477+
);
478+
}
479+
}
480+
481+
/// Ensures that attempting to copy a buffer to a texture with a forbidden
482+
/// destination format/aspect combination fails validation.
483+
#[test]
484+
fn copy_buffer_to_texture_forbidden_format_aspect() {
485+
let required_features = wgpu::Features::DEPTH32FLOAT_STENCIL8;
486+
let device_desc = wgpu::DeviceDescriptor {
487+
required_features,
488+
..Default::default()
489+
};
490+
let (device, _queue) = wgpu::Device::noop(&device_desc);
491+
492+
let aspect = wgpu::TextureAspect::DepthOnly;
493+
494+
for (format, bytes_per_texel) in [
495+
(wgpu::TextureFormat::Depth24PlusStencil8, 4),
496+
(wgpu::TextureFormat::Depth32FloatStencil8, 8),
497+
] {
498+
let encoder = encode_copy_buffer_to_texture(&device, format, aspect, bytes_per_texel);
499+
500+
fail(
501+
&device,
502+
|| {
503+
encoder.finish();
504+
},
505+
Some(&format!(
506+
"Copying to textures with format {format:?} and aspect {aspect:?} is forbidden"
507+
)),
508+
);
509+
}
510+
}

‎wgpu-core/src/command/transfer.rs

Lines changed: 61 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,6 @@ use crate::device::trace::Command as TraceCommand;
1313
use crate::{
1414
api_log,
1515
command::{clear_texture, CommandEncoderError, EncoderStateError},
16-
conv,
1716
device::{Device, MissingDownlevelFlags},
1817
global::Global,
1918
id::{BufferId, CommandEncoderId, TextureId},
@@ -133,13 +132,17 @@ pub enum TransferError {
133132
CopyDstMissingAspects,
134133
#[error("Copy aspect must refer to a single aspect of texture format")]
135134
CopyAspectNotOne,
135+
#[error("Copying from textures with format {0:?} is forbidden")]
136+
CopyFromForbiddenTextureFormat(wgt::TextureFormat),
136137
#[error("Copying from textures with format {format:?} and aspect {aspect:?} is forbidden")]
137-
CopyFromForbiddenTextureFormat {
138+
CopyFromForbiddenTextureFormatAspect {
138139
format: wgt::TextureFormat,
139140
aspect: wgt::TextureAspect,
140141
},
142+
#[error("Copying to textures with format {0:?} is forbidden")]
143+
CopyToForbiddenTextureFormat(wgt::TextureFormat),
141144
#[error("Copying to textures with format {format:?} and aspect {aspect:?} is forbidden")]
142-
CopyToForbiddenTextureFormat {
145+
CopyToForbiddenTextureFormatAspect {
143146
format: wgt::TextureFormat,
144147
aspect: wgt::TextureAspect,
145148
},
@@ -200,8 +203,10 @@ impl WebGpuError for TransferError {
200203
| Self::CopySrcMissingAspects
201204
| Self::CopyDstMissingAspects
202205
| Self::CopyAspectNotOne
203-
| Self::CopyFromForbiddenTextureFormat { .. }
204-
| Self::CopyToForbiddenTextureFormat { .. }
206+
| Self::CopyFromForbiddenTextureFormat(..)
207+
| Self::CopyFromForbiddenTextureFormatAspect { .. }
208+
| Self::CopyToForbiddenTextureFormat(..)
209+
| Self::CopyToForbiddenTextureFormatAspect { .. }
205210
| Self::ExternalCopyToForbiddenTextureFormat(..)
206211
| Self::TextureFormatsNotCopyCompatible { .. }
207212
| Self::MissingDownlevelFlags(..)
@@ -364,6 +369,54 @@ pub(crate) fn validate_linear_texture_data(
364369
Ok((bytes_in_copy, image_stride_bytes))
365370
}
366371

372+
/// Validate the source format of a texture copy.
373+
///
374+
/// This performs the check from WebGPU's [validating texture buffer copy][vtbc]
375+
/// algorithm that ensures that the format and aspect form a valid texel copy source
376+
/// as defined in the [depth-stencil formats][dsf].
377+
///
378+
/// [vtbc]: https://gpuweb.github.io/gpuweb/#abstract-opdef-validating-texture-buffer-copy
379+
/// [dsf]: https://gpuweb.github.io/gpuweb/#depth-formats
380+
pub(crate) fn validate_texture_copy_src_format(
381+
format: wgt::TextureFormat,
382+
aspect: wgt::TextureAspect,
383+
) -> Result<(), TransferError> {
384+
use wgt::TextureAspect as Ta;
385+
use wgt::TextureFormat as Tf;
386+
match (format, aspect) {
387+
(Tf::Depth24Plus, _) => Err(TransferError::CopyFromForbiddenTextureFormat(format)),
388+
(Tf::Depth24PlusStencil8, Ta::DepthOnly) => {
389+
Err(TransferError::CopyFromForbiddenTextureFormatAspect { format, aspect })
390+
}
391+
_ => Ok(()),
392+
}
393+
}
394+
395+
/// Validate the destination format of a texture copy.
396+
///
397+
/// This performs the check from WebGPU's [validating texture buffer copy][vtbc]
398+
/// algorithm that ensures that the format and aspect form a valid texel copy destination
399+
/// as defined in the [depth-stencil formats][dsf].
400+
///
401+
/// [vtbc]: https://gpuweb.github.io/gpuweb/#abstract-opdef-validating-texture-buffer-copy
402+
/// [dsf]: https://gpuweb.github.io/gpuweb/#depth-formats
403+
pub(crate) fn validate_texture_copy_dst_format(
404+
format: wgt::TextureFormat,
405+
aspect: wgt::TextureAspect,
406+
) -> Result<(), TransferError> {
407+
use wgt::TextureAspect as Ta;
408+
use wgt::TextureFormat as Tf;
409+
match (format, aspect) {
410+
(Tf::Depth24Plus | Tf::Depth32Float, _) => {
411+
Err(TransferError::CopyToForbiddenTextureFormat(format))
412+
}
413+
(Tf::Depth24PlusStencil8 | Tf::Depth32FloatStencil8, Ta::DepthOnly) => {
414+
Err(TransferError::CopyToForbiddenTextureFormatAspect { format, aspect })
415+
}
416+
_ => Ok(()),
417+
}
418+
}
419+
367420
/// Validation for texture/buffer copies.
368421
///
369422
/// This implements the following checks from WebGPU's [validating texture buffer copy][vtbc]
@@ -376,7 +429,7 @@ pub(crate) fn validate_linear_texture_data(
376429
/// * Invocation of other validation algorithms.
377430
/// * The texture usage (COPY_DST / COPY_SRC) check.
378431
/// * The check for non-copyable depth/stencil formats. The caller must perform
379-
/// this check using `is_valid_copy_src_format` / `is_valid_copy_dst_format`
432+
/// this check using `validate_texture_copy_src_format` / `validate_texture_copy_dst_format`
380433
/// before calling this function. This function will panic if
381434
/// [`wgt::TextureFormat::block_copy_size`] returns `None` due to a
382435
/// non-copyable format.
@@ -945,14 +998,7 @@ impl Global {
945998
.map(|pending| pending.into_hal(dst_raw))
946999
.collect::<Vec<_>>();
9471000

948-
if !conv::is_valid_copy_dst_texture_format(dst_texture.desc.format, destination.aspect)
949-
{
950-
return Err(TransferError::CopyToForbiddenTextureFormat {
951-
format: dst_texture.desc.format,
952-
aspect: destination.aspect,
953-
}
954-
.into());
955-
}
1001+
validate_texture_copy_dst_format(dst_texture.desc.format, destination.aspect)?;
9561002

9571003
validate_texture_buffer_copy(
9581004
destination,
@@ -1073,13 +1119,7 @@ impl Global {
10731119
.into());
10741120
}
10751121

1076-
if !conv::is_valid_copy_src_texture_format(src_texture.desc.format, source.aspect) {
1077-
return Err(TransferError::CopyFromForbiddenTextureFormat {
1078-
format: src_texture.desc.format,
1079-
aspect: source.aspect,
1080-
}
1081-
.into());
1082-
}
1122+
validate_texture_copy_src_format(src_texture.desc.format, source.aspect)?;
10831123

10841124
validate_texture_buffer_copy(
10851125
source,

‎wgpu-core/src/conv.rs

Lines changed: 0 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -5,31 +5,6 @@ use crate::resource::{self, TextureDescriptor};
55
// Some core-only texture format helpers. The helper methods on `TextureFormat`
66
// defined in wgpu-types may need to be modified along with the ones here.
77

8-
pub fn is_valid_copy_src_texture_format(
9-
format: wgt::TextureFormat,
10-
aspect: wgt::TextureAspect,
11-
) -> bool {
12-
use wgt::TextureAspect as Ta;
13-
use wgt::TextureFormat as Tf;
14-
match (format, aspect) {
15-
(Tf::Depth24Plus, _) | (Tf::Depth24PlusStencil8, Ta::DepthOnly) => false,
16-
_ => true,
17-
}
18-
}
19-
20-
pub fn is_valid_copy_dst_texture_format(
21-
format: wgt::TextureFormat,
22-
aspect: wgt::TextureAspect,
23-
) -> bool {
24-
use wgt::TextureAspect as Ta;
25-
use wgt::TextureFormat as Tf;
26-
match (format, aspect) {
27-
(Tf::Depth24Plus | Tf::Depth32Float, _)
28-
| (Tf::Depth24PlusStencil8 | Tf::Depth32FloatStencil8, Ta::DepthOnly) => false,
29-
_ => true,
30-
}
31-
}
32-
338
#[cfg_attr(any(not(webgl)), expect(unused))]
349
pub fn is_valid_external_image_copy_dst_texture_format(format: wgt::TextureFormat) -> bool {
3510
use wgt::TextureFormat as Tf;

0 commit comments

Comments
 (0)