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.
