-
Notifications
You must be signed in to change notification settings - Fork 15
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
High CPU usage of pipewire-pulse with ladspa_dsp once playback is stopped #36
Comments
I don't think Aqualung should be taking any significant CPU when paused. Certainly it doesn't in other environments I've used (granted, mostly the sndio output, and no ladspa_dsp). Does the problem go away when you remove the ladspa_dsp? That would help diagnose the issue. |
There is no issue if ladspa_dsp is not used. |
Thank you for checking. I'm not sure if this is something caused by Aqualung or could be avoided by changes to Aqualung, but I'm going to leave it open in case someone else has ideas. I don't have much experience with the LADSPA code or the pulseaudio support. @tomscii, do you have any ideas regarding this? |
@cepamoi Why are you using ladspa_dsp instead of Aqualung's built-in support for applying LADSPA plugins to the audio playback? I normally do not use it, but tested it just now - works as expected. If I understand correctly, ladspa_dsp is a separate LADSPA effects processor that exists outside of Aqualung (or any other player). It has nothing to do with our LADSPA code. I have never heard of it before today (and no, I have not tried it - your link does not work and it is not clear what your setup is other than including Aqualung and ladspa_dsp). I'm unfortunately also not an expert on pulseaudio/pipewire/whatever it's called these days... but for me Aqualung plays back music (even via pulse) without any issue (CPU usage or otherwise). So I would just drop ladspa_dsp from the setup (or you could debug the issue given you have the time and knowledge to do so). And/or, given the comparative maturity of Aqualung/ladspa_dsp I suggest raising this with ladspa_dsp people. |
Sorry for the wrong link. Here is the corrected one: As the issue only affects Aqualung and not the other audio players, I'm afraid that it is an Aqualung issue. |
I used the pw-top command which gives some information and statistics about the pipewire process. Now is the output of pw-top when the playback is stopped in Aqualung: To compare, here is the output of pw-top when the playback is stopped in Audacious: So maybe something is missing in Aqualung when stopping the playback with Pulseaudio/Pipewire. |
From some googling, Audacious natively supports Pipewire, but Aqualung does not. Aqualung supports Pulseaudio, and then you are using pipewire-pulse to support Pipewire. Perhaps the problem is in pipewire-pulse and not in Aqualung? You could see if you could reproduce the problem using Pulseaudio directly instead of Pipewire. |
Seconding @jeremyevans here, please try to run Aqualung with Another random thought: I see that your output rate is 48k while Aqualung is doing 44.1k, maybe there is something to do with resampling? Try setting Aqualung to output 48k (thus doing resampling internally) so pipe/pulse/* don't have to. |
Maybe, but I don't think so. Only thing I can think of that Aqualung perhaps does differently is that it does not close the audio device when playback is stopped (or paused) - it merely stops pushing data into the audio sink. I would definitely file this as a bug against the program that actually consumes lots of CPU. |
Running Aqualung with -o pulse does not change anything to the issue. I checked the source code of Audacious and VLC. Both use the asynchronous API of PulseAudio, while Aqualung uses the simple API of PulseAudio. In Aqualung, there is no equivalent call to some function of the simple API of PulseAudio when the palyback is stopped.
According to the documentation of the simple API of PulseAudio:
So when the playback is stopped or is complete, Aqualung should probably call pa_simple_free(), and then call pa_simple_new() again when the playback is restarted. |
I don't foresee any issues with that approach, so if you want to send in a pull request for it, I think it could be accepted. |
No, it won't be a problem to change the current implementation the way you describe. So go on and change it! |
Attached is a first tentative. |
If you don't want to provide a pull request, could you post the output of |
Sorry, I'm not very familiar with pull requests and GitHub. diff --git a/src/core.c b/src/core.c
index b18b0d9..772d228 100644
--- a/src/core.c
+++ b/src/core.c
@@ -836,7 +836,6 @@ pulse_thread(void * arg) {
int ret, err;
char recv_cmd;
- pa_simple* pa = info->pa;
short * pa_short_buf = NULL;
if ((info->pa_short_buf = malloc(2*bufsize * sizeof(short))) == NULL) {
@@ -869,6 +868,17 @@ pulse_thread(void * arg) {
rb_read(rb, (char *)pa_short_buf, n_avail);
}
rb_write(rb_out2disk, (char *)&driver_offset, sizeof(guint32));
+ if (info->is_streaming == 0 && info->pa) {
+ /* when the playback is paused or stopped */
+ if (pa_simple_flush(info->pa, &err) < 0) {
+ fprintf(stderr, "pulse_thread: pa_simple_flush() failed: %s\n", pa_strerror(err));
+ }
+ if (pa_simple_drain(info->pa, &err) < 0) {
+ fprintf(stderr, "pulse_thread: pa_simple_drain() failed: %s\n", pa_strerror(err));
+ }
+ pa_simple_free(info->pa);
+ info->pa = 0;
+ }
goto pulse_wake;
break;
case CMD_FINISH:
@@ -881,6 +891,11 @@ pulse_thread(void * arg) {
}
if ((n_avail = rb_read_space(rb) / (2*sample_size)) == 0) {
+ if (info->is_streaming == 0 && info->pa) {
+ /* when the playback ends */
+ pa_simple_free(info->pa);
+ info->pa = 0;
+ }
g_usleep(100000);
goto pulse_wake;
}
@@ -903,7 +918,15 @@ pulse_thread(void * arg) {
}
/* write data to audio device */
- ret = pa_simple_write(pa, pa_short_buf, 2*n_avail * sizeof(short), &err);
+ if (!info->pa) {
+ info->pa = pa_simple_new(NULL, "Aqualung", PA_STREAM_PLAYBACK, NULL,
+ "Music", &info->pa_spec, NULL, NULL, &err);
+ if (!info->pa) {
+ fprintf(stderr, "Unable to initilize PulseAudio: %s\n", pa_strerror(err));
+ return err;
+ }
+ }
+ ret = pa_simple_write(info->pa, pa_short_buf, 2*n_avail * sizeof(short), &err);
if (ret != 0)
fprintf(stderr, "pulse_thread: Error writing to audio device\n%s", pa_strerror(err));
@@ -1673,14 +1696,7 @@ pulse_init(thread_info_t * info, int verbose, gboolean realtime, int priority) {
info->pa_spec.channels = 2;
info->pa_spec.rate = info->out_SR;
- info->pa = pa_simple_new(NULL, "Aqualung", PA_STREAM_PLAYBACK, NULL,
- "Music", &info->pa_spec, NULL, NULL, &err);
- if (!info->pa) {
- if (verbose) {
- fprintf(stderr, "Unable to initilize PulseAudio: %s\n", pa_strerror(err));
- }
- return err;
- }
+ info->pa = 0;
/* start PulseAudio output thread */
AQUALUNG_THREAD_CREATE(info->pulse_thread_id, NULL, pulse_thread, info)
@@ -3213,7 +3229,10 @@ main(int argc, char ** argv) {
if (output == PULSE_DRIVER) {
AQUALUNG_THREAD_JOIN(thread_info.pulse_thread_id)
free(thread_info.pa_short_buf);
- pa_simple_free(thread_info.pa);
+ if (thread_info.pa) {
+ pa_simple_free(thread_info.pa);
+ thread_info.pa = 0;
+ }
}
#endif /* HAVE_PULSE */
|
That looks good to me, thank you for the patch. I don't have access to test compilation with PulseAudio at the moment. @tomscii any chance you review and test? |
I will take a look and test this when I get a chance (soon) |
So does this actually solve your problem with high CPU usage in pipewire? |
I have applied and (briefly) tested this patch. Problems:
Given the above inferior (unacceptable) experience (for all users, not only those affected by whatever issue pipewire + ladspa_dsp is causing) I must say no to this otherwise reasonably looking patch. Clearly, a better approach is needed. It might be possible to solve the issue with a more in-depth rewrite of the pulse output code - I guess there is a reason other clients you looked at use a more elaborate API. |
Oh, and I still think the bug is in the program actually having high CPU usage. Just my $.02 |
Thanks for the review. Yes, this patch solves the issue of the high CPU usage in pipewire-uplse. About 1: yes, some audio seems to be lost while pausing and resuming. However, I don't hear much difference with the unmodified version. With the unmodified version, I have some unpleasant glitches when pausing and resuming. With the patched version these glitches might be more important in some cases. But it is not that obvious. About 2: yes. Aqualung now indeed disappears from the list of clients using audio when the playback is stopped. I tried with VLC, Audacious, Firefox, they all disappear when the playback is stopped. But they do not disappear when the playback is paused. This is probably because they use the asynchronous API of PulseAudio. With the simple API, I don't see any way of keeping Aqualung in the client list when the playback is paused. Do you have any suggestion to improve the patch? Maybe the bug is in pipewire-pulse. I may try to open a bug there, but my guess is that they will just reply that the PulseAudio API is used in a wrong way in Aqualung: the resources should be freed when the playback is stopped. |
Cool, thanks for confirming. So we can be certain that the bug is in pipewire-pulse, triggered by this client behaviour - I presume it starts busy spinning (or something along those lines) when it does not get input.
I do, and it is absolutely critical. (No, not for casual listening. But trust me on this one. Aqualung is a player for listeners who care and are passionate about subtle stuff like this.)
That is correct. And most probably the reason Aqualung uses the simple API the way it does. N.B.: Aqualung uses the same approach (stop sending data into the sink when paused/stopped) with other audio backends, too.
Well, not this patch. But quoting my previous post:
|
Here I'm a bit surprised. As I mentioned, in my setup, I have some unpleasant glitches when pausing and resuming with Aqualung, even with the unmodified version, and even without using ladspa_dsp. I have a much nicer behavior with Audacious on this aspect. Maybe that's another issue? And again, did you try to remove the call to pa_simple_flush and/or pa_simple_drain? Does it improve the pause/resume issue? On my side, I can't tell if this is better or not because of the glitches I can hear in any case. About a rewrite of the pulse output code, I have zero experience in this area, so I'm not volunteering to do it. But would you? |
Look, I am not interested in arguing about this.
No. I pointed out obvious showstopper issues for you to fix (if you want to take this further). I do not have more time to donate.
No. My opinion is that Aqualung is fine as is. And you already have a patch to fix your issue. It's just not accepted into upstream. But you are allowed to use it, if it makes you happy! |
I have installed Aqualung 1.2 compiled from the source on Kubuntu 24.04.
I use it with Pipewire, Pulseaudio driver and with ladspa_dsp. See here:
https://github.com/bmc0/dsp
The playback works just fine. During the playback, the pipewire-pulse process uses around 1% or 2% of CPU.
But once the playback is stopped, the pipewire-pulse process uses between 40% and 50% of CPU.
When Aqualung is exited, the CPU usage of the pipewire-pulse process is nearly 0%.
I tried with other audio players such as Audacious or VLC, and there is no such issue.
What is wrong here? Why a such high CPU usage while Aqualung does nothing?
The text was updated successfully, but these errors were encountered: