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

39ali · 2023-07-30T00:39:03Z

Checklist

Run cargo clippy.
Run cargo clippy --target wasm32-unknown-unknown if applicable.
Add change to CHANGELOG.md. See simple instructions inside file.

Connections
fixes #3072

Description
Fixes Queue::write_texture failing on the DX12 backend and added a new test to test writing/copying from different mipmap levels .
Testing
Run tests for write_texture.rs

cwfitzgerald

Great stuff, one minor nit

wgpu-hal/src/dx12/command.rs

cwfitzgerald

Nice!

cwfitzgerald · 2023-08-01T03:49:13Z

Seems to not pass tests

teoxoy · 2023-08-02T11:29:32Z

I haven't looked at the details but we are now ignoring

self.buffer_layout.rows_per_image
self.buffer_layout.bytes_per_row
self.texture_base.aspect

39ali · 2023-08-02T13:44:07Z

@teoxoy self.texture_base.aspect is now being used , correct me if I'm wrong but I believe rows_per_image and bytes_per_row are redundant since they're being calculated by GetCopyableFootprints.

teoxoy · 2023-08-02T16:36:25Z

rows_per_image and bytes_per_row are passed by the user and they can be higher than the minimum required to perform the copy, so we can't assume they will be the minimum.

Related: https://gpuweb.github.io/gpuweb/#abstract-opdef-validating-linear-texture-data

teoxoy · 2023-08-02T16:39:20Z

I doubt we can use GetCopyableFootprints as it seems to calculate the minimums.

But maybe we can use it to figure out why our calculations were wrong previously by comparing their outputs.

stale

39ali · 2023-08-02T18:32:05Z

@teoxoy looks like it, I'll look into it.

… write_texture.rs

39ali · 2023-08-26T20:34:42Z

ran all tests and it's working as expected

teoxoy · 2023-09-01T15:54:17Z

tests/tests/write_texture.rs

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


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

teoxoy · 2023-09-01T15:58:08Z

tests/tests/write_texture.rs

+        mips_count,
+        tex_size,
+        copy_size,
+        copy_size,


Suggested change

copy_size,

tex_size,

We should copy back the whole texture.

teoxoy · 2023-09-01T16:02:10Z

tests/tests/write_texture.rs

+
+        let val = (mip_level + 1) as u8;
+        let data = vec![val; total_size_in_bytes as usize];
+
        // Write the first two rows


Comment is outdated.

teoxoy · 2023-09-01T16:03:54Z

tests/tests/write_texture.rs

        );

        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,


Should be large enough to copy back the whole texture.

teoxoy · 2023-09-01T16:07:13Z

tests/tests/write_texture.rs

+        tex_size,
+        tex_size,
+        tex_size,


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

teoxoy · 2023-09-01T16:15:21Z

wgpu-hal/src/dx12/command.rs

-                Height: self
-                    .buffer_layout
-                    .rows_per_image
-                    .map_or(self.size.height, |count| count * block_height),
+                Height: self.size.height,


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?

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

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.

teoxoy

Marking as "Request changes"

cwfitzgerald reviewed Jul 31, 2023

View reviewed changes

wgpu-hal/src/dx12/command.rs Outdated Show resolved Hide resolved

teoxoy reviewed Jul 31, 2023

View reviewed changes

wgpu-hal/src/dx12/command.rs Outdated Show resolved Hide resolved

cwfitzgerald previously approved these changes Aug 1, 2023

View reviewed changes

39ali force-pushed the write_texture_subset_dx12 branch from 9eb78c7 to d655986 Compare August 4, 2023 23:21

39ali force-pushed the write_texture_subset_dx12 branch from 5096e23 to 013f712 Compare August 26, 2023 16:04

Fixed Queue::write_texture on dx12 + refactored and added new test to…

995aaf7

… write_texture.rs

39ali force-pushed the write_texture_subset_dx12 branch from 013f712 to 995aaf7 Compare August 26, 2023 16:18

fix clippy warnings

ff78ba1

teoxoy reviewed Sep 1, 2023

View reviewed changes

cwfitzgerald added the PR: waiting for author label Sep 14, 2023

teoxoy mentioned this pull request Sep 18, 2023

[d3d12] Support padded rows_per_image for 3D texture writes #4150

Open

teoxoy requested changes Sep 20, 2023

View reviewed changes

cwfitzgerald removed the PR: waiting for author label Oct 4, 2023

cwfitzgerald mentioned this pull request Jan 4, 2024

Fix Queue::write_texture, Fix DX12 write_texture_subset_2d and re-enable the test. #4990

Merged

6 tasks

cwfitzgerald closed this in #4990 Jan 5, 2024


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

		test(

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

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

Uh oh!

Conversation

39ali commented Jul 30, 2023 • edited by teoxoy Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cwfitzgerald left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

cwfitzgerald left a comment

Choose a reason for hiding this comment

Uh oh!

cwfitzgerald commented Aug 1, 2023

Uh oh!

teoxoy commented Aug 2, 2023

Uh oh!

39ali commented Aug 2, 2023

Uh oh!

teoxoy commented Aug 2, 2023

Uh oh!

teoxoy commented Aug 2, 2023

Uh oh!

39ali commented Aug 2, 2023

Uh oh!

39ali commented Aug 26, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

teoxoy left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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

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

39ali commented Jul 30, 2023 •

edited by teoxoy

Loading