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

Calling retro_serialize needs to advance the core state if the CPU is in a unsafe state #504

Open
N7Alpha opened this issue Sep 4, 2023 · 6 comments
Labels
architectural bug help wanted Extra attention is needed mitigate question Further information is requested revisit later

Comments

@N7Alpha
Copy link

N7Alpha commented Sep 4, 2023

It seems like the current implementation of retro_serialize is dependent on passing control flow to mupen64 and advancing 1 frame. This looks like a deliberate design choice, so I'm guessing modifying retro_serialize so it doesn't tick the core would be too difficult. Alternatively I think just adding an log_cb(RETRO_LOG_INFO, "retro_serialize called ticking one frame..."); or something similar would be a good enough solution. So I'd at least see my console getting spammed and would know I was doing something unintended.

Why I care

This behavior was pretty confusing though as I thought the core might be running on a background thread or something. This was pretty tricky to debug though and I spent like 4 hours trying to figure it out. I was looking into different compression algorithms for the savestate that's why I was saving every frame.

@m4xw
Copy link
Collaborator

m4xw commented Sep 4, 2023

This behaviour is expected and required. The core might be in a unsafe state to take a savestate, so it needs to be advanced to the next VI (not frame!) to be in a consistent internal state to serialize a savestate.
It may or may not generate a frame in that time

@m4xw m4xw closed this as completed Sep 4, 2023
@N7Alpha
Copy link
Author

N7Alpha commented Sep 4, 2023

@m4xw Could you address the idea about adding a log message? I feel like that is small enough of a change that I would have caught the issue I was having earlier. If not that’s fine. You just didn’t mention it.

For whatever reason the VI were always advancing to the next frame which regressed retro_serialize to retro_run practically. Although this might not always be the case it just so happened to be with the way I had the core setup. I commented out retro_run and the core was running at my monitors refresh rate/60 times speed with only calling retro_serialize.

@m4xw
Copy link
Collaborator

m4xw commented Sep 4, 2023

Its a perf sensitive codepath for some features so can't be doing a log, maybe with NDEBUG.
If VI == Frame then it might be a game where the INI forces "On Color Image Change" VI Mode and I think threaded renderer had also some behavioural caveats there. This can also happen if FB Emu isnt active for some games.
I cant remember what I did when I added support for runahead locally, maybe its in some of my stashes on my workstation.
Technically a savestate after a Frame (on the very next VI) is guaranteed to be safe, thats what I relied on pre-threaded renderer. The savestate job is also done on another thread with threaded renderer and we need to yield back there to not have the core queue be stuck on pushing more GL commands to the FIFO, but without threaded renderer we could even just poke the internal state and do a hot-path and just fallback to advancing VI when the timing is really bad.
Wish i could remember the solution i had back then right now :P

@m4xw m4xw added help wanted Extra attention is needed question Further information is requested revisit later architectural bug labels Sep 4, 2023
@m4xw
Copy link
Collaborator

m4xw commented Sep 4, 2023

You are free to play around with the codepaths, if you really want. Its on my backlog of problems for future M4

@m4xw m4xw added the mitigate label Sep 4, 2023
@m4xw m4xw changed the title Calling retro_serialize ticks the core one frame Calling retro_serialize needs to advance the core state if the CPU is in a unsafe state Sep 4, 2023
@m4xw
Copy link
Collaborator

m4xw commented Sep 4, 2023

Usually i leave the revisit later ones closed until i get to them but for visibility I will keep this one around. Thats the best tradeoff you get instead of a LOG :P

@m4xw m4xw reopened this Sep 4, 2023
@m4xw
Copy link
Collaborator

m4xw commented Sep 4, 2023

Actually i think i remembered, for the 1 savestate per frame case I forced the threaded renderer to only run-ahead 1 frame (this means just render one) and i changed it to yield to RetroArch once per frame, so that went in sync and I think worked just fine in my case. But there were some uncertainties that I cant recall, one of the reasons that didnt go in

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
architectural bug help wanted Extra attention is needed mitigate question Further information is requested revisit later
Projects
None yet
Development

No branches or pull requests

2 participants