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.

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

Android: Minor activity lifecycle stuff #18230

Merged
merged 6 commits into from
Sep 25, 2023
Merged

Conversation

hrydgard
Copy link
Owner

Tried to keep the changes very safe, while still potentially fixing some minor things. Tests fine.

My goal is to see if we can fix some common ANRs. (application not responding).

@hrydgard hrydgard added this to the v1.16.4 milestone Sep 24, 2023
@hrydgard hrydgard changed the title Minor java lifecycle stuff Android: Minor java lifecycle stuff Sep 24, 2023
@hrydgard hrydgard changed the title Android: Minor java lifecycle stuff Android: Minor activity lifecycle stuff Sep 24, 2023
@@ -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.

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.

@hrydgard
Copy link
Owner Author

Tested this on a whole bunch of devices and it behaves well.

Even before this though, I wasn't really able to reproduce the app-not-responding reports I see in the logs from Google Play...

@hrydgard hrydgard merged commit 165225d into master Sep 25, 2023
18 checks passed
@hrydgard hrydgard deleted the minor-java-lifecycle-stuff branch September 25, 2023 08:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants