Merge pull request #16958 from hrydgard/rewind-cleanups

Rewind cleanups

Common/Serialize/Serializer.h

@@ -192,47 +192,65 @@ class CChunkFileReader
 		return (size_t)ptr;
 	}
 
-	// Expects ptr to have at least MeasurePtr bytes at ptr.
+	// If *saved is null, will allocate storage using malloc.
+	// If it's not null, it will be used, but only hope can save you from overruns at the end. For libretro.
 	template<class T>
-	static Error SavePtr(u8 *ptr, T &_class, size_t expected_size)
+	static Error MeasureAndSavePtr(T &_class, u8 **saved, size_t *savedSize)
 	{
-		const u8 *expected_end = ptr + expected_size;
-		PointerWrap p(&ptr, PointerWrap::MODE_WRITE);
+		u8 *ptr = nullptr;
+		PointerWrap p(&ptr, PointerWrap::MODE_MEASURE);
+		_class.DoState(p);
+		_assert_(p.error == PointerWrap::ERROR_NONE);
+
+		size_t measuredSize = p.Offset();
+		u8 *data;
+		if (*saved) {
+			data = *saved;
+		} else {
+			data = (u8 *)malloc(measuredSize);
+			if (!data)
+				return ERROR_BAD_ALLOC;
+		}
+
+		p.RewindForWrite(data);
 		_class.DoState(p);
 
-		if (p.error != PointerWrap::ERROR_FAILURE && (expected_end == ptr || expected_size == 0)) {
+		if (p.CheckAfterWrite()) {
+			*saved = data;
+			*savedSize = measuredSize;
 			return ERROR_NONE;
 		} else {
+			if (!*saved) {
+				free(data);
+			}
 			return ERROR_BROKEN_STATE;
 		}
 	}
 
+	// Duplicate of the above but takes and modifies a vector. Less invasive
+	// than modifying the rewind manager to keep things in something else than vectors.
 	template<class T>
-	static Error MeasureAndSavePtr(T &_class, u8 **saved, size_t *savedSize)
+	static Error MeasureAndSavePtr(T &_class, std::vector<u8> *saved)
 	{
 		u8 *ptr = nullptr;
 		PointerWrap p(&ptr, PointerWrap::MODE_MEASURE);
 		_class.DoState(p);
 		_assert_(p.error == PointerWrap::ERROR_NONE);
 
 		size_t measuredSize = p.Offset();
-		u8 *data = (u8 *)malloc(measuredSize);
-		if (!data)
-			return ERROR_BAD_ALLOC;
-
+		saved->resize(measuredSize);
+		u8 *data = saved->data();
 		p.RewindForWrite(data);
 		_class.DoState(p);
-
 		if (p.CheckAfterWrite()) {
-			*saved = data;
-			*savedSize = measuredSize;
 			return ERROR_NONE;
 		} else {
-			free(data);
+			saved->clear();
 			return ERROR_BROKEN_STATE;
 		}
 	}
 
+
 	// Load file template
 	template<class T>
 	static Error Load(const Path &filename, std::string *gitVersion, T& _class, std::string *failureReason)

Core/Config.cpp

@@ -537,7 +537,7 @@ static const ConfigSetting generalSettings[] = {
 	ConfigSetting("StateLoadUndoGame", &g_Config.sStateLoadUndoGame, "NA", true, false),
 	ConfigSetting("StateUndoLastSaveGame", &g_Config.sStateUndoLastSaveGame, "NA", true, false),
 	ConfigSetting("StateUndoLastSaveSlot", &g_Config.iStateUndoLastSaveSlot, -5, true, false), // Start with an "invalid" value
-	ConfigSetting("RewindFlipFrequency", &g_Config.iRewindFlipFrequency, 0, true, true),
+	ConfigSetting("RewindSnapshotInterval", &g_Config.iRewindSnapshotInterval, 0, true, true),
 
 	ConfigSetting("ShowOnScreenMessage", &g_Config.bShowOnScreenMessages, true, true, false),
 	ConfigSetting("ShowRegionOnGameIcon", &g_Config.bShowRegionOnGameIcon, false),

Core/Config.h

@@ -224,7 +224,7 @@ struct Config {
 	int iAnalogFpsMode; // 0 = auto, 1 = single direction, 2 = mapped to opposite
 	int iMaxRecent;
 	int iCurrentStateSlot;
-	int iRewindFlipFrequency;
+	int iRewindSnapshotInterval;
 	bool bUISound;
 	bool bEnableStateUndo;
 	std::string sStateLoadUndoGame;

Core/SaveState.cpp

@@ -91,35 +91,49 @@ namespace SaveState
 
 	CChunkFileReader::Error SaveToRam(std::vector<u8> &data) {
 		SaveStart state;
-		size_t sz = CChunkFileReader::MeasurePtr(state);
-		if (data.size() < sz)
-			data.resize(sz);
-		return CChunkFileReader::SavePtr(&data[0], state, sz);
+		return CChunkFileReader::MeasureAndSavePtr(state, &data);
 	}
 
 	CChunkFileReader::Error LoadFromRam(std::vector<u8> &data, std::string *errorString) {
 		SaveStart state;
 		return CChunkFileReader::LoadPtr(&data[0], state, errorString);
 	}
 
-	struct StateRingbuffer
-	{
-		StateRingbuffer(int size) : first_(0), next_(0), size_(size), base_(-1)
-		{
-			states_.resize(size);
-			baseMapping_.resize(size);
+	// This ring buffer of states is for rewind save states, which are kept in RAM.
+	// Save states are compressed against one of two reference saves (bases_), and the reference
+	// is switched to a fresh save every N saves, where N is BASE_USAGE_INTERVAL.
+	// The compression is a simple block based scheme where 0 means to copy a block from the base,
+	// and 1 means that the following bytes are the next block. See Compress/LockedDecompress.
+	class StateRingbuffer {
+	public:
+		StateRingbuffer() {
+			size_ = REWIND_NUM_STATES;
+			states_.resize(size_);
+			baseMapping_.resize(size_);
+		}
+
+		~StateRingbuffer() {
+			if (compressThread_.joinable()) {
+				compressThread_.join();
+			}
 		}
 
 		CChunkFileReader::Error Save()
 		{
+			rewindLastTime_ = time_now_d();
+
+			// Make sure we're not processing a previous save. That'll cause a hitch though, but at least won't
+			// crash due to contention over buffer_.
+			if (compressThread_.joinable())
+				compressThread_.join();
+
 			std::lock_guard<std::mutex> guard(lock_);
 
 			int n = next_++ % size_;
 			if ((next_ % size_) == first_)
 				++first_;
 
-			static std::vector<u8> buffer;
-			std::vector<u8> *compressBuffer = &buffer;
+			std::vector<u8> *compressBuffer = &buffer_;
 			CChunkFileReader::Error err;
 
 			if (base_ == -1 || ++baseUsage_ > BASE_USAGE_INTERVAL)
@@ -131,12 +145,13 @@ namespace SaveState
 				compressBuffer = &bases_[base_];
 			}
 			else
-				err = SaveToRam(buffer);
+				err = SaveToRam(buffer_);
 
 			if (err == CChunkFileReader::ERROR_NONE)
 				ScheduleCompress(&states_[n], compressBuffer, &bases_[base_]);
 			else
 				states_[n].clear();
+
 			baseMapping_[n] = base_;
 			return err;
 		}
@@ -155,7 +170,9 @@ namespace SaveState
 
 			static std::vector<u8> buffer;
 			LockedDecompress(buffer, states_[n], bases_[baseMapping_[n]]);
-			return LoadFromRam(buffer, errorString);
+			CChunkFileReader::Error error = LoadFromRam(buffer, errorString);
+			rewindLastTime_ = time_now_d();
+			return error;
 		}
 
 		void ScheduleCompress(std::vector<u8> *result, const std::vector<u8> *state, const std::vector<u8> *base)
@@ -177,18 +194,23 @@ namespace SaveState
 			if (first_ == 0 && next_ == 0)
 				return;
 
+			double start_time = time_now_d();
 			result.clear();
+			result.reserve(512 * 1024);
 			for (size_t i = 0; i < state.size(); i += BLOCK_SIZE)
 			{
 				int blockSize = std::min(BLOCK_SIZE, (int)(state.size() - i));
 				if (i + blockSize > base.size() || memcmp(&state[i], &base[i], blockSize) != 0)
 				{
 					result.push_back(1);
-					result.insert(result.end(), state.begin() + i, state.begin() +i + blockSize);
+					result.insert(result.end(), state.begin() + i, state.begin() + i + blockSize);
 				}
 				else
 					result.push_back(0);
 			}
+
+			double taken_s = time_now_d() - start_time;
+			DEBUG_LOG(SAVESTATE, "Rewind: Compressed save from %d bytes to %d in %0.2f ms.", (int)state.size(), (int)result.size(), taken_s * 1000.0);
 		}
 
 		void LockedDecompress(std::vector<u8> &result, const std::vector<u8> &compressed, const std::vector<u8> &base)
@@ -211,7 +233,11 @@ namespace SaveState
 					int blockSize = std::min(BLOCK_SIZE, (int)(compressed.size() - i));
 					result.insert(result.end(), compressed.begin() + i, compressed.begin() + i + blockSize);
 					i += blockSize;
-					basePos += blockSize;
+					// This check is to avoid advancing basePos out of range, which MSVC catches.
+					// When this happens, we're at the end of decoding anyway.
+					if (base.end() - basePos >= blockSize) {
+						basePos += blockSize;
+					}
 				}
 			}
 		}
@@ -225,31 +251,68 @@ namespace SaveState
 			std::lock_guard<std::mutex> guard(lock_);
 			first_ = 0;
 			next_ = 0;
+			for (auto &b : bases_) {
+				b.clear();
+			}
+			baseMapping_.clear();
+			baseMapping_.resize(size_);
+			for (auto &s : states_) {
+				s.clear();
+			}
+			buffer_.clear();
+			base_ = -1;
+			baseUsage_ = 0;
+			rewindLastTime_ = time_now_d();
 		}
 
 		bool Empty() const
 		{
 			return next_ == first_;
 		}
 
-		static const int BLOCK_SIZE;
+		void Process() {
+			if (g_Config.iRewindSnapshotInterval <= 0) {
+				return;
+			}
+
+			// For fast-forwarding, otherwise they may be useless and too close.
+			double now = time_now_d();
+			double diff = now - rewindLastTime_;
+			if (diff < g_Config.iRewindSnapshotInterval)
+				return;
+
+			DEBUG_LOG(SAVESTATE, "Saving rewind state");
+			Save();
+		}
+
+		void NotifyState() {
+			// Prevent saving snapshots immediately after loading or saving a state.
+			rewindLastTime_ = time_now_d();
+		}
+
+	private:
+		static const int BLOCK_SIZE = 8192;
+		static const int REWIND_NUM_STATES = 20;
 		// TODO: Instead, based on size of compressed state?
