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

Add BGRA8UNORM_STORAGE extension #3634

Closed
wants to merge 3 commits into from

Conversation

jinleili
Copy link
Contributor

@jinleili jinleili commented Apr 2, 2023

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
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.

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/dx12/adapter.rs Outdated Show resolved Hide resolved
phd,
vk::Format::B8G8R8A8_UNORM,
vk::ImageTiling::OPTIMAL,
vk::FormatFeatureFlags::STORAGE_IMAGE,
Copy link
Member

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
Contributor Author

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
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

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
Member

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

wgpu-core/src/device/mod.rs Outdated Show resolved Hide resolved
@crowlKats
Copy link
Collaborator

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

@cwfitzgerald
Copy link
Member

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
Contributor Author

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

@cwfitzgerald
Copy link
Member

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.

@teoxoy
Copy link
Member

teoxoy commented Sep 27, 2023

There is still an unresolved discussion about the Vulkan impl.

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.

Dismissing review request due to the above issue.

@nical
Copy link
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
Contributor Author

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
2 tasks
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.

Support bgra8unorm-storage extension
5 participants