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.

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

Jump to bottom

Support texture-compression-bc-sliced-3d in wgpu #5751

Merged
merged 25 commits into from
Aug 10, 2024
Merged

Support texture-compression-bc-sliced-3d in wgpu #5751

merged 25 commits into from
Aug 10, 2024

Conversation

mehmetoguzderin
Copy link
Contributor

Fixes #5750

Since it has been a while since I created a PR for wgpu, I might have overlooked some aspects. Your guidance would be appreciated. Thank you!

@mehmetoguzderin mehmetoguzderin requested a review from a team as a code owner May 28, 2024 23:33
@teoxoy
Copy link
Member

teoxoy commented May 29, 2024

I do have a larger concern about this though; with the availability of ASTC/ETC formats on Vulkan gpuweb/gpuweb#3183 (comment) and it seems ETC formats on Metal as well (contrary to their docs) gpuweb/gpuweb#3183 (comment).

@mehmetoguzderin
Copy link
Contributor Author

@teoxoy right, seems like further investigation is necessary as to only allow BCn or also ASTC (ETC2 seems harder).

teoxoy
teoxoy reviewed May 31, 2024
View reviewed changes
wgpu-types/src/lib.rs Outdated Show resolved Hide resolved
@teoxoy
Copy link
Member

teoxoy commented Jun 3, 2024

The macOS runner seems to be failing a lot of tests (looks like the ones that actually execute something on the queue) right after we start executing clear_texture_compressed_bcn.

        SLOW [> 45.000s] wgpu-test::wgpu-test [Executed] [Metal/Apple Paravirtual device/0] wgpu_test::clear_texture::clear_texture_compressed_bcn
        SLOW [> 45.000s] wgpu-test::wgpu-test [Executed] [Metal/Apple Paravirtual device/0] wgpu_test::clear_texture::clear_texture_uncompressed
        SLOW [> 45.000s] wgpu-test::wgpu-test [Executed] [Metal/Apple Paravirtual device/0] wgpu_test::clear_texture::clear_texture_uncompressed_gles
        PASS [  60.282s] wgpu-test::wgpu-test [Executed] [Metal/Apple Paravirtual device/0] wgpu_test::clear_texture::clear_texture_uncompressed
        PASS [  61.647s] wgpu-test::wgpu-test [Executed] [Metal/Apple Paravirtual device/0] wgpu_test::clear_texture::clear_texture_compressed_bcn
...
 TERMINATING [> 90.000s] wgpu-test::wgpu-test [Executed] [Metal/Apple Paravirtual device/0] wgpu_test::clear_texture::clear_texture_uncompressed_gles
     TIMEOUT [  90.003s] wgpu-test::wgpu-test [Executed] [Metal/Apple Paravirtual device/0] wgpu_test::clear_texture::clear_texture_uncompressed_gles
