Vulkan rendering support on Android by pouleyKetchoupp · Pull Request #36919 · godotengine/godot

pouleyKetchoupp · 2020-03-08T21:53:04Z

This change enables Vulkan rendering for Android, based on the SurfaceView implementation from PR #36603. There are still some issues to solve, but as a first step Android is now functional again on the 4.0 branch.

Only Vulkan rendering is supported for now, but I've kept the old GL implementation along with this changes, so it should be easy to enable GLES2 again when it's ready.

CC @RandomShaper @m4gr3d @akien-mga @reduz

Remaining issues:

Missing 3D support (needs a new renderer implementation & shaders)
Shaders currently fail because of descriptor set limits (tested on Galaxy Note 5):

ERROR: On shader stage 'Vertex', uniform 'transforms' uses a set (4) index larger than what is supported by the hardware (4).
   at: shader_create (drivers\vulkan\rendering_device_vulkan.cpp:3728) - Condition "set >= limits.maxBoundDescriptorSets" is true. returned: RID()

Colors are off, maybe a texture format issue?
Example with TextureRect :

Normal	Android

Vulkan validation layers are not working (fails on vkGetInstanceProcAddr)
It's disabled on Android for now to avoid crashes.
Errors when using format list in RenderingDeviceVulkan::texture_create
vmaCreateImage fails with VK_IMAGE_CREATE_MUTABLE_FORMAT_BIT (returns VK_ERROR_OUT_OF_HOST_MEMORY)
It's disabled on Android for now to avoid crashes.
Errors in RenderingDeviceVulkan::compute_pipeline_create
vkCreateComputePipelines returns VK_ERROR_INITIALIZATION_FAILED in some cases

ERROR: vkCreateComputePipelines returned error -3
   at: compute_pipeline_create (drivers\vulkan\rendering_device_vulkan.cpp:5143) - Condition "err" is true. returned: RID()

Missing VR support
There's specific initialization for GL, based on xrMode, it's not handled for Vulkan.
Missing fallback to GLES2
Building templates with Android API 18 to 23
Vulkan support should be disabled in this case.
~~Properly handle onVkSurfaceChanged event to update the rendering system~~
~~Right now, pausing and resuming the app causes it to restart completely instead.~~
edit: addressed in Proper surface reset when resuming app on Android #39004

Implementation Notes:

New Android SDK requirements for building templates (Vulkan headers and libs):
NDK version: 20.0.5594570
Min Android API: 24
The Vulkan loader is taken from the Android NDK, because the one we're using for other platforms doesn't support Android platform.
[Android] wrong signatures for terminator_CreateAndroidSurfaceKHR() and vkCreateAndroidSurfaceKHR() KhronosGroup/Vulkan-Loader#96
VkRenderer calls GodotLib directly, so I got rid of vk_renderer_jni. It seems easier to have one common place for calls from GL or Vulkan implementations.
Fix for GodotInstrumentation in AndroidManifest.xml
This is used to force the app to restart.
I've kept the same hack that was used for GL to restart the whole app after paused, but we might want to improve it later by restarting only the rendering system.
Plugins:
Added vulkan specific callbacks in addition to the GL ones, but maybe the interface could be harmonized between the two to make it easier for the user.

Bugsquad edit: Fixes #36919.

pouleyKetchoupp · 2020-03-09T12:19:19Z

@akien-mga Thanks for the review! I've just pushed some changes based on your comments.

akien-mga

Buildsystem and C++ changes look good. I'll let @m4gr3d review the Java/Kotlin parts more in-depth.

reduz · 2020-03-09T18:50:17Z

I think this is OK, although it may need to be refactored to use the DisplayServer code that will be merged in a week or two.

m4gr3d · 2020-03-17T14:23:38Z

This has to be invoked on the rendering thread; I imagine you moved it here in order to be able to access the value for the video driver.
Instead, in export.cpp, can you push the driver type as a command line argument the same way we do it for the xrMode.

Yes that's the reason.

I can do it by command line instead as you suggested, but in this case we'll have less options when we want to handle the fallback to OpenGL when Vulkan is not supported on the device (at this point I'm not sure if we'll do that in c++ or java).

