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

VK_EXT_swapchain_colorspace and new constants #442

Closed
tomaka opened this issue Feb 2, 2017 · 14 comments
Closed

VK_EXT_swapchain_colorspace and new constants #442

tomaka opened this issue Feb 2, 2017 · 14 comments

Comments

@tomaka
Copy link
Contributor

tomaka commented Feb 2, 2017

The VK_EXT_swapchain_colorspace extension added new constants in the VkColorSpaceKHR enum.

If you don't enable this extension, can vkGetPhysicalDeviceSurfaceFormatsKHR return any of these new constants?

  • If the function can return any of the new constants, then backward compatibility is broken.
  • If the function can't return any of the new constants, then does that mean that it is mandatory for implementations to support VK_COLOR_SPACE_SRGB_NONLINEAR_KHR (as they have no other choice but to return this value)? If so I think it should be written explicitly in the specs.
@krOoze
Copy link
Contributor

krOoze commented Feb 2, 2017

If you don't enable this extension, can vkGetPhysicalDeviceSurfaceFormatsKHR return any of these new constants?

Of course not. What gave you the idea?

If the function can't return any of the new constants, then does that mean that it is mandatory for implementations to support VK_COLOR_SPACE_SRGB_NONLINEAR_KHR (as they have no other choice but to return this value)?

