Skip to content

audio: fix OpenAL buffer leak (~26 MB/min) — alDeleteBuffers never succeeded - #24

Open
marcoThede wants to merge 3 commits into
ammaarreshi:mainfrom
marcoThede:fix/openal-audio-cache-size-accounting
Open

audio: fix OpenAL buffer leak (~26 MB/min) — alDeleteBuffers never succeeded#24
marcoThede wants to merge 3 commits into
ammaarreshi:mainfrom
marcoThede:fix/openal-audio-cache-size-accounting

Conversation

@marcoThede

Copy link
Copy Markdown

The problem

Long sessions kept growing until the machine started swapping. Measured on
macOS/arm64: +25.7 MB/min, perfectly linear over 97 samples, no plateau.
After roughly two hours the process sat at ~5 GB and the system was thrashing
(2.2% CPU, 5.1M pageouts on an 8 GB machine).

97% of the heap growth came from a single allocator:

GameEngine::update()
└─ OpenALAudioManager::update()
   └─ processPlayingList()
      └─ notifyOfAudioCompletion()
         └─ startNextLoop()
            └─ playSample3D()
               └─ OpenALAudioFileCache::getBufferForFile()
                  └─ decodeFFmpeg()
                     └─ alBufferData → alBufferStorageDirectSOFT

Root cause

releaseOpenAudioFile() only detached the buffer from playing sources when
m_openCount > 0. But the cache reference is dropped (closeBuffer) before
the still-playing source is handed a new buffer — so a buffer with
m_openCount == 0 can very much still be bound to a live OpenAL source.
alDeleteBuffers() then fails with AL_INVALID_OPERATION, and nobody checks
the return value
.

Instrumenting alGetError() right after the call made it unambiguous:

before:  0 of 1158 alDeleteBuffers calls succeeded
after:   880 of 889 succeeded

Not a single audio buffer was ever freed for the lifetime of the process.

Result

before after
successful buffer deletions 0 / 1158 880 / 889
memory growth 25.7 MB/min 1.45 MB/min
per hour ~1.6 GB ~87 MB

Measured over 11 minutes, then re-confirmed on the committed tree. The
remainder is ordinary cache warm-up — the footprint now also drops again as
the cache evicts, which it never did before.

The commits

  1. account for decoded PCM sizedecodeFFmpeg() accumulates the decoded
    size into m_fileSize, but getBufferForFile() immediately overwrote it
    with file->size() (the compressed size), so the cache under-reported its
    own footprint. Explicitly not the cause of the leak — verified by
    applying it alone, which changed nothing (+1392 buffers/3 min vs +1451).
    Wrong accounting regardless.
  2. fix OpenAL buffer leak — the actual fix described above.
  3. skip cache lookup for audio events without a filenameTurretMoveLoop
    and RadarEvent ship with an empty Sounds = in the retail INIZH.big.
    TurretMoveLoop is loop all random, so it regenerated the empty filename and
    walked down to TheFileSystem->openFile("") every single frame. Nothing
    leaked, but up to 12813 futile calls in five minutes against 2948 real ones.

Commit 1 is the weakest of the three and can be dropped without affecting the
fix, if you prefer to keep this tight.

Reproducing

No gameplay required — the main menu is enough, since the menu music loops
and every loop re-decodes a sample. Watch GeneralsXZH in Activity Monitor, or:

footprint -p <pid>      # not `ps -o rss` — heap/leaks suspend the process and
                        # RSS then drops by hundreds of MB without any real free

Note that leaks reports nothing useful here (only ~18 KB of system XPC
cycles): the buffers stay reachable, so this is a cache that never drains
rather than a classic leak.

Scope

Core/GameEngineDevice/Source/OpenALAudioDevice/ is shared, so this affects
Linux as well as macOS. Windows uses Miles Audio and is unaffected.
Verified on macOS 15 / arm64 (MoltenVK + DXVK + SDL3); I have not been able to
test the Linux build.

marcoThede and others added 3 commits August 24, 2026 11:36
decodeFFmpeg() accumulates the decoded size into m_fileSize frame by frame,
but getBufferForFile() immediately overwrote it with file->size() - the
compressed on-disk size. The cache therefore under-reported its own footprint
and evicted much later than the 14 MB budget intended.

This corrects the accounting only. It is not the cause of the buffer leak
fixed in the following commit.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
releaseOpenAudioFile() only called closeAnySamplesUsingFile() when
m_openCount > 0. But the cache reference is dropped (closeBuffer) before the
playing source is handed a new buffer, so a buffer with m_openCount == 0 can
still be bound to a live OpenAL source. alDeleteBuffers() then fails with
AL_INVALID_OPERATION - a return value nobody checks - and the buffer is
leaked for the rest of the session.

Measured on macOS/arm64 (Zero Hour, main menu, no gameplay needed - the menu
music loops and every loop re-decodes a fresh sample):

  before:  0 of 1158 alDeleteBuffers calls succeeded, +25.7 MB/min
           (97% of all heap growth came from alBufferStorageDirectSOFT)
  after:   880 of 889 succeeded, +1.45 MB/min over 11 minutes

The leak accumulated ~1.6 GB per hour and drove the process into swap
thrashing after roughly two hours on an 8 GB machine.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Some stock audio events ship with an empty "Sounds =" list - TurretMoveLoop
and RadarEvent among them, straight from the retail INIZH.big. TurretMoveLoop
is additionally declared "loop all random", so startNextLoop() regenerates
the (empty) filename and calls getBufferForFile() again on the very next
frame, every time descending all the way to TheFileSystem->openFile("")
before giving up.

Nothing leaked - the failing openFile() path returns before alGenBuffers() -
but the hash lookup and the BIG-archive access were pure waste, several times
per second for as long as a turret was turning or the radar was pinging.
Measured up to 12813 such calls in five minutes, against 2948 real ones.

An early return keeps the behaviour identical (openFile("") would have
failed anyway) and skips the work. A DEBUG_ASSERTLOG for exactly this case
already existed further down, so the situation was known.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Copilot AI lite review requested due to automatic review settings August 24, 2026 11:06

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Changes recommended

alDeleteBuffers() failures are still not checked/logged, which risks silent regressions of the “buffers never freed” behavior if deletion fails again for any reason.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR addresses a long-session memory growth issue in the shared OpenAL audio cache (Core/GameEngineDevice/...) by ensuring OpenAL buffers can actually be deleted (preventing unbounded buffer accumulation), while also improving cache sizing accuracy and avoiding wasteful cache lookups for empty sound filenames.

Changes:

  • Always detach OpenAL buffers from any still-referencing sources before attempting deletion, avoiding AL_INVALID_OPERATION on alDeleteBuffers.
  • Fix cache size accounting by keeping decoded PCM size (as accumulated during FFmpeg decode) instead of overwriting with compressed file size.
  • Skip cache lookup/work when the resolved filename is empty (avoids repeated openFile("") calls for stock events with empty Sounds = lists).
File summaries
File Description
Core/GameEngineDevice/Source/OpenALAudioDevice/OpenALAudioCache.cpp Fixes OpenAL buffer lifetime handling, corrects cache footprint accounting, and avoids futile empty-filename lookups.
Review details
  • Files reviewed: 1/1 changed files
  • Comments generated: 1
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment on lines +238 to 247
// GeneralsX @bugfix Marco 24/08/2026 Always detach the buffer from any source
// still referencing it - not just when m_openCount > 0. The cache reference is
// dropped (closeBuffer) before the source gets a new buffer assigned, so a
// buffer with m_openCount == 0 can still be bound to a live OpenAL source.
// alDeleteBuffers then fails with AL_INVALID_OPERATION and the buffer leaks;
// measured: 1158 of 1158 deletions failed, ~26 MB/min lost.
if (fileToRelease->m_buffer) {
// This thing needs to be terminated IMMEDIATELY.
TheAudio->closeAnySamplesUsingFile((const void*)(uintptr_t)fileToRelease->m_buffer);
}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants