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

Revert "Support nv12 texture format" #4823

Closed
wants to merge 2 commits into from

Conversation

teoxoy
Copy link
Member

@teoxoy teoxoy commented Dec 4, 2023

Reverts #4573 due to CI failures that make it difficult to merge other PRs.

See also #4573 (comment).

@teoxoy teoxoy requested a review from a team December 4, 2023 13:10
@teoxoy teoxoy requested a review from a team as a code owner December 4, 2023 13:10
@cwfitzgerald
Copy link
Member

Instead of rolling back the whole MR, can we just fix the failure? It's just some validation that's acting up

@teoxoy
Copy link
Member Author

teoxoy commented Dec 4, 2023

cc @xiaopengli89

@cwfitzgerald
Copy link
Member

Or more specifically, I don't quite understand why this is failing now but didn't fail inside the PR

@teoxoy
Copy link
Member Author

teoxoy commented Dec 4, 2023

Most likely due to updated validation layers. Interesting timing though.

@cwfitzgerald
Copy link
Member

Validation layers haven't updated since mid october

@teoxoy
Copy link
Member Author

teoxoy commented Dec 4, 2023

Indeed, CI logs show the same versions being installed.

Setting up vulkan-validationlayers (1.3.268.0~rc2-1lunarg22.04-1) ...
Setting up lunarg-vulkan-layers (1.3.268.0~rc1-1lunarg22.04-1) ...

Idk why the error didn't show up previously then, odd. But I think it's correct nonetheless.

@cwfitzgerald
Copy link
Member

Heh, it was a mesa update. We went from 23.2.1 to 23.3.0 and lavapipe learned how to make NV12 textures 😆

@teoxoy
Copy link
Member Author

teoxoy commented Dec 4, 2023

Ah, that explains it :)

@cwfitzgerald
Copy link
Member

I'm going to close this, I don't want to revert the whole PR.

We should either:

@teoxoy teoxoy deleted the revert-4573-texture-format-nv12 branch December 4, 2023 18:17
@xiaopengli89
Copy link
Contributor

@teoxoy I can successfully create nv12 texture and r8/rg8 views on windows with vulkan backend. I think this is an implementation issue with llvmpipe. In addition, we must add the plane view formats to the format list, otherwise the corresponding view cannot be created. I consider disabling the nv12 feature for llvmpipe.

@teoxoy
Copy link
Member Author

teoxoy commented Dec 5, 2023

In addition, we must add the plane view formats to the format list, otherwise the corresponding view cannot be created.

I don't think this is the case, we don't have to add the individual aspect formats for combined depth-stencil formats either. I added references to the Vulkan spec in #4573 (comment). If things work on one Vulkan implementation despite the Vulkan spec saying otherwise that doesn't mean it's correct.

@teoxoy
Copy link
Member Author

teoxoy commented Dec 5, 2023

We have to set VK_IMAGE_CREATE_MUTABLE_FORMAT_BIT for plane selection to work but don't have to pass additional view formats.

References:

VUID-VkImageViewCreateInfo-image-01586
If image was created with the VK_IMAGE_CREATE_MUTABLE_FORMAT_BIT flag, if the format of the image is a multi-planar format, and if subresourceRange.aspectMask is one of the multi-planar aspect mask bits, then format must be compatible with the VkFormat for the plane of the image format indicated by subresourceRange.aspectMask, as defined in https://registry.khronos.org/vulkan/specs/1.3-extensions/html/vkspec.html#formats-compatible-planes

VUID-VkImageViewCreateInfo-image-01762
If image was not created with the VK_IMAGE_CREATE_MUTABLE_FORMAT_BIT flag, or if the format of the image is a multi-planar format and if subresourceRange.aspectMask is VK_IMAGE_ASPECT_COLOR_BIT, format must be identical to the format used to create image

from https://registry.khronos.org/vulkan/specs/1.3-extensions/man/html/VkImageViewCreateInfo.html

VK_IMAGE_CREATE_MUTABLE_FORMAT_BIT specifies that the image can be used to create a VkImageView with a different format from the image. For multi-planar formats, VK_IMAGE_CREATE_MUTABLE_FORMAT_BIT specifies that a VkImageView can be created of a plane of the image.

from https://registry.khronos.org/vulkan/specs/1.3-extensions/man/html/VkImageCreateFlagBits.html

VUID-VkImageCreateInfo-pNext-06722
If a VkImageFormatListCreateInfo structure was included in the pNext chain and VkImageFormatListCreateInfo::viewFormatCount is not zero, then each format in VkImageFormatListCreateInfo::pViewFormats must either be compatible with the format as described in the compatibility table or, if flags contains VK_IMAGE_CREATE_BLOCK_TEXEL_VIEW_COMPATIBLE_BIT, be an uncompressed format that is size-compatible with format

from https://registry.khronos.org/vulkan/specs/1.3-extensions/man/html/VkImageCreateInfo.html

@xiaopengli89
Copy link
Contributor

We can't add image format list at all, including the original format:

[2023-12-05T11:32:40Z ERROR wgpu_test::expectations] Unexpected failure due to: ValidationError(Some("Validation Error: [ VUID-VkImageViewCreateInfo-pNext-01585 ] Object 0: handle = 0x967dd1000000000e, type = VK_OBJECT_TYPE_IMAGE; | MessageID = 0xdd1d0240 | vkCreateImageView(): pCreateInfo->pNext<VkImageFormatListCreateInfo>.pViewFormats has no formats that match the VkImageViewCreateInfo::format (VK_FORMAT_R8G8_UNORM). The Vulkan spec states: If a VkImageFormatListCreateInfo structure was included in the pNext chain of the VkImageCreateInfo structure used when creating image and VkImageFormatListCreateInfo::viewFormatCount is not zero then format must be one of the formats in VkImageFormatListCreateInfo::pViewFormats (https://vulkan.lunarg.com/doc/view/1.3.268.0/linux/1.3-extensions/vkspec.html#VUID-VkImageViewCreateInfo-pNext-01585)"))

@teoxoy
Copy link
Member Author

teoxoy commented Dec 5, 2023

Yeah, that shouldn't be necessary for the NV12 format; wgpu users shouldn't have to pass the r8 and rg8 formats into view_formats.

@xiaopengli89
Copy link
Contributor

wgpu users shouldn't have to pass the r8 and rg8 formats into view_formats.

I don't think this is reasonable, because view_formats should limit which views the texture supports at the abstraction level. I think it is more appropriate to handle it internally in wgpu (#4832).

@teoxoy
Copy link
Member Author

teoxoy commented Dec 5, 2023

That's not consistent with combined depth-stencil formats, users of the API don't have to pass the aspect formats to view_formats. view_formats is only currently used to create srgb views from non-srgb textures and vice versa.

which views the texture supports at the abstraction level

We can enforce this without users having to pass the plane formats to view_formats in the same way we do for combined depth-stencil formats (especially that Vulkan doesn't need us to pass the view formats to pViewFormats).

@xiaopengli89
Copy link
Contributor

xiaopengli89 commented Dec 6, 2023

Okay, it seems better to have plane as an aspect.

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