-		static const int BASE_USAGE_INTERVAL;
+		static const int BASE_USAGE_INTERVAL = 15;
 
 		typedef std::vector<u8> StateBuffer;
 
-		int first_;
-		int next_;
+		int first_ = 0;
+		int next_ = 0;
 		int size_;
 
 		std::vector<StateBuffer> states_;
 		StateBuffer bases_[2];
 		std::vector<int> baseMapping_;
 		std::mutex lock_;
 		std::thread compressThread_;
+		std::vector<u8> buffer_;
 
-		int base_;
-		int baseUsage_;
+		int base_ = -1;
+		int baseUsage_ = 0;
+
+		double rewindLastTime_ = 0.0f;
 	};
 
 	static bool needsProcess = false;
@@ -267,14 +330,8 @@ namespace SaveState
 	static std::string saveStateInitialGitVersion = "";
 
 	// TODO: Should this be configurable?
-	static const int REWIND_NUM_STATES = 20;
 	static const int SCREENSHOT_FAILURE_RETRIES = 15;
-	static StateRingbuffer rewindStates(REWIND_NUM_STATES);
-	// TODO: Any reason for this to be configurable?
-	const static float rewindMaxWallFrequency = 1.0f;
-	static double rewindLastTime = 0.0f;
-	const int StateRingbuffer::BLOCK_SIZE = 8192;
-	const int StateRingbuffer::BASE_USAGE_INTERVAL = 15;
+	static StateRingbuffer rewindStates;
 
 	void SaveStart::DoState(PointerWrap &p)
 	{
@@ -345,13 +402,15 @@ namespace SaveState
 
 	void Load(const Path &filename, int slot, Callback callback, void *cbUserData)
 	{
+		rewindStates.NotifyState();
 		if (coreState == CoreState::CORE_RUNTIME_ERROR)
 			Core_EnableStepping(true, "savestate.load", 0);
 		Enqueue(Operation(SAVESTATE_LOAD, filename, slot, callback, cbUserData));
 	}
 
 	void Save(const Path &filename, int slot, Callback callback, void *cbUserData)
 	{
+		rewindStates.NotifyState();
 		if (coreState == CoreState::CORE_RUNTIME_ERROR)
 			Core_EnableStepping(true, "savestate.save", 0);
 		Enqueue(Operation(SAVESTATE_SAVE, filename, slot, callback, cbUserData));
@@ -766,22 +825,6 @@ namespace SaveState
 		return false;
 	}
 
-	static inline void CheckRewindState()
-	{
-		if (gpuStats.numFlips % g_Config.iRewindFlipFrequency != 0)
-			return;
-
-		// For fast-forwarding, otherwise they may be useless and too close.
-		double now = time_now_d();
-		float diff = now - rewindLastTime;
-		if (diff < rewindMaxWallFrequency)
-			return;
-
-		rewindLastTime = now;
-		DEBUG_LOG(BOOT, "Saving rewind state");
-		rewindStates.Save();
-	}
-
 	bool HasLoadedState() {
 		return hasLoadedState;
 	}
@@ -838,8 +881,7 @@ namespace SaveState
 
 	void Process()
 	{
-		if (g_Config.iRewindFlipFrequency != 0 && gpuStats.numFlips != 0)
-			CheckRewindState();
+		rewindStates.Process();
 
 		if (!needsProcess)
 			return;

UI/GameSettingsScreen.cpp

@@ -1054,8 +1054,8 @@ void GameSettingsScreen::CreateSystemSettings(UI::ViewGroup *systemSettings) {
 		return UI::EVENT_CONTINUE;
 	});
 	lockedMhz->SetZeroLabel(sy->T("Auto"));
-	PopupSliderChoice *rewindFreq = systemSettings->Add(new PopupSliderChoice(&g_Config.iRewindFlipFrequency, 0, 1800, sy->T("Rewind Snapshot Frequency", "Rewind Snapshot Frequency (mem hog)"), screenManager(), sy->T("frames, 0:off")));
-	rewindFreq->SetZeroLabel(sy->T("Off"));
+	PopupSliderChoice *rewindInterval = systemSettings->Add(new PopupSliderChoice(&g_Config.iRewindSnapshotInterval, 0, 60, sy->T("Rewind Snapshot Interval"), screenManager(), sy->T("seconds, 0:off")));
+	rewindInterval->SetZeroLabel(sy->T("Off"));
 
 	systemSettings->Add(new ItemHeader(sy->T("General")));
 

UI/PauseScreen.cpp

@@ -302,7 +302,7 @@ void GamePauseScreen::CreateViews() {
 		saveUndoButton->OnClick.Handle(this, &GamePauseScreen::OnLastSaveUndo);
 	}
 
-	if (g_Config.iRewindFlipFrequency > 0) {
+	if (g_Config.iRewindSnapshotInterval > 0) {
 		UI::Choice *rewindButton = buttonRow->Add(new Choice(pa->T("Rewind")));
 		rewindButton->SetEnabled(SaveState::CanRewind());
 		rewindButton->OnClick.Handle(this, &GamePauseScreen::OnRewind);

assets/lang/ar_AE.ini

@@ -1138,7 +1138,7 @@ Failed to save state. Error in the file system. = ‎فشل في حفظ الحا
 Fast (lag on slow storage) = ‎سريع (بطي علي الذواكر البطئيه)
 Fast Memory = ‎الذاكرة السريعة (غير ثابت)
 Force real clock sync (slower, less lag) = Force real clock sync (slower, less lag)
-frames, 0:off = إطارات, 0 = ‎مُغلق
+seconds, 0:off = إطارات, 0 = ‎مُغلق
 Games list settings = Games list settings
 General = ‎العام
 Grid icon size = Grid icon size