diff --git a/AGENTS.md b/AGENTS.md index 7999d7af0d..4ea446a975 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -12,6 +12,7 @@ for it: | Doc | When you need it | |---|---| | [docs/building.md](docs/building.md) | Build commands for every target (VS/MSBuild, CMake, UWP, legacy Android NDK, libretro), unit tests, pspautotests | +| [docs/android-device-testing.md](docs/android-device-testing.md) | Running a build on a real Android device or a Quest over adb: installing the build you think you are, forcing activity recreates, measuring retained activities, VR | | [docs/debugging.md](docs/debugging.md) | Driving the WebSocket debugger and PPSSPPHeadless from a script, measuring a commercial game with headless, comparing binaries' speed (`Tools/headless_bench.py`), breakpoint reliability per CPU backend, debugging a game that works on hardware | | [docs/DebuggerThreading.md](docs/DebuggerThreading.md) | `Core_RunOnCPUThread` / `g_frameMutex` / shutdown-lock rules - required reading before touching debugger code | | [docs/HLEModules.md](docs/HLEModules.md) | Adding an HLE module or function, and the seven build files a new source file goes in | @@ -175,6 +176,13 @@ took minutes with a Debug build, software rendering and a 300MB log, and about a check that a game renders, `--screenshot-save=FILE.png` beats grepping the log. (pspautotests through `test.py` are different: they have their own per-test timeouts and use `--graphics=software` like CI.) +Testing on a real Android device has its own traps, the worst of which is silent: `adb install -r` +of a shorter branch over a longer one fails with `INSTALL_FAILED_VERSION_DOWNGRADE`, because +`versionCode` is the commit count. The device keeps running the old build and every measurement after +that describes the wrong binary. Use `adb install -r -d`, don't swallow the install output, and check +`adb shell dumpsys package org.ppsspp.ppsspp | grep versionName` afterwards. See +[docs/android-device-testing.md](docs/android-device-testing.md). + New unit tests are added to `availableTests`; large ones go in their own file in `unittest/`, which has to be listed in **three** build files, not two: `CMakeLists.txt`, `unittest/UnitTests.vcxproj` (and its `.filters`), and `android/jni/Android.mk`, which builds a unit test executable of its own. Miss the last diff --git a/Common/File/AndroidStorage.cpp b/Common/File/AndroidStorage.cpp index d65129d91a..08dcdf8075 100644 --- a/Common/File/AndroidStorage.cpp +++ b/Common/File/AndroidStorage.cpp @@ -1,4 +1,5 @@ #include +#include #include "Common/File/AndroidStorage.h" #include "Common/StringUtils.h" @@ -26,14 +27,37 @@ static jmethodID filePathGetFreeStorageSpace; static jmethodID isExternalStoragePreservedLegacy; static jmethodID computeRecursiveDirectorySize; +// A global ref owned by whoever calls Android_StorageSetActivity, who may replace it and delete the old +// one at any time. So we only look at it under the mutex, and call with a local ref of our own. static jobject g_nativeActivity; +static std::mutex g_nativeActivityLock; static jclass g_classContentUri; void Android_StorageSetActivity(jobject nativeActivity) { + std::lock_guard guard(g_nativeActivityLock); g_nativeActivity = nativeActivity; } +// Only for the one call that doesn't pass the activity to Java. Anything that does must use +// AcquireActivity and check the ref it got - between a check here and the call, the UI thread +// can swap the activity out from under us. +static bool HasActivity() { + std::lock_guard guard(g_nativeActivityLock); + return g_nativeActivity != nullptr; +} + +// Returns a local ref, to be freed by the caller's JNILocalFrame. Can be null if we just lost the activity. +static jobject AcquireActivity(JNIEnv *env) { + std::lock_guard guard(g_nativeActivityLock); + return g_nativeActivity ? env->NewLocalRef(g_nativeActivity) : nullptr; +} + void Android_RegisterStorageCallbacks(JNIEnv * env, jobject obj) { + if (g_classContentUri) { + // Already done. This gets called for each new activity, but the class and the method IDs stay + // valid, and other threads may be using them right now. + return; + } jclass localClass = env->FindClass("org/ppsspp/ppsspp/ContentUri"); _dbg_assert_(localClass); @@ -75,7 +99,7 @@ void Android_UnregisterStorageCallbacks(JNIEnv * env) { env->DeleteGlobalRef(g_classContentUri); g_classContentUri = nullptr; } - g_nativeActivity = nullptr; + Android_StorageSetActivity(nullptr); openContentUri = nullptr; listContentUriDir = nullptr; contentUriCreateFile = nullptr; @@ -97,12 +121,6 @@ bool Android_IsContentUri(std::string_view filename) { } int Android_OpenContentUriFd(std::string_view filename, Android_OpenContentUriMode mode) { - if (!g_nativeActivity) { - // Hit this in shortcut creation. - ERROR_LOG(Log::IO, "Android_OpenContentUriFd: No native activity"); - return -1; - } - /* // Should breakpoint here to try to find and move as many of these off the EmuThread as possible if (!strcmp(GetCurrentThreadName(), "EmuThread")) { @@ -117,6 +135,13 @@ int Android_OpenContentUriFd(std::string_view filename, Android_OpenContentUriMo fname.pop_back(); auto env = getEnv(); + JNILocalFrame frame(env); + jobject activity = AcquireActivity(env); + if (!activity) { + // Hit this in shortcut creation. + ERROR_LOG(Log::IO, "Android_OpenContentUriFd: No native activity"); + return -1; + } const char *modeStr = ""; switch (mode) { case Android_OpenContentUriMode::READ: modeStr = "r"; break; @@ -125,68 +150,80 @@ int Android_OpenContentUriFd(std::string_view filename, Android_OpenContentUriMo } jstring j_filename = env->NewStringUTF(fname.c_str()); jstring j_mode = env->NewStringUTF(modeStr); - int fd = env->CallStaticIntMethod(g_classContentUri, openContentUri, g_nativeActivity, j_filename, j_mode); + int fd = env->CallStaticIntMethod(g_classContentUri, openContentUri, activity, j_filename, j_mode); return fd; } StorageError Android_CreateDirectory(const std::string &rootTreeUri, const std::string &dirName) { - if (!g_nativeActivity) { + auto env = getEnv(); + JNILocalFrame frame(env); + jobject activity = AcquireActivity(env); + if (!activity) { return StorageError::UNKNOWN; } - auto env = getEnv(); jstring paramRoot = env->NewStringUTF(rootTreeUri.c_str()); jstring paramDirName = env->NewStringUTF(dirName.c_str()); - return StorageErrorFromInt(env->CallStaticIntMethod(g_classContentUri, contentUriCreateDirectory, g_nativeActivity, paramRoot, paramDirName)); + return StorageErrorFromInt(env->CallStaticIntMethod(g_classContentUri, contentUriCreateDirectory, activity, paramRoot, paramDirName)); } StorageError Android_CreateFile(const std::string &parentTreeUri, const std::string &fileName) { - if (!g_nativeActivity) { + auto env = getEnv(); + JNILocalFrame frame(env); + jobject activity = AcquireActivity(env); + if (!activity) { return StorageError::UNKNOWN; } - auto env = getEnv(); jstring paramRoot = env->NewStringUTF(parentTreeUri.c_str()); jstring paramFileName = env->NewStringUTF(fileName.c_str()); - return StorageErrorFromInt(env->CallStaticIntMethod(g_classContentUri, contentUriCreateFile, g_nativeActivity, paramRoot, paramFileName)); + return StorageErrorFromInt(env->CallStaticIntMethod(g_classContentUri, contentUriCreateFile, activity, paramRoot, paramFileName)); } StorageError Android_CopyFile(const std::string &fileUri, const std::string &destParentUri) { - if (!g_nativeActivity) { + auto env = getEnv(); + JNILocalFrame frame(env); + jobject activity = AcquireActivity(env); + if (!activity) { return StorageError::UNKNOWN; } - auto env = getEnv(); jstring paramFileName = env->NewStringUTF(fileUri.c_str()); jstring paramDestParentUri = env->NewStringUTF(destParentUri.c_str()); - return StorageErrorFromInt(env->CallStaticIntMethod(g_classContentUri, contentUriCopyFile, g_nativeActivity, paramFileName, paramDestParentUri)); + return StorageErrorFromInt(env->CallStaticIntMethod(g_classContentUri, contentUriCopyFile, activity, paramFileName, paramDestParentUri)); } StorageError Android_MoveFile(const std::string &fileUri, const std::string &srcParentUri, const std::string &destParentUri) { - if (!g_nativeActivity) { + auto env = getEnv(); + JNILocalFrame frame(env); + jobject activity = AcquireActivity(env); + if (!activity) { return StorageError::UNKNOWN; } - auto env = getEnv(); jstring paramFileName = env->NewStringUTF(fileUri.c_str()); jstring paramSrcParentUri = env->NewStringUTF(srcParentUri.c_str()); jstring paramDestParentUri = env->NewStringUTF(destParentUri.c_str()); - return StorageErrorFromInt(env->CallStaticIntMethod(g_classContentUri, contentUriMoveFile, g_nativeActivity, paramFileName, paramSrcParentUri, paramDestParentUri)); + return StorageErrorFromInt(env->CallStaticIntMethod(g_classContentUri, contentUriMoveFile, activity, paramFileName, paramSrcParentUri, paramDestParentUri)); } StorageError Android_RemoveFile(const std::string &fileUri) { - if (!g_nativeActivity) { + auto env = getEnv(); + JNILocalFrame frame(env); + jobject activity = AcquireActivity(env); + if (!activity) { return StorageError::UNKNOWN; } - auto env = getEnv(); jstring paramFileName = env->NewStringUTF(fileUri.c_str()); - return StorageErrorFromInt(env->CallStaticIntMethod(g_classContentUri, contentUriRemoveFile, g_nativeActivity, paramFileName)); + return StorageErrorFromInt(env->CallStaticIntMethod(g_classContentUri, contentUriRemoveFile, activity, paramFileName)); } StorageError Android_RenameFileTo(const std::string &fileUri, const std::string &newName) { - if (!g_nativeActivity) { + auto env = getEnv(); + JNILocalFrame frame(env); + jobject activity = AcquireActivity(env); + if (!activity) { return StorageError::UNKNOWN; } - auto env = getEnv(); jstring paramFileUri = env->NewStringUTF(fileUri.c_str()); jstring paramNewName = env->NewStringUTF(newName.c_str()); - return StorageErrorFromInt(env->CallStaticIntMethod(g_classContentUri, contentUriRenameFileTo, g_nativeActivity, paramFileUri, paramNewName)); + return StorageErrorFromInt(env->CallStaticIntMethod(g_classContentUri, contentUriRenameFileTo, activity, paramFileUri, paramNewName)); } // NOTE: Does not set fullName - you're supposed to already know it. @@ -226,50 +263,62 @@ static bool ParseFileInfo(std::string_view line, File::FileInfo *fileInfo) { } bool Android_GetFileInfo(const std::string &fileUri, File::FileInfo *fileInfo) { - if (!g_nativeActivity) { + auto env = getEnv(); + JNILocalFrame frame(env); + jobject activity = AcquireActivity(env); + if (!activity) { return false; } - auto env = getEnv(); jstring paramFileUri = env->NewStringUTF(fileUri.c_str()); - jstring str = (jstring)env->CallStaticObjectMethod(g_classContentUri, contentUriGetFileInfo, g_nativeActivity, paramFileUri); + jstring str = (jstring)env->CallStaticObjectMethod(g_classContentUri, contentUriGetFileInfo, activity, paramFileUri); if (!str) { return false; } const char *charArray = env->GetStringUTFChars(str, 0); + if (!charArray) { + return false; + } bool retval = ParseFileInfo(charArray, fileInfo); + env->ReleaseStringUTFChars(str, charArray); fileInfo->fullName = Path(fileUri); - - env->DeleteLocalRef(str); return retval && fileInfo->exists; } bool Android_FileExists(const std::string &fileUri) { - if (!g_nativeActivity) { + auto env = getEnv(); + JNILocalFrame frame(env); + jobject activity = AcquireActivity(env); + if (!activity) { return false; } - auto env = getEnv(); jstring paramFileUri = env->NewStringUTF(fileUri.c_str()); - bool exists = env->CallStaticBooleanMethod(g_classContentUri, contentUriFileExists, g_nativeActivity, paramFileUri); + bool exists = env->CallStaticBooleanMethod(g_classContentUri, contentUriFileExists, activity, paramFileUri); return exists; } std::vector Android_ListContentUri(const std::string &uri, const std::string &prefix, bool *exists) { - if (!g_nativeActivity) { + auto env = getEnv(); + JNILocalFrame frame(env); + jobject activity = AcquireActivity(env); + if (!activity) { *exists = false; return {}; } - auto env = getEnv(); *exists = true; double start = time_now_d(); jstring param = env->NewStringUTF(uri.c_str()); jstring filenamePrefix = prefix.empty() ? nullptr : env->NewStringUTF(prefix.c_str()); - jobject retval = env->CallStaticObjectMethod(g_classContentUri, listContentUriDir, g_nativeActivity, param, filenamePrefix); + jobject retval = env->CallStaticObjectMethod(g_classContentUri, listContentUriDir, activity, param, filenamePrefix); jobjectArray fileList = (jobjectArray)retval; std::vector items; + if (!fileList) { + *exists = false; + return items; + } int size = env->GetArrayLength(fileList); for (int i = 0; i < size; i++) { jstring str = (jstring)env->GetObjectArrayElement(fileList, i); @@ -286,11 +335,11 @@ std::vector Android_ListContentUri(const std::string &uri, const // INFO_LOG(Log::IO, "%s", info.name.c_str()); items.push_back(info); } + env->ReleaseStringUTFChars(str, charArray); } - env->ReleaseStringUTFChars(str, charArray); + // Don't leave these to the frame, there can be thousands. env->DeleteLocalRef(str); } - env->DeleteLocalRef(fileList); double elapsed = time_now_d() - start; double threshold = 0.1; @@ -301,21 +350,25 @@ std::vector Android_ListContentUri(const std::string &uri, const } int64_t Android_GetFreeSpaceByContentUri(const std::string &uri) { - if (!g_nativeActivity) { + auto env = getEnv(); + JNILocalFrame frame(env); + jobject activity = AcquireActivity(env); + if (!activity) { return false; } - auto env = getEnv(); jstring param = env->NewStringUTF(uri.c_str()); - return env->CallStaticLongMethod(g_classContentUri, contentUriGetFreeStorageSpace, g_nativeActivity, param); + return env->CallStaticLongMethod(g_classContentUri, contentUriGetFreeStorageSpace, activity, param); } // Hm, this is never used? We use statvfs instead. int64_t Android_GetFreeSpaceByFilePath(const std::string &filePath) { - if (!g_nativeActivity) { + auto env = getEnv(); + JNILocalFrame frame(env); + jobject activity = AcquireActivity(env); + if (!activity) { return false; } - auto env = getEnv(); if (System_GetPropertyInt(SYSPROP_SYSTEMVERSION) < 26) { // This is available from Android O. @@ -323,19 +376,21 @@ int64_t Android_GetFreeSpaceByFilePath(const std::string &filePath) { } jstring param = env->NewStringUTF(filePath.c_str()); - return env->CallStaticLongMethod(g_classContentUri, filePathGetFreeStorageSpace, g_nativeActivity, param); + return env->CallStaticLongMethod(g_classContentUri, filePathGetFreeStorageSpace, activity, param); } int64_t Android_ComputeRecursiveDirectorySize(const std::string &uri) { - if (!g_nativeActivity) { + auto env = getEnv(); + JNILocalFrame frame(env); + jobject activity = AcquireActivity(env); + if (!activity) { return false; } - auto env = getEnv(); jstring param = env->NewStringUTF(uri.c_str()); double start = time_now_d(); - int64_t size = env->CallStaticLongMethod(g_classContentUri, computeRecursiveDirectorySize, g_nativeActivity, param); + int64_t size = env->CallStaticLongMethod(g_classContentUri, computeRecursiveDirectorySize, activity, param); double elapsed = time_now_d() - start; INFO_LOG(Log::IO, "ComputeRecursiveDirectorySize(%s) in %0.3f s", uri.c_str(), elapsed); @@ -343,10 +398,11 @@ int64_t Android_ComputeRecursiveDirectorySize(const std::string &uri) { } bool Android_IsExternalStoragePreservedLegacy() { - if (!g_nativeActivity) { + if (!HasActivity()) { return false; } auto env = getEnv(); + JNILocalFrame frame(env); // Note: No activity param return env->CallStaticBooleanMethod(g_classContentUri, isExternalStoragePreservedLegacy); } diff --git a/Common/Render/Text/draw_text_android.cpp b/Common/Render/Text/draw_text_android.cpp index 4f0ea7742e..5cbb6f8d4e 100644 --- a/Common/Render/Text/draw_text_android.cpp +++ b/Common/Render/Text/draw_text_android.cpp @@ -16,20 +16,32 @@ #include -jobject TextDrawerAndroid::activity_; +// A new TextDrawer is created every time graphics are brought up, which on Android is every resume. +// The class stays valid for the life of the process, so hold one global ref to it, forever. +static jclass GetTextRendererClass(JNIEnv *env) { + static jclass cls; + if (!cls) { + const char *textRendererClassName = "org/ppsspp/ppsspp/TextRenderer"; + jclass localClass = findClass(textRendererClassName); + if (localClass) { + cls = reinterpret_cast(env->NewGlobalRef(localClass)); + env->DeleteLocalRef(localClass); + } else { + env->ExceptionClear(); // ClassNotFoundException + ERROR_LOG(Log::G3D, "Failed to find class: '%s'", textRendererClassName); + } + } + return cls; +} TextDrawerAndroid::TextDrawerAndroid(Draw::DrawContext *draw) : TextDrawer(draw) { auto env = getEnv(); - const char *textRendererClassName = "org/ppsspp/ppsspp/TextRenderer"; - jclass localClass = findClass(textRendererClassName); - cls_textRenderer = reinterpret_cast(env->NewGlobalRef(localClass)); + cls_textRenderer = GetTextRendererClass(env); if (cls_textRenderer) { method_allocFont = env->GetStaticMethodID(cls_textRenderer, "allocFont", "(Landroid/content/Context;Ljava/lang/String;)I"); method_freeAllFonts = env->GetStaticMethodID(cls_textRenderer, "freeAllFonts", "()V"); method_measureText = env->GetStaticMethodID(cls_textRenderer, "measureText", "(Ljava/lang/String;ID)I"); method_renderText = env->GetStaticMethodID(cls_textRenderer, "renderText", "(Ljava/lang/String;IDII)[I"); - } else { - ERROR_LOG(Log::G3D, "Failed to find class: '%s'", textRendererClassName); } dpiScale_ = CalculateDPIScale(); @@ -37,9 +49,7 @@ TextDrawerAndroid::TextDrawerAndroid(Draw::DrawContext *draw) : TextDrawer(draw) } TextDrawerAndroid::~TextDrawerAndroid() { - // Not sure why we can't do this but it crashes. Likely some deeper threading issue. - // At worst we leak one ref... - // env_->DeleteGlobalRef(cls_textRenderer); + // cls_textRenderer is shared and never released, see GetTextRendererClass. ClearCache(); fontMap_.clear(); // size is precomputed using dpiScale_. } @@ -61,9 +71,10 @@ void TextDrawerAndroid::SetOrCreateFont(const FontStyle &style) { auto fontIter = allocatedFonts_.find(filename); if (fontIter == allocatedFonts_.end()) { auto env = getEnv(); + JNILocalFrame frame(env); + jobject activity = Android_GetActivity(env); jstring jstr = env->NewStringUTF(filename.c_str()); - int fontId = env->CallStaticIntMethod(cls_textRenderer, method_allocFont, activity_, jstr); - env->DeleteLocalRef(jstr); + int fontId = env->CallStaticIntMethod(cls_textRenderer, method_allocFont, activity, jstr); if (fontId >= 0) { allocatedFonts_[filename] = fontId; @@ -130,6 +141,12 @@ bool TextDrawerAndroid::DrawStringBitmap(std::vector &bitmapData, TextS // TODO: Handle bold/italic jintArray imageData = (jintArray)env->CallStaticObjectMethod(cls_textRenderer, method_renderText, jstr, iter->second.font, iter->second.size, imageWidth, imageHeight); env->DeleteLocalRef(jstr); + if (!imageData) { + // Most likely an OutOfMemoryError allocating the bitmap. + env->ExceptionClear(); + ERROR_LOG(Log::G3D, "TextRenderer failed to render text"); + return false; + } entry.texture = nullptr; entry.bmWidth = imageWidth; diff --git a/Common/Render/Text/draw_text_android.h b/Common/Render/Text/draw_text_android.h index a097694266..3262aa42ab 100644 --- a/Common/Render/Text/draw_text_android.h +++ b/Common/Render/Text/draw_text_android.h @@ -21,7 +21,6 @@ public: bool IsReady() const override; void SetOrCreateFont(const FontStyle &style) override; - static void SetActivity(jobject activity) { activity_ = activity; } bool DrawStringBitmap(std::vector &bitmapData, TextStringEntry &entry, Draw::DataFormat texFormat, std::string_view str, int align, bool fullColor) override; protected: @@ -32,12 +31,11 @@ protected: private: // JNI functions - static jobject activity_; - jclass cls_textRenderer; - jmethodID method_allocFont; - jmethodID method_freeAllFonts; - jmethodID method_measureText; - jmethodID method_renderText; + jclass cls_textRenderer = nullptr; + jmethodID method_allocFont = nullptr; + jmethodID method_freeAllFonts = nullptr; + jmethodID method_measureText = nullptr; + jmethodID method_renderText = nullptr; std::map fontMap_; std::map allocatedFonts_; diff --git a/android/jni/app-android.cpp b/android/jni/app-android.cpp index 492252290a..6143c270ba 100644 --- a/android/jni/app-android.cpp +++ b/android/jni/app-android.cpp @@ -164,7 +164,9 @@ static jmethodID postCommand; static jmethodID getDebugString; static jmethodID getNativeCrashHistory; +// Global ref. Written on the UI thread under g_activityLock. Other threads go through Android_GetActivity. static jobject ppssppActivity; +static std::mutex g_activityLock; static std::atomic exitRenderLoop; static std::atomic renderLoopRunning; @@ -198,8 +200,13 @@ JNIEnv* getEnv() { return env; } +// Returns a local reference. jclass findClass(const char* name) { - return static_cast(getEnv()->CallObjectMethod(gClassLoader, gFindClassMethod, getEnv()->NewStringUTF(name))); + JNIEnv *env = getEnv(); + jstring jname = env->NewStringUTF(name); + jclass cls = static_cast(env->CallObjectMethod(gClassLoader, gFindClassMethod, jname)); + env->DeleteLocalRef(jname); + return cls; } void Android_AttachThreadToJNI() { @@ -249,6 +256,11 @@ JNIEXPORT jint JNICALL JNI_OnLoad(JavaVM *pjvm, void *reserved) { return JNI_VERSION_1_6; } +jobject Android_GetActivity(JNIEnv *env) { + std::lock_guard guard(g_activityLock); + return ppssppActivity ? env->NewLocalRef(ppssppActivity) : nullptr; +} + static void PushCommand(std::string_view cmd, std::string_view param) { std::lock_guard guard(frameCommandLock); g_frameCommands.emplace_back(std::string(cmd), std::string(param)); @@ -450,33 +462,39 @@ bool System_GetPropertyBool(SystemProperty prop) { } std::string Android_GetInputDeviceDebugString() { - if (!ppssppActivity) { + auto env = getEnv(); + JNILocalFrame frame(env); + jobject activity = Android_GetActivity(env); + if (!activity) { return "(N/A)"; } - auto env = getEnv(); jstring jparam = env->NewStringUTF("InputDevice"); - jstring jstr = (jstring)env->CallObjectMethod(ppssppActivity, getDebugString, jparam); + jstring jstr = (jstring)env->CallObjectMethod(activity, getDebugString, jparam); if (!jstr) { - env->DeleteLocalRef(jparam); return "(N/A)"; } const char *charArray = env->GetStringUTFChars(jstr, nullptr); - std::string retVal = charArray; - env->ReleaseStringUTFChars(jstr, charArray); - env->DeleteLocalRef(jstr); - env->DeleteLocalRef(jparam); + std::string retVal = charArray ? charArray : "(N/A)"; + if (charArray) { + env->ReleaseStringUTFChars(jstr, charArray); + } return retVal; } std::vector Android_GetNativeCrashHistory(int maxEntries) { std::vector crashHistory; - if (!ppssppActivity || !getNativeCrashHistory) { + if (!getNativeCrashHistory) { return crashHistory; } auto env = getEnv(); - jobject jlist = env->CallObjectMethod(ppssppActivity, getNativeCrashHistory, maxEntries); + JNILocalFrame frame(env); + jobject activity = Android_GetActivity(env); + if (!activity) { + return crashHistory; + } + jobject jlist = env->CallObjectMethod(activity, getNativeCrashHistory, maxEntries); if (!jlist) { return crashHistory; } @@ -508,8 +526,13 @@ std::string GetJavaString(JNIEnv *env, jstring jstr) { } extern "C" void Java_org_ppsspp_ppsspp_PpssppActivity_registerCallbacks(JNIEnv *env, jobject obj) { - ppssppActivity = env->NewGlobalRef(obj); - TextDrawerAndroid::SetActivity(ppssppActivity); + jobject newActivity = env->NewGlobalRef(obj); + jobject oldActivity; + { + std::lock_guard guard(g_activityLock); + oldActivity = ppssppActivity; + ppssppActivity = newActivity; + } postCommand = env->GetMethodID(env->GetObjectClass(obj), "postCommand", "(Ljava/lang/String;Ljava/lang/String;)V"); getDebugString = env->GetMethodID(env->GetObjectClass(obj), "getDebugString", "(Ljava/lang/String;)Ljava/lang/String;"); getNativeCrashHistory = env->GetMethodID(env->GetObjectClass(obj), "getNativeCrashHistory", "(I)Ljava/util/ArrayList;"); @@ -519,13 +542,26 @@ extern "C" void Java_org_ppsspp_ppsspp_PpssppActivity_registerCallbacks(JNIEnv * // It's OK if getNativeCrashHistory is missing. Android_RegisterStorageCallbacks(env, obj); - Android_StorageSetActivity(ppssppActivity); + Android_StorageSetActivity(newActivity); + + // An activity that gets recreated (rotation, resize) never unregisters, so this is where we let go + // of it. Used to leak the whole activity, once per rotation. + if (oldActivity) { + env->DeleteGlobalRef(oldActivity); + } } extern "C" void Java_org_ppsspp_ppsspp_PpssppActivity_unregisterCallbacks(JNIEnv *env, jobject obj) { Android_StorageSetActivity(nullptr); - env->DeleteGlobalRef(ppssppActivity); - ppssppActivity = nullptr; + jobject oldActivity; + { + std::lock_guard guard(g_activityLock); + oldActivity = ppssppActivity; + ppssppActivity = nullptr; + } + if (oldActivity) { + env->DeleteGlobalRef(oldActivity); + } } // This is now only used as a trigger for GetAppInfo as a function to all before Init. @@ -1421,6 +1457,7 @@ extern "C" jint JNICALL Java_org_ppsspp_ppsspp_NativeApp_getDisplayFramerateMode } std::vector System_GetCameraDeviceList() { + JNILocalFrame frame(getEnv()); jclass cameraClass = findClass("org/ppsspp/ppsspp/CameraHelper"); jmethodID deviceListMethod = getEnv()->GetStaticMethodID(cameraClass, "getDeviceList", "()Ljava/util/ArrayList;"); jobject deviceListObject = getEnv()->CallStaticObjectMethod(cameraClass, deviceListMethod); @@ -1470,14 +1507,16 @@ extern "C" void JNICALL Java_org_ppsspp_ppsspp_NativeApp_pushCameraImageAndroid( } } -// Call this under frameCommandLock. static void ProcessFrameCommands() { JNIEnv *env = getEnv(); + jobject activity = Android_GetActivity(env); std::vector frameCommands; { std::lock_guard guard(frameCommandLock); - if (!ppssppActivity) { - ERROR_LOG(Log::System, "No activity, clearing commands"); + if (!activity) { + if (!g_frameCommands.empty()) { + ERROR_LOG(Log::System, "No activity, clearing commands"); + } } else { frameCommands = std::move(g_frameCommands); } @@ -1485,17 +1524,20 @@ static void ProcessFrameCommands() { } if (!frameCommands.empty()) { - INFO_LOG(Log::System, "Processing %zu frame commands", g_frameCommands.size()); + INFO_LOG(Log::System, "Processing %zu frame commands", frameCommands.size()); for (const FrameCommand &frameCmd : frameCommands) { DEBUG_LOG(Log::System, "frameCommand '%s' '%s'", frameCmd.command.c_str(), frameCmd.params.c_str()); jstring cmd = env->NewStringUTF(frameCmd.command.c_str()); jstring param = env->NewStringUTF(frameCmd.params.c_str()); - env->CallVoidMethod(ppssppActivity, postCommand, cmd, param); + env->CallVoidMethod(activity, postCommand, cmd, param); env->DeleteLocalRef(cmd); env->DeleteLocalRef(param); } } + if (activity) { + env->DeleteLocalRef(activity); + } } std::thread g_renderLoopThread; diff --git a/android/jni/app-android.h b/android/jni/app-android.h index 727e15a7bf..e1af557b13 100644 --- a/android/jni/app-android.h +++ b/android/jni/app-android.h @@ -22,6 +22,32 @@ std::vector Android_GetNativeCrashHistory(int maxEntries); jclass findClass(const char* name); JNIEnv* getEnv(); +// Returns a new local reference to the current activity, or null if there is none. The activity gets +// replaced whenever Android recreates it (rotation, resize), so don't hang on to anything longer-lived. +jobject Android_GetActivity(JNIEnv *env); + +// Frees the JNI local references created in its scope. Java only does that for us when a native +// method returns to it, which never happens on the threads we attach ourselves (EmuThread, IO and +// worker threads), so there every local ref leaks until the thread detaches. Before Android 8 there's +// only room for 512 of them, and running out aborts the process. +class JNILocalFrame { +public: + explicit JNILocalFrame(JNIEnv *env, int capacity = 16) : env_(env) { + pushed_ = env_->PushLocalFrame(capacity) == 0; + } + ~JNILocalFrame() { + if (pushed_) { + env_->PopLocalFrame(nullptr); + } + } + JNILocalFrame(const JNILocalFrame &) = delete; + JNILocalFrame &operator=(const JNILocalFrame &) = delete; + +private: + JNIEnv *env_; + bool pushed_; +}; + #endif #else diff --git a/android/src/org/ppsspp/ppsspp/PowerSaveModeReceiver.java b/android/src/org/ppsspp/ppsspp/PowerSaveModeReceiver.java index 9b23cbfdf2..087e9c660c 100644 --- a/android/src/org/ppsspp/ppsspp/PowerSaveModeReceiver.java +++ b/android/src/org/ppsspp/ppsspp/PowerSaveModeReceiver.java @@ -14,6 +14,7 @@ import android.util.Log; public class PowerSaveModeReceiver extends BroadcastReceiver { private static final String TAG = PowerSaveModeReceiver.class.getSimpleName(); private static boolean isBatteryLow = false; + private final ContentObserver settingsObserver; @Override public void onReceive(final Context context, final Intent intent) { @@ -36,7 +37,7 @@ public class PowerSaveModeReceiver extends BroadcastReceiver { filter.addAction(PowerManager.ACTION_POWER_SAVE_MODE_CHANGED); activity.registerReceiver(this, filter); - activity.getContentResolver().registerContentObserver(Settings.System.CONTENT_URI, true, new ContentObserver(null) { + settingsObserver = new ContentObserver(null) { @Override public void onChange(boolean selfChange, Uri uri) { super.onChange(selfChange, uri); @@ -50,12 +51,15 @@ public class PowerSaveModeReceiver extends BroadcastReceiver { sendPowerSaving(activity); } } - }); + }; + activity.getContentResolver().registerContentObserver(Settings.System.CONTENT_URI, true, settingsObserver); sendPowerSaving(activity); } public void destroy(final Context context) { context.unregisterReceiver(this); + // The observer holds on to the activity, and the system holds on to a registered observer. + context.getContentResolver().unregisterContentObserver(settingsObserver); } private static boolean getNativePowerSaving(final Context context) { diff --git a/android/src/org/ppsspp/ppsspp/PpssppActivity.java b/android/src/org/ppsspp/ppsspp/PpssppActivity.java index 53d069369a..44299c0588 100644 --- a/android/src/org/ppsspp/ppsspp/PpssppActivity.java +++ b/android/src/org/ppsspp/ppsspp/PpssppActivity.java @@ -78,6 +78,11 @@ public class PpssppActivity extends AppCompatActivity implements SensorEventList // Allows us to skip a lot of initialization on secondary calls to onCreate. private static boolean initialized = false; + // Bumped by every onCreate. An activity whose number is no longer the latest has been replaced + // by a newer one, which now owns the native state. + private static int latestInstance = 0; + private int instance; + // Lifecycle tracker, to detect erroneous states. private final LifeCycle lifeCycle = new LifeCycle(); @@ -478,12 +483,14 @@ public class PpssppActivity extends AppCompatActivity implements SensorEventList } } + // These are static and outlive the activity, and the system services they look up hang on to + // the context they came from. So don't give them the activity. if (mLocationHelper == null) { - mLocationHelper = new LocationHelper(this); + mLocationHelper = new LocationHelper(getApplicationContext()); } try { if (mInfraredHelper == null) { - mInfraredHelper = new InfraredHelper(this); + mInfraredHelper = new InfraredHelper(getApplicationContext()); } } catch (Exception e) { mInfraredHelper = null; @@ -650,6 +657,8 @@ public class PpssppActivity extends AppCompatActivity implements SensorEventList return; } + instance = ++latestInstance; + WindowCompat.setDecorFitsSystemWindows(getWindow(), false); // In case app launched from homescreen shortcut, get shortcut parameter @@ -882,6 +891,10 @@ public class PpssppActivity extends AppCompatActivity implements SensorEventList Log.w(TAG, "startRenderLoopThread - not starting thread, needs surface"); return; } + if (instance != latestInstance) { + Log.w(TAG, "startRenderLoopThread - not starting thread, a newer activity has taken over"); + return; + } Log.w(TAG, "startRenderLoopThread: Starting thread"); @@ -890,6 +903,13 @@ public class PpssppActivity extends AppCompatActivity implements SensorEventList } private synchronized void joinRenderLoopThread() { + if (instance != latestInstance) { + // There's only one render thread, and it's rendering to the newer activity's surface now. + // We get here when our surface goes away late, after a finish() and a quick relaunch. + Log.w(TAG, "joinRenderLoopThread - leaving the thread alone, a newer activity has taken over"); + return; + } + // This will wait until the thread has exited. Log.i(TAG, "requestExitRenderLoop"); requestExitRenderLoop(); @@ -926,16 +946,25 @@ public class PpssppActivity extends AppCompatActivity implements SensorEventList mPowerSaveModeReceiver = null; } - // TODO: Can we ensure that the GL thread has stopped rendering here? - // I've seen crashes that seem to indicate that sometimes it hasn't... - NativeApp.audioShutdown(); - if (shuttingDown) { - Log.i(TAG, "in onDestroy, shutting down. Calling NativeApp.shutdown()."); - NativeApp.shutdown(); - unregisterCallbacks(); - initialized = false; + if (instance != latestInstance) { + // After finish(), Android is free to deliver onDestroy late, after it has already created + // and resumed a new activity if the user was quick to relaunch. That one found initialized + // still set and adopted the native state, so shutting down here would pull the audio and + // the graphics context out from under it. It's not ours to shut down anymore. + // (recreate() is not affected, it destroys the old activity before creating the new one.) + Log.w(TAG, "in onDestroy, but a newer activity has taken over. Leaving the native side alone."); } else { - Log.i(TAG, "in onDestroy, but not shutting down."); + // TODO: Can we ensure that the GL thread has stopped rendering here? + // I've seen crashes that seem to indicate that sometimes it hasn't... + NativeApp.audioShutdown(); + if (shuttingDown) { + Log.i(TAG, "in onDestroy, shutting down. Calling NativeApp.shutdown()."); + NativeApp.shutdown(); + unregisterCallbacks(); + initialized = false; + } else { + Log.i(TAG, "in onDestroy, but not shutting down."); + } } navigationCallbackView = null; diff --git a/docs/android-device-testing.md b/docs/android-device-testing.md new file mode 100644 index 0000000000..73862110cf --- /dev/null +++ b/docs/android-device-testing.md @@ -0,0 +1,108 @@ +# Testing an Android build on a device + +Notes from driving real devices over adb. Most of what follows is not about Android being hard, but +about ways the tooling quietly answers a question you did not ask - you measure the wrong build, or +measure garbage that has not been collected yet, and get a confident wrong answer. + +## Install the build you think you are testing + +`versionCode` is derived from the git commit count (see `gitVersionCode` in +`android/build.gradle.kts`), so a feature branch has a *lower* code than a longer branch. Installing +one over the other fails: + + adb: failed to install ...: Failure [INSTALL_FAILED_VERSION_DOWNGRADE] + +`-r` does not permit downgrades; `-d` does. Use `adb install -r -d `. + +Do not pipe the install through `tail -1` or `grep Success` and move on. The failure is a single +line and easy to swallow, and the device then keeps running whatever was there before - every +measurement afterwards describes the wrong binary, including A/B comparisons where both arms end up +being the same build and therefore agree with each other. + +Verify after every install, not just when something looks odd: + + adb shell dumpsys package org.ppsspp.ppsspp | grep versionName + +A second, independent check is worth it when a result is surprising: grep the log for a symbol that +exists only on the branch under test (a renamed class or log tag, say). + +The version string itself can also lie. The Android build regenerates `git-version.cpp` but does not +always recompile it, so the APK can carry the previous build's string while containing current code. +When the version matters: + + find android/.cxx -name git-version.cpp.o -delete + +## Forcing an activity recreate without touching the device + +- `adb shell settings put system font_scale 1.15`, then back. `fontScale` is not in the activity's + `configChanges`, so each change destroys and recreates the activity. Independent of orientation. +- `adb shell wm density ` also recreates - but only while `density` is absent from + `configChanges`. +- `adb shell settings put system user_rotation 1` does **not** rotate PPSSPP. The activity follows + the sensor, so this needs a physical rotation (or a cooperative device). + +Put back whatever you changed: `font_scale`, `accelerometer_rotation`, `user_rotation`. + +`wm density reset` is actively wrong on a device with a resolution override - a Galaxy S8 running +1080x2220 on a 1440x2960 panel resets to the *physical* panel's density and leaves the UI oversized. +Read the original with `wm density` first and set it back explicitly. + +## Measuring retained activities + +`adb shell dumpsys meminfo ` prints an Objects block with `Activities` and `AppContexts`. + +The counts include objects that are garbage but not yet collected. Read it straight after N +recreates and you will see roughly N whether or not anything leaks, which looks exactly like a leak. +Force collection instead: + + adb shell am send-trim-memory org.ppsspp.ppsspp COMPLETE + +then wait around eight seconds and read. Repeat until two readings agree - three or four rounds is +normal, and a single trim is not enough. + +The protocol that gives an answer you can trust: cold start and read the baseline, do N recreates, +trim until stable, compare. A sound build returns to the baseline; a leaking one sits at baseline+N +and stays there. + +Worked example, Galaxy S8 on Android 9, same device and protocol, both builds confirmed by +`versionName`. Before the activity handover fix: 13 recreates left 13 Activities and 18 AppContexts, +stable across repeated trims. After: the same 13 recreates settled back to 1 and 6. + +A JNI global reference is a GC root, so an activity pinned by one shows up here. This is a usable +check for JNI ownership bugs, not only Java ones. + +## logcat on a noisy device + +Filter to the process, since the tag alone is not enough: + + adb logcat -d --pid=$(adb shell pidof org.ppsspp.ppsspp) + +On a device with a chatty system - a Quest especially - the ring buffer can rotate the app's lines +away within seconds, which reads as "logging stopped" rather than "logging was lost". Start a stream +*before* launching the app instead: + + adb logcat -c && adb logcat -s PPSSPP:V PpssppActivity:V # in the background, then start the app + +## VR (Quest) + +- The VR flavor is a separate package: `./gradlew :android:assembleVrDebug`, installing as + `org.ppsspp.ppssppvr`. It can sit alongside the normal build. +- `adb exec-out screencap` returns solid black for an immersive app - it captures the 2D panel + layer, which such an app does not have. Use the runtime's own line instead: a logcat entry like + `VrApi: FPS=73/72 ... App=2.67ms` proves frames are being submitted and how much GPU time the app + spent on them. Zero app time means the render loop is not producing. +- `am start` makes the activity `topResumedActivity`, but the shell keeps focus, so the app never + gets a surface and the render loop does not restart. Re-entering an immersive app has to be done + from inside the headset. A fresh `am start -S` after a `force-stop` does work. +- `debuggerd -b ` needs root, so no native stacks from a retail headset. + +## Check which paths the device actually exercises + +Before concluding that a test covered something: + + adb shell grep -E '^(MemStickDirectory|GraphicsBackend)' /storage/emulated/0/PSP/SYSTEM/ppsspp.ini + +A plain filesystem path for the memstick means the SAF/content-URI code +(`Common/File/AndroidStorage.cpp`, `ContentUri.java`) is never entered, so a run on that device says +nothing about it - a recents list full of plain paths says the same. The VR flavor reads +`ppssppvr.ini`, not `ppsspp.ini`. diff --git a/headless/Headless.cpp b/headless/Headless.cpp index bb96ff3c7b..a54269d8c0 100644 --- a/headless/Headless.cpp +++ b/headless/Headless.cpp @@ -112,6 +112,11 @@ jclass findClass(const char *name) { return nullptr; } +// Defined in app-android.cpp for the app itself, which this doesn't link. +jobject Android_GetActivity(JNIEnv *env) { + return nullptr; +} + bool System_AudioRecordingIsAvailable() { return false; } bool System_AudioRecordingState() { return false; } #endif diff --git a/unittest/UnitTest.cpp b/unittest/UnitTest.cpp index d382e34b1f..25b7b3934b 100644 --- a/unittest/UnitTest.cpp +++ b/unittest/UnitTest.cpp @@ -178,6 +178,11 @@ jclass findClass(const char *name) { return nullptr; } +// Defined in app-android.cpp for the app itself, which this doesn't link. +jobject Android_GetActivity(JNIEnv *env) { + return nullptr; +} + bool System_AudioRecordingIsAvailable() { return false; } bool System_AudioRecordingState() { return false; } #endif