SwapchainBuilder format_feature_flags default doesn't seem appropriate
ovatonne opened this issue · 7 comments
The default format feature flag in SwapchainBuilder specified in this line doesn't make sense to me.
vk-bootstrap/src/VkBootstrap.h
Lines 844 to 846 in 5b15f6e
Because the default image usage flag is VK_IMAGE_USAGE_COLOR_ATTACHMENT_BIT, the default format feature should be VK_FORMAT_FEATURE_COLOR_ATTACHMENT_BIT instead of VK_FORMAT_FEATURE_SAMPLED_IMAGE_BIT.
Also, I think for bootstraping Vulkan applications, swapchain readback by sampling the image is not the very first use-case one wants to consider.
By default, this prevents any format that is "write/transfer-only" to be used. I think this never happens on any implementation (usually formats can be sampled but not written to optimally), but still this default doesn't seem right.
Furthermore, there are no checks done during the selection to verify that the requested image usage is possible with the format. This should be caught by validation layers at VUID-VkSwapchainCreateInfoKHR-imageFormat-01778 (sorry, I don't know how to link to the correct line), but because most of this library checks valid usages, this verification seems missed.
If I understand properly I think the functions to set the desired VkFormatFeatureFlags shouldn't be used. The VkFormatFeatureFlags should only be a mean to verify that the VkImageUsageFlags set by set_image_usage_flags
are supported by the desired formats.
Disclaimer: This is the first time I create an issue on a Vulkan github. Sorry if I am missing something in my understanding or explanation.
If any of the above makes sense, a plan of action to change the behavior would be from less to more breaking:
- Deprecating the use of the requested format feature flag
- Changing the default
- Not using the provided format feature flag
- Checking VUID-VkSwapchainCreateInfoKHR-imageFormat-01778 during the format selection, and return an error no format supports the provided image usage.
I would be happy to make a MR if you think some of the proposed changes are reasonable.
Yes, this looks like a wrong default. That definitely should be changed.
Furthermore, there are no checks done during the selection to verify that the requested image usage is possible with the format.
This should be caught by validation layers, and there is no clean way (that I know of) to validate the users choice here, so I rely on the user providing a 'sane' choice.
VkFormatFeatureFlags and VkImageUsageFlags are two separate things from my understanding, though I am by no means an expert. It certainly is true that format feature flags are used to check the formats while usage flags is just passed on through. So I am not against revising this bit of the code at all, just need more information about what these two types represent or a PR to compare against.
FYI for something like this, breaking the API is fine since I consider it a low-use part of the API. Especially if we move from potentially useless behavior to more correct & more helpful behavior.
Yes, PR's are welcome here! :D
Okay, I have looked into the specifics, and I concur with your assesment. Vk-bootstrap shouldn't allow users to set the format feature flags, only the format & the image usage. Then vk-bootstrap should do some validation that the format & usage are valid. Not sure if vk-bootstrap can do extensive validation (as I don't want to re-implement what exists in the validation layers) but I imagine basic 'must meet VUID's that are in swapchain create info are easy enough.
Hi, thanks for your investigation!
I also read the spec about this again, and I am almost sure the full validation is not possible using only usage flags because there is not a 1-1 mapping with feature flags. I think some features can only be checked against later, by validation (not sure though)
At the very least, the default flag could be changed.
I will take a better look into this at the start of next week. Do you prefer discussing a solution here first, or start with a PR ?
Shoot for a PR cause then we can talk about specifics rather than just 'general ideas'.
I created PR #170 because I had this code lying around (for a different, much bigger PR) but figured it should be separated out and brought in on its own, to serve as a basis for your PR.
I think you can close this. It is solved in my opinion.
Oops thanks for reminding me!