-
Notifications
You must be signed in to change notification settings - Fork 50
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
Add PulseAudio support #194
Conversation
@drowe67, this is nowhere near done but due to the significant code changes involved, it's worth testing here before proceeding with the actual PulseAudio implementation. Also, please feel free to add anything to the test plan that I missed. |
I have built and installed Fedora 35 rpms for freedv using this code and as far as I can tell it works essentially the same as before, the audio config and PTT setup dialogs work normally, I can receive and transmit (not found an on-air freedv signal but I don't see it would make any difference) so I'd say so far, so good. I see Qt Pulseaudio entries in the audio dialogs now, but so far have not tried selecting them (assume they are stubs at present), I normally use JACK at 48kbps which interfaces with pipewire through the JACK compatibility libraries. |
@tmiw wow this is great 🥇 ! I'm taking a look at the code now. I'm a bit busy (my) today, but will also run it over the next few days. |
delete m_rxOutPa; | ||
m_RxRunning = false; | ||
Pa_Terminate(); | ||
|
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 nice to clean all this up 👍 I really was "feeling my way" when I wrote this (about 10 years ago now).
src/main.cpp
Outdated
{ | ||
// RX-only setup. | ||
// Note: we assume 2 channels, but IAudioEngine will automatically downgrade to 1 channel if needed. | ||
rxInSoundDevice = engine->getAudioDevice(std::string(wxGetApp().m_soundCard1InDeviceName.ToUTF8()), IAudioEngine::IN, g_soundCard1SampleRate, 2); |
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, that's a neat trick!
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.
It's something handled on the AudioEngine side, so there'll need to be similar logic internally for both PulseAudio and PortAudio. I'm not sure if there's a way to not have to duplicate such logic, but at least it'll be contained to AudioEngine and its classes.
CallAfter([&]() { | ||
wxMessageBox(wxString::Format("Error encountered while processing audio: %s", error), wxT("Error"), wxOK); | ||
}); | ||
}; |
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 not across this C++ syntax to understand this fully - but is this popping up a Dialog in the thread/context of a callback? If so, I'm wondering it that's OK? It would be a good idea to exercise any code that generates a MessageBox.
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.
Yep, CallAfter() is provided by wxWidgets to ensure that all GUI related actions happen on the GUI thread. Bad things happen on the wxWidgets side otherwise.
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.
BTW the [&] ... syntax is a C++11 lambda function.
Interesting, maybe Fedora 35 somehow has a version of PortAudio that has the proposed Pulse support? IIRC that PR still hasn't been merged up to their master. |
src/main.cpp
Outdated
std::unique_lock<std::mutex> lk(cbData->infifo1Mutex); | ||
if (codec2_fifo_write(cbData->infifo1, indata, size)) | ||
{ | ||
g_infifo1_full++; |
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.
Wow if I understand this - the callbacks are now declared inline. That's pretty compact. Just a minor style comment, if you use this style:
if () {
}
about 30% of the lines would be removed from the source, making it more compact to read. Having said that, style is not a huge issue for me. Your call 🙂
For simple for loops I'm also leaning towards:
for (i=0; i<10; i++) x[i] = i++;
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.
Yep, they're inline. By virtue of there being one for each device (vs. how txCallback and rxCallback were shared across all four), it also makes it easier to understand what's going on. 👍
Nice work @tmiw, the interface is nice and neat 👍 Is the audio engine choice set in the persistent config? Or is it hard coded say according to operating system? I'm wondering if I can select PortAudio or Pulse if I'm running Linux (ie two audio engines available). Sorry if I missed that in the source somewhere. |
The intention is for that to be something that happens at compile time (e.g. cmake -DUSE_PULSEAUDIO). It probably could be something that's selectable at runtime but you'd likely need to restart the FreeDV process entirely for it to take effect. |
That's fine as a first pass and first PR. If its necessary or desirable to have more than one AE per OS, we can add that feature later. It would be far simpler to standardise on one AE per OS for support purposes and simplicity. |
On looking a bit further it shows the QtPulseAudio devices as being supported via the JACK API so it looks like PortAudio is not providing a Pulseaudio interface. It should be easy to see what is supported in the PA changelog in the relevant place on koji. The PA code used in Fedora 35 packages (unchanged since July) is this version pa_stable_v190700_20210406 from April. The only devices freedv now offers are either via ALSA or JACK. That's all I know. |
I have now rebuilt again with the code from late today and it all seems just fine with no obvious changes from the removal of mutexes etc. |
Do you hear the same thing @tmiw I don't see why this would only affect me. |
Finally found some freedv at good signal levels on 80m this afternoon, I can now confirm that there is still some clicking in the audio with the current git version after the last revert. I don't know how to identify it any better but I can hear it even listening to analogue noise, the freedv signals are at about 7-10dB SNR with BERs of 0.7% and below. Of course these are off-air signals with 2 stations using different radios and unknown freedv versions and audio interfaces. They have different IF filtering set up too from what I hear, one is using a Flex SDR. Plenty of fading across the signal bandwidth, typically 2 notches across the 700E modulation spectrum from 750-2250Hz. 700D might be better, but the fades sometimes use up all the SNR improvement when I hear 700D on air. One station is showing a fairly large and variable clock offset, the other is much more stable. Maybe too little PC horsepower. This is usable, but there is definitely something where extra errors are getting into the audio stream for no obvious reason. |
Are you hearing this when playing the sample .wav files over the RX sound device as well? (e.g. |
@Tyrbiter, I played with my other VM environment tonight (Parallels, M1 Mac) and I think I was able to duplicate what you were experiencing. I refactored the PulseAudio logic and it seems to be better, plus I can use much lower latencies than previously. Let me know if that works better for you. 👍 (If not, I also added some additional debugging output on the console that may help.) |
Hi @tmiw Yes this new version does seem much better, I will see if I can find the normal freedv net on 80m on air later today but the analogue output noise and the recorded wav files all sound good to me. I wonder if it is worth including some sort of dynamic buffer usage display within freedv, not sure how difficult that would be. |
An update, just listened to the daily 80m net and the new code in git has performed very much better, good audio, no obvious artefacts of bit or burst errors other than those provided by the RF channel. Here is a screenshot of the waterfall during an over from the stronger of the 2 stations active today: It looks interesting, you can see the fading moving cyclically through the audio spectrum. SNR was about 5-8dB. If this performance is common to other platforms and audio sub-systems I would be happy to see this go for wider testing to confirm that on varied hardware. |
Hmm, I'm not sure we'd want anything API specific in the GUI. Ideally the behavior of each subsystem would be transparent. However, perhaps buffer usage (along with targets as of now) can be included in the console output? If good, that can easily be disabled again before merging.
Excellent! |
Really just some warnings if things are getting a bit near the knuckle from freedv's point of view. And only there if the debug options are enabled. That's all I think is needed so people can tell if their system is up to the processing load if they hear odd behaviour. Maybe the existing debug does that, but it can be a bit difficult to interpret. |
Oh yes, and CPU usage seems to be significantly lower too, don't have exact numbers but I can see numbers as low as 3% whereas in the past it's been pretty much 10% minimum. |
Sounds like good progress, well done 👍 |
@tmiw - could you please remind me what the 1.7.0 test build was demonstrating/tetsing? For example:
|
Yeah, most people have been using Windows or otherwise another OS with PortAudio. I've been using the PulseAudio version occasionally on the air and it seems okay, and @Tyrbiter above was able to tune into the UK 80 meter net with it too. I listed PulseAudio as optional in the release notes (as well as defaulting to PortAudio in build_linux.sh) and we can revisit making PulseAudio the default in a future PR after we've gotten some more runtime. |
Just tried the latest git, and I get this if I select the filter dialog: Thread 1 "freedv" received signal SIGSEGV, Segmentation fault. More detail follows: (gdb) bt full
#23 g_signal_emit_valist (instance=0x5555559a65a0, signal_id=, detail=0, var_args=var_args@entry=0x7fffffffcf80) at ../gobject/gsignal.c:3406
#24 0x00007ffff472ec03 in g_signal_emit (instance=instance@entry=0x5555559a65a0, signal_id=, detail=detail@entry=0) at ../gobject/gsignal.c:3553 Let me know if more is needed. |
@Tyrbiter, try again? Seems to have been a merge issue. |
Yes I've taken a few looks at the code over the life of the PR and commented along the way. So I'm happy if you are 🙂 |
This PR is to eventually support PulseAudio in FreeDV. Once merged, this will resolve #16 and #191.
TODO:
Create library-independent audio subsystemDone, see IAudioEngine.h et alUse IAudioEngine for Audio Options dialogDoneUse IAudioEngine for main encode/decode pipelineDoneRemove all PortAudio specific UI elements.DoneWrite PulseAudioEngine classes and compile into codebase.DoneUpdate Create "Easy Setup" dialog to simplify initial setup. #189 to use IAudioEngine.PR on hold, we can proceed without it for nowUpdate user manual and other documentation.Done, see 47d5b60Stereo results in incorrect speed .wav playback for PulseAudioIncorrect plots when using test buttons in Audio Options (PulseAudio)Done, see d584579Noise immediately on playback/record start (PulseAudio and PortAudio)Done, see d584579May be a local/VMware issue, will need others to confirmPartially resolved by d14f6a6Possible full resolution by ff88cf1 and 9d24a41Device detection issues with some Windows machinesDoneTest Plan: