Skip to content

fix audiostream cutouts from openal buffer underrun#7456

Open
Goober5000 wants to merge 2 commits into
scp-fs2open:masterfrom
Goober5000:fix/openal_underrun
Open

fix audiostream cutouts from openal buffer underrun#7456
Goober5000 wants to merge 2 commits into
scp-fs2open:masterfrom
Goober5000:fix/openal_underrun

Conversation

@Goober5000
Copy link
Copy Markdown
Contributor

When a streaming OpenAL source plays through all queued buffers it transitions to AL_STOPPED. Per the spec, queueing more buffers after that does not auto-resume playback -- alSourcePlay must be called again. This is the likely cause of music and briefing voices cutting out.

To fix this, check for underrun and call alSourcePlay if needed. Also double MAX_STREAM_BUFFERS from 4 to 8, bumping the margin to ~2s of queued audio.

Tested with menu and mission audio; also tested with a forced underrun to prove that underrun recovery works successfully.

When a streaming OpenAL source plays through all queued buffers it
transitions to AL_STOPPED. Per the spec, queueing more buffers after
that does not auto-resume playback -- alSourcePlay must be called
again. This is the likely cause of music and briefing voices cutting
out.

To fix this, check for underrun and call alSourcePlay if needed. Also
double MAX_STREAM_BUFFERS from 4 to 8, bumping the margin to ~2s
of queued audio.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@Goober5000 Goober5000 added fix A fix for bugs, not-a-bugs, and/or regressions. sound A feature or issue specific to music and sound labels May 14, 2026
@Goober5000 Goober5000 force-pushed the fix/openal_underrun branch from 03f6b79 to 1245546 Compare May 14, 2026 05:51
@notimaginative
Copy link
Copy Markdown
Contributor

notimaginative commented May 14, 2026

MAX_STREAM_BUFFERS should not be changed. Each buffer is intended to contain 1 second of audio, so you're bumping it from 4 seconds to 8 seconds, not 2 seconds. It is dependent on the audio format of course, with the current setup being for 1 second of 44.1kHz stereo per buffer (as defined by BIGBUF_SIZE). Higher quality formats would be less than a second per buffer, but hopefully 1 second total buffered. It isn't normally an issue so long as the code can buffer > 250ms of audio (the timer interval to feed more data).

So increasing BIGBUF_SIZE would be the proper way to address underruns, in addition to your other changes to restart playback if it does underrun. Simply doubling BIGBUF_SIZE would likely solve this issue all on it's own. And adding a debug warning when m_cbBufSize is less than a quarter BIGBUF_SIZE before it gets capped might also help identify files likely to underrun.

I should point out that the SDL3 audio changes (maybe for 26.0) should eliminate this problem as well as it doesn't have the same shortcoming. But that doesn't change the need for this PR in the meantime.

@notimaginative
Copy link
Copy Markdown
Contributor

And adding a debug warning when m_cbBufSize is less than a quarter BIGBUF_SIZE before it gets capped might also help identify files likely to underrun.

Sorry, that made no sense.

Basically you want if ((m_cbBufSize / MAX_STREAM_BUFFERS) > BIGBUF_SIZE) { mprintf(...) } just before m_cbBufSize gets capped to BIGBUF_SIZE to warn if there would be less than one second of audio buffered in total.

@Goober5000
Copy link
Copy Markdown
Contributor Author

Goober5000 commented May 14, 2026

Claude is pushing back:

The formula, traced:

audiostr.cpp:287: m_cbBufSize = (sample_rate * bytes_per_sample * num_channels) >> 2;

For 44.1kHz stereo 16-bit (the typical FS2 music format):

  • 44100 × 2 × 2 = 176400 bytes/sec — which equals BIGBUF_SIZE
  • >> 2 divides by 4 → 44100 bytes per buffer = 0.25 seconds
  • The BIGBUF_SIZE cap on the next line never triggers (44100 < 176400)

So in master each buffer holds 0.25s, not 1s. With MAX_STREAM_BUFFERS = 4 that's ~1s of total queued audio, not 4s. Bumping to 8 takes it to ~2s, not 8s. The reviewer appears to have read BIGBUF_SIZE (which is named/sized as "1 second at the canonical format") as the per-buffer size, but the >> 2 happens first and the cap only kicks in at very high sample rates (96kHz+).

So the >> 2 makes a difference here. With it, Claude says the warning would only kick in above 384kHz.

@notimaginative
Copy link
Copy Markdown
Contributor

Ahh, I looked for it to divide by MAX_STREAM_BUFFERS before I said anything and assumed it used the full size when I didn't find it. Honestly that seems like something I would do and not comment on it. :(

But Claude is correct and if it gets capped at all then there's a chance for it to underrun. I'd still recommend bumping BIGBUF_SIZE over adding MAX_STREAM_BUFFERS though. Having more stream buffers of a smaller size tends to result in audio issues (crackling, static, etc.) when it transitions between buffers due to alignment and what not.

@Goober5000
Copy link
Copy Markdown
Contributor Author

Oh, is the >> 2 supposed to be a division by MAX_STREAM_BUFFERS? That would affect the interpretation.

Claude would like to politely disagree with the max buffers vs size claim:

On BIGBUF_SIZE: for the actual bug case (44.1kHz stereo 16-bit music), BIGBUF_SIZE isn't doing anything today — the natural per-buffer size from the formula (44100 bytes) is well below the cap (176400). So bumping the cap from 176400 to 352800 has zero effect for the common case. The cap only engages for ~96kHz+ audio.

On crackling from "more buffers of smaller size": this is a mental-model slip. Bumping MAX_STREAM_BUFFERS doesn't change per-buffer size at all — each buffer is still computed from the audio format independently. We'd have more buffers of the same size in the queue, not smaller ones. Transition rate (transitions per second of playback) stays identical: same 0.25s per buffer, so still ~4 transitions/sec. The crackling concern would apply to a redesign that subdivided a fixed audio budget into more, smaller chunks — but that's not what MAX_STREAM_BUFFERS does. The alignment guards at audiostr.cpp:289-290 already ensure each buffer ends on a sample-frame boundary regardless of count.

But this is separate from the actual fix, so I can take that out of the PR if you want.

@notimaginative
Copy link
Copy Markdown
Contributor

Hmm, my brain is mush I think. Far too many versions of that code in too many code bases for me to keep it straight any longer. It all makes sense that it's right, and all makes sense that it's wrong. 🤷

So I think this is fine based on Claude + the few brain cells I can muster to look at audiostr.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

fix A fix for bugs, not-a-bugs, and/or regressions. sound A feature or issue specific to music and sound

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants