Fix crash when increasing engine speed with VRC7 enabled #62
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
CSoundGen and CAPU actually have independent engine/tick timing systems. CSoundGen::m_iUpdateCycles is the sequencer/driver period (in clocks), and CSoundGen::m_iConsumedCycles tracks the current progress. CAPU::m_iFrameCycleCount is the emulation/output period (in clocks), and CAPU::m_iFrameClock and CAPU::m_iFrameCycles track the current progress. Honestly I don't like CAPU's design, since it has too many unnecessary variables, with m_iFrameClock counting down and m_iFrameCycles counting up. Currently, when you increase engine speed (from 16 Hz to 60 Hz), then CAPU::ChangeMachineRate() calls CVRC7::SetSampleSpeed(), which replaces CVRC7::m_pBuffer with a new buffer of length 2/60 s (though honestly I'd prefer for it to be a fixed-size very long buffer). However, the next chunk of audio coming from CAPU still uses the old engine speed (slower, longer duration), because CAPU::m_iFrameClock (used to determine tick durations) isn't reinitialized until a tick later. As a result, CVRC7::EndFrame() tries to write 1/16 s of data (WantSamples) into 1/30 s of memory (CVRC7::m_pBuffer with length CVRC7::m_iMaxSamples), corrupting subsequent heap memory and setting the tracker up for an eventual crash. This commit causes CSoundGen::LoadMachineSettings() to call CAPU::Reset() to force reinitializing CAPU::m_iFrameClock. Honestly the better approach is to either couple CAPU's processing to the audio callback rather than audio ticks (replacing DirectSound with PortAudio/RtAudio), or to copy 0CC-FT and drive CAPU's audio generation chunks to CSoundGen's sequencer/driver ticks. Either way, I'd want to remove some of CAPU's frame-cycle-counting variables.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Bug
If you create/load a module with VRC7 enabled, then increase engine speed by 2x or more, then FT will corrupt heap memory and eventually crash.
Context
CSoundGen and CAPU actually have independent engine/tick timing systems. CSoundGen::m_iUpdateCycles is the sequencer/driver period (in clocks), and CSoundGen::m_iConsumedCycles tracks the current progress. CAPU::m_iFrameCycleCount is the emulation/output period (in clocks), and CAPU::m_iFrameClock and CAPU::m_iFrameCycles track the current progress. Honestly I don't like CAPU's design, since it has too many unnecessary variables, with m_iFrameClock counting down and m_iFrameCycles counting up.
Explanation
Switching to a default or custom engine speed calls CSoundGen::LoadMachineSettings(). This acquires a mutex to ensure that all work happens between "CAPU engine outputs" (which usually line up with CSoundGen ticks, but I'm not sure that's the case 100% of the time). Then it calls CAPU::ChangeMachineRate().
Currently, when you increase engine speed (from 16 Hz to 60 Hz), then CAPU::ChangeMachineRate() calls CVRC7::SetSampleSpeed(), which replaces CVRC7::m_pBuffer with a new buffer of length 2/60 s (though honestly I'd prefer for it to be a fixed-size very long buffer).
However, when CAPU resumes generating audio, the next chunk of audio still uses the old engine speed (slower, longer duration), which is undesirable behavior in of itself. This happens because CAPU::m_iFrameClock (used to determine tick durations) isn't reinitialized immediately, but only on the next tick. The crash occurs because CVRC7::EndFrame() is asked to generate 1/16 s of data (WantSamples), but writes it into 1/30 s of memory (CVRC7::m_pBuffer pointing to heap memory with length CVRC7::m_iMaxSamples). This corrupts subsequent heap structures and memory, and sets the tracker up for an eventual crash.
Fix
This commit ensures that changing engine speed reloads CAPU's engine speed timer/period immediately. It makes CSoundGen::LoadMachineSettings() call CAPU::Reset() to reloads CAPU's engine speed timer (CAPU::m_iFrameClock) from the current period (CAPU::m_iFrameCycleCount).
This works, but doesn't fix CAPU's bad design. Honestly I'd prefer to eventually either couple CAPU's processing to the audio callback rather than audio ticks (replacing DirectSound with PortAudio/RtAudio), or to copy 0CC-FT and drive CAPU's audio generation chunks to CSoundGen's sequencer/driver ticks. Either way, I'd want to remove some of CAPU's frame-cycle-counting variables.