Igalia / wolvic

A fast and secure browser for standalone virtual-reality and augmented-reality headsets.
https://wolvic.org
Mozilla Public License 2.0
796 stars 100 forks source link

Additional color format checks #1396

Closed javifernandez closed 4 months ago

javifernandez commented 4 months ago

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.

javifernandez commented 4 months ago

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.

svillar commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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