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

AudioIO uses private PaUtil_GetTime API of PortAudio #871

Closed
Be-ing opened this issue May 11, 2021 · 8 comments · Fixed by #1431
Closed

AudioIO uses private PaUtil_GetTime API of PortAudio #871

Be-ing opened this issue May 11, 2021 · 8 comments · Fixed by #1431
Labels
community build bugs Bug in non-official builds, such as distro-repos or self-compiled ones

Comments

@Be-ing
Copy link

Be-ing commented May 11, 2021

PaUtil_GetTime is defined in src/common/pa_util.h in PortAudio. This header is not installed by the build system as confirmed by checking the files installed by Fedora's portaudio-devel package:

❯ rpm -ql portaudio-devel
/usr/include/pa_jack.h
/usr/include/pa_linux_alsa.h
/usr/include/pa_unix_oss.h
/usr/include/portaudio.h
/usr/include/portaudiocpp
/usr/include/portaudiocpp/AutoSystem.hxx
/usr/include/portaudiocpp/BlockingStream.hxx
/usr/include/portaudiocpp/CFunCallbackStream.hxx
/usr/include/portaudiocpp/CallbackInterface.hxx
/usr/include/portaudiocpp/CallbackStream.hxx
/usr/include/portaudiocpp/CppFunCallbackStream.hxx
/usr/include/portaudiocpp/Device.hxx
/usr/include/portaudiocpp/DirectionSpecificStreamParameters.hxx
/usr/include/portaudiocpp/Exception.hxx
/usr/include/portaudiocpp/HostApi.hxx
/usr/include/portaudiocpp/InterfaceCallbackStream.hxx
/usr/include/portaudiocpp/MemFunCallbackStream.hxx
/usr/include/portaudiocpp/PortAudioCpp.hxx
/usr/include/portaudiocpp/SampleDataFormat.hxx
/usr/include/portaudiocpp/Stream.hxx
/usr/include/portaudiocpp/StreamParameters.hxx
/usr/include/portaudiocpp/System.hxx
/usr/include/portaudiocpp/SystemDeviceIterator.hxx
/usr/include/portaudiocpp/SystemHostApiIterator.hxx
/usr/lib64/libportaudio.so
/usr/lib64/libportaudiocpp.so
/usr/lib64/pkgconfig/portaudio-2.0.pc
/usr/lib64/pkgconfig/portaudiocpp.pc

pa_util.h is not listed in the PortAudio documentation either. Audacity is using a private function of PortAudio. This prevents linking Audacity to packages of PortAudio.

The code that uses PaUtil_GetTime is guarded by an #ifdef:

#ifdef EXPERIMENTAL_MIDI_OUT
// return the system time as a double
static double streamStartTime = 0; // bias system time to small number

static double SystemTime(bool usingAlsa)
{
#ifdef __WXGTK__
   if (usingAlsa) {
      struct timespec now;
      // CLOCK_MONOTONIC_RAW is unaffected by NTP or adj-time
      clock_gettime(CLOCK_MONOTONIC_RAW, &now);
      //return now.tv_sec + now.tv_nsec * 0.000000001;
      return (now.tv_sec + now.tv_nsec * 0.000000001) - streamStartTime;
   }
#else
   static_cast<void>(usingAlsa);//compiler food.
#endif

   return PaUtil_GetTime() - streamStartTime;
}
#endif

So I tried building with the -Daudacity_use_portaudio=system -DUSE_PORTMIXER=OFF CMake options and the patch below to use the system PortAudio without Audacity's PortMixer fork which requires Audacity's PortAudio fork to build and without MIDI support:

