Skip to content
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.

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

sdl2-mixer sound effect playback with Mix_PlayChannel is truncated #304

Closed
zent1n0 opened this issue Feb 3, 2025 · 18 comments
Closed

sdl2-mixer sound effect playback with Mix_PlayChannel is truncated #304

zent1n0 opened this issue Feb 3, 2025 · 18 comments
Assignees
Milestone

Comments

@zent1n0
Copy link

zent1n0 commented Feb 3, 2025

sdl2-mixer sound effect playback with Mix_PlayChannel performs differently with sdl2-compact compared to an actual sdl2 library. In my case, sound become truncated.

I noticed this while playing SpaceCadetPinball, after Arch's upgrade to sdl3 and sdl2-compact for compatibility.

It seemed the game implements sound playback in SpaceCadetPinball/Sound.cpp:Sound::PlaySound: L60: auto channel = Mix_PlayChannel(-1, wavePtr, 0);, where the wavePtr is previously loaded from Mix_LoadWAV. With sdl2-compact the sound effect is truncated to less than a half of duration (the actual playback duration is not fixed for every chunk), while sdl2 performs well.

I tried to figure this issue out with a snippet from libsdl-org/SDL_mixer#646:

// minimal example

// #include <SDL3/SDL.h>
// #include <SDL3_mixer/SDL_mixer.h>
#include <SDL2/SDL.h>
#include <SDL2/SDL_mixer.h>
#include <stdio.h>

int main() {
  if (SDL_Init(SDL_INIT_AUDIO) < 0) {
    SDL_Log("Couldn't initialize SDL: %s\n", SDL_GetError());
    return 255;
  }

  if (Mix_Init(MIX_INIT_OGG) <= 0) {
    printf("fail to init : %s", SDL_GetError());
    return -1;
  }

  if (Mix_OpenAudio(MIX_DEFAULT_FREQUENCY, MIX_DEFAULT_FORMAT,
                    MIX_DEFAULT_CHANNELS, 1024) < 0) {
    SDL_Log("Couldn't open audio: %s\n", SDL_GetError());
    return -1;
  }

  Mix_AllocateChannels(100);

  Mix_Volume(-1, MIX_MAX_VOLUME);

  Mix_Chunk *chunk = Mix_LoadWAV("SOUND1.WAV");
  if (!chunk) {
    printf("failed to load chunk\n");
    return -1;
  }

  int channel = Mix_PlayChannel(-1, chunk, 0);
  Mix_SetPosition(channel, 0, 0);

  channel = Mix_PlayChannel(-1, chunk, 0);
  Mix_SetPosition(channel, 90, 0);

  SDL_Delay(2000);

  channel = Mix_PlayChannel(-1, chunk, 0);
  Mix_SetPosition(channel, 180, 0);

  SDL_Delay(2000);

  channel = Mix_PlayChannel(-1, chunk, 0);
  Mix_SetPosition(channel, 270, 0); // plays at the right speaker

  SDL_Delay(2000);


  return 0;
}

But the issue is gone with this situation, also another version with sdl3-mixer built from source just worked fine. No sound truncate felt except for last one (when main exited). As the loading process of chunk seems fine, I guess the issue comes from playback.

As migration into sdl3 is still in progress, I suppose we keep a similar behavior in sdl2-compact 😄

@bbaa-bbaa
Copy link

bbaa-bbaa commented Feb 3, 2025

For reference, I also have a similar issue when using Moonlight.

The output audio waveform looks like the following image.
Image

UPDATE: It seems that Moonlight does not use sdl-mixer. There may be a buffer underrun in moonlight.

@slouken
Copy link
Collaborator

slouken commented Feb 3, 2025

We fixed an audio problem that could cause this over the weekend. What version of sdl2-compat are you using?

@bbaa-bbaa
Copy link

I am using the latest commit from the main branch.

@icculus icculus self-assigned this Feb 3, 2025
@icculus icculus added this to the 2.30.54 milestone Feb 3, 2025
@bbaa-bbaa
Copy link

bbaa-bbaa commented Feb 3, 2025

https://github.com/moonlight-stream/moonlight-qt/blob/dd2a99a96ba871c0fd9ea68e3202276c2929c6e3/app/streaming/audio/renderers/sdlaud.cpp#L39

