fixes edge cases in Android Native App rendering #32

mgood7123 · 2021-06-27T18:25:46Z

adapted from https://github.com/151706061/OpenGLSamples/blob/1e871c0c7fe0d8d30825f467bfdc8de50961b6f9/extensions/src/NvAppBase/NvAndroidNativeAppGlue.c#L128

this fixes possible rendering edge cases mentioned in https://developer.nvidia.com/fixing-common-android-lifecycle-issues-games

this additionally fixes an uncommon crash when the assistant is brought up over the application
this additionally fixes an uncommon crash when the application is switched from the top split screen to the bottom split screen, in Samsung One UI devices

TheMostDiligent · 2021-06-27T18:36:49Z

NativeApp/include/Android/AndroidAppBase.hpp

@@ -81,15 +84,39 @@ class AndroidAppBase : public AppBase
    android_app* app_ = nullptr;
    std::string  native_activity_class_name_;

+    pthread_mutex_t mutex;
+    pthread_cond_t cond;


What is this for? Nothing waits on this conditional

TheMostDiligent · 2021-06-27T18:37:14Z

NativeApp/include/Android/AndroidAppBase.hpp

@@ -81,15 +84,39 @@ class AndroidAppBase : public AppBase
    android_app* app_ = nullptr;
    std::string  native_activity_class_name_;

+    pthread_mutex_t mutex;


It is better to use std::mutex

ok, though i think std::mutex is slightly slower than pthread_mutex in terms of overhead, but i will switch to std::mutex

TheMostDiligent · 2021-06-27T18:37:39Z

NativeApp/include/Android/AndroidAppBase.hpp

+    {
+        // Android lifecycle status flags.  Not app-specific
+        // Set between onCreate and onDestroy
+        NV_APP_STATUS_RUNNING          = 0x00000001,


No need for NV_ prefix

TheMostDiligent · 2021-06-27T18:38:32Z

NativeApp/include/Android/AndroidAppBase.hpp

+        NV_APP_STATUS_INTERACTABLE     = 0x0000000f
+    };
+
+    unsigned int lifecycleFlags = 0;


lifecycle_flags_ to be consistent with the rest

TheMostDiligent · 2021-06-27T18:38:48Z

NativeApp/include/Android/AndroidAppBase.hpp

+    };
+
+    unsigned int lifecycleFlags = 0;
+    int32_t window_width        = 0;


window_width_

TheMostDiligent · 2021-06-27T18:40:08Z

NativeApp/src/Android/AndroidAppBase.cpp

 namespace Diligent
 {

+AndroidAppBase::AndroidAppBase() {
+    pthread_mutex_init(&mutex, NULL);


No need to initialize if you use std::mutex

TheMostDiligent · 2021-06-27T18:40:22Z

NativeApp/src/Android/AndroidAppBase.cpp

-        case APP_CMD_SAVE_STATE:
+        case APP_CMD_INPUT_CHANGED:
+#if(LOG_NATIVE_APPLICATION)
+            LOG_ERROR_MESSAGE("APP_CMD_INPUT_CHANGED");


Why is it an error?

to make it stand out in the log as it is important for seeing what is going on with the state of the native app internally when debugging the state, since errors have red text in logcat

TheMostDiligent · 2021-06-27T18:40:43Z

NativeApp/src/Android/AndroidAppBase.cpp

+            //pthread_mutex_lock(&android_app->mutex);
+            //if (android_app->inputQueue != NULL) {
+            //    AInputQueue_detachLooper(android_app->inputQueue);
+            //}


Delete this?

TheMostDiligent · 2021-06-27T18:40:53Z

NativeApp/src/Android/AndroidAppBase.cpp

-            {
+            pthread_mutex_lock(&android_app->mutex);
+#if(LOG_NATIVE_APPLICATION)
+            LOG_ERROR_MESSAGE("APP_CMD_INIT_WINDOW");


to make it stand out in the log as it is important for seeing what is going on with the state of the native app internally when debugging the state, since errors have red text in logcat

TheMostDiligent · 2021-06-27T18:41:30Z

NativeApp/src/Android/AndroidAppBase.cpp

-            // The window is being shown, get it ready.
-            if (app->window != NULL)
-            {
+            pthread_mutex_lock(&android_app->mutex);


std::lock_guard lock{mutex}; here and in similar places

TheMostDiligent

There are 11 commits in this branch already. Please squash them into one.

NativeApp/include/Android/AndroidAppBase.hpp

TheMostDiligent · 2021-06-27T20:00:24Z

NativeApp/include/Android/AndroidAppBase.hpp

+        APP_STATUS_HAS_REAL_SURFACE = 0x00000008,
+    };
+
+    unsigned int lifecycleFlags_ = 0;


APP_STATUS_FLAGS app_status_ = APP_STATUS_FLAG_NONE

TheMostDiligent · 2021-06-27T20:01:31Z

NativeApp/src/Android/AndroidAppBase.cpp

@@ -30,8 +30,12 @@
 //#include <android/log.h>
 //#include <android_native_app_glue.h>
 #include <android/native_window_jni.h>
+#include <Errors.hpp>


"Errors.hpp", this is not a system header

TheMostDiligent · 2021-06-27T20:02:07Z

NativeApp/src/Android/AndroidAppBase.cpp

+#if (LOG_NATIVE_APPLICATION)
+                LOG_INFO_MESSAGE("HAS SURFACE");
+#endif
+                std::lock_guard<std::mutex> give_me_a_name(eng->mutex);


lock{eng->mutex}

TheMostDiligent · 2021-06-27T20:04:39Z

NativeApp/include/Android/AndroidAppBase.hpp

@@ -89,7 +91,24 @@ class AndroidAppBase : public AppBase
    void UpdateFPS(float fFPS);

    bool initialized_resources_ = false;
-    bool has_focus_             = false;
+
+    enum


I suggest give it a name (e.g. APP_STATUS_FLAGS) and use DEFINE_FLAG_ENUM_OPERATORS(APP_STATUS_FLAGS)

TheMostDiligent · 2021-06-27T20:08:25Z

NativeApp/src/Android/AndroidAppBase.cpp

+        case APP_CMD_LOST_FOCUS:
+#if (LOG_NATIVE_APPLICATION)
+            LOG_INFO_MESSAGE("APP_CMD_LOST_FOCUS");
+            LOG_INFO_MESSAGE("LOST FOCUS");


Why extra message?

TheMostDiligent · 2021-06-27T20:08:38Z

NativeApp/src/Android/AndroidAppBase.cpp

+        case APP_CMD_RESUME:
+#if (LOG_NATIVE_APPLICATION)
+            LOG_INFO_MESSAGE("APP_CMD_RESUME");
+            LOG_INFO_MESSAGE("IS ACTIVE");


Why extra message?

TheMostDiligent · 2021-06-27T20:08:51Z

NativeApp/src/Android/AndroidAppBase.cpp

-            eng->has_focus_ = false;
+#if (LOG_NATIVE_APPLICATION)
+            LOG_INFO_MESSAGE("APP_CMD_PAUSE");
+            LOG_INFO_MESSAGE("IS NOT ACTIVE");


Why extra message?

TheMostDiligent · 2021-06-27T20:09:07Z

NativeApp/src/Android/AndroidAppBase.cpp

+        case APP_CMD_DESTROY:
+#if (LOG_NATIVE_APPLICATION)
+            LOG_INFO_MESSAGE("APP_CMD_DESTROY");
+            LOG_INFO_MESSAGE("IS NOT RUNNING");


Why extra message?

TheMostDiligent · 2021-06-27T20:11:49Z

NativeApp/src/Android/AndroidAppBase.cpp

-
-    return false;
+    std::lock_guard<std::mutex> give_me_a_name(mutex);
+    return lifecycleFlags_ & APP_STATUS_ACTIVE &&


This does not look pretty.
(app_status_ & APP_STATUS_FLAG_ACTIVE) != APP_STATUS_FLAG_NONE
Would be clearer

mgood7123 · 2021-06-28T02:20:35Z

There are 11 commits in this branch already. Please squash them into one.

is it possible to do this via github.com or would i manually need to do some git command line kung fu? (eg git reset, git add, git force push)

TheMostDiligent · 2021-06-28T02:26:30Z

You can force-push to the PR branch

mgood7123 · 2021-06-28T02:29:32Z

You can force-push to the PR branch

ok

mgood7123 · 2021-06-28T03:24:07Z

apparently force pushing a branch with no changes closes the pull request...

TheMostDiligent · 2021-06-28T03:47:52Z

NativeApp/src/Android/AndroidAppBase.cpp

        case APP_CMD_INIT_WINDOW:
-            // The window is being shown, get it ready.
-            if (app->window != NULL)
+#if (DILIGENT_DEVLOPMENT)


I actually think it makes sense to always log this - there is really no reason not to...
Same for other places.

so this?

case APP_CMD_INIT_WINDOW: LOG_INFO_MESSAGE("APP_CMD_INIT_WINDOW");

logging of all events

how is this? 60f08d8

mgood7123 · 2021-06-28T05:10:25Z

is this better?

mgood7123 · 2021-07-04T22:59:55Z

NativeApp/include/Android/AndroidAppBase.hpp

+
+    std::atomic<APP_STATUS_FLAGS> app_status_{APP_STATUS_FLAG_NONE};
+
+    int32_t window_width_  = 0;


this variable is unused, i need to remove it when i have time

mgood7123 · 2021-07-04T23:00:04Z

NativeApp/include/Android/AndroidAppBase.hpp

+    std::atomic<APP_STATUS_FLAGS> app_status_{APP_STATUS_FLAG_NONE};
+
+    int32_t window_width_  = 0;
+    int32_t window_height_ = 0;


this variable is unused, i need to remove it when i have time

Squashed commit of the following: commit ba7271f Author: mgood7123 <[email protected]> Date: Sat Jul 3 03:12:45 2021 +1000 use std::atomic instead of std::mutex commit 54e3973 Author: mgood7123 <[email protected]> Date: Tue Jun 29 07:52:35 2021 +1000 fixes edge cases in Android Native App rendering Revert "fix possible rendering edge cases" This reverts commit 3d76fb8. fix possible rendering edge cases Update AndroidMain.cpp add functions for app status apply validator suggestions Update AndroidAppBase.hpp Update AndroidAppBase.cpp remove unused variables

TheMostDiligent requested changes Jun 27, 2021

View reviewed changes

mgood7123 requested a review from TheMostDiligent June 27, 2021 19:31

TheMostDiligent requested changes Jun 27, 2021

View reviewed changes

TheMostDiligent requested a review from azhirnov June 28, 2021 02:27

mgood7123 force-pushed the patch-1 branch 2 times, most recently from be09133 to 96ead7d Compare June 28, 2021 03:10

mgood7123 closed this Jun 28, 2021

mgood7123 force-pushed the patch-1 branch from 96ead7d to 7ecb18d Compare June 28, 2021 03:13

mgood7123 reopened this Jun 28, 2021

mgood7123 force-pushed the patch-1 branch 5 times, most recently from 58a9e7e to 2d43095 Compare June 28, 2021 03:45

TheMostDiligent reviewed Jun 28, 2021

View reviewed changes

mgood7123 force-pushed the patch-1 branch 2 times, most recently from 60f08d8 to 3d76fb8 Compare June 28, 2021 04:51

mgood7123 force-pushed the patch-1 branch 7 times, most recently from 67c0830 to e09d30c Compare July 2, 2021 23:31

mgood7123 commented Jul 4, 2021

View reviewed changes

mgood7123 force-pushed the patch-1 branch from e09d30c to ac219fa Compare July 6, 2021 06:08


		std::atomic<APP_STATUS_FLAGS> app_status_{APP_STATUS_FLAG_NONE};

		int32_t window_width_ = 0;

fixes edge cases in Android Native App rendering #32

Are you sure you want to change the base?

fixes edge cases in Android Native App rendering #32

Conversation

mgood7123 commented Jun 27, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mgood7123 Jun 27, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheMostDiligent left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mgood7123 commented Jun 28, 2021

TheMostDiligent commented Jun 28, 2021

mgood7123 commented Jun 28, 2021

mgood7123 commented Jun 28, 2021

TheMostDiligent Jun 28, 2021 • edited Loading

Choose a reason for hiding this comment

mgood7123 Jun 28, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mgood7123 Jun 28, 2021 • edited Loading

Choose a reason for hiding this comment

mgood7123 commented Jun 28, 2021

mgood7123 Jul 4, 2021 • edited Loading

Choose a reason for hiding this comment

mgood7123 Jul 4, 2021 • edited Loading

Choose a reason for hiding this comment

mgood7123 commented Jun 27, 2021 •

edited

Loading

mgood7123 Jun 27, 2021 •

edited

Loading

TheMostDiligent Jun 28, 2021 •

edited

Loading

mgood7123 Jun 28, 2021 •

edited

Loading

mgood7123 Jun 28, 2021 •

edited

Loading

mgood7123 Jul 4, 2021 •

edited

Loading

mgood7123 Jul 4, 2021 •

edited

Loading