[PATCH 0/2] Fix usb-audio delay calculation

[PATCH 0/2] Fix usb-audio delay calculation

[Takashi Iwai]

Hi,

the following two patches are for fixing the wrong PCM delay reported
from usb-audio driver.  The first one is trivial, just to fix / optimize
for the capture direction.  The second one is a deeper fix for corner
cases like playback pause.


Takashi

[PATCH 1/2] ALSA: usb-audio: ignore delay calculation for capture stream

[Takashi Iwai]

It doesn't make sense to calculate the delay for capture streams in
the current implementation.  It's always zero, so we should skip the
computation in snd_usb_pcm_pointer() in the case of capture.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/usb/pcm.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/sound/usb/pcm.c b/sound/usb/pcm.c
index e919c2e..8e1d5e0 100644
--- a/sound/usb/pcm.c
+++ b/sound/usb/pcm.c
@@ -75,7 +75,8 @@ static snd_pcm_uframes_t snd_usb_pcm_pointer(struct snd_pcm_substream *substream
 		return SNDRV_PCM_POS_XRUN;
 	spin_lock(&subs->lock);
 	hwptr_done = subs->hwptr_done;
-	substream->runtime->delay = snd_usb_pcm_delay(subs,
+	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
+		substream->runtime->delay = snd_usb_pcm_delay(subs,
 						substream->runtime->rate);
 	spin_unlock(&subs->lock);
 	return hwptr_done / (substream->runtime->frame_bits >> 3);
-- 
1.8.0

Re: [PATCH 1/2] ALSA: usb-audio: ignore delay calculation for capture stream

[Pierre-Louis Bossart]

On 11/23/2012 09:21 AM, Takashi Iwai wrote:
> It doesn't make sense to calculate the delay for capture streams in
> the current implementation.  It's always zero, so we should skip the
> computation in snd_usb_pcm_pointer() in the case of capture.

Shouldn't we add support for delay on the capture path instead? We could 
use the same frame counter to report the delay and resync when the urb 
is retired.
-Pierre

Re: [PATCH 1/2] ALSA: usb-audio: ignore delay calculation for capture stream

[Takashi Iwai]

At Mon, 26 Nov 2012 08:24:16 -0600,
Pierre-Louis Bossart wrote:
> 
> On 11/23/2012 09:21 AM, Takashi Iwai wrote:
> > It doesn't make sense to calculate the delay for capture streams in
> > the current implementation.  It's always zero, so we should skip the
> > computation in snd_usb_pcm_pointer() in the case of capture.
> 
> Shouldn't we add support for delay on the capture path instead? We could 
> use the same frame counter to report the delay and resync when the urb 
> is retired.

Sure, it must be possible like that way.
Wouldn't you like to volunteer? ;)


Takashi

Re: [PATCH 1/2] ALSA: usb-audio: ignore delay calculation for capture stream

[Pierre-Louis Bossart]

>>> It doesn't make sense to calculate the delay for capture streams in
>>> the current implementation.  It's always zero, so we should skip the
>>> computation in snd_usb_pcm_pointer() in the case of capture.
>>
>> Shouldn't we add support for delay on the capture path instead? We could
>> use the same frame counter to report the delay and resync when the urb
>> is retired.
>
> Sure, it must be possible like that way.
> Wouldn't you like to volunteer? ;)

Sounds like a good skunkworks for the end of the year...If we have both 
HDA and USB behaving correctly on delays, this would raise the bar for 
everyone.

Re: [PATCH 1/2] ALSA: usb-audio: ignore delay calculation for capture stream

[Pierre-Louis Bossart]

[-- Attachment #1: Type: text/plain, Size: 628 bytes --]


>> Shouldn't we add support for delay on the capture path instead? We could
>> use the same frame counter to report the delay and resync when the urb
>> is retired.
>
> Sure, it must be possible like that way.
> Wouldn't you like to volunteer? ;)

here's a suggestion. Since we don't submit multiple URBs as in the 
playback case, we can only track the delay using the frame counter for a 
single URB, and we reset the counters when the URB is retired.
Did a couple of tests, seems to work ok.
I couldn't figure out if the related patches are already in git, I 
didn't find them. Takashi, did you merge them already?
-Pierre



[-- Attachment #2: usb_delay.patch --]
[-- Type: text/x-patch, Size: 1578 bytes --]

diff --git a/sound/usb/pcm.c b/sound/usb/pcm.c
index c659310..65dc53e 100644
--- a/sound/usb/pcm.c
+++ b/sound/usb/pcm.c
@@ -59,7 +59,12 @@ snd_pcm_uframes_t snd_usb_pcm_delay(struct snd_usb_substream *subs,
 
 	/* 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 / 1000);
+	est_delay =  frame_diff * rate / 1000;
+	if (subs->direction == SNDRV_PCM_STREAM_PLAYBACK)
+		est_delay = subs->last_delay - est_delay;
+	else
+		est_delay = subs->last_delay + est_delay;
+
 	if (est_delay < 0)
 		est_delay = 0;
 	return est_delay;
@@ -1147,6 +1152,10 @@ static void retire_capture_urb(struct snd_usb_substream *subs,
 	int i, period_elapsed = 0;
 	unsigned long flags;
 	unsigned char *cp;
+	int current_frame_number;
+
+	/* read frame number here, update pointer in critical section */
+	current_frame_number = usb_get_current_frame_number(subs->dev);
 
 	stride = runtime->frame_bits >> 3;
 
@@ -1180,6 +1189,13 @@ static void retire_capture_urb(struct snd_usb_substream *subs,
 			subs->transfer_done -= runtime->period_size;
 			period_elapsed = 1;
 		}
+		/* capture delay is by construction limited to one URB, reset delays here */
+		runtime->delay = subs->last_delay = 0;
+
+		/* realign last_frame_number */
+		subs->last_frame_number = current_frame_number;
+		subs->last_frame_number &= 0xFF; /* keep 8 LSBs */
+
 		spin_unlock_irqrestore(&subs->lock, flags);
 		/* copy a data chunk */
 		if (oldptr + bytes > runtime->buffer_size * stride) {

[-- Attachment #3: Type: text/plain, Size: 0 bytes --]

Re: [PATCH 1/2] ALSA: usb-audio: ignore delay calculation for capture stream

[Takashi Iwai]

At Mon, 10 Dec 2012 15:33:56 -0600,
Pierre-Louis Bossart wrote:
> 
> 
> >> Shouldn't we add support for delay on the capture path instead? We could
> >> use the same frame counter to report the delay and resync when the urb
> >> is retired.
> >
> > Sure, it must be possible like that way.
> > Wouldn't you like to volunteer? ;)
> 
> here's a suggestion. Since we don't submit multiple URBs as in the 
> playback case, we can only track the delay using the frame counter for a 
> single URB, and we reset the counters when the URB is retired.
> Did a couple of tests, seems to work ok.

Looks good, but I'd like to merge this after 3.8 merge window.
It's no urgent fix, so better to have a long test for 3.9.

> I couldn't figure out if the related patches are already in git, I 
> didn't find them. Takashi, did you merge them already?

Yes, these have been already queued for 3.8.
See for-next branch of sound git tree.


thanks,

Takashi

> -Pierre
> 
> 
> [2 usb_delay.patch <text/x-patch (7bit)>]
> diff --git a/sound/usb/pcm.c b/sound/usb/pcm.c
> index c659310..65dc53e 100644
> --- a/sound/usb/pcm.c
> +++ b/sound/usb/pcm.c
> @@ -59,7 +59,12 @@ snd_pcm_uframes_t snd_usb_pcm_delay(struct snd_usb_substream *subs,
>  
>  	/* 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 / 1000);
> +	est_delay =  frame_diff * rate / 1000;
> +	if (subs->direction == SNDRV_PCM_STREAM_PLAYBACK)
> +		est_delay = subs->last_delay - est_delay;
> +	else
> +		est_delay = subs->last_delay + est_delay;
> +
>  	if (est_delay < 0)
>  		est_delay = 0;
>  	return est_delay;
> @@ -1147,6 +1152,10 @@ static void retire_capture_urb(struct snd_usb_substream *subs,
>  	int i, period_elapsed = 0;
>  	unsigned long flags;
>  	unsigned char *cp;
> +	int current_frame_number;
> +
> +	/* read frame number here, update pointer in critical section */
> +	current_frame_number = usb_get_current_frame_number(subs->dev);
>  
>  	stride = runtime->frame_bits >> 3;
>  
> @@ -1180,6 +1189,13 @@ static void retire_capture_urb(struct snd_usb_substream *subs,
>  			subs->transfer_done -= runtime->period_size;
>  			period_elapsed = 1;
>  		}
> +		/* capture delay is by construction limited to one URB, reset delays here */
> +		runtime->delay = subs->last_delay = 0;
> +
> +		/* realign last_frame_number */
> +		subs->last_frame_number = current_frame_number;
> +		subs->last_frame_number &= 0xFF; /* keep 8 LSBs */
> +
>  		spin_unlock_irqrestore(&subs->lock, flags);
>  		/* copy a data chunk */
>  		if (oldptr + bytes > runtime->buffer_size * stride) {

[PATCH 2/2] ALSA: usb-audio: fix delay account during pause

[Takashi Iwai]

When a playback stream is paused, the stream isn't actually stopped,
thus we still need to take care of the in-flight data amount for the
delay calculation.  Otherwise the value of subs->last_delay is no
longer reliable and can give a bogus value after resuming from pause.
This will result in "delay: estimated XX, actual YY" error messages.

Also, during pause after all in flight data are processed
(i.e. last_delay = 0), we don't have to calculate the actual delay
from the current frame.  Give a short path in such a case.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/usb/pcm.c | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/sound/usb/pcm.c b/sound/usb/pcm.c
index 8e1d5e0..7c64b95 100644
--- a/sound/usb/pcm.c
+++ b/sound/usb/pcm.c
@@ -46,6 +46,9 @@ snd_pcm_uframes_t snd_usb_pcm_delay(struct snd_usb_substream *subs,
 	int frame_diff;
 	int est_delay;
 
+	if (!subs->last_delay)
+		return 0; /* short path */
+
 	current_frame_number = usb_get_current_frame_number(subs->dev);
 	/*
 	 * HCD implementations use different widths, use lower 8 bits.
@@ -1195,6 +1198,9 @@ static void retire_playback_urb(struct snd_usb_substream *subs,
 		return;
 
 	spin_lock_irqsave(&subs->lock, flags);
+	if (!subs->last_delay)
+		goto out; /* short path */
+
 	est_delay = snd_usb_pcm_delay(subs, runtime->rate);
 	/* update delay with exact number of samples played */
 	if (processed > subs->last_delay)
@@ -1212,6 +1218,15 @@ static void retire_playback_urb(struct snd_usb_substream *subs,
 		snd_printk(KERN_DEBUG "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);
 }
 
@@ -1253,7 +1268,8 @@ static int snd_usb_substream_playback_trigger(struct snd_pcm_substream *substrea
 		return 0;
 	case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
 		subs->data_endpoint->prepare_data_urb = NULL;
-		subs->data_endpoint->retire_data_urb = NULL;
+		/* keep retire_data_urb for delay calculation */
+		subs->data_endpoint->retire_data_urb = retire_playback_urb;
 		subs->running = 0;
 		return 0;
 	}
-- 
1.8.0