diff --git a/cmake-proxies/CMakeLists.txt b/cmake-proxies/CMakeLists.txt
index 35250773c..c3ffc4710 100644
--- a/cmake-proxies/CMakeLists.txt
+++ b/cmake-proxies/CMakeLists.txt
@@ -13,7 +13,7 @@ addlib( expat              expat       EXPAT       YES   YES   "expat >= 2.1.0"
 addlib( lame               lame        LAME        YES   YES   "lame >= 3.100" )
 addlib( libsndfile         sndfile     SNDFILE     YES   YES   "sndfile >= 1.0.28" )
 addlib( libsoxr            soxr        SOXR        YES   YES   "soxr >= 0.1.1" )
-addlib( portaudio-v19      portaudio   PORTAUDIO   YES   YES   "" )
+addlib( portaudio-v19      portaudio   PORTAUDIO   YES   YES   "portaudio-2.0" )
 addlib( sqlite             sqlite      SQLITE      YES   YES   "sqlite3 >= 3.32.0" )
 
 # Optional libraries
@@ -36,7 +36,7 @@ if (NOT USE_MIDI AND
 endif ()
 
 
-addlib( portmixer          portmixer   PORTMIXER   NO    YES   "" )
+#addlib( portmixer          portmixer   PORTMIXER   NO    YES   "" )
 if (NOT USE_PORTMIXER AND
    "EXPERIMENTAL_AUTOMATED_INPUT_LEVEL_ADJUSTMENT" IN_LIST
       EXPERIMENTAL_OPTIONS_LIST )
diff --git a/src/Experimental.cmake b/src/Experimental.cmake
index 30c01792a..ad0cc9374 100644
--- a/src/Experimental.cmake
+++ b/src/Experimental.cmake
@@ -121,7 +121,7 @@ set( EXPERIMENTAL_OPTIONS_LIST
    # RBD, 1 Sep 2008
    # Enables MIDI Output of NoteTrack (MIDI) data during playback
    # USE_MIDI must be defined in order for MIDI_OUT to work
-   MIDI_OUT
+   #MIDI_OUT
 
    # JKC, 17 Aug 2017
    # Enables the MIDI note stretching feature, which currently

but this fails to build:

../src/AudioIO.cpp: In member function ‘virtual void AudioIO::StopStream()’:
../src/AudioIO.cpp:2452:4: error: ‘mMidiPlaybackTracks’ was not declared in this scope; did you mean ‘mPlaybackTracks’?
 2452 |    mMidiPlaybackTracks.clear();
      |    ^~~~~~~~~~~~~~~~~~~
      |    mPlaybackTracks
@Be-ing
Copy link
Author

Be-ing commented May 11, 2021

Debian hacks around this with a really ugly hack to just implement the PaUtil_GetTime function directly in src/AudioIO.cpp.

@crsib
Copy link
Contributor

crsib commented May 11, 2021

This fix will be easy, but I feel like that #853 should go first. The reason is - stuff like https://github.com/audacity/audacity/blob/master/src/AudioIO.cpp#L477 looks a bit spooky when you build against system library :)

@Be-ing
Copy link
Author

Be-ing commented May 11, 2021

Copying over a comment from @rbdannenberg:

Pa_GetStreamTime and PaUtil_GetTime are different clocks running at
(slightly) different rates. If you replaced one with the other, then you
either created or fixed a problem.

I for one will not bother figuring out whether that creates or fixes a problem.

@Paul-Licameli
Copy link
Collaborator

My dear Be-ing, accept my apologies for the inconveniences caused you by the state of the source tree.

Please see the recently pushed c3babc1 which makes it possible once again to compile with EXPERIMENTAL_MIDI_OUT undefined.

Let me explain that in the summer of 2017, for version 2.2.0, @Pokechu22 (a friend of Audacity then and now), Roger Dannenberg (the co-founder of Audacity and a portaudio contributor), Steve Daulton, I, and others worked hard together to make MIDI playback work cross-platform. Getting the timing to work right on all platforms was difficult, especially in the case of Linux with ALSA. It was Roger's hard work especially that led to the present expedient after much trial and error.

The EXPERIMENTAL_MIDI_OUT branch was at last made good enough to be turned on in production code by default. But unfortunately, we were then negligent in ensuring the continued possibility to build the program with the same turned off.

Doing so will now build and give you a version of Audacity without its MIDI playback capability. But perhaps you and your users would find that restriction of functionality unacceptable.

If so, then propose a fix that preserves well synchronized MIDI playback on all platforms, as verified by testing.

@Be-ing
Copy link
Author

Be-ing commented May 12, 2021

Thanks for fixing the build without the EXPERIMENTAL_MIDI_OUT option and providing that context. I don't think that building without MIDI playback would be a good compromise. However, if this issue is solved, I think it would be a good compromise for Linux distribution packages to disable building with PortMixer and link to their system's PortAudio package.

#210 looks like it may be a viable solution if it is rebased and updated for CMake.

I am doubtful anyone would have seriously considered the present implementation using a library's private API in the first place if PortAudio was not vendored.

@Pokechu22
Copy link
Contributor

The current implementation was used without knowledge that it didn't work on all platforms; if I recall correctly, what I did was resurrect a lot of very old code that already existed in Audacity, which had some design decisions that probably wouldn't have been made if written from scratch.

I can't recall if there were timing/synchronization issues with Pa_GetStreamTime versus PaUtil_GetTime; I think there might have been but I don't remember doing extensive testing. It would have been mentioned on the mailing list (but I don't have time to dig through that right now).

It's also worth noting that the reason audacity does the patch to PortAudio is because pa_util.h has extern "C", but the actual implementations in pa_win_util.c and pa_unix_util.c do not have extern "C". IMO that's a bug in PortAudio, and I think that linking with the system PortAudio would work if it were fixed (even if it is an internal API). (This has almost certainly been discussed on the list before, but I don't remember the details and it's been a long time.)

@Be-ing
Copy link
Author

Be-ing commented May 12, 2021

IMO that's a bug in PortAudio, and I think that linking with the system PortAudio would work if it were fixed (even if it is an internal API).

I don't think it's a bug in PortAudio. Audacity is using a private API. There is no way to use this API when PortAudio is used normally; the header for the symbol is not installed and Audacity fails to link.

@imciner2
Copy link
Contributor

It's also worth noting that the reason audacity does the patch to PortAudio is because pa_util.h has extern "C", but the actual implementations in pa_win_util.c and pa_unix_util.c do not have extern "C". IMO that's a bug in PortAudio,

You shouldn't need to put the extern on the function definition and the declaration. Putting it on the declaration in the header and then having that included in the file containing the definitions before the definitions should make the functions all have C linkage on every standards-compliant compiler.

and I think that linking with the system PortAudio would work if it were fixed (even if it is an internal API). (This has almost certainly been discussed on the list before, but I don't remember the details and it's been a long time.)

No, I don't think it would be. PortAudio tells the linker exactly what it wants exposed through a .def file on Windows, https://github.com/PortAudio/portaudio/blob/master/cmake_support/template_portaudio.def, so only those functions are considered its public API and will be implicitly linked to. They do the same thing when building with autotools and specify the public linkage here: https://github.com/PortAudio/portaudio/blob/master/Makefile.in#L47.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community build bugs Bug in non-official builds, such as distro-repos or self-compiled ones
Projects
None yet
6 participants