Rewind cleanups #16958
Rewind cleanups #16958
Conversation
hrydgard
force-pushed
the
rewind-cleanups
branch
from
6fc31e1 to
975e5f8
Compare
There was a problem hiding this comment.
It's probably fine if it's based on real seconds, although it does mean if you suffered some lag or got some notifications or something you might have some unnecessarily close states.
I've abused rewind to trap an event and export out the rewind state before last as an actual save state file, so as to make it easier to find things. I think that would still work using wall or game seconds.
-[Unknown]
| _class.DoState(p); | ||
|
|
||
| if (p.error != PointerWrap::ERROR_FAILURE && (expected_end == ptr || expected_size == 0)) { |
There was a problem hiding this comment.
It might still be nice to validate that the size we got to matched measuredSize, we've had bugs in the past so it's nice if it trips.
-[Unknown]
There was a problem hiding this comment.
This is done internally in CheckAfterWrite.
UI/GameSettingsScreen.cpp
Outdated
| @@ -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, 30, sy->T("Rewind Snapshot Interval"), screenManager(), sy->T("seconds, 0:off"))); | |||
There was a problem hiding this comment.
There's probably no reason the max couldn't be 60, giving you one every minute.
-[Unknown]
There was a problem hiding this comment.
I'm fine with that, changing.
| if (!data) | ||
| return ERROR_BAD_ALLOC; | ||
|
|
||
| saved->resize(measuredSize); |
There was a problem hiding this comment.
Hm, I think this was a problem before but it'd be more likely now (and especially if using zstd.)
If the compress thread is running and didn't finish before this one, the resize might relocate the memory and cause a crash. Before it would only resize up, which was still a problem, but perhaps less likely.
One solution would be to stall on compressThread_.join(); before starting a new Save().
Maybe this was the crash you saw before.
-[Unknown]
There was a problem hiding this comment.
No, the flaky crash I saw before was just the iterator assert, which for whatever reason wasn't triggered every time.
But yes, we should join the compressThread_ before saving (or allow concurrent ones with their own buffers, but kinda overkill). Fixing.
hrydgard
force-pushed
the
rewind-cleanups
branch
from
f6ebd72 to
cebb885
Compare
Support was added on the PPSSPP side in 2021 (crashing bug fixed): hrydgard/ppsspp@379f075 And further enhanced recently in 2023: hrydgard/ppsspp#16958 However, this info file was never updated, so the 2021 update was made incompatible again after this RA update in 2022: libretro/RetroArch@e541dd5 This commit should bring everything back in line.
Replaces #16955 .
Cleans up some code, switches to MeasureAndSavePtr everywhere for safety.
Also changes the setting to be based on wall-time seconds. Spamming rewind saves when fast-forwarding doesn't make that much sense, IMHO, and this is simply easier to understand when configuring and framerate-independent.
I guess could also be based on in-game seconds to keep the ease of configuration, but I think this is fine. Opinions?