jeremyevans / aqualung

Advanced music player
http://aqualung.jeremyevans.net
GNU General Public License v2.0
48 stars 15 forks source link

High CPU usage of pipewire-pulse with ladspa_dsp once playback is stopped #36

Open cepamoi opened 2 weeks ago

cepamoi commented 2 weeks ago

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?

jeremyevans commented 2 weeks ago

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.

cepamoi commented 2 weeks ago

There is no issue if ladspa_dsp is not used. The Aqualung process does not use much CPU when the playback is stopped. Only the pipewire-pulse process uses a lot of CPU. And I did not noticed any issue with other audio players. So it really looks to be an issue which is specific to the combination Aqualung + pipewire-pulse + ladspa_dsp.

jeremyevans commented 2 weeks ago

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?

tomscii commented 2 weeks ago

@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.

cepamoi commented 2 weeks ago

Sorry for the wrong link. Here is the corrected one: https://github.com/bmc0/dsp I have been using ladspa_dsp for many years, for room correction and headphone equalization. The main advantage of ladspa_dsp is that it is system wide: it works in any application that plays sounds. So for example, I can have headphone equalization while playing some song from YouTube in Firefox. So ladspa_dsp should work with Aqualung too.

As the issue only affects Aqualung and not the other audio players, I'm afraid that it is an Aqualung issue. So any help would be appreciated.

cepamoi commented 2 weeks ago

I used the pw-top command which gives some information and statistics about the pipewire process. The documentation is available here. During the playback of Aqualung, the output of the pw-top command looks like this: S ID QUANT RATE WAIT BUSY W/Q B/Q ERR FORMAT NAME R 48 2048 48000 650,6us 16,9us 0,02 0,00 68 S32LE 2 48000 alsa_output.pci-0000_00_1b.0.hdmi-stereo R 57 0 0 5,0us 4,5us 0,00 0,00 0 F32P 2 0 + ATH-M50X R 59 0 0 3,0us 241,8us 0,00 0,01 153 F32P 2 0 + output.filter-chain-2344-12 R 80 4630 44100 89,3us 198,5us 0,00 0,00 0 S16LE 2 44100 + Aqualung And the output of the pw-top command looks the same with any other audio application during the playback.

Now is the output of pw-top when the playback is stopped in Aqualung: S ID QUANT RATE WAIT BUSY W/Q B/Q ERR FORMAT NAME R 48 2048 48000 16,8ms 17,1us 0,39 0,00 87 S32LE 2 48000 alsa_output.pci-0000_00_1b.0.hdmi-stereo R 57 0 0 4,4us 4,2us 0,00 0,00 0 F32P 2 0 + ATH-M50X R 59 0 0 2,7us 16,3ms 0,00 0,38 187 F32P 2 0 + output.filter-chain-2344-12 R 80 4630 44100 189,2us 186,6us 0,00 0,00 0 S16LE 2 44100 + Aqualung We can notice two things: The node status is still "Running" (R) whereas the playback is stopped. The waiting time of the node alsa_output.pci-0000_00_1b.0.hdmi-stereo is much higher (16.8 ms vs 650.6 us).

To compare, here is the output of pw-top when the playback is stopped in Audacious: S ID QUANT RATE WAIT BUSY W/Q B/Q ERR FORMAT NAME I 48 2048 48000 20,3us 38,4us 0,00 0,00 126 S32LE 2 48000 alsa_output.pci-0000_00_1b.0.hdmi-stereo I 57 0 0 91,2us 14,8us 0,00 0,00 0 F32P 2 0 + ATH-M50X I 59 0 0 11,8us 273,5us 0,00 0,01 268 F32P 2 0 + output.filter-chain-2344-12 We can notice that the node status is "Idle" (I). And after a few seconds, the node status switches to "Suspended" (S).

So maybe something is missing in Aqualung when stopping the playback with Pulseaudio/Pipewire.

jeremyevans commented 2 weeks ago

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.

tomscii commented 1 week ago

Seconding @jeremyevans here, please try to run Aqualung with -o pulse

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.

tomscii commented 1 week ago

So maybe something is missing in Aqualung when stopping the playback with Pulseaudio/Pipewire.

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.

cepamoi commented 1 week ago

Running Aqualung with -o pulse does not change anything to the issue. Audacious indeed natively supports Pipewire, but the output can be selected explicitly to use PulseAudio. And when I do that, there is no issue with Audacious.

I checked the source code of Audacious and VLC. Both use the asynchronous API of PulseAudio, while Aqualung uses the simple API of PulseAudio. When the playback is stopped in Audacious and in VLC, the following functions are called (in that order): pa_stream_disconnect(), pa_stream_unref(), pa_context_disconnect(), pa_context_unref(), pa_mainloop_free(). In these functions, it seems that the call to pa_context_disconnect() effectively frees the resources and the node status as reported by pw-top goes back to idle and then to suspended.

In Aqualung, there is no equivalent call to some function of the simple API of PulseAudio when the palyback is stopped. This most probably confirms what @tomscii says:

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.

According to the documentation of the simple API of PulseAudio:

Once playback or capture is complete, the connection should be closed and resources freed. This is done through: pa_simple_free(s);

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. Would that be a problem to change the current implementation to do that? Thanks.

jeremyevans commented 1 week ago

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.

tomscii commented 1 week ago

No, it won't be a problem to change the current implementation the way you describe. So go on and change it!

cepamoi commented 1 week ago

Attached is a first tentative. I checked play, stop, pause, resume, seek, next track, end of playback. All seem to be ok. Could you please check on your side? As I have zero knowledge of the Aqualung code, please bear with me. Thanks. core.c.gz

jeremyevans commented 1 week ago

If you don't want to provide a pull request, could you post the output of git diff?

cepamoi commented 1 week ago

Sorry, I'm not very familiar with pull requests and GitHub. Attached is the output of git diff. core.c.patch.gz

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 */
jeremyevans commented 1 week ago

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?

tomscii commented 1 week ago

I will take a look and test this when I get a chance (soon)

tomscii commented 1 week ago

Attached is a first tentative. I checked play, stop, pause, resume, seek, next track, end of playback. All seem to be ok. Could you please check on your side? As I have zero knowledge of the Aqualung code, please bear with me. Thanks. core.c.gz

So does this actually solve your problem with high CPU usage in pipewire?

tomscii commented 1 week ago

I have applied and (briefly) tested this patch.

Problems:

  1. Pausing and resuming - a small bit of audio is lost each time. I.e., it does not resume from the point it stopped at, but jumps ahead a small bit and resumes from there. Presumably due to flushing. Very annoying and makes Aqualung unusable for certain use cases (for which it is otherwise excellent).
  2. Closing the device means that Aqualung completely disappears from the clients list. E.g., upon realizing that sound is coming from the wrong output, I cannot pause Aqualung, go to the volume control app (e.g., pavucontrol) and switch it to a different output. Because after pausing Aqualung, it is completely absent from pavucontrol. I have to have it playing (on the wrong output) so I have a chance to interact with the mixer settings. Extremely disturbing and distracting.

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.

tomscii commented 1 week ago

Oh, and I still think the bug is in the program actually having high CPU usage. Just my $.02

cepamoi commented 1 week ago

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. Did you try to remove the call to pa_simple_flush and/or pa_simple_drain ?

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.

tomscii commented 1 week ago

Yes, this patch solves the issue of the high CPU usage in pipewire-uplse.

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.

About 1: yes, some audio seems to be lost while pausing and resuming. However, I don't hear much difference with the unmodified version.

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.)

About 2: yes. [...] I don't see any way of keeping Aqualung in the client list when the playback is paused.

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.

Do you have any suggestion to improve the patch?

Well, not this patch. But quoting my previous post:

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.

cepamoi commented 1 week ago
About 1: yes, some audio seems to be lost while pausing and resuming. However, I don't hear much difference with the unmodified version.

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.)

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?

tomscii commented 1 week ago

Here I'm a bit surprised. As I mentioned, in my setup, I have some unpleasant glitches when pausing and resuming with Aqualung

Look, I am not interested in arguing about this.

And again, did you try to remove the call to pa_simple_flush and/or pa_simple_drain

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.

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?

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!