Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Android: Minor activity lifecycle stuff #18230

Merged
merged 6 commits into from
Sep 25, 2023
Merged

Android: Minor activity lifecycle stuff #18230

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
2291855
Remove dead code path
hrydgard Sep 24, 2023
2b0bbb1
Remove isFinishing check in onDestroy - not relevant.
hrydgard Sep 24, 2023
0013c6f
Rename ensureRenderLoop -> startRenderLoopThread
hrydgard Sep 24, 2023
1b8b441
Tighten up some renderloop logic in app-android.cpp
hrydgard Sep 24, 2023
8b9836a
SizeManager: Don't send notifySurface if paused. Cleaner exits / task…
hrydgard Sep 24, 2023
0fd22ea
Comment clarifications. Slightly extend renderlock use in shutdown.
hrydgard Sep 25, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 25 additions & 15 deletions android/jni/app-android.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -688,8 +688,9 @@ extern "C" void Java_org_ppsspp_ppsspp_NativeApp_init
EARLY_LOG("NativeApp.init() -- begin");
PROFILE_INIT();

std::lock_guard<std::mutex> guard(renderLock); // Note: This is held for the rest of this function - intended?
std::lock_guard<std::mutex> guard(renderLock); // Note: This is held for the rest of this function - intended? and even necessary?
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would think so. I think it should be on shutdown too. It should not be possible to update renderer_inited to true, update renderer_inited to false, or render at the same time. Those operations should all be sequenced. Remember that there are annoying things happening with resize events and etc.

-[Unknown]

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, true. I'll throw in a lock in shutdown too.

renderer_inited = false;
exitRenderLoop = false;
androidVersion = jAndroidVersion;
deviceType = jdeviceType;

Expand Down Expand Up @@ -872,6 +873,7 @@ extern "C" void Java_org_ppsspp_ppsspp_NativeApp_pause(JNIEnv *, jclass) {
}

extern "C" void Java_org_ppsspp_ppsspp_NativeApp_shutdown(JNIEnv *, jclass) {
INFO_LOG(SYSTEM, "NativeApp.shutdown() -- begin");
if (renderer_inited && useCPUThread && graphicsContext) {
// Only used in Java EGL path.
EmuThreadStop("shutdown");
Expand All @@ -891,8 +893,7 @@ extern "C" void Java_org_ppsspp_ppsspp_NativeApp_shutdown(JNIEnv *, jclass) {
EmuThreadJoin();
}

INFO_LOG(SYSTEM, "NativeApp.shutdown() -- begin");
if (renderer_inited) {
if (graphicsContext) {
INFO_LOG(G3D, "Shutting down renderer");
graphicsContext->Shutdown();
delete graphicsContext;
Expand Down Expand Up @@ -1135,6 +1136,9 @@ void UpdateRunLoopAndroid(JNIEnv *env) {
}

extern "C" void Java_org_ppsspp_ppsspp_NativeRenderer_displayRender(JNIEnv *env, jobject obj) {
// This doesn't get called on the Vulkan path.
_assert_(useCPUThread);

static bool hasSetThreadName = false;
if (!hasSetThreadName) {
hasSetThreadName = true;
Expand All @@ -1144,13 +1148,9 @@ extern "C" void Java_org_ppsspp_ppsspp_NativeRenderer_displayRender(JNIEnv *env,
if (IsVREnabled() && !StartVRRender())
return;

if (useCPUThread) {
// This is the "GPU thread". Call ThreadFrame.
if (!graphicsContext || !graphicsContext->ThreadFrame()) {
return;
}
} else {
UpdateRunLoopAndroid(env);
// This is the "GPU thread". Call ThreadFrame.
if (!graphicsContext || !graphicsContext->ThreadFrame()) {
return;
}

if (IsVREnabled()) {
Expand Down Expand Up @@ -1457,15 +1457,24 @@ static void ProcessFrameCommands(JNIEnv *env) {
}

// This runs in Vulkan mode only.
// This handles the entire lifecycle of the Vulkan context, init and exit.
extern "C" bool JNICALL Java_org_ppsspp_ppsspp_NativeActivity_runVulkanRenderLoop(JNIEnv *env, jobject obj, jobject _surf) {
_assert_(!useCPUThread);

if (!graphicsContext) {
ERROR_LOG(G3D, "runVulkanRenderLoop: Tried to enter without a created graphics context.");
renderLoopRunning = false;
exitRenderLoop = false;
return false;
}

exitRenderLoop = false;
if (exitRenderLoop) {
WARN_LOG(G3D, "runVulkanRenderLoop: ExitRenderLoop requested at start, skipping the whole thing.");
renderLoopRunning = false;
exitRenderLoop = false;
return true;
}

// This is up here to prevent race conditions, in case we pause during init.
renderLoopRunning = true;

Expand Down Expand Up @@ -1507,11 +1516,11 @@ extern "C" bool JNICALL Java_org_ppsspp_ppsspp_NativeActivity_runVulkanRenderLoo
hasSetThreadName = true;
SetCurrentThreadName("AndroidRender");
}
}

while (!exitRenderLoop) {
LockedNativeUpdateRender();
ProcessFrameCommands(env);
while (!exitRenderLoop) {
LockedNativeUpdateRender();
ProcessFrameCommands(env);
}
}

INFO_LOG(G3D, "Leaving EGL/Vulkan render loop.");
Expand All @@ -1525,6 +1534,7 @@ extern "C" bool JNICALL Java_org_ppsspp_ppsspp_NativeActivity_runVulkanRenderLoo
INFO_LOG(G3D, "Shutting down graphics context from render thread...");
graphicsContext->ShutdownFromRenderThread();
renderLoopRunning = false;
exitRenderLoop = false;

WARN_LOG(G3D, "Render loop function exited.");
return true;
Expand Down
24 changes: 13 additions & 11 deletions android/src/org/ppsspp/ppsspp/NativeActivity.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -662,7 +662,7 @@ public void onCreate(Bundle savedInstanceState) {
Log.i(TAG, "setcontentview before");
setContentView(mSurfaceView);
Log.i(TAG, "setcontentview after");
ensureRenderLoop();
startRenderLoopThread();
}
}

Expand All @@ -677,20 +677,26 @@ public void onWindowFocusChanged(boolean hasFocus) {

public void notifySurface(Surface surface) {
mSurface = surface;

if (!initialized) {
Log.e(TAG, "Can't deal with surfaces while not initialized");
return;
}

if (!javaGL) {
// If we got a surface, this starts the thread. If not, it doesn't.
if (mSurface == null) {
joinRenderLoopThread();
} else {
ensureRenderLoop();
startRenderLoopThread();
}
}
updateSustainedPerformanceMode();
}

// Invariants: After this, mRenderLoopThread will be set, and the thread will be running,
// if in Vulkan mode.
protected synchronized void ensureRenderLoop() {
protected synchronized void startRenderLoopThread() {
if (javaGL) {
Log.e(TAG, "JavaGL mode - should not get into ensureRenderLoop.");
return;
Expand Down Expand Up @@ -740,12 +746,6 @@ void setupSystemUiCallback() {
navigationCallbackView = decorView;
}

@Override
protected void onStop() {
super.onStop();
Log.i(TAG, "onStop - do nothing special");
}

@Override
protected void onDestroy() {
super.onDestroy();
Expand Down Expand Up @@ -785,7 +785,7 @@ protected void onDestroy() {
// 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 || isFinishing()) {
if (shuttingDown) {
NativeApp.shutdown();
unregisterCallbacks();
initialized = false;
Expand All @@ -803,6 +803,7 @@ protected void onPause() {
super.onPause();
Log.i(TAG, "onPause");
loseAudioFocus(this.audioManager, this.audioFocusChangeListener);
sizeManager.setPaused(true);
NativeApp.pause();
if (!javaGL) {
mSurfaceView.onPause();
Expand Down Expand Up @@ -838,6 +839,7 @@ private boolean detectOpenGLES30() {
protected void onResume() {
super.onResume();
updateSustainedPerformanceMode();
sizeManager.setPaused(false);
if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.ICE_CREAM_SANDWICH) {
updateSystemUiVisibility();
}
Expand Down Expand Up @@ -866,7 +868,7 @@ protected void onResume() {

if (!javaGL) {
// Restart the render loop.
ensureRenderLoop();
startRenderLoopThread();
}
}

Expand Down
14 changes: 13 additions & 1 deletion android/src/org/ppsspp/ppsspp/SizeManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,10 +39,18 @@ public class SizeManager implements SurfaceHolder.Callback {
private Point desiredSize = new Point();
private int badOrientationCount = 0;


private boolean paused = false;

public SizeManager(final NativeActivity a) {
activity = a;
}


public void setPaused(boolean p) {
paused = p;
}

@TargetApi(Build.VERSION_CODES.P)
public void setSurfaceView(SurfaceView view) {
surfaceView = view;
Expand Down Expand Up @@ -107,7 +115,11 @@ public void surfaceChanged(SurfaceHolder holder, int format, int width, int heig
NativeApp.backbufferResize(width, height, format);
updateDisplayMeasurements();

activity.notifySurface(holder.getSurface());
if (!paused) {
activity.notifySurface(holder.getSurface());
} else {
Log.i(TAG, "Skipping notifySurface while paused");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can't cause it to have an outdated surface, can it?

-[Unknown]

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does appear to be OK because we always get a surface notification after the next resume, which is when we want it.

I believe spurious surfaceChanged events after pause are a bug that seems to have been fixed around Android 12.

}
}

@Override
Expand Down