fuchsia: Support multiple formats of Vulkan surface images. #24807
Conversation
|
It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat. If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
|
@dragostis @dreveman @arbreng Could you please take a look? |
dreveman
left a comment
dreveman
left a comment
There was a problem hiding this comment.
If we need to support BGRA then please use setBufferCollectionImageConstraintsFUCHSIA instead and set multiple formats. That way we can allow the driver to decide what format is ideal and keep this type of workaround (if needed) in a more appropriate location.
Change flutter#23488 has switched all the Vulkan surfaces on Fuchsia to use RGBA8 color type. However, Using RGBA8 format on Intel GPUs has caused graphical artifacts on emulators (See http://fxbug.dev/70232). This change modifies vulkan_surface.cc to use the multi-format vkSetBufferCollectionImageConstraintsFUCHSIA() Vulkan API, so that it could specify multiple formats when allocating buffer collections. Bug: fxbug.dev/70232
gnoliyil
changed the title
Done. PTAL. |
|
This change won't build until #24829 and flutter/buildroot#441 are submitted to support the latest Vulkan headers in Flutter repository. |
| // | ||
| // | ||
| // | ||
| // | ||
| // #if defined(__aarch64__) | ||
| // constexpr SkColorType kSkiaColorType = kRGBA_8888_SkColorType; | ||
| // constexpr VkFormat kVulkanFormat = VK_FORMAT_R8G8B8A8_UNORM; | ||
| // constexpr VkImageCreateFlags kVulkanImageCreateFlags = 0; | ||
| // #else | ||
| // constexpr VkImageCreateFlags kVulkanImageCreateFlags = | ||
| // VK_IMAGE_CREATE_MUTABLE_FORMAT_BIT; | ||
| // #endif | ||
|
|
There was a problem hiding this comment.
please remove this before landing
| .queueFamilyIndexCount = 0, | ||
| .pQueueFamilyIndices = nullptr, | ||
| .initialLayout = VK_IMAGE_LAYOUT_UNDEFINED, | ||
| const VkImageCreateInfo kDefaultImageCreateInfo = { |
There was a problem hiding this comment.
this is not really a global constant as you're setting pnext and extent to local values. please rename to image_create_info or maybe better, fold it into the loop I'm suggesting below
| .initialLayout = VK_IMAGE_LAYOUT_UNDEFINED, | ||
| }; | ||
|
|
||
| constexpr size_t kNumFormats = 2u; |
There was a problem hiding this comment.
please replace this with an array of VK_FORMATs and iterate over it to build create_infos.
| // Immutable format is technically limited to R8G8B8A8_SRGB but | ||
| // R8G8B8A8_UNORM works with existing ARM drivers so we allow that | ||
| // until we have a more reliable API for creating external Vulkan | ||
| // images using sysmem. TODO(fxb/52835) |
There was a problem hiding this comment.
this commend doesn't make sense when using sysmem. why are you adding this back?
| create_infos[0].format = VK_FORMAT_R8G8B8A8_UNORM; | ||
| format_constraints[1].sysmemFormat = static_cast<uint64_t>(fuchsia::sysmem::PixelFormatType::BGRA32); | ||
| create_infos[1].format = VK_FORMAT_B8G8R8A8_UNORM; | ||
| create_infos[1].flags = VK_IMAGE_CREATE_MUTABLE_FORMAT_BIT; |
There was a problem hiding this comment.
why add this flag? it shouldn't be needed when using sysmem
| const VkImageConstraintsInfoFUCHSIA constraints_info = { | ||
| .sType = VK_STRUCTURE_TYPE_IMAGE_CONSTRAINTS_INFO_FUCHSIA, | ||
| .pNext = nullptr, | ||
| .createInfoCount = kNumFormats, |
|
We found a better way to solve this. In the driver. So this is no longer needed. |
5 participants
Change #23488
has switched all the Vulkan surfaces on Fuchsia to use
RGBA8 color type. However, Using RGBA8 format on Intel GPUs
has caused graphical artifacts on emulators
(See http://fxbug.dev/70232).
This change modifies vulkan_surface.cc to use the multi-format
vkSetBufferCollectionImageConstraintsFUCHSIA() Vulkan API,
so that it could specify multiple formats when allocating
buffer collections.
Bug: fxbug.dev/70232