From: Miguel F. <mfr...@gm...> - 2009-02-07 18:15:51
Attachments:
xine-lib-safe-audio-pause.patch
|
Hi folks, Our friends from kde/amarok have asked me to take a look into an issue with xine. Their currently open bug is https://bugs.kde.org/show_bug.cgi?id=180339 I have been able to reproduce this problem using our pulseaudio driver (which, imho, might not be ready for prime time - but that's another question). It is just a matter of pausing/unpausing stream until it freezes completely. The thing is that pulse is more sensitive to the currently unsafe xine pause code than alsa. By unsafe i mean that it is possible that we may try to write to a paused device. For pulse this is catastrophic: write will never complete but the lock will be held, therefore we will not be able to "uncork" it from a different thread => deadlock. Pulse's latency to obtain delay and stuff should also contribute to trigger this bug. I have reworked pause logic a bit just to make sure it can't happed. The patch is attached (not committed though). If anybody could give it a try before i commit, i would be very grateful. I'm a bit rusty on xine hacking and i'd like avoid doing stupid things ;-) regards, Miguel |
From: Rex D. <rd...@ma...> - 2009-02-08 14:38:06
|
Miguel Freitas wrote: > Hi Rex, > > On Wed, Jan 21, 2009 at 4:51 PM, Rex Dieter <rd...@ma...> wrote: >> Many Fedora users are experiencing a choppy audio experience, >> https://bugzilla.redhat.com/show_bug.cgi?id=470568 >> >> and an enterprising fellow investigated and came up with an analysis and >> a partial workaround, >> https://bugzilla.redhat.com/show_bug.cgi?id=470568#c27 >> >> comments? > > I would say your patch is fine for a packager, but not really good for > upstream. It seems reasonable to disable sync for audio only streams, > but remember you will lose sync with visualization plugins. besides it > doesn't fix video playback. > > The investigation seems correct: pulse indeed returns wide varying > values of latency when starting a new playback. Do you have any > comment from Lennart on this? I had also posted similarly to the pa mailing list, https://tango.0pointer.de/pipermail/pulseaudio-discuss/2009-January/003011.html but no comment yet there or in bugzilla. Still awaiting any sort of comment or feedback from pa folk. Any sort of additional poking appreciated. -- Rex |
From: Darren S. <li...@yo...> - 2009-02-07 22:07:39
|
I demand that Miguel Freitas may or may not have written... [snip] > pulse is more sensitive to the currently unsafe xine pause code than alsa. > By unsafe i mean that it is possible that we may try to write to a paused > device. For pulse this is catastrophic: [...] > I have reworked pause logic a bit just to make sure it can't happed. The > patch is attached (not committed though). > If anybody could give it a try before i commit, i would be very grateful. > I'm a bit rusty on xine hacking and i'd like avoid doing stupid things ;-) NAK. I'm seeing too many dropped frames (vdr input plugin in 1.2) with this patch. -- | Darren Salt | linux or ds at | nr. Ashington, | Toon | RISC OS, Linux | youmustbejoking,demon,co,uk | Northumberland | Army | + Use more efficient products. Use less. BE MORE ENERGY EFFICIENT. Californians are not without their faults. |
From: Miguel F. <mfr...@gm...> - 2009-02-08 00:57:10
|
On Sat, Feb 7, 2009 at 8:04 PM, Darren Salt <li...@yo...> wrote: > > If anybody could give it a try before i commit, i would be very grateful. > > I'm a bit rusty on xine hacking and i'd like avoid doing stupid things ;-) > > NAK. I'm seeing too many dropped frames (vdr input plugin in 1.2) with this > patch. :( any easier test case? i don't have vdr here... regards, Miguel |
From: Darren S. <li...@yo...> - 2009-02-09 18:55:25
|
I demand that Miguel Freitas may or may not have written... > On Sat, Feb 7, 2009 at 8:04 PM, Darren Salt > <li...@yo...> wrote: >>> If anybody could give it a try before i commit, i would be very grateful. >>> I'm a bit rusty on xine hacking and i'd like avoid doing stupid things >>> ;-) >> NAK. I'm seeing too many dropped frames (vdr input plugin in 1.2) with >> this patch. > :( > any easier test case? i don't have vdr here... Not that I know of... -- | Darren Salt | linux or ds at | nr. Ashington, | Toon | RISC OS, Linux | youmustbejoking,demon,co,uk | Northumberland | Army | + Use more efficient products. Use less. BE MORE ENERGY EFFICIENT. Profanity is the one language all programmers know best. |
From: Miguel F. <mfr...@gm...> - 2009-02-09 19:34:45
|
On Mon, Feb 9, 2009 at 4:48 PM, Darren Salt <li...@yo...> wrote: >>> NAK. I'm seeing too many dropped frames (vdr input plugin in 1.2) with >>> this patch. > >> :( >> any easier test case? i don't have vdr here... > > Not that I know of... are you sure the dropped frames are related to the patch? the patch to xine.c is only executed when you change playback speed. the new mutex in ao_loop is used by this thread only, unless (guess what) the playback speed is changed... is vdr plugin doing something fancy with playback speed? regards, Miguel |
From: Christophe T. <hf...@fr...> - 2009-02-09 20:28:33
|
Le lundi 9 février 2009 20:34:41 Miguel Freitas, vous avez écrit : > On Mon, Feb 9, 2009 at 4:48 PM, Darren Salt > > <li...@yo...> wrote: > >>> NAK. I'm seeing too many dropped frames (vdr input plugin in 1.2) with > >>> this patch. > >>> > >> :( > >> > >> any easier test case? i don't have vdr here... > > > > Not that I know of... > > are you sure the dropped frames are related to the patch? > > the patch to xine.c is only executed when you change playback speed. > the new mutex in ao_loop is used by this thread only, unless (guess > what) the playback speed is changed... > > is vdr plugin doing something fancy with playback speed? AFAIR, it does indeed. I think it slows down playback at channel switch until enought data is buffered. -- Christophe Thommeret |
From: Miguel F. <mfr...@gm...> - 2009-02-09 22:52:59
|
On Mon, Feb 9, 2009 at 6:28 PM, Christophe Thommeret <hf...@fr...> wrote: >> is vdr plugin doing something fancy with playback speed? > > AFAIR, it does indeed. I think it slows down playback at channel switch until > enought data is buffered. it still seem strange to me: if speed is different than XINE_SPEED_NORMAL then audio will be dropped. unless, of course, audio.synchronization.slow_fast_audio is set. i used to do something similar in pvr plugin, that is, trying to match the rate data is produced. however i registered my own scr (system clock reference) plugin. this way it is the clock that runs faster/slower not the playback speed. besides, calling set_speed_internal() frequently is not a good idea anyway: it will cause tickets revocation which will stall the pipeline. never mind. i will change the patch to not grab the lock unless we are pausing. this should revert pretty much to the previous behavior. regards, Miguel |
From: Miguel F. <mfr...@gm...> - 2009-02-10 00:42:50
Attachments:
xine-lib-safe-audio-pause2.patch
|
Hi Darren, On Sat, Feb 7, 2009 at 8:04 PM, Darren Salt <li...@yo...> wrote: > NAK. I'm seeing too many dropped frames (vdr input plugin in 1.2) with this > patch. ok, second try. may you check if the attached patch works better? regards, Miguel |
From: Darren S. <li...@yo...> - 2009-02-10 16:54:42
|
I demand that Miguel Freitas may or may not have written... > On Sat, Feb 7, 2009 at 8:04 PM, Darren Salt > <li...@yo...> wrote: >> NAK. I'm seeing too many dropped frames (vdr input plugin in 1.2) with >> this patch. > ok, second try. > may you check if the attached patch works better? Still problematic... -- | Darren Salt | linux or ds at | nr. Ashington, | Toon | RISC OS, Linux | youmustbejoking,demon,co,uk | Northumberland | Army | + Use more efficient products. Use less. BE MORE ENERGY EFFICIENT. How many weeks are there in a light year? |
From: Miguel F. <mfr...@gm...> - 2009-02-10 18:39:34
|
Hi Darren, On Tue, Feb 10, 2009 at 2:53 PM, Darren Salt <li...@yo...> wrote: >> ok, second try. >> may you check if the attached patch works better? > > Still problematic... we will need to take a decision here. with current xine code, pulseaudio driver is practically guaranteed to deadlock if user press pause (xine will try to write to a paused device, the mutex will be held, the other thread will never be able to obtain the mutex to resume playback) since xine is used by default by phonon, and (it seems) that both ubuntu and fedora are shipping it with pulseaudio, you can imagine how many users are affected. but the patch causes problem to the vdr plugin. i have no vdr to understand why such incompatibility. i would vote for including the patch anyway, but i'm not going to decide it alone. any ideas? regards, Miguel |
From: Thibaut M. <thi...@gm...> - 2009-02-13 15:22:23
|
Hi Miguel, 2009/2/10 Miguel Freitas <mfr...@gm...> > Hi Darren, > > On Tue, Feb 10, 2009 at 2:53 PM, Darren Salt > <li...@yo...> wrote: > >> ok, second try. > >> may you check if the attached patch works better? > > > > Still problematic... > > we will need to take a decision here. > > with current xine code, pulseaudio driver is practically guaranteed to > deadlock if user press pause (xine will try to write to a paused > device, the mutex will be held, the other thread will never be able to > obtain the mutex to resume playback) Good catch, the bug is real. The engine works well with other drivers "by chance", maybe there is bigger internal buffer with alsa, i don't know or maybe the data is just discarded when writing while paused. > > since xine is used by default by phonon, and (it seems) that both > ubuntu and fedora are shipping it with pulseaudio, you can imagine how > many users are affected. > > but the patch causes problem to the vdr plugin. i have no vdr to > understand why such incompatibility. You know like me that VDR used to play with xine internal private stuff. > i would vote for including the patch anyway, but i'm not going to > decide it alone. any ideas? IMHO the priority is to fix the core engine, then the plugins. I don't really understand how your patch is affecting the number of dropped frames. Maybe your patch exhibits a bug in VDR, or maybe VDR changes too often the clock speed. Reinhard ? > > regards, > > Miguel > > Thibaut |
From: Miguel F. <mfr...@gm...> - 2009-02-13 16:18:48
|
Hi Thibaut, On Fri, Feb 13, 2009 at 1:22 PM, Thibaut Mattern <thi...@gm...> wrote: >> with current xine code, pulseaudio driver is practically guaranteed to >> deadlock if user press pause (xine will try to write to a paused >> device, the mutex will be held, the other thread will never be able to >> obtain the mutex to resume playback) > > Good catch, the bug is real. The engine works well with other drivers "by > chance", maybe there is bigger internal buffer with alsa, i don't know or > maybe the data is just discarded when writing while paused. yes, i have other guesses as well: most likely alsa's function to obtain the delay is very fast, therefore the chance it might get paused at this point is remote. pulse, otoh, will probably need to query the server about the latency, therefore there will be at least 2 context switches involved. much more likely to be affected by the bug. >> since xine is used by default by phonon, and (it seems) that both >> ubuntu and fedora are shipping it with pulseaudio, you can imagine how >> many users are affected. i've just noticed that opensuse (at least with packman repositories installed) is also using xine+pulse as default. regards, Miguel |
From: Rex D. <rd...@ma...> - 2009-02-13 18:27:05
|
Miguel Freitas wrote: >>> since xine is used by default by phonon, and (it seems) that both >>> ubuntu and fedora are shipping it with pulseaudio, you can imagine how >>> many users are affected. > > i've just noticed that opensuse (at least with packman repositories > installed) is also using xine+pulse as default. To interject a bit, as a fedora developer, I'd be fabulously happy to receive any sort of advice or feedback wrt to issues like these, esp if you disagree with any decisions or policies made. I'd venture folks from other distros would feel likewise. That said, thank you all for your efforts. -- Rex |
From: Miguel F. <mfr...@gm...> - 2009-02-17 02:54:00
|
Hi Rex, On Fri, Feb 13, 2009 at 3:26 PM, Rex Dieter <rd...@ma...> wrote: > Miguel Freitas wrote: >>>> since xine is used by default by phonon, and (it seems) that both >>>> ubuntu and fedora are shipping it with pulseaudio, you can imagine how >>>> many users are affected. > > To interject a bit, as a fedora developer, I'd be fabulously happy to > receive any sort of advice or feedback wrt to issues like these, esp if you > disagree with any decisions or policies made. I'd venture folks from other > distros would feel likewise. My "complain" here is simple. I'm sympathetic to the idea of pulseaudio in general, it seems well coded and (last time i checked - 2 years ago) well supported. I have also helped coding the first xine pulseaudio driver at the time it was called something else. That said, i never used pulseaudio myself. I was using alsa directly until very recently (2009) when i upgraded 3 computers and i noticed pulse being used everywhere. It doesn't take more than a few days using xine + pulse combination to note (1) the stuttering when starting playback and (2) the pause/resume lockup. So i can only guess that people might not be using (testing) what they are packaging. Anyway, my suggestion to distros is: pack the latest 1.1.16.2 release + pause/resume patch (changeset 2208e465026c) and make it available to users asap because it fixes several lockups. You may also include that workaround to remove stuttering from audio-only streams if you want. Then, with the pressing issues out of the way, we continue tracking the remaining issues either in xine or pulse bugtrackers. I would remind that last big rewrite of our pulse driver (the same that increased it's priority above ALSA) was contributed by Lennart, 10 months ago, so he probably knows that code better. regards, Miguel |
From: Rex D. <rd...@ma...> - 2009-02-17 17:14:13
|
Miguel Freitas wrote: > Anyway, my suggestion to distros is: pack the latest 1.1.16.2 release > + pause/resume patch (changeset 2208e465026c) and make it available to > users asap because it fixes several lockups. You may also include that > workaround to remove stuttering from audio-only streams if you want. Thank you, I'll do just that for fedora. > Then, with the pressing issues out of the way, we continue tracking > the remaining issues either in xine or pulse bugtrackers. I would > remind that last big rewrite of our pulse driver (the same that > increased it's priority above ALSA) was contributed by Lennart, 10 > months ago, so he probably knows that code better. nod. -- Rex |
From: <Rei...@st...> - 2009-04-01 13:19:06
|
Hello, On Sat, Feb 07, 2009 at 04:15:48PM -0200, Miguel Freitas wrote: > I have been able to reproduce this problem using our pulseaudio driver > (which, imho, might not be ready for prime time - but that's another > question). It is just a matter of pausing/unpausing stream until it freezes > completely. > > The thing is that pulse is more sensitive to the currently unsafe xine pause > code than alsa. By unsafe i mean that it is possible that we may try to > write to a paused device. For pulse this is catastrophic: write will never > complete but the lock will be held, therefore we will not be able to > "uncork" it from a different thread => deadlock. Pulse's latency to obtain > delay and stuff should also contribute to trigger this bug. Sorry for heating up this old thread, but how sure are you this is the issue? MPlayer is single-threaded (so none of these issues should apply) but still there is a reproducible hang related to pausing. For some reason pa_stream_writable_size gets stuck at 0 since around pulseaudio version 0.9.11-0.9.13 (see also http://www.pulseaudio.org/ticket/440). Simply calling pa_stream_flush before unpausing seems to fix it (but is a bad hack)... Of course I don't know if you hit the same issue or not, neither can I be sure that MPlayer isn't wrong, given that no documentation at all seems to exist for pa_stream_cork... Greetings, Reimar Döffinger |
From: Lennart P. <mz...@0p...> - 2009-04-01 14:44:14
|
On Wed, 01.04.09 15:18, Rei...@st... (Rei...@st...) wrote: > > Hello, > On Sat, Feb 07, 2009 at 04:15:48PM -0200, Miguel Freitas wrote: > > I have been able to reproduce this problem using our pulseaudio driver > > (which, imho, might not be ready for prime time - but that's another > > question). It is just a matter of pausing/unpausing stream until it freezes > > completely. > > > > The thing is that pulse is more sensitive to the currently unsafe xine pause > > code than alsa. By unsafe i mean that it is possible that we may try to > > write to a paused device. For pulse this is catastrophic: write will never > > complete but the lock will be held, therefore we will not be able to > > "uncork" it from a different thread => deadlock. Pulse's latency to obtain > > delay and stuff should also contribute to trigger this bug. > > Sorry for heating up this old thread, but how sure are you this is the > issue? > MPlayer is single-threaded (so none of these issues should apply) but > still there is a reproducible hang related to pausing. > For some reason pa_stream_writable_size gets stuck at 0 since around pulseaudio > version 0.9.11-0.9.13 (see also http://www.pulseaudio.org/ticket/440). > Simply calling pa_stream_flush before unpausing seems to fix it (but is > a bad hack)... > Of course I don't know if you hit the same issue or not, neither can I > be sure that MPlayer isn't wrong, given that no documentation at all > seems to exist for pa_stream_cork... This whole issue is almost certainly a bug in PA and should be fixed there. i.e. it's all my fault. It's one of the few remaining things to fix before PA 0.9.15. See PA bug #440. Lennart -- Lennart Poettering Red Hat, Inc. lennart [at] poettering [dot] net ICQ# 11060553 http://0pointer.net/lennart/ GnuPG 0x1A015CC4 |
From: Reimar D. <Rei...@gm...> - 2009-04-01 13:14:25
|
Hello, On Sat, Feb 07, 2009 at 04:15:48PM -0200, Miguel Freitas wrote: > I have been able to reproduce this problem using our pulseaudio driver > (which, imho, might not be ready for prime time - but that's another > question). It is just a matter of pausing/unpausing stream until it freezes > completely. > > The thing is that pulse is more sensitive to the currently unsafe xine pause > code than alsa. By unsafe i mean that it is possible that we may try to > write to a paused device. For pulse this is catastrophic: write will never > complete but the lock will be held, therefore we will not be able to > "uncork" it from a different thread => deadlock. Pulse's latency to obtain > delay and stuff should also contribute to trigger this bug. Sorry for heating up this old thread, but how sure are you this is the issue? MPlayer is single-threaded (so none of these issues should apply) but still there is a reproducible hang related to pausing. For some reason pa_stream_writable_size gets stuck at 0 since around pulseaudio version 0.9.11-0.9.13 (see also http://www.pulseaudio.org/ticket/440). Simply calling pa_stream_flush before unpausing seems to fix it (but is a bad hack)... Of course I don't know if you hit the same issue or not, neither can I be sure that MPlayer isn't wrong, given that no documentation at all seems to exist for pa_stream_cork... Greetings, Reimar Döffinger |