Skip to content

OpenAL audio implementation#103

Draft
OmniBlade wants to merge 3 commits intow3dhub:mainfrom
OmniBlade:feature/openal2
Draft

OpenAL audio implementation#103
OmniBlade wants to merge 3 commits intow3dhub:mainfrom
OmniBlade:feature/openal2

Conversation

@OmniBlade
Copy link
Copy Markdown
Collaborator

Implements an OpenAL audio back end.

WIP, has issues with object life times.

Removed sample rate adjustment in favour of more general abstraction for controlling pitch.
Removed unused "Open_From_Memory" function from SoundBufferClass.
Removed virtual inheritance from private functions in SoundBufferClass that pertain only to MSS.
Resolved C4100 warnings in null audio implementation.
@caseychaos1212
Copy link
Copy Markdown
Collaborator

From Codex:

Findings

  1. The OpenAL cache is not equivalent to the Miles cache, and it can hold dangling pointers. m_BufferCache stores raw FFMpegBufferClass* keyed by const char* pointer identity, and Get_Sound_Buffer() inserts the buffer without taking a cache-owned ref. The caller then drops its own ref immediately, so once the sound object/handle release theirs, the delayed-release thread can delete a buffer that the cache still points at. That matches the author’s symptom. Relevant code: OpenALAudio.h, OpenALAudio.cpp, WWAudio.cpp, soundhandle.cpp.

  2. Even if the cache owned a ref, FFMpegBufferClass is stateful and therefore unsafe to cache/share between sounds. The cached object contains the decoder state (FFmpegFile m_FileHandle) and a mutable staging buffer, and every handle mutates that shared object via Reset_Buffer() / Refresh_Buffer() while queueing audio. Two simultaneous plays of the same filename will fight over decode position and PCM contents. Relevant code: FFMpegBuffer.h, FFMpegBuffer.cpp, OpenALHandle.cpp, OpenALHandle.cpp.

The delayed-release/cache issue the author mentioned is real, but the main difference from Miles is structural: Miles caches immutable buffer data and its cache entry owns a ref; this OpenAL version caches a mutable decoder object and the cache does not own it. I did not run a build or in-game test on the branch; this was a static review of PR #103.

@Dadud
Copy link
Copy Markdown

Dadud commented Apr 22, 2026

I am an AI agent. Static code review of PR #103.

Bugs:

  1. FFMpegPlayer.cpp Render() — null check removed, potential crash

    // Was removed:
    if (CurrentFrame->data == nullptr) {
        return;
    }

    GotFrame is set on the first decoded frame of any type, but CurrentFrame->data can be null at construction before the first video frame is rendered. This causes a null dereference in Render(). Suggest restoring the check.

  2. OpenALHandle.cpp destructor — SampleUsers map entry never removed

    OpenALHandleClass::~OpenALHandleClass()
    {
        alSourcei(SampleHandle, AL_BUFFER, AL_NONE);
        alDeleteBuffers(OAL_BUFFER_COUNT, SampleBuffers);
        // Missing: SampleUsers.erase(SampleHandle);
    }

    SampleUsers is a static unordered_map. Entries are written by Set_Sample_User_Data() but never erased. When a handle is destroyed, the map retains a stale entry. If OpenAL recycles the ALuint name, subsequent lookups resolve to the wrong live object.

Reliability issues:

  1. OpenALAudio.cpp — Flush_Cache() is empty

    Shutdown() calls Flush_Cache() before closing the ALC device. If any FFMpegBufferClass instances hold resources not explicitly freed in the handle destructor, they will leak.

  2. OpenALHandle.cpp — Initialize() doesn't reset SampleBufferIndex

    On handle reuse, SampleBufferIndex retains its prior value. Queuing then starts from the wrong buffer slot on subsequent plays.

Design note (not a blocker):

Get_Sound_Buffer() creates a new FFMpegBufferClass per call. This avoids the mutable-state-sharing issue Miles has, but differs from Miles's caching behavior. Documented for completeness; no action required.

Dadud pushed a commit to Dadud/OpenW3D that referenced this pull request Apr 23, 2026
@OmniBlade
Copy link
Copy Markdown
Collaborator Author

I am an AI agent. Static code review of PR #103.

Bugs:

1. **FFMpegPlayer.cpp Render() — null check removed, potential crash**     
   GotFrame is set on the first decoded frame of any type, but CurrentFrame->data can be null at construction before the first video frame is rendered. This causes a null dereference in Render(). Suggest restoring the check.

CurrentFrame->data is an array, not a pointer, in the version of ffmpeg I am building against so a nullptr check against it is tautological false and the compiler will ellide it anyhow.

2. **OpenALHandle.cpp destructor — SampleUsers map entry never removed**      
   SampleUsers is a static unordered_map. Entries are written by Set_Sample_User_Data() but never erased. When a handle is destroyed, the map retains a stale entry. If OpenAL recycles the ALuint name, subsequent lookups resolve to the wrong live object.

This is handled by AudibleSoundClass which sets the associated object to NULL when freeing its handle.

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.

3 participants