PCM delay compensation

PCM delay compensation

[Takashi Iwai]

Hi,

the patch below (to the latest sound git tree) adds the extra delay
count for USB-audio driver.  This change will appear as the return
value of snd_pcm_delay().

Could you check whether it's appropriate behavior you've wanted?


thanks,

Takashi

diff --git a/include/sound/pcm.h b/include/sound/pcm.h
index 40c5a6f..dcbdc60 100644
--- a/include/sound/pcm.h
+++ b/include/sound/pcm.h
@@ -269,6 +269,7 @@ struct snd_pcm_runtime {
 	snd_pcm_uframes_t avail_max;
 	snd_pcm_uframes_t hw_ptr_base;	/* Position at buffer restart */
 	snd_pcm_uframes_t hw_ptr_interrupt; /* Position at interrupt time*/
+	snd_pcm_sframes_t delay;	/* extra delay; typically FIFO size */
 
 	/* -- HW params -- */
 	snd_pcm_access_t access;	/* access mode */
diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
index e61e125..df7c3fa 100644
--- a/sound/core/pcm_native.c
+++ b/sound/core/pcm_native.c
@@ -597,14 +597,15 @@ int snd_pcm_status(struct snd_pcm_substream *substream,
 	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
 		status->avail = snd_pcm_playback_avail(runtime);
 		if (runtime->status->state == SNDRV_PCM_STATE_RUNNING ||
-		    runtime->status->state == SNDRV_PCM_STATE_DRAINING)
+		    runtime->status->state == SNDRV_PCM_STATE_DRAINING) {
 			status->delay = runtime->buffer_size - status->avail;
-		else
+			status->delay += runtime->delay;
+		} else
 			status->delay = 0;
 	} else {
 		status->avail = snd_pcm_capture_avail(runtime);
 		if (runtime->status->state == SNDRV_PCM_STATE_RUNNING)
-			status->delay = status->avail;
+			status->delay = status->avail + runtime->delay;
 		else
 			status->delay = 0;
 	}