Obviously (that's how it worked for the past year BTW). Or the implementation can choose not to support presenting at all.

The spec should perhaps say that the vkGetPhysicalDeviceSurfaceFormatsKHR with the extension enabled must return superset of values that would be returned with the extension off. (Am I right to assume it should work like that?)

@tomaka
Copy link
Contributor Author

tomaka commented Feb 2, 2017

Of course not. What gave you the idea?

Nothing in the specs mentions that the extension must be enabled for the new constants to be returned.

Suppose you're writing a Vulkan implementation that presents directly to a monitor, and you're a bit careless, it's easy to not check for the extension's status and simply return the color space of the monitor.

That's why I'm opening the issue. Right now you're supposed to deduce the way it works from various places of the specs, instead of reading clear instructions.

@krOoze
Copy link
Contributor

krOoze commented Feb 2, 2017

Enabling an extension does not change behavior of functionality exposed by the core Vulkan API or any other extension, other than making valid the use of the commands, enums and structures defined by that extension.

Yeah, I suppose the sentence is missing "enumerants".
Technically (being a nitpicker) it is missing the negative case too.

@tomaka
Copy link
Contributor Author

tomaka commented Feb 2, 2017

The other problem is that if you look at this, it's not obvious that all of the constants but one are part of an extension: https://www.khronos.org/registry/vulkan/specs/1.0-extensions/html/vkspec.html#VkSurfaceFormatKHR

@krOoze
Copy link
Contributor

krOoze commented Feb 2, 2017

Technically both are part of an extension. One tell is the suffix. Second tell is the offset of the values.

Sure, it would be pretty if the source extension name would be there (maybe as a C comment before each first enumerant of each extension). (EDIT: made that suggestion now as #444)

On that note I think sometimes the spec normal text is worse and should contain "If X extension enabled" more.

@courtney-g
Copy link

The other problem is that if you look at this, it's not obvious that all of the constants but one are part of an extension

FYI - When the spec is made "with all registered Vulkan extensions", it documents the behavior as if all extensions have been enabled. So, other than the extension listed in the extensions chapter, extension language is integrated into the spec and may not be explicitly called out. As @krOoze mentioned, you can tell they come from an extension by the enumerate suffix.

@courtney-g
Copy link

Enabling an extension does not change behavior of functionality exposed by the core Vulkan API or any other extension, other than making valid the use of the commands, enums and structures defined by that extension.

I've logged an issue on the internal Khronos bug tracker regarding the question of what should an implementation do with extension enums when the extension is not enabled. The app side is clear and the validation layers will complain about use without enable. But it's not as clear if having extension attributes be returned to an application when the corresponding extension is not enabled.

@oddhack
Copy link
Contributor

oddhack commented Mar 19, 2017

We have an internal resolution that queries may return enums and bitfields from extensions that were not explicitly enabled by the app, and some changes to that effect in the 1.0.44 release, but leaving this assigned to @courtney-g to finish closing out the topic here.

@oddhack
Copy link
Contributor

oddhack commented Mar 31, 2017

This should be fixed in the 1.0.46 spec update.

@oddhack oddhack closed this as completed Mar 31, 2017
@krOoze
Copy link
Contributor

krOoze commented Apr 1, 2017

@oddhack That's quite surprising and unorthodox resolution. So I have some Qs about its effect:

  1. So is it actually true? Now any enum returned from Vulkan can be whatever (as long as it is defined in the official vulkan.h) irregardless if its providing extension is enabled or disabled by me? (IMO if so, the new spec Note should say that more directly.) So, does this also mean that vkGetPhysicalDeviceSurfaceFormatsKHR can return arbitrary VkSurfaceFormatKHRs (because it has two enum members) which are not existent in core Vulkan spec?

  2. Can I further use this extension enumerant even if it is not part of any enabled extension? E.g. VkSwapchainCreateInfoKHR's VU says it consumes anything returned by vkGetPhysicalDeviceSurfaceFormatsKHR, so yes? How is it supposed to behave (as if an extension was enabled?)? Or am I supposed to filter out structs with enumerants that I do not know (i.e. those I have not enabled through extensions) before further use?

  3. If I detect an enumerant value from some extension I did not enable, can I treat that extension as enabled to its full extent anyway (e.g. use its pNext structs)?

  4. So drivers can have "hidden" extensions. Does it mean some driver can support some Vulkan+extension combination but have no support for core Vulkan? Without telling me (the user) no less?

  5. Relatedly to 2, vkGetPhysicalDeviceSurfaceFormatsKHR says it must return at least one format — it would be weird if it was one that I have to filter out. Is there some guarantee that that one is actually enum I know? Oppositely if the colorspace extension is enabled am I guaranteed to still get one surface format that the core Vulkan knows? Or can the one (guaranteed) format-colorspace be one of the extended ones?

  6. VkResult is an enum too. Does this practically mean the Return Codes section of the command in the spec is bit pointless? (Seems so, because e.g. VK_ERROR_VALIDATION_FAILED_EXT already behaves that way), Can potentially any command return any VkResult?

@courtney-g
Copy link

So is it actually true? Now any enum returned from Vulkan can be whatever (as long as it is defined in the official vulkan.h) irregardless if its providing extension is enabled or disabled by me? (IMO if so, the new spec Note should say that more directly.) So, does this also mean that vkGetPhysicalDeviceSurfaceFormatsKHR can return arbitrary VkSurfaceFormatKHRs (because it has two enum members) which are not existent in core Vulkan spec?

We spent quite a bit of time debating this topic. Basically it makes queries simpler for the driver. Vulkan is all about keeping the driver as lean as possible. So yes, it is true. Likely to see it affect vkGetPhysicalDeviceSurfaceFormatsKHR as it simplifies that driver code to return all supported formats.
That also means that an application must be tolerant/ignore enums / flags it does not know about.

Can I further use this extension enumerant even if it is not part of any enabled extension?

No. You cannot use the enums without enabling the associated extension. That will cause a validation failure. We still want use by the application be explicit.

For example, vkGetPhysicalDeviceSurfaceFormatsKHR can return a VkSurfaceFormatKHR with a colorSpace of VK_COLOR_SPACE_DISPLAY_P3_NONLINEAR_EXT.
To use that surface format in VkSwapchainCreateInfoKHR, the application must have enabled VK_EXT_swapchain_colorspace. The validation layers will enforce that.

If I detect an enumerant value from some extension I did not enable, can I treat that extension as enabled to its full extent anyway (e.g. use its pNext structs)?

No. Enums from extensions that are not enabled cannot be used (passed as input values to Vulkan calls) by the application.

So drivers can have "hidden" extensions. Does it mean some driver can support some Vulkan+extension combination but have no support for core Vulkan? Without telling me (the user) no less?

I don't understand this question. An application may see enums / flags it does not recognize and must ignore those flags. One of the motivations for this behavior is what happens with applications written to Vulkan 1.0 when they are running on 1.1? Requiring applications to ignore enums / flags they do not recognize makes that situation easier to manage.

Relatedly to 2, vkGetPhysicalDeviceSurfaceFormatsKHR says it must return at least one format — it would be weird if it was one that I have to filter out. Is there some guarantee that that one is actually enum I know? Oppositely if the colorspace extension is enabled am I guaranteed to still get one surface format that the core Vulkan knows? Or can the one (guaranteed) format-colorspace be one of the extended ones?

For vkGetPhysicalDeviceSurfaceFormatsKHR, I believe VK_COLOR_SPACE_SRGB_NONLINEAR_KHR is a required color space. I believe there are required elements that must always be present. Have to look a bit more closely.

VkResult is an enum too. Does this practically mean the Return Codes section of the command in the spec is bit pointless?

I don't think so, though may require clarification.
The behavior we want to enable is allowing the driver to have a static list that it can use for a query. Thus vkGetPhysicalDeviceSurfaceFormatsKHR can always just copy from the static list and doesn't need to filter it based on what extensions are enabled or not.

@krOoze
Copy link
Contributor

krOoze commented Apr 7, 2017

@courtney-g thanks for the answers!

We spent quite a bit of time debating this topic. Basically it makes queries simpler for the driver. Vulkan is all about keeping the driver as lean as possible. So yes, it is true. Likely to see it affect vkGetPhysicalDeviceSurfaceFormatsKHR as it simplifies that driver code to return all supported formats.
That also means that an application must be tolerant/ignore enums / flags it does not know about.

Alright then.
(Hmm, would be interesting if it just returned const reference to its guts, further simplifying driver + eliding copy).

Anyway, AIS could use to be more clear. The added Note seems to try to do that but IMO fails.

What is also not clear is to what degree this property is transitive (i.e. if or when it makes any structure housing that enum value also to be ignored as a whole.)

No. You cannot use the enums without enabling the associated extension. That will cause a validation failure. We still want use by the application be explicit.

Good. I don't see that reflected in the spec though.
It only says about valid enumerant "The enumerant is defined as part of the enumerated type." and then on only uses the "valid" word.
"defined" elsewhere seems to be used pretty much in the C sense. E.g. "an extension defines a new enumerant". I cannot find anywhere that the extension has to be enabled.

I don't understand this question.

I was refering to the wording in the newly added Note (that I criticized in 1. as a whole, so I was probably just extrapolating too much from its unfortunate wording). So probably irrelevant, but anyway I was refering specifically to this: "[...]cases such as 'hidden' extensions known only to driver internals, or layers enabling extensions without knowledge of the application[...]"

For vkGetPhysicalDeviceSurfaceFormatsKHR, I believe VK_COLOR_SPACE_SRGB_NONLINEAR_KHR is a required color space. I believe there are required elements that must always be present. Have to look a bit more closely.

It would not help, because I could be forced to discard the guaranteed one format-colorspace pair because of the format enum too even if VK_COLOR_SPACE_SRGB_NONLINEAR_KHR.
I noticed this super confusing paragraph though:

If pSurfaceFormats includes an entry whose value for colorSpace is VK_COLOR_SPACE_SRGB_NONLINEAR_KHR and whose value for format is a UNORM (or SRGB) format and the corresponding SRGB (or UNORM) format is a color renderable format for VK_IMAGE_TILING_OPTIMAL, then pSurfaceFormats must also contain an entry with the same value for colorSpace and format equal to the corresponding SRGB (or UNORM) format.

Being highly self-referential and circular, I don't know what it means, Though it further discusses something that has to be in the array conditionally.

I don't think so, though may require clarification.
The behavior we want to enable is allowing the driver to have a static list that it can use for a query. Thus vkGetPhysicalDeviceSurfaceFormatsKHR can always just copy from the static list and doesn't need to filter it based on what extensions are enabled or not.

Possibly VkResult should be exception to this then.
Maybe things like VkPhysicalDeviceProperties::deviceType too -- in this case I cannot even discard all those unknown values to get the known ones.

@Carandiru0
Copy link

Well - I get no such validation error for using the enums returned from vkGetPhysicalDeviceSurfaceFormats2KHR , when I have not explicitly enabled the instance extension VK_EXT_swapchain_colorspace.

Vulkan SDK 1.2.154.1

Was this extension internally promoted to no longer be an extension ? Is it built-in now so to speak that explicitly enabling the extension is no longer required to use these colorspace enumerations?

Currently on the "Vulkan Hardware Capability Database" this extension has vendor gpu support of 7.7% vs no support at 92.3%

What is going on here? Is this extension deprecated?

I'm not sure if enabling this extension is required for HDR/WideGamut Colorspaces. These formats are important for implementations supporting true hdr monitors.

@billhollings
Copy link

billhollings commented Jul 13, 2022

Apologies for resurrecting an old discussion...but this is a murky area for implementation requirements.

I read the above discussion to at best indicate that implementations are allowed to return enumerants from extensions that are not enabled, but I don't read it as requiring the implementation to do so.

Is that a correct assumption? Or are implementations expected or required to return all supported enumerants from associated queries, regardless of which extensions are enabled?

EDIT: Based on other feedback, and the general consensus of other implementations, we've modified MoltenVK to return the full set of color spaces, regardless of whether the extension is enabled or not.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants