OpenXR: Properly skip frame render when the XR runtime is not yet ready

[decacis]

This PR essentially recreates the workaround found here: GodotVR/godot_openxr@484b1a8 to recover when swapchains can't be released.

As mentioned, this is a workaround, but in my opinion is a pretty important one because the issue it fixes prevents people from recording videos of the game among other things, and worst of all, it happens seemingly randomly, which can frustrate players.

I just tried my best to adapt the previous code to the new OpenXR API, so please let me know if something could be done a better way!

Closes #82751

[dhoverml]

I think the issue is that XR_FAILED only checks for return codes < 0. xrAcquireSwapchainImage and xrWaitSwapchainImage can both return > 0 (0 is XR_SUCCESS, and can be checked with the XR_UNQUALIFIED_SUCCESS macro). When receiving non-XR_SUCCESS from either of those functions, then the image swapchain should not be released and xrEndFrame should not be called (I'm surprised you aren't seeing errors for this one too, maybe I did something wrong). It is okay to call xrBeginFrame (which will likely return XR_FRAME_DISCARDED in this scenario) again to restart the frame on the next update loop.

Anyway, instead of potentially destroying/creating image swapchains every frame...

1. if xrAcquireSwapchainImage != XR_SUCCESS, do not xrEndFrame.
2. If xrWaitSwapchainImage != XR_SUCCESS, do not xrEndFrame. On the next frame, skip xrAcquireSwapchainImage and try xrWaitSwapchainImage again.
3. Step '2' can repeat for many frames
4. Once all image swapchains have been acquired and waited, release all image swapchains and xrEndFrame (OpenXRAPI::end_frame() does this).

[BastiaanOlij]

@dhoverml thats really good feedback thanks, I'm going to look into that more closely asap.

I didn't re-apply the workaround we did in Godot 3 because I want this to be fixed properly this time. Just hadn't come high enough on the priority list yet.

I'm hoping Meta is willing to have a closer look to see why this is happening in the first place, but it could indeed be as simple as us not reacting properly on certain return statusses.

[decacis]

I did a quick test with the changes suggested by @dhoverml and they seem to work! From my tests at least, only xrWaitSwapchainImage returns a result > 0 that is not success, and it can happen for a few frames, but checking both xrAcquireSwapchainImage and xrWaitSwapchainImage is better of course.

[BastiaanOlij]

@decacis any chance you have time to adjust your PR to fix it using that return value?

[decacis]

@BastiaanOlij done! I tested it with both Vulkan Mobile and GL Compatibility.

By the way, I found that the only way to reproduce the issue consistently is to turn off the screen and turn it back on. Recording triggers it very rarely, taking a screenshot has never triggered it for me and neither has switching to passthrough by double tapping (all of this without the new changes, of course)

[BastiaanOlij]

@decacis Hmm, this is pretty interesting, we've already called xrBeginFrame here, so if we skip calling xrEndFrame alltogether we get a mismatch between begin frame and end frame.

I think you can simplify the check to:

if (!XR_UNQUALIFIED_SUCCESS(result)) {
	// Make sure end_frame knows we need to submit an empty frame
	frame_state.shouldRender = false;

	if (XR_FAILED(result)) {
		// Unexpected failure, log this!
		print_line("OpenXR: failed to acquire swapchain image [", get_error_string(result), "]");
		return false;
	} else {
		// In this scenario we silently fail, the XR runtime is simply not ready yet to acquire the swapchain.
		return false;
	}
}

This way we do get a closing xrEndFrame but one that doesn't submit buffers.

It might also be safer to add skip_acquire_swapchain to our OpenXRSwapChainInfo struct. While it is unlikely we can acquire the color texture but not the depth texture, it would prevent issues with a mismatch.

[decacis]

@BastiaanOlij thank you for the feedback. It looks way cleaner!

[BastiaanOlij]

I think it would be good to wait a day or two before merging to get some feedback from others to see if this indeed resolves the issue. I've asked @Malcolmnixon to test as well as he ran into this issue this week too.

But other than getting confirmation this fixes the issue, this looks really good.

Thanks @decacis !

[dhoverml]

This way we do get a closing xrEndFrame but one that doesn't submit buffers.

It should be okay to call xrBeginFrame without a previous call to xrEndFrame, since that means the previous frame should be discarded (xrBeginFrame should return XR_FRAME_DISCARDED in this case).

[Malcolmnixon]

I just tested this and it fixes the record issue for me.

[akien-mga]

Thanks! And congrats for your first merged Godot contribution 🎉

[YuriSizov]

Cherry-picked for 4.1.3.