diff --git a/platforms/android/app/src/main/cpp/savers/flux/flux_port.cpp b/platforms/android/app/src/main/cpp/savers/flux/flux_port.cpp index cf4767fa68..8c299e2765 100644 --- a/platforms/android/app/src/main/cpp/savers/flux/flux_port.cpp +++ b/platforms/android/app/src/main/cpp/savers/flux/flux_port.cpp @@ -40,7 +40,14 @@ int flux_port_new(int preset) saver_flux::initSaver(); g_started = saver_flux::readyToDraw != 0; - return g_started ? 1 : 0; + if (!g_started) { + /* Returning 0 means the JNI never calls port_free, so this is the only chance to give + * gl1 back. Leaving it up would strand g.ready with names from a context that is about + * to die, and gl1_init() early-returns on g.ready -- poisoning the NEXT saver. */ + gl1_shutdown(); + return 0; + } + return 1; } void flux_port_resize(int width, int height) diff --git a/platforms/android/app/src/main/cpp/savers/lattice/lattice_port.cpp b/platforms/android/app/src/main/cpp/savers/lattice/lattice_port.cpp index 44fe6f911b..bcd52d2ad5 100644 --- a/platforms/android/app/src/main/cpp/savers/lattice/lattice_port.cpp +++ b/platforms/android/app/src/main/cpp/savers/lattice/lattice_port.cpp @@ -53,11 +53,21 @@ void lattice_port_draw() void lattice_port_free() { - if (!g_started) return; - saver_lattice::cleanUp(); + /* NOT "if (!g_started) return": this saver waits for a surface size before it initialises, + * so it can be created and torn down having never started. See the note below. */ + if (g_started) { + saver_lattice::cleanUp(); + saver_lattice::readyToDraw = 0; + g_started = false; + } + /* gl1_init() ran in port_new, and everything it holds -- the shader program, the vertex + * buffers -- belongs to the EGL context that is about to be destroyed. Returning without + * gl1_shutdown() leaves gl1's g.ready set with GL names from a DEAD context, and gl1_init() + * early-returns on g.ready. The next saver, in a NEW context, would then run against those + * dead names: undefined behaviour that some drivers answer with a segfault rather than a GL + * error, which takes the whole app down. So gl1 is torn down whether or not this saver's own + * init ever got as far as running. gl1_shutdown() is idempotent. */ gl1_shutdown(); - saver_lattice::readyToDraw = 0; - g_started = false; } } /* extern "C" */ diff --git a/platforms/android/app/src/main/cpp/savers/plasma/plasma_port.cpp b/platforms/android/app/src/main/cpp/savers/plasma/plasma_port.cpp index 33645edacb..2d4f5a1e18 100644 --- a/platforms/android/app/src/main/cpp/savers/plasma/plasma_port.cpp +++ b/platforms/android/app/src/main/cpp/savers/plasma/plasma_port.cpp @@ -42,7 +42,14 @@ int plasma_port_new(int preset) saver_plasma::initSaver(); g_started = saver_plasma::readyToDraw != 0; - return g_started ? 1 : 0; + if (!g_started) { + /* Returning 0 means the JNI never calls port_free, so this is the only chance to give + * gl1 back. Leaving it up would strand g.ready with names from a context that is about + * to die, and gl1_init() early-returns on g.ready -- poisoning the NEXT saver. */ + gl1_shutdown(); + return 0; + } + return 1; } void plasma_port_resize(int width, int height) diff --git a/platforms/android/app/src/main/cpp/savers/savers_jni.cpp b/platforms/android/app/src/main/cpp/savers/savers_jni.cpp index 85dff4c2bb..8e1e13b07b 100644 --- a/platforms/android/app/src/main/cpp/savers/savers_jni.cpp +++ b/platforms/android/app/src/main/cpp/savers/savers_jni.cpp @@ -7,6 +7,7 @@ #include #include +#include "gl1.h" #include #include @@ -82,6 +83,21 @@ Java_com_armsx2_ui_home_SaverNative_nativeInit(JNIEnv *, jobject, jint effect, j /* A previous saver may still be up if the outgoing view has not torn down yet. */ if (g_active) g_active->destroy(); + /* Every nativeInit arrives on a freshly created EGL context -- one view, one render thread, + * one context, one create. So whatever GL names gl1 is still holding belong to a context + * that no longer exists, and gl1_init() early-returns on g.ready, which would hand those + * dead names to the saver we are about to start. gl1_lost() drops them without calling GL + * on them (gl1_shutdown() would try to delete them, in the wrong context). + * + * The ports above each give gl1 back on their own failure and teardown paths, so this + * should already be a no-op. It is here because it is the invariant that actually matters + * -- a saver added later that forgets, or an upstream cleanup that returns early, would + * otherwise poison the NEXT saver rather than fail visibly in its own. Drivers answer a + * draw against a dead program name with anything from a black screen to a segfault, and a + * segfault here is unrecoverable: the library is the first screen, so the app would crash + * on every launch. */ + gl1_lost(); + g_active = &k_savers[effect]; if (!g_active->create(preset)) { LOGE("%s failed to start (preset %d)", g_active->name, preset); diff --git a/platforms/android/app/src/main/cpp/savers/skyrocket/skyrocket_port.cpp b/platforms/android/app/src/main/cpp/savers/skyrocket/skyrocket_port.cpp index 596561b105..9f21c55872 100644 --- a/platforms/android/app/src/main/cpp/savers/skyrocket/skyrocket_port.cpp +++ b/platforms/android/app/src/main/cpp/savers/skyrocket/skyrocket_port.cpp @@ -69,11 +69,21 @@ void skyrocket_port_draw() void skyrocket_port_free() { - if (!g_started) return; - saver_skyrocket::cleanup(); + /* NOT "if (!g_started) return": this saver waits for a surface size before it initialises, + * so it can be created and torn down having never started. See the note below. */ + if (g_started) { + saver_skyrocket::cleanup(); + saver_skyrocket::readyToDraw = 0; + g_started = false; + } + /* gl1_init() ran in port_new, and everything it holds -- the shader program, the vertex + * buffers -- belongs to the EGL context that is about to be destroyed. Returning without + * gl1_shutdown() leaves gl1's g.ready set with GL names from a DEAD context, and gl1_init() + * early-returns on g.ready. The next saver, in a NEW context, would then run against those + * dead names: undefined behaviour that some drivers answer with a segfault rather than a GL + * error, which takes the whole app down. So gl1 is torn down whether or not this saver's own + * init ever got as far as running. gl1_shutdown() is idempotent. */ gl1_shutdown(); - saver_skyrocket::readyToDraw = 0; - g_started = false; } } /* extern "C" */ diff --git a/platforms/android/app/src/main/cpp/savers/solarwinds/solarwinds_port.cpp b/platforms/android/app/src/main/cpp/savers/solarwinds/solarwinds_port.cpp index e664bd4272..b9eb11cf47 100644 --- a/platforms/android/app/src/main/cpp/savers/solarwinds/solarwinds_port.cpp +++ b/platforms/android/app/src/main/cpp/savers/solarwinds/solarwinds_port.cpp @@ -27,7 +27,14 @@ int solarwinds_port_new(int preset) saver_solarwinds::initSaver(); g_started = saver_solarwinds::readyToDraw != 0; - return g_started ? 1 : 0; + if (!g_started) { + /* Returning 0 means the JNI never calls port_free, so this is the only chance to give + * gl1 back. Leaving it up would strand g.ready with names from a context that is about + * to die, and gl1_init() early-returns on g.ready -- poisoning the NEXT saver. */ + gl1_shutdown(); + return 0; + } + return 1; } void solarwinds_port_resize(int width, int height) diff --git a/platforms/android/app/src/main/java/com/armsx2/ui/home/HomeScreen.kt b/platforms/android/app/src/main/java/com/armsx2/ui/home/HomeScreen.kt index 3a8e1fa6d1..6087330426 100644 --- a/platforms/android/app/src/main/java/com/armsx2/ui/home/HomeScreen.kt +++ b/platforms/android/app/src/main/java/com/armsx2/ui/home/HomeScreen.kt @@ -146,6 +146,19 @@ fun HomeScreen( var showClearRecentsConfirm by remember { mutableStateOf(false) } // #9 custom library background — inert until the user picks an image. LaunchedEffect(Unit) { LibraryBackground.ensureLoaded(); CoverArtStyle.load() } + // The animated background switched itself off because the last run died with it on screen + // (LibraryBackground.armSaver). Say so -- silently reverting a setting the user chose reads + // as the setting being broken, and the name tells them which one to avoid. + LaunchedEffect(LibraryBackground.crashedSaver.value) { + LibraryBackground.crashedSaver.value?.let { kind -> + LibraryBackground.crashedSaver.value = null + Toast.makeText( + context, + "Animated background turned off: ${LibraryBackground.saverName(kind)} crashed last time.", + Toast.LENGTH_LONG, + ).show() + } + } val backgroundPicker = rememberLauncherForActivityResult(ActivityResultContracts.OpenDocument()) { picked -> picked?.let { LibraryBackground.set(context, it) } } diff --git a/platforms/android/app/src/main/java/com/armsx2/ui/home/LibraryBackground.kt b/platforms/android/app/src/main/java/com/armsx2/ui/home/LibraryBackground.kt index 44289afabf..d21025c688 100644 --- a/platforms/android/app/src/main/java/com/armsx2/ui/home/LibraryBackground.kt +++ b/platforms/android/app/src/main/java/com/armsx2/ui/home/LibraryBackground.kt @@ -21,6 +21,12 @@ object LibraryBackground { private const val PREF_FLURRY_PRESET = "library.background.flurry.preset" private const val PREF_SAVER_KIND = "library_saver_kind" private const val PREF_RSS_PRESET = "library_rss_preset" + + /** + * Which saver is CURRENTLY running, written synchronously before its GL thread starts and + * cleared when that thread exits in an orderly way. See [armSaver]. + */ + private const val PREF_ARMED = "library.saver.armed" val uri = mutableStateOf(null) /** @@ -61,6 +67,47 @@ object LibraryBackground { /** Preset for whichever Really Slick saver is selected, 1..6. 99 = pick one each time. */ val rssPreset = mutableStateOf(99) + /** + * Set at startup when the previous run died with a saver on screen. Holds the [saverKind] that + * was running so the library can say which one, and so the user knows their background was + * turned off deliberately rather than forgotten. Read once and cleared by the reader. + */ + val crashedSaver = mutableStateOf(null) + + /** Display name for a [saverKind], for the message above. Matches the settings list. */ + fun saverName(kind: Int): String = when (kind) { + 1 -> "Flux"; 2 -> "Plasma"; 3 -> "SolarWinds" + 4 -> "Hyperspace"; 5 -> "Lattice"; 6 -> "Skyrocket" + else -> "Flurry" + } + + /** + * Crash-loop breaker. + * + * The savers are native GL code, and native GL code can take the process down in ways no + * `runCatching` can see -- a SIGSEGV in a driver, or a hang that Android resolves by killing + * us. Because the choice is persisted and the library is the FIRST screen, a saver that dies + * on startup dies again on every launch: the app never gets far enough for anyone to reach + * Settings and switch it off. The only escape is clearing app data, which on Android takes + * the memory cards and save states with it. A user hit exactly that and lost their saves. + * + * So the setting arms itself before the GL thread starts and disarms when that thread exits + * normally. Finding it still armed at startup means last run ended while a saver was on + * screen -- the background is switched off and the user is told which one did it. The write + * must be commit() rather than apply(): apply() is asynchronous, and the whole point is that + * the process may be about to die. + */ + fun armSaver() { + runCatching { + MainActivityRuntime.prefs.edit().putInt(PREF_ARMED, saverKind.value).commit() + } + } + + /** Orderly teardown -- the saver ran without taking the process with it. Idempotent. */ + fun disarmSaver() { + runCatching { MainActivityRuntime.prefs.edit().remove(PREF_ARMED).apply() } + } + private var loaded = false fun ensureLoaded() { @@ -72,6 +119,14 @@ object LibraryBackground { flurryPreset.value = runCatching { MainActivityRuntime.prefs.getInt(PREF_FLURRY_PRESET, 99) }.getOrDefault(99) saverKind.value = runCatching { MainActivityRuntime.prefs.getInt(PREF_SAVER_KIND, 0) }.getOrDefault(0) rssPreset.value = runCatching { MainActivityRuntime.prefs.getInt(PREF_RSS_PRESET, 99) }.getOrDefault(99) + + // Still armed = the previous run died with a saver up. Break the loop (see armSaver). + val armed = runCatching { MainActivityRuntime.prefs.getInt(PREF_ARMED, -1) }.getOrDefault(-1) + if (armed >= 0) { + crashedSaver.value = armed + setFlurry(false) + disarmSaver() + } } fun setAnimated2D(on: Boolean) { diff --git a/platforms/android/app/src/main/java/com/armsx2/ui/home/SaverGlView.kt b/platforms/android/app/src/main/java/com/armsx2/ui/home/SaverGlView.kt index 25a2a7ea89..09aeaf889d 100644 --- a/platforms/android/app/src/main/java/com/armsx2/ui/home/SaverGlView.kt +++ b/platforms/android/app/src/main/java/com/armsx2/ui/home/SaverGlView.kt @@ -97,8 +97,22 @@ class SaverGlView(context: Context, private val spec: SaverSpec) : } override fun onSurfaceTextureAvailable(st: SurfaceTexture, w: Int, h: Int) { - thread = RenderThread(st, w, h, spec) { ok -> post { onGlStatus?.invoke(ok) } } - .also { it.start() } + // Arm the crash-loop breaker for as long as native GL code is running on our behalf; the + // thread disarms it when it exits in an orderly way. See LibraryBackground.armSaver. + LibraryBackground.armSaver() + // start() asks for a 16MB stack (see STACK_BYTES) and can throw OutOfMemoryError on a + // constrained device. That would be an uncaught throw on the MAIN thread -- the process + // dies, and since this is the first screen the app would be unlaunchable. Fall back to the + // 2D backdrop instead, the same way an EGL failure does. + thread = runCatching { + RenderThread(st, w, h, spec) { ok -> post { onGlStatus?.invoke(ok) } } + .also { it.start() } + }.getOrElse { + Log.w(TAG, "saver thread failed to start", it) + LibraryBackground.disarmSaver() + onGlStatus?.invoke(false) + null + } } override fun onSurfaceTextureSizeChanged(st: SurfaceTexture, w: Int, h: Int) { @@ -152,9 +166,26 @@ class SaverGlView(context: Context, private val spec: SaverSpec) : private var saver: Saver? = null fun resize(w: Int, h: Int) { width = w; height = h; sizeDirty = true } - fun finish() { running = false; runCatching { join(500) } } + fun finish() { + running = false + runCatching { join(500) } + // join() is bounded, so the thread's own finally may not have run yet. We asked it to + // stop and the process is still here, which is all the breaker needs to know. + LibraryBackground.disarmSaver() + } override fun run() { + // Reaching the end of this function at all -- however the saver did -- means the + // process survived it, which is the only thing the breaker is asking about. A native + // crash or a kill never gets here, and that is what leaves the flag set. + try { + render() + } finally { + LibraryBackground.disarmSaver() + } + } + + private fun render() { if (!initEgl()) { onStatus(false); teardown(); return } val s = spec.newSaver()