mirror of
https://github.com/ARMSX2/ARMSX2.git
synced 2026-08-24 16:50:16 -07:00
Android: fix pause/resume crashes, stalls and silent save-state refusals
OboeAudioStream had no synchronisation at all. onError() runs on Oboe's own callback thread and tears the stream down and back up, while the CPU thread can be inside SetPaused() on the same object - its null check and dereference of m_stream are not atomic against onError's reset(), so the stream could be destroyed between them. Android reclaims an idle low-latency stream a few seconds into the pause menu, so touching any setting at that moment re-entered SPU2 and crashed. Fixed with a recursive lock over the lifecycle. setOutputPauseSuppressed and onNativeSurfaceDestroyed were both fully implemented, both documented as having callers, and neither was ever called. Without the first, every overlay pause let Android reclaim the audio device and the RESUME had to rebuild it inline on the CPU thread ahead of Host::OnVMResumed - seconds of apparent hang. Without the second, the GS thread was never told the surface died and could block in vkAcquireNextImageKHR forever, which is the permanent freeze on backgrounding. pauseForOverlay now goes through the same executor as resume, so the FIFO ordering both comments claimed is actually true. Save states: every refusal was silent. MemcardBusy is decremented only by VSyncStart, so it is frozen while the pause menu is up and waiting in the menu can never clear it - only resuming can, which is why it appeared to need two or three tries. The refusal is correct; the silence was not. Also adds Achievements hardcore reporting and a per-exit diagnostic line. Includes Brian Degenhardt's PR #426 (JNI thread ownership).
This commit is contained in:
@@ -12,6 +12,7 @@
|
||||
|
||||
#include <atomic>
|
||||
#include <chrono>
|
||||
#include <mutex>
|
||||
#include <thread>
|
||||
#if defined(__ANDROID__)
|
||||
#include <sched.h>
|
||||
@@ -41,8 +42,20 @@ namespace {
|
||||
bool onError(oboe::AudioStream* oboeStream, oboe::Result error) override;
|
||||
|
||||
private:
|
||||
// ★ Serialises the stream lifecycle. onError() runs on OBOE'S OWN callback thread and
|
||||
// tears the stream down and back up (Stop/Close/Open/Start), while the CPU thread can be
|
||||
// inside SetPaused()/Close() on the very same object. SetPaused's `if (m_stream)` followed
|
||||
// by `m_stream->requestPause()` is not atomic against onError's `m_stream.reset()`, so the
|
||||
// stream could be destroyed between the null check and the dereference — a use-after-free.
|
||||
// That window opens exactly where users report crashing: Android reclaims the audio device
|
||||
// a few seconds into the pause menu (#333), onError fires to reopen it, and touching any
|
||||
// setting at that moment re-enters SPU2 from the CPU thread (#422).
|
||||
// Recursive because Close() calls Stop(), and onError() calls all four in sequence.
|
||||
std::recursive_mutex m_lock;
|
||||
|
||||
bool m_playing = false;
|
||||
bool m_stop_requested = false;
|
||||
// Written by Start()/Stop() on the CPU thread, read by onError() on the callback thread.
|
||||
std::atomic<bool> m_stop_requested{false};
|
||||
|
||||
std::shared_ptr<oboe::AudioStream> m_stream;
|
||||
|
||||
@@ -115,8 +128,11 @@ oboe::DataCallbackResult OboeAudioStream::onAudioReady(oboe::AudioStream* p_audi
|
||||
bool OboeAudioStream::onError(oboe::AudioStream* oboeStream, oboe::Result error)
|
||||
{
|
||||
Console.Error("(Oboe) ErrorCB %d", error);
|
||||
if (error == oboe::Result::ErrorDisconnected && !m_stop_requested)
|
||||
if (error == oboe::Result::ErrorDisconnected && !m_stop_requested.load(std::memory_order_acquire))
|
||||
{
|
||||
// Held across the whole teardown/rebuild so the CPU thread can't observe (or destroy) a
|
||||
// half-open stream partway through. See the m_lock comment.
|
||||
const std::lock_guard<std::recursive_mutex> guard(m_lock);
|
||||
Console.Error("(Oboe) Stream disconnected, reopening...");
|
||||
Stop();
|
||||
Close();
|
||||
@@ -186,6 +202,7 @@ bool OboeAudioStream::Initialize(bool stretch_enabled)
|
||||
|
||||
bool OboeAudioStream::Open()
|
||||
{
|
||||
const std::lock_guard<std::recursive_mutex> guard(m_lock);
|
||||
// Each Open() spawns a fresh Oboe audio thread with a new TID, so the
|
||||
// per-stream pin latch needs to clear here. Without this, an error-
|
||||
// recovery re-Open() (onError → Stop/Close/Open) keeps the latch set
|
||||
@@ -227,11 +244,12 @@ bool OboeAudioStream::Open()
|
||||
|
||||
bool OboeAudioStream::Start()
|
||||
{
|
||||
const std::lock_guard<std::recursive_mutex> guard(m_lock);
|
||||
if (m_playing)
|
||||
return true;
|
||||
|
||||
Console.WriteLn("(Oboe) Starting stream...");
|
||||
m_stop_requested = false;
|
||||
m_stop_requested.store(false, std::memory_order_release);
|
||||
|
||||
oboe::Result result = m_stream->requestStart();
|
||||
if (result != oboe::Result::OK)
|
||||
@@ -245,11 +263,12 @@ bool OboeAudioStream::Start()
|
||||
|
||||
void OboeAudioStream::Stop()
|
||||
{
|
||||
const std::lock_guard<std::recursive_mutex> guard(m_lock);
|
||||
if (!m_playing)
|
||||
return;
|
||||
|
||||
Console.WriteLn("(Oboe) Stopping stream...");
|
||||
m_stop_requested = true;
|
||||
m_stop_requested.store(true, std::memory_order_release);
|
||||
|
||||
oboe::Result result = m_stream->requestStop();
|
||||
if (result != oboe::Result::OK)
|
||||
@@ -260,6 +279,7 @@ void OboeAudioStream::Stop()
|
||||
|
||||
void OboeAudioStream::Close()
|
||||
{
|
||||
const std::lock_guard<std::recursive_mutex> guard(m_lock);
|
||||
Console.WriteLn("(Oboe) Closing stream...");
|
||||
if (m_playing)
|
||||
Stop();
|
||||
@@ -272,6 +292,9 @@ void OboeAudioStream::Close()
|
||||
|
||||
void OboeAudioStream::SetPaused(bool paused)
|
||||
{
|
||||
// This is the CPU-thread side of the race with onError(): without the lock, m_stream can be
|
||||
// reset by the reopen between the null check and the dereference below.
|
||||
const std::lock_guard<std::recursive_mutex> guard(m_lock);
|
||||
if (m_paused == paused)
|
||||
return;
|
||||
|
||||
|
||||
@@ -956,6 +956,25 @@ void MTGS::Freeze(FreezeAction mode, MTGS::FreezeData& data)
|
||||
|
||||
void MTGS::RunOnGSThread(AsyncCallType func)
|
||||
{
|
||||
// The ring is single-producer: s_WritePos is owned by the CPU/EE thread, and the send path
|
||||
// below is a relaxed load / slot write / release store with no CAS. A second producer makes
|
||||
// both writers claim the same slot and both advance the position, so one packet is dropped —
|
||||
// if the loser was a data-packet header the GS thread then parses payload qwords as command
|
||||
// tags and dereferences a garbage pointer as an AsyncCallType. It also desyncs the
|
||||
// pending-packet count, which lost-wakeup-deadlocks a WaitGS'ing EE against a sleeping GS
|
||||
// thread (see the same reasoning spelled out in PINE.cpp's BuildStatsJson).
|
||||
//
|
||||
// So: marshal first. Host::RunOnGSThread() is the primitive for that — it chains through
|
||||
// Host::RunOnCPUThread(), whose queue the CPU thread drains every vsync via
|
||||
// PollInputOnCPUThread(). Dev-only because the remaining Android/iOS offenders should surface
|
||||
// as a debuggable assert during development, not an abort in a shipped build.
|
||||
//
|
||||
// The data-packet path (PrepDataPacket/SendDataPacket/SendSimpleGSPacket) is deliberately not
|
||||
// asserted: it is reached only from Gif_Unit, which is EE-thread code by construction, and it
|
||||
// is hot enough that even a dev-build check per GIF packet is not worth it.
|
||||
pxAssertMsg(VMManager::Internal::IsOnCPUThread(),
|
||||
"MTGS::RunOnGSThread() off the CPU thread — use Host::RunOnGSThread() instead");
|
||||
|
||||
SendPointerPacket(Command::AsyncCall, 0, new AsyncCallType(std::move(func)));
|
||||
|
||||
// wake the gs thread in case it's sleeping
|
||||
|
||||
@@ -327,6 +327,13 @@ namespace VMManager
|
||||
/// Cleans up common host state, called on the CPU thread.
|
||||
void CPUThreadShutdown();
|
||||
|
||||
/// Whether the caller is the CPU thread, i.e. the thread that ran CPUThreadInitialize().
|
||||
/// The CPU thread owns EmuConfig and is the sole producer into the MTGS ring, so most core
|
||||
/// mutation is only legal from it — everything else must marshal via Host::RunOnCPUThread()
|
||||
/// (or Host::RunOnGSThread(), which chains through it). Returns true when no CPU thread is
|
||||
/// registered, so startup/teardown and CPU-thread-less test harnesses stay unencumbered.
|
||||
bool IsOnCPUThread();
|
||||
|
||||
/// Android: affinity mask of the performance ("big") CPU cluster hosting the
|
||||
/// EE/VU/GS threads, so adjacent helper threads (e.g. the Oboe audio callback)
|
||||
/// can pin onto the same cluster. Returns 0 when pinning is off / unresolved.
|
||||
|
||||
File diff suppressed because it is too large
Load Diff
@@ -734,6 +734,9 @@ public class NativeApp {
|
||||
public static native String getTitlesForSerial(String serial);
|
||||
|
||||
public static native boolean saveStateToSlot(int slot);
|
||||
/** True while the emulated memory card is mid-write, when a state save is refused to protect
|
||||
* the card. The counter only ticks down while the VM runs, so it does NOT clear while paused. */
|
||||
public static native boolean isMemcardBusy();
|
||||
public static native boolean loadStateFromSlot(int slot);
|
||||
public static native String getGamePathSlot(int slot);
|
||||
public static native byte[] getImageSlot(int slot);
|
||||
|
||||
@@ -2971,11 +2971,14 @@ static std::string ARMSX2PerGameSettingsPath(const std::string& serial, u32 crc)
|
||||
eeCycleRateOverride, eeCycleRate, fastBootOverride, fastBoot,
|
||||
enableCheats, enablePatches, enableGameFixes, enableGameDBHardwareFixes);
|
||||
|
||||
if (VMManager::HasValidVM()) {
|
||||
// EmuConfig and the MTGS ring are the CPU thread's; this runs on the UI thread.
|
||||
Host::RunOnCPUThread([]() {
|
||||
if (!VMManager::HasValidVM())
|
||||
return;
|
||||
VMManager::ReloadGameSettings();
|
||||
if (MTGS::IsOpen())
|
||||
MTGS::ApplySettings();
|
||||
}
|
||||
});
|
||||
}
|
||||
|
||||
+ (nullable NSString *)linkedDiscPathForELF:(nonnull NSString *)elfName {
|
||||
@@ -3527,9 +3530,13 @@ static std::string ARMSX2PerGameSettingsPath(const std::string& serial, u32 crc)
|
||||
if (!VMManager::HasValidVM())
|
||||
return;
|
||||
|
||||
VMManager::ApplySettings();
|
||||
if (MTGS::IsOpen())
|
||||
MTGS::ApplySettings();
|
||||
// ApplySettings owns EmuConfig and resets the JIT caches, and MTGS::ApplySettings pushes to
|
||||
// the single-producer ring — both the CPU thread's, and this runs on the UI thread.
|
||||
Host::RunOnCPUThread([]() {
|
||||
VMManager::ApplySettings();
|
||||
if (MTGS::IsOpen())
|
||||
MTGS::ApplySettings();
|
||||
});
|
||||
}
|
||||
|
||||
// Force any deferred base-settings INI write to disk immediately.
|
||||
@@ -3697,9 +3704,12 @@ static std::string ARMSX2PerGameSettingsPath(const std::string& serial, u32 crc)
|
||||
si.SetIntValue(section.UTF8String, key.UTF8String, value);
|
||||
Error error;
|
||||
si.Save(&error);
|
||||
VMManager::ReloadGameSettings();
|
||||
if (MTGS::IsOpen())
|
||||
MTGS::ApplySettings();
|
||||
// EmuConfig and the MTGS ring are the CPU thread's; this runs on the UI thread.
|
||||
Host::RunOnCPUThread([]() {
|
||||
VMManager::ReloadGameSettings();
|
||||
if (MTGS::IsOpen())
|
||||
MTGS::ApplySettings();
|
||||
});
|
||||
}
|
||||
|
||||
+ (void)setPerGameINIBoolForCurrentGame:(nonnull NSString *)section key:(nonnull NSString *)key value:(BOOL)value {
|
||||
@@ -3712,9 +3722,12 @@ static std::string ARMSX2PerGameSettingsPath(const std::string& serial, u32 crc)
|
||||
si.SetBoolValue(section.UTF8String, key.UTF8String, value);
|
||||
Error error;
|
||||
si.Save(&error);
|
||||
VMManager::ReloadGameSettings();
|
||||
if (MTGS::IsOpen())
|
||||
MTGS::ApplySettings();
|
||||
// EmuConfig and the MTGS ring are the CPU thread's; this runs on the UI thread.
|
||||
Host::RunOnCPUThread([]() {
|
||||
VMManager::ReloadGameSettings();
|
||||
if (MTGS::IsOpen())
|
||||
MTGS::ApplySettings();
|
||||
});
|
||||
}
|
||||
|
||||
+ (float)getPerGameINIFloat:(nonnull NSString *)section key:(nonnull NSString *)key defaultValue:(float)def forISO:(nonnull NSString *)isoName {
|
||||
@@ -3761,9 +3774,12 @@ static std::string ARMSX2PerGameSettingsPath(const std::string& serial, u32 crc)
|
||||
si.SetFloatValue(section.UTF8String, key.UTF8String, value);
|
||||
Error error;
|
||||
si.Save(&error);
|
||||
VMManager::ReloadGameSettings();
|
||||
if (MTGS::IsOpen())
|
||||
MTGS::ApplySettings();
|
||||
// EmuConfig and the MTGS ring are the CPU thread's; this runs on the UI thread.
|
||||
Host::RunOnCPUThread([]() {
|
||||
VMManager::ReloadGameSettings();
|
||||
if (MTGS::IsOpen())
|
||||
MTGS::ApplySettings();
|
||||
});
|
||||
}
|
||||
|
||||
+ (void)deletePerGameINIValueForCurrentGame:(nonnull NSString *)section key:(nonnull NSString *)key {
|
||||
@@ -3778,9 +3794,12 @@ static std::string ARMSX2PerGameSettingsPath(const std::string& serial, u32 crc)
|
||||
si.RemoveEmptySections();
|
||||
Error error;
|
||||
si.Save(&error);
|
||||
VMManager::ReloadGameSettings();
|
||||
if (MTGS::IsOpen())
|
||||
MTGS::ApplySettings();
|
||||
// EmuConfig and the MTGS ring are the CPU thread's; this runs on the UI thread.
|
||||
Host::RunOnCPUThread([]() {
|
||||
VMManager::ReloadGameSettings();
|
||||
if (MTGS::IsOpen())
|
||||
MTGS::ApplySettings();
|
||||
});
|
||||
}
|
||||
|
||||
+ (int)limiterMode
|
||||
|
||||
@@ -28,6 +28,7 @@
|
||||
#include "pcsx2/Config.h" // EmuConfig, GSConfig
|
||||
#include "pcsx2/Host.h"
|
||||
#include "pcsx2/Host/AudioStreamTypes.h"
|
||||
#include "pcsx2/MTGS.h" // Host::RunOnGSThread
|
||||
#include "pcsx2/INISettingsInterface.h"
|
||||
#include "pcsx2/PerformanceMetrics.h"
|
||||
#include "pcsx2/R5900.h"
|
||||
@@ -227,6 +228,18 @@ namespace Host
|
||||
std::fprintf(stderr, "@@CPU_TASK_WAIT_OK@@ id=%llu\n", task->id);
|
||||
std::fflush(stderr);
|
||||
}
|
||||
// Post to the GS thread from anywhere. Mirrors pcsx2-qt (QtHost.cpp): the MTGS ring is
|
||||
// single-producer and s_WritePos belongs to the CPU thread, so a UI-thread caller has to hop
|
||||
// to the CPU thread first and let it push the packet. Our UIKit callbacks and Swift bridge
|
||||
// entry points all run on the main thread, so this is the only correct route for them.
|
||||
// Fire-and-forget — never block a UIKit callback on the GS thread.
|
||||
void RunOnGSThread(std::function<void()> function)
|
||||
{
|
||||
RunOnCPUThread([fn = std::move(function)]() {
|
||||
if (MTGS::IsOpen())
|
||||
MTGS::RunOnGSThread(std::move(fn));
|
||||
}, false);
|
||||
}
|
||||
void ReportInfoAsync(std::string_view, std::string_view) {}
|
||||
void ReportErrorAsync(std::string_view title, std::string_view msg) {
|
||||
Console.Error("Host::ReportErrorAsync: %s - %s", std::string(title).c_str(), std::string(msg).c_str());
|
||||
|
||||
@@ -321,7 +321,10 @@ void ARMSX2ConfigureImGuiFonts(const char* reason)
|
||||
// Indent corner-anchored OSD by a small fixed clearance so it isn't clipped by the display's rounded corners
|
||||
constexpr double kOsdCornerInsetPt = 18.0;
|
||||
const float osd_inset = (float)(kOsdCornerInsetPt * scale);
|
||||
MTGS::RunOnGSThread([w, h, s, osd_inset]() {
|
||||
// -layoutSubviews is UIKit, i.e. the main thread, and it fires on every rotation and resize
|
||||
// with the VM running. The MTGS ring is single-producer and belongs to the CPU thread, so hop
|
||||
// there first (Host::RunOnGSThread chains RunOnCPUThread -> MTGS::RunOnGSThread).
|
||||
Host::RunOnGSThread([w, h, s, osd_inset]() {
|
||||
GSResizeDisplayWindow(w, h, s);
|
||||
ImGuiManager::SetOSDSafeAreaInsets(osd_inset, osd_inset, osd_inset, osd_inset);
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user