Skip to content
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.

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

Support stencil-only views and copying to/from combined depth-stencil textures #3436

Merged
merged 3 commits into from
Feb 15, 2023

Conversation

teoxoy
Copy link
Member

@teoxoy teoxoy commented Jan 30, 2023

General changes:

  • break down describe into block_dimensions, required_features, guaranteed_format_features, sample_type and block_size
  • sample_type and block_size now take an optional TextureAspect

To enable copying to/from combined depth-stencil textures:

  • [validation] copy aspect must refer to a single aspect of format
  • [validation] update validate_linear_texture_data according to the WebGPU spec
  • [validation] require missing DEPTH_TEXTURE_AND_BUFFER_COPIES downlevel flag for queue_write_texture and command_encoder_copy_buffer_to_texture
  • [metal] set the right MTLBlitOption for texture/buffer copies
  • [dx12] use specific copy texture formats and set the right texture plane

To create stencil-only views from combined depth-stencil textures:

  • [metal] set the right format X24_Stencil8/X32_Stencil8 for the view and MTLTextureUsage::PixelFormatView for the texture
  • [gles] set the right DEPTH_STENCIL_TEXTURE_MODE to either DEPTH_COMPONENT or STENCIL_INDEX
  • [dx12] set the right formats DXGI_FORMAT_X32_TYPELESS_G8X24_UINT and DXGI_FORMAT_X24_TYPELESS_G8_UINT

@teoxoy teoxoy added this to the WebGPU Specification V1 milestone Jan 30, 2023
Copy link
Member

@cwfitzgerald cwfitzgerald left a comment

Choose a reason for hiding this comment

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

This is amazing stuff - my only main ask is if there could be a test for it - with this amount of code it's hard to judge if it's actually correct

CHANGELOG.md Show resolved Hide resolved
@teoxoy teoxoy force-pushed the depth-stencil-only-support branch from 4e04893 to b6a6ccf Compare February 10, 2023 17:50
@teoxoy
Copy link
Member Author

teoxoy commented Feb 10, 2023

I created a ReadbackBuffers abstraction which allows us to copy textures to buffers regardless if they support COPY_SRC and updated clear_texture and zero_init_texture_after_discard to use it.

It should test depth view creation indirectly since it's needed for the copy via shader and to test stencil view creation, there is some commented out code to do so.

One thing worth noting is that once we get the CTS up and running, it should also test copying to/from combined depth-stencil textures and creation of stencil-only views.

@teoxoy teoxoy force-pushed the depth-stencil-only-support branch 7 times, most recently from b538f9f to 36e4df7 Compare February 10, 2023 23:39
@teoxoy teoxoy requested a review from cwfitzgerald February 10, 2023 23:43
@teoxoy teoxoy force-pushed the depth-stencil-only-support branch from 36e4df7 to 1bcd63e Compare February 11, 2023 00:26
@teoxoy
Copy link
Member Author

teoxoy commented Feb 11, 2023

WARP is failing with...

[2023-02-11T00:11:10Z ERROR wgpu_hal::auxil::dxgi::result] Map buffer failed: 0x887A0005
[2023-02-11T00:11:10Z ERROR wgpu_core::device::life] Mapping failed Device(Lost)

My Nvidia dGPU and Intel iGPU run the test without issues.

@teoxoy teoxoy force-pushed the depth-stencil-only-support branch from 1bcd63e to 0133bcc Compare February 11, 2023 00:41
@teoxoy
Copy link
Member Author

teoxoy commented Feb 11, 2023

The clear_texture GLES implementation is also failing with "texture was not fully cleared".

… textures

- break down `describe` into `block_dimensions`, `required_features`, `guaranteed_format_features`, `sample_type` and `block_size`
- `sample_type` and `block_size` now take an optional `TextureAspect`

To enable copying to/from combined depth-stencil textures:
- [validation] copy aspect must refer to a single aspect of format
- [validation] update `validate_linear_texture_data` according to the WebGPU spec
- [validation] require missing `DEPTH_TEXTURE_AND_BUFFER_COPIES` downlevel flag for `queue_write_texture` and `command_encoder_copy_buffer_to_texture`
- [metal] set the right `MTLBlitOption` for texture/buffer copies
- [dx12] use specific copy texture formats and set the right texture plane

To create stencil-only views from combined depth-stencil textures:
- [metal] set the right format `X24_Stencil8`/`X32_Stencil8` for the view and `MTLTextureUsage::PixelFormatView` for the texture
- [gles] set the right `DEPTH_STENCIL_TEXTURE_MODE` to either `DEPTH_COMPONENT` or `STENCIL_INDEX`
- [dx12] set the right formats `DXGI_FORMAT_X32_TYPELESS_G8X24_UINT` and `DXGI_FORMAT_X24_TYPELESS_G8_UINT`
@teoxoy teoxoy force-pushed the depth-stencil-only-support branch from 5c6c1b7 to 00209f3 Compare February 14, 2023 20:40
@teoxoy
Copy link
Member Author

teoxoy commented Feb 14, 2023

Should be good to go now.

Copy link
Member

@cwfitzgerald cwfitzgerald left a comment

Choose a reason for hiding this comment

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

Absolutely stunning work all over, as always!

@cwfitzgerald cwfitzgerald merged commit c51edd3 into gfx-rs:master Feb 15, 2023
@teoxoy teoxoy deleted the depth-stencil-only-support branch February 15, 2023 23:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants