SaveState: Misc refactoring and improvements

- Block until saves are completed when resuming.
 - Save shutdown state asynchronously.
 - Add function to read screenshot out of zip file (useful for
   previous, now we're using zstd).

pcsx2-qt/EmuThread.cpp

@@ -249,6 +249,7 @@ void EmuThread::run()
 	stopBackgroundControllerPollTimer();
 	destroyBackgroundControllerPollTimer();
 	InputManager::CloseSources();
+	VMManager::WaitForSaveStateFlush();
 	VMManager::Internal::ReleaseMemory();
 	PerformanceMetrics::SetCPUThread(Threading::ThreadHandle());
 	moveToThread(m_ui_thread);

pcsx2-qt/MainWindow.cpp

@@ -835,6 +835,9 @@ void MainWindow::onGameListEntryActivated()
 		return;
 	}
 
+	// we might still be saving a resume state...
+	VMManager::WaitForSaveStateFlush();
+
 	const std::optional<bool> resume = promptForResumeState(
 		QString::fromStdString(VMManager::GetSaveStateFileName(entry->serial.c_str(), entry->crc, -1)));
 	if (!resume.has_value())
@@ -928,6 +931,9 @@ void MainWindow::onStartFileActionTriggered()
 	std::shared_ptr<VMBootParameters> params = std::make_shared<VMBootParameters>();
 	params->filename = filename.toStdString();
 
+	// we might still be saving a resume state...
+	VMManager::WaitForSaveStateFlush();
+
 	const std::optional<bool> resume(
 		promptForResumeState(
 			QString::fromStdString(VMManager::GetSaveStateFileName(params->filename.c_str(), -1))));

pcsx2/SaveState.cpp

@@ -780,45 +780,109 @@ static bool SaveState_CompressScreenshot(SaveStateScreenshotData* data, zip_t* z
 	return true;
 }
 
+static bool SaveState_ReadScreenshot(zip_t* zf, u32* out_width, u32* out_height, std::vector<u32>* out_pixels)
+{
+	auto zff = zip_fopen_managed(zf, EntryFilename_Screenshot, 0);
+	if (!zff)
+		return false;
+
+	png_structp png_ptr = png_create_read_struct(PNG_LIBPNG_VER_STRING, nullptr, nullptr, nullptr);
+	if (!png_ptr)
+		return false;
+
+	png_infop info_ptr = png_create_info_struct(png_ptr);
+	if (!info_ptr)
+	{
+		png_destroy_read_struct(&png_ptr, nullptr, nullptr);
+		return false;
+	}
+
+	ScopedGuard cleanup([&png_ptr, &info_ptr]() {
+		png_destroy_read_struct(&png_ptr, &info_ptr, nullptr);
+	});
+
+	if (setjmp(png_jmpbuf(png_ptr)))
+		return false;
+
+	png_set_read_fn(png_ptr, zff.get(), [](png_structp png_ptr, png_bytep data_ptr, png_size_t size) {
+		zip_fread(static_cast<zip_file_t*>(png_get_io_ptr(png_ptr)), data_ptr, size);
+	});
+
+	png_read_info(png_ptr, info_ptr);
+
+	png_uint_32 width = 0;
+	png_uint_32 height = 0;
+	int bitDepth = 0;
+	int colorType = -1;
+	if (png_get_IHDR(png_ptr, info_ptr, &width, &height, &bitDepth, &colorType, nullptr, nullptr, nullptr) != 1 ||
+		width == 0 || height == 0)
+	{
+		return false;
+	}
+
+	const png_uint_32 bytesPerRow = png_get_rowbytes(png_ptr, info_ptr);
+	std::vector<u8> rowData(bytesPerRow);
+
+	*out_width = width;
+	*out_height = height;
+	out_pixels->resize(width * height);
+
+	for (u32 y = 0; y < height; y++)
+	{
+		png_read_row(png_ptr, static_cast<png_bytep>(rowData.data()), nullptr);
+
+		const u8* row_ptr = rowData.data();
+		u32* out_ptr = &out_pixels->at(y * width);
+		if (colorType == PNG_COLOR_TYPE_RGB)
+		{
+			for (u32 x = 0; x < width; x++)
+			{
+				u32 pixel = static_cast<u32>(*(row_ptr)++);
+				pixel |= static_cast<u32>(*(row_ptr)++) << 8;
+				pixel |= static_cast<u32>(*(row_ptr)++) << 16;
+				pixel |= static_cast<u32>(*(row_ptr)++) << 24;
+				*(out_ptr++) = pixel | 0xFF000000u; // make opaque
+			}
+		}
+		else if (colorType == PNG_COLOR_TYPE_RGBA)
+		{
+			for (u32 x = 0; x < width; x++)
+			{
+				u32 pixel;
+				std::memcpy(&pixel, row_ptr, sizeof(u32));
+				row_ptr += sizeof(u32);
+				*(out_ptr++) = pixel | 0xFF000000u; // make opaque
+			}
+		}
+	}
+
+	return true;
+}
+
 // --------------------------------------------------------------------------------------
 //  CompressThread_VmState
 // --------------------------------------------------------------------------------------
-void SaveState_ZipToDisk(std::unique_ptr<ArchiveEntryList> srclist, std::unique_ptr<SaveStateScreenshotData> screenshot, std::string filename, s32 slot_for_message)
+static bool SaveState_AddToZip(zip_t* zf, ArchiveEntryList* srclist, SaveStateScreenshotData* screenshot)
 {
-#ifndef PCSX2_CORE
-	wxGetApp().StartPendingSave();
-#endif
-
-	zip_error_t ze = {};
-	auto zf = zip_open_managed(filename.c_str(), ZIP_CREATE | ZIP_TRUNCATE, &ze);
-	if (!zf)
-	{
-		Console.Error("Failed to open zip file '%s' for save state: %s", filename.c_str(), zip_error_strerror(&ze));
-		return;
-	}
-
-	// discard zip file if we fail saving something
-	ScopedGuard zip_discarder([&zf]() { zip_discard(zf.release()); });
-
 	// use zstd compression, it can be 10x+ faster for saving.
 	const u32 compression = EmuConfig.SavestateZstdCompression ? ZIP_CM_ZSTD : ZIP_CM_DEFLATE;
 	const u32 compression_level = 0;
 
 	// version indicator
 	{
-		zip_source_t* const zs = zip_source_buffer(zf.get(), &g_SaveVersion, sizeof(g_SaveVersion), 0);
+		zip_source_t* const zs = zip_source_buffer(zf, &g_SaveVersion, sizeof(g_SaveVersion), 0);
 		if (!zs)
-			return;
+			return false;
 
 		// NOTE: Source should not be freed if successful.
-		const s64 fi = zip_file_add(zf.get(), EntryFilename_StateVersion, zs, ZIP_FL_ENC_UTF_8);
+		const s64 fi = zip_file_add(zf, EntryFilename_StateVersion, zs, ZIP_FL_ENC_UTF_8);
 		if (fi < 0)
 		{
 			zip_source_free(zs);
-			return;
+			return false;
 		}
 
-		zip_set_file_compression(zf.get(), fi, ZIP_CM_STORE, 0);
+		zip_set_file_compression(zf, fi, ZIP_CM_STORE, 0);
 	}
 
 	const uint listlen = srclist->GetLength();
@@ -828,40 +892,67 @@ void SaveState_ZipToDisk(std::unique_ptr<ArchiveEntryList> srclist, std::unique_
 		if (!entry.GetDataSize())
 			continue;
 
-		zip_source_t* const zs = zip_source_buffer(zf.get(), srclist->GetPtr(entry.GetDataIndex()), entry.GetDataSize(), 0);
+		zip_source_t* const zs = zip_source_buffer(zf, srclist->GetPtr(entry.GetDataIndex()), entry.GetDataSize(), 0);
 		if (!zs)
-			return;
+			return false;
 
-		const s64 fi = zip_file_add(zf.get(), entry.GetFilename().c_str(), zs, ZIP_FL_ENC_UTF_8);
+		const s64 fi = zip_file_add(zf, entry.GetFilename().c_str(), zs, ZIP_FL_ENC_UTF_8);
 		if (fi < 0)
 		{
 			zip_source_free(zs);
-			return;
+			return false;
 		}
 
-		zip_set_file_compression(zf.get(), fi, compression, compression_level);
+		zip_set_file_compression(zf, fi, compression, compression_level);
 	}
 
 	if (screenshot)
-		SaveState_CompressScreenshot(screenshot.get(), zf.get());
+	{
+		if (!SaveState_CompressScreenshot(screenshot, zf))
+			return false;
+	}
 
-	// force the zip to close, this is the expensive part with libzip.
-	zf.reset();
-
-#ifdef PCSX2_CORE
-	if (slot_for_message >= 0 && VMManager::HasValidVM())
-		Host::AddKeyedFormattedOSDMessage(StringUtil::StdStringFromFormat("SaveStateSlot%d", slot_for_message), 10.0f, "State saved to slot %d.", slot_for_message);
-#else
-	Console.WriteLn("(gzipThread) Data saved to disk without error.");
-	wxGetApp().ClearPendingSave();
-#endif
+	return true;
 }
 
-
-void SaveState_ZipToDiskOnThread(std::unique_ptr<ArchiveEntryList> srclist, std::unique_ptr<SaveStateScreenshotData> screenshot, std::string filename, s32 slot_for_message)
+bool SaveState_ZipToDisk(std::unique_ptr<ArchiveEntryList> srclist, std::unique_ptr<SaveStateScreenshotData> screenshot, const char* filename)
 {
-	std::thread threaded_save(SaveState_ZipToDisk, std::move(srclist), std::move(screenshot), std::move(filename), slot_for_message);
-	threaded_save.detach();
+	zip_error_t ze = {};
+	zip_source_t* zs = zip_source_file_create(filename, 0, 0, &ze);
+	zip_t* zf = nullptr;
+	if (zs && !(zf = zip_open_from_source(zs, ZIP_CREATE | ZIP_TRUNCATE, &ze)))
+	{
+		Console.Error("Failed to open zip file '%s' for save state: %s", filename, zip_error_strerror(&ze));
+
+		// have to clean up source
+		zip_source_free(zs);
+		return false;
+	}
+
+	// discard zip file if we fail saving something
+	if (!SaveState_AddToZip(zf, srclist.get(), screenshot.get()))
+	{
+		Console.Error("Failed to save state to zip file '%s'", filename);
+		zip_discard(zf);
+		return false;
+	}
+
+	// force the zip to close, this is the expensive part with libzip.
+	zip_close(zf);
+	return true;
+}
+
+bool SaveState_ReadScreenshot(const std::string& filename, u32* out_width, u32* out_height, std::vector<u32>* out_pixels)
+{
+	zip_error_t ze = {};
+	auto zf = zip_open_managed(filename.c_str(), ZIP_RDONLY, &ze);
+	if (!zf)
+	{
+		Console.Error("Failed to open zip file '%s' for save state screenshot: %s", filename.c_str(), zip_error_strerror(&ze));
+		return false;
+	}
+
+	return SaveState_ReadScreenshot(zf.get(), out_width, out_height, out_pixels);
 }
 
 static void CheckVersion(const std::string& filename, zip_t* zf)

pcsx2/SaveState.h

@@ -58,8 +58,8 @@ class ArchiveEntryList;
 // These functions assume that the caller has paused the core thread.
 extern std::unique_ptr<ArchiveEntryList> SaveState_DownloadState();
 extern std::unique_ptr<SaveStateScreenshotData> SaveState_SaveScreenshot();
-extern void SaveState_ZipToDisk(std::unique_ptr<ArchiveEntryList> srclist, std::unique_ptr<SaveStateScreenshotData> screenshot, std::string filename, s32 slot_for_message);
-extern void SaveState_ZipToDiskOnThread(std::unique_ptr<ArchiveEntryList> srclist, std::unique_ptr<SaveStateScreenshotData> screenshot, std::string filename, s32 slot_for_message);
+extern bool SaveState_ZipToDisk(std::unique_ptr<ArchiveEntryList> srclist, std::unique_ptr<SaveStateScreenshotData> screenshot, const char* filename);
+extern bool SaveState_ReadScreenshot(const std::string& filename, u32* out_width, u32* out_height, std::vector<u32>* out_pixels);
 extern void SaveState_UnzipFromDisk(const std::string& filename);
 
 // --------------------------------------------------------------------------------------

pcsx2/VMManager.cpp

@@ -84,7 +84,13 @@ namespace VMManager
 
 	static std::string GetCurrentSaveStateFileName(s32 slot);
 	static bool DoLoadState(const char* filename);
-	static bool DoSaveState(const char* filename, s32 slot_for_message, bool save_on_thread);
+	static bool DoSaveState(const char* filename, s32 slot_for_message, bool zip_on_thread);
+	static void ZipSaveState(std::unique_ptr<ArchiveEntryList> elist,
+		std::unique_ptr<SaveStateScreenshotData> screenshot, std::string osd_key,
+		const char* filename, s32 slot_for_message);
+	static void ZipSaveStateOnThread(std::unique_ptr<ArchiveEntryList> elist,
+		std::unique_ptr<SaveStateScreenshotData> screenshot, std::string osd_key,
+		std::string filename, s32 slot_for_message);
 
 	static void SetTimerResolutionIncreased(bool enabled);
 	static void SetEmuThreadAffinities(bool force);
@@ -98,6 +104,9 @@ static u64 s_emu_thread_affinity;
 static std::atomic<VMState> s_state{VMState::Shutdown};
 static std::atomic_bool s_cpu_implementation_changed{false};
 
+static std::deque<std::thread> s_save_state_threads;
+static std::mutex s_save_state_threads_mutex;
+
 static std::mutex s_info_mutex;
 static std::string s_disc_path;
 static u32 s_game_crc;
@@ -772,7 +781,7 @@ void VMManager::Shutdown(bool save_resume_state)
 	if (!GSDumpReplayer::IsReplayingDump() && save_resume_state)
 	{
 		std::string resume_file_name(GetCurrentSaveStateFileName(-1));
-		if (!resume_file_name.empty() && !DoSaveState(resume_file_name.c_str(), -1, false))
+		if (!resume_file_name.empty() && !DoSaveState(resume_file_name.c_str(), -1, true))
 			Console.Error("Failed to save resume state");
 	}
 	else if (GSDumpReplayer::IsReplayingDump())
@@ -914,25 +923,91 @@ bool VMManager::DoSaveState(const char* filename, s32 slot_for_message, bool zip
 	if (GSDumpReplayer::IsReplayingDump())
 		return false;
 
+	std::string osd_key(StringUtil::StdStringFromFormat("SaveStateSlot%d", slot_for_message));
+
 	try
 	{
-		std::unique_ptr<ArchiveEntryList> elist = SaveState_DownloadState();
-		if (zip_on_thread)
-			SaveState_ZipToDiskOnThread(std::move(elist), SaveState_SaveScreenshot(), filename, slot_for_message);
-		else
-			SaveState_ZipToDisk(std::move(elist), SaveState_SaveScreenshot(), filename, slot_for_message);
+		std::unique_ptr<ArchiveEntryList> elist(SaveState_DownloadState());
+		std::unique_ptr<SaveStateScreenshotData> screenshot(SaveState_SaveScreenshot());
+
+		if (zip_on_thread)
+		{
+			// lock order here is important; the thread could exit before we resume here.
+			std::unique_lock lock(s_save_state_threads_mutex);
+			s_save_state_threads.emplace_back(&VMManager::ZipSaveStateOnThread,
+				std::move(elist), std::move(screenshot), std::move(osd_key), std::string(filename),
+				slot_for_message);
+		}
+		else
+		{
+			ZipSaveState(std::move(elist), std::move(screenshot), std::move(osd_key), filename, slot_for_message);
+		}
 
-		Host::InvalidateSaveStateCache();
 		Host::OnSaveStateSaved(filename);
 		return true;
 	}
 	catch (Exception::BaseException& e)
 	{
-		Host::AddFormattedOSDMessage(15.0f, "Failed to save save state: %s", static_cast<const char*>(e.DiagMsg().c_str()));
+		Host::AddKeyedFormattedOSDMessage(std::move(osd_key), 15.0f, "Failed to save save state: %s", static_cast<const char*>(e.DiagMsg().c_str()));
 		return false;
 	}
 }
 
+void VMManager::ZipSaveState(std::unique_ptr<ArchiveEntryList> elist,
+	std::unique_ptr<SaveStateScreenshotData> screenshot, std::string osd_key,
+	const char* filename, s32 slot_for_message)
+{
+	Common::Timer timer;
+
+	if (SaveState_ZipToDisk(std::move(elist), std::move(screenshot), filename))
+	{
+		if (slot_for_message >= 0 && VMManager::HasValidVM())
+			Host::AddKeyedFormattedOSDMessage(std::move(osd_key), 10.0f, "State saved to slot %d.", slot_for_message);
+	}
+	else
+	{
+		Host::AddKeyedFormattedOSDMessage(std::move(osd_key), 15.0f, "Failed to save save state to slot %d", slot_for_message);
+	}
+
+	DevCon.WriteLn("Zipping save state to '%s' took %.2f ms", filename, timer.GetTimeMilliseconds());
+
+	Host::InvalidateSaveStateCache();
+}
+
+void VMManager::ZipSaveStateOnThread(std::unique_ptr<ArchiveEntryList> elist, std::unique_ptr<SaveStateScreenshotData> screenshot,
+	std::string osd_key, std::string filename, s32 slot_for_message)
+{
+	ZipSaveState(std::move(elist), std::move(screenshot), std::move(osd_key), filename.c_str(), slot_for_message);
+
+	// remove ourselves from the thread list. if we're joining, we might not be in there.
+	const auto this_id = std::this_thread::get_id();
+	std::unique_lock lock(s_save_state_threads_mutex);
+	for (auto it = s_save_state_threads.begin(); it != s_save_state_threads.end(); ++it)
+	{
+		if (it->get_id() == this_id)
+		{
+			it->detach();
+			s_save_state_threads.erase(it);
+			break;
+		}
+	}
+}
+
+void VMManager::WaitForSaveStateFlush()
+{
+	std::unique_lock lock(s_save_state_threads_mutex);
+	while (!s_save_state_threads.empty())
+	{
+		// take a thread from the list and join with it. it won't self detatch then, but that's okay,
+		// since we're joining with it here.
+		std::thread save_thread(std::move(s_save_state_threads.front()));
+		s_save_state_threads.pop_front();
+		lock.unlock();
+		save_thread.join();
+		lock.lock();
+	}
+}
+
 bool VMManager::LoadState(const char* filename)
 {
 	// TODO: Save the current state so we don't need to reset.

pcsx2/VMManager.h

@@ -117,6 +117,9 @@ namespace VMManager
 	/// Saves state to the specified slot.
 	bool SaveStateToSlot(s32 slot, bool zip_on_thread = true);
 
+	/// Waits until all compressing save states have finished saving to disk.
+	void WaitForSaveStateFlush();
+
 	/// Returns the current limiter mode.
 	LimiterModeType GetLimiterMode();
 

pcsx2/gui/SysState.cpp

@@ -61,7 +61,20 @@ protected:
 		std::unique_ptr<ArchiveEntryList> elist = SaveState_DownloadState();
 		UI_EnableStateActions();
 		paused_core.AllowResume();
-		SaveState_ZipToDiskOnThread(std::move(elist), nullptr, StringUtil::wxStringToUTF8String(m_filename), -1);
+
+		std::thread kickoff(&SysExecEvent_SaveState::ZipThreadProc,
+			std::move(elist), StringUtil::wxStringToUTF8String(m_filename));
+		kickoff.detach();
+	}
+
+	static void ZipThreadProc(std::unique_ptr<ArchiveEntryList> elist, std::string filename)
+	{
+		wxGetApp().StartPendingSave();
+		if (SaveState_ZipToDisk(std::move(elist), nullptr, filename.c_str()))
+			Console.WriteLn("(gzipThread) Data saved to disk without error.");
+		else
+			Console.Error("Failed to zip state to '%s'", filename.c_str());
+		wxGetApp().ClearPendingSave();
 	}
 };