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

The SDL3 audio subsystem redesign! #7704

Merged
merged 138 commits into from
Aug 4, 2023
Merged

Conversation

icculus
Copy link
Collaborator

@icculus icculus commented May 13, 2023

This is a work in progress! (and this commit will probably get force-pushed over at some point).

This rips up the entire SDL audio subsystem! While we still feed the audio device from a separate thread, the audio callback into the app is now gone a totally optional alternative.

Now the app will bind an SDL_AudioStream to a given device and feed data to it. As many streams as one likes can be bound to a device; SDL will mix them all into a single buffer and feed the device from there.

So not only does this function as a basic mixer, it also means that multiple device opens are handled seamlessly (so if you want to open the device for your game, but you also link to a library that provides VoIP and it wants to open the device separately, you don't have to worry about stepping on each other, or that the OS will fail to allow multiple opens of the same device, etc).

Here is a simple program that just opens a device, binds two streams to it, and plays them both at the same time, ending when the first stream is exhausted, and looping the other:

#include <SDL3/SDL.h>

int main(int argc, char **argv)
{
    SDL_Init(SDL_INIT_AUDIO);

    SDL_AudioSpec musicspec, soundspec;
    Uint8 *musicbuf, *soundbuf;
    Uint32 musicbuflen, soundbuflen;
    SDL_LoadWAV("music.wav", &musicspec, &musicbuf, &musicbuflen);
    SDL_LoadWAV("tink.wav", &soundspec, &soundbuf, &soundbuflen);

    SDL_AudioDeviceID device = SDL_OpenAudioDevice(SDL_AUDIO_DEVICE_DEFAULT_OUTPUT, &musicspec);
    if (device) {
        SDL_AudioStream *musicstream = SDL_CreateAndBindAudioStream(device, &musicspec);
        SDL_AudioStream *soundstream = SDL_CreateAndBindAudioStream(device, &soundspec);

        SDL_PutAudioStreamData(musicstream, musicbuf, musicbuflen);

        while (SDL_GetAudioStreamAvailable(musicstream) > 0) {
            if (SDL_GetAudioStreamAvailable(soundstream) < soundbuflen) {
                SDL_PutAudioStreamData(soundstream, soundbuf, soundbuflen);
            }
            SDL_Delay(10);
        }

        SDL_DestroyAudioStream(musicstream);
        SDL_DestroyAudioStream(soundstream);
    }

    SDL_free(musicbuf);
    SDL_free(soundbuf);

    SDL_Quit();
    return 0;
}

There are many many other changes; the best plan is to find README-migration.md in the commit and read up on the differences. Notably: the commit deletes more code than it adds, so in many ways the new audio code is a simplication of the code and the API.

There is a lot to be done still, but this has been churning in my working copy for weeks now. Since this has finally gotten far enough that it can be made to work in the right conditions, I intend to work out of this PR and then squash it down before merging.

Some notable things to be done, still:

  • Right now I've updated the dummy, disk, and pulseaudio backends, just for testing purposes. Everything else will fail to build.
  • None of the test apps (except a hacked up loopwave) have been updated for the new API yet.
  • A proper test app to test multiple streams has not yet been written.
  • I'm still not thrilled with the device open semantics, still rethinking those.
  • Opening a default device does not work at the moment, because this is not yet hooked up to do anything.
  • SDL_GetDefaultAudioInfo is missing in action. I need to decide how to handle this, still. It will return!
  • I'd like to have the backends send notifications if the default device changes, and have SDL_audio.c manage hand-off of playback to new devices, instead of each backend that supports this having to manage it, but this work hasn't been started.
  • A single-header library that simulates the old audio callback, for apps that can't or won't migrate to a different paradigm, needs to be written (but I don't think it'll be too terrible to build).
  • sdl2-compat updates

List of backends to be updated:

  • aaudio
  • alsa
  • android
  • coreaudio
  • directsound
  • disk
  • dsp
  • dummy
  • emscripten
  • haiku
  • jack
  • n3ds
  • netbsd
  • openslES
  • pipewire
  • ps2
  • psp
  • pulseaudio
  • qnx
  • sndio
  • vita
  • wasapi

So we have a ways to go here, but this is the basic idea I'm moving towards. I have to spend some time on SDL 2.28.0 and sdl12-compat next week, and then I'll be returning to this. Feedback is certainly welcome in the meantime!

Fixes #7379.
Reference Issue #6889.
Reference Issue #6632.

@ccawley2011
Copy link
Contributor

Now the app will bind an SDL_AudioStream to a given device and feed data to it. As many streams as one likes can be bound to a device; SDL will mix them all into a single buffer and feed the device from there.

Would it be possible to allow mixing to be done by the audio drivers using this new API in order to take advantage of hardware capabilities?

@icculus
Copy link
Collaborator Author

icculus commented May 13, 2023

Would it be possible to allow mixing to be done by the audio drivers using this new API in order to take advantage of hardware capabilities?

I'd have to look into what is available on various APIs, but my suspicion is that most don't offer this, we'd have to keep a separate buffer for each stream, and mixing isn't a high-overhead operation in general.

I'd be more inclined to add SIMD versions of SDL_MixAudioFormat instead.

@icculus
Copy link
Collaborator Author

icculus commented May 13, 2023

I'm wondering if maybe it was too aggressive to remove SDL_AudioSpec entirely. A struct with just format, channels and sample rate could be nice, and probably save some code changes.

@icculus
Copy link
Collaborator Author

icculus commented May 13, 2023

I think I might expose three extra functions in SDL_AudioStream:

  • SDL_LockAudioStream
  • SDL_UnlockAudioStream

(There's already a mutex, this just lets others use it explicitly.)

  • SDL_SetAudioStreamCallback

Register a function that runs at the start of a SDL_GetAudioStreamData call. The callback can take the chance to add more data to the stream, or query the current amount, etc.

The end result is you can have the SDL2 callback interface, if you want it, and you can have it for each bound stream.

Actual code added to SDL is minimal, the old interface can be implemented without a lot of drama, or latency, or an extra single-header library. If SDL_mixer were so-inclined, it could move each audio channel to a stream and still be able to do posteffects, on-demand decoding, etc.

@slouken
Copy link
Collaborator

slouken commented May 13, 2023

That sounds awesome. :)

@mausimus
Copy link
Contributor

Hey, many thanks for all your work on SDL and apologies if this is not the right place to raise this.

One issue I've been running into when using SDL with SDL_mixer is that doing volume fades creates a popping sound because volume is only changed on chunk boundaries in a hard step fashion. This was not something that could be easily fixed in SDL_mixer as it was simply calling MixAudioFormat() with a single volume for the whole chunk. Do you see any way to make the new audio API more flexible so that smooth fades could be more easily implemented by devs and/or in SDL_mixer? There's an old discussion of this in SDL_mixer repo libsdl-org/SDL_mixer#190 .

@icculus
Copy link
Collaborator Author

icculus commented May 14, 2023

One issue I've been running into when using SDL with SDL_mixer is that doing volume fades creates a popping sound because volume is only changed on chunk boundaries in a hard step fashion.

Yeah, this is a legit bug in SDL_mixer, but the interpolation should happen there, I think.

libsdl-org/SDL_mixer#190 is the right place to discuss this.

@ccawley2011
Copy link
Contributor

A few more observations:

  • SDL_audio.h contains the comment \brief Access to the raw audio mixing buffer for the SDL library., which is what the old API provided rather than the new one.
  • Would it be possible to have default values for SDL_OpenAudioDevice()?
  • Am I correct in thinking that calling SDL_GetAudioStreamData() for a stream that's bound to a device is undefined behaviour? Is this something that should be guarded against?
  • Since the lower level mixing function supports setting the volume, is this something that can be set for streams for simpler use cases?
  • It would be nice to have a function that lists the formats, channel counts and sample rates supported natively by the device, so that it can be used in e.g. configuration dialogs.

and mixing isn't a high-overhead operation in general.

I suspect resampling and channel conversion are likely to be the key issues here (especially on CPUs without hardware floating point), since that needs to be done before mixing happens. That said, there doesn't seem to be anything major in the proposed API that prevents hardware mixing, so this might be something to investigate separately at a later date.

@icculus
Copy link
Collaborator Author

icculus commented May 18, 2023

SDL_audio.h contains the comment \brief Access to the raw audio mixing buffer for the SDL library., which is what the old API provided rather than the new one.

Good catch, I'll fix that.

Would it be possible to have default values for SDL_OpenAudioDevice()?

This should accept a device ID of 0 to request the default device, but this isn't hooked up at the moment, and there are still some logistics to figure out. There are going to be some API changes here still, I think.

Am I correct in thinking that calling SDL_GetAudioStreamData() for a stream that's bound to a device is undefined behaviour? Is this something that should be guarded against?

It isn't really undefined so much as it will absolutely ruin your output. :) But it's thread-safe and won't crash the app or anything.

I've thought about this (and also refusing to let the app change the stream's output format when it's bound to an output device), but my thinking is that if you put your finger on a hot stove, eventually you'll figure out to not do that.

Since the lower level mixing function supports setting the volume, is this something that can be set for streams for simpler use cases?

I think I have a FIXME in there to consider this. I was not going to do this, since it opens up a world of One More Things people would like added until we just reimplement SDL_mixer, but with the callback plan, maybe we can avoid feature creep, so maybe I will add this.

It would be nice to have a function that lists the formats, channel counts and sample rates supported natively by the device, so that it can be used in e.g. configuration dialogs.

There's an SDL2 API that was lost in here, SDL_GetDefaultAudioInfo, which needs to be readded once I figure out the default device politics. For specific devices, the current format (which is our best guess at a preferred format when not opened), is already available. SDL has never listed all possible formats, and I don't think it's useful to do so...in many cases, you're just moving where data conversion happens if you try to pick a "native" format.

@icculus
Copy link
Collaborator Author

icculus commented May 19, 2023

Wishlist item: there should be a way to query if device permission is available, or forbidden, or pending user response, if this is something various platforms expose.

iOS and Android obviously do this for the microphone, but web browsers will forbid access to audio output, too, until the user has interacted with the page, and WinRT makes approval of WASAPI device opens async, presumably for situations where they want users to approve it.

Having a more formal way to deal with that in SDL apps would be nice. I don't know what, yet.

@icculus
Copy link
Collaborator Author

icculus commented May 29, 2023

SDL_AudioStream callbacks are in, and loopwave has been updated to use them for testing purposes, and the changes to move from SDL2 to SDL3 are pretty small with this approach. This is a good improvement.

@icculus icculus changed the title audio: First shot at the SDL3 audio subsystem redesign! The SDL3 audio subsystem redesign! May 30, 2023
@icculus
Copy link
Collaborator Author

icculus commented May 30, 2023

SDL_AudioSpec is back in, but just as a thing that holds format/channels/frequency. It actually tightens up a bunch of code, and its purpose is really clear now vs SDL2, so I'm happy with its return.

@icculus
Copy link
Collaborator Author

icculus commented Jun 19, 2023

So one stumbling point is that I wanted to remove device open/close, and just let people bind streams to devices, but this causes other problems (people will want to have a definite shutdown point where "closing" the device will stop all their sounds, but what do you do if something else also has streams bound to a device? If you want to pause the device, there isn't an easy button here beyond unbinding all your streams at once, etc).

So I guess we're going to keep an open/close API, and opening will return a new device ID, even though internally these fake devices will all just mix onto a single physical device, but the VoIP library's streams will be logically separated from the movie playback library's streams, and the app's own streams, and pausing a device will just stop mixing one logical group, and closing a device will just unbind that group from the device.

It adds a little internal complexity, but it seems like the right thing to do, and will be less confusing for app developers.

@icculus
Copy link
Collaborator Author

icculus commented Jun 20, 2023

Of course, now we have device ids that can be used with some APIs (SDL_BindAudioStream needs a logical device) and device ids that can be used with others (SDL_OpenAudioDevice needs a physical device) and some that can reasonably be used with both (SDL_GetAudioDeviceName)

Have to think on this more.

@icculus
Copy link
Collaborator Author

icculus commented Jun 20, 2023

Actually, this is probably fine. Binding a stream to a physical device will fail, which might be confusing, but everything else can be made to reasonably work, including opening a new logical device from an existing logical device (and might even be useful if you want to make a temporary logical grouping of streams).

@icculus
Copy link
Collaborator Author

icculus commented Jun 21, 2023

Ok, logical audio devices are in, here's the silly test program doing the two streams with music and sound, plus a second open of the same device (done by opening the logical device's id, so you don't have to keep the original physical device id around!), playing the sound at an offset, so you can hear them all mixing into a single buffer for the actual hardware:

#include <SDL3/SDL.h>

int main(int argc, char **argv)
{
    SDL_Init(SDL_INIT_AUDIO);

    Uint8 *musicbuf, *soundbuf;
    Uint32 musicbuflen, soundbuflen;
    SDL_AudioSpec musicspec, soundspec;
    SDL_LoadWAV("music.wav", &musicspec, &musicbuf, &musicbuflen);
    SDL_LoadWAV("tink.wav", &soundspec, &soundbuf, &soundbuflen);

    SDL_AudioDeviceID *devices = SDL_GetAudioOutputDevices(NULL);
    const SDL_AudioDeviceID device = devices ? SDL_OpenAudioDevice(devices[0], &musicspec) : 0;
    SDL_free(devices);
    if (device) {
        const SDL_AudioDeviceID device2 = SDL_OpenAudioDevice(device, &musicspec);
        SDL_AudioStream *musicstream = SDL_CreateAndBindAudioStream(device, &musicspec);
        SDL_AudioStream *soundstream = SDL_CreateAndBindAudioStream(device, &soundspec);
        SDL_AudioStream *soundstream2 = SDL_CreateAndBindAudioStream(device2, &soundspec);
        Uint64 nextsound = 0;

        SDL_PutAudioStreamData(musicstream, musicbuf, musicbuflen);
        SDL_free(musicbuf);

        while (SDL_GetAudioStreamAvailable(musicstream) > 0) {
            if (SDL_GetAudioStreamAvailable(soundstream) < soundbuflen) {
                SDL_PutAudioStreamData(soundstream, soundbuf, soundbuflen);
            }
            if (SDL_GetAudioStreamAvailable(soundstream2) == 0) {
                if (!nextsound) {
                    nextsound = SDL_GetTicks() + 1000;
                } else if (nextsound <= SDL_GetTicks()) {
                    SDL_PutAudioStreamData(soundstream2, soundbuf, soundbuflen);
                }
            }
            SDL_Delay(10);
        }

        SDL_DestroyAudioStream(musicstream);
        SDL_DestroyAudioStream(soundstream);
        SDL_DestroyAudioStream(soundstream2);

        SDL_CloseAudioDevice(device2);
        SDL_CloseAudioDevice(device);
    }

    SDL_free(soundbuf);

    SDL_Quit();
    return 0;
}

This is complexity the average app won't need directly; it's intended to make things work when some external library wants to open a device too, and doesn't coordinate with the app to share one.

But it's also kinda glorious.

@icculus
Copy link
Collaborator Author

icculus commented Jun 23, 2023

Latest commit still has some loose ends to tie up, but not only are most of the details for default devices back in place, SDL can now handle migrating playback to a new default device when the system default changes.

Before this was pretty much something we handled explicitly in the CoreAudio backend (and just asked PulseAudio to manage for us implicitly), but now for any backend, we can just scoot all the logical devices over to different physical hardware, change the format of the business end of their audio streams, and keep going.

The backend just needs to be able to tell us when a new default device was selected (user changed it in system controls, they plugged in headphones, etc), and the higher level does the rest!

@slouken
Copy link
Collaborator

slouken commented Jun 23, 2023

Nice!

@icculus
Copy link
Collaborator Author

icculus commented Jun 24, 2023

  • Rebased this to branch from the top of main, since I was getting behind.
  • Default device opens still migrate, as mentioned before, but now it's smart enough to manage a USB cable being yanked out without warning until the OS picks a new default device. Previously this was only smart enough to deal with the user choosing a new default while all involved hardware was still functioning.
  • SDL_GetDefaultAudioInfo() is removed; you can request default format info from SDL_GetAudioDeviceFormat(SDL_AUDIO_DEVICE_DEFAULT_OUTPUT, &spec) now, and since we no longer open devices with a name string and default device opens can quietly migrate to different physical hardware, it's better to show the user a name like "System default" than the specific current default device anyhow.

@icculus
Copy link
Collaborator Author

icculus commented Jun 27, 2023

sdl2-compat work is sitting in libsdl-org/sdl2-compat#80, which was like climbing a mountain, but I've almost reached the summit now.

@icculus
Copy link
Collaborator Author

icculus commented Jun 27, 2023

So I'm reworking the Pipewire backend, and while this is proving to be a good test of the new system for backends that provide their own threads, I'm wondering if the Pipewire implementation is wrong. I suspect the API is meant to work like the new PulseAudio threading code, where you let it spin one thread to dispatch PulseAudio events, and then all your threads cooperate around that.

Right now it is spinning a thread for each device, which is not bad in itself and what we would do, but I'm wondering if each thread they spin is fighting for the same socket and event queue anyhow, and we should restructure this to match the new PulseAudio code.

As an added bonus, then it can use the standard SDL device thread.

This is allegedly lower-latency than the AAudioStream_write interface,
but more importantly, it let me set this up to block in WaitDevice.

Also turned on the low-latency performance mode, which trades battery life
for a more efficient audio thread to some unspecified degree.
I can't ever find this when it's in the middle! It's a "me" problem.  :)
@madebr
Copy link
Contributor

madebr commented Jul 30, 2023

On Linux, testaudiostreamdynamicresample segfaults for me:

#0  SDL_BindAudioStreams_REAL (devid=9, streams=0x7fffffffd9e0, num_streams=1)
    at /home/maarten/projects/SDL/src/audio/SDL_audio.c:1383
        j = 0
        stream = 0x0
        i = 0
        islogical = SDL_TRUE
        logdev = 0x11dca70
        __func__ = "SDL_BindAudioStreams_REAL"
        device = 0x7ffff000d240
        retval = -1
#1  0x00007ffff7c311c3 in SDL_BindAudioStream_REAL (devid=9, stream=0x0)
    at /home/maarten/projects/SDL/src/audio/SDL_audio.c:1423
#2  0x00007ffff7c50aa9 in SDL_BindAudioStream (a=9, b=0x0)
    at /home/maarten/projects/SDL/src/dynapi/SDL_dynapi_procs.h:933
#3  0x00000000004013cd in main (argc=1, argv=0x7fffffffdca8)
    at /home/maarten/projects/SDL/test/testaudiostreamdynamicresample.c:41
        window = 0x46a270
        renderer = 0x59da30
        done = SDL_FALSE
        slider_area = {x = 70, y = 190, w = 500, h = 100}
        slider_fill_area = {x = 70, y = 190, w = 500, h = 100}
        multiplier = 100
        spec = {format = 0, channels = 6, freq = 60}
        audio_buf = 0x0
        audio_len = 0
        stream = 0x0
        device = 9
(gdb) p j
$1 = 0
(gdb) p streams[j]
$2 = (SDL_AudioStream *) 0x0

On Android, testmultiaudio plays corrupted audio (the song plays). I recorded the audio with my laptop.

Also on Android, testaudiocapture segfaults on start. I found the following backtrace in the tombstone:

backtrace:
      #00 pc 000372fc  /apex/com.android.runtime/lib/bionic/libc.so (__memcpy_base_a55+244) (BuildId: 6a4f2456a60f5e24e5bb9a6ea5e68013)
      #01 pc 0008ec6f  /data/app/~~ZH7iQQmyS3F9jSTTPGzKaA==/org.libsdl.sdl.test.testaudiocapture-LUrCFO645pFx_SDUDdBtvQ==/lib/arm/libSDL3.so (SDL_memcpy+20) (BuildId: 84bd037c074848442e45eff66c9dbb8040f2d5bc)
      #02 pc 000ebd05  /data/app/~~ZH7iQQmyS3F9jSTTPGzKaA==/org.libsdl.sdl.test.testaudiocapture-LUrCFO645pFx_SDUDdBtvQ==/lib/arm/libSDL3.so (AAUDIO_dataCallback+96) (BuildId: 84bd037c074848442e45eff66c9dbb8040f2d5bc)
      #03 pc 0001e62d  /system/lib/libaaudio_internal.so (aaudio::AudioStream::maybeCallDataCallback(void*, int)+124) (BuildId: d4474414403512bc993720901e3376b3)
      #04 pc 00021b6d  /system/lib/libaaudio_internal.so (aaudio::AudioStreamLegacy::callDataCallbackFrames(unsigned char*, int)+256) (BuildId: d4474414403512bc993720901e3376b3)
      #05 pc 000294ed  /system/lib/libaaudio_internal.so (FixedBlockWriter::processVariableBlock(unsigned char*, int)+220) (BuildId: d4474414403512bc993720901e3376b3)
      #06 pc 00021331  /system/lib/libaaudio_internal.so (aaudio::AudioStreamLegacy::onMoreData(android::AudioRecord::Buffer const&)+628) (BuildId: d4474414403512bc993720901e3376b3)
      #07 pc 00045e03  /system/lib/libaudioclient.so (android::AudioRecord::processAudioBuffer()+1330) (BuildId: 28c5158b4da6808b5d413e114e280cd6)
      #08 pc 00045673  /system/lib/libaudioclient.so (android::AudioRecord::AudioRecordThread::threadLoop()+186) (BuildId: 28c5158b4da6808b5d413e114e280cd6)
      #09 pc 0000d789  /system/lib/libutils.so (android::Thread::_threadLoop(void*)+264) (BuildId: 8cf7b6359f56de8d1c47c0ca3a8671b1)
      #10 pc 00088695  /system/lib/libandroid_runtime.so (android::AndroidRuntime::javaThreadShell(void*)+84) (BuildId: d69ce7249cf5644a9fe0b55d648dddbc)
      #11 pc 00083d3f  /apex/com.android.runtime/lib/bionic/libc.so (__pthread_start(void*)+40) (BuildId: 6a4f2456a60f5e24e5bb9a6ea5e68013)
      #12 pc 0003abad  /apex/com.android.runtime/lib/bionic/libc.so (__start_thread+30) (BuildId: 6a4f2456a60f5e24e5bb9a6ea5e68013)

@icculus
Copy link
Collaborator Author

icculus commented Jul 31, 2023

Also on Android, testaudiocapture segfaults on start. I found the following backtrace in the tombstone:

This was a dumb bug, fixed now.

I'll look at testaudiostreamdynamicresample.

@icculus
Copy link
Collaborator Author

icculus commented Jul 31, 2023

I'll look at testaudiostreamdynamicresample.

It needed some help to find sample.wav after the test building changes. This is fixed now (but this program really needs to be reworked to use the test framework stuff that the "real" test apps use, at some point).

@madebr
Copy link
Contributor

madebr commented Jul 31, 2023

It needed some help to find sample.wav after the test building changes. This is fixed now (but this program really needs to be reworked to use the test framework stuff that the "real" test apps use, at some point).

Oops, I launched it from the wrong directory. Sorry about that.

I see the following behavior with testaudiostreamdynamicresample:

  • it works great for normal and faster resample, but when going slower then 4x slowdown, popping starts to become audible (Linux)
  • android "opensles" and "android" audio driver have the same behavior as Linux
  • android "aaudio" driver has the annoying audio artifacts (see The SDL3 audio subsystem redesign! #7704 (comment))

testaudiocapture works great on Linux, but behaves strange on Android:

  • the android "opensles" driver plays the audio (approximately) twice too fast
  • the android "aaudio" driver also plays the audio back too fast, and corrupts it a bit.
  • does not work with the "android" driver, but that is expected

@icculus
Copy link
Collaborator Author

icculus commented Jul 31, 2023

There's a resampling bug in mainline SDL3 pending a fix, which is where your pops are coming from on all platforms.

I'll take a look at the other issues. Wrapping up 2.28.2 and such today and then I can get to that.

@icculus
Copy link
Collaborator Author

icculus commented Aug 2, 2023

We have a test app!

And a demonstration of the new stuff on YouTube:

Demo on YouTube

So we're ready to merge this now!

@icculus icculus marked this pull request as ready for review August 2, 2023 19:06
@slouken
Copy link
Collaborator

slouken commented Aug 2, 2023

Sweet!

Does it make sense to squash this to a single commit?

@slouken
Copy link
Collaborator

slouken commented Aug 2, 2023

Demo on YouTube

This is super sexy!

@icculus
Copy link
Collaborator Author

icculus commented Aug 2, 2023

Does it make sense to squash this to a single commit?

I was thinking of leaving it as a merge, not a rebase, because we could very well want to step through some of these 138 commits in the future as a pile of commits on an unnamed branch, but the main timeline is just going to see "this is where the audio changes merged" and not have to deal with the mess when bisecting, etc.

Normally I'd say squash, but this is a ton of changes to make look like one unit of work.

I might take a run at interactive rebasing to squash out "patch to compile on [whatever platform]" commits in any case.

But let's CC a few people that might have opinions on the right way to merge this beast: @libsdl-org/a-team @smcv

@icculus
Copy link
Collaborator Author

icculus commented Aug 2, 2023

Also, shoot, the test app made this add code:

image

Subtract a thousand lines for the actual subsystem work, lol.

@madebr
Copy link
Contributor

madebr commented Aug 2, 2023

Great presentation, and the test app looks really slick!

I don't have a reproducer, other then me creating, removing and moving around logical audio devices (from my laptop capture device) in the testaudio app, but the following assertion error throws after some time:

Assertion failure at OpenPhysicalAudioDevice (/home/maarten/projects/SDL/src/audio/SDL_audio.c:1193), triggered 1 time:
  'device->logical_devices == ((void *)0)'

Another Android change, which I don't know is intentional: when I open the testmultiaudio app through scrcpy, the audio starts playing on the speakers of my computer instead of those of the phone (without artifacts!).

@icculus
Copy link
Collaborator Author

icculus commented Aug 2, 2023

Another Android change, which I don't know is intentional: when I open the testmultiaudio app through scrcpy, the audio starts playing on the speakers of my computer instead of those of the phone (without artifacts!).

That's good, right? :)

Right now we don't have a way to determine the default audio device in Android, so it goes with the first one it sees during device enumeration (and on a phone, this generally works out). I haven't found an API that tells me the actual default yet, but there are various things that mention the "preferred" device in the API reference, so I'm assuming I'll stumble upon this at some point. Right now it's still an open question to be resolved. But that (deciding which integer is the default for a given device) just needs to be settled before 3.2.0 ships.

I'll try to track down the assert over here.

@smcv
Copy link
Contributor

smcv commented Aug 3, 2023

I'm writing this based entirely on recent comments and have not looked at the actual changes.

I was thinking of leaving it as a merge, not a rebase

I think you're right. I would say that even if the commits get rebased onto main before merging, something this size likely makes more sense as git merge --no-ff sdl3-audio-redesign, with a merge commit to mark the point at which it was merged.

Some projects like GLib and dbus do everything as a merge with a merge commit, even if the commits could have been fast-forwarded ("semi-linear history" - look at one of those projects in gitk or similar and you'll see what I mean). I personally like that approach better than always rebasing: one advantage of doing that is that the git commit history ends up with a reference to the pull request/merge request, rather than the PRs being Github-/Gitlab-specific metadata that becomes invisible if you clone the repo elsewhere.

Normally I'd say squash, but this is a ton of changes to make look like one unit of work.

Almost 8K lines of diff seems unhelpfully big for a single commit! I think git blame would point to that mega-commit unhelpfully frequently.

I might take a run at interactive rebasing to squash out "patch to compile on [whatever platform]" commits in any case.

If there are places where a commit was just wrong, and a subsequent commit fixes it up to be right, then doing the interactive rebase to make it look as if you had got it right the first time makes sense - when doing archaeology to find out why some code is the way it is, the history of what would have happened in a hypothetical past where you had never made mistakes is often more useful than the history of what actually happened.

But if it's hard to achieve that, for a branch of this size it's not going to be realistic to bisect into the middle of it in any case, so it doesn't matter so much if the tree passed through some broken intermediate states that can't be compiled or don't work; better for future code-historians to preserve the context of separate commits than to squash everything into fewer/larger commits, IMO.

(One of the things I've learned from contributing to projects like SDL, GLib and dbus is that "maintainer" and "code historian" have a surprising amount in common!)

@icculus
Copy link
Collaborator Author

icculus commented Aug 3, 2023

Okay, I'm happy to see I'm on the right path here.

I guess this is last call; I might do some minor cleanup to the revision history if possible, but I'll click the "merge" button later today unless someone says "stop!"

@icculus icculus merged commit 18c59cc into libsdl-org:main Aug 4, 2023
35 checks passed
@icculus icculus deleted the sdl3-audio-redesign branch August 4, 2023 01:28
@icculus
Copy link
Collaborator Author

icculus commented Aug 4, 2023

We are merged! 😬

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.

Remove the audio callback!
9 participants