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

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

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