Add Swappy & Pre-Transformed Swapchain

[darksylinc]

• Adds Swappy for Android for stable frame pacing
• Implements pre-transformed Swapchain so that Godot's compositor is in charge of rotating the screen instead of Android's compositor (performance optimization for phones that don't have HW rotator)

============================

The work was performed by collaboration of TheForge and Google. I am merely splitting it up into smaller PRs and cleaning it up.

Changes from original PR:

• Removed "display/window/frame_pacing/android/target_frame_rate" option to use Engine::get_max_fps instead.
• Target framerate can be changed at runtime using Engine::set_max_fps.
• Swappy is enabled by default.
• Added documentation.
• enable_auto_swap setting is replaced with swappy_mode.

---

• Production edit: This closes https://github.com/godotengine/godot-proposals/issues/2351.

[darksylinc]

A little bit of background from TheForge's PR. They introduced the following:

• Swappy, to improve frame pacing (I do not know about XR, but on regular devices Swappy makes a huge difference).
• Pre-transformed swapchain: This makes a tiny bit of performance difference on devices that do not have a HW rotator. For this to work Godot needs the return value of get_current_screen_rotation.
• Thermal code.

The most controversial change is likely the Thermal code:

• It is not actually used, except for logging.
  • Thermal events help when troubleshooting performance and pacing issues, because phones tend to throttle themselves when warm or hot. That's the main reason of its inclusion. Perhaps there could be a better use for it in the future; but were it not for its debugging usefulness, it's borderline a solution looking for a problem.
• It's not exposed to GDScript (should it? is there a use case of it?), but it is exposed in C++.
• I suspect TheForget didn't know how/where to place them other than the main activity (after all, we must start listening in onCreate()). TBH I didn't know what to do with it either.

[darksylinc]

CI is blocking the PR on the basis libswappy_static.a is blocked from being pushed.

The traditional way of adding Swappy is by adding the library after downloading the AGDK from Android.

However the AGDK is FOSS (Apache License), but it is a PITA to build from source code.

How do you want to proceed? Include *.a file as an exception? Build from source?

[bruvzg]

How do you want to proceed? Include *.a file as an exception? Build from source?

We are using separate repos for slow/hard to build libraries: Mesa/NIR, MoltenVK and ANGLE, it's build by from source, but main repo CI (and release build system) simply download release binaries (and there's build flags to override path to these).

[darksylinc]

Ah I remember those repos.

How should I proceed? Do I prepare a repo, add it to github and then if it's Ok'ed it you push it to godotengine? And SCons should always download (and cache) the lib when building for Android?

[bruvzg]

How should I proceed? Do I prepare a repo, add it to github and then if it's Ok'ed it you push it to godotengine?

That's what was done for these repos. When PR is merged, you can transfer repo to the godotengine organization.

And SCons should always download (and cache) the lib when building for Android?

For the other libs, SCons is not downloading anything, there's a separate script which is called by CI.

[darksylinc]

OK after some fighting with everything, I managed to put up https://github.com/darksylinc/godot-swappy/ , have the CI download the prebuilt version from there and now CI is passing.

I will now apply/fix some of the suggestions; but I have something to ask.

This change undoubtedly changes how Android is built from source, from a regular user perspective, i.e. a user trying to build from source will fail to link because of missing swappy lib.

Thus I have the following questions:

1. Should Scons detect if Swappy is there and disable Swappy supports if it's not included?
   • Or simply refuse to start building and tell the user to download Swappy? (like it asks for NIR if D3D12 support is enabled)
2. Do I have to submit a separate PR to add the step of downloading Swappy in the documentation?.
3. Should Scons instead try to detect if Swappy is there, and if it's not, download it from the internet?

[clayjohn]

@darksylinc My preference is for 1.

I think the simplest thing for you to do will be to make swappy a build option (like ANGLE). By default, we would build official builds with swappy=yes. But end users would default to swappy=no.

[darksylinc]

My preference is for 1.

I think the simplest thing for you to do will be to make swappy a build option (like ANGLE). By default, we would build official builds with swappy=yes. But end users would default to swappy=no.

Done!

I added disable_swappy CLI setting. It defaults to False. Thus users must compile with disable_swappy=True if they don't have Swappy and want to ignore the dependency.

1. If users try to compile without Swappy and without disable_swappy=True they'll be met with an error explaining they must either download Swappy and unpack it, or compile with disable_swappy=True
2. If users try to compile without Swappy and with disable_swappy=True they'll be met with a warning that Swappy was not found and they should download Swappy and unpack it. But compilation will continue and succeed.

[m4gr3d]

@darksylinc My preference is for 1.

I think the simplest thing for you to do will be to make swappy a build option (like ANGLE). By default, we would build official builds with swappy=yes. But end users would default to swappy=no.

@darksylinc Based on @clayjohn's comment, shouldn't disable_swappy=True be the default behavior?

Unless I'm misunderstanding the setup, I'd lean to have that being the default behavior as well so that Android builds from source are not 'broken' after this PR is merged. As Clay mentioned though, we should update https://github.com/godotengine/godot-build-scripts/blob/main/build-android/build.sh to enable building the official builds with swappy enabled.

[darksylinc]

Hi!

@darksylinc My preference is for 1. I think the simplest thing for you to do will be to make swappy a build option (like ANGLE). By default, we would build official builds with swappy=yes. But end users would default to swappy=no.

@darksylinc Based on @clayjohn's comment, shouldn't disable_swappy=True be the default behavior?

Unless I'm misunderstanding the setup, I'd lean to have that being the default behavior as well so that Android builds from source are not 'broken' after this PR is merged. As Clay mentioned though, we should update https://github.com/godotengine/godot-build-scripts/blob/main/build-android/build.sh to enable building the official builds with swappy enabled.

I initially thought the same, but now I strongly recommend that disable_swappy=False be the default behavior, due to the following:

1. Unlike D3D12 and other similar features, it is not obvious when Godot was compiled without them. No D3D12? Oh that's obvious. The project will fail to launch with D3D12, the option in the Editor won't be available. It's immediately obvious it was not included. But Swappy? It's too easy not to notice it. Android's behavior will be exactly the same except for poor frame pacing (i.e. stutter), which psychologically can be misattributed to a number of things: slow phone, device temperature / throttling behaviors, unoptimized Godot code, unoptimized user code, poor LOD settings, driver issues, poor CPU/kernel scheduling, power saving behavior, apps in the background, etc. Even with Swappy enabled the things I mentioned can cause poor frame pacing (like in any other platform, though throttling & CPU scheduling tend to be much bigger issues on Android) which can make Swappy look like snake oil. But without Swappy stutter will happen even when everything else is not a problem. All the user will see is: The Editor launches. The project exports. The game runs on all supported devices. There's no apparent indication that something's wrong because the user compiled without Swappy.
2. I modified the Scons files so that users are immediately told of what happened (rather than having a bizarre clang compile error with no obvious way of how to fix) and telling them how to fix it (either compile with disable_swappy=False or download the dependency and unpack it).
3. Due to point 1, by "shoving" it into the user's build it becomes conscious decisions: basically I want to say "You're building for Android. Without Swappy. Are you sure?". Perhaps we should print the explanation from point 1 point in SCons. Explain far more thoroughly to the one building from source why Swappy is important.
4. This issue can accidentally creep into official builds. Failing to include disable_swappy=False into official build scripts CI can stay unnoticed for a long time. In the future, any new maintainer who needs to refactor the build scripts can incorrectly think disable_swappy=False is not needed or miss it by mistake.

[Calinou]

Also, the setting should probably be swappy=yes|no to match how other settings are worded (in a positive fashion, e.g. vulkan=yes|no).

[darksylinc]

Also, the setting should probably be swappy=yes|no to match how other settings are worded (in a positive fashion, e.g. vulkan=yes|no).

I absolutely agree. It's just that I saw other disable_* settings and thought that was the norm.

[m4gr3d]

Unlike D3D12 and other similar features, it is not obvious when Godot was compiled without them. No D3D12? Oh that's obvious. The project will fail to launch with D3D12, the option in the Editor won't be available. It's immediately obvious it was not included. But Swappy? It's too easy not to notice it. Android's behavior will be exactly the same except for poor frame pacing (i.e. stutter), which psychologically can be misattributed to a number of things: slow phone, device temperature / throttling behaviors, unoptimized Godot code, unoptimized user code, poor LOD settings, driver issues, poor CPU/kernel scheduling, power saving behavior, apps in the background, etc. Even with Swappy enabled the things I mentioned can cause poor frame pacing (like in any other platform, though throttling & CPU scheduling tend to be much bigger issues on Android) which can make Swappy look like snake oil. But without Swappy stutter will happen even when everything else is not a problem. All the user will see is: The Editor launches. The project exports. The game runs on all supported devices. There's no apparent indication that something's wrong because the user compiled without Swappy.

No regular users compile Godot from source on their own, they'll get access to the official builds which will have swappy enabled. Whereas this will be a blocker for folks that actually build the editor from source (and an annoyance for folks building from source that don't need / use swappy (e.g: OpenXR)) and unfortunately not everyone stay up-to-date with github PRs, so this will lead to an influx of build 'issues'.

It's immediately obvious it was not included. But Swappy? It's too easy not to notice it.

Let's put the swappy project settings under a #if defined(SWAPPY_FRAME_PACING_ENABLED). This way, if swappy is not included in the build, the project settings will be missing which will be a clear signal.

This issue can accidentally creep into official builds. Failing to include disable_swappy=False into official build scripts CI can stay unnoticed for a long time. In the future, any new maintainer who needs to refactor the build scripts can incorrectly think disable_swappy=False is not needed or miss it by mistake.

Switching to a 'positive' setting should help here a bit, but this is a good callout that we should have a mean (preferably automatic) to validate whether the behavior is enabled or not. Are there any suggestions in the swappy documentation of tests that could be used to validate swappy's impact when it's enabled, and thus that we could use to detect whenever the swappy implementation is accidentally disabled or broken?

cc @Calinou I wonder if we can update the benchmarks to take swappy into account on Android?

[darksylinc]

OK I've done some changes to the PR:

