A few more items that came to my mind later:
It'd be better to align both input and output; namely, the input struct and output struct must be compatible. Currently, report_delay flag is overwritten in return. This bit should be kept upon read back.
This essentially unifies snd_pcm_audio_tstamp_config and snd_pcm_audio_tstamp_report. We don't have to pass two structs to callbacks.
ok. this is what I had before and I thought it'd be cleaner to clearly make the distinction between config and report. no issue changing this.
Or, we may avoid packing/unpacking by defining the struct like:
unsigned char type; unsigned char flags; unsigned char accuracy_mantessa; unsigned char accuracy_exponent;where flags field contains the bit flags for report_delay and accuracy_report.
That said, I'm worrying a bit about the complexity of the new callback function.
I would prefer a single structure if we want to add something later.
- The biggest concern I have now is whether it's really feasible to change the semantics of snd_pcm_status ioctl, i.e. from the read-only to the write-read. I guess this would work as the padding field is very likely zero even in the very old alsa-lib version.
The change doesn't affect anyone really, all my defaults are zero.
- Another concern is the compatibility with the current wallclock implementation. Judging from your patch, the audio_tstamp won't be obtained from get_time_info callback in the default tstamp mode, right? This may result in a regression, as currently the driver always gives the h/w audio_tstamp when the driver supports the wallclock.
Is this that big of a deal? To the best of my knowledge this wallclk thing was implemented for HDaudio only when we were prototyping the new hardware, and I don't think we ended-up contributing the corresponding patches for PulseAudio. We've since realized that the wallclock can't be available in all cases and that we need this selection capability in a variety of cases.
Also even if we kept the .wall_clock callback, the wallclock handling could be relative (start at zero) or absolute. I implemented a reset to zero on stream startup, since the counter is not maintained when the hardware is idle, but there are implementations where the wallclock is really absolute and not reset (see below).
Last but not least: we're receiving multiple enhancement requests regarding tstamp at the very same time. This patchset conflicts with Tim and Nick's start_at extention.
I believe this can be resolved later, but let's discuss the ground line at first: the requirement and influence on both changes.
I am aware of this and it's why I posted my patches earlier than planned to avoid merging different concepts later, it's probably best to have compatibility from day1.
My proposal was to have a start_at functionality based on the timestamp definitions I suggested and keep audio and system timestamps separate rather than add mixed typestamps such as SND_PCM_TSTAMP_TYPE_AUDIO_WALLCLOCK. the code could be something like:
start_at(typestamp type, timestamp subtype, timespec value ) {
if (type == SYSTEM) { _start_using_hrtimers(subtype, value) // would be CLOCK_REALTIME, MONOTONIC, maybe RAW } else if (type == AUDIO) { if (subtype == ABSOLUTE_WALLCLOCK) // not reset on audio subsystem startup _start_using_hardware(value) else // not sure what to do with regular counters, probably bail. error; }
That way you can set what sort of system timestamp and what sort of audio timestamp you want reported by snd_pcm_status, and you can independently select the start timestamp of your choice.