-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Make multi-planar textures renderable #8307
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
base: trunk
Are you sure you want to change the base?
Conversation
658925c to
5a10d31
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for writing all the tests, helps me feel good about getting this in! Some comments.
| if desc.format.is_multi_planar_format() { | ||
| raw_flags |= vk::ImageCreateFlags::MUTABLE_FORMAT; | ||
| raw_flags |= | ||
| vk::ImageCreateFlags::MUTABLE_FORMAT | vk::ImageCreateFlags::EXTENDED_USAGE; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit regarding extended usage, do we already ensure that we have vulkan 1.1 before we allow multi-planar textures? If not we should add that check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I'm reading the code correctly, TEXTURE_FORMAT_NV12 and TEXTURE_FORMAT_P010 features require VK_KHR_sampler_ycbcr_conversion extension which depends on:
- Vulkan 1.1, OR
VK_KHR_maintenance1andVK_KHR_bind_memory2andVK_KHR_get_memory_requirements2andVK_KHR_get_physical_device_properties2
From what I checked, wgpu does not require all of those extensions so the only way we could use multi-planar textures would be if we had at least vulkan 1.1 and I think wgpu ensures that (unless I read the code wrong)
| let (width, height) = match (self.format, plane) { | ||
| (TextureFormat::NV12 | TextureFormat::P010, Some(0)) => (width, height), | ||
| (TextureFormat::NV12 | TextureFormat::P010, Some(1)) => (width / 2, height / 2), | ||
| _ => (width, height), | ||
| }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My only worry here is that when we add future multi-planar formats, there will be no error to tell us to populate this. Maybe a (debug) assert in the catchall branch to assert that the format is single-planar and maybe a test which calls this function on all texture formats?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added the test but I'm not sure if I put it in the right file. I wanted to reuse TEXTURE_FORMAT_LIST from wgpu-info but it's not available from wgpu-types so the test is in wgpu-info which seems wrong to me
5a10d31 to
a87815a
Compare
a87815a to
201c251
Compare
Connections
Closes #8205
Description
Currently it was not possible to render onto planes of multiplanar textures (in this case, it's only possible for NV12).
To make it work:
render_extentis calculated for texture viewsTesting
I tried to render on both Vulkan and DX12 (the only supported backends) + I've added tests.
987 tests run: 982 passed, 5 failed, 12 skipped.On my machine 5 tests were failing before and after my changes.
Checklist
cargo fmt.taplo format.cargo clippy --tests. If applicable, add:--target wasm32-unknown-unknowncargo xtask testto run tests.CHANGELOG.mdentry.