drhelius / Gearcoleco

ColecoVision emulator for macOS, Windows, Linux, BSD and RetroArch.
https://x.com/drhelius
GNU General Public License v3.0
62 stars 15 forks source link

[libretro] Audio is distorted when using Runahead single-instance mode #31

Open msheehan79 opened 12 months ago

msheehan79 commented 12 months ago

When the runahead feature is enabled in RetroArch for this core, the audio becomes distorted and requires you to enable the second instance mode to render audio properly.

Ideally, if the audio information can be serialized fully in the save state, this should allow runahead to work in single-instance mode without distortion.

I found during some testing my side that if I added m_pApu and m_pBuffer to the save state in Audio.cpp, and removed the reset + clear functions from the load state that audio was clear with runahead enabled. But I'm not sure if there is any negative impacts to this, apart from the obvious impact to compatibility of existing states.

msheehan79 commented 10 months ago

@drhelius I've been testing the audio save state adjustment I made on https://github.com/msheehan79/Gearcoleco/commits/test-serialization and this seems to fix audio distortion with runahead enabled on all the games I have tried. The only part I'm not 100% sure on is how big the serialized data really needs to be; I set it to 255 on both and that seems to work, but not sure if there is more accurate way to calculate the exact size needed.

If there is a desire to preserve the existing Save State format, one solution is the libretro API has a callback (RETRO_ENVIRONMENT_GET_AUDIO_VIDEO_ENABLE) that allows you to identify if the save state is a runahead-type save state (in which case the additional audio values need to be serialized) or a normal save state (in which case we could omit these audio flags and retain the original size and format to retain compatibility). We could pass a parameter down to the core to indicate which type of save state to create and return the correct type accordingly.

If you think the above approach is worth implementing, I could take a crack at it when time allows.

drhelius commented 10 months ago

Sorry for the late reply. Let me check your code and understand what is going on. Never tested run ahead before.

I like the idea of generating a different save state for the run ahead only, yes that's ok.

I'll get back to you.

drhelius commented 6 months ago

Sorry for the delay. Saving the state of the APU is a bit more complex that what you have in your code. I may be able to add it but is not that simple. Your method can trash the memory.

But it looks like is working fine if you enable the second instance in RA. Just for me to understand, the benefit of running it single-instanced is just performance right?

msheehan79 commented 6 months ago

Thanks for your inputs on this, and honestly not surprised my method did not work - it was definitely just a shot in the dark approach that seemed to work on the surface but I had my doubts.

In my experience its not just performance for runahead second-instance. I've run into weird issues before with the 2 cores not syncing up, where for example the core is in one state and the audio is out of sync. I don't remember specifics as it was several months ago but I know that the general view is that second instance mode is basically a workaround for the audio issues but it had its drawbacks, in addition to the additional overhead of having to run 2 instances of the core.

I've tried to use single-instance mode in as many cores as I can for that reason. But I totally understand if its a non-trivial implementation for a feature you don't use.

drhelius commented 6 months ago

Understood. Thanks for the clarification. I'll keep it as a feature to implement when time allows.