1. disable_swappy=no is now swappy=yes (options shouldn't be expressive as negatives).
2. Expanded on the warning when Swappy is not found so that we explain why it is important (and not just say it is important / recommended).
3. Swappy is disabled by default BUT if "production=yes" is found, it defaults to enabled. I hope this is a reasonable compromise. Production builds should be held a higher standard, even if that occasionally breaks other build scripts.
4. Other misc / minor fixes pointed out by reviewers of the PR.

cc @Calinou I wonder if we can update the benchmarks to take swappy into account on Android?

I don't know how to implement that. I mean in the sense we can do:

if( production && !swappy )
   fail_tests();

But this should break other people's automated builds, whether it's at build time or at benchmark time.

If by "benchmark", you mean actually testing on the device and check framerate variance, then yes. Any benchmark should check:

double avg_fps, std_dev_fps;

diff = abs( monitor_fps - avg_fps );

if( diff / monitor_fps > threshold )
   fail();

if( std_dev_fps > std_dev_threshold )
   fail();

And then make sure the thresholds are low enough to fail without Swappy but high enough pass with Swappy.

Update: Rather than monitor refresh rate, target FPS should be used. This can be tweaked via Engine::set_max_fps

[clayjohn]

Thank you!!!!

[Alex2782]

I first tried with "Source code", there were only the README and LICENSE files in there. The correct file is "godot-swappy.7z". A more detailed description might be good later.

        print_warning(
            "Swappy Frame Pacing not detected! It is strongly recommended you download it from https://github.com/darksylinc/godot-swappy/releases and extract it so that the following files can be found:\n"
            + " thirdparty/swappy-frame-pacing/arm64-v8a/libswappy_static.a\n"
            + " thirdparty/swappy-frame-pacing/armeabi-v7a/libswappy_static.a\n"
            + " thirdparty/swappy-frame-pacing/x86/libswappy_static.a\n"
            + " thirdparty/swappy-frame-pacing/x86_64/libswappy_static.a\n"
            + "Without Swappy, Godot apps on Android will inevitable suffer stutter and struggle to keep consistent 30/60/90/120 fps. Though Swappy cannot guarantee your app will be stutter-free, not having Swappy will guarantee there will be stutter even on the best phones and the most simple of scenes."

[Alex2782]

Does anyone know which Vulkan extensions are necessary for this?

Samsung Galaxy Tab S6 lite. Cannot initialize "VULKAN". https://forum.godotengine.org/t/code-keeps-duplicating-when-i-click-it/87411/12

Seems to work on my Samsung Tab S7. But the (custom) splash screen is rotated 180° when Vulkan renderer (Forward+ / Mobile) is used. Tested with "Starter Kit City Builder", original splash screen is not affected.

#if defined(SWAPPY_FRAME_PACING_ENABLED)
	if (swappy_frame_pacer_enable) {
		char **swappy_required_extensions;
		uint32_t swappy_required_extensions_count = 0;
		// Determine number of extensions required by Swappy frame pacer.
		SwappyVk_determineDeviceExtensions(physical_device, device_extension_count, device_extensions.ptr(), &swappy_required_extensions_count, nullptr);

[darksylinc]

Hi @Alex2782 !

Are you saying you've nailed down the problem to this specific PR?

Assuming the answer is yes, there's two things going on Swappy and Pre-Transformed swapchain.:

1. Swappy requires the extension VK_GOOGLE_display_timing. If it's not available, Swappy is turned off (or at least should be turned off). It's more likely though, that VK_GOOGLE_display_timing is just buggy on your drivers and should be blocklisted.
2. The splash screen issue seems to be a silly bug with pre-transformed swapchains that we missed (Godot is now in charge of rotating the screen instead of the OS). Sometimes the problem is with Android which reported the wrong orientation and Godot is now stuck processing and can't query again for the orientation.

[Alex2782]

I will check it more closely swappy=yes/no on Firebase Test Lab to see whether and how many devices are affected.

[m4gr3d]

@darksylinc I'm also seeing the inverted splash screen issue. I observed it on a debug build of the Android editor with swappy disabled running on Samsung Galaxy tab S8 using Kenney's FPS starter sample.

Do you think that can be addressed before the dev4 release?

[Alex2782]

On Firebase Test Lab I couldn't find any differences between swappy=yes and swappy=no (with custom splash screen Kenney's FPS starter sample, but with Godot 4.3 stable it is correct)

However, the user kibby_Nibb says that with swappy=no the Android Godot editor no longer shows Vulkan errors. I asked about the project if it could be shared.

[darksylinc]

Hi!

I don't know about the schedule but as for the problem itself, luckily there are not many things that can go wrong:

After this PR Godot sets:

VkSurfaceTransformFlagBitsKHR surface_transform_bits = surface_capabilities.currentTransform;

Which is basically saying "we're in charge of rotating the final image, not the OS".

Afterwards to accomplish this we added the following:

blit.push_constant.rotation_cos = Math::cos(screen_rotation);
blit.push_constant.rotation_sin = Math::sin(screen_rotation);

Which should be self explanatory: The final blit done by the compositor performs the rotation in the pixel shader.

What can go wrong?

1. The splash is not using the blit pass. I was certain everything Godot renders to the screen goes through that final blit pass from RendererCompositorRD::blit_render_targets_to_screen. If we somehow missed a render path that does not use it, then the screen won't be properly rotated.
   • I don't this is it. I downloaded the project you pointed to and replaced DisplayServer::get_singleton()->screen_get_internal_current_rotation() for a hardcoded "180" on PC, and the splash was inverted. Which means the blit pass must be in use.
2. DisplayServer::get_singleton()->screen_get_internal_current_rotation() is not returning the proper value. This might be some Android shenanigan where the value the OS returns is wrong up until a few seconds or presentations; but the splash screen never gets to see the correct value.
3. Android returned the wrong value in surface_capabilities.currentTransform on boot and will soon be corrected as soon as Godot catches the VK_SUBOPTIMAL_KHR "error" during presentation so that we recreate the swapchain and receive the right surface_capabilities.currentTransform .
4. Somehow the values used to send to the blit pass shader are uninitialized or the data we're pulling from is 1 frame behind (i.e. off by 1) and since nothing has been rendered, it's uninitialized. This should not happen because we're using push constants. But who knows.

As you noticed, Swappy has nothing to do with the inverted splash.

My money is on either 2 or 3, which would be Android being... Android. If so, the solution perhaps may be to try to render a black screen for a single frame, then the splash. Or to try to detect this situation and see how Android is behaving to detect the situation and correct it.

[m4gr3d]

@darksylinc On the Quest, the entire editor is inverted 180 all the time.

I think we may need to take into account that not all Android devices (or forks of Android) will return the correct value. Is it possible to have a fallback where we let the OS be in charge of rotating the final image?

[m4gr3d]

@darksylinc On the Quest, the entire editor is inverted 180 all the time.

I think we may need to take into account that not all Android devices (or forks of Android) will return the correct value. Is it possible to have a fallback where we let the OS be in charge of rotating the final image?

Or are we properly detecting the rotation changes? In Google's documentation, I don't see references to Display#getRotation(); instead this is what they're using: https://developer.android.com/games/optimize/vulkan-prerotation#detect_device_orientation_changes

[darksylinc]

OK I found the source of the problem. I'll submit a PR shortly with the fix.

On the Quest, the entire editor is inverted 180 all the time.

Mmmm I think we are not the ones creating the swapchain on the Quest, and perhaps that's the problem. However I first want to see if the PR's fix will fix the problem on the Quest.

If not, then yes, we'll have to disable the feature for the Quest, since it's a special case.

I think we may need to take into account that not all Android devices (or forks of Android) will return the correct value. Is it possible to have a fallback where we let the OS be in charge of rotating the final image?

I think I made myself not clear: I meant that getRotation() may temporarily return the wrong value. This is normal and expected (you may have experienced as a user that sometimes you tilt the device 90° and an app rotates -90°; then 180°).

If getRotation() always returned the wrong value, no Android app would be running correctly. I know Android is bad, but it is not THAT broken :rofl: .

Anyway, this is the source of the problem but I found an easy solution, that makes things simpler too. I'll elaborate on it on the PR.

[clayjohn]

With respect to the splash screen, it is drawn by RendererCompositorRD::set_boot_image We probably need matching code in their to handle the rotation.

[darksylinc]

PR submitted to fix the "Splash is upside down / 180°" bug: https://github.com/godotengine/godot/pull/98709

It may also fix the Quest issue too (I don't know).

[DarioSamo]

I suspect this PR has added a regression when it concerns the handling of the extent of the swap chain.

https://github.com/godotengine/godot/blob/1bffd6c73b44b85e5889f54e14b2193940cf5bb1/drivers/vulkan/rendering_device_driver_vulkan.cpp#L2888-L2912

VkSwapchainCreateInfoKHR now exclusively uses native_display_size, which is assigned from the value of surface_capabilities.currentExtent. However, right below the code that does this, you can see there was handling for the case that this returns UINT32_MAX.

The handling is now gone and the UINT32_MAX value can go straight to the VkSwapchainCreateInfoKHR without being validated.

Since the logic for this was altered on this PR, I imagine you'll want to take a second look to see at what the intended handling was there. I suspect issues like this are happening due to this change being recent.