Fix ambient sample mixing (#281)

[zear]

The sound engine used in Dethrace is unable to play the same sample more than once at a time. If two or more sound sources request to play the same sound id, the one to use it last will overwrite any previous effects on the sample. This is most prominent for car engine noises and cop sirens. This issue also occurs in Windows builds of OG, but not in DOS ones.

Fix this issue by duplicating sound samples. Note that the game does not clean up ambient sounds, so extra care is taken to reuse the already allocated descriptors.

Note: This patch depends on https://github.com/dethrace-labs/dethrace/pull/285.

[zear]

This is a proposed solution for https://github.com/dethrace-labs/dethrace/issues/281.

Note that we cannot transparently fix this at the audio backend, because libminiaudio (and probably most other audio engines) expects a separate data source for each sound that it reproduces: https://github.com/mackron/miniaudio/discussions/491#discussioncomment-3027287

Marked this as a "draft", as discussion is welcome.

[dethrace-labs]

Hmm, this is a tricky one! However we fix it requires moving some things around.

I was wondering if we should extend the existing channel object to create its own sound_buffer, from the passed in descriptor. Each channel already functions as an instance of a playing sound, so maybe that fits?

For example, at the top of S3PlaySample, if we cloned the ma_sound * from the descriptor and kept it as a field on the channel, then inside of S3SyncSampleRate we would manipulate the channel's copy of the buffer?

In that case, the sound_buffer on the descriptor might just be the raw audio bytes, or some sort of ma_data_source that we can use to load instances of ma_sound?

[zear]

I was wondering if we should extend the existing channel object to create its own sound_buffer, from the passed in descriptor. Each channel already functions as an instance of a playing sound, so maybe that fits?

Absolutely, that's the right approach. My patch tried to fix it at the lowest possible code impact, however if we're ok with larger changes, then we should do it proper. I will look into this over the weekend.

[zear]

@dethrace-labs Second draft with the approach we discussed.

With the current solution, the original sound samples are still being kept in the descriptors:

    descriptor->sound_buffer = S3LoadWavFile(filename, sample);

...but every time a sample is to be played, I clone the sound buffer with ma_sound_init_copy and store that in chan->sound_buffer. This simplifies the code and allows for backwards compatibility with the old behavior (just compile without DETHRACE_FIX_BUGS), at the cost of the buffer in the descriptor never being used.

The matter complicates a bit given that every time a new sound is played through any of the following functions: S3StartSound, S3StartSound2, S3ServiceSoundSource, S3StartSound3D ...they grab the first available channel (S3AllocateChannel) and assign a new descriptor to it. In consequence, channels almost never reference the same descriptor twice in a row, resulting in a constant ma_sound_uninit/ma_sound_init_copy cycle, which probably takes its toll on CPU usage (I haven't measured). But at least I was able to eliminate heap fragmentation by re-using chan->sound_buffer memory for each iteration of ma_sound_init_copy.

On an unrelated note, I think we might be leaking in S3DisposeDescriptor, as a lot of sound samples are being unloaded once the race is over (just not ambient sounds):

@@ -140,6 +140,9 @@ int S3DisposeDescriptor(tS3_sound_id id) {
         }
         S3MemFree(desc->sound_data);
         desc->sound_data = NULL;
+        ma_sound_uninit(desc->sound_buffer);
+        free(desc->sound_buffer);
+        desc->sound_buffer = NULL;
     }

[dethrace-labs]

I had a quick playtest on the latest version of this branch, and it does still seem to exhibit the same behavior - my car audio appears to get cut off.

[zear]

@dethrace-labs You'll need to apply https://github.com/dethrace-labs/dethrace/pull/285 first, so that DETHRACE_FIX_BUGS is visible in src/S3 subdirectory. Right now you're testing the "backwards compatible" path :)

[zear]

@dethrace-labs Rebased on top of main. With https://github.com/dethrace-labs/dethrace/pull/285 merged, DETHRACE_FIX_BUGS should apply to files in src/S3. Please re-test.

[MastaG]

Is this close to being merged?

[dethrace-labs]

@zear I had another playtest, behavior looks (sounds) good to me now :) I didn't experience any instances of my engine sound dropping out. Is there anything left to do in your mind? If not, we could mark this as ready to review and get it merged.

A random idea I had for the future could be to keep a cache of unused sound instances around in a list, and instead of copying a new instance every time, we see if we have an unused one in the cache. We only create a new instance if we don't have one in cache. Within a short amount of time we should get to a steady state where we have enough copies of sound instances to cover our concurrent needs.

[dethrace-labs]

Actually, I think I just got lucky with the car selection that time. I played again and had my engine sound cut out. I'll do some debugging locally 👍

[dethrace-labs]

Closing this in favor of #329 where we pulled the correct logic from the DOS build