Skip to content

Make Queue::write_texture work Correctly on DX12 + Add new test write_texture_subset_2d_mips #3992

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 2 commits into from
Closed

Make Queue::write_texture work Correctly on DX12 + Add new test write_texture_subset_2d_mips #3992

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
995aaf7
Fixed Queue::write_texture on dx12 + refactored and added new test to…
39ali Aug 26, 2023
ff78ba1
fix clippy warnings
39ali Aug 26, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,7 @@ By @Valaphee in [#3402](https://github.com/gfx-rs/wgpu/pull/3402)

- DX12 doesn't support `Features::POLYGON_MODE_POINT``. By @teoxoy in [#4032](https://github.com/gfx-rs/wgpu/pull/4032).
- Set `Features::VERTEX_WRITABLE_STORAGE` based on the right feature level. By @teoxoy in [#4033](https://github.com/gfx-rs/wgpu/pull/4033).

- Fix Queue::write_texture failing on the DX12 backend and added a new test to test writing/copying from different mipmap levels. By @39ali in [#3992](https://github.com/gfx-rs/wgpu/pull/3992)
### Documentation

- Add an overview of `RenderPass` and how render state works. By @kpreid in [#4055](https://github.com/gfx-rs/wgpu/pull/4055)
Expand Down
286 changes: 165 additions & 121 deletions tests/tests/write_texture.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,55 +4,91 @@ use wgpu_test::{initialize_test, TestParameters};

use wasm_bindgen_test::*;

#[test]
#[wasm_bindgen_test]
fn write_texture_subset_2d() {
let size = 256;
let parameters = TestParameters::default().backend_failure(wgpu::Backends::DX12);
struct LayoutDesc {
bytes_per_row: u32,
rows_per_image: u32,
total_size_in_bytes: u32,
}

fn total_bytes_in_copy(
texture_format: wgpu::TextureFormat,
bytes_per_row: u32,
rows_per_image: u32,
copy_extent: wgpu::Extent3d,
) -> u32 {
let block_size = texture_format.block_size(None).unwrap_or(1);

let block_dim = texture_format.block_dimensions();

let block_width = copy_extent.width / block_dim.0;
let block_height = copy_extent.height / block_dim.1;

let bytes_per_image = bytes_per_row * rows_per_image;
let mut total_bytes = bytes_per_image * (copy_extent.depth_or_array_layers - 1);

if block_height != 0 {
let last_row_bytes = block_width * block_size;
let last_image_bytes = bytes_per_row * (block_height - 1) + last_row_bytes;
total_bytes += last_image_bytes;
}

total_bytes
}

fn test(
format: wgpu::TextureFormat,
layout_desc: LayoutDesc,
mip_level: u32,
mips_count: u32,
tex_size: wgpu::Extent3d,
write_size: wgpu::Extent3d,
copy_size: wgpu::Extent3d,
) {
let LayoutDesc {
bytes_per_row,
total_size_in_bytes,
rows_per_image,
} = layout_desc;
let parameters = TestParameters::default();
initialize_test(parameters, |ctx| {
let tex = ctx.device.create_texture(&wgpu::TextureDescriptor {
label: None,
dimension: wgpu::TextureDimension::D2,
size: wgpu::Extent3d {
width: size,
height: size,
depth_or_array_layers: 1,
},
format: wgpu::TextureFormat::R8Uint,
size: tex_size,
format,
usage: wgpu::TextureUsages::COPY_DST
| wgpu::TextureUsages::COPY_SRC
| wgpu::TextureUsages::TEXTURE_BINDING,
mip_level_count: 1,
mip_level_count: mips_count,
sample_count: 1,
view_formats: &[],
});
let data = vec![1u8; size as usize * 2];

let val = (mip_level + 1) as u8;
let data = vec![val; total_size_in_bytes as usize];

// Write the first two rows
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment is outdated.

ctx.queue.write_texture(
wgpu::ImageCopyTexture {
texture: &tex,
mip_level: 0,
mip_level,
origin: wgpu::Origin3d::ZERO,
aspect: wgpu::TextureAspect::All,
},
bytemuck::cast_slice(&data),
wgpu::ImageDataLayout {
offset: 0,
bytes_per_row: Some(size),
rows_per_image: Some(size),
},
wgpu::Extent3d {
width: size,
height: 2,
depth_or_array_layers: 1,
bytes_per_row: Some(bytes_per_row),
rows_per_image: Some(rows_per_image),
},
write_size,
);

ctx.queue.submit(None);

let read_buffer = ctx.device.create_buffer(&wgpu::BufferDescriptor {
label: None,
size: (size * size) as u64,
size: (total_size_in_bytes) as u64,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be large enough to copy back the whole texture.

usage: wgpu::BufferUsages::MAP_READ | wgpu::BufferUsages::COPY_DST,
mapped_at_creation: false,
});
Expand All @@ -64,23 +100,19 @@ fn write_texture_subset_2d() {
encoder.copy_texture_to_buffer(
wgpu::ImageCopyTexture {
texture: &tex,
mip_level: 0,
mip_level,
origin: wgpu::Origin3d::ZERO,
aspect: wgpu::TextureAspect::All,
},
wgpu::ImageCopyBuffer {
buffer: &read_buffer,
layout: wgpu::ImageDataLayout {
offset: 0,
bytes_per_row: Some(size),
rows_per_image: Some(size),
bytes_per_row: Some(bytes_per_row),
rows_per_image: Some(rows_per_image),
},
},
wgpu::Extent3d {
width: size,
height: size,
depth_or_array_layers: 1,
},
copy_size,
);

ctx.queue.submit(Some(encoder.finish()));
Expand All @@ -90,107 +122,119 @@ fn write_texture_subset_2d() {
ctx.device.poll(wgpu::Maintain::Wait);
let data: Vec<u8> = slice.get_mapped_range().to_vec();

for byte in &data[..(size as usize * 2)] {
assert_eq!(*byte, 1);
for byte in &data[..(total_size_in_bytes as usize)] {
assert_eq!(*byte, val);
}
for byte in &data[(size as usize * 2)..] {
for byte in &data[(total_size_in_bytes as usize)..] {
assert_eq!(*byte, 0);
}
});
}

#[test]
#[wasm_bindgen_test]
fn write_texture_subset_3d() {
let size = 256;
let depth = 4;
let parameters = TestParameters::default();
initialize_test(parameters, |ctx| {
let tex = ctx.device.create_texture(&wgpu::TextureDescriptor {
label: None,
dimension: wgpu::TextureDimension::D3,
size: wgpu::Extent3d {
width: size,
height: size,
depth_or_array_layers: depth,
},
format: wgpu::TextureFormat::R8Uint,
usage: wgpu::TextureUsages::COPY_DST
| wgpu::TextureUsages::COPY_SRC
| wgpu::TextureUsages::TEXTURE_BINDING,
mip_level_count: 1,
sample_count: 1,
view_formats: &[],
});
let data = vec![1u8; (size * size) as usize * 2];
// Write the first two slices
ctx.queue.write_texture(
wgpu::ImageCopyTexture {
texture: &tex,
mip_level: 0,
origin: wgpu::Origin3d::ZERO,
aspect: wgpu::TextureAspect::All,
},
bytemuck::cast_slice(&data),
wgpu::ImageDataLayout {
offset: 0,
bytes_per_row: Some(size),
rows_per_image: Some(size),
},
wgpu::Extent3d {
width: size,
height: size,
depth_or_array_layers: 2,
},
);

ctx.queue.submit(None);

let read_buffer = ctx.device.create_buffer(&wgpu::BufferDescriptor {
label: None,
size: (size * size * depth) as u64,
usage: wgpu::BufferUsages::MAP_READ | wgpu::BufferUsages::COPY_DST,
mapped_at_creation: false,
});

let mut encoder = ctx
.device
.create_command_encoder(&wgpu::CommandEncoderDescriptor { label: None });
fn write_texture_subset_2d() {
let format = wgpu::TextureFormat::R8Uint;
let mips_count = 1;
let tex_size = wgpu::Extent3d {
width: 256,
height: 256,
depth_or_array_layers: 1,
};

let bytes_per_row = tex_size.width;
let rows_per_image = tex_size.height;

let total_bytes_in_copy = total_bytes_in_copy(format, bytes_per_row, rows_per_image, tex_size);

test(
format,
LayoutDesc {
bytes_per_row,
rows_per_image,
total_size_in_bytes: total_bytes_in_copy,
},
0,
mips_count,
tex_size,
tex_size,
tex_size,
Comment on lines +159 to +161
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test seems to now copy the whole texture instead of a subset (prev. height: 2).

);
}

encoder.copy_texture_to_buffer(
wgpu::ImageCopyTexture {
texture: &tex,
mip_level: 0,
origin: wgpu::Origin3d::ZERO,
aspect: wgpu::TextureAspect::All,
},
wgpu::ImageCopyBuffer {
buffer: &read_buffer,
layout: wgpu::ImageDataLayout {
offset: 0,
bytes_per_row: Some(size),
rows_per_image: Some(size),
},
},
wgpu::Extent3d {
width: size,
height: size,
depth_or_array_layers: depth,
#[test]
#[wasm_bindgen_test]
fn write_texture_subset_2d_mips() {
let format = wgpu::TextureFormat::R8Uint;
let mips_count = 3;
let tex_size = wgpu::Extent3d {
width: 2048,
height: 2048,
depth_or_array_layers: 1,
};
for mip_level in 0..mips_count {
let mip_w = tex_size.width / (1 << mip_level);
let mip_h = tex_size.height / (1 << mip_level);
let bytes_per_row = mip_w;
let rows_per_image = mip_h;

let mip_extent = wgpu::Extent3d {
width: mip_w,
height: 2,
depth_or_array_layers: tex_size.depth_or_array_layers,
};

let total_bytes_in_copy =
total_bytes_in_copy(format, bytes_per_row, rows_per_image, mip_extent);

test(
format,
LayoutDesc {
bytes_per_row,
rows_per_image,
total_size_in_bytes: total_bytes_in_copy,
},
mip_level,
mips_count,
tex_size,
mip_extent,
mip_extent,
);
}
}

ctx.queue.submit(Some(encoder.finish()));

let slice = read_buffer.slice(..);
slice.map_async(wgpu::MapMode::Read, |_| ());
ctx.device.poll(wgpu::Maintain::Wait);
let data: Vec<u8> = slice.get_mapped_range().to_vec();

for byte in &data[..((size * size) as usize * 2)] {
assert_eq!(*byte, 1);
}
for byte in &data[((size * size) as usize * 2)..] {
assert_eq!(*byte, 0);
}
});
#[test]
#[wasm_bindgen_test]
fn write_texture_subset_3d() {
let format = wgpu::TextureFormat::R8Uint;
let mips_count = 1;
let tex_size = wgpu::Extent3d {
width: 256,
height: 256,
depth_or_array_layers: 4,
};
let copy_size = wgpu::Extent3d {
width: 256,
height: 256,
depth_or_array_layers: 2,
};

let bytes_per_row = tex_size.width;
let rows_per_image = tex_size.height;

let total_bytes_in_copy = total_bytes_in_copy(format, bytes_per_row, rows_per_image, copy_size);

test(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The dimension is hardcoded to be wgpu::TextureDimension::D2 but should be 3d for this test.

format,
LayoutDesc {
bytes_per_row,
rows_per_image,
total_size_in_bytes: total_bytes_in_copy,
},
0,
mips_count,
tex_size,
copy_size,
copy_size,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
copy_size,
tex_size,

We should copy back the whole texture.

);
}
7 changes: 2 additions & 5 deletions wgpu-hal/src/dx12/command.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ impl crate::BufferTextureCopy {
&self,
format: wgt::TextureFormat,
) -> d3d12_ty::D3D12_PLACED_SUBRESOURCE_FOOTPRINT {
let (block_width, block_height) = format.block_dimensions();
let block_width = format.block_dimensions().0;
d3d12_ty::D3D12_PLACED_SUBRESOURCE_FOOTPRINT {
Offset: self.buffer_layout.offset,
Footprint: d3d12_ty::D3D12_SUBRESOURCE_FOOTPRINT {
Expand All @@ -30,10 +30,7 @@ impl crate::BufferTextureCopy {
)
.unwrap(),
Width: self.size.width,
Height: self
.buffer_layout
.rows_per_image
.map_or(self.size.height, |count| count * block_height),
Height: self.size.height,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks correct but I'm not sure what happens if you copy a 3D texture with a non-default rows_per_image.
We have RowPitch but no ColPitch.
Vulkan has bufferImageHeight and Metal has sourceBytesPerImage.

@cwfitzgerald any ideas?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dawn appears to issue one copy per layer, though I'm not 100% sure on this, as dawn has a lot of layers to their d3d12 copy code.

It seems like we should issue one copy if it's dense, otherwise one copy per subresource if it's not

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's my conclusion as well, Vulkan and Metal support specifying rows_per_image for 3D textures but D3D12 doesn't.
So, for a non-default rows_per_image we should set depth: 1 and issue multiple copies by advancing D3D12_PLACED_SUBRESOURCE_FOOTPRINT.Offset by rows_per_image * depth_index.

This is a pre-existing issue, will open an issue to track it.

Depth: self.size.depth,
RowPitch: {
let actual = self.buffer_layout.bytes_per_row.unwrap_or_else(|| {
Expand Down