This part of the code is supposed to handle the case where Main::setup is called on the UI thread and it seems to work fine so far:

godot/platform/android/java_godot_lib_jni.cpp

Lines 197 to 199 in 989a223

// Since Godot is initialized on the UI thread, _main_thread_id was set to that thread's id,

// but for Godot purposes, the main thread is the one running the game loop

Main::setup2(Thread::get_caller_id());

What part of the setup do you think is going to cause problems when not run on the render thread?

My main worry is regarding thread safety since setup initializes some data structures that setup2 and the rest of the logic access at runtime.
With your changes, setup concludes before the render thread is started, so data access should be safe, especially given that Godot's data structures should be thread-safe.

@akien-mga @reduz It would be good to have someone familiar with Godot's C/C++ thread safety validates these changes as well.

I think there should not be a problem calling setup2() on a different thread as setup(), if setup2() will later become the rendering thread. That said, eventually we need to look at some point at optionally supporting a dedicated rendering thread on Android, given this works on Godot already in the rest of the platforms.

Ah nevermind, this will probably need to be changed and probably done again in setup2.
https://github.com/godotengine/godot/blob/master/main/main.cpp#L404

Ah nevermind, this will probably need to be changed and probably done again in setup2.
https://github.com/godotengine/godot/blob/master/main/main.cpp#L404

This is already done in https://github.com/godotengine/godot/blob/master/platform/android/java_godot_lib_jni.cpp#L201.

I think there should not be a problem calling setup2() on a different thread as setup(), if setup2() will later become the rendering thread. That said, eventually we need to look at some point at optionally supporting a dedicated rendering thread on Android, given this works on Godot already in the rest of the platforms.

@reduz The thread that setup2() is invoked on is the dedicated rendering thread. Or were you referring to something else?

oh no, it's the main thread for Godot. On android there is no render thread support, so you should assume both render and main should be the same for now.

I'm sure it could eventually be done, but I would get everything to work first.

m4gr3d · 2020-03-17T14:45:27Z

I'd rename the interface to GodotRenderView or RenderView for short, since it's not typical to have the type as a suffix to the name.

m4gr3d · 2020-03-17T14:53:38Z

To be consistent with Android naming pattern, can this instead be GodotGLView or GodotGLRenderView to match the interface name.

m4gr3d · 2020-03-17T14:56:23Z

To be consistent, these should be onActivityPaused and onActivityResumed.

m4gr3d · 2020-03-17T14:57:14Z

To be consistent with Android naming pattern, can this be renamed to GodotVulkanView or GodotVulkanRenderView to match the interface name.

m4gr3d · 2020-03-17T15:02:01Z

I'd merge these with the GL* callbacks by renaming them to make them generic. For example, onRenderDrawFrame, onRenderSurfaceChanged, onRenderSurfaceCreated.

Do you mean that the common functions would take both a Surface and a GL10 as arguments, and one of them would be null depending on the renderer used?

Or maybe we could simply pass a SurfaceView argument instead, but I don't have an example of these callbacks being used so I'm not sure what the actual use cases are.

On second though let's keep them separate since passing null values seems like a hack as well.
Given that GLESv2 will remain, this might be useful for plugins that'd like to support one but not the other.

There are a couple other methods that use the onGL prefix that should be updated though:

onGLRegisterPluginWithGodotNative

onGLGodotMainLoopStarted

I'll provide a PR to update the naming for these since I'd like these changes to be in as soon as possible so as to be stable enough for plugin writers to start using them.

m4gr3d · 2020-03-17T15:02:48Z

You can use val here since the variable is final.

m4gr3d · 2020-03-17T15:14:56Z

The method name should be updated to be camelCase (surfaceCreated).

In the native layer, the logic for this method is almost identical to newcontext. The two should be merged into one.

m4gr3d · 2020-03-17T15:23:22Z

It's likely that more vulkan logic will be added inside and outside of this class. Can you restore the vulkan directory and keep all android platform vulkan specific logic (including this class) in there.

m4gr3d

@pouleyKetchoupp Nice job! I've added some feedback to be addressed.

pouleyKetchoupp · 2020-03-19T23:26:34Z

Thanks @m4gr3d! I've just pushed some changes that should address your previous comments.

So here are the remaining things to do before this PR can be merged:

Validate it's safe to call Main::setup() on the UI thread, then Main::setup2(Thread::get_caller_id()); on the render thread after it's created
~~Change the calls for plugins after PR Update the naming scheme for the GodotPlugin's methods #37174 is merged~~
~~Refactor the c++ part after the DisplayServer branch is merged~~

m4gr3d

Looks great!

One feedback to address, alongside your comment.

m4gr3d · 2020-03-23T16:58:19Z

newcontext must only be called once in onVkSurfaceCreated (see GodotRenderer for reference).

Ah yes, good point! Right now, I'm not handling the surface re-creation properly, so the workaround is to reset the app by calling newcontext. Instead it should update the surface in the native code, but I haven't looked into that yet.

The consequence is the app gets restarted when paused and resumed again.

It's important to fix and I'll see if I have time for it in the coming days, but I don't think it's a blocker to merge this PR. In the meantime I'll add a comment in the code and in the PR description.

Sounds good.
For the moment, you could then remove the call to newcontext from onVkSurfaceCreated since it's always followed by onVkSurfaceChanged and leave a comment like you mentioned to restore the proper logic flow when surface recreation is properly handled.

reduz · 2020-04-03T14:54:35Z

-	VkImageFormatListCreateInfoKHR format_list_create_info;
-	Vector<VkFormat> allowed_formats;
-
+	// TODO: vkCreateImage fails with format list on Android (VK_ERROR_OUT_OF_HOST_MEMORY)


I suppose we should be able to check for support via RenderingDevice. Can you add a #warning there ? I am sure many mobile GPUs support it and we should be able to take advantage of it.

reduz · 2020-04-03T14:58:28Z

I think there should not be a problem calling setup2() on a different thread as setup(), if setup2() will later become the rendering thread. That said, eventually we need to look at some point at optionally supporting a dedicated rendering thread on Android, given this works on Godot already in the rest of the platforms.

reduz · 2020-04-03T15:01:41Z

Android does support native video, you should be able to port it from 3.x branch

@reduz The 3.x native video implementation is brittle, lack flexibility and possibly not working. The thinking for 4.0 (and 3.2.2) is to provide video support via the revamped Android plugin system instead.

m4gr3d · 2020-04-03T15:39:12Z

Ah nevermind, this will probably need to be changed and probably done again in setup2.
https://github.com/godotengine/godot/blob/master/main/main.cpp#L404

This is already done in https://github.com/godotengine/godot/blob/master/platform/android/java_godot_lib_jni.cpp#L201.

m4gr3d · 2020-04-03T15:40:08Z

I think there should not be a problem calling setup2() on a different thread as setup(), if setup2() will later become the rendering thread. That said, eventually we need to look at some point at optionally supporting a dedicated rendering thread on Android, given this works on Godot already in the rest of the platforms.

@reduz The thread that setup2() is invoked on is the dedicated rendering thread. Or were you referring to something else?

pouleyKetchoupp · 2020-04-03T20:48:14Z

Thanks for the review @reduz! I've just added the #warning you asked for.

If everything's good around the display server code, this PR should be ready to merge.

thebestnom · 2020-04-07T20:49:16Z

after trying to rebase android editor onto this branch, I found some small things that I think should be refactored...
by my understanding and how it looks display server should process everything that the user input and outputs, that means that process_{input} (process_touch for example) should be moved to display_server_android from os_android as well, I can see that when I tried to merge my ( @m4gr3d for now, later I'll try your implementation) that things doesn't go where they should be

I can open a quick PR to this branch if you're interesed

pouleyKetchoupp · 2020-04-07T21:51:34Z

@thebestnom Ok, that makes sense. Thanks for the feedback! I can change it some time this week, but if you decide to do it in the meantime, please let me know and feel free to open a PR :)

thebestnom · 2020-04-07T22:01:45Z

@pouleyKetchoupp working on it :)

thebestnom · 2020-04-07T23:02:22Z

@pouleyKetchoupp hey, I commited everything but it doesn't let me open PR into https://github.com/nekomatata/godot/tree/android-vulkan-rendering

pouleyKetchoupp · 2020-04-07T23:40:27Z

@thebestnom It looks like you would have to actually fork from my repo to be able to do that :/
I'll just cherry-pick the changes from your commit instead.

thebestnom · 2020-04-07T23:44:47Z

It looks like you would have to actually fork from my repo to be able to do that :/

I'm not sure, I can open PR to different forks, I think the problem is that it's team only repo or somthing like that, but it doesn't matter that much...

I wanted to make some comment on the code in the PR... so Ill just say it here,
while I moved most of the things as is, I changed process_event to be process_key_event and moved the implementation from java_godot_ lib_jni to the display server cause it made no sense for it to be there

pouleyKetchoupp · 2020-04-07T23:49:41Z

Ok! I've seen the change with key events and it makes sense too. I've just pushed a new version that includes your changes.

akien-mga · 2020-04-08T22:16:21Z

Some comments from IRC:

23:39 <reduz> m4gr3d[m]: I think it's ok, just to make clear, Main::setup2() , Main::iteration() and the events that come from DisplayServer, should all be in the same thread
23:40 <reduz> m4gr3d[m]: which on android so far is the same thread used by Vulkan/OpenGL to render
23:40 <reduz> m4gr3d[m]: we could move the game logic to an other thread and leave the render thread only for rendering, but that will need some small changes to the engine, and I would probably prefer to implement this once everything else is working

Let's merge as is and refine in future PRs :)

akien-mga · 2020-04-08T22:16:30Z

Thanks!

akien-mga · 2020-04-08T22:29:15Z

Tested a local build, it's not finding vk_mem_alloc.h. #thirdparty/vulkan/ should be added to the CPPPATH.

akien-mga · 2020-04-08T22:37:29Z

Tested a local build, it's not finding vk_mem_alloc.h. #thirdparty/vulkan/ should be added to the CPPPATH.

It works after upgrading the NDK from r20 to r21 as r21/sources/third_party/vulkan/src/layers/vk_mem_alloc.h as been added, so we'll have to document r21 as minimum required version.

m4gr3d · 2020-04-08T22:52:13Z

Tested a local build, it's not finding vk_mem_alloc.h. #thirdparty/vulkan/ should be added to the CPPPATH.

It works after upgrading the NDK from r20 to r21 as r21/sources/third_party/vulkan/src/layers/vk_mem_alloc.h as been added, so we'll have to document r21 as minimum required version.

@akien-mga @pouleyKetchoupp We can specify an NDK version in the build.gradle file as shown here.
We could use that to enforce the NDK version that's used.

pouleyKetchoupp · 2020-04-09T07:42:21Z

@m4gr3d In order to be more flexible when GLES2 is supported again, we could detect the NDK version during the build, and when the requirement is not met, show a warning and disable Vulkan support.

m4gr3d · 2020-04-09T12:51:39Z

@m4gr3d In order to be more flexible when GLES2 is supported again, we could detect the NDK version during the build, and when the requirement is not met, show a warning and disable Vulkan support.

@pouleyKetchoupp that sounds overly complex and I'm not sure what the benefit would be done resolving the correct ndk version is a fairly simple task.

thebestnom · 2020-04-09T21:35:28Z

can't you just use ndkVersion in build.gradle? there is no benefit from using older ndk from my understanding

Anutrix · 2020-05-25T08:00:39Z

Can someone make a tracker issue for all the issues mentioned in first comment?

pouleyKetchoupp · 2020-05-25T08:07:43Z

@Anutrix Ok, I can make one.

pouleyKetchoupp · 2020-05-25T08:16:37Z

@Anutrix Tracker issue: #39033

Calinou added enhancement platform:android topic:platforms topic:rendering labels Mar 8, 2020

pouleyKetchoupp force-pushed the android-vulkan-rendering branch from 7354153 to 9596089 Compare March 8, 2020 22:23

m4gr3d requested review from akien-mga, m4gr3d and reduz March 8, 2020 23:51

akien-mga added this to the 4.0 milestone Mar 9, 2020