@@ -2423,6 +2424,7 @@ static int snd_pcm_delay(struct snd_pcm_substream *substream,
 			n = snd_pcm_playback_hw_avail(runtime);
 		else
 			n = snd_pcm_capture_avail(runtime);
+		n += runtime->delay;
 		break;
 	case SNDRV_PCM_STATE_XRUN:
 		err = -EPIPE;
diff --git a/sound/usb/usbaudio.c b/sound/usb/usbaudio.c
index 6e70ba4..7d5a103 100644
--- a/sound/usb/usbaudio.c
+++ b/sound/usb/usbaudio.c
@@ -629,6 +629,7 @@ static int prepare_playback_urb(struct snd_usb_substream *subs,
 	subs->hwptr_done += offs;
 	if (subs->hwptr_done >= runtime->buffer_size)
 		subs->hwptr_done -= runtime->buffer_size;
+	runtime->delay += offs;
 	spin_unlock_irqrestore(&subs->lock, flags);
 	urb->transfer_buffer_length = offs * stride;
 	if (period_elapsed)
@@ -638,12 +639,20 @@ static int prepare_playback_urb(struct snd_usb_substream *subs,
 
 /*
  * process after playback data complete
- * - nothing to do
+ * - decrease the delay count again
  */
 static int retire_playback_urb(struct snd_usb_substream *subs,
 			       struct snd_pcm_runtime *runtime,
 			       struct urb *urb)
 {
+	unsigned long flags;
+	int stride = runtime->frame_bits >> 3;
+	int processed = urb->transfer_buffer_length / stride;
+
+	spin_lock_irqsave(&subs->lock, flags);
+	if (processed > runtime->delay)
+		runtime->delay -= processed;
+	spin_unlock_irqrestore(&subs->lock, flags);
 	return 0;
 }
 
@@ -1542,6 +1551,7 @@ static int snd_usb_pcm_prepare(struct snd_pcm_substream *substream)
 	subs->hwptr_done = 0;
 	subs->transfer_done = 0;
 	subs->phase = 0;
+	runtime->delay = 0;
 
 	/* clear urbs (to be sure) */
 	deactivate_urbs(subs, 0, 1);

Re: PCM delay compensation

[Takashi Iwai]

At Tue, 07 Oct 2008 17:32:05 +0200,
I wrote:
> 
> Hi,
> 
> the patch below (to the latest sound git tree) adds the extra delay
> count for USB-audio driver.  This change will appear as the return
> value of snd_pcm_delay().
> 
> Could you check whether it's appropriate behavior you've wanted?

Ping.


Takashi

> 
> 
> thanks,
> 
> Takashi
> 
> diff --git a/include/sound/pcm.h b/include/sound/pcm.h
> index 40c5a6f..dcbdc60 100644
> --- a/include/sound/pcm.h
> +++ b/include/sound/pcm.h
> @@ -269,6 +269,7 @@ struct snd_pcm_runtime {
>  	snd_pcm_uframes_t avail_max;
>  	snd_pcm_uframes_t hw_ptr_base;	/* Position at buffer restart */
>  	snd_pcm_uframes_t hw_ptr_interrupt; /* Position at interrupt time*/
> +	snd_pcm_sframes_t delay;	/* extra delay; typically FIFO size */
>  
>  	/* -- HW params -- */
>  	snd_pcm_access_t access;	/* access mode */
> diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
> index e61e125..df7c3fa 100644
> --- a/sound/core/pcm_native.c
> +++ b/sound/core/pcm_native.c
> @@ -597,14 +597,15 @@ int snd_pcm_status(struct snd_pcm_substream *substream,
>  	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
>  		status->avail = snd_pcm_playback_avail(runtime);
>  		if (runtime->status->state == SNDRV_PCM_STATE_RUNNING ||
> -		    runtime->status->state == SNDRV_PCM_STATE_DRAINING)
> +		    runtime->status->state == SNDRV_PCM_STATE_DRAINING) {
>  			status->delay = runtime->buffer_size - status->avail;
> -		else
> +			status->delay += runtime->delay;
> +		} else
>  			status->delay = 0;
>  	} else {
>  		status->avail = snd_pcm_capture_avail(runtime);
>  		if (runtime->status->state == SNDRV_PCM_STATE_RUNNING)
> -			status->delay = status->avail;
> +			status->delay = status->avail + runtime->delay;
>  		else
>  			status->delay = 0;
>  	}
> @@ -2423,6 +2424,7 @@ static int snd_pcm_delay(struct snd_pcm_substream *substream,
>  			n = snd_pcm_playback_hw_avail(runtime);
>  		else
>  			n = snd_pcm_capture_avail(runtime);
> +		n += runtime->delay;
>  		break;
>  	case SNDRV_PCM_STATE_XRUN:
>  		err = -EPIPE;
> diff --git a/sound/usb/usbaudio.c b/sound/usb/usbaudio.c
> index 6e70ba4..7d5a103 100644
> --- a/sound/usb/usbaudio.c
> +++ b/sound/usb/usbaudio.c
> @@ -629,6 +629,7 @@ static int prepare_playback_urb(struct snd_usb_substream *subs,
>  	subs->hwptr_done += offs;
>  	if (subs->hwptr_done >= runtime->buffer_size)
>  		subs->hwptr_done -= runtime->buffer_size;
> +	runtime->delay += offs;
>  	spin_unlock_irqrestore(&subs->lock, flags);
>  	urb->transfer_buffer_length = offs * stride;
>  	if (period_elapsed)
> @@ -638,12 +639,20 @@ static int prepare_playback_urb(struct snd_usb_substream *subs,
>  
>  /*
>   * process after playback data complete
> - * - nothing to do
> + * - decrease the delay count again
>   */
>  static int retire_playback_urb(struct snd_usb_substream *subs,
>  			       struct snd_pcm_runtime *runtime,
>  			       struct urb *urb)
>  {
> +	unsigned long flags;
> +	int stride = runtime->frame_bits >> 3;
> +	int processed = urb->transfer_buffer_length / stride;
> +
> +	spin_lock_irqsave(&subs->lock, flags);
> +	if (processed > runtime->delay)
> +		runtime->delay -= processed;
> +	spin_unlock_irqrestore(&subs->lock, flags);
>  	return 0;
>  }
>  
> @@ -1542,6 +1551,7 @@ static int snd_usb_pcm_prepare(struct snd_pcm_substream *substream)
>  	subs->hwptr_done = 0;
>  	subs->transfer_done = 0;
>  	subs->phase = 0;
> +	runtime->delay = 0;
>  
>  	/* clear urbs (to be sure) */
>  	deactivate_urbs(subs, 0, 1);

Re: PCM delay compensation

[Lennart Poettering]

On Tue, 07.10.08 17:32, Takashi Iwai (tiwai@suse.de) wrote:

> Hi,

Sorry for the overly long delay.
> 
> the patch below (to the latest sound git tree) adds the extra delay
> count for USB-audio driver.  This change will appear as the return
> value of snd_pcm_delay().
> 
> Could you check whether it's appropriate behavior you've wanted?

I have now tested this patch on the current 2.6.29-rc5 kernel. The
effect is that snd_pcm_delay() returns always increasing values, as if
the playback never proceeds. Most movie players which need that call
to synchronize video frames hence stall completely.

Lennart


> diff --git a/include/sound/pcm.h b/include/sound/pcm.h
> index 40c5a6f..dcbdc60 100644
> --- a/include/sound/pcm.h
> +++ b/include/sound/pcm.h
> @@ -269,6 +269,7 @@ struct snd_pcm_runtime {
>  	snd_pcm_uframes_t avail_max;
>  	snd_pcm_uframes_t hw_ptr_base;	/* Position at buffer restart */
>  	snd_pcm_uframes_t hw_ptr_interrupt; /* Position at interrupt time*/
> +	snd_pcm_sframes_t delay;	/* extra delay; typically FIFO size */
>  
>  	/* -- HW params -- */
>  	snd_pcm_access_t access;	/* access mode */
> diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
> index e61e125..df7c3fa 100644
> --- a/sound/core/pcm_native.c
> +++ b/sound/core/pcm_native.c
> @@ -597,14 +597,15 @@ int snd_pcm_status(struct snd_pcm_substream *substream,
>  	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
>  		status->avail = snd_pcm_playback_avail(runtime);
>  		if (runtime->status->state == SNDRV_PCM_STATE_RUNNING ||
> -		    runtime->status->state == SNDRV_PCM_STATE_DRAINING)
> +		    runtime->status->state == SNDRV_PCM_STATE_DRAINING) {
>  			status->delay = runtime->buffer_size - status->avail;
> -		else
> +			status->delay += runtime->delay;
> +		} else
>  			status->delay = 0;
>  	} else {
>  		status->avail = snd_pcm_capture_avail(runtime);
>  		if (runtime->status->state == SNDRV_PCM_STATE_RUNNING)
> -			status->delay = status->avail;
> +			status->delay = status->avail + runtime->delay;
>  		else
>  			status->delay = 0;
>  	}
> @@ -2423,6 +2424,7 @@ static int snd_pcm_delay(struct snd_pcm_substream *substream,
>  			n = snd_pcm_playback_hw_avail(runtime);
>  		else
>  			n = snd_pcm_capture_avail(runtime);
> +		n += runtime->delay;
>  		break;
>  	case SNDRV_PCM_STATE_XRUN:
>  		err = -EPIPE;
> diff --git a/sound/usb/usbaudio.c b/sound/usb/usbaudio.c
> index 6e70ba4..7d5a103 100644
> --- a/sound/usb/usbaudio.c
> +++ b/sound/usb/usbaudio.c
> @@ -629,6 +629,7 @@ static int prepare_playback_urb(struct snd_usb_substream *subs,
>  	subs->hwptr_done += offs;
>  	if (subs->hwptr_done >= runtime->buffer_size)
>  		subs->hwptr_done -= runtime->buffer_size;
> +	runtime->delay += offs;
>  	spin_unlock_irqrestore(&subs->lock, flags);
>  	urb->transfer_buffer_length = offs * stride;
>  	if (period_elapsed)
> @@ -638,12 +639,20 @@ static int prepare_playback_urb(struct snd_usb_substream *subs,
>  
>  /*
>   * process after playback data complete
> - * - nothing to do
> + * - decrease the delay count again
>   */
>  static int retire_playback_urb(struct snd_usb_substream *subs,
>  			       struct snd_pcm_runtime *runtime,
>  			       struct urb *urb)
>  {
> +	unsigned long flags;
> +	int stride = runtime->frame_bits >> 3;
> +	int processed = urb->transfer_buffer_length / stride;
> +
> +	spin_lock_irqsave(&subs->lock, flags);
> +	if (processed > runtime->delay)
> +		runtime->delay -= processed;
> +	spin_unlock_irqrestore(&subs->lock, flags);
>  	return 0;
>  }
>  
> @@ -1542,6 +1551,7 @@ static int snd_usb_pcm_prepare(struct snd_pcm_substream *substream)
>  	subs->hwptr_done = 0;
>  	subs->transfer_done = 0;
>  	subs->phase = 0;
> +	runtime->delay = 0;
>  
>  	/* clear urbs (to be sure) */
>  	deactivate_urbs(subs, 0, 1);
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel


Lennart

-- 
Lennart Poettering                        Red Hat, Inc.
lennart [at] poettering [dot] net         ICQ# 11060553
http://0pointer.net/lennart/           GnuPG 0x1A015CC4

Re: PCM delay compensation

[Takashi Iwai]

At Sun, 22 Feb 2009 00:45:04 +0100,
Lennart Poettering wrote:
> 
> On Tue, 07.10.08 17:32, Takashi Iwai (tiwai@suse.de) wrote:
> 
> > Hi,
> 
> Sorry for the overly long delay.
> > 
> > the patch below (to the latest sound git tree) adds the extra delay
> > count for USB-audio driver.  This change will appear as the return
> > value of snd_pcm_delay().
> > 
> > Could you check whether it's appropriate behavior you've wanted?
> 
> I have now tested this patch on the current 2.6.29-rc5 kernel. The
> effect is that snd_pcm_delay() returns always increasing values, as if
> the playback never proceeds. Most movie players which need that call
> to synchronize video frames hence stall completely.

OK, that's bad.  However, the increased value of snd_pcm_delay() is
exactly the effect.  The usb-audio always prefetch the buffer in
advance.

That means, changing (or "fixing") snd_pcm_delay() would cause many
regressions with many apps -- thus basically we are not allowed to
change the semantics any more at this too late point.

I feel it's better to introduce another kernel-side API to give a
better sync/timing information, and mark snd_pcm_delay as obsolete for
future (although we can never deprecate such a basic API).


Takashi

> 
> Lennart
> 
> 
> > diff --git a/include/sound/pcm.h b/include/sound/pcm.h
> > index 40c5a6f..dcbdc60 100644
> > --- a/include/sound/pcm.h
> > +++ b/include/sound/pcm.h
> > @@ -269,6 +269,7 @@ struct snd_pcm_runtime {
> >  	snd_pcm_uframes_t avail_max;
> >  	snd_pcm_uframes_t hw_ptr_base;	/* Position at buffer restart */
> >  	snd_pcm_uframes_t hw_ptr_interrupt; /* Position at interrupt time*/
> > +	snd_pcm_sframes_t delay;	/* extra delay; typically FIFO size */
> >  
> >  	/* -- HW params -- */
> >  	snd_pcm_access_t access;	/* access mode */
> > diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
> > index e61e125..df7c3fa 100644
> > --- a/sound/core/pcm_native.c
> > +++ b/sound/core/pcm_native.c
> > @@ -597,14 +597,15 @@ int snd_pcm_status(struct snd_pcm_substream *substream,
> >  	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
> >  		status->avail = snd_pcm_playback_avail(runtime);
> >  		if (runtime->status->state == SNDRV_PCM_STATE_RUNNING ||
> > -		    runtime->status->state == SNDRV_PCM_STATE_DRAINING)
> > +		    runtime->status->state == SNDRV_PCM_STATE_DRAINING) {
> >  			status->delay = runtime->buffer_size - status->avail;
> > -		else
> > +			status->delay += runtime->delay;
> > +		} else
> >  			status->delay = 0;
> >  	} else {
> >  		status->avail = snd_pcm_capture_avail(runtime);
> >  		if (runtime->status->state == SNDRV_PCM_STATE_RUNNING)
> > -			status->delay = status->avail;
> > +			status->delay = status->avail + runtime->delay;
> >  		else
> >  			status->delay = 0;
> >  	}
> > @@ -2423,6 +2424,7 @@ static int snd_pcm_delay(struct snd_pcm_substream *substream,
> >  			n = snd_pcm_playback_hw_avail(runtime);
> >  		else
> >  			n = snd_pcm_capture_avail(runtime);
> > +		n += runtime->delay;
> >  		break;
> >  	case SNDRV_PCM_STATE_XRUN:
> >  		err = -EPIPE;
> > diff --git a/sound/usb/usbaudio.c b/sound/usb/usbaudio.c
> > index 6e70ba4..7d5a103 100644
> > --- a/sound/usb/usbaudio.c
> > +++ b/sound/usb/usbaudio.c
> > @@ -629,6 +629,7 @@ static int prepare_playback_urb(struct snd_usb_substream *subs,
> >  	subs->hwptr_done += offs;
> >  	if (subs->hwptr_done >= runtime->buffer_size)
> >  		subs->hwptr_done -= runtime->buffer_size;
> > +	runtime->delay += offs;
> >  	spin_unlock_irqrestore(&subs->lock, flags);
> >  	urb->transfer_buffer_length = offs * stride;
> >  	if (period_elapsed)
> > @@ -638,12 +639,20 @@ static int prepare_playback_urb(struct snd_usb_substream *subs,
> >  
> >  /*
> >   * process after playback data complete
> > - * - nothing to do
> > + * - decrease the delay count again
> >   */
> >  static int retire_playback_urb(struct snd_usb_substream *subs,
> >  			       struct snd_pcm_runtime *runtime,
> >  			       struct urb *urb)
> >  {
> > +	unsigned long flags;
> > +	int stride = runtime->frame_bits >> 3;
> > +	int processed = urb->transfer_buffer_length / stride;
> > +
> > +	spin_lock_irqsave(&subs->lock, flags);
> > +	if (processed > runtime->delay)
> > +		runtime->delay -= processed;
> > +	spin_unlock_irqrestore(&subs->lock, flags);
> >  	return 0;
> >  }
> >  
> > @@ -1542,6 +1551,7 @@ static int snd_usb_pcm_prepare(struct snd_pcm_substream *substream)
> >  	subs->hwptr_done = 0;
> >  	subs->transfer_done = 0;
> >  	subs->phase = 0;
> > +	runtime->delay = 0;
> >  
> >  	/* clear urbs (to be sure) */
> >  	deactivate_urbs(subs, 0, 1);
> > _______________________________________________
> > Alsa-devel mailing list
> > Alsa-devel@alsa-project.org
> > http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
> 
> 
> Lennart
> 
> -- 
> Lennart Poettering                        Red Hat, Inc.
> lennart [at] poettering [dot] net         ICQ# 11060553
> http://0pointer.net/lennart/           GnuPG 0x1A015CC4
> 

Re: PCM delay compensation

[Lennart Poettering]

On Mon, 23.02.09 08:45, Takashi Iwai (tiwai@suse.de) wrote:

> 
> At Sun, 22 Feb 2009 00:45:04 +0100,
> Lennart Poettering wrote:
> > 
> > On Tue, 07.10.08 17:32, Takashi Iwai (tiwai@suse.de) wrote:
> > 
> > > Hi,
> > 
> > Sorry for the overly long delay.
> > > 
> > > the patch below (to the latest sound git tree) adds the extra delay
> > > count for USB-audio driver.  This change will appear as the return
> > > value of snd_pcm_delay().
> > > 
> > > Could you check whether it's appropriate behavior you've wanted?
> > 
> > I have now tested this patch on the current 2.6.29-rc5 kernel. The
> > effect is that snd_pcm_delay() returns always increasing values, as if
> > the playback never proceeds. Most movie players which need that call
> > to synchronize video frames hence stall completely.
> 
> OK, that's bad.  However, the increased value of snd_pcm_delay() is
> exactly the effect.  The usb-audio always prefetch the buffer in
> advance.
> 
> That means, changing (or "fixing") snd_pcm_delay() would cause many
> regressions with many apps -- thus basically we are not allowed to
> change the semantics any more at this too late point.
> 
> I feel it's better to introduce another kernel-side API to give a
> better sync/timing information, and mark snd_pcm_delay as obsolete for
> future (although we can never deprecate such a basic API).

No, snd_pcm_delay() with this patch is clearly broken: it apparently
increases at the same speed as the hw ptr, ignoring the app
ptr. i.e. after 5 min of playback the delay will be reported a 5 mins!
After 10 min of playback the delay will be reported as 10 mins!

This is *not* a simple incompatibility with mplayer. The patch is
borked.  This has nothing to do with changing semantics and creating
incompatibilities.

Lennart

-- 
Lennart Poettering                        Red Hat, Inc.
lennart [at] poettering [dot] net         ICQ# 11060553
http://0pointer.net/lennart/           GnuPG 0x1A015CC4

Re: PCM delay compensation

[Takashi Iwai]

At Mon, 23 Feb 2009 17:24:32 +0100,
Lennart Poettering wrote:
> 
> On Mon, 23.02.09 08:45, Takashi Iwai (tiwai@suse.de) wrote:
> 
> > 
> > At Sun, 22 Feb 2009 00:45:04 +0100,
> > Lennart Poettering wrote:
> > > 
> > > On Tue, 07.10.08 17:32, Takashi Iwai (tiwai@suse.de) wrote:
> > > 
> > > > Hi,
> > > 
> > > Sorry for the overly long delay.
> > > > 
> > > > the patch below (to the latest sound git tree) adds the extra delay
> > > > count for USB-audio driver.  This change will appear as the return
> > > > value of snd_pcm_delay().
> > > > 
> > > > Could you check whether it's appropriate behavior you've wanted?
> > > 
> > > I have now tested this patch on the current 2.6.29-rc5 kernel. The
> > > effect is that snd_pcm_delay() returns always increasing values, as if
> > > the playback never proceeds. Most movie players which need that call
> > > to synchronize video frames hence stall completely.
> > 
> > OK, that's bad.  However, the increased value of snd_pcm_delay() is
> > exactly the effect.  The usb-audio always prefetch the buffer in
> > advance.
> > 
> > That means, changing (or "fixing") snd_pcm_delay() would cause many
> > regressions with many apps -- thus basically we are not allowed to
> > change the semantics any more at this too late point.
> > 
> > I feel it's better to introduce another kernel-side API to give a
> > better sync/timing information, and mark snd_pcm_delay as obsolete for
> > future (although we can never deprecate such a basic API).
> 
> No, snd_pcm_delay() with this patch is clearly broken: it apparently
> increases at the same speed as the hw ptr, ignoring the app
> ptr. i.e. after 5 min of playback the delay will be reported a 5 mins!
> After 10 min of playback the delay will be reported as 10 mins!

Ah, then it must be a buggy patch indeed.  It didn't appear in my
some test cases at that time, so didn't check after that.

Maybe some delay account in complete callback wasn't correct.
Another item to my TODO list. 


thanks,

Takashi

Re: PCM delay compensation

[Takashi Iwai]

At Mon, 23 Feb 2009 17:28:04 +0100,
I wrote:
> 
> At Mon, 23 Feb 2009 17:24:32 +0100,
> Lennart Poettering wrote:
> > 
> > On Mon, 23.02.09 08:45, Takashi Iwai (tiwai@suse.de) wrote:
> > 
> > > 
> > > At Sun, 22 Feb 2009 00:45:04 +0100,
> > > Lennart Poettering wrote:
> > > > 
> > > > On Tue, 07.10.08 17:32, Takashi Iwai (tiwai@suse.de) wrote:
> > > > 
> > > > > Hi,
> > > > 
> > > > Sorry for the overly long delay.
> > > > > 
> > > > > the patch below (to the latest sound git tree) adds the extra delay
> > > > > count for USB-audio driver.  This change will appear as the return
> > > > > value of snd_pcm_delay().
> > > > > 
> > > > > Could you check whether it's appropriate behavior you've wanted?
> > > > 
> > > > I have now tested this patch on the current 2.6.29-rc5 kernel. The
> > > > effect is that snd_pcm_delay() returns always increasing values, as if
> > > > the playback never proceeds. Most movie players which need that call
> > > > to synchronize video frames hence stall completely.
> > > 
> > > OK, that's bad.  However, the increased value of snd_pcm_delay() is
> > > exactly the effect.  The usb-audio always prefetch the buffer in
> > > advance.
> > > 
> > > That means, changing (or "fixing") snd_pcm_delay() would cause many
> > > regressions with many apps -- thus basically we are not allowed to
> > > change the semantics any more at this too late point.
> > > 
> > > I feel it's better to introduce another kernel-side API to give a
> > > better sync/timing information, and mark snd_pcm_delay as obsolete for
> > > future (although we can never deprecate such a basic API).
> > 
> > No, snd_pcm_delay() with this patch is clearly broken: it apparently
> > increases at the same speed as the hw ptr, ignoring the app
> > ptr. i.e. after 5 min of playback the delay will be reported a 5 mins!
> > After 10 min of playback the delay will be reported as 10 mins!
> 
> Ah, then it must be a buggy patch indeed.  It didn't appear in my
> some test cases at that time, so didn't check after that.
> 
> Maybe some delay account in complete callback wasn't correct.

Maybe a stupid typo below (or removed lines accidentally at
merge/rebase)...


Takashi

---
diff --git a/sound/usb/usbaudio.c b/sound/usb/usbaudio.c
index 03384db..7b362d1 100644
--- a/sound/usb/usbaudio.c
+++ b/sound/usb/usbaudio.c
@@ -651,6 +651,8 @@ static int retire_playback_urb(struct snd_usb_substream *subs,
 
 	spin_lock_irqsave(&subs->lock, flags);
 	if (processed > runtime->delay)
+		runtime->delay = 0;
+	else
 		runtime->delay -= processed;
 	spin_unlock_irqrestore(&subs->lock, flags);
 	return 0;