On Sun, 29 Mar 2020 09:43:13 +0200, Takashi Iwai wrote:
On Sat, 28 Mar 2020 23:20:21 +0100, sylvain.bertrand@gmail.com wrote:
On Sat, Mar 28, 2020 at 04:34:01PM -0500, Pierre-Louis Bossart wrote:
Using MONOTONIC_RAW is very nice on paper, until you realize you can't program a timer using the information. You can only read the timestamp and not really do much if you want to sleep/wait.
In practice, if you really really need super-precise information you'll get use rdtsc(), and apply you own formulas. And otherwise stick with MONOTONIC, it's rather unlikely you will ever notice the NTP changes. PulseAudio, CRAS and a number of Android HALs use MONOTONIC and nobody ever complained.
The pb is not about using monotonic_raw, the thing is: it is documented valid to use it which I did as expected from a naive reading of the api documentation and found those issues. I can reasonably believe it will be the case for any new alsa programmer.
For my code, in the end, I think I'll use the best "audio timestamp" I can get from the status ioctl for linear interpolation with ffmpeg timestamps.
But this is off topic here.
The topic is discussing how to fix this bug, since I had to dig a bit in alsa. It appears to me the recursive fix might be a good way, since it is done for other api functions, but I am not Jaroslav Kysela neither Takashi Iwai then far from grasping all the details of alsa.
A problem for now is that we used to allow the arbitrary parameter in sw_params because it's sw_params. Propagating an error would break this assumption, and that's the (rather only) concern when we introduce the error for an invalid tstamp type.
OTOH, although the translation of timestamp can work around this compatibility problem, it would result in an inaccurate timing that applications don't expect, either; apps set up a different tstamp type just because they want accurate timing, after all, so it'd becomes rather useless.
So, judging from the both above, I find returning an error is a better approach. Above all, it's simpler.
And for dmix, we may add a new asoundrc option to specify the tstamp type. sw_params returns an error if an incompatible tstamp type is specified. This will leave users the least possibility to use the expected tstamp type while keeping the system consistent.
Below is a totally untested patch. Let me know if this works.
thanks,
Takashi
--- diff --git a/src/pcm/pcm_direct.c b/src/pcm/pcm_direct.c index 54d99005461b..37dc30780623 100644 --- a/src/pcm/pcm_direct.c +++ b/src/pcm/pcm_direct.c @@ -991,8 +991,13 @@ int snd_pcm_direct_hw_free(snd_pcm_t *pcm ATTRIBUTE_UNUSED) return 0; }
-int snd_pcm_direct_sw_params(snd_pcm_t *pcm ATTRIBUTE_UNUSED, snd_pcm_sw_params_t * params ATTRIBUTE_UNUSED) +int snd_pcm_direct_sw_params(snd_pcm_t *pcm, snd_pcm_sw_params_t *params) { + snd_pcm_direct_t *dmix = pcm->private_data; + + if ((int)params->tstamp_type != dmix->tstamp_type) + return -EINVAL; + /* values are cached in the pcm structure */ return 0; } @@ -1318,6 +1323,15 @@ int snd_pcm_direct_initialize_slave(snd_pcm_direct_t *dmix, snd_pcm_t *spcm, str return ret; }
+ if (dmix->tstamp_type != -1) { + ret = snd_pcm_sw_params_set_tstamp_type(spcm, &sw_params, + dmix->tstamp_type); + if (ret < 0) { + SNDERR("unable to set tstamp type"); + return ret; + } + } + if (dmix->type != SND_PCM_TYPE_DMIX && dmix->type != SND_PCM_TYPE_DSHARE) goto __skip_silencing; @@ -1341,6 +1355,9 @@ int snd_pcm_direct_initialize_slave(snd_pcm_direct_t *dmix, snd_pcm_t *spcm, str return ret; }
+ /* copy back the slave setup */ + dmix->tstamp_type = sw_params.tstamp_type; + if (dmix->type == SND_PCM_TYPE_DSHARE) { const snd_pcm_channel_area_t *dst_areas; dst_areas = snd_pcm_mmap_areas(spcm); @@ -1878,6 +1895,7 @@ int snd_pcm_direct_parse_open_conf(snd_config_t *root, snd_config_t *conf, rec->var_periodsize = 0; rec->direct_memory_access = 1; rec->hw_ptr_alignment = SND_PCM_HW_PTR_ALIGNMENT_AUTO; + rec->tstamp_type = -1;
/* read defaults */ if (snd_config_search(root, "defaults.pcm.dmix_max_periods", &n) >= 0) { @@ -1941,6 +1959,27 @@ int snd_pcm_direct_parse_open_conf(snd_config_t *root, snd_config_t *conf,
continue; } + if (strcmp(id, "tstamp_type") == 0) { + const char *str; + err = snd_config_get_string(n, &str); + if (err < 0) { + SNDERR("Invalid type for %s", id); + return -EINVAL; + } + if (strcmp(str, "default") == 0) + rec->tstamp_type = -1; + else if (strcmp(str, "gettimeofday") == 0) + rec->tstamp_type = SND_PCM_TSTAMP_TYPE_GETTIMEOFDAY; + else if (strcmp(str, "monotonic") == 0) + rec->tstamp_type = SND_PCM_TSTAMP_TYPE_MONOTONIC; + else if (strcmp(str, "monotonic_raw") == 0) + rec->tstamp_type = SND_PCM_TSTAMP_TYPE_MONOTONIC_RAW; + else { + SNDERR("The field tstamp_type is invalid : %s", str); + return -EINVAL; + } + continue; + } if (strcmp(id, "ipc_gid") == 0) { char *group; char *endp; diff --git a/src/pcm/pcm_direct.h b/src/pcm/pcm_direct.h index 221edbe16879..63dd645ba45a 100644 --- a/src/pcm/pcm_direct.h +++ b/src/pcm/pcm_direct.h @@ -173,6 +173,7 @@ struct snd_pcm_direct { unsigned int recoveries; /* mirror of executed recoveries on slave */ int direct_memory_access; /* use arch-optimized buffer RW */ snd_pcm_direct_hw_ptr_alignment_t hw_ptr_alignment; + int tstamp_type; union { struct { int shmid_sum; /* IPC global sum ring buffer memory identification */ @@ -357,6 +358,7 @@ struct snd_pcm_direct_open_conf { int var_periodsize; int direct_memory_access; snd_pcm_direct_hw_ptr_alignment_t hw_ptr_alignment; + int tstamp_type; snd_config_t *slave; snd_config_t *bindings; }; diff --git a/src/pcm/pcm_dmix.c b/src/pcm/pcm_dmix.c index d533f40c5892..410f34a63d54 100644 --- a/src/pcm/pcm_dmix.c +++ b/src/pcm/pcm_dmix.c @@ -1237,6 +1237,9 @@ pcm.name { # roundup # rounddown # auto (default) + tstamp_type STR # timestamp type + # STR can be one of the below strings : + # default, gettimeofday, monotonic, monotonic_raw slave STR # or slave { # Slave definition diff --git a/src/pcm/pcm_dshare.c b/src/pcm/pcm_dshare.c index 59448cfb5883..f497c00a360b 100644 --- a/src/pcm/pcm_dshare.c +++ b/src/pcm/pcm_dshare.c @@ -929,6 +929,9 @@ pcm.name { # roundup # rounddown # auto (default) + tstamp_type STR # timestamp type + # STR can be one of the below strings : + # default, gettimeofday, monotonic, monotonic_raw slave STR # or slave { # Slave definition diff --git a/src/pcm/pcm_dsnoop.c b/src/pcm/pcm_dsnoop.c index 24f472c72c8e..bbb4a344dd9d 100644 --- a/src/pcm/pcm_dsnoop.c +++ b/src/pcm/pcm_dsnoop.c @@ -780,6 +780,9 @@ pcm.name { # roundup # rounddown # auto (default) + tstamp_type STR # timestamp type + # STR can be one of the below strings : + # default, gettimeofday, monotonic, monotonic_raw slave STR # or slave { # Slave definition diff --git a/src/pcm/pcm_local.h b/src/pcm/pcm_local.h index 05ed935f1f16..89d4125b875d 100644 --- a/src/pcm/pcm_local.h +++ b/src/pcm/pcm_local.h @@ -928,6 +928,8 @@ int INTERNAL(snd_pcm_hw_params_set_buffer_size_last)(snd_pcm_t *pcm, snd_pcm_hw_
int snd_pcm_sw_params_set_tstamp_mode(snd_pcm_t *pcm, snd_pcm_sw_params_t *params, snd_pcm_tstamp_t val); int INTERNAL(snd_pcm_sw_params_get_tstamp_mode)(const snd_pcm_sw_params_t *params, snd_pcm_tstamp_t *val); +int snd_pcm_sw_params_set_tstamp_type(snd_pcm_t *pcm, snd_pcm_sw_params_t *params, snd_pcm_tstamp_type_t val); +int snd_pcm_sw_params_get_tstamp_type(const snd_pcm_sw_params_t *params, snd_pcm_tstamp_type_t *val); int snd_pcm_sw_params_set_avail_min(snd_pcm_t *pcm, snd_pcm_sw_params_t *params, snd_pcm_uframes_t val); int INTERNAL(snd_pcm_sw_params_get_avail_min)(const snd_pcm_sw_params_t *params, snd_pcm_uframes_t *val); int snd_pcm_sw_params_set_start_threshold(snd_pcm_t *pcm, snd_pcm_sw_params_t *params, snd_pcm_uframes_t val);