[PATCH] ALSA: usb: refine delay information with USB frame counter

[PATCH] ALSA: usb: refine delay information with USB frame counter

[Pierre-Louis Bossart]

Existing code only updates the audio delay when URBs were
submitted/retired. This can introduce an uncertainty of 8ms
on the number of samples played out with the default settings,
and a lot more when URBs convey more packets to reduce the
interrupt rate and power consumption.

This patch relies on the USB frame counter to reduce the
uncertainty to ~1ms. The delay information essentially becomes
independent of the URB size and number of packets. This should
improve audio/video sync and help applications like PulseAudio
which require accurate audio timing.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 sound/usb/card.h |    2 ++
 sound/usb/pcm.c  |   23 +++++++++++++++++++++++
 sound/usb/pcm.h  |    3 +++
 sound/usb/urb.c  |   20 +++++++++++++++++---
 4 files changed, 45 insertions(+), 3 deletions(-)

diff --git a/sound/usb/card.h b/sound/usb/card.h
index ae4251d..a39edcc 100644
--- a/sound/usb/card.h
+++ b/sound/usb/card.h
@@ -94,6 +94,8 @@ struct snd_usb_substream {
 	spinlock_t lock;
 
 	struct snd_urb_ops ops;		/* callbacks (must be filled at init) */
+	int last_frame_number;          /* stored frame number */
+	int last_delay;                 /* stored delay */
 };
 
 struct snd_usb_stream {
diff --git a/sound/usb/pcm.c b/sound/usb/pcm.c
index b8dcbf4..1696e2b 100644
--- a/sound/usb/pcm.c
+++ b/sound/usb/pcm.c
@@ -34,6 +34,27 @@
 #include "clock.h"
 #include "power.h"
 
+/* return the estimated delay based on USB frame counters */
+snd_pcm_uframes_t snd_usb_pcm_delay(struct snd_usb_substream *subs,
+				    unsigned int rate)
+{
+	int current_frame_number;
+	int frame_diff;
+	int est_delay;
+
+	current_frame_number = usb_get_current_frame_number(subs->dev);
+	frame_diff = current_frame_number-subs->last_frame_number;
+	if (frame_diff < 0)
+		frame_diff += 1024; /* handle 10-bit wrap-around */
+
+	/* Approximation based on number of samples per USB frame (ms),
+	   some truncation for 44.1 but the estimate is good enough */
+	est_delay =  subs->last_delay-(frame_diff*rate/1000L);
+	if (est_delay < 0)
+		est_delay = 0;
+	return est_delay;
+}
+
 /*
  * return the current pcm pointer.  just based on the hwptr_done value.
  */
@@ -45,6 +66,8 @@ static snd_pcm_uframes_t snd_usb_pcm_pointer(struct snd_pcm_substream *substream
 	subs = (struct snd_usb_substream *)substream->runtime->private_data;
 	spin_lock(&subs->lock);
 	hwptr_done = subs->hwptr_done;
+	substream->runtime->delay = snd_usb_pcm_delay(subs,
+						substream->runtime->rate);
 	spin_unlock(&subs->lock);
 	return hwptr_done / (substream->runtime->frame_bits >> 3);
 }
diff --git a/sound/usb/pcm.h b/sound/usb/pcm.h
index ed3e283..df7a003 100644
--- a/sound/usb/pcm.h
+++ b/sound/usb/pcm.h
@@ -1,6 +1,9 @@
 #ifndef __USBAUDIO_PCM_H
 #define __USBAUDIO_PCM_H
 
+snd_pcm_uframes_t snd_usb_pcm_delay(struct snd_usb_substream *subs,
+				    unsigned int rate);
+
 void snd_usb_set_pcm_ops(struct snd_pcm *pcm, int stream);
 
 int snd_usb_init_pitch(struct snd_usb_audio *chip, int iface,
diff --git a/sound/usb/urb.c b/sound/usb/urb.c
index e184349..ccf77d1 100644
--- a/sound/usb/urb.c
+++ b/sound/usb/urb.c
@@ -718,7 +718,15 @@ static int prepare_playback_urb(struct snd_usb_substream *subs,
 	subs->hwptr_done += bytes;
 	if (subs->hwptr_done >= runtime->buffer_size * stride)
 		subs->hwptr_done -= runtime->buffer_size * stride;
+
+	/* update delay with exact number of samples queued */
+	runtime->delay = subs->last_delay;
 	runtime->delay += frames;
+	subs->last_delay = runtime->delay;
+
+	/* realign last_frame_number */
+	subs->last_frame_number = usb_get_current_frame_number(subs->dev);
+
 	spin_unlock_irqrestore(&subs->lock, flags);
 	urb->transfer_buffer_length = bytes;
 	if (period_elapsed)
@@ -737,12 +745,18 @@ static int retire_playback_urb(struct snd_usb_substream *subs,
 	unsigned long flags;
 	int stride = runtime->frame_bits >> 3;
 	int processed = urb->transfer_buffer_length / stride;
+	int est_delay;
 
 	spin_lock_irqsave(&subs->lock, flags);
-	if (processed > runtime->delay)
-		runtime->delay = 0;
+
+	est_delay = snd_usb_pcm_delay(subs, runtime->rate);
+	/* update delay with exact number of samples played */
+	if (processed > subs->last_delay)
+		subs->last_delay = 0;
 	else
-		runtime->delay -= processed;
+		subs->last_delay -= processed;
+	runtime->delay = subs->last_delay;
+
 	spin_unlock_irqrestore(&subs->lock, flags);
 	return 0;
 }
-- 
1.7.6

Re: [PATCH] ALSA: usb: refine delay information with USB frame counter

[Clemens Ladisch]

Pierre-Louis Bossart wrote:
> This patch relies on the USB frame counter ...

> +	if (frame_diff < 0)
> +		frame_diff += 1024; /* handle 10-bit wrap-around */

After reading through some HCD source files, I deduced the following
wrap-arounds:

EHCI: 8-10 bits (default 9)
FHCI: 11 bits
IMX21: 16 bits
ISP1760: 10 bits
OHCI: 16 bits
OXU210HP: 10 bits
R8A66597: 10 bits
SL811: 5 bits(!?)
UHCI: 32 bits
xHCI: ?

So which hardware did you test this on?  :-)

As long as there isn't an API that tells us about the valid range of the
frame counter, we should probably mask off all except the lowest few bits.


Regards,
Clemens

Re: [PATCH] ALSA: usb: refine delay information with USB frame counter

[Pierre-Louis Bossart]

> > +	if (frame_diff < 0)
> > +		frame_diff += 1024; /* handle 10-bit wrap-around */
> 
> After reading through some HCD source files, I deduced the following
> wrap-arounds:
> 
> EHCI: 8-10 bits (default 9)
> FHCI: 11 bits
> IMX21: 16 bits
> ISP1760: 10 bits
> OHCI: 16 bits
> OXU210HP: 10 bits
> R8A66597: 10 bits
> SL811: 5 bits(!?)
> UHCI: 32 bits
> xHCI: ?
> 
> As long as there isn't an API that tells us about the valid range of
> the
> frame counter, we should probably mask off all except the lowest few
> bits.

Argh. Good catch, thanks for looking into this. 
If we only used the 8 least-significant bits, it'd still leave us with up to
256 ms. That should work I guess. But the SL811 with 5 bits is a problem,
that makes 32ms, this is probably too low. In that case we'd need to fall
back to the existing scheme and ignore the frame counter information.
-Pierre

Re: [PATCH] ALSA: usb: refine delay information with USB frame counter

[Clemens Ladisch]

Pierre-Louis Bossart wrote:
> > > +	if (frame_diff < 0)
> > > +		frame_diff += 1024; /* handle 10-bit wrap-around */
> > 
> > After reading through some HCD source files, I deduced the following
> > wrap-arounds:
> > 
> > EHCI: 8-10 bits (default 9)
> > FHCI: 11 bits
> > IMX21: 16 bits
> > ISP1760: 10 bits
> > OHCI: 16 bits
> > OXU210HP: 10 bits
> > R8A66597: 10 bits
> > SL811: 5 bits(!?)
> > UHCI: 32 bits
> > xHCI: ?
> > 
> > As long as there isn't an API that tells us about the valid range of the
> > frame counter, we should probably mask off all except the lowest few bits.
> 
> If we only used the 8 least-significant bits, it'd still leave us with up to
> 256 ms. That should work I guess. But the SL811 with 5 bits is a problem,
> that makes 32ms, this is probably too low.

Sorry, I read the code wrong; that driver returns a 16-bit frame counter, it
just doesn't allow to schedule more than 32 frame AFAICT.  And that driver
doesn't support USB audio anyway, so 8 bits should be fine.


Regards,
Clemens

[PATCH] ALSA: usb: refine delay information with USB frame counter

[Pierre-Louis Bossart]

Existing code only updates the audio delay when URBs were
submitted/retired. This can introduce an uncertainty of 8ms
on the number of samples played out with the default settings,
and a lot more when URBs convey more packets to reduce the
interrupt rate and power consumption.

This patch relies on the USB frame counter to reduce the
uncertainty to less than 2ms worst-case. The delay information
essentially becomes independent of the URB size and number of
packets. This should improve audio/video sync and help
applications like PulseAudio which require accurate audio
timing.

As pointed out by Clemens Ladisch <clemens@ladisch.de>, the
implementation of frame counters varies between different HCDs.
Only the 8 lowest-bits are used to estimate the delay.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 sound/usb/card.h |    2 ++
 sound/usb/pcm.c  |   30 ++++++++++++++++++++++++++++++
 sound/usb/pcm.h  |    3 +++
 sound/usb/urb.c  |   28 +++++++++++++++++++++++++---
 4 files changed, 60 insertions(+), 3 deletions(-)

diff --git a/sound/usb/card.h b/sound/usb/card.h
index ae4251d..a39edcc 100644
--- a/sound/usb/card.h
+++ b/sound/usb/card.h
@@ -94,6 +94,8 @@ struct snd_usb_substream {
 	spinlock_t lock;
 
 	struct snd_urb_ops ops;		/* callbacks (must be filled at init) */
+	int last_frame_number;          /* stored frame number */
+	int last_delay;                 /* stored delay */
 };
 
 struct snd_usb_stream {
diff --git a/sound/usb/pcm.c b/sound/usb/pcm.c
index b8dcbf4..6076863 100644
--- a/sound/usb/pcm.c
+++ b/sound/usb/pcm.c
@@ -34,6 +34,32 @@
 #include "clock.h"
 #include "power.h"
 
+/* return the estimated delay based on USB frame counters */
+snd_pcm_uframes_t snd_usb_pcm_delay(struct snd_usb_substream *subs,
+				    unsigned int rate)
+{
+	int current_frame_number;
+	int frame_diff;
+	int est_delay;
+
+	current_frame_number = usb_get_current_frame_number(subs->dev);
+	/* HCD implementations use different widths, use lower 8 bits.
+	 * The delay will be managed up to 256ms, which is more than
+	 * enough
+	 */
+	current_frame_number &= 0xFF;
+	frame_diff = current_frame_number-subs->last_frame_number;
+	if (frame_diff < 0)
+		frame_diff += 256; /* handle 8-bit wrap-around */
+
+	/* Approximation based on number of samples per USB frame (ms),
+	   some truncation for 44.1 but the estimate is good enough */
+	est_delay =  subs->last_delay-(frame_diff*rate/1000L);
+	if (est_delay < 0)
+		est_delay = 0;
+	return est_delay;
+}
+
 /*
  * return the current pcm pointer.  just based on the hwptr_done value.
  */
@@ -45,6 +71,8 @@ static snd_pcm_uframes_t snd_usb_pcm_pointer(struct snd_pcm_substream *substream
 	subs = (struct snd_usb_substream *)substream->runtime->private_data;
 	spin_lock(&subs->lock);
 	hwptr_done = subs->hwptr_done;
+	substream->runtime->delay = snd_usb_pcm_delay(subs,
+						substream->runtime->rate);
 	spin_unlock(&subs->lock);
 	return hwptr_done / (substream->runtime->frame_bits >> 3);
 }
@@ -417,6 +445,8 @@ static int snd_usb_pcm_prepare(struct snd_pcm_substream *substream)
 	subs->hwptr_done = 0;
 	subs->transfer_done = 0;
 	subs->phase = 0;
+	subs->last_delay = 0;
+	subs->last_frame_number = 0;
 	runtime->delay = 0;
 
 	return snd_usb_substream_prepare(subs, runtime);
diff --git a/sound/usb/pcm.h b/sound/usb/pcm.h
index ed3e283..df7a003 100644
--- a/sound/usb/pcm.h
+++ b/sound/usb/pcm.h
@@ -1,6 +1,9 @@
 #ifndef __USBAUDIO_PCM_H
 #define __USBAUDIO_PCM_H
 
+snd_pcm_uframes_t snd_usb_pcm_delay(struct snd_usb_substream *subs,
+				    unsigned int rate);
+
 void snd_usb_set_pcm_ops(struct snd_pcm *pcm, int stream);
 
 int snd_usb_init_pitch(struct snd_usb_audio *chip, int iface,
diff --git a/sound/usb/urb.c b/sound/usb/urb.c
index e184349..9e3e3f9 100644
--- a/sound/usb/urb.c
+++ b/sound/usb/urb.c
@@ -718,7 +718,16 @@ static int prepare_playback_urb(struct snd_usb_substream *subs,
 	subs->hwptr_done += bytes;
 	if (subs->hwptr_done >= runtime->buffer_size * stride)
 		subs->hwptr_done -= runtime->buffer_size * stride;
+
+	/* update delay with exact number of samples queued */
+	runtime->delay = subs->last_delay;
 	runtime->delay += frames;
+	subs->last_delay = runtime->delay;
+
+	/* realign last_frame_number */
+	subs->last_frame_number = usb_get_current_frame_number(subs->dev);
+	subs->last_frame_number &= 0xFF; /* keep 8 LSBs */
+
 	spin_unlock_irqrestore(&subs->lock, flags);
 	urb->transfer_buffer_length = bytes;
 	if (period_elapsed)
@@ -737,12 +746,25 @@ static int retire_playback_urb(struct snd_usb_substream *subs,
 	unsigned long flags;
 	int stride = runtime->frame_bits >> 3;
 	int processed = urb->transfer_buffer_length / stride;
+	int est_delay;
 
 	spin_lock_irqsave(&subs->lock, flags);
-	if (processed > runtime->delay)
-		runtime->delay = 0;
+
+	est_delay = snd_usb_pcm_delay(subs, runtime->rate);
+	/* update delay with exact number of samples played */
+	if (processed > subs->last_delay)
+		subs->last_delay = 0;
 	else
-		runtime->delay -= processed;
+		subs->last_delay -= processed;
+	runtime->delay = subs->last_delay;
+
+	/* Report when delay estimate is off by more than 2ms.
+	 * The error should be lower than 2ms since the estimate relies
+	 * on two reads of a counter updated every ms */
+	if (abs(est_delay-subs->last_delay) > (runtime->rate*2/1000L))
+		snd_printk(KERN_DEBUG "ALSA usb.c: Delay %d actual delay %d\n",
+			est_delay, subs->last_delay);
+
 	spin_unlock_irqrestore(&subs->lock, flags);
 	return 0;
 }
-- 
1.7.6

Re: [PATCH] ALSA: usb: refine delay information with USB frame counter

[Clemens Ladisch]

Pierre-Louis Bossart wrote:
> Existing code only updates the audio delay when URBs were
> submitted/retired. This can introduce an uncertainty of 8ms
> on the number of samples played out with the default settings,
> and a lot more when URBs convey more packets to reduce the
> interrupt rate and power consumption.
> 
> This patch relies on the USB frame counter to reduce the
> uncertainty to less than 2ms worst-case. The delay information
> essentially becomes independent of the URB size and number of
> packets. This should improve audio/video sync and help
> applications like PulseAudio which require accurate audio
> timing.

I haven't yet found the time to test this, but there are a few
theoretical nitpicks ...

> +	/* HCD implementations use different widths, use lower 8 bits.
> +	 * The delay will be managed up to 256ms, which is more than
> +	 * enough
> +	 */

/*
 * Multi-line comments are supposed to use this formatting style.
 */

> +	current_frame_number &= 0xFF;
> +	frame_diff = current_frame_number-subs->last_frame_number;
> +	if (frame_diff < 0)
> +		frame_diff += 256; /* handle 8-bit wrap-around */

This could be replaced with just:
	frame_diff = (current_frame_number - subs->last_frame_number) & 0xff;

> +	/* Report when delay estimate is off by more than 2ms.
> +	 * The error should be lower than 2ms since the estimate relies
> +	 * on two reads of a counter updated every ms */
> +	if (abs(est_delay-subs->last_delay) > (runtime->rate*2/1000L))
> +		snd_printk(KERN_DEBUG "ALSA usb.c: Delay %d actual delay %d\n",
> +			est_delay, subs->last_delay);

Do you know how large the difference actually becomes?

I'm not sure if it might be possible to construct a scenario where
rounding errors could accumulate ...


Regards,
Clemens

Re: [PATCH] ALSA: usb: refine delay information with USB frame counter

[Pierre-Louis Bossart]

> /*
>  * Multi-line comments are supposed to use this formatting style.
>  */

Ok

> This could be replaced with just:
> 	frame_diff = (current_frame_number - subs->last_frame_number) &
> 0xff;

ok
 
> Do you know how large the difference actually becomes?
> 
> I'm not sure if it might be possible to construct a scenario where
> rounding errors could accumulate ...

I don't think so. I've use a classic technique, I've seen similar code used
to improve a/v sync when the audio time is updated in steps. The estimate
starts from the delay as computed before (number of samples queued), then
decreased down (possibly to zero) using the frame counter. When the urbs are
retired, the delay is reset as previously. The frame number is only used to
predict the delay more accurately, and the delay is always based on 2 reads
of the frame counter. Worst case one case be off by one ms on each read.

Re: [PATCH] ALSA: usb: refine delay information with USB frame counter

[Clemens Ladisch]

Pierre-Louis Bossart wrote:
> ... This should improve audio/video sync

It does indeed; mplayer's A-V difference gets lowered from up to
nrpacks ms down to at most 1 ms.

> +	est_delay =  subs->last_delay-(frame_diff*rate/1000L);

Why a long dividend?

The spacing around operators is inconsistent; please use checkpatch.

> +		snd_printk(KERN_DEBUG "ALSA usb.c: Delay %d actual delay %d\n",
> +			est_delay, subs->last_delay);

snd_printk already can print the "ALSA" and file name prefixes.
(And it uses the correct file name.  ;-)


Regards,
Clemens