On 2011-04-20 12:09, Maarten Lankhorst wrote: > Hi David, > > Op 20-04-11 09:33, David Henningsson schreef: >> On 2011-04-19 18:12, Maarten Lankhorst wrote: >>> Hi all, >>> >>> For wine I was investigating a bug with pulseaudio, it seems >>> alsa-plugins' pulse driver ignores underruns. >> >> Hmm, doesn't wine come with a native pulse driver these days? > Nope, but distros patch in a buggy winepulse driver, wine is working on > rearchitecting its driver model, so that a pulseaudio driver might be > added after that, but the current winepulse driver was a bad > copy/replace job of the wrong sound driver. :) > >>> This is probably because >>> an underrun will force you to call snd_pcm_prepare, this will destroy >>> the stream and set up a similar new one again. >> >> ...whereas the proper way to resolve the underrun in the pulse plugin is >> just to feed it with more data, which is likely what would happen if you >> do not report the underrun. >> >>> This causes more >>> susceptibility to underruns, so that code was disabled. >> >> Takashi added some kind of option so you can turn the reporting of >> underruns on again, if that helps. > If you read my patch, that's what it does. > >>> However if I >>> force underruns to occur,the state will stay running and it appears >>> there is still some data left in the buffer, so sound stalls entirely. >> >> This is nothing I've heard of. > Sadly all too common here, if I feed data and it underruns, it may > appear to be running but is stalled entirely. Could you provide a test case where this occurs? (Pulseaudio/ALSA version, client applications/libraries, etc) >>> The latency gets updated to something like 0x7bdXXXXXXXXXXXXX which >>> looks suspiciously much like a pointer to me, which may be a bug. If I >>> instead make pulse_prepare call pulse_start on underrun, it's handled >>> properly and sound will continue to work. >>> >>> So my questions are. >>> 1. Is the weird latency value update a bug? >> >> I guess so, but what sense would it make to read the latency if you're >> in an underrun condition - and btw, exactly what latency variable is >> this? > In a response to a latency event I printf the latency inside the > callback, with pa_stream_get_latency. > >>> Digging it up I can only >>> assume it's a bug in src/stream.c , but I haven't figured out why yet. >>> Tested with pulseaudio.c >>> 2. Any comments on this patch for alsa-plugins? >> >> IIRC, by setting handle_underrun to 1, you're reopening bugs of >> broken/skipping audio for mpg123 and other programs, which get stuck in >> an infinite loop of "write one sample, force start the stream, write >> more samples, asynchronusly get an underrun, drop the stream and drop >> samples already written". >> >> There should be threads on at least alsa-devel about this, from a year >> back or so. > This is exactly what my patch was addressing, I was fixing that bug by > handling underrun correctly. snd_pcm_prepare() is called when an > underrun occurs, so instead I make it only restart the stream if > underrun. Not handling underruns at all seems to allow it to stall on > xrun, while claiming to be running. This sometimes appears to happen in > other programs too though, like mplayer. Could it be because of the > weird latency value? Ok, thanks for the clarifications. I've taken a closer look at your patch now and have the following comments: When you're calling pulse_start instead of continuing in pulse_prepare - pulse_start will call uncork/trigger, won't that just cause another underrun? Would it perhaps be better to just return without doing anything? mpg123 was used as a test case for the original bug, and mpg123 calls snd_pcm_drop on an underrun, so you will be regressing mpg123 by changing handle_underrun to 1. (Now of course we could fix mpg123, but I don't know if there are a lot of other programs out there doing similar things?) My main point is that the underrun is often obsolete when the message reached the application, because more data has already been written, therefore reporting it to the app does more harm than good. At least until the underrun callback (and PA protocol) supports sending the position of underrun, together with the underrun message. If we had that position, we could compare that with the current write pointer to determine whether the underrun is actually obsolete or not. -- David Henningsson, Canonical Ltd. http://launchpad.net/~diwic
