add libsoundio audio backend

[andrewrk]

This adds libsoundio (http://libsound.io/) as an available audio
backend. libsoundio supports JACK, PulseAudio, ALSA, CoreAudio,
WASAPI, and a dummy backend.

[tresf]

😄

[andrewrk]

@tresf do you know where the source is to tobydox's mingw ppa?

[tresf]

@tresf do you know where the source is to tobydox's mingw ppa?

No, I've never witnessed it. Here's a convo I had with him in November 2014

Tres: Loaded question... From a non c/c++ developer's perspective... how hard would it be to setup the mingw repos so that we could take that over?

Toby: What do you mean by take over? You can of course copy the source packages into your own PPA and/or build them on your own in a Debian-based environment but I don't see how this would help. I'm still maintaining the packages so if you do this on your own, it'll be extra work.

He's quite nonchalant, but AFAIK, he does some specific things to get it all working including providing PPAs for both Ubuntu 12.04 and 14.04. The good part is that he promises to keep maintaining them, so tagging him should be enough.

[andrewrk]

@tobydox want to add libsoundio to your mingw PPA? It should be very easy since the official Windows binaries of libsoundio are compiled with mingw, and it has no dependencies.

[andrewrk]

Linux travis checks are passing now. To pass the remaining checks we need:

• @tobydox to add libsoundio to his mingw PPA
• someone to make a homebrew pull request to add libsoundio

[andrewrk]

Alright, I went ahead and disabled libsoundio in the travis build for macos and windows. Once libsoundio becomes available in those target environments we can re-enable travis testing for it.

In the mean time, the build is green, anyone want to take a look and merge?

[Wallacoloo]

@andrewrk the travis builds should not be failing on any platform. If libsoundio is not present on the system (i.e. Windows and Mac travis builds, for now), lmms should still compile, just without support for libsoundio as an audio backend. This is the way all the other audio backends are done.

here's a log of when I attempt to compile your branch without installing libsoundio. You'll notice I don't have portaudio installed either, but previously LMMS would still compile without it.

[Wallacoloo]

Ah, I see the travis tests are passing, so ignore the first part of my previous comment. The rest of it still stands though, in that default compilation of LMMS (i.e. cmake .) should not fail if libsoundio is not found.

[andrewrk]

@Wallacoloo good point. I made it build successfully without libsoundio and I reverted the changes to travis and the checks still pass. I think we're good to go now.

[andrewrk]

@Wallacoloo are you OK with me merging this? Travis checks out and this time it's not explicitly disabling libsoundio in the travis scripts.

[Wallacoloo]

@andrewrk give me some time to test it out & review the code first. I'll try to do that today.

[Wallacoloo]

Working on building this. I'm getting linker errors when compiling libsoundio from source, related to jack:

[ 21%] Linking C executable backend_disconnect_recover
libsoundio.so.1.0.1: undefined reference to `jack_set_port_rename_callback'

Info:

$ ls /lib | grep libjack
libjackserver.so
libjackserver.so.0
libjackserver.so.0.0.28
libjack.so
libjack.so.0
libjack.so.0.0.28

$ pacman -Q jack
jack 0.124.1-3

This is on 64-bit Arch. I guess the libjack.so versioning differs from the library versioning. Do you think there's a chance you're using any deprecated APIs or something? By the way, this is a known Arch issue - the arch package for libsoundio includes a patch to explicitly disable jack support: https://aur.archlinux.org/packages/libsoundio-git/

I'm going to proceed by building without JACK support.

[andrewrk]

See andrewrk/libsoundio#7

FWIW Debian unstable's jackd2 package includes the jack2 commit in question.

[andrewrk]

Oh, oops, you're trying to build with jack1. See this issue instead: andrewrk/libsoundio#11

[andrewrk]

@Wallacoloo re: your comment on the AUR. Is it perhaps because libsoundio uses GNUInstallDirs in CMakeLists.txt?

[Wallacoloo]

@andrewrk I have no idea, unfortunately. The conventions across linux OS's are all subtly different.

Anyway, lmms is working with libsoundio. Audio output works flawlessly. However, I am consistently getting a hang upon exit when using libsoundio as output, under any backend (ALSA, Pulse, Dummy). The gui closes, but lmms never actually exits. Here's a backtrace when using the Dummy libsoundio interface: https://gist.github.com/Wallacoloo/b540d0d37a94512019a7

I would guess it has to do with a thread interfacing with libsoundio not being notified to exit, or something along those lines.

[andrewrk]

I put print statements in AudioSoundIo::~AudioSoundIo() {} and none of them got called. I think LMMS is not properly cleaning up AudioDevice objects.

[andrewrk]

Mixer::~Mixer()
{
    // ...
    while( m_fifo->available() )
    {
        delete[] m_fifo->read(); // <--------- hangs forever
    }
    // this code never gets run
    delete m_audioDev;
    // ...
}

[andrewrk]

Alright I think I know the problem. stopProcessing is returning before audio callback is finished, so there's a cleanup race condition. Fix coming shortly.

[andrewrk]

I made stopProcessing not return until the audio callback is guaranteed to be finished. However there's still an issue. It looks like an internal LMMS issue though. See this valgrind output: http://paste.ubuntu.com/12401795/

void Mixer::stopProcessing()
{
    if( m_fifoWriter != NULL )
    {
        m_fifoWriter->finish();
        m_audioDev->stopProcessing();
        m_fifoWriter->wait( 1000 );
        m_fifoWriter->terminate();
        delete m_fifoWriter;
        m_fifoWriter = NULL;
    }
    else
    {
        m_audioDev->stopProcessing();
    }
}

        m_fifoWriter->wait( 1000 );
        m_fifoWriter->terminate();

Yikes. This should be:

        m_fifoWriter->wait();

When I do this I discover that the fifo writer thread is not exiting for some reason.

[andrewrk]

@Wallacoloo alright. I found and fixed the bug. It was a race condition in Mixer.cpp:

--- a/src/core/Mixer.cpp
+++ b/src/core/Mixer.cpp
@@ -221,11 +221,10 @@ void Mixer::stopProcessing()
        if( m_fifoWriter != NULL )
        {
                m_fifoWriter->finish();
-               m_audioDev->stopProcessing();
-               m_fifoWriter->wait( 1000 );
-               m_fifoWriter->terminate();
+               m_fifoWriter->wait();
                delete m_fifoWriter;
                m_fifoWriter = NULL;
+               m_audioDev->stopProcessing();
        }
        else
        {

Before, the fifo writer thread was waiting on m_fifo->write(buffer) trying to acquire the write semaphore. But in order to acquire the write semaphore, the audio backend would have to call getNextBuffer (the reader). But the mixer just called stopProcessing on the audio backend, so the audio backend is done and won't call getNextBuffer.

So, to fix it, instead we first wait until the fifo writer is done, then tell the audio backend to stop processing.

Have another go at the code, if you will :-)

[Wallacoloo]

Not sure how big of a deal this is, but all the other audio/midi backends configure their includes in /src/CMakeLists.txt instead of /CMakeLists.txt so as not to affect the building of plugins.

[andrewrk]

The first thing I tried was using PortAudio's CMake lines as a template but that lead to inability to build if the library is not installed. Instead of trying to figure out why that wasn't working, I opted to just do it how I do it in my own projects, since I know that will work.

[Wallacoloo]

Ok. Probably not a big deal - I wouldn't sink any time into changing that then.

[tresf]

The first thing I tried was using PortAudio's CMake lines as a template but that lead to inability to build if the library is not installed. Instead of trying to figure out why that wasn't working, I opted to just do it how I do it in my own projects, since I know that will work.

In regards to the CMakeLists lines, I'll be the odd man out and say I'd prefer we keep this consistent -- speaking less about where it should be but rather on behalf of the recent cleanup efforts.

From my experience, variable scope and order of operations is what seems to cause most CMakeLists.txt problems. At a glance, LMMS_REQUIRED_LIBS adds ${PORTAUDIO_LIBRARIES} to parent scope. In my experience, the PARENT_SCOPE related issues are usually the cause of ADD_SUBDIRECTORY headaches and this is because a local variable in cmake isn't exposed to its parent.

Anyway, happy to help. 👍

[andrewrk]

I'm not sure I understand what you're suggesting. I basically grepped for portaudio, copied all the cmake configuration, then replaced portudio with soundio. The only difference is the cmake/modules/Find*.cmake file, which I suspect is the culprit. But I'm sure that the FindSoundIo.cmake is OK and the PortAudio one looks complicated and not how I would do it.

[tresf]

grepped for portaudio, copied all the cmake configuration, then replaced portudio with soundio

That's what I get for only reading the comments. 😄

I see now that it's a single line... INCLUDE_DIRECTORIES. Did you clone the IF(NOT ("${PULSEAUDIO_INCLUDE_DIR}" STREQUAL "")) logic? Because if you don't define SOUNDIO_INCLUDE_DIR, you're better off doing an IF(WANT_SOUNDIO AND SOUNDIO_FOUND), right (in src/CMakeLists.txt that is)?

Edit: Hmm... I see you do define SOUNDIO_INCLUDE_DIR in FindSoundIo.cmake. I'm confused now too.

[andrewrk]

Did you clone the IF(NOT ("${PULSEAUDIO_INCLUDE_DIR}" STREQUAL "")) logic?

Yes I cloned that logic too.

[Wallacoloo]

Make sure to use tabs for indentation, not spaces (lmms convention).

[andrewrk]

Alright.

• made the requested fields private
• hard tabs instead of spaces
• camelCase instead of snake_case
• curly brace on the new line

[Wallacoloo]

Hey, thanks for fixing those up so quickly @andrewrk.

I've recompiled with libsoundio support. The original hang has been fixed, but while testing the other backends, some of them now have hangs in a similar location that weren't there before. Specifically, SDL. Set the backend to SDL (with device="", a.k.a. default), restart so changes take place, and then close lmms. For me, it hangs upon exit 2/3rds of the time. Backtrace: https://gist.github.com/Wallacoloo/0ad08ea27ee6abc9df81

Currently testing the other backends - so far, SDL seems to be an exception & the rest seem to work fine.

[Wallacoloo]

Yeah, all other backends are working fine for me, though I couldn't test JACK or OSS.

[andrewrk]

I tested JACK; it exited cleanly 3/3 times.

[andrewrk]

I think SDL is broken in master branch, I put a print statement in sdlAudioCallback and it is never run.

Either way, if the SDL backend is messed up, it's a bug in the SDL backend, not the libsoundio backend. The core fix is necessary to clean up without a race condition.

[Wallacoloo]

@andrewrk SDL is working on master for me:

(gdb) b AudioSdl::sdlAudioCallback
(gdb) r
Starting program: /usr/local/bin/lmms 
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/usr/lib/libthread_db.so.1".
Notice: could not set realtime priority.
[New Thread 0x7fffdfdf6700 (LWP 29418)]
[New Thread 0x7fffd7fff700 (LWP 29419)]
[New Thread 0x7fffdf5f5700 (LWP 29420)]
VST sync support disabled in your configuration
[New Thread 0x7fffbe558700 (LWP 29435)]
[Thread 0x7fffbe558700 (LWP 29435) exited]
[New Thread 0x7fffbe558700 (LWP 29436)]
[New Thread 0x7fffbdd57700 (LWP 29437)]
[New Thread 0x7fffbd556700 (LWP 29438)]
[Switching to Thread 0x7fffbe558700 (LWP 29436)]

Breakpoint 7, 0x000000000056229c in AudioSdl::sdlAudioCallback(void*, unsigned char*, int) ()
(gdb) bt
#0  0x000000000056229c in AudioSdl::sdlAudioCallback(void*, unsigned char*, int) ()
#1  0x00007ffff61aa8c9 in ?? () from /usr/lib/libSDL-1.2.so.0
#2  0x00007ffff61b2fe8 in ?? () from /usr/lib/libSDL-1.2.so.0
#3  0x00007ffff61f48a9 in ?? () from /usr/lib/libSDL-1.2.so.0
#4  0x00007ffff7bc54a4 in start_thread () from /usr/lib/libpthread.so.0
#5  0x00007ffff431c13d in clone () from /usr/lib/libc.so.6

(/usr/local/bin/lmms is the location of my build of lmms master - I just pulled master & rebuilt about an hour ago)

[andrewrk]

OK I think I might have spotted the bug by eyeballing it (I can't test since SDL isn't working for me).

I bet that this is happening:

            // frames depend on the sample rate
            const fpp_t frames = getNextBuffer( m_outBuf );
            if( !frames )
            {
                m_stopped = true;
                m_stopSemaphore.release();
                memset( _buf, 0, _len );
                return;
            }

Then next time the audio callback is run:

    if( m_stopped )
    {
        memset( _buf, 0, _len );
        return;
    }

So that happens, then we call stopProcessing:

        m_stopSemaphore.acquire();

Oops. too late. And checking m_stopped is a race condition as long as the callback is running. But, check out these docs: https://www.libsdl.org/release/SDL-1.2.15/docs/html/sdllockaudio.html
Thus the semaphore is completely unnecessary and is causing the problem. So here's a fix:

diff --git a/include/AudioSdl.h b/include/AudioSdl.h
index ba2f09f..a5ce279 100644
--- a/include/AudioSdl.h
+++ b/include/AudioSdl.h
@@ -82,8 +82,7 @@ private:

    bool m_convertEndian;

-   volatile bool m_stopped;
-   QSemaphore m_stopSemaphore;
+   bool m_stopped;

 } ;

diff --git a/src/core/audio/AudioSdl.cpp b/src/core/audio/AudioSdl.cpp
index 4587a61..9d16b5e 100644
--- a/src/core/audio/AudioSdl.cpp
+++ b/src/core/audio/AudioSdl.cpp
@@ -42,8 +42,7 @@ AudioSdl::AudioSdl( bool & _success_ful, Mixer*  _mixer ) :
    AudioDevice( DEFAULT_CHANNELS, _mixer ),
    m_outBuf( new surroundSampleFrame[mixer()->framesPerPeriod()] ),
    m_convertedBufPos( 0 ),
-   m_convertEndian( false ),
-   m_stopSemaphore( 1 )
+   m_convertEndian( false )
 {
    _success_ful = false;

@@ -78,8 +77,6 @@ AudioSdl::AudioSdl( bool & _success_ful, Mixer*  _mixer ) :
    }
    m_convertEndian = ( m_audioHandle.format != actual.format );

-   m_stopSemaphore.acquire();
-
    _success_ful = true;
 }

@@ -89,7 +86,6 @@ AudioSdl::AudioSdl( bool & _success_ful, Mixer*  _mixer ) :
 AudioSdl::~AudioSdl()
 {
    stopProcessing();
-   m_stopSemaphore.release();

    SDL_CloseAudio();
    SDL_Quit();
@@ -114,9 +110,8 @@ void AudioSdl::stopProcessing()
 {
    if( SDL_GetAudioStatus() == SDL_AUDIO_PLAYING )
    {
-       m_stopSemaphore.acquire();
-
        SDL_LockAudio();
+        m_stopped = true;
        SDL_PauseAudio( 1 );
        SDL_UnlockAudio();
    }
@@ -176,8 +171,6 @@ void AudioSdl::sdlAudioCallback( Uint8 * _buf, int _len )
            const fpp_t frames = getNextBuffer( m_outBuf );
            if( !frames )
            {
-               m_stopped = true;
-               m_stopSemaphore.release();
                memset( _buf, 0, _len );
                return;
            }

Can you test it?

[Wallacoloo]

I'll go ahead and test that.

[Wallacoloo]

Well that patch fixes it. No deadlock & SDL output is still working reliably. Nicely done!

Technically, we still need some sort of synchronization for m_stopped though, right? C++ allows for memory reordering such that the write to m_stopped in stopProcessing() could happen at a later time, or never go through if the compiler cached it in a register. Same with in the callback - if the compiler had enough context it would be legal for it to read m_stopped once in that thread and then cache it forever.

Luckily, I think the callback is called from the SDL library, and so the compiler doesn't have enough information to avoid the read, and in the first case it doesn't make sense to reorder the m_stopped write or save it in a register. I do believe we are still walking in undefined territory though. Correct me if I'm wrong.

But the previous code wasn't fully protected against both cases anyway. A lot of people misuse volatile. Accesses to a volatile variable cannot be optimized away and cannot be reordered with respect to eachother, but they can be reordered with respect to other memory accesses. So the callback is protected against optimization by declaring m_stopped as volatile, but the stopProcessing() thread could still delay the write to m_stopped indefinitely.

Technically, I believe we require a memory fence after each write to m_stopped in the main thread and before each read from it in the callback thread to ensure correct behavior. But we could totally leave it as-is and get away with it. God knows how much other undefined behavior LMMS relies on.

edit: Actually, I think locking a semaphore probably implies a memory barrier, which would mean the previous code was well-defined.

[andrewrk]

Technically, we still need some sort of synchronization for m_stopped though, right? C++ allows for memory reordering such that the write to m_stopped in stopProcessing() could happen at a later time, or never go through if the compiler cached it in a register. Same with in the callback - if the compiler had enough context it would be legal for it to read m_stopped once in that thread and then cache it forever.

SDL_LockAudio() and SDL_UnlockAudio() are acting as mutexes here, and it is their job to implement the memory fence semantics. http://stackoverflow.com/questions/24137964/does-pthread-mutex-lock-contains-memory-fence-instruction

I checked a little bit of SDL source code and on linux they use pthread mutexes, which have memory barrier semantics. So the memory barrier is when locking and unlocking and m_stopped is protected by being modified only when the audio is locked.

[Wallacoloo]

@andrewrk 👍 Looks like the only thing left to do before merging is for you & @tresf to figure out that cmake stuff then.

[tresf]

Looks like the only thing left to do before merging is for you & @tresf to figure out that cmake stuff then.

I hadn't realized it was a one-liner. I thought your comment on the audio-backend cmake logic was more than a single line. Don't hold up progress on my behalf, although at a glance it should work either using the original syntax, or using the IF(WANT_SOUNDIO AND SOUNDIO_FOUND) logic.

[andrewrk]

Yes I think the CMake stuff is reasonably neat. Perhaps even more neat, if you take into account how simple FindSoundIo.cmake is. Anyway, looks like we have the go-ahead from all parties! 👍

[andrewrk]

For the record, we still need @tobydox to add libsoundio to his mingw ppa so Windows users can take advantage. Also I don't know how the builds are created for macos, does it use homebrew? In that case it would be nice for a volunteer to add libsoundio to homebrew (they discourage upstream maintainers from packaging their own software).

[tresf]

we still need @tobydox to add libsoundio to his mingw ppa so Windows users can take advantage.

Agreed.

Also I don't know how the builds are created for macos, does it use homebrew?

Yes, but very recently via #2271. We still support MacPorts as well. @ryandesign has been our contact there, but we've no active Brew contributor AFAIK.

[tobydox]

FYI: libsoundio packaged and available at both https://launchpad.net/~tobydox/+archive/ubuntu/mingw-x-trusty and https://launchpad.net/~tobydox/+archive/ubuntu/mingw-x-precise as "mingw32-x-libsoundio" and "mingw64-x-libsoundio".

[tresf]

@tobydox much obliged.

• The precise package is putting the DLL in lib instead of bin inside ${MINGW_PREFIX}.
• I've updated src/CMakeLists.txt to reflect this for now via dabfc63

Updated tutorials:

https://github.com/LMMS/lmms/wiki/Compiling-lmms-(Windows)

  mingw32-x-libgig mingw32-x-libsamplerate mingw32-x-pkgconfig \
- mingw32-x-binutils mingw32-x-gcc mingw32-x-runtime mingw32-x-libsoundio
+ mingw32-x-binutils mingw32-x-gcc mingw32-x-runtime
@@ -- @@
  mingw64-x-libgig mingw64-x-libsamplerate mingw64-x-pkgconfig \
- mingw64-x-binutils mingw64-x-gcc mingw64-x-runtime
+ mingw64-x-binutils mingw64-x-gcc mingw64-x-runtime mingw64-x-libsoundio

https://github.com/LMMS/lmms/wiki/Compiling-lmms

Optional, but strongly recommended (with devel-files each):
@@ -- @@
  * [libportaudio](http://www.portaudio.com/)
+ * [libsoundio](http://libsound.io/)

I couldn't find a libsoundio package for precise to add to the list, so this is UNCHANGED.

Installing all dependencies on Ubuntu 12.04 (or later) at once, run:
@@ -- @@
sudo apt-get install [...]
libxinerama-dev libxft-dev libgig-dev git
- FIXME: libsoundio-dev