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

Clarify xrEnumerateInstanceExtensionProperties #173

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

fredemmott
Copy link
Contributor

@fredemmott fredemmott commented Jul 23, 2024

Per https://registry.khronos.org/OpenXR/specs/1.1/html/xrspec.html#api-initialization , this function can be called before calling xrCreateInstance.

This function may: be called before an instance has been created; implementations must: not assume an instance exists.

Concretely:

  • a bug in past versions of the Ultraleap API layer crashed if an instance had not been created.
  • I've added the paragraph explaining about API layers invoking these, as a past version of the Ultraleap API layer entirely removed their implementation of this and xrEnumerateApiLayerProperties as they appeared to be dead code

@fredemmott
Copy link
Contributor Author

Related loader docs PR: KhronosGroup/OpenXR-SDK-Source#490

@rpavlik-bot
Copy link
Collaborator

An issue (number 2322) has been filed to correspond to this pull request in the internal Khronos GitLab (Khronos members only: KHR:openxr/openxr#2322 ), to facilitate working group processes.

This GitHub pull request will continue to be the main site of discussion.

@rpavlik-bot rpavlik-bot added the synced to gitlab A corresponding issue has been filed in the Khronos internal GitLab label Jul 23, 2024
Comment on lines +189 to +192
When applications invoke this function, the OpenXR loader's implementation
is used, which enumerates the registered runtime and API layer manifests;
this function must: still be implemented by API layers and runtimes, as it may
be invoked by API layers.
Copy link

@rblenkinsopp rblenkinsopp Aug 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I'd also like to see:

Suggested change
When applications invoke this function, the OpenXR loader's implementation
is used, which enumerates the registered runtime and API layer manifests;
this function must: still be implemented by API layers and runtimes, as it may
be invoked by API layers.
When applications invoke this function, the OpenXR loader's implementation
is used, which enumerates the registered runtime and API layer manifests;
this function must: still be implemented by API layers and runtimes, as it may
be invoked by API layers, and must: return the same information as the manifest.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would that belong here, or in the loader docs over on https://github.com/KhronosGroup/OpenXR-SDK-Source/pull/490/files ?

Copy link

@rblenkinsopp rblenkinsopp Aug 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question, I think it belongs here because it's specifying the behaviour of the function rather than the detail of the manifest. It probably should have an accompanying note in the loader docs that mirrors it form the manifest perspective.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem with adding it here is that the main spec doesn't know about the loader - or implementation details of a potential loader like the manifest. It does sound good to add it to the loader spec though.

Copy link
Contributor Author

@fredemmott fredemmott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a few other paths we could/should take, in order of BC-breaking:

Adding a recommendation to always call with an instance (not bc breaking)

Calling without an instance will result in almost-certainly undesired behavior:

  • if the layer is the last layer, it will retrieve all extensions advertised by the runtime
  • if there are later layers, it will retrieve all extensions advertised by only the very next layer - not the runtime's extensions, and not any later layers

This would not be a 'must'. Probably 'should'.

Require an instance - mildly BC-breaking

Layers MUST create an instance before calling this.

Runtimes MUST support multiple instances sequentially (i.e. create -> destroy -> create -> destroy, not CCDD) in order for layers to do this before the app creates its' instance. I believe this is already required

Ban this function in API layers, add xrEnumerateApiLayerInstanceExtensionProperties - severely BC breaking

This would be similar to how API layers must not call or define xrCreateInstance and must use xrCreateApiLayerInstance

This new function would take a mandatory XrInstance property, to incorporate the previous suggestion into the function signature.

Comment on lines +189 to +192
When applications invoke this function, the OpenXR loader's implementation
is used, which enumerates the registered runtime and API layer manifests;
this function must: still be implemented by API layers and runtimes, as it may
be invoked by API layers.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would that belong here, or in the loader docs over on https://github.com/KhronosGroup/OpenXR-SDK-Source/pull/490/files ?

implementations must: not assume an instance exists.

When applications invoke this function, the OpenXR loader's implementation
is used, which enumerates the registered runtime and API layer manifests;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar to my comment below, the spec does not really know anything about a potential loader's implementation. If we have something that we are going to require here, we need to require it of the runtime or of the API layer.


When applications invoke this function, the OpenXR loader's implementation
is used, which enumerates the registered runtime and API layer manifests;
this function must: still be implemented by API layers and runtimes, as it may

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a new unenforceable must: - it was already required that runtimes implemented this function, but it wasn't required for API layers. Is there some path to make this enforceable?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As for making it enforceable:

  • we don't currently have any CTS that's really usable for API layers. Perhaps this should change?
  • we could make the loader refuse to load extensions that do not implement the functions and where the detail doesn't matter. This would have a minor performance cost on startup.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not new via the loader spec; but the loader spec is full of things like this. What's being proposed here is a change to the OpenXR spec where we are much more careful. You can see the difference in style / enforcement in the MRs / PRs where I moved the negotiate functions from the loader spec to the OpenXR spec.

Copy link
Contributor Author

@fredemmott fredemmott Aug 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is new for API layers - it's already a must:

My assumption here is that the 'must:' for API layers in the loader specification are actually requirements for API layers; is this the actually the case, or is this just describing assumptions that Khronos's loader implementation is allowed to make?

If the latter, that implies there's potentially such a thing as a conformant API layer that the Khronos loader is unable to load - that seems undesirable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
synced to gitlab A corresponding issue has been filed in the Khronos internal GitLab
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants