search

owntone / owntone-server

Linux/FreeBSD DAAP (iTunes) and MPD audio server with support for AirPlay 1 and 2 speakers (multiroom), Apple Remote (and compatibles), Chromecast, Spotify and internet radio.
https://owntone.github.io/owntone-server
GNU General Public License v2.0
2.09k stars 237 forks source link

Merge b7cfb65 to fix local ALSA underruns #87

Closed pcoultha closed 9 years ago

pcoultha commented 9 years ago

In attempting to use local audio with ALSA, I encountered severe quality problems I believe were being caused by underruns. Commit b7cfb651da92023101d720ec07da5b59df2f9204 "Prevent buffer underrun (I think...)" completely fixed the issue for me on Debian with kernel 3.2.

I'd like to suggest merging at least the ALSA portion of this commit. I do not have access to an OSS setup to test that.

I found this commit attached to pull request #41.

ejurgensen commented 9 years ago

I kind of remember that commit, and as I remember it, it didn't actually do anything, so I didn't include it. I know there is a bug in the current alsa code that sometimes gives what may be buffer underruns, but I think it may just be a coincidence that you didn't run into it again after restarting forked-daapd with this commit.

Of course, I'd like to know if it really consistently solves the problem. In that case I'm wrong and need to look at it again!

pcoultha commented 9 years ago

In my case, it definitely wasn't a fluke or coincidence. Prior to this change, I tried dozens of things to resolve choppy, un-listenable local audio. I restarted forked-daapd dozens of times in the process, tried precompiled binaries, building from source, tweaking ALSA, etc. The audio problems were obvious and immediate after every restart.

Applying this one line change to laudio_alsa.c fixed things immediately, completely, and permanently for me.

I've struggled to follow the local audio code, so I can't point to the actual bug or if this is the "correct" fix, but it works for me. I also just remembered that my investigation of this issue, I did add some additional logging in laudio_xrun_recover, so I can confirm that before this change forked_daapd was repeatedly underrunning the ALSA buffer and then recovering successfully. No further underruns after applying this change.

ejurgensen commented 9 years ago

Thank you, sounds like you have been very thorough, that is very helpful.

I have a poor understanding of what is going on in this part of the local audio code, but I can see that the commit is effectively the same as removing the possible return from the write funtion (since nsamp is never going to be more than AIRTUNES_V2_PACKET_SAMPLES). So that's what I did in the laudio branch linked above. And you're right that this solves the underrun, so I think I will commit this, even though I don't fully understand it. First I'll test it some more.

I actually do understand why it solves the underrun: That's because forked-daapd will first build its own queue of packages (a buffer), and at a certain point begin playback, which means sending packages to alsa at the playback sample rate. If this gets slightly out of sync you get sound problems. With this change, the entire queue/buffer gets sent to alsa right in the beginning, which makes sure alsa won't underrun, even if there are sync problems. Maybe that sounds ok, but I have a feeling the original author had a reason he didn't want to do this. Perhaps he wanted to prevent overruns, or maybe it was to sync better with Airplay. Not sure...

ejurgensen commented 9 years ago

Seems to work fine with the change, so it's in now. Thanks for bringing this up.