bool SdlAudioRenderer::prepareForPlayback(const OPUS_MULTISTREAM_CONFIGURATION* opusConfig)
{
    SDL_AudioSpec want, have;

    SDL_zero(want);
    want.freq = opusConfig->sampleRate;
    want.format = AUDIO_F32SYS;
    want.channels = opusConfig->channelCount;

    // On PulseAudio systems, setting a value too small can cause underruns for other
    // applications sharing this output device. We impose a floor of 480 samples (10 ms)
    // to mitigate this issue. Otherwise, we will buffer up to 3 frames of audio which
    // is 15 ms at regular 5 ms frames and 30 ms at 10 ms frames for slow connections.
    // The buffering helps avoid audio underruns due to network jitter.
#ifndef Q_OS_DARWIN
    want.samples = SDL_max(480, opusConfig->samplesPerFrame * 3);
#else
    // HACK: Changing the buffer size can lead to Bluetooth HFP
    // audio issues on macOS, so we're leaving this alone.
    // https://github.com/moonlight-stream/moonlight-qt/issues/1071
    want.samples = SDL_max(480, opusConfig->samplesPerFrame);
#endif

    m_FrameSize = opusConfig->samplesPerFrame *
                  opusConfig->channelCount *
                  getAudioBufferSampleSize();

    m_AudioDevice = SDL_OpenAudioDevice(NULL, 0, &want, &have, 0);
}

Moonlight's want.samples seems to be set to 480. In my test, it appears that want.samples values below 1000 always cause the audio to be truncated.

@zent1n0
Copy link
Author

zent1n0 commented Feb 3, 2025

My sdl2-compact is v2.30.52. I'll try with latest master later.

@svenstaro
Copy link

I can reproduce the moonlight problems and running both sdl2-compat and sdl3 straight from git doesn't seem to resolve the issues.

@svenstaro
Copy link

Somebody reported the same issue on the moonlight repo and I directed them into this issue.

@alama
Copy link

alama commented Feb 3, 2025

I think I have this issue with our game, SRB2

We have a few set of DOOM SFX lumps that just plain PCM8 WAV data that we feed into Mix_LoadWAV_RW().

so, our DOOM lump DSJUMP are not converted correctly and just play cut off.

@zent1n0
Copy link
Author

zent1n0 commented Feb 4, 2025

My sdl2-compact is v2.30.52. I'll try with latest master later.

Could tell with code from master the issue still exists.

My test case is LD_LIBRARY_PATH=/mypathto/Libs/lib/ bin/SpaceCadetPinball. With modification of LD_LIBRARY_PATH the sound cuts off, while system-wide library sdl2 works fine. Same issue occurs no matter which provider of library passed during compilation time, so at least we could tell the compact library is still ABI-compatible.

@bbaa-bbaa
Copy link

bbaa-bbaa commented Feb 4, 2025

The issue on Moonlight mentions that version 2.30.51 works fine.
git bisect log here.

# bad: [97acb6a6622cbb39b3d5b93747bc63bf17aaf1d7] change a few passthrough SDL_xxx() calls to SDL3_xxx().
# good: [0c526f15c65f7cde54f246ceca060e9219c3b906] Updated to version 2.30.51 for release
git bisect start 'HEAD' 'release-2.30.51'
# bad: [188cb119325aa9a99899f1ff9e2e0d99ae5abf87] minor coding style clean-up after PR/281.
git bisect bad 188cb119325aa9a99899f1ff9e2e0d99ae5abf87
# bad: [0ff6b3cd77defebfa7878179f063ba37d514f574] event: Drop PIXEL_SIZE_CHANGED events if the window is hidden.
git bisect bad 0ff6b3cd77defebfa7878179f063ba37d514f574
# bad: [f56d4f9559dd4fb84fefd5c84a1a8cfea4ba0e1e] fix build using c++ compilers.
git bisect bad f56d4f9559dd4fb84fefd5c84a1a8cfea4ba0e1e
# bad: [9110cc1f391aee8c5d6b6ca323a3337b26355bc8] audio: Call audio callbacks with consistent buffer size.
git bisect bad 9110cc1f391aee8c5d6b6ca323a3337b26355bc8
# good: [b2d2190946b4c15ba0ef425f8a54ba6f9056fd2e] Give SDL2 applications the audio spec they expect
git bisect good b2d2190946b4c15ba0ef425f8a54ba6f9056fd2e
# first bad commit: [9110cc1f391aee8c5d6b6ca323a3337b26355bc8] audio: Call audio callbacks with consistent buffer size.

Use the following reproduction case for testing.

#include <SDL2/SDL.h>

int main() {
  SDL_AudioSpec want, wavSpec;
  SDL_Event e;
  Uint32 wavLength;
  Uint8 *wavBuffer;

  SDL_zero(want);
  if (SDL_Init(SDL_INIT_AUDIO|SDL_INIT_EVENTS) < 0) {
    SDL_Log("Couldn't initialize SDL: %s\n", SDL_GetError());
    return 255;
  }

  SDL_LoadWAV("test.wav", &wavSpec, &wavBuffer, &wavLength);
  SDL_Log("Loaded WAV file\n");
  SDL_Log("Frequency: %d\n", wavSpec.freq);
  SDL_Log("Samples: %d\n", wavSpec.samples);
  SDL_Log("Channels: %d\n", wavSpec.channels);
  SDL_Log("Size: %d\n", wavLength);

  want.freq = wavSpec.freq;
  want.format = wavSpec.format;
  want.channels = wavSpec.channels;
  want.samples = 480; // moonlight value

  SDL_AudioDeviceID devid =
      SDL_OpenAudioDevice(NULL, 0, &want, NULL, 0);
  if (devid == 0) {
    SDL_LogError(SDL_LOG_CATEGORY_APPLICATION,"Failed to open audio device: %s", SDL_GetError());
    return 0;
  }
  SDL_PauseAudioDevice(devid, 0);
  SDL_QueueAudio(devid, wavBuffer, wavLength);
  
  while (1) {
    SDL_PollEvent(&e);
    if (e.type == SDL_QUIT || SDL_GetQueuedAudioSize(devid) == 0) {
      SDL_Log("exit\n");
      break;
    }
    SDL_Delay(100);
  }
  SDL_Quit();
  return 0;
}

@zent1n0
Copy link
Author

zent1n0 commented Feb 4, 2025

The issue on Moonlight mentions that version 2.30.51 works fine.

My issue still reproduces when loading with v2.30.50 and v2.30.51.

i have the same sound issue with the newest sld2-compat update (2.30.52). rolling back to sdl2-compat-2.30.51 resolve the sound issue

I'm still confused. Rolling back can't solve my issue. Nor building from scratch with master does.

@bbaa-bbaa
Copy link

May different components be affected. Moonlight does not use sdl2-mixer, so I might need to open another issue about Moonlight.

@zent1n0
Copy link
Author

zent1n0 commented Feb 4, 2025

@bbaa-bbaa tested again with your example code. With sdl2 the wav plays to the end, then exits. With sdl2-compact the binary simply exits and I could hear nothing.

May different components be affected. Moonlight does not use sdl2-mixer, so I might need to open another issue about Moonlight.

Yep. I think yours is not mixer issue, but it could be another issue with sdl2-compact, or even sdl3, as @svenstaro mentioned it could be reproduced with sdl3.

@icculus maybe there is an missing queue check for sound playback in sdl2-compact or sdl3?

@cooltyp100
Copy link

I think the problem is that SDL_BuildAudioCVT in sdl2-compat doesn't account for different bitsizes when the source and destination format are different. That results in the len_mult of SDL_AudioCVT being too low and SDL_ConvertAudio calling SDL_AudioStreamGet with a wrong (too small) len parameter.

(In SpaceCadetPinball's case the source format is AUDIO_U8 and the dest format is AUDIO_S16LSB)

This patch fixes the truncated audio in SpaceCadetPinball for me (code originates from SDL2's SDL_BuildAudioTypeCVTToFloat):

diff --git a/src/sdl2_compat.c b/src/sdl2_compat.c
index 91df91d..662f78d 100644
--- a/src/sdl2_compat.c
+++ b/src/sdl2_compat.c
@@ -9579,6 +9579,20 @@ SDL_BuildAudioCVT(SDL_AudioCVT *cvt,
             cvt->needed = 0;
         }
 
+        if (src_format != dst_format) {
+            const Uint16 src_bitsize = SDL_AUDIO_BITSIZE(src_format);
+            const Uint16 dst_bitsize = SDL_AUDIO_BITSIZE(dst_format);
+
+            if (src_bitsize < dst_bitsize) {
+                const int mult = (dst_bitsize / src_bitsize);
+                cvt->len_mult *= mult;
+                cvt->len_ratio *= mult;
+            } else if (src_bitsize > dst_bitsize) {
+                const int div = (src_bitsize / dst_bitsize);
+                cvt->len_ratio /= div;
+            }
+        }
+
         if (src_channels < dst_channels) {
             cvt->len_mult = ((cvt->len_mult * dst_channels) + (src_channels - 1)) / src_channels;
         }

@alama
Copy link

alama commented Feb 4, 2025

OK, I can confirm that this patch to SDL_BuildAudioCVT() fixes the playback of the 8-bit WAV data in SRB2 as well.

@zent1n0
Copy link
Author

zent1n0 commented Feb 4, 2025

The patch also fixed my issue. As there is already conversion in SDL_BuildAudioCVT(), I suppose we should fix it in sdl2-compact layer, or there will be a final fix in sdl3?

@icculus
Copy link
Collaborator

icculus commented Feb 4, 2025

Just pushed a separate fix for Moonlight; checking this diff now.

@icculus icculus closed this as completed in 599195c Feb 4, 2025
@icculus
Copy link
Collaborator

icculus commented Feb 4, 2025

Okay, this looks reasonable, so I've applied it. You should be good to go now!

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

No branches or pull requests

7 participants