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: Get game metadata from core #9078

Merged
merged 5 commits into from Oct 21, 2020

Conversation

JosJuice
Copy link
Member

Trying to get it from a GameFile before emulation starts is unreliable.

Copy link
Member

@Ebola16 Ebola16 left a comment

Choose a reason for hiding this comment

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

This will display the gameId for the in-game emulation menu's game title. What do you think about removing the gameID there to maximize space for menu options?

@JosJuice
Copy link
Member Author

JosJuice commented Sep 13, 2020

I guess that makes sense. But what do we do in the case where the title database doesn't recognize the running game? Do we show just the game ID (like how this PR works right now) or nothing?

@Ebola16
Copy link
Member

Ebola16 commented Sep 13, 2020

I think displaying the game ID as a fallback if no title is found is fine.

Copy link
Member

@Ebola16 Ebola16 left a comment

Choose a reason for hiding this comment

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

Also, GeckoOS is giving me this exception on startup:
2020-09-13 09:42:12.622 8486-8486/org.dolphinemu.dolphinemu.debug E/AndroidRuntime: FATAL EXCEPTION: main
Process: org.dolphinemu.dolphinemu.debug, PID: 8486
java.lang.IllegalStateException: No game is running
at org.dolphinemu.dolphinemu.NativeLibrary.CheckGameMetadataValid(NativeLibrary.java:456)
at org.dolphinemu.dolphinemu.NativeLibrary.IsEmulatingWii(NativeLibrary.java:436)
at org.dolphinemu.dolphinemu.overlay.InputOverlay.refreshControls(InputOverlay.java:706)
at org.dolphinemu.dolphinemu.overlay.InputOverlay.initTouchPointer(InputOverlay.java:140)
at org.dolphinemu.dolphinemu.fragments.EmulationFragment.initInputPointer(EmulationFragment.java:158)
at org.dolphinemu.dolphinemu.activities.EmulationActivity.initInputPointer(EmulationActivity.java:1141)
at org.dolphinemu.dolphinemu.-$$Lambda$VaQ0DnlfVw6B_598YlWOOA_cqe0.run(Unknown Source:2)
at android.os.Handler.handleCallback(Handler.java:873)
at android.os.Handler.dispatchMessage(Handler.java:99)
at android.os.Looper.loop(Looper.java:214)
at android.app.ActivityThread.main(ActivityThread.java:7073)
at java.lang.reflect.Method.invoke(Native Method)
at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:494)
at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:965)

@JosJuice
Copy link
Member Author

Both problems should now be fixed. (Regarding Gecko OS, I hadn't taken into account that Host_TitleChanged doesn't get called on boot for DOL/ELF files.)

@Ebola16
Copy link
Member

Ebola16 commented Sep 13, 2020

Even with the most recent push, I'm getting a (unhelpful) DeadObjectException when loading GeckoOS. I double checked by trying on Master and it works fine so the problem isn't with my setup.

@Ebola16
Copy link
Member

Ebola16 commented Sep 13, 2020

Nevermind, it's the combination of https://bugs.dolphin-emu.org/issues/11113 and Android Dolphin resetting my default controls upon reinstall.

@JosJuice
Copy link
Member Author

I don't really get how that could cause a DeadObjectException... But I'll ignore it then.

@Ebola16
Copy link
Member

Ebola16 commented Sep 13, 2020

  • GeckoOS now launches properly but but I'm getting the same DeadObjectException when choosing "Launch Game", even with accounting for https://bugs.dolphin-emu.org/issues/11113.
  • When the point where noting is displayed for the in-game emulation menu game title is reached, can we instead display the game filename? This is much more useful info. It can let users know if they're currently using a mod launcher if they can't remember.

@JosJuice
Copy link
Member Author

JosJuice commented Sep 13, 2020

I tried turning off Wii Remotes and launching Xenoblade Chronicles through Gecko OS (saying yes to when Gecko OS asked me to install IOS 53), and it worked perfectly for me. Do you have any details on what's triggering the exception? I would assume it has something to do with the call to Host_TitleChanged, but I'm not sure.

I will try to create a separate PR (for all platforms) for displaying a name for DOL/ELF files. (I don't believe it's possible to reach the case where nothing, not even a game ID, is displayed when running a disc or WAD.)

@JosJuice
Copy link
Member Author

JosJuice commented Sep 13, 2020

It can let users know if they're currently using a mod launcher if they can't remember.

Perhaps I should mention this explicitly: With this PR, when you launch a game through something like Gecko OS, the title will update to match the game you launched. This is one of the advantages of getting the title from the core.

Copy link
Member

@Ebola16 Ebola16 left a comment

Choose a reason for hiding this comment

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

Perhaps I should mention this explicitly: With this PR, when you launch a game through something like Gecko OS, the title will update to match the game you launched. This is one of the advantages of getting the title from the core.

Now that I've gotten to the point where mods were loaded in this PR, I was able to notice that change and it makes sense.

Source/Android/jni/MainAndroid.cpp Show resolved Hide resolved
Copy link
Member

@Ebola16 Ebola16 left a comment

Choose a reason for hiding this comment

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

That fixed the problem. I don't have much C++ review experience but as far as I can tell this PR LGTM.

@JosJuice
Copy link
Member Author

I've managed to reproduce the crash now (with the last commit reverted). The reason why I couldn't reproduce it before was because it only happens with fastmem on and I had accidentally left fastmem off (since I need it off when using the debugger). Unfortunately, this means that I can't use the debugger to figure out what's going on, and I would really like to know what's going on before this gets merged. I'm also not getting a DeadObjectException – my old phone says SIGSEGV and my new phone says nothing at all. I'm assuming the DeadObjectException is actually just a symptom of the underlying native crash.

@JosJuice JosJuice force-pushed the android-metadata-from-core branch 4 times, most recently from 98e6d3d to d8de4c2 Compare September 17, 2020 16:12
@@ -152,8 +152,10 @@ void Host_TitleChanged()
{
s_game_metadata_is_valid = true;

JNIEnv* env = IDCache::GetEnvForThread();
env->CallStaticVoidMethod(IDCache::GetNativeLibraryClass(), IDCache::GetOnTitleChanged());
std::thread([] {
Copy link
Member

Choose a reason for hiding this comment

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

This should have a comment explaining why this is required.

Copy link
Member Author

Choose a reason for hiding this comment

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

The problem is that I have no idea why this is required. It's the one thing keeping this PR from being mergeable in my opinion.

Copy link
Contributor

Choose a reason for hiding this comment

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

I had a similar issue and know where it crashes. The culprit is JVM's AttachCurrentThread in GetEnvForThread.

In IDCache.cpp:

if (status == JNI_EDETACHED)
  s_java_vm->AttachCurrentThread(&env, nullptr);

The things is, for me at least, it raises randomly many different type of exceptions/errors such as segfault, libc/stack corruption, memory access errors, etc. I tried to find any relevant pieces of documentation regarding this behaviour but couldn't find any. I also don't know (yet) how to debug native code so I couldn't go deeper.

I suspect there is something fishy in that function and that depending on how the thread was created it might produce an undefined behaviour.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, so AttachCurrentThread fails if we call it from the CPU thread, but only when fastmem is enabled... Maybe we could make sure to attach the CPU thread before enabling fastmem? I'll give that a try.

Copy link
Member Author

Choose a reason for hiding this comment

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

That seems to do the trick. I'm much happier with this than the earlier workaround.

My new commit calls AttachCurrentThread before EMM::InstallExceptionHandler, but calling it immediately afterwards also seems to work. Perhaps the problem only occurs after the first fastmem fault rather than after the exception handler is installed?

@JosJuice
Copy link
Member Author

This should be ready for merge now.

A title change to a title with no game ID is still a title change.
Trying to get it from a GameFile before emulation starts is unreliable.
@leoetlino leoetlino merged commit 18b2553 into dolphin-emu:master Oct 21, 2020
10 checks passed
@JosJuice JosJuice deleted the android-metadata-from-core branch October 21, 2020 20:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
4 participants