Attempt to use Portaudio (v2) by npostavs · Pull Request #870 · jamulussoftware/jamulus

npostavs · 2021-01-20T02:45:04Z

Installer for Windows, as of 2021-06-17: https://github.com/jamulussoftware/jamulus/suites/3016078650/artifacts/68302467

Use the "Jamulus WASAPI" shortcut to test WASAPI mode.

For Linux (e.g., to try ALSA), you currently need to compile yourself with qmake CONFIG='portaudio portaudio_shared_lib' and have the relevant Portaudio dev package installed (it's called portaudio19-dev on Debian).

Start Jamulus with Jamulus --api ALSA to test ALSA.

Second round (first attempt was #821).

The current code successfully replaces the ASIO backend (as far as I can tell), except for channel selection. It's not entirely clear to me how the channel stuff works from the Jamulus side. It looks like Jamulus is treating certain channel number specially, e.g. // special case with 4 input channels: support adding channels in windows/sound.cpp and GetSelCHAndAddCH in src/soundbase.h?

corrados · 2021-01-21T07:04:47Z

Thanks, looks much better now. As soon as I have time, I'll review and test your code.

npostavs · 2021-01-21T14:02:26Z

The current code successfully replaces the ASIO backend (as far as I can tell), except for channel selection

I was missing the ASIO control panel, added now.

corrados · 2021-01-21T20:30:12Z

I'll do just some quick comments on your code. A more deep analysis of the code will take place later on.

Jamulus.pro

corrados · 2021-01-21T20:35:23Z

src/client.h

 #else
 # if defined ( _WIN32 ) && !defined ( JACK_REPLACES_ASIO )
-#  include "../windows/sound.h"
+//#  include "../windows/sound.h"


Here you disable the old Windows sound interface?

Yeah, I should have ifdef'd it though.

src/portaudio-sound.cpp

src/portaudio-sound.h

corrados · 2021-01-21T20:37:50Z

.gitmodules

 	url = https://github.com/vdellamea/jamulus-server-remote
+[submodule "libs/portaudio"]
+	path = libs/portaudio
+	url = https://github.com/npostavs/portaudio.git


You have referenced your fork. I would like to have to original repo referenced here.

The repo commit you have used is 3988fd. Is this an official release version of the portaudio library?

The repo commit you have used is 3988fd. Is this an official release version of the portaudio library?

No, I added a couple of commits to last stable release (hence why I used my fork instead of upstream):

Backport to let the wasapi code build

Modification to handle ASIO buffer size changes

Can you please create pull requests for your changes in the official repo?

The first is already from portaudio's master branch. I've just made a pull request for the second one: PortAudio/portaudio#473

I can see you've not had much progress over on the PortAudio PR. Had any luck finding out about the thread safety issues?

Had any luck finding out about the thread safety issues?

No. I guess the fact that Jamulus' current Windows code has ASIOMutex is suggestive, although it's not used for CSound::LoadAndInitializeDriver(!?)

OK, quick review of the threading model from memory - I've not looked at it closely on the client:

the client main thread starts (single thread)

if there's an audio driver, the audio thread should start (single thread)

the network thread starts (single thread)

if there's a GUI, the GUI thread should start (single thread)

I think that's it. I may be mistaken about the separate audio thread - it could be the main thread (which would otherwise only be co-ordinating network and audio and responding to and informing the GUI).

Where would multi-threading cause contention in audio driver processing in the client? I'd guess it can't before the driver is running, at least.

So if the device's buffer size gets changed, it sends a kAsioResetRequest message in the audio thread. Jamulus' handler calls SoundBase::EmitReinitRequestSignal which does emit ReinitRequest. That's connected to the CClient::OnSndCrdReinitRequest slot, which would run in the GUI thread?

pljones · 2021-01-22T18:26:30Z

Note that the CodeQL checks are failing with

Project ERROR: portaudio only tested on mingw for now

npostavs · 2021-01-24T23:42:31Z

Okay, I seem to have it building with msvc now. By the way, I can't figure out how to build the debug version from QtCreator:

18:36:05: Starting: "C:\Qt\Tools\QtCreator\bin\jom.exe" 
	C:\Qt\5.15.2\msvc2015_64\bin\qmake.exe -o Makefile ..\Jamulus.pro -spec win32-msvc "CONFIG+=debug" "CONFIG+=qml_debug"
	C:\Qt\Tools\QtCreator\bin\jom.exe -f Makefile.Release

Even though the debug configuration is selected as evidenced by the CONFIG+=debug, it insists on using -f Makefile.Release. At least with mingw I can use make debug from the command line.

corrados · 2021-01-25T17:37:02Z

src/portaudiosound.cpp

+    vecsAudioData.Init ( iPrefMonoBufferSize * 2 );
+    if ( deviceStream && deviceIndex >= 0 )
+    {
+        LoadAndInitializeDriver ( deviceIndex );


Calling this function is not supposed to be done in the Init function. LoadAndInitializeDriver should only be called if a new sound card device is selected. The Init function is called very often on any configuration change of the current sound card (like mono/stereo, buffer sizes, etc.).

I guess I could rename LoadAndInitializeDriver(int) (as ReloadCurrentDriver perhaps?) to make things clearer, but since the portaudio stream needs to be re-opened to apply a new buffer size, that call is necessary.

corrados · 2021-01-25T17:37:50Z

src/portaudiosound.cpp

+    if ( deviceIndex >= 0 )
+    {
+        long minBufferSize, maxBufferSize, prefBufferSize, granularity;
+        PaAsio_GetAvailableBufferSizes ( deviceIndex, &minBufferSize, &maxBufferSize, &prefBufferSize, &granularity );


Interesting. This looks very similar to the native ASIO interface function.

pljones · 2021-01-26T16:48:38Z

Even though the debug configuration is selected as evidenced by the CONFIG+=debug, it insists on using -f Makefile.Release. At least with mingw I can use make debug from the command line.

Hm. Not only that but the created EXEs are the same size, which indicate it is, actually, the same build. Manually overriding to Makefile.Debug gives me

16:46:51: Starting: "D:\Qt\Tools\QtCreator\bin\jom.exe" -f Makefile.Debug
Error: dependent 'D:\Qt\5.12.2\msvc2017_64-static\lib\Qt5Widgetsd.lib' does not exist.
16:46:51: The process "D:\Qt\Tools\QtCreator\bin\jom.exe" exited with code 2.

I'm guessing QtCreator has seen that the debug libraries were not installed and decided not to try to use them by default - so forcing it fails.

ann0see · 2021-02-01T15:37:58Z

Great! Not sure how we handle this, but I'm really happy if this works one day!

ann0see · 2021-02-01T20:33:36Z

@npostavs is it ready to be tested/does it work somehow?

pljones · 2021-02-01T21:50:02Z

Needs the conflict resolved
It's still draft

pljones · 2021-06-17T18:31:19Z

Windows PortAudio Installer requests:

be a different install from the standard installer
do offer to use the same locations, though

npostavs · 2021-06-17T18:34:03Z

Hmm, I've never seen that dll problem, and I can't see how it could be caused by my changes.

pljones · 2021-06-17T21:11:23Z

OK... just to clear that one up -- I had to uninstall the Visual C++ Runtime 2015-2019. Jamulus 3.8.0 then reinstalled it and that install is okay. Now, let's try this one again...

pljones · 2021-06-17T21:23:44Z

Hm. Same again. After the Windows Installer installs the runtime, it's no longer usable...

It's very weird. The VC++ runtime in both 3.8.0 and this is 14.29.30037. But the 3.8.0 installer makes it work and this installer breaks it.

However, if I keep Jamulus PortAudio installed and re-install 3.8.0 alongside it, both then work! 👯 🎆

(PortAudio is pickier about which drivers it will use - it only listed three out of the five ASIO drivers I have and threw multiple errors about one of the ones it excluded, whilst silently ignoring another.)

pljones · 2021-06-17T21:43:15Z

So, outstanding tasks:

See if anyone else can reproduce the problem I had above with VC++ Runtime 2015-2019
Rebase to latest jamulussoftware/jamulus master, squashing commits

Note to self: try installing other Windows installer artifacts, post-3.8.0.

npostavs · 2021-06-17T22:25:33Z

It's very weird. The VC++ runtime in both 3.8.0 and this is 14.29.30037. But the 3.8.0 installer makes it work and this installer breaks it.

Huh. Seems quite impossible. But the installer stuff is all black magic to me, I'm afraid.

Rebase to latest jamulussoftware/jamulus master, squashing commits

It is rebased to latest master, as far as I know. The non-squashing is intentional, but if you guys want to squash-merge the PR I'm fine with that.

windows/deploy_windows.ps1

This lets people who are unfamiliar with command line options try the other APIs out. Note that the shortcuts also use a separate .inifile, since the device names tend to be a bit different across APIs; and Jamulus doesn't handle missing devices very nicely.

And potentially on macOS too, but that is completely untested.

dcorson-ticino-com · 2021-06-18T16:30:59Z

A quick summary of the audio I get with this new version on Win10:
WASAPI
Using USB soundcard (NI Komplete Audio 2)
Exclusive mode, sound can be heard, but is covered by static and clicking with all buffer and audio selections.
Shared mode, noise and clicking with 64 sample buffer with and without Small network buffers.
128 and 256 sample buffers sound usable.
In all cases the jitter is very high, 11 or above. With ASIO normally I have jitter of 2 or 3. This means that the total delay is also very high, ping + 50ms or above.

The PC internal mic has a lot of static and is not usable with all buffering and audio possibilities.
The jitter is also 11 or more.

WDM
Using USB soundcard (NI Komplete Audio 2)
Seems to be working fine, jitter just slightly more than using ASIO.
All of the other sound sources and sinks make an error message as with the earlier version of port audio.

Things are moving in the right direction, but we still have a way to go.
Let me know if there is any way I can help with further testing.

pljones · 2021-06-18T18:02:56Z

It's very weird. The VC++ runtime in both 3.8.0 and this is 14.29.30037. But the 3.8.0 installer makes it work and this installer breaks it.

Huh. Seems quite impossible. But the installer stuff is all black magic to me, I'm afraid.

Well, I'm going to have to put it down to something local getting broken and now being fixed sort of fixed. But it seems to happen intermittently. I can reliably fix it by uninstalling the runtime before installing Jamulus... but it also seems to work now anyway. So I don't quite know what changed. Definitely local, though, as I used the same installer!!

pljones · 2021-06-18T18:14:52Z

src/soundbase.h


-    virtual void OpenDriverSetup() {}
+    virtual void           OpenDriverSetup() {}
+    virtual EPaApiSettings GetExtraSettings() { return PaApiNoExtraSettings; }


Why do this line and the next need to be here? src/soundbase.h shouldn't have driver or underlying API-specifics in.

pljones · 2021-06-18T18:15:50Z

src/soundbase.h

    None
 };

+enum EPaApiSettings


Not particularly happy with PortAudio code being in src/soundbase.h. Definitely not happy with WASAPI code being in here.

Why can't they be in src/portaudiosound.h?

pljones · 2021-06-18T18:20:02Z

src/main.cpp

+                             strMIDISetup,
+                             bNoAutoJackConnect,
+                             strClientName,
+#ifdef USE_PORTAUDIO


I'd drop the #else block and have the client not have the parameter except when using PortAudio, rather than have an empty string going nowhere.

pljones · 2021-06-18T18:23:30Z

src/clientsettingsdlgbase.ui

             </property>
            </spacer>
           </item>
+           <item>


Hm. This doesn't look right. I prefer JACK's approach of listing all the devices as "API: device driver name". Then the existing panel layout can remain and the command line argument isn't needed, making it easy to use.

This doesn't account for the shared vs exclusive of WASAPI mode though (which is what the code you are commenting is about)?

I also dispute that multiplying the number of device entries by (up to) 3 will make things easier. Having n^2 entries due to the Cartesian product of input and output devices is already bad enough.

Why can't they be returned as a single driver's list of inputs and outputs, anyway, rather than having to be returned each as a separate driver as they are now?

I don't understand what you mean.

OK, go about it more list ASIO - but, rather than separating out each hardware device, have the PortAudio API be the device driver. This way you could choose the API without having to have --api.

Then you have Input 1, Input 2, Output 1 and Output 2 boxes. For the chosen API, you'd return all the inputs to populate the input boxes and all the outputs to populate the output boxes.

Benefits:

No cartesian product.

Similar to the existing user experience.

No command line option required.

Negative points:

Might not work for PA-ASIO?

Oh, I see. I guess that works as long as the devices themselves don't have multiple channels, I think this is usually the case for non-ASIO devices. I'm wouldn't be too happy about special-casing the code like that. Also it still doesn't solve the shared vs exclusive WASAPI mode thing.

Similar to the existing user experience.

The Cartesian product thing is the existing user experience on macOS, by the way.

I'm explicitly referring to ASIO here as the primary target for this change is Windows ASIO4ALL users.

pljones · 2021-06-18T18:23:55Z

src/clientsettingsdlg.h

    void OnNewClientLevelEditingFinished() { pSettings->iNewClientFaderLevel = edtNewClientLevel->text().toInt(); }
    void OnInputBoostChanged();
    void OnSndCrdBufferDelayButtonGroupClicked ( QAbstractButton* button );
+    void OnWasapiModeButtonGroupClicked ( QAbstractButton* button );


As elsewhere, not happy with this.

pljones · 2021-06-18T18:24:25Z

src/clientsettingsdlg.cpp

    chbDetectFeedback->setWhatsThis ( strDetectFeedback );
    chbDetectFeedback->setAccessibleName ( tr ( "Feedback Protection check box" ) );

+    if ( pNCliP->GetExtraSettings() != PaApiWasapiModes )


There shouldn't be hardware-specific code in here.

pljones · 2021-06-18T18:25:13Z

src/client.cpp

                   const QString& strMIDISetup,
                   const bool     bNoAutoJackConnect,
                   const QString& strNClientName,
+                   const QString& strApiName,


#ifdef around this.

pljones · 2021-06-18T18:26:01Z

src/client.h

              const QString& strMIDISetup,
              const bool     bNoAutoJackConnect,
              const QString& strNClientName,
+              const QString& strApiName,


#ifdef around this.

pljones · 2021-06-18T18:26:35Z

src/client.h

    int     GetSndCrdLeftOutputChannel() { return Sound.GetLeftOutputChannel(); }
    int     GetSndCrdRightOutputChannel() { return Sound.GetRightOutputChannel(); }

+    EPaApiSettings GetExtraSettings() { return Sound.GetExtraSettings(); }


As elsewhere, not happy seeing hardware-specific code here.

pljones · 2021-06-19T07:40:23Z

OK, so now I've tried using it without the VC++ runtime issue.

The PA-ASIO is annoying. For a driver it doesn't like, it doesn't simply ignore it, it issues repetitive errors. And it does it on every change to anything driver-related, of course.

I also found it thought one of my drivers was in use when I selected it, whereas the standard Jamulus ASIO code had no problem.

The WDM-KS mode didn't work - it listed the devices, let me select my sound card in/out pair but had no sound in or out. (That's the device the PA-ASIO code thought was in use.)

The WASAPI mode didn't work - it didn't list any devices.

(One way around or the other for WDM-KS / WASAPI - I forgot to note which was which.)

This was with jamulus_portaudio_3.8.0dev-7d7e464_win.exe from the autobuild.

ann0see · 2021-06-19T21:16:47Z

I also had the crackling sound. I think it’s similar to the sound I hear if I choose pull or push (?) mode in ASIO4ALL.

ann0see · 2021-09-05T15:18:13Z

@npostavs I think this should go to a feature branch (not master) then? What do you think?

npostavs · 2021-09-13T18:40:33Z

Yeah, I'm not feeling very optimistic about completing this right now. It might make more sense to work out the kinks with PortAudio using KoordASIO (#1936) and then maybe revisit this more integrated approach later.

ann0see · 2021-09-13T19:23:04Z

Your work is now here: https://github.com/jamulussoftware/jamulus/tree/feature/np/portaudio
At least it's somehow official now ;-)

npostavs mentioned this pull request Jan 20, 2021

Attempt to use Portaudio (v1) #821

Closed