Skip to content

Rewrite ALSA buffering#123

Merged
devgianlu merged 2 commits intodevgianlu:masterfrom
aykevl:alsa-period-size
Oct 16, 2024
Merged

Rewrite ALSA buffering#123
devgianlu merged 2 commits intodevgianlu:masterfrom
aykevl:alsa-period-size

Conversation

@aykevl
Copy link
Copy Markdown
Contributor

@aykevl aykevl commented Oct 9, 2024

This PR fixes two main issues:

  • On some hardware, the output stutters a bit. This was the case on my laptop (Fedora Asahi) which despite being very fast still couldn't play audio without crackling on the laptop speakers (it did work over USB though). This was fixed by setting the period size to bufferSize/4, which was previously set to a default (very short) duration.
    I suspect this is the root cause of https://community.volumio.com/t/new-2023-spotify-plugin/63381/354
  • When pausing and resuming (possibly with a seek in between), some audio from before the pause would be repeated. This wasn't very noticeable with the previous short period size (80ms or 160ms or so), but with the new period size it suddenly became 2 seconds and very noticeable.
    An easy way to test this bug is to play a loud part of a song, pause, seek to a quiet part (the beginning is usually quiet), and resume. Before this PR, there was a bit of a stutter. After this PR, the stutter is gone entirely.

Note that this PR doesn't close/reopen the output device anymore on every pause/resume. I don't know whether anybody relied on this behavior but it's a change so noting it just in case.

The default period size on my system (Fedora Asahi, with PipeWire) is
220. This is much too short: it leads to lots of crackling even though
the system itself is most certainly fast enough.

Instead, request a period size that's a quarter of the buffer size. This
fixes the crackling and should also reduce CPU consumption.
Comment thread output/driver_unix.go
@devgianlu
Copy link
Copy Markdown
Owner

Note that this PR doesn't close/reopen the output device anymore on every pause/resume. I don't know whether anybody relied on this behavior but it's a change so noting it just in case.

This is required when working on systems that have multiple players. Imagine that I have go-librespot for Spotify and mpd for local files: if I want to switch from one player to the other it's easier if they release the device when pausing so that the other one can takeover without them having to coordinate.

@aykevl
Copy link
Copy Markdown
Contributor Author

aykevl commented Oct 13, 2024

Right, I see. I will update the PR to restore the previous behavior of closing and reopening the ALSA device on every pause/resume.

(My personal opinion is that this should not be handled by applications, but rather by an audio server like PulseAudio or PipeWire. They are designed to fill this exact role. But I checked, and MPD indeed closes the audio device on pause when using the ALSA output).

This is a partial rewrite of the ALSA output:

  * Reading data from the source and writing data out to the ALSA device
    is now done in a single goroutine. There's no internal ringbuffer
    anymore.
  * Instead of closing and reopening the ALSA device, it now uses
    snd_pcm_drop and snd_pcm_prepare.

Thie main benefit is that when you pause, seek, and then resume the
audio, the audio of the previous playback position is no longer played
for a short bit. Even when pausing or seeking, the buffer wasn't getting
cleared. This was not super noticeable due to the relatively short
buffer size (80ms on my laptop, 160ms on my Raspberry Pi 3) but with the
previous commit increasing the period size this became _very_ noticeable
and grew to 2 seconds.
@aykevl
Copy link
Copy Markdown
Contributor Author

aykevl commented Oct 14, 2024

Updated to close and restore PCM on pause/resume.

The behavior is still slightly different from what it was before in a different way: it is now only opened when it is first resumed (so it isn't opened directly after connecting to go-librespot). This is consistent with mpd for example and makes more sense to me, because it won't lead to unexpected "ALSA device is busy" errors after connecting but not playing any music.

Tested it to confirm it still works fine and still fixes the two issues in the PR description. I've also updated my Raspberry Pi so that I'll be running this code whenever I'm playing music there for more extensive testing.

@devgianlu devgianlu merged commit 2bd5800 into devgianlu:master Oct 16, 2024
@aykevl aykevl deleted the alsa-period-size branch October 16, 2024 20:56
@aykevl aykevl mentioned this pull request Oct 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants