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

[Takashi Iwai]

On Wed, 04 Apr 2018 00:35:24 +0200,
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.

The fact here is that hwptr still remains same but only the delay
changes.  As the prior-4.1 traditional behavior, the timestamp isn't
updated in such a case.  Before the commit, the timestamp is always 
updated no matter whether hwptr is updated or not, and it broke
applications.

So, the question is how we should look at the timestamp.  The fix is
based on the assumption that tstamp is always coupled with
audio_tstamp, where the latter calculation depends on the tstamp
config flag.

OTOH, if we take rather audio_tstamp always coupled with the
hwptr+delay, we may fix the code in a different way, too.  But this
would need to remember the last delay, and moreover, I don't think
it's better interpretation -- if we read tstamp in that way, what the
heck audio_tstamp is for?


thanks,

Takashi