Skip to content

Fix badly architectured audio file reads #1747

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fix badly architectured audio file reads #1747

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/engine/audio/ALObjects.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,7 +135,7 @@ namespace AL {
ALuint format = Format(audioData.byteDepth, audioData.numberOfChannels);

CHECK_AL_ERROR();
alBufferData(alHandle, format, audioData.rawSamples.get(), audioData.size,
alBufferData(alHandle, format, audioData.rawSamples.data(), audioData.size,
audioData.sampleRate);

return ClearALError();
Expand Down
6 changes: 4 additions & 2 deletions src/engine/audio/Audio.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -383,8 +383,10 @@ namespace Audio {

streams[streamNum]->SetGain(volume);

AudioData audioData(rate, width, channels, (width * numSamples * channels),
reinterpret_cast<const char*>(data));
AudioData audioData { rate, width, channels, width * numSamples * channels };
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like this leaks data. Though the existing code is an unused and broken mess that frees data using the wrong type

audioData.rawSamples.reserve( audioData.size );
memcpy( audioData.rawSamples.data(), data, audioData.size * sizeof( char ) );

AL::Buffer buffer;

int feedError = buffer.Feed(audioData);
Expand Down
11 changes: 5 additions & 6 deletions src/engine/audio/AudioData.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
#ifndef AUDIO_DATA_H
#define AUDIO_DATA_H
#include <memory>
#include <vector>

namespace Audio {

Expand All @@ -40,15 +41,13 @@ struct AudioData {
, byteDepth{0}
, numberOfChannels{0}
, size{0}
, rawSamples{nullptr}
{}

AudioData(int sampleRate, int byteDepth, int numberOfChannels, int size, const char* rawSamples)
AudioData(int sampleRate, int byteDepth, int numberOfChannels, int newSize = 0 )
: sampleRate{sampleRate}
, byteDepth{byteDepth}
, numberOfChannels{numberOfChannels}
, size{size}
, rawSamples{rawSamples}
, size( newSize )
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We shouldn't need size now that the vector knows its own size?

{}

AudioData(AudioData&& that)
Expand All @@ -64,8 +63,8 @@ struct AudioData {
const int sampleRate;
const int byteDepth;
const int numberOfChannels;
const int size;
std::unique_ptr<const char[]> rawSamples;
int size;
std::vector<char> rawSamples;
};
} // namespace Audio
#endif
37 changes: 30 additions & 7 deletions src/engine/audio/OggCodec.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -151,20 +151,43 @@ AudioData LoadOggCodec(std::string filename)
int sampleRate = oggInfo->rate;
int numberOfChannels = oggInfo->channels;

char buffer[4096];
/* The largest ogg file we have so far is res-soundtrack/sound/ui/heartbeat.ogg
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The 2 comments I made on OpusCodec apply here as well. Also when I logged it, heartbeat.ogg was 49M samples / 98MB.

It uncompress to ~30mb. Use 64mb here just in case */
static constexpr uint32_t MAX_USED_FILE_SIZE = 64 * 1024 * 1024;
static constexpr uint32_t MAX_READ_SIZE = 1 * 1024 * 1024;

static char buffer[MAX_USED_FILE_SIZE];
uint32_t bufferPointer = 0;
uint32_t rawSamplesPointer = 0;

int bytesRead = 0;
int bitStream = 0;

std::vector<char> samples;
AudioData out { sampleRate, sampleWidth, numberOfChannels };

while ((bytesRead = ov_read(vorbisFile.get(), buffer + bufferPointer, MAX_READ_SIZE, 0, sampleWidth, 1, &bitStream)) > 0) {
bufferPointer += bytesRead;

if ( MAX_USED_FILE_SIZE - bufferPointer <= MAX_READ_SIZE ) {
out.rawSamples.resize( out.rawSamples.capacity() + bufferPointer );
std::copy_n( buffer, bufferPointer, out.rawSamples.data() );

while ((bytesRead = ov_read(vorbisFile.get(), buffer, 4096, 0, sampleWidth, 1, &bitStream)) > 0) {
std::copy_n(buffer, bytesRead, std::back_inserter(samples));
rawSamplesPointer += bufferPointer;
bufferPointer = 0;
}
}

if ( bufferPointer ) {
out.rawSamples.resize( out.rawSamples.capacity() + bufferPointer );
std::copy_n( buffer, bufferPointer, out.rawSamples.data() );
rawSamplesPointer += bufferPointer;
}

out.size = rawSamplesPointer;

ov_clear(vorbisFile.get());

char* rawSamples = new char[samples.size()];
std::copy_n(samples.data(), samples.size(), rawSamples);
return AudioData(sampleRate, sampleWidth, numberOfChannels, samples.size(), rawSamples);
return out;
}

} //namespace Audio
40 changes: 29 additions & 11 deletions src/engine/audio/OpusCodec.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,25 +145,43 @@ AudioData LoadOpusCodec(std::string filename)
int sampleRate = 48000;
int numberOfChannels = opusInfo->channel_count;

// The buffer is big enough to hold 120ms worth of samples per channel
opus_int16* buffer = new opus_int16[numberOfChannels * 5760];
/* The largest opus file we have so far is sound/thunder/weather.opus
It uncompress to ~7mb. Use 64mb here just in case */
static constexpr uint32_t MAX_USED_FILE_SIZE = 64 * 1024 * 1024;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

64M of int16 is 128MB

static constexpr uint32_t MAX_READ_SIZE = 1 * 1024 * 1024;

static opus_int16 buffer[MAX_USED_FILE_SIZE];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having the extra 64M of memory be allocated permanently seems suboptimal. Why not reserve 64M in the vector and call shrink_to_fit at the end? The code would also be simpler that way.

Copy link
Contributor Author

@VReaperV VReaperV Aug 23, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How would that ever work? ov_read() and op_read() expect a pointer, you can't use reserve() there.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reserve MAX_USED_FILE_SIZE once at the beginning, then resize adding an additional MAX_READ_SIZE before each read.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is straight up worse than the static buffer since it generates an extra ~300mb because each sound's samples have to be >= MAX_READ_SIZE. Reducing MAX_READ_SIZE would defeat the point.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Additionally, with reserve() + resize(), the vector's size() could no longer be used in place of AudioData::size since the sizes would no longer match.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How would that ever work? ov_read() and op_read() expect a pointer, you can't use reserve() there.

Yeah you'd have to resize it up by the read size first.

That is straight up worse than the static buffer since it generates an extra ~300mb because each sound's samples have to be >= MAX_READ_SIZE. Reducing MAX_READ_SIZE would defeat the point.

Hence shrink_to_fit.

But anyway, I thought of another solution that allows using the current version of the code mostly unchanged: use Hunk_AllocateTempMemory. This will allow the memory used by the large music file to be recycled, assuming that enough large allocations are made afterward (which should be the case when loading a map).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hence shrink_to_fit.
Only if MAX_READ_SIZE is reduced.

But anyway, I thought of another solution that allows using the current version of the code mostly unchanged: use Hunk_AllocateTempMemory. This will allow the memory used by the large music file to be recycled, assuming that enough large allocations are made afterward (which should be the case when loading a map).
True, that'd probably be the best solution.

uint32_t bufferPointer = 0;
uint32_t rawSamplesPointer = 0;

int samplesPerChannelRead = 0;

std::vector<opus_int16> samples;
AudioData out { sampleRate, sampleWidth, numberOfChannels };

while ((samplesPerChannelRead =
op_read(opusFile, buffer, sampleWidth * numberOfChannels * 5760, nullptr)) > 0) {
std::copy_n(buffer, samplesPerChannelRead * numberOfChannels, std::back_inserter(samples));
op_read(opusFile, buffer + bufferPointer, sampleWidth * numberOfChannels * 5760, nullptr)) > 0) {
bufferPointer += samplesPerChannelRead * numberOfChannels;

if ( MAX_USED_FILE_SIZE - bufferPointer <= MAX_READ_SIZE ) {
out.rawSamples.resize( out.rawSamples.capacity() + bufferPointer );
std::copy_n( buffer, bufferPointer, out.rawSamples.data() );

rawSamplesPointer += bufferPointer;
bufferPointer = 0;
}
}

delete[] buffer;
op_free(opusFile);
if ( bufferPointer ) {
out.rawSamples.resize( out.rawSamples.capacity() + bufferPointer );
std::copy_n( buffer, bufferPointer, out.rawSamples.data() );
rawSamplesPointer += bufferPointer;
}

char* rawSamples = new char[sampleWidth * samples.size()];
std::copy_n(reinterpret_cast<char*>(samples.data()), sampleWidth * samples.size(), rawSamples);
out.size = rawSamplesPointer;

op_free(opusFile);

return AudioData(sampleRate, sampleWidth, numberOfChannels, samples.size() * sampleWidth,
rawSamples);
return out;
}

} //namespace Audio
8 changes: 4 additions & 4 deletions src/engine/audio/WavCodec.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,11 +113,11 @@ AudioData LoadWavCodec(std::string filename)
return AudioData();
}

char* data = new char[size];
AudioData out { sampleRate, byteDepth, numChannels, size };
out.rawSamples.reserve( size );
std::copy_n(audioFile.data() + dataOffset + 8, size, out.rawSamples.data());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rawSamples still has size 0 at this point so you can't write to it. You can use assign here


std::copy_n(audioFile.data() + dataOffset + 8, size, data);

return AudioData(sampleRate, byteDepth, numChannels, size, data);
return out;
}

} // namespace Audio