Re: [PATCH 2/2] pcm_pulse: set prebuf parameter according to software parameters

[David Henningsson <david.henningsson@canonical.com>]

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