-
Notifications
You must be signed in to change notification settings - Fork 225
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
Attempt to use Portaudio (v2) #870
Conversation
Thanks, looks much better now. As soon as I have time, I'll review and test your code. |
I was missing the ASIO control panel, added now. |
I'll do just some quick comments on your code. A more deep analysis of the code will take place later on. |
src/client.h
Outdated
@@ -44,7 +44,9 @@ | |||
# include "vstsound.h" | |||
#else | |||
# if defined ( _WIN32 ) && !defined ( JACK_REPLACES_ASIO ) | |||
# include "../windows/sound.h" | |||
//# include "../windows/sound.h" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here you disable the old Windows sound interface?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I should have ifdef'd it though.
.gitmodules
Outdated
@@ -22,3 +22,6 @@ | |||
[submodule "tools/jamulus-server-remote"] | |||
path = tools/jamulus-server-remote | |||
url = https://github.com/vdellamea/jamulus-server-remote | |||
[submodule "libs/portaudio"] | |||
path = libs/portaudio | |||
url = https://github.com/npostavs/portaudio.git |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You have referenced your fork. I would like to have to original repo referenced here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The repo commit you have used is 3988fd. Is this an official release version of the portaudio library?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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):
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please create pull requests for your changes in the official repo?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The first is already from portaudio's master branch. I've just made a pull request for the second one: PortAudio/portaudio#473
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can see you've not had much progress over on the PortAudio PR. Had any luck finding out about the thread safety issues?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
(!?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
Note that the CodeQL checks are failing with
|
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:
Even though the debug configuration is selected as evidenced by the |
src/portaudiosound.cpp
Outdated
vecsAudioData.Init ( iPrefMonoBufferSize * 2 ); | ||
if ( deviceStream && deviceIndex >= 0 ) | ||
{ | ||
LoadAndInitializeDriver ( deviceIndex ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
src/portaudiosound.cpp
Outdated
if ( deviceIndex >= 0 ) | ||
{ | ||
long minBufferSize, maxBufferSize, prefBufferSize, granularity; | ||
PaAsio_GetAvailableBufferSizes ( deviceIndex, &minBufferSize, &maxBufferSize, &prefBufferSize, &granularity ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting. This looks very similar to the native ASIO interface function.
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
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. |
a702953
to
32c5f76
Compare
This comment has been minimized.
This comment has been minimized.
Great! Not sure how we handle this, but I'm really happy if this works one day! |
@npostavs is it ready to be tested/does it work somehow? |
This comment has been minimized.
This comment has been minimized.
|
b156696
to
88af567
Compare
This comment has been minimized.
This comment has been minimized.
0883a5b
to
6cb3da0
Compare
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... |
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.) |
So, outstanding tasks:
Note to self: try installing other Windows installer artifacts, post-3.8.0. |
Huh. Seems quite impossible. But the installer stuff is all black magic to me, I'm afraid.
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. |
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.
ff268ce
to
8113c4f
Compare
A quick summary of the audio I get with this new version on Win10: The PC internal mic has a lot of static and is not usable with all buffering and audio possibilities. WDM Things are moving in the right direction, but we still have a way to go. |
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!! |
@@ -103,7 +117,9 @@ class CSoundBase : public QThread | |||
|
|||
virtual float GetInOutLatencyMs() { return 0.0f; } // "0.0" means no latency is available | |||
|
|||
virtual void OpenDriverSetup() {} | |||
virtual void OpenDriverSetup() {} | |||
virtual EPaApiSettings GetExtraSettings() { return PaApiNoExtraSettings; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do this line and the next need to be here? src/soundbase.h
shouldn't have driver or underlying API-specifics in.
@@ -51,6 +51,20 @@ enum EMidiCtlType | |||
None | |||
}; | |||
|
|||
enum EPaApiSettings |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
?
strMIDISetup, | ||
bNoAutoJackConnect, | ||
strClientName, | ||
#ifdef USE_PORTAUDIO |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -358,6 +358,31 @@ | |||
</property> | |||
</spacer> | |||
</item> | |||
<item> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand what you mean.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm explicitly referring to ASIO here as the primary target for this change is Windows ASIO4ALL users.
@@ -87,6 +88,7 @@ public slots: | |||
void OnNewClientLevelEditingFinished() { pSettings->iNewClientFaderLevel = edtNewClientLevel->text().toInt(); } | |||
void OnInputBoostChanged(); | |||
void OnSndCrdBufferDelayButtonGroupClicked ( QAbstractButton* button ); | |||
void OnWasapiModeButtonGroupClicked ( QAbstractButton* button ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As elsewhere, not happy with this.
@@ -367,13 +367,33 @@ CClientSettingsDlg::CClientSettingsDlg ( CClient* pNCliP, CClientSettings* pNSet | |||
chbDetectFeedback->setWhatsThis ( strDetectFeedback ); | |||
chbDetectFeedback->setAccessibleName ( tr ( "Feedback Protection check box" ) ); | |||
|
|||
if ( pNCliP->GetExtraSettings() != PaApiWasapiModes ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There shouldn't be hardware-specific code in here.
@@ -31,6 +31,7 @@ CClient::CClient ( const quint16 iPortNumber, | |||
const QString& strMIDISetup, | |||
const bool bNoAutoJackConnect, | |||
const QString& strNClientName, | |||
const QString& strApiName, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#ifdef
around this.
@@ -113,6 +107,7 @@ class CClient : public QObject | |||
const QString& strMIDISetup, | |||
const bool bNoAutoJackConnect, | |||
const QString& strNClientName, | |||
const QString& strApiName, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#ifdef
around this.
@@ -194,6 +189,9 @@ class CClient : public QObject | |||
int GetSndCrdLeftOutputChannel() { return Sound.GetLeftOutputChannel(); } | |||
int GetSndCrdRightOutputChannel() { return Sound.GetRightOutputChannel(); } | |||
|
|||
EPaApiSettings GetExtraSettings() { return Sound.GetExtraSettings(); } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As elsewhere, not happy seeing hardware-specific code here.
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 |
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. |
@npostavs I think this should go to a feature branch (not master) then? What do you think? |
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. |
Your work is now here: https://github.com/jamulussoftware/jamulus/tree/feature/np/portaudio |
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 calledportaudio19-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
inwindows/sound.cpp
andGetSelCHAndAddCH
insrc/soundbase.h
?