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);
}
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
