On 04.04.2018 08:01, Takashi Iwai wrote:
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
As already said in the bug discussion, I think the right way to fix
the issue is to exclude runtime->delay from the delay estimation
when report_delay is not set. What caused the problems for me
was that the delay snapshot was taken at another time than the
time stamp indicated. snd_pcm_status_get_delay() includes
the delay, while audio time stamp does not and time stamp is
kept at the time of the last audio time stamp update. So the
values are inconsistent which renders the returned delay unusable
if combined with the time stamp.
If the returned delay is also kept at the value corresponding to the
time stamp, it would be all consistent. This does not match the
historical behavior, however.
Regards
Georg
_______________________________________________
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
