jarikomppa / soloud

Free, easy, portable audio engine for games
http://soloud-audio.com
Other
1.79k stars 285 forks source link

Bug: CurrentChannelVolume[] effectively applying a short fade-in on play (with fix) #136

Closed ocornut closed 8 years ago

ocornut commented 8 years ago
// Current channel volumes, used to ramp the volume changes to avoid clicks
float mCurrentChannelVolume[MAX_CHANNELS];

In Soloud::mixBus():

float pan[MAX_CHANNELS]; // current speaker volume
float pand[MAX_CHANNELS]; // destination speaker volume
float pani[MAX_CHANNELS]; // speaker volume increment per sample
for (k = 0; k < aChannels; k++)
{
    pan[k] = voice->mCurrentChannelVolume[k];
    pand[k] = voice->mChannelVolume[k] * voice->mOverallVolume;
    pani[k] = (pand[k] - pan[k]) / aSamples;
}

The code is effectively applying fading of arbitrary length (over aSamples samples). I wonder what the incentive is for that? Does it just makes things "feels" like abrupt when doing an arbitrary volume change? I am sort of fine with that even thought it is arbitrary. But note that rampling over aSamples samples makes the output not totally deterministic across backends and settings, under I misunderstand the code and aSamples is effectively always fixed.

Anyway, the actual issue that affects me is that on play(), currentChannelVolume[] are all set to zero and effectively this create a undesirable short fade-in over a few samples. I've got maniac sound designer here and he caught it immediately :)

It feels like after calling setVoiceVolume() in play() we should add:

memcpy(&mVoice[ch]->mCurrentChannelVolume, &mVoice[ch]->mChannelVolume, sizeof(float) * MAX_CHANNELS); // copy start volumes

What do you say?

Thanks again for making this :musical_note:

ocornut commented 8 years ago

Attached is a short WAV file, it has a very short amount of audio at the beginning (a click). With the master version of Soloud it can never be heard (unless looped I assume). With the memcpy line added it is heard correctly. TestVeryShortSound.zip

ocornut commented 8 years ago

Sorry, the right fix seems to be:

for (i = 0; i < MAX_CHANNELS; i++)
{
    mVoice[ch]->mCurrentChannelVolume[i] = mVoice[ch]->mChannelVolume[i] * mVoice[ch]->mOverallVolume;      
}

Instead of the memcpy().

ocornut commented 8 years ago

There's a commit in my branch. I tried to split all my commits into different branches to do different PR ("the right way") but my git-fu wasn't enough and I screwed multiple times.

If you checkout my branch locally you should be able to git cherry-pick ad8fe1e708f0769ec9a92558b273de888f117b0a and have this merged if you agree/want.