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

Additional color format checks #1396

Closed
wants to merge 1 commit into from
Closed

Conversation

javifernandez
Copy link
Member

We already have a function to ensure that the XrSwapchainCreateInfo structure provides a compatible color format. We were using this check when creating the swapchains for the views, as part of the GetSwapChainCreateInfo() logic,

However, when we create the OpenXRLayerCube instance we pass a color format Id that is later used in the XrSwapchainCreateInfo structure during the layer initialization. This PR provides a more descriptive error and avoids the failure on the OpenXR code.

Implemented a utility function to call the SupportsColorFormat
check, with additional logging about the selected color format.

Ensure that selected color format is supported by the runtime's
swapchain.
Copy link
Member

@svillar svillar left a comment

Choose a reason for hiding this comment

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

Hmm I don't see how we could pass an unsupported color format there. Mind giving an example?

@javifernandez
Copy link
Member Author

Hmm I don't see how we could pass an unsupported color format there. Mind giving an example?

We decide the layerCube's color format in BrowserWorld::CreateSkybox based on the extension of the images. In the case of ktx we assign to the glFormat variable the GL_COMPRESSED_RGB8_ETC2 value.

The glFormat variable is passed as one of the DeviceDelegateOpenXR::CreateLayerCube parameters, which is then assigned as attribute of the OpenXRLayerCube and VRLayerCube classes.

Finally, as part of the initialization of the cubeLayer, we use it create the XrSwapchainCreateInfo structure for the swapchain.

@javifernandez javifernandez requested a review from svillar May 3, 2024 07:58
@svillar
Copy link
Member

svillar commented May 3, 2024

Hmm I don't see how we could pass an unsupported color format there. Mind giving an example?

We decide the layerCube's color format in BrowserWorld::CreateSkybox based on the extension of the images. In the case of ktx we assign to the glFormat variable the GL_COMPRESSED_RGB8_ETC2 value.

The glFormat variable is passed as one of the DeviceDelegateOpenXR::CreateLayerCube parameters, which is then assigned as attribute of the OpenXRLayerCube and VRLayerCube classes.

Finally, as part of the initialization of the cubeLayer, we use it create the XrSwapchainCreateInfo structure for the swapchain.

Yeah I know the codepath, the thing is why would we ever pass an unsupported value? As you mentioned is something that we control in the code, not something that could depend on some variable.

@javifernandez
Copy link
Member Author

Hmm I don't see how we could pass an unsupported color format there. Mind giving an example?

We decide the layerCube's color format in BrowserWorld::CreateSkybox based on the extension of the images. In the case of ktx we assign to the glFormat variable the GL_COMPRESSED_RGB8_ETC2 value.
The glFormat variable is passed as one of the DeviceDelegateOpenXR::CreateLayerCube parameters, which is then assigned as attribute of the OpenXRLayerCube and VRLayerCube classes.
Finally, as part of the initialization of the cubeLayer, we use it create the XrSwapchainCreateInfo structure for the swapchain.

Yeah I know the codepath, the thing is why would we ever pass an unsupported value? As you mentioned is something that we control in the code, not something that could depend on some variable.

Well, we select the color format depending on the skybox's image file extension, which can be come externally via the remote environments. Also, this code is multi-platform, so it'd might hit in some devices but not in others. The alternative is to let the OpenXR calls to fail, but I think the error can be less clear and harder to figure out its root cause.

@svillar
Copy link
Member

svillar commented May 13, 2024

We were in doubts to land this as the benefit is questionable. Let's close this now, we can always retake it in the future

@svillar svillar closed this May 13, 2024
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.

2 participants