* [PATCH 0/2] alsa-plugins: fix PulseAudio prebuf
@ 2012-11-14 18:07 Rémi Denis-Courmont
2012-11-14 18:07 ` [PATCH 1/2] pcm_pulse: do not trigger immediately at start Rémi Denis-Courmont
0 siblings, 1 reply; 7+ messages in thread
From: Rémi Denis-Courmont @ 2012-11-14 18:07 UTC (permalink / raw)
To: Alsa-devel
Hello,
The following changes since commit 9df5dc7dd628b0d692cf896c210a134e85e88915:
Release v1.0.26 (2012-09-06 09:52:58 +0200)
make the ALSA<->PulseAudio PCM plugin abide by sw_params. This fixes
frequently broken playback with VLC, or potentially any other
application that expects playback to start within a short or configured time.
Best regards,
----------------------------------------------------------------
Rémi Denis-Courmont (2):
pcm_pulse: do not trigger immediately at start
pcm_pulse: set prebuf parameter according to software parameters
pulse/pcm_pulse.c | 37 ++++++++++++++++++++++++++++---------
1 file changed, 28 insertions(+), 9 deletions(-)
--
Rémi Denis-Courmont
http://www.remlab.net/
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
^ permalink raw reply [flat|nested] 7+ messages in thread* [PATCH 1/2] pcm_pulse: do not trigger immediately at start 2012-11-14 18:07 [PATCH 0/2] alsa-plugins: fix PulseAudio prebuf Rémi Denis-Courmont @ 2012-11-14 18:07 ` Rémi Denis-Courmont 2012-11-14 18:07 ` [PATCH 2/2] pcm_pulse: set prebuf parameter according to software parameters Rémi Denis-Courmont 2012-11-15 7:55 ` [PATCH 1/2] pcm_pulse: do not trigger immediately at start David Henningsson 0 siblings, 2 replies; 7+ messages in thread From: Rémi Denis-Courmont @ 2012-11-14 18:07 UTC (permalink / raw) To: alsa-devel; +Cc: Rémi Denis-Courmont There are no samples to play at that point so this is useless. The non-zero buffer_attr.prebuf ensures that the PulseAudio daemon will start streaming automatically (like ALSA sw_params start_threshold). --- pulse/pcm_pulse.c | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-) diff --git a/pulse/pcm_pulse.c b/pulse/pcm_pulse.c index 24fd4da..0165120 100644 --- a/pulse/pcm_pulse.c +++ b/pulse/pcm_pulse.c @@ -204,8 +204,8 @@ static void stream_success_cb(pa_stream * p, int success, void *userdata) static int pulse_start(snd_pcm_ioplug_t * io) { snd_pcm_pulse_t *pcm = io->private_data; - pa_operation *o, *u; - int err = 0, err_o = 0, err_u = 0; + pa_operation *o; + int err = 0, err_o = 0; assert(pcm); @@ -224,18 +224,12 @@ static int pulse_start(snd_pcm_ioplug_t * io) goto finish; } - u = pa_stream_trigger(pcm->stream, stream_success_cb, pcm); - pcm->underrun = 0; err_o = pulse_wait_operation(pcm->p, o); - if (u) - err_u = pulse_wait_operation(pcm->p, u); pa_operation_unref(o); - if (u) - pa_operation_unref(u); - if (err_o < 0 || err_u < 0) { + if (err_o < 0) { err = -EIO; goto finish; } -- 1.7.10.4 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH 2/2] pcm_pulse: set prebuf parameter according to software parameters 2012-11-14 18:07 ` [PATCH 1/2] pcm_pulse: do not trigger immediately at start Rémi Denis-Courmont @ 2012-11-14 18:07 ` Rémi Denis-Courmont 2012-11-15 8:00 ` David Henningsson 2012-11-15 7:55 ` [PATCH 1/2] pcm_pulse: do not trigger immediately at start David Henningsson 1 sibling, 1 reply; 7+ messages in thread From: Rémi Denis-Courmont @ 2012-11-14 18:07 UTC (permalink / raw) To: alsa-devel; +Cc: Rémi Denis-Courmont The current default value for prebuf is very high, almost the full virtual ALSA buffer. This breaks some application especially where low latency is involved. This patch makes pcm_pulse implement the sw_params callback and get the prebuf value from the ALSA software parameters. Thus the trigger latency is much more like what an ALSA application should expect from an ALSA PCM device. --- pulse/pcm_pulse.c | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/pulse/pcm_pulse.c b/pulse/pcm_pulse.c index 0165120..08b6cea 100644 --- a/pulse/pcm_pulse.c +++ b/pulse/pcm_pulse.c @@ -859,6 +859,30 @@ static int pulse_hw_params(snd_pcm_ioplug_t * io, return err; } +static int pulse_sw_params(snd_pcm_ioplug_t *io, snd_pcm_sw_params_t *params) +{ + snd_pcm_pulse_t *pcm = io->private_data; + snd_pcm_uframes_t start_threshold; + + assert(pcm); + + if (!pcm->p || !pcm->p->mainloop) + return -EBADFD; + + pa_threaded_mainloop_lock(pcm->p->mainloop); + + snd_pcm_sw_params_get_start_threshold(params, &start_threshold); + + if (start_threshold < io->period_size) + start_threshold = io->period_size; + + pcm->buffer_attr.prebuf = start_threshold * pcm->frame_size; + + pa_threaded_mainloop_unlock(pcm->p->mainloop); + + return 0; +} + static int pulse_close(snd_pcm_ioplug_t * io) { snd_pcm_pulse_t *pcm = io->private_data; @@ -925,6 +949,7 @@ static const snd_pcm_ioplug_callback_t pulse_playback_callback = { .poll_revents = pulse_pcm_poll_revents, .prepare = pulse_prepare, .hw_params = pulse_hw_params, + .sw_params = pulse_sw_params, .close = pulse_close, .pause = pulse_pause }; -- 1.7.10.4 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] pcm_pulse: set prebuf parameter according to software parameters 2012-11-14 18:07 ` [PATCH 2/2] pcm_pulse: set prebuf parameter according to software parameters Rémi Denis-Courmont @ 2012-11-15 8:00 ` David Henningsson 2012-11-16 15:49 ` Rémi Denis-Courmont 0 siblings, 1 reply; 7+ messages in thread From: David Henningsson @ 2012-11-15 8:00 UTC (permalink / raw) To: Rémi Denis-Courmont; +Cc: alsa-devel On 11/14/2012 07:07 PM, Rémi Denis-Courmont wrote: > The current default value for prebuf is very high, almost the full > virtual ALSA buffer. This breaks some application especially where > low latency is involved. > > This patch makes pcm_pulse implement the sw_params callback and get > the prebuf value from the ALSA software parameters. Thus the > trigger latency is much more like what an ALSA application should > expect from an ALSA PCM device. This seems reasonable I believe; see review comments below. > --- > pulse/pcm_pulse.c | 25 +++++++++++++++++++++++++ > 1 file changed, 25 insertions(+) > > diff --git a/pulse/pcm_pulse.c b/pulse/pcm_pulse.c > index 0165120..08b6cea 100644 > --- a/pulse/pcm_pulse.c > +++ b/pulse/pcm_pulse.c > @@ -859,6 +859,30 @@ static int pulse_hw_params(snd_pcm_ioplug_t * io, > return err; > } > > +static int pulse_sw_params(snd_pcm_ioplug_t *io, snd_pcm_sw_params_t *params) > +{ > + snd_pcm_pulse_t *pcm = io->private_data; > + snd_pcm_uframes_t start_threshold; > + > + assert(pcm); > + > + if (!pcm->p || !pcm->p->mainloop) > + return -EBADFD; > + > + pa_threaded_mainloop_lock(pcm->p->mainloop); > + > + snd_pcm_sw_params_get_start_threshold(params, &start_threshold); > + > + if (start_threshold < io->period_size) > + start_threshold = io->period_size; Why do we need the above constraint? > + > + pcm->buffer_attr.prebuf = start_threshold * pcm->frame_size; This works only if sw_params is set after hw_params. It would be better if the patch could handle also if hw_params is set after sw_params. > + > + pa_threaded_mainloop_unlock(pcm->p->mainloop); > + > + return 0; > +} > + > static int pulse_close(snd_pcm_ioplug_t * io) > { > snd_pcm_pulse_t *pcm = io->private_data; > @@ -925,6 +949,7 @@ static const snd_pcm_ioplug_callback_t pulse_playback_callback = { > .poll_revents = pulse_pcm_poll_revents, > .prepare = pulse_prepare, > .hw_params = pulse_hw_params, > + .sw_params = pulse_sw_params, > .close = pulse_close, > .pause = pulse_pause > }; > -- David Henningsson, Canonical Ltd. https://launchpad.net/~diwic ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] pcm_pulse: set prebuf parameter according to software parameters 2012-11-15 8:00 ` David Henningsson @ 2012-11-16 15:49 ` Rémi Denis-Courmont 2012-11-16 16:35 ` David Henningsson 0 siblings, 1 reply; 7+ messages in thread From: Rémi Denis-Courmont @ 2012-11-16 15:49 UTC (permalink / raw) To: David Henningsson; +Cc: alsa-devel Le jeudi 15 novembre 2012 10:00:03, David Henningsson a écrit : > On 11/14/2012 07:07 PM, Rémi Denis-Courmont wrote: > > The current default value for prebuf is very high, almost the full > > virtual ALSA buffer. This breaks some application especially where > > low latency is involved. > > > > This patch makes pcm_pulse implement the sw_params callback and get > > the prebuf value from the ALSA software parameters. Thus the > > trigger latency is much more like what an ALSA application should > > expect from an ALSA PCM device. > > This seems reasonable I believe; see review comments below. > > > --- > > > > pulse/pcm_pulse.c | 25 +++++++++++++++++++++++++ > > 1 file changed, 25 insertions(+) > > > > diff --git a/pulse/pcm_pulse.c b/pulse/pcm_pulse.c > > index 0165120..08b6cea 100644 > > --- a/pulse/pcm_pulse.c > > +++ b/pulse/pcm_pulse.c > > @@ -859,6 +859,30 @@ static int pulse_hw_params(snd_pcm_ioplug_t * io, > > > > return err; > > > > } > > > > +static int pulse_sw_params(snd_pcm_ioplug_t *io, snd_pcm_sw_params_t > > *params) +{ > > + snd_pcm_pulse_t *pcm = io->private_data; > > + snd_pcm_uframes_t start_threshold; > > + > > + assert(pcm); > > + > > + if (!pcm->p || !pcm->p->mainloop) > > + return -EBADFD; > > + > > + pa_threaded_mainloop_lock(pcm->p->mainloop); > > + > > + snd_pcm_sw_params_get_start_threshold(params, &start_threshold); > > + > > + if (start_threshold < io->period_size) > > + start_threshold = io->period_size; > > Why do we need the above constraint? In my experience, PulseAudio really does not like having less than a period worth of buffered samples. I guess the code assumes there is always at least that much, and may underrun if not. But sure enough, we could remove the constraint and blame the ALSA application for using inadequate parameters if a problem occurs. What do you prefer? -- Rémi Denis-Courmont http://www.remlab.net/ ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] pcm_pulse: set prebuf parameter according to software parameters 2012-11-16 15:49 ` Rémi Denis-Courmont @ 2012-11-16 16:35 ` David Henningsson 0 siblings, 0 replies; 7+ messages in thread From: David Henningsson @ 2012-11-16 16:35 UTC (permalink / raw) To: Rémi Denis-Courmont; +Cc: alsa-devel On 11/16/2012 04:49 PM, Rémi Denis-Courmont wrote: > Le jeudi 15 novembre 2012 10:00:03, David Henningsson a écrit : >> On 11/14/2012 07:07 PM, Rémi Denis-Courmont wrote: >>> The current default value for prebuf is very high, almost the full >>> virtual ALSA buffer. This breaks some application especially where >>> low latency is involved. >>> >>> This patch makes pcm_pulse implement the sw_params callback and get >>> the prebuf value from the ALSA software parameters. Thus the >>> trigger latency is much more like what an ALSA application should >>> expect from an ALSA PCM device. >> >> This seems reasonable I believe; see review comments below. >> >>> --- >>> >>> pulse/pcm_pulse.c | 25 +++++++++++++++++++++++++ >>> 1 file changed, 25 insertions(+) >>> >>> diff --git a/pulse/pcm_pulse.c b/pulse/pcm_pulse.c >>> index 0165120..08b6cea 100644 >>> --- a/pulse/pcm_pulse.c >>> +++ b/pulse/pcm_pulse.c >>> @@ -859,6 +859,30 @@ static int pulse_hw_params(snd_pcm_ioplug_t * io, >>> >>> return err; >>> >>> } >>> >>> +static int pulse_sw_params(snd_pcm_ioplug_t *io, snd_pcm_sw_params_t >>> *params) +{ >>> + snd_pcm_pulse_t *pcm = io->private_data; >>> + snd_pcm_uframes_t start_threshold; >>> + >>> + assert(pcm); >>> + >>> + if (!pcm->p || !pcm->p->mainloop) >>> + return -EBADFD; >>> + >>> + pa_threaded_mainloop_lock(pcm->p->mainloop); >>> + >>> + snd_pcm_sw_params_get_start_threshold(params, &start_threshold); >>> + >>> + if (start_threshold < io->period_size) >>> + start_threshold = io->period_size; >> >> Why do we need the above constraint? > > In my experience, PulseAudio really does not like having less than a period > worth of buffered samples. I guess the code assumes there is always at least > that much, and may underrun if not. > > But sure enough, we could remove the constraint and blame the ALSA application > for using inadequate parameters if a problem occurs. What do you prefer? Assuming you're right about the almost-empty buffer being subject to "false underruns" (which, IIRC I confirmed a while ago), probably just a comment explaining why we need it or so. I didn't know that the limit was one period_size though. -- David Henningsson, Canonical Ltd. https://launchpad.net/~diwic ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] pcm_pulse: do not trigger immediately at start 2012-11-14 18:07 ` [PATCH 1/2] pcm_pulse: do not trigger immediately at start Rémi Denis-Courmont 2012-11-14 18:07 ` [PATCH 2/2] pcm_pulse: set prebuf parameter according to software parameters Rémi Denis-Courmont @ 2012-11-15 7:55 ` David Henningsson 1 sibling, 0 replies; 7+ messages in thread From: David Henningsson @ 2012-11-15 7:55 UTC (permalink / raw) To: Rémi Denis-Courmont; +Cc: alsa-devel Hi Remi, and thanks for your patch, this is indeed a thorny area where you change behaviour and fix one application and break another...that has at least been my experience with the alsa-pulse plugin... On 11/14/2012 07:07 PM, Rémi Denis-Courmont wrote: > There are no samples to play at that point so this is useless. I'm not following this reasoning. The start callback corresponds to the _trigger callback in kernel, which wants the playback to start immediately. For playback streams, you should not call the start callback (or any alsa-lib function that ends up calling it) from your application unless there are already samples in the buffer, or you will get an immediate underrun. For recording streams, starting with an empty buffer is expected and not useless at all. > The non-zero buffer_attr.prebuf ensures that the PulseAudio daemon will > start streaming automatically (like ALSA sw_params start_threshold). The start callback is for the cases where the application wants to explicitly start the stream before prebuf has been reached. > --- > pulse/pcm_pulse.c | 12 +++--------- > 1 file changed, 3 insertions(+), 9 deletions(-) > > diff --git a/pulse/pcm_pulse.c b/pulse/pcm_pulse.c > index 24fd4da..0165120 100644 > --- a/pulse/pcm_pulse.c > +++ b/pulse/pcm_pulse.c > @@ -204,8 +204,8 @@ static void stream_success_cb(pa_stream * p, int success, void *userdata) > static int pulse_start(snd_pcm_ioplug_t * io) > { > snd_pcm_pulse_t *pcm = io->private_data; > - pa_operation *o, *u; > - int err = 0, err_o = 0, err_u = 0; > + pa_operation *o; > + int err = 0, err_o = 0; > > assert(pcm); > > @@ -224,18 +224,12 @@ static int pulse_start(snd_pcm_ioplug_t * io) > goto finish; > } > > - u = pa_stream_trigger(pcm->stream, stream_success_cb, pcm); > - > pcm->underrun = 0; > err_o = pulse_wait_operation(pcm->p, o); > - if (u) > - err_u = pulse_wait_operation(pcm->p, u); > > pa_operation_unref(o); > - if (u) > - pa_operation_unref(u); > > - if (err_o < 0 || err_u < 0) { > + if (err_o < 0) { > err = -EIO; > goto finish; > } > -- David Henningsson, Canonical Ltd. https://launchpad.net/~diwic ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2012-11-16 16:35 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2012-11-14 18:07 [PATCH 0/2] alsa-plugins: fix PulseAudio prebuf Rémi Denis-Courmont 2012-11-14 18:07 ` [PATCH 1/2] pcm_pulse: do not trigger immediately at start Rémi Denis-Courmont 2012-11-14 18:07 ` [PATCH 2/2] pcm_pulse: set prebuf parameter according to software parameters Rémi Denis-Courmont 2012-11-15 8:00 ` David Henningsson 2012-11-16 15:49 ` Rémi Denis-Courmont 2012-11-16 16:35 ` David Henningsson 2012-11-15 7:55 ` [PATCH 1/2] pcm_pulse: do not trigger immediately at start David Henningsson
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.