[pulseaudio-discuss] [PATCH] alsa-util: Set ALSA report_delay flag in pa_alsa_safe_delay()

[Georg Chini]

On 04.04.2018 00:35, Pierre-Louis Bossart wrote:
> On 4/2/18 3:14 PM, Georg Chini wrote:
>> On 02.04.2018 21:35, Pierre-Louis Bossart wrote:
>>>
>>>
>>> On 04/02/2018 07:54 AM, Georg Chini wrote:
>>>> The current code does not call 
>>>> snd_pcm_status_set_audio_htstamp_config()
>>>> to configure the way timestamps are updated in ALSA. This leads to
>>>> incorrect time stamps in the status object returned by 
>>>> snd_pcm_status(),
>>>> so the computed latencies are wrong.
>>>>
>>>> This patch uses snd_pcm_status_set_audio_htstamp_config() to set the
>>>> ALSA report_delay flag to 1 before the call to snd_pcm_status(). With
>>>> this, time stamps are updated as expected.
>>>> ---
>>>>   src/modules/alsa/alsa-util.c | 7 +++++++
>>>>   1 file changed, 7 insertions(+)
>>>>
>>>> diff --git a/src/modules/alsa/alsa-util.c 
>>>> b/src/modules/alsa/alsa-util.c
>>>> index 61fb4903..b91a0e98 100644
>>>> --- a/src/modules/alsa/alsa-util.c
>>>> +++ b/src/modules/alsa/alsa-util.c
>>>> @@ -1187,6 +1187,7 @@ int pa_alsa_safe_delay(snd_pcm_t *pcm, 
>>>> snd_pcm_status_t *status, snd_pcm_sframes
>>>>       size_t abs_k;
>>>>       int err;
>>>>       snd_pcm_sframes_t avail = 0;
>>>> +    snd_pcm_audio_tstamp_config_t tstamp_config;
>>>>         pa_assert(pcm);
>>>>       pa_assert(delay);
>>>> @@ -1200,6 +1201,12 @@ int pa_alsa_safe_delay(snd_pcm_t *pcm, 
>>>> snd_pcm_status_t *status, snd_pcm_sframes
>>>>        * avail, delay and timestamp values in a single kernel call 
>>>> to improve
>>>>        * timer-based scheduling */
>>>>   +    /* The time stamp configuration needs to be set so that the
>>>> +     * ALSA code will use the internal delay reported by the 
>>>> driver */
>>>> +    tstamp_config.type_requested = 1; /* ALSA default time stamp 
>>>> type */
>>>> +    tstamp_config.report_delay = 1;
>>>> +    snd_pcm_status_set_audio_htstamp_config(status, &tstamp_config);
>>>> +
>>> are you sure it's necessary or is this possibly a misunderstanding 
>>> of what audio_tstamps are?
>>>
>>> this command is only for the audio timestamp, and to the best of my 
>>> knowledge you are not using the results using one of the 
>>> snd_pcm_status_get_audio_htstamp_* commands
>>>
>>> the typical usage (see alsa-lib/test/audio_time.c) is this:
>>>
>>>     snd_pcm_status_set_audio_htstamp_config(status, 
>>> audio_tstamp_config);
>>>
>>>     if ((err = snd_pcm_status(handle, status)) < 0) {
>>>         printf("Stream status error: %s\n", snd_strerror(err));
>>>         exit(0);
>>>     }
>>>     snd_pcm_status_get_trigger_htstamp(status, trigger_timestamp);
>>>     snd_pcm_status_get_htstamp(status, timestamp);
>>>     snd_pcm_status_get_audio_htstamp(status, audio_timestamp);
>>>     snd_pcm_status_get_audio_htstamp_report(status, 
>>> audio_tstamp_report);
>>>
>>> if you are not using the _get_audio_hstamp() then the config has 
>>> essentially no effect, and the delay is available separately in the 
>>> status command as before.
>>>
>>>>       if ((err = snd_pcm_status(pcm, status)) < 0)
>>>>           return err;
>>
>> See this bug report, why it is needed:
>>
>> https://bugzilla.kernel.org/show_bug.cgi?id=199235
>>
>> It finally turned out that there was not a bug but just the flag 
>> missing.
>> We are using  snd_pcm_status_get_htstamp() with the time smoother
>> to calculate sink/source latency.
>
> Humm, that looks more like a bug in the fix (20e3f9 'ALSA: pcm: update 
> tstamp only in audio_tstamp changed'). I don't think we intended that 
> changes in the way the audio_tstamp is calculated (with or without 
> delay) would impact when the system timestamp is updated. I am pretty 
> sure we only wanted to update the timestamp when the hw_ptr changed 
> with this fix so as to go back to the traditional behavior before 
> kernel 4.1.
>
> Can you check if just using tstamp_config.type_requested = 1; isn't 
> enough for PulseAudio? If not, we have two conflicting desires on when 
> the system timestamp should be updated.
>
>
>
It isn't enough. For the time stamp to be updated you need to have 
report_delay set.