[metal] set the image stride to zero when updating a single layer of a 3D texture

[teoxoy]

Checklist

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

Connections
Follow up to #3063.

Description
See #3063 (comment).

Testing
Test has been modified.

[teoxoy]

It seems that Metal's validation errors don't surface in CI.
I removed the fix added in the previous PR and the test still succeeded (a68dc10).

[teoxoy]

Ah, I think I see why. Is there a way to run the tests under xcode (in CI)? I think we should do that.

cc @nical @cwfitzgerald

[cwfitzgerald]

We need to set the env var METAL_DEVICE_WRAPPER_TYPE=1 - we should do it in the test harness before wgpu is initialized so they get enabled locally too.

[Wumpf]

would imho be nice to parameterize this test to have it both do a full update and two piecewise.
The second one can then point to this PR, pointing out that it protects against regressing this

[teoxoy]

I can probably trigger the validation error with a simpler test case.
Will try to do that instead as opposed to modifying this one.

[teoxoy]

We need to set the env var METAL_DEVICE_WRAPPER_TYPE=1 - we should do it in the test harness before wgpu is initialized so they get enabled locally too.

Wouldn't that require #3873? I can add it in CI for now though.

[jimblandy]

I like the idea of separating out the test case. The code changes look good.

[teoxoy]

Besides some other validation errors that we get when I enabled metal's validation, I can't seem to get metal's validation to trigger for this specific case.

Maybe newer versions don't care but older do?
Should we just land the code change to make things consistent? (and revert the test & CI changes for now?)

[cwfitzgerald]

Wouldn't that require #3873? I can add it in CI for now though

@teoxoy No, you can just add the env var around here https://github.com/gfx-rs/wgpu/blob/trunk/tests/src/lib.rs#L328

As for what to do - split this however you feel is best, as long as at the end we end up:

• With the change landed
• With the env var set
• Test case written

We're where we need to be :)

[teoxoy]

@cwfitzgerald question about the env variable:
I can't find any reference to METAL_DEVICE_WRAPPER_TYPE (even though it does seem to work).
I did however find MTL_DEBUG_LAYER & MTL_SHADER_VALIDATION.
I feel like we should use those instead. Thoughts?

[cwfitzgerald]

I have no idea why we used this, I think this might be what xcode uses? I'm not particularly strongly either way, as long as it does what we want.

[cwfitzgerald]

#3873 now includes both MTL_DEBUG_LAYER and MTL_SHADER_VALIDATION

[cwfitzgerald]

@teoxoy what's the status on this?

[teoxoy]

From my testing I did at the time, I couldn't produce the validation error I was expecting by not setting image_byte_stride to 0 if extent.depth > 1.

We ran into the validation error for copy_buffer_to_texture in #3063 but not yet for copy_texture_to_buffer. The metal docs have the same requirements for image_byte_stride in both those cases. So, I don't know why I could not produce the validation error.

If someone more familiar with macOS/Metal could help on this front that would be great but it seems that this issue isn't a high priority right now.

I mainly left this open and as draft to remind me to look at it at some later time but closing it and opening an issue sounds like the better approach.