| Message ID | 577FC0C4.1060508@IEE.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 7/8/16 10:03 AM, Alan Young wrote: > On 07/06/16 07:44, Alan Young wrote: >> I'll work on developing and testing a patch for consideration before >> coming back to the last. It will be easier to discuss the merits or >> otherwise of my proposal with a concrete, working patch to consider. > > > Well, it has been a few weeks but this is what I have come up with. > > It is not perfect because of the issue noted in the comments but so far > I have not been able to discover any downside. It many (most) cases, the > reported delay (and audio_tstamp) is more accurate than it was before. > In other cases there is no change. I just looked at the code and I am probably missing something. in update_delay() you apply a delta between the last timestamp and the current one and modify the runtime->delay. Immediately after, in update_audio_tstamp() runtime->delay is used as well to compute audio_frames which in turn is used to find the audio_tstamp, on which another delta between current tstamp and last timestamp is applied. Overall it looks like you are correcting twice for the same delay? Even if this was correct, you would want to make sure the delta is positive to keep audio timestamps monotonous.
On 15/07/16 21:13, Pierre-Louis Bossart wrote: > in update_delay() you apply a delta between the last timestamp and the > current one and modify the runtime->delay. > > Immediately after, in update_audio_tstamp() runtime->delay is used as > well to compute audio_frames which in turn is used to find the > audio_tstamp, on which another delta between current tstamp and last > timestamp is applied. > > Overall it looks like you are correcting twice for the same delay? > In update_audio_tstamp() it either usedruntime->delay, if runtime->audio_tstamp_config.report_delay is true, or applies a delta - not both. > Even if this was correct, you would want to make sure the delta is > positive to keep audio timestamps monotonous. Hmm, maybe. In what circumstances could the delay ever be negative?
On 7/19/16 10:33 AM, Alan Young wrote: > On 15/07/16 21:13, Pierre-Louis Bossart wrote: >> in update_delay() you apply a delta between the last timestamp and the >> current one and modify the runtime->delay. >> >> Immediately after, in update_audio_tstamp() runtime->delay is used as >> well to compute audio_frames which in turn is used to find the >> audio_tstamp, on which another delta between current tstamp and last >> timestamp is applied. >> >> Overall it looks like you are correcting twice for the same delay? >> > In update_audio_tstamp() it either usedruntime->delay, if > runtime->audio_tstamp_config.report_delay is true, or applies a delta - > not both. ah yes, I did miss it in the code. maybe a comment would be nice to avoid being thrown. I still have mixed feelings about the code, it seems to make sense for the case where the .pointer is updated at every period, but it's not using the regular BATCH definition. With the tests such as runtime->status->hw_ptr == runtime->hw_ptr_interrupt you could end-up modifying the results by a small amount for other hardware platforms depending on when the timestamp is taken (in other words scheduling latency would affect audio timestamps). > >> Even if this was correct, you would want to make sure the delta is >> positive to keep audio timestamps monotonous. > > Hmm, maybe. In what circumstances could the delay ever be negative? if your timestamps are REALTIME since they can jump backwards. The expectation is to use MONOTONOUS (or better MONOTONOUS_RAW to avoid NTP corrections), but with the ALSA API the application can choose REALTIME.
On 19/07/16 16:58, Pierre-Louis Bossart wrote: >> In update_audio_tstamp() it either usedruntime->delay, if >> runtime->audio_tstamp_config.report_delay is true, or applies a delta - >> not both. > > ah yes, I did miss it in the code. maybe a comment would be nice to > avoid being thrown. ok > I still have mixed feelings about the code, it seems to make sense for > the case where the .pointer is updated at every period, but it's not > using the regular BATCH definition. With the tests such as > runtime->status->hw_ptr == runtime->hw_ptr_interrupt you could end-up > modifying the results by a small amount for other hardware platforms > depending on when the timestamp is taken (in other words scheduling > latency would affect audio timestamps). > Yes, that could be true - there could be some jitter - but I think it will still result in more accurate results. Note that the adjustment to the reported audio_tstamp will only occur for the AUDIO_TSTAMP_TYPE_DEFAULT case and when the platform has not updated the (hw_ptr) position outside of the interrupt callback independent of whether the BATCH flag is used. There is actually an argument for being less restrictive. Hardware platform updates to position, where they happen outside of an interrupt, may (generally will) be less accurate than the update mechanism that I propose because such position updates are mostly restricted to the level of DMA residue granularity, which is relatively coarse (usually). > > if your timestamps are REALTIME since they can jump backwards. The > expectation is to use MONOTONOUS (or better MONOTONOUS_RAW to avoid > NTP corrections), but with the ALSA API the application can choose > REALTIME. Ok, I'll put in a check. Of course there are cases where one might actually want REALTIME. Note: For my application, I only actually care about the changes implemented using update_delay(). The refinement to update_audio_tstamp() just seemed to me to be part of the same issue. If the update_audio_tstamp() change is considered too controversial then I'd be happy to drop it.
On 7/20/16 1:59 AM, Alan Young wrote: > On 19/07/16 16:58, Pierre-Louis Bossart wrote: >>> In update_audio_tstamp() it either usedruntime->delay, if >>> runtime->audio_tstamp_config.report_delay is true, or applies a delta - >>> not both. >> >> ah yes, I did miss it in the code. maybe a comment would be nice to >> avoid being thrown. > ok > >> I still have mixed feelings about the code, it seems to make sense for >> the case where the .pointer is updated at every period, but it's not >> using the regular BATCH definition. With the tests such as >> runtime->status->hw_ptr == runtime->hw_ptr_interrupt you could end-up >> modifying the results by a small amount for other hardware platforms >> depending on when the timestamp is taken (in other words scheduling >> latency would affect audio timestamps). >> > > Yes, that could be true - there could be some jitter - but I think it > will still result in more accurate results. Note that the adjustment to > the reported audio_tstamp will only occur for the > AUDIO_TSTAMP_TYPE_DEFAULT case and when the platform has not updated the > (hw_ptr) position outside of the interrupt callback independent of > whether the BATCH flag is used. > > There is actually an argument for being less restrictive. Hardware > platform updates to position, where they happen outside of an interrupt, > may (generally will) be less accurate than the update mechanism that I > propose because such position updates are mostly restricted to the level > of DMA residue granularity, which is relatively coarse (usually). I am not hot on changing a default behavior and end-up with platforms getting worse results and some getting better. It'd really be better if you used a new timestamp (I added the LINK_ESTIMATED_ATIME that isn't used by anyone and could be reclaimed) and modified the delay estimation in your own driver rather than in the core. > >> >> if your timestamps are REALTIME since they can jump backwards. The >> expectation is to use MONOTONOUS (or better MONOTONOUS_RAW to avoid >> NTP corrections), but with the ALSA API the application can choose >> REALTIME. > > Ok, I'll put in a check. Of course there are cases where one might > actually want REALTIME. > > Note: For my application, I only actually care about the changes > implemented using update_delay(). The refinement to > update_audio_tstamp() just seemed to me to be part of the same issue. If > the update_audio_tstamp() change is considered too controversial then > I'd be happy to drop it. if you change the delay by default then it changes the audio timestamp as well, not sure how you can isolate the two parts.
Hi Pierre, Thanks for your continued engagement on this thread. On 01/08/16 22:56, Pierre-Louis Bossart wrote: > On 7/20/16 1:59 AM, Alan Young wrote: >> >> Yes, that could be true - there could be some jitter - but I think it >> will still result in more accurate results. Note that the adjustment to >> the reported audio_tstamp will only occur for the >> AUDIO_TSTAMP_TYPE_DEFAULT case and when the platform has not updated the >> (hw_ptr) position outside of the interrupt callback independent of >> whether the BATCH flag is used. >> >> There is actually an argument for being less restrictive. Hardware >> platform updates to position, where they happen outside of an interrupt, >> may (generally will) be less accurate than the update mechanism that I >> propose because such position updates are mostly restricted to the level >> of DMA residue granularity, which is relatively coarse (usually). > > I am not hot on changing a default behavior and end-up with platforms > getting worse results and some getting better. I am not sure that any platforms would get worse results (notwithstanding the jitter point above). Some would get better results. > > It'd really be better if you used a new timestamp (I added the > LINK_ESTIMATED_ATIME that isn't used by anyone and could be reclaimed) > and modified the delay estimation in your own driver rather than in > the core. > Well, I'm not looking at a single driver here. I am looking at several that use large parts of the common soc framework in various ways. I'll look at LINK_ESTIMATED_ATIME and see if I could adopt that. I'm not sure how much it will help with the delay calculation but I suspect that the right answer could be deduced. >> Note: For my application, I only actually care about the changes >> implemented using update_delay(). The refinement to >> update_audio_tstamp() just seemed to me to be part of the same issue. If >> the update_audio_tstamp() change is considered too controversial then >> I'd be happy to drop it. > > if you change the delay by default then it changes the audio timestamp > as well, not sure how you can isolate the two parts. > It only changes the audio timestamp if the user requests that the delay be included in it. Stepping back for a moment, the delay calculation essentially consists of two parts: 1. How much data is still in the ring buffer. 2. How much data has been removed from the ring buffer but not yet played out. In many respects it is artificial to separate these but that is what the API does. In some cases the first factor is dominant, because DMA is consuming the buffer and one has - at the very best - only coarse-grained data about the position at any moment. It is unlikely ever to be sample-position accurate and, for most platforms, is much poorer. In some cases the second factor is dominant because some data has been taken from the ring buffer and is then in some other significant size buffer. USB is a good example and, in that case, one can see that the generic driver does indeed used an elapsed-time calculation to generate (estimate) the delay report. The more that I think about it the more it seems to me that using a time-based estimate for position (hw_ptr), outside of an interrupt callback, will always be more accurate than that returned by substream->ops->pointer(). Perhaps the results of that call should simply be ignored outside an interrupt callback - the call not even made, so as not to pollute the estimate with changed delay data. Alan.
Alan Young wrote: > Stepping back for a moment, the delay calculation essentially > consists of two parts: > > 1. How much data is still in the ring buffer. > 2. How much data has been removed from the ring buffer but not yet > played out. > [...] > The more that I think about it the more it seems to me that using > a time-based estimate for position (hw_ptr), outside of an interrupt > callback, will always be more accurate than that returned by > substream->ops->pointer(). Please note that "hw_ptr" (or "avail" in the API) specifies the DMA position, i.e., it is guaranteed that the samples up to this position have been handled by the DMA controller and can now be accessed by the program. An estimate that gets this wrong can lead to data corruption. Using an estimate for the delay is perfectly fine. Regards, Clemens
>> It'd really be better if you used a new timestamp (I added the >> LINK_ESTIMATED_ATIME that isn't used by anyone and could be reclaimed) >> and modified the delay estimation in your own driver rather than in >> the core. >> > > Well, I'm not looking at a single driver here. I am looking at several > that use large parts of the common soc framework in various ways. > > I'll look at LINK_ESTIMATED_ATIME and see if I could adopt that. I'm not > sure how much it will help with the delay calculation but I suspect that > the right answer could be deduced. The LINK timestamps are supposed to be read from hardware counters close to the interface for better accuracy. When I added the LINK_ESTIMATED part, I thought this could be useful when those counters are not supported, but realized that if the delay is accurate you can just as well use the default method of adding the delay information to the DMA position to get the timestamps - there is really no benefit in defining a new timestamp type. In the case where the delay is not accurate (on the platforms you are experimenting with) then this might be the right solution. And we could add this timestamp in the core rather than in each driver for simplicity. It would be interesting if you share results with different timestamps to see if there is any benefit (with e.g. alsa-lib/test/audio_time.c) > The more that I think about it the more it seems to me that using a > time-based estimate for position (hw_ptr), outside of an interrupt > callback, will always be more accurate than that returned by > substream->ops->pointer(). Perhaps the results of that call should > simply be ignored outside an interrupt callback - the call not even > made, so as not to pollute the estimate with changed delay data. then you'd have to account for drift between audio and timer source, it's not simpler at all and as Clemens wrote can lead to corrupted pointers if you screw up.
From 0c1378b8faa91e7bebf63a60ece3c5c942652a53 Mon Sep 17 00:00:00 2001 From: Alan Young <consult.awy@gmail.com> Date: Fri, 8 Jul 2016 14:08:10 +0100 Subject: [PATCH] Improve accuracy of delay and audio_tstamp reporting. pcm_lib.c:snd_pcm_update_hw_ptr0() is responsible for updating the data used for various status reporting calls. Many drivers do not update the position upon a call to substream->ops->pointer() other then within an interrupt callback. Most drivers also do not update substream->runtime->delay (in the pointer() call) -- the main exception being USB drivers. Consequently reported delay and audio_tstamp values will, in these cases, be inaccurate by the time elapsed since the last interrupt callback. By recording the delay at the time of an interrupt callback, and the timestamp at that time, the reported delay and audio_tstamp values can be adjusted to compensate for the time elapsed since the last interrupt. These adjustments are only made if the reported position or delay (as appropriate) have not changed since those recorded at the time of that interrupt. This approach fails if reported delay is adjusted to account from some data (such as the CODEC delay) but the position is not adjusted to the (probably more significant) current DMA offset. --- include/sound/pcm.h | 2 ++ sound/core/pcm_lib.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 50 insertions(+) diff --git a/include/sound/pcm.h b/include/sound/pcm.h index b0be092..55da224 100644 --- a/include/sound/pcm.h +++ b/include/sound/pcm.h @@ -417,6 +417,8 @@ struct snd_pcm_runtime { struct snd_pcm_audio_tstamp_config audio_tstamp_config; struct snd_pcm_audio_tstamp_report audio_tstamp_report; struct timespec driver_tstamp; + snd_pcm_sframes_t interrupt_delay; + struct timespec interrupt_tstamp; #if defined(CONFIG_SND_PCM_OSS) || defined(CONFIG_SND_PCM_OSS_MODULE) /* -- OSS things -- */ diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c index 6b5a811..96cde4e 100644 --- a/sound/core/pcm_lib.c +++ b/sound/core/pcm_lib.c @@ -263,6 +263,12 @@ static void update_audio_tstamp(struct snd_pcm_substream *substream, audio_nsecs = div_u64(audio_frames * 1000000000LL, runtime->rate); *audio_tstamp = ns_to_timespec(audio_nsecs); + + if (!runtime->audio_tstamp_config.report_delay && runtime->interrupt_tstamp.tv_sec + && runtime->status->hw_ptr == runtime->hw_ptr_interrupt) { + struct timespec delta_time = timespec_sub(*curr_tstamp, runtime->interrupt_tstamp); + *audio_tstamp = timespec_add(*audio_tstamp, delta_time); + } } runtime->status->audio_tstamp = *audio_tstamp; runtime->status->tstamp = *curr_tstamp; @@ -275,6 +281,40 @@ static void update_audio_tstamp(struct snd_pcm_substream *substream, runtime->driver_tstamp = driver_tstamp; } +static void update_delay(struct snd_pcm_substream *substream, + struct timespec *curr_tstamp) +{ + struct snd_pcm_runtime *runtime = substream->runtime; + struct timespec delta_time; + snd_pcm_sframes_t delta; + + /* Can only adjust the delay if we have base timestamp. */ + if (runtime->tstamp_mode != SNDRV_PCM_TSTAMP_ENABLE || !runtime->interrupt_tstamp.tv_sec) + return; + + if (runtime->delay != runtime->interrupt_delay) { + /* + * Assume accurate if changed, + * which is not correct if driver supports variable + * codec delay reporting or similar + */ + return; + } + + delta_time = timespec_sub(*curr_tstamp, runtime->interrupt_tstamp); + delta = (delta_time.tv_sec * USEC_PER_SEC + delta_time.tv_nsec / NSEC_PER_USEC) + * runtime->rate / USEC_PER_SEC; + + /* sanity check */ + if (delta > runtime->period_size) + return; + + if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) + runtime->delay = runtime->interrupt_delay - delta; + else + runtime->delay = runtime->interrupt_delay + delta; +} + static int snd_pcm_update_hw_ptr0(struct snd_pcm_substream *substream, unsigned int in_interrupt) { @@ -311,6 +351,11 @@ static int snd_pcm_update_hw_ptr0(struct snd_pcm_substream *substream, snd_pcm_gettime(runtime, (struct timespec *)&curr_tstamp); } else snd_pcm_gettime(runtime, (struct timespec *)&curr_tstamp); + + if (in_interrupt) { + runtime->interrupt_delay = runtime->delay; + runtime->interrupt_tstamp = curr_tstamp; + } } if (pos == SNDRV_PCM_POS_XRUN) { @@ -453,6 +498,9 @@ static int snd_pcm_update_hw_ptr0(struct snd_pcm_substream *substream, } no_delta_check: + if (!in_interrupt && new_hw_ptr == runtime->hw_ptr_interrupt) { + update_delay(substream, &curr_tstamp); + } if (runtime->status->hw_ptr == new_hw_ptr) { update_audio_tstamp(substream, &curr_tstamp, &audio_tstamp); return 0; -- 2.5.5