| Message ID | 20180404135744.3921-1-georg@chini.tk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 04/04/2018 08:57 AM, Georg Chini wrote: > Currently, the kernel uses only the number of USB frames to interpolate the delay > between sending or receiving URB's. This limits the granuarity of the delay reports > to one millisecond because the frame counter is updated in a millisecond interval. > > This patch additionally uses time stamps to achieve higher precision of the reported > delay. If the time difference computed using time stamps does not deviate more than > one millisecond from the difference computed using frames, it is assumed that the > system time difference delivers the more precise delay estimate. > > This will significantly increase the precision of the reported delay in the majority > of cases where sound card clock and system clock are almost in sync while falling back > to the old method for the rare cases where the two times differ too much. I am all for better delay estimates but I have mixed feelings about this - and some hardware-related parts are broken. The frame counter is already an approximation of the actual delay which doesn't really account for the type of USB interface (Isoc, Async) used on the device side. You'd be adding a lot of precision without really solving the fact that you don't know what the actual playback rate is. You'd really need a time smoother that models playback rate v. system time if you want to be fully accurate below the ms level for all types of USB audio devices. In addition I've only seen drivers report the delay based on hardware counters. If we start doing interpolations based on system time, then why not do it for other devices as well with a fix at the core level And last, I think you have an issue for the pause case and you cannot remove the code that limits the frame counter to 8 bits, this will not work across multiple versions of [e|x]HCI controllers, see below. > > As a downside, the possible worst case error increases from 2ms to 3ms. > --- > sound/usb/card.h | 1 + > sound/usb/pcm.c | 47 +++++++++++++++++++++++++++++++---------------- > 2 files changed, 32 insertions(+), 16 deletions(-) > > diff --git a/sound/usb/card.h b/sound/usb/card.h > index ed87cc83eb47..8835d9628cac 100644 > --- a/sound/usb/card.h > +++ b/sound/usb/card.h > @@ -149,6 +149,7 @@ struct snd_usb_substream { > > int last_frame_number; /* stored frame number */ > int last_delay; /* stored delay */ > + struct timespec last_update_time; /* time when last_delay was updated */ > > struct { > int marker; > diff --git a/sound/usb/pcm.c b/sound/usb/pcm.c > index 3cbfae6604f9..6d40cb0b8777 100644 > --- a/sound/usb/pcm.c > +++ b/sound/usb/pcm.c > @@ -46,9 +46,8 @@ snd_pcm_uframes_t snd_usb_pcm_delay(struct snd_usb_substream *subs, > int current_frame_number; > int frame_diff; > int est_delay; > - > - if (!subs->last_delay) > - return 0; /* short path */ NAK. This was added on purpose by 48779a0b8ffc ('ALSA: usb-audio: fix delay account during pause') And since you kept the code that sets last_delay there is no reason to take this out. > + int time_diff; > + struct timespec now; > > current_frame_number = usb_get_current_frame_number(subs->dev); > /* > @@ -58,9 +57,21 @@ snd_pcm_uframes_t snd_usb_pcm_delay(struct snd_usb_substream *subs, > */ > frame_diff = (current_frame_number - subs->last_frame_number) & 0xff; > > - /* Approximation based on number of samples per USB frame (ms), > - some truncation for 44.1 but the estimate is good enough */ > - est_delay = frame_diff * rate / 1000; > + /* Estimate time since last update of last_delay */ > + snd_pcm_gettime(subs->pcm_substream->runtime, &now); > + time_diff = (now.tv_sec - subs->last_update_time.tv_sec) * 1000000000; > + time_diff += (int)now.tv_nsec - subs->last_update_time.tv_nsec; > + time_diff /= 1000; > + > + if (abs(time_diff - frame_diff * 1000) < 1000) { that's a 1ms error, this is quite high IMO to assess a divergence between system and frame deltas > + /* System time and sound card time do not differ > + * too much. Use system time to calculate delay */ > + est_delay = time_diff * rate / 1000000; > + } else { > + /* Approximation based on number of samples per USB frame (ms), > + some truncation for 44.1 but the estimate is good enough */ > + est_delay = frame_diff * rate / 1000; > + } > if (subs->direction == SNDRV_PCM_STREAM_PLAYBACK) > est_delay = subs->last_delay - est_delay; > else > @@ -853,6 +864,8 @@ static int snd_usb_pcm_prepare(struct snd_pcm_substream *substream) > subs->hwptr_done = 0; > subs->transfer_done = 0; > subs->last_delay = 0; > + subs->last_update_time.tv_sec = 0; > + subs->last_update_time.tv_nsec = 0; > subs->last_frame_number = 0; > runtime->delay = 0; > > @@ -1334,7 +1347,9 @@ static void retire_capture_urb(struct snd_usb_substream *subs, > > /* realign last_frame_number */ > subs->last_frame_number = current_frame_number; > - subs->last_frame_number &= 0xFF; /* keep 8 LSBs */ You can't remove this, this models the fact that only 8 bits are valid in the frame counter. > + > + /* Take time stamp for delay accounting */ > + snd_pcm_gettime(runtime, &subs->last_update_time); > > spin_unlock_irqrestore(&subs->lock, flags); > /* copy a data chunk */ > @@ -1550,7 +1565,9 @@ static void prepare_playback_urb(struct snd_usb_substream *subs, > > /* realign last_frame_number */ > subs->last_frame_number = usb_get_current_frame_number(subs->dev); > - subs->last_frame_number &= 0xFF; /* keep 8 LSBs */ same here, can't do this > + > + /* Take time stamp for delay accounting */ > + snd_pcm_gettime(runtime, &subs->last_update_time); > > if (subs->trigger_tstamp_pending_update) { > /* this is the first actual URB submitted, > @@ -1597,6 +1614,12 @@ static void retire_playback_urb(struct snd_usb_substream *subs, > subs->last_delay -= processed; > runtime->delay = subs->last_delay; > > + /* realign last_frame_number */ > + subs->last_frame_number = usb_get_current_frame_number(subs->dev); need to filter on 8 bits.. > + > + /* Take time stamp for delay accounting */ > + snd_pcm_gettime(runtime, &subs->last_update_time); > + > /* > * Report when delay estimate is off by more than 2ms. > * The error should be lower than 2ms since the estimate relies > @@ -1607,14 +1630,6 @@ static void retire_playback_urb(struct snd_usb_substream *subs, > "delay: estimated %d, actual %d\n", > est_delay, subs->last_delay); > > - if (!subs->running) { > - /* update last_frame_number for delay counting here since > - * prepare_playback_urb won't be called during pause > - */ > - subs->last_frame_number = > - usb_get_current_frame_number(subs->dev) & 0xff; > - } > - and why is this removed. > out: > spin_unlock_irqrestore(&subs->lock, flags); > }
On 04.04.2018 21:24, Pierre-Louis Bossart wrote: > On 04/04/2018 08:57 AM, Georg Chini wrote: > >> Currently, the kernel uses only the number of USB frames to >> interpolate the delay >> between sending or receiving URB's. This limits the granuarity of the >> delay reports >> to one millisecond because the frame counter is updated in a >> millisecond interval. >> >> This patch additionally uses time stamps to achieve higher precision >> of the reported >> delay. If the time difference computed using time stamps does not >> deviate more than >> one millisecond from the difference computed using frames, it is >> assumed that the >> system time difference delivers the more precise delay estimate. >> >> This will significantly increase the precision of the reported delay >> in the majority >> of cases where sound card clock and system clock are almost in sync >> while falling back >> to the old method for the rare cases where the two times differ too >> much. > I am all for better delay estimates but I have mixed feelings about > this - and some hardware-related parts are broken. Well, I only stumbled into this because of the recent delay estimation issues in pulseaudio, so forgive my ignorance about ALSA. If the general idea of the patch is not right, just ignore my other comments and drop the patch. > > The frame counter is already an approximation of the actual delay > which doesn't really account for the type of USB interface (Isoc, > Async) used on the device side. You'd be adding a lot of precision > without really solving the fact that you don't know what the actual > playback rate is. You'd really need a time smoother that models > playback rate v. system time if you want to be fully accurate below > the ms level for all types of USB audio devices. The goal is not to be fully accurate, only to be somewhat better than the current estimate. I'll copy here what I already said to Takashi in private communication: The mismatch between sound card time and system time should not matter on the timescale we are talking about. Let's say we are using 10 USB frames (which is more than I have seen while debugging), so hw_ptr is updated every 10 ms. Assume that the deviation between sound card and system time is 1% (which again should be more than you will ever see). Then using system time, the computed delay can be off by 100 microseconds at maximum, while the delay computed using USB frames can be off by 2ms. Therefore using system time should be acceptable in that situation because it delivers higher precision results as can be achieved using USB frames. For the rare cases where system time and sound card time are completely different, the patch falls back to the original method. > > In addition I've only seen drivers report the delay based on hardware > counters. If we start doing interpolations based on system time, then > why not do it for other devices as well with a fix at the core level I could not do that, I don't have enough ALSA knowledge. A motivation to improve the delay reports for USB devices only could be that they are probably the most common devices apart from HDA. > > And last, I think you have an issue for the pause case and you cannot > remove the code that limits the frame counter to 8 bits, this will not > work across multiple versions of [e|x]HCI controllers, see below. > >> >> As a downside, the possible worst case error increases from 2ms to 3ms. >> --- >> sound/usb/card.h | 1 + >> sound/usb/pcm.c | 47 +++++++++++++++++++++++++++++++---------------- >> 2 files changed, 32 insertions(+), 16 deletions(-) >> >> diff --git a/sound/usb/card.h b/sound/usb/card.h >> index ed87cc83eb47..8835d9628cac 100644 >> --- a/sound/usb/card.h >> +++ b/sound/usb/card.h >> @@ -149,6 +149,7 @@ struct snd_usb_substream { >> int last_frame_number; /* stored frame number */ >> int last_delay; /* stored delay */ >> + struct timespec last_update_time; /* time when last_delay was >> updated */ >> struct { >> int marker; >> diff --git a/sound/usb/pcm.c b/sound/usb/pcm.c >> index 3cbfae6604f9..6d40cb0b8777 100644 >> --- a/sound/usb/pcm.c >> +++ b/sound/usb/pcm.c >> @@ -46,9 +46,8 @@ snd_pcm_uframes_t snd_usb_pcm_delay(struct >> snd_usb_substream *subs, >> int current_frame_number; >> int frame_diff; >> int est_delay; >> - >> - if (!subs->last_delay) >> - return 0; /* short path */ > > NAK. This was added on purpose by 48779a0b8ffc ('ALSA: usb-audio: fix > delay account during pause') > And since you kept the code that sets last_delay there is no reason to > take this out. OK, I did not know that this is needed. I took it out because otherwise the delay reported for capture will always be 0. retire_capture_urb() sets last_delay to 0. Does the problem that was fixed in the commit only apply to playback streams? Then I could change the if condition accordingly. >> + int time_diff; >> + struct timespec now; >> current_frame_number = usb_get_current_frame_number(subs->dev); >> /* >> @@ -58,9 +57,21 @@ snd_pcm_uframes_t snd_usb_pcm_delay(struct >> snd_usb_substream *subs, >> */ >> frame_diff = (current_frame_number - subs->last_frame_number) & >> 0xff; >> - /* Approximation based on number of samples per USB frame (ms), >> - some truncation for 44.1 but the estimate is good enough */ >> - est_delay = frame_diff * rate / 1000; >> + /* Estimate time since last update of last_delay */ >> + snd_pcm_gettime(subs->pcm_substream->runtime, &now); >> + time_diff = (now.tv_sec - subs->last_update_time.tv_sec) * >> 1000000000; >> + time_diff += (int)now.tv_nsec - subs->last_update_time.tv_nsec; >> + time_diff /= 1000; >> + >> + if (abs(time_diff - frame_diff * 1000) < 1000) { > that's a 1ms error, this is quite high IMO to assess a divergence > between system and frame deltas Because time_diff is used to interpolate between frames and frames are updated every millisecond, it is perfectly normal if time_diff is for example 900 microseconds larger than frame_diff * 1000. Only when the difference is larger than 1 ms we can conclude that something is wrong. That is the reason why the patch increases the worst case error from 2 ms to 3 ms. > >> + /* System time and sound card time do not differ >> + * too much. Use system time to calculate delay */ >> + est_delay = time_diff * rate / 1000000; >> + } else { >> + /* Approximation based on number of samples per USB frame (ms), >> + some truncation for 44.1 but the estimate is good enough */ >> + est_delay = frame_diff * rate / 1000; >> + } >> if (subs->direction == SNDRV_PCM_STREAM_PLAYBACK) >> est_delay = subs->last_delay - est_delay; >> else >> @@ -853,6 +864,8 @@ static int snd_usb_pcm_prepare(struct >> snd_pcm_substream *substream) >> subs->hwptr_done = 0; >> subs->transfer_done = 0; >> subs->last_delay = 0; >> + subs->last_update_time.tv_sec = 0; >> + subs->last_update_time.tv_nsec = 0; >> subs->last_frame_number = 0; >> runtime->delay = 0; >> @@ -1334,7 +1347,9 @@ static void retire_capture_urb(struct >> snd_usb_substream *subs, >> /* realign last_frame_number */ >> subs->last_frame_number = current_frame_number; >> - subs->last_frame_number &= 0xFF; /* keep 8 LSBs */ > > You can't remove this, this models the fact that only 8 bits are valid > in the frame counter. If the high order bits are not arbitrary, this is not necessary because the value is only used in snd_usb_pcm_delay() and (a - b) & 0xff = (a - b & 0xff) & 0xff. You can prove this by writing a and b as a = 256 * a1 + a2 and b = 256 * b1 + b2 with a2 and b2 smaller than 256. > >> + >> + /* Take time stamp for delay accounting */ >> + snd_pcm_gettime(runtime, &subs->last_update_time); >> spin_unlock_irqrestore(&subs->lock, flags); >> /* copy a data chunk */ >> @@ -1550,7 +1565,9 @@ static void prepare_playback_urb(struct >> snd_usb_substream *subs, >> /* realign last_frame_number */ >> subs->last_frame_number = usb_get_current_frame_number(subs->dev); >> - subs->last_frame_number &= 0xFF; /* keep 8 LSBs */ > same here, can't do this > >> + >> + /* Take time stamp for delay accounting */ >> + snd_pcm_gettime(runtime, &subs->last_update_time); >> if (subs->trigger_tstamp_pending_update) { >> /* this is the first actual URB submitted, >> @@ -1597,6 +1614,12 @@ static void retire_playback_urb(struct >> snd_usb_substream *subs, >> subs->last_delay -= processed; >> runtime->delay = subs->last_delay; >> + /* realign last_frame_number */ >> + subs->last_frame_number = usb_get_current_frame_number(subs->dev); > > need to filter on 8 bits.. >> + >> + /* Take time stamp for delay accounting */ >> + snd_pcm_gettime(runtime, &subs->last_update_time); >> + >> /* >> * Report when delay estimate is off by more than 2ms. >> * The error should be lower than 2ms since the estimate relies >> @@ -1607,14 +1630,6 @@ static void retire_playback_urb(struct >> snd_usb_substream *subs, >> "delay: estimated %d, actual %d\n", >> est_delay, subs->last_delay); >> - if (!subs->running) { >> - /* update last_frame_number for delay counting here since >> - * prepare_playback_urb won't be called during pause >> - */ >> - subs->last_frame_number = >> - usb_get_current_frame_number(subs->dev) & 0xff; >> - } >> - > and why is this removed. This is removed because the last_frame_number is now always updated, so it is no longer necessary. >> out: >> spin_unlock_irqrestore(&subs->lock, flags); >> }
On 4/5/18 6:49 AM, Georg Chini wrote: > On 04.04.2018 21:24, Pierre-Louis Bossart wrote: >> On 04/04/2018 08:57 AM, Georg Chini wrote: >> >>> Currently, the kernel uses only the number of USB frames to >>> interpolate the delay >>> between sending or receiving URB's. This limits the granuarity of the >>> delay reports >>> to one millisecond because the frame counter is updated in a >>> millisecond interval. >>> >>> This patch additionally uses time stamps to achieve higher precision >>> of the reported >>> delay. If the time difference computed using time stamps does not >>> deviate more than >>> one millisecond from the difference computed using frames, it is >>> assumed that the >>> system time difference delivers the more precise delay estimate. >>> >>> This will significantly increase the precision of the reported delay >>> in the majority >>> of cases where sound card clock and system clock are almost in sync >>> while falling back >>> to the old method for the rare cases where the two times differ too >>> much. >> I am all for better delay estimates but I have mixed feelings about >> this - and some hardware-related parts are broken. > > Well, I only stumbled into this because of the recent delay estimation > issues in pulseaudio, > so forgive my ignorance about ALSA. If the general idea of the patch is > not right, just ignore > my other comments and drop the patch. It's complicated to change this part of the code. When I added the use of the frame counter some 5 years back, it created all sorts of issues on various platforms that I was not even aware of. Your implementation raises obvious questions and would need quite a bit of revalidation. In its current state it's not ready for prime time. > >> >> The frame counter is already an approximation of the actual delay >> which doesn't really account for the type of USB interface (Isoc, >> Async) used on the device side. You'd be adding a lot of precision >> without really solving the fact that you don't know what the actual >> playback rate is. You'd really need a time smoother that models >> playback rate v. system time if you want to be fully accurate below >> the ms level for all types of USB audio devices. > > The goal is not to be fully accurate, only to be somewhat better than > the current estimate. Define 'better'. What is the goal and the expected benefit for applications? > I'll copy here what I already said to Takashi in private communication: > > The mismatch between sound card time and system time should not matter > on the > timescale we are talking about. Let's say we are using 10 USB frames > (which is > more than I have seen while debugging), so hw_ptr is updated every 10 > ms. Assume > that the deviation between sound card and system time is 1% (which again > should > be more than you will ever see). Then using system time, the computed > delay can > be off by 100 microseconds at maximum, while the delay computed using > USB frames > can be off by 2ms. Therefore using system time should be acceptable in that > situation because it delivers higher precision results as can be > achieved using > USB frames. > > For the rare cases where system time and sound card time are completely > different, > the patch falls back to the original method. > >> >> In addition I've only seen drivers report the delay based on hardware >> counters. If we start doing interpolations based on system time, then >> why not do it for other devices as well with a fix at the core level > > I could not do that, I don't have enough ALSA knowledge. A motivation to > improve > the delay reports for USB devices only could be that they are probably > the most > common devices apart from HDA. Exactly, why not do this for HDAudio then... > >> >> And last, I think you have an issue for the pause case and you cannot >> remove the code that limits the frame counter to 8 bits, this will not >> work across multiple versions of [e|x]HCI controllers, see below. >> >>> >>> As a downside, the possible worst case error increases from 2ms to 3ms. >>> --- >>> sound/usb/card.h | 1 + >>> sound/usb/pcm.c | 47 +++++++++++++++++++++++++++++++---------------- >>> 2 files changed, 32 insertions(+), 16 deletions(-) >>> >>> diff --git a/sound/usb/card.h b/sound/usb/card.h >>> index ed87cc83eb47..8835d9628cac 100644 >>> --- a/sound/usb/card.h >>> +++ b/sound/usb/card.h >>> @@ -149,6 +149,7 @@ struct snd_usb_substream { >>> int last_frame_number; /* stored frame number */ >>> int last_delay; /* stored delay */ >>> + struct timespec last_update_time; /* time when last_delay was >>> updated */ >>> struct { >>> int marker; >>> diff --git a/sound/usb/pcm.c b/sound/usb/pcm.c >>> index 3cbfae6604f9..6d40cb0b8777 100644 >>> --- a/sound/usb/pcm.c >>> +++ b/sound/usb/pcm.c >>> @@ -46,9 +46,8 @@ snd_pcm_uframes_t snd_usb_pcm_delay(struct >>> snd_usb_substream *subs, >>> int current_frame_number; >>> int frame_diff; >>> int est_delay; >>> - >>> - if (!subs->last_delay) >>> - return 0; /* short path */ >> >> NAK. This was added on purpose by 48779a0b8ffc ('ALSA: usb-audio: fix >> delay account during pause') >> And since you kept the code that sets last_delay there is no reason to >> take this out. > > OK, I did not know that this is needed. I took it out because otherwise > the delay > reported for capture will always be 0. retire_capture_urb() sets > last_delay to 0. > Does the problem that was fixed in the commit only apply to playback > streams? > Then I could change the if condition accordingly. That's precisely the issue: the code was contributed by multiple people to work on multiple platforms. What parts do what exactly is complicated to reverse-engineer. > > >>> + int time_diff; >>> + struct timespec now; >>> current_frame_number = usb_get_current_frame_number(subs->dev); >>> /* >>> @@ -58,9 +57,21 @@ snd_pcm_uframes_t snd_usb_pcm_delay(struct >>> snd_usb_substream *subs, >>> */ >>> frame_diff = (current_frame_number - subs->last_frame_number) & >>> 0xff; >>> - /* Approximation based on number of samples per USB frame (ms), >>> - some truncation for 44.1 but the estimate is good enough */ >>> - est_delay = frame_diff * rate / 1000; >>> + /* Estimate time since last update of last_delay */ >>> + snd_pcm_gettime(subs->pcm_substream->runtime, &now); >>> + time_diff = (now.tv_sec - subs->last_update_time.tv_sec) * >>> 1000000000; >>> + time_diff += (int)now.tv_nsec - subs->last_update_time.tv_nsec; >>> + time_diff /= 1000; >>> + >>> + if (abs(time_diff - frame_diff * 1000) < 1000) { >> that's a 1ms error, this is quite high IMO to assess a divergence >> between system and frame deltas > > Because time_diff is used to interpolate between frames and frames are > updated every millisecond, > it is perfectly normal if time_diff is for example 900 microseconds > larger than frame_diff * 1000. > Only when the difference is larger than 1 ms we can conclude that > something is wrong. > That is the reason why the patch increases the worst case error from 2 > ms to 3 ms. Yeah, well, given that the actual playback rate is not aligned with the frame counter for async interfaces, this test is questionable. > >> >>> + /* System time and sound card time do not differ >>> + * too much. Use system time to calculate delay */ >>> + est_delay = time_diff * rate / 1000000; >>> + } else { >>> + /* Approximation based on number of samples per USB frame (ms), >>> + some truncation for 44.1 but the estimate is good enough */ >>> + est_delay = frame_diff * rate / 1000; >>> + } >>> if (subs->direction == SNDRV_PCM_STREAM_PLAYBACK) >>> est_delay = subs->last_delay - est_delay; >>> else >>> @@ -853,6 +864,8 @@ static int snd_usb_pcm_prepare(struct >>> snd_pcm_substream *substream) >>> subs->hwptr_done = 0; >>> subs->transfer_done = 0; >>> subs->last_delay = 0; >>> + subs->last_update_time.tv_sec = 0; >>> + subs->last_update_time.tv_nsec = 0; >>> subs->last_frame_number = 0; >>> runtime->delay = 0; >>> @@ -1334,7 +1347,9 @@ static void retire_capture_urb(struct >>> snd_usb_substream *subs, >>> /* realign last_frame_number */ >>> subs->last_frame_number = current_frame_number; >>> - subs->last_frame_number &= 0xFF; /* keep 8 LSBs */ >> >> You can't remove this, this models the fact that only 8 bits are valid >> in the frame counter. > > If the high order bits are not arbitrary, this is not necessary because > the value is > only used in snd_usb_pcm_delay() and (a - b) & 0xff = (a - b & 0xff) & > 0xff. You can > prove this by writing a and b as a = 256 * a1 + a2 and b = 256 * b1 + b2 > with a2 > and b2 smaller than 256. the number of valid bits varies in different controllers so the goal was to align on the common denominator - 8 bits only. > >> >>> + >>> + /* Take time stamp for delay accounting */ >>> + snd_pcm_gettime(runtime, &subs->last_update_time); >>> spin_unlock_irqrestore(&subs->lock, flags); >>> /* copy a data chunk */ >>> @@ -1550,7 +1565,9 @@ static void prepare_playback_urb(struct >>> snd_usb_substream *subs, >>> /* realign last_frame_number */ >>> subs->last_frame_number = usb_get_current_frame_number(subs->dev); >>> - subs->last_frame_number &= 0xFF; /* keep 8 LSBs */ >> same here, can't do this >> >>> + >>> + /* Take time stamp for delay accounting */ >>> + snd_pcm_gettime(runtime, &subs->last_update_time); >>> if (subs->trigger_tstamp_pending_update) { >>> /* this is the first actual URB submitted, >>> @@ -1597,6 +1614,12 @@ static void retire_playback_urb(struct >>> snd_usb_substream *subs, >>> subs->last_delay -= processed; >>> runtime->delay = subs->last_delay; >>> + /* realign last_frame_number */ >>> + subs->last_frame_number = usb_get_current_frame_number(subs->dev); >> >> need to filter on 8 bits.. >>> + >>> + /* Take time stamp for delay accounting */ >>> + snd_pcm_gettime(runtime, &subs->last_update_time); >>> + >>> /* >>> * Report when delay estimate is off by more than 2ms. >>> * The error should be lower than 2ms since the estimate relies >>> @@ -1607,14 +1630,6 @@ static void retire_playback_urb(struct >>> snd_usb_substream *subs, >>> "delay: estimated %d, actual %d\n", >>> est_delay, subs->last_delay); >>> - if (!subs->running) { >>> - /* update last_frame_number for delay counting here since >>> - * prepare_playback_urb won't be called during pause >>> - */ >>> - subs->last_frame_number = >>> - usb_get_current_frame_number(subs->dev) & 0xff; >>> - } >>> - >> and why is this removed. > > This is removed because the last_frame_number is now always updated, > so it is no longer necessary. makes no sense to me. > >>> out: >>> spin_unlock_irqrestore(&subs->lock, flags); >>> } > > > _______________________________________________ > Alsa-devel mailing list > Alsa-devel@alsa-project.org > http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
On 05.04.2018 16:13, Pierre-Louis Bossart wrote: > On 4/5/18 6:49 AM, Georg Chini wrote: >> On 04.04.2018 21:24, Pierre-Louis Bossart wrote: >>> On 04/04/2018 08:57 AM, Georg Chini wrote: >>> >>>> Currently, the kernel uses only the number of USB frames to >>>> interpolate the delay >>>> between sending or receiving URB's. This limits the granuarity of >>>> the delay reports >>>> to one millisecond because the frame counter is updated in a >>>> millisecond interval. >>>> >>>> This patch additionally uses time stamps to achieve higher >>>> precision of the reported >>>> delay. If the time difference computed using time stamps does not >>>> deviate more than >>>> one millisecond from the difference computed using frames, it is >>>> assumed that the >>>> system time difference delivers the more precise delay estimate. >>>> >>>> This will significantly increase the precision of the reported >>>> delay in the majority >>>> of cases where sound card clock and system clock are almost in sync >>>> while falling back >>>> to the old method for the rare cases where the two times differ too >>>> much. >>> I am all for better delay estimates but I have mixed feelings about >>> this - and some hardware-related parts are broken. >> >> Well, I only stumbled into this because of the recent delay >> estimation issues in pulseaudio, >> so forgive my ignorance about ALSA. If the general idea of the patch >> is not right, just ignore >> my other comments and drop the patch. > > It's complicated to change this part of the code. When I added the use > of the frame counter some 5 years back, it created all sorts of issues > on various platforms that I was not even aware of. > > Your implementation raises obvious questions and would need quite a > bit of revalidation. In its current state it's not ready for prime time. OK, so as I said, just forget about it. > >>>> @@ -1334,7 +1347,9 @@ static void retire_capture_urb(struct >>>> snd_usb_substream *subs, >>>> /* realign last_frame_number */ >>>> subs->last_frame_number = current_frame_number; >>>> - subs->last_frame_number &= 0xFF; /* keep 8 LSBs */ >>> >>> You can't remove this, this models the fact that only 8 bits are >>> valid in the frame counter. >> >> This is not necessary because the value is >> only used in snd_usb_pcm_delay() and (a - b) & 0xff = (a - b & 0xff) >> & 0xff. You can >> prove this by writing a and b as a = 256 * a1 + a2 and b = 256 * b1 + >> b2 with a2 >> and b2 smaller than 256. > > the number of valid bits varies in different controllers so the goal > was to align on the common denominator - 8 bits only. The common denominator will still be 8 bits, using & 0xff once in snd_usb_pcm_delay() when the frame difference is calculated has exactly the same effect, so doing it here is unnecessary.
diff --git a/sound/usb/card.h b/sound/usb/card.h index ed87cc83eb47..8835d9628cac 100644 --- a/sound/usb/card.h +++ b/sound/usb/card.h @@ -149,6 +149,7 @@ struct snd_usb_substream { int last_frame_number; /* stored frame number */ int last_delay; /* stored delay */ + struct timespec last_update_time; /* time when last_delay was updated */ struct { int marker; diff --git a/sound/usb/pcm.c b/sound/usb/pcm.c index 3cbfae6604f9..6d40cb0b8777 100644 --- a/sound/usb/pcm.c +++ b/sound/usb/pcm.c @@ -46,9 +46,8 @@ snd_pcm_uframes_t snd_usb_pcm_delay(struct snd_usb_substream *subs, int current_frame_number; int frame_diff; int est_delay; - - if (!subs->last_delay) - return 0; /* short path */ + int time_diff; + struct timespec now; current_frame_number = usb_get_current_frame_number(subs->dev); /* @@ -58,9 +57,21 @@ snd_pcm_uframes_t snd_usb_pcm_delay(struct snd_usb_substream *subs, */ frame_diff = (current_frame_number - subs->last_frame_number) & 0xff; - /* Approximation based on number of samples per USB frame (ms), - some truncation for 44.1 but the estimate is good enough */ - est_delay = frame_diff * rate / 1000; + /* Estimate time since last update of last_delay */ + snd_pcm_gettime(subs->pcm_substream->runtime, &now); + time_diff = (now.tv_sec - subs->last_update_time.tv_sec) * 1000000000; + time_diff += (int)now.tv_nsec - subs->last_update_time.tv_nsec; + time_diff /= 1000; + + if (abs(time_diff - frame_diff * 1000) < 1000) { + /* System time and sound card time do not differ + * too much. Use system time to calculate delay */ + est_delay = time_diff * rate / 1000000; + } else { + /* Approximation based on number of samples per USB frame (ms), + some truncation for 44.1 but the estimate is good enough */ + est_delay = frame_diff * rate / 1000; + } if (subs->direction == SNDRV_PCM_STREAM_PLAYBACK) est_delay = subs->last_delay - est_delay; else @@ -853,6 +864,8 @@ static int snd_usb_pcm_prepare(struct snd_pcm_substream *substream) subs->hwptr_done = 0; subs->transfer_done = 0; subs->last_delay = 0; + subs->last_update_time.tv_sec = 0; + subs->last_update_time.tv_nsec = 0; subs->last_frame_number = 0; runtime->delay = 0; @@ -1334,7 +1347,9 @@ static void retire_capture_urb(struct snd_usb_substream *subs, /* realign last_frame_number */ subs->last_frame_number = current_frame_number; - subs->last_frame_number &= 0xFF; /* keep 8 LSBs */ + + /* Take time stamp for delay accounting */ + snd_pcm_gettime(runtime, &subs->last_update_time); spin_unlock_irqrestore(&subs->lock, flags); /* copy a data chunk */ @@ -1550,7 +1565,9 @@ static void prepare_playback_urb(struct snd_usb_substream *subs, /* realign last_frame_number */ subs->last_frame_number = usb_get_current_frame_number(subs->dev); - subs->last_frame_number &= 0xFF; /* keep 8 LSBs */ + + /* Take time stamp for delay accounting */ + snd_pcm_gettime(runtime, &subs->last_update_time); if (subs->trigger_tstamp_pending_update) { /* this is the first actual URB submitted, @@ -1597,6 +1614,12 @@ static void retire_playback_urb(struct snd_usb_substream *subs, subs->last_delay -= processed; runtime->delay = subs->last_delay; + /* realign last_frame_number */ + subs->last_frame_number = usb_get_current_frame_number(subs->dev); + + /* Take time stamp for delay accounting */ + snd_pcm_gettime(runtime, &subs->last_update_time); + /* * Report when delay estimate is off by more than 2ms. * The error should be lower than 2ms since the estimate relies @@ -1607,14 +1630,6 @@ static void retire_playback_urb(struct snd_usb_substream *subs, "delay: estimated %d, actual %d\n", est_delay, subs->last_delay); - if (!subs->running) { - /* update last_frame_number for delay counting here since - * prepare_playback_urb won't be called during pause - */ - subs->last_frame_number = - usb_get_current_frame_number(subs->dev) & 0xff; - } - out: spin_unlock_irqrestore(&subs->lock, flags); }