...
     Summary [ 881.135s] 382 tests run: 360 passed (15 slow), 17 failed, 5 timed out, 0 skipped
     TIMEOUT [  90.003s] wgpu-test::wgpu-test [Executed] [Metal/Apple Paravirtual device/0] wgpu_test::clear_texture::clear_texture_uncompressed_gles
        FAIL [  60.095s] wgpu-test::wgpu-test [Executed] [Metal/Apple Paravirtual device/0] wgpu_test::compute_pass_ownership::compute_pass_resource_ownership
        FAIL [  60.116s] wgpu-test::wgpu-test [Executed] [Metal/Apple Paravirtual device/0] wgpu_test::occlusion_query::occlusion_query
        FAIL [  60.150s] wgpu-test::wgpu-test [Executed] [Metal/Apple Paravirtual device/0] wgpu_test::push_constants::partial_update
        FAIL [  60.115s] wgpu-test::wgpu-test [Executed] [Metal/Apple Paravirtual device/0] wgpu_test::regression::issue_3349::multi_stage_data_binding
     TIMEOUT [  90.008s] wgpu-test::wgpu-test [Executed] [Metal/Apple Paravirtual device/0] wgpu_test::regression::issue_4122::clear_buffer_range_respected
        FAIL [  60.143s] wgpu-test::wgpu-test [Executed] [Metal/Apple Paravirtual device/0] wgpu_test::scissor_tests::scissor_test_custom_rect
        FAIL [  60.111s] wgpu-test::wgpu-test [Executed] [Metal/Apple Paravirtual device/0] wgpu_test::scissor_tests::scissor_test_full_rect
        FAIL [  60.115s] wgpu-test::wgpu-test [Executed] [Metal/Apple Paravirtual device/0] wgpu_test::shader::data_builtins::pack4x_i8
        FAIL [  60.126s] wgpu-test::wgpu-test [Executed] [Metal/Apple Paravirtual device/0] wgpu_test::shader::data_builtins::pack4x_u8
        FAIL [  60.104s] wgpu-test::wgpu-test [Executed] [Metal/Apple Paravirtual device/0] wgpu_test::shader::data_builtins::unpack4x_i8
        FAIL [  60.102s] wgpu-test::wgpu-test [Executed] [Metal/Apple Paravirtual device/0] wgpu_test::shader::data_builtins::unpack4x_u8
        FAIL [  60.155s] wgpu-test::wgpu-test [Executed] [Metal/Apple Paravirtual device/0] wgpu_test::shader::numeric_builtins::numeric_builtins
     TIMEOUT [  90.010s] wgpu-test::wgpu-test [Executed] [Metal/Apple Paravirtual device/0] wgpu_test::shader::struct_layout::push_constant_input
     TIMEOUT [  90.010s] wgpu-test::wgpu-test [Executed] [Metal/Apple Paravirtual device/0] wgpu_test::shader::struct_layout::storage_input
        FAIL [  61.269s] wgpu-test::wgpu-test [Executed] [Metal/Apple Paravirtual device/0] wgpu_test::shader::struct_layout::uniform_input
        FAIL [  60.112s] wgpu-test::wgpu-test [Executed] [Metal/Apple Paravirtual device/0] wgpu_test::shader_primitive_index::draw
        FAIL [  60.126s] wgpu-test::wgpu-test [Executed] [Metal/Apple Paravirtual device/0] wgpu_test::shader_primitive_index::draw_indexed
        FAIL [  60.185s] wgpu-test::wgpu-test [Executed] [Metal/Apple Paravirtual device/0] wgpu_test::shader_view_format::reinterpret_srgb
     TIMEOUT [  90.010s] wgpu-test::wgpu-test [Executed] [Metal/Apple Paravirtual device/0] wgpu_test::vertex_indices::vertex_indices
        FAIL [  60.075s] wgpu-test::wgpu-test [Executed] [Metal/Apple Paravirtual device/0] wgpu_test::write_texture::write_texture_subset_2d
        FAIL [  60.112s] wgpu-test::wgpu-test [Executed] [Metal/Apple Paravirtual device/0] wgpu_test::write_texture::write_texture_subset_3d

For reference, this is how long the clear texture ones take on trunk:

        PASS [   0.347s] wgpu-test::wgpu-test [Executed] [Metal/Apple Paravirtual device/0] wgpu_test::clear_texture::clear_texture_uncompressed
        PASS [   0.730s] wgpu-test::wgpu-test [Executed] [Metal/Apple Paravirtual device/0] wgpu_test::clear_texture::clear_texture_compressed_bcn
        PASS [   2.736s] wgpu-test::wgpu-test [Executed] [Metal/Apple Paravirtual device/0] wgpu_test::clear_texture::clear_texture_uncompressed_gles

This is odd since the BC test eventually succeeds but there is some internal state getting messed up making the device unusable even across tests?

@mehmetoguzderin
Copy link
Contributor Author

@teoxoy On my local device (just baseline M1), I get the following runtime for BCn clear tests:

PASS [   3.343s] wgpu-test::wgpu-test [Executed] [Metal/Apple M1/1] wgpu_test::clear_texture::clear_texture_compressed_bcn
PASS [   3.295s] wgpu-test::wgpu-test [Executed] [Vulkan/Apple M1/0] wgpu_test::clear_texture::clear_texture_compressed_bcn

