Skip to content

Add BGRA8UNORM_STORAGE extension#3634

Closed
jinleili wants to merge 3 commits intogfx-rs:trunkfrom
jinleili:bgra8unorm-storage
Closed

Add BGRA8UNORM_STORAGE extension#3634
jinleili wants to merge 3 commits intogfx-rs:trunkfrom
jinleili:bgra8unorm-storage

Conversation

@jinleili
Copy link
Copy Markdown
Contributor

@jinleili jinleili commented Apr 2, 2023 •
edited by teoxoy
Loading

Checklist

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

Connections
resolves #3359

Testing
Tested locally on metal, vulkan(--features=vulkan-portability) and dx12 backends:

fn required_features() -> wgpu::Features {
    // wgpu::Features::TEXTURE_ADAPTER_SPECIFIC_FORMAT_FEATURES
    // OR
    wgpu::Features::BGRA8UNORM_STORAGE
}

// ...

let a_texture = device.create_texture(&wgpu::TextureDescriptor {
    format: wgpu::TextureFormat::Bgra8Unorm,
    usage: wgpu::TextureUsages::STORAGE_BINDING,
    ...,
});

let test_tv = a_texture.create_view(&wgpu::TextureViewDescriptor {
    format: Some(wgpu::TextureFormat::Bgra8Unorm),
    ..Default::default()
});

@jinleili jinleili requested a review from crowlKats as a code owner April 2, 2023 07:57
@jinleili jinleili force-pushed the bgra8unorm-storage branch from 36b15a0 to 9f3d024 Compare April 2, 2023 08:02
teoxoy
teoxoy requested changes Apr 4, 2023
View reviewed changes
Copy link
Copy Markdown
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.

Thanks for the PR!

I think we should wait for naga to add support for the texel format before we merge this so that we can test it.

wgpu-hal/src/vulkan/adapter.rs
phd,
vk::Format::B8G8R8A8_UNORM,
vk::ImageTiling::OPTIMAL,
vk::FormatFeatureFlags::STORAGE_IMAGE,
Copy link
Copy Markdown
Member

@teoxoy teoxoy Apr 4, 2023

Choose a reason for hiding this comment

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

On the shader side we need to use Unknown since bgra8unorm is not in this table.

So, we need to check for VK_FORMAT_FEATURE_2_STORAGE_WRITE_WITHOUT_FORMAT_BIT instead.

See KhronosGroup/Vulkan-Docs#2027 (comment) for more background.

Copy link
Copy Markdown
Contributor Author

@jinleili jinleili Apr 17, 2023

Choose a reason for hiding this comment

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

I'm sorry, I don't know how to do the check of VK_FORMAT_FEATURE_2_STORAGE_WRITE_WITHOUT_FORMAT_BIT . Check for FORMAT_FEATURE_2 requires obtaining FormatProperties3 first, but no relevant method was found in ash.

Copy link
Copy Markdown
Member

@teoxoy teoxoy Apr 17, 2023

Choose a reason for hiding this comment

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

Isn't it this structure: https://docs.rs/ash/latest/ash/vk/struct.FormatProperties3.html ?

Copy link
Copy Markdown
Contributor Author

@jinleili jinleili Apr 17, 2023

Choose a reason for hiding this comment

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

Yes, but no way to get it is provided, onlyget_physical_device_format_properties and get_physical_device_format_properties to get FormatProperties FormatProperties2.

Copy link
Copy Markdown
Member

@teoxoy teoxoy Apr 18, 2023

Choose a reason for hiding this comment

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

Hmm, how I think this should work is we call get_physical_device_format_properties2 and pass it a FormatProperties2 with a FormatProperties3 in it's pNext chain.

This is quite convoluted as it requires extensions or specific vulkan versions.

VK_KHR_get_physical_device_properties2 or VK1.1 for the query function and FormatProperties2 structure +
VK_KHR_format_feature_flags2 or VK1.3 for FormatProperties3

@crowlKats
Copy link
Copy Markdown
Collaborator

crowlKats commented Apr 4, 2023

the feature also needs to be added to https://github.com/gfx-rs/wgpu/blob/master/deno_webgpu/02_idl_types.js#L105-L152

@jinleili jinleili force-pushed the bgra8unorm-storage branch from 9f3d024 to 5125578 Compare April 17, 2023 04:13
@teoxoy teoxoy mentioned this pull request Apr 17, 2023
Merged
3 tasks
@cwfitzgerald
Copy link
Copy Markdown
Member

cwfitzgerald commented Sep 14, 2023

Hey this got lost in the PR stack, could you rebase this?

With naga not supporting it yet, how about we land all the infrastructure, but we keep the feature disabled.

@jinleili
Copy link
Copy Markdown
Contributor Author

jinleili commented Sep 17, 2023

@cwfitzgerald Already rebased, but I'm unsure how to keep the feature disabled.

@cwfitzgerald
Copy link
Copy Markdown
Member

cwfitzgerald commented Sep 18, 2023

Probably through wgpu-core removing the feature after it queries the features from the device. Can remove it and link back to the naga issue.

@cwfitzgerald cwfitzgerald requested a review from a team September 20, 2023 18:57
@teoxoy
Copy link
Copy Markdown
Member

teoxoy commented Sep 27, 2023

There is still an unresolved discussion about the Vulkan impl.

cwfitzgerald
cwfitzgerald requested changes Oct 3, 2023
View reviewed changes
Copy link
Copy Markdown
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.

Dismissing review request due to the above issue.

This was referenced Oct 5, 2023
Closed
Merged
Merged
@nical
Copy link
Copy Markdown
Contributor

nical commented Oct 10, 2023

Heads up, I'm continuing the work on this branch: https://github.com/nical/wgpu/tree/bgra8unorm-storage
I'll propose change to this PR or open a new one soon (currently writing a test and hoping it'll work).

@jinleili
Copy link
Copy Markdown
Contributor Author

jinleili commented Oct 10, 2023

I'm sorry I didn't deal with it for so long, since @nical is already done, it's more appropriate to open a new PR

@jinleili jinleili closed this Oct 10, 2023
@nical nical mentioned this pull request Oct 10, 2023
Merged
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@teoxoy teoxoy teoxoy requested changes

@cwfitzgerald cwfitzgerald cwfitzgerald requested changes

@crowlKats crowlKats Awaiting requested review from crowlKats

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Support bgra8unorm-storage extension

5 participants

@jinleili @crowlKats @cwfitzgerald @teoxoy @nical