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

Implement view_formats for SurfaceConfiguration #3409

Merged
merged 13 commits into from
Jan 25, 2023

Conversation

jinleili
Copy link
Contributor

@jinleili jinleili commented Jan 20, 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
Follow up to #3237

Testing
Tested locally except deno part

@jinleili jinleili changed the title View formats follow Implement view_formats for SurfaceConfiguration Jan 20, 2023
Copy link
Collaborator

@crowlKats crowlKats left a comment

Choose a reason for hiding this comment

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

Deno part looks good to me.

If you want, i can test the deno change, but am unsure how viewformats are used. Though, given its a simple & straight-forward change, testing it doesnt seem too necessary

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.

We should validate the viewFormats in validate_surface_configuration
according to https://gpuweb.github.io/gpuweb/#dom-gpucanvascontext-configure.

Otherwise, looks good!

@jinleili
Copy link
Contributor Author

Looking at the #3399 implementation, I'm not sure if the Vulkan backend needs to be handled separately here as well.

@teoxoy
Copy link
Member

teoxoy commented Jan 20, 2023

Thanks for updating the validation but there is one more rule:

Validate texture format required features of each element of configuration.viewFormats with device.[[device]].

Ideally, we'd share some validation for the TextureDescriptor and the configure fn... but not a blocker.

Looking at the #3399 implementation, I'm not sure if the Vulkan backend needs to be handled separately here as well.

I don't think we need to do anything extra for Vulkan here. @cwfitzgerald can maybe confirm?

@cwfitzgerald
Copy link
Member

Yeah, I think we need mutable format on the swapchain via https://registry.khronos.org/vulkan/specs/1.3-extensions/man/html/VkSwapchainCreateFlagBitsKHR.html and VK_KHR_swapchain_mutable_format. This is fine on desktop, but has zero support on android.

@jinleili jinleili force-pushed the view_formats_follow branch 2 times, most recently from 9e89f65 to d30bc2c Compare January 22, 2023 09:14
@jinleili
Copy link
Contributor Author

Yeah, I think we need mutable format on the swapchain via https://registry.khronos.org/vulkan/specs/1.3-extensions/man/html/VkSwapchainCreateFlagBitsKHR.html and VK_KHR_swapchain_mutable_format. This is fine on desktop, but has zero support on android.

The addition of the corresponding extension KhrImageFormatListFn was in #3412

@teoxoy
Copy link
Member

teoxoy commented Jan 23, 2023

VK_KHR_image_format_list can be optional but it seems to be required if we require VK_KHR_swapchain_mutable_format?
Maybe we should land #3412 first and make it required in this PR if that's the case.

Or can we still set VK_SWAPCHAIN_CREATE_MUTABLE_FORMAT_BIT_KHR and not give a format list?

@jinleili
Copy link
Contributor Author

I checked the VK_KHR_swapchain_mutable_format extension on Android 13 devices, and sure enough, it's not supported on Android devices.

VK_KHR_image_format_list can be optional but it seems to be required if we require VK_KHR_swapchain_mutable_format?

Yes, after re-reading the documentation, they should all be required. Since VK_KHR_imageless_framebuffer also requires VK_KHR_image_format_list, the permission of this extension has to be determined separately.

wgpu-hal/src/vulkan/adapter.rs Outdated Show resolved Hide resolved
wgpu-hal/src/vulkan/device.rs Show resolved Hide resolved
wgpu-hal/src/vulkan/device.rs Outdated Show resolved Hide resolved
@jinleili jinleili force-pushed the view_formats_follow branch 2 times, most recently from 1fc9229 to 2458a18 Compare January 24, 2023 14:24
wgpu-core/src/device/mod.rs Outdated Show resolved Hide resolved
wgpu-hal/src/vulkan/device.rs Outdated Show resolved Hide resolved
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.

2 remaining things and we should be good to go.
Thanks for addressing all the comments!

wgpu-hal/src/vulkan/adapter.rs Outdated Show resolved Hide resolved
wgpu-hal/src/vulkan/device.rs Outdated Show resolved Hide resolved
@cwfitzgerald
Copy link
Member

Adopting this PR so we can get it in this release

@cwfitzgerald cwfitzgerald enabled auto-merge (squash) January 25, 2023 20:01
@cwfitzgerald cwfitzgerald merged commit 33f94c7 into gfx-rs:master Jan 25, 2023
@jinleili jinleili deleted the view_formats_follow branch January 26, 2023 04:00
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.

4 participants