So, unfortunately, I am unable to reproduce the CI's situation locally. BTW, I just checked the validation layers on my macOS environment, and they are all set to valid paths, which seem to work across many apps, including Vulkan Configurator itself, yet I still get the same [Vulkan/Apple M1/0] wgpu_test::shader::struct_layout::uniform_input failure (the only test fail on my system).

@teoxoy
Copy link
Member

teoxoy commented Jun 3, 2024

That sounds like another curious issue tracked by #5665.

@mehmetoguzderin
Copy link
Contributor Author

@teoxoy apologies for the sudden ping during these busy times. As I was looking into migrating to extension, I got curious, although the tests pass with my local, non-paravirtualized M1 drivers, in case restructuring into extension keeps throwing errors for paravirtual driver, is it reasonable to tentatively blocklist this extension for Apple platforms? Is there a policy to allow partial forward progress with corner cases like this? All the best.

@teoxoy
Copy link
Member

teoxoy commented Jul 26, 2024

No worries. Yes there is a .expect_fail() method on TestParameters.

@mehmetoguzderin
Copy link
Contributor Author

@teoxoy thank you for the insight! I will be conscious of it as I update the PR.

@mehmetoguzderin mehmetoguzderin changed the title Enable compressed formats in wgpu for 3D textures Support texture-compression-bc-sliced-3d in wgpu Jul 26, 2024
@mehmetoguzderin
Copy link
Contributor Author

(Merging trunk made this branch go green with CI, interesting! Hopefully that'll hold valid by the time I update into extension)

@mehmetoguzderin
Copy link
Contributor Author

@teoxoy sorry for the ping again. I finally got around to looking at migrating to the extension structure. First of all, apologies for the increase in surface area of this PR due to the delay in putting the support behind a feature. I tried my best to:

  1. Comment that the BC base feature guarantees things,
  2. Cover backends where possible (DX12, Metal, Vulkan, GL).

Furthermore, I noticed that the feature comment for BC was outdated; nowadays, all Apple9 iOS and iPadOS devices, as well as some Apple7 and Apple8 iPadOS devices, support BC as mobile alongside desktops.

However, the changes might have been less than optimal for a few aspects:

  1. I set the flag to 1 << 11. I don't know if that's optimal for the project's future; there might be too few bits and too many features at some point, potentially.
  2. I use the require_features and map the error to reject if the feature is not enabled. However, it is the only use of that kind for texture creation, and I am OK with updating if there's a better way.

I just enabled the SLICED_3D feature for testing, as BC simply guarantees it.

I'd appreciate it if you could take a look at your convenience. I'll try to improve in case there's a less-than-desirable aspect in the current shape of PR depending on review. On the good news, CI got green once again (which might let us put things in TODO if need be and go incremental)!

teoxoy
teoxoy requested changes Aug 9, 2024
View reviewed changes
Copy link
Member

@teoxoy teoxoy left a comment

Choose a reason for hiding this comment

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

No problem, thanks for updating the PR!

For point 1, I think we should keep all the feature flags ordered, there is also this comment that needs updating:

wgpu/wgpu-types/src/lib.rs

Lines 418 to 420 in f6a3eef

// Bits 11-19 available for webgpu features. Should you chose to use some of them for
// for native features, don't forget to update `all_webgpu_mask` and `all_native_mask`
// accordingly.

For point 2, I think it's fine as it is.

@mehmetoguzderin
Copy link
Contributor Author

@teoxoy thank you very much for your kind review.

Good catch, sorry for the overlook! The latest commit addresses that and makes the flag counting in order as they appear in spec.

I appreciate the feedback, please let me know if there's anything further to refine this PR to make sure it is a reasonable commit for the project, and thank you and the team for all the efforts to keep wgpu a great project.

@teoxoy
Copy link
Member

teoxoy commented Aug 10, 2024

Thanks for your kind words and your contributions! :)

@teoxoy teoxoy merged commit 34b0df2 into gfx-rs:trunk Aug 10, 2024
27 checks passed
@mehmetoguzderin
Copy link
Contributor Author

@teoxoy Likewise, thank you for the merge!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@teoxoy teoxoy teoxoy approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support texture-compression-bc-sliced-3d
2 participants
@mehmetoguzderin @teoxoy