All of lore.kernel.org
 help / color / mirror / Atom feed
* bug in alsa-lib/snd_pcm_plugin_delay for capture?
@ 2010-11-16  7:08 pl bossart
  2010-11-18  8:28 ` Jaroslav Kysela
  0 siblings, 1 reply; 10+ messages in thread
From: pl bossart @ 2010-11-16  7:08 UTC (permalink / raw)
  To: alsa-devel; +Cc: wim.taymans

Since the PulseAudio commit 29acfd0e0413a9bd126782763ee2dcf10357546 I
am seeing errors messages telling me that the information reported by
snd_pcm_avail_delay is not accurate on the capture path, more
specifically the delay is lower than the available samples.
I spent a little bit of time to find the root cause, and I believe
this is a problem with alsa-lib plugins. I never see any errors when
using hw devices. Conversely I always have these errors when using the
front: plugin (the pulseaudio default).
When I looked at the code, I saw that snd_pcm_plugin_delay and
snd_pcm_plugin_avail_update don't really match. There is additional
code in snd_pcm_plugin_avail_update for the capture case that is not
present when in the delay code, and that causes the delta between the
values reported.
Since I don't understand this code that has all kinds of
mmap_commit/forward/undos, I have to ask higher powers for advice.
Jaroslav, could you take a look at the code and let me know if my
analysis is correct?
Thanks,
-Pierre

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: bug in alsa-lib/snd_pcm_plugin_delay for capture?
  2010-11-16  7:08 bug in alsa-lib/snd_pcm_plugin_delay for capture? pl bossart
@ 2010-11-18  8:28 ` Jaroslav Kysela
  2010-11-22 13:35   ` pl bossart
  0 siblings, 1 reply; 10+ messages in thread
From: Jaroslav Kysela @ 2010-11-18  8:28 UTC (permalink / raw)
  To: pl bossart; +Cc: alsa-devel, wim.taymans

On Tue, 16 Nov 2010, pl bossart wrote:

> Since the PulseAudio commit 29acfd0e0413a9bd126782763ee2dcf10357546 I
> am seeing errors messages telling me that the information reported by
> snd_pcm_avail_delay is not accurate on the capture path, more
> specifically the delay is lower than the available samples.

Could you test patch bellow?

http://git.alsa-project.org/?p=alsa-lib.git;a=commitdiff;h=ba9332e9192814a5431a3a2505d25d74a9232124

 						Jaroslav

-----
Jaroslav Kysela <perex@perex.cz>
Linux Kernel Sound Maintainer
ALSA Project, Red Hat, Inc.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: bug in alsa-lib/snd_pcm_plugin_delay for capture?
  2010-11-18  8:28 ` Jaroslav Kysela
@ 2010-11-22 13:35   ` pl bossart
  2010-11-22 14:01     ` Jaroslav Kysela
  0 siblings, 1 reply; 10+ messages in thread
From: pl bossart @ 2010-11-22 13:35 UTC (permalink / raw)
  To: Jaroslav Kysela; +Cc: alsa-devel, wim.taymans

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

>> Since the PulseAudio commit 29acfd0e0413a9bd126782763ee2dcf10357546 I
>> am seeing errors messages telling me that the information reported by
>> snd_pcm_avail_delay is not accurate on the capture path, more
>> specifically the delay is lower than the available samples.
>
> Could you test patch bellow?
>
> http://git.alsa-project.org/?p=alsa-lib.git;a=commitdiff;h=ba9332e9192814a5431a3a2505d25d74a9232124

Nah, no luck. PulseAudio still reports an error and if I add
assert(*delayp >= *availp) the code bombs out.
Here is one way to solve the problem. I don't claim it's the right
one, but it's compatible with the way the availp value is computed,
and it makes the error message go away in PulseAudio.

While I was at it I also removed the .client_frames and .slave_frames
methods that no one seems to use. This seems to be dead code that
hasn't been touched in years.
-Pierre

[-- Attachment #2: 0001-remove-unused-client_frames-and-slave_frames-routine.patch --]
[-- Type: application/octet-stream, Size: 4109 bytes --]

From 670c7a32ad21239f49b78e5176ac095c345c3ac3 Mon Sep 17 00:00:00 2001
From: Pierre-Louis Bossart <pierre-louis.bossart@intel.com>
Date: Fri, 19 Nov 2010 10:35:31 -0600
Subject: [PATCH 1/2] remove unused client_frames and slave_frames routines

plug->client_frames and plug->slave_frames are not used by
any plugin, remove dead code.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@intel.com>
---
 src/pcm/pcm_plugin.c |   33 ++++-----------------------------
 src/pcm/pcm_plugin.h |    2 --
 2 files changed, 4 insertions(+), 31 deletions(-)

diff --git a/src/pcm/pcm_plugin.c b/src/pcm/pcm_plugin.c
index 0ef394a..19d0dee 100644
--- a/src/pcm/pcm_plugin.c
+++ b/src/pcm/pcm_plugin.c
@@ -144,8 +144,6 @@ static int snd_pcm_plugin_delay(snd_pcm_t *pcm, snd_pcm_sframes_t *delayp)
 	int err = snd_pcm_delay(plugin->gen.slave, &sd);
 	if (err < 0)
 		return err;
-	if (plugin->client_frames)
-		sd = plugin->client_frames(pcm, sd);
 	*delayp = sd;
 	return 0;
 }
@@ -208,18 +206,13 @@ static snd_pcm_sframes_t snd_pcm_plugin_rewind(snd_pcm_t *pcm, snd_pcm_uframes_t
 	if (frames == 0)
 		return 0;
 	
-	if (plugin->slave_frames)
-		sframes = plugin->slave_frames(pcm, (snd_pcm_sframes_t) frames);
-	else
-		sframes = frames;
+        sframes = frames;
 	snd_atomic_write_begin(&plugin->watom);
 	sframes = snd_pcm_rewind(plugin->gen.slave, sframes);
 	if (sframes < 0) {
 		snd_atomic_write_end(&plugin->watom);
 		return sframes;
 	}
-	if (plugin->client_frames)
-		frames = plugin->client_frames(pcm, sframes);
 	snd_pcm_mmap_appl_backward(pcm, (snd_pcm_uframes_t) frames);
 	snd_atomic_write_end(&plugin->watom);
 	return (snd_pcm_sframes_t) frames;
@@ -241,18 +234,13 @@ static snd_pcm_sframes_t snd_pcm_plugin_forward(snd_pcm_t *pcm, snd_pcm_uframes_
 	if (frames == 0)
 		return 0;
 	
-	if (plugin->slave_frames)
-		sframes = plugin->slave_frames(pcm, (snd_pcm_sframes_t) frames);
-	else
-		sframes = frames;
+        sframes = frames;
 	snd_atomic_write_begin(&plugin->watom);
 	sframes = INTERNAL(snd_pcm_forward)(plugin->gen.slave, sframes);
 	if (sframes < 0) {
 		snd_atomic_write_end(&plugin->watom);
 		return sframes;
 	}
-	if (plugin->client_frames)
-		frames = plugin->client_frames(pcm, sframes);
 	snd_pcm_mmap_appl_forward(pcm, (snd_pcm_uframes_t) frames);
 	snd_atomic_write_end(&plugin->watom);
 	return (snd_pcm_sframes_t) frames;
@@ -469,15 +457,8 @@ static snd_pcm_sframes_t snd_pcm_plugin_avail_update(snd_pcm_t *pcm)
 	    pcm->access != SND_PCM_ACCESS_RW_INTERLEAVED &&
 	    pcm->access != SND_PCM_ACCESS_RW_NONINTERLEAVED)
 		goto _capture;
-	if (plugin->client_frames) {
-		*pcm->hw.ptr = plugin->client_frames(pcm, *slave->hw.ptr);
-		if (slave_size <= 0)
-			return slave_size;
-		return plugin->client_frames(pcm, slave_size);
-	} else {
-		*pcm->hw.ptr = *slave->hw.ptr;
-		return slave_size;
-	}
+        *pcm->hw.ptr = *slave->hw.ptr;
+        return slave_size;
  _capture:
  	{
 		const snd_pcm_channel_area_t *areas;
@@ -547,16 +528,10 @@ static int snd_pcm_plugin_status(snd_pcm_t *pcm, snd_pcm_status_t * status)
 	}
 	status->appl_ptr = *pcm->appl.ptr;
 	status->hw_ptr = *pcm->hw.ptr;
-	if (plugin->client_frames) {
-		status->delay = plugin->client_frames(pcm, status->delay);
-		status->avail = plugin->client_frames(pcm, status->avail);
-	}
 	if (!snd_atomic_read_ok(&ratom)) {
 		snd_atomic_read_wait(&ratom);
 		goto _again;
 	}
-	if (plugin->client_frames)
-		status->avail_max = plugin->client_frames(pcm, (snd_pcm_sframes_t) status->avail_max);
 	return 0;
 }
 
diff --git a/src/pcm/pcm_plugin.h b/src/pcm/pcm_plugin.h
index dfcf6de..7ee7c7f 100644
--- a/src/pcm/pcm_plugin.h
+++ b/src/pcm/pcm_plugin.h
@@ -44,8 +44,6 @@ typedef struct {
 	snd_pcm_slave_xfer_areas_func_t write;
 	snd_pcm_slave_xfer_areas_undo_func_t undo_read;
 	snd_pcm_slave_xfer_areas_undo_func_t undo_write;
-	snd_pcm_sframes_t (*client_frames)(snd_pcm_t *pcm, snd_pcm_sframes_t frames);
-	snd_pcm_sframes_t (*slave_frames)(snd_pcm_t *pcm, snd_pcm_sframes_t frames);
 	int (*init)(snd_pcm_t *pcm);
 	snd_pcm_uframes_t appl_ptr, hw_ptr;
 	snd_atomic_write_t watom;
-- 
1.7.2.3


[-- Attachment #3: 0002-pcm_plugin-fix-capture-delay.patch --]
[-- Type: application/octet-stream, Size: 1656 bytes --]

From c8288abde58aa46ff013772365d6dbe9839ae88d Mon Sep 17 00:00:00 2001
From: Pierre-Louis Bossart <pierre-louis.bossart@intel.com>
Date: Fri, 19 Nov 2010 11:28:40 -0600
Subject: [PATCH 2/2] pcm_plugin: fix capture delay

The existing code only reported the slave delay. Add additional
buffering done at the plugin level.
This makes sure delay >= avail, otherwise the values reported
by snd_pcm_avail_delay don't make any sense.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@intel.com>
---
 src/pcm/pcm_plugin.c |   14 +++++++++++++-
 1 files changed, 13 insertions(+), 1 deletions(-)

diff --git a/src/pcm/pcm_plugin.c b/src/pcm/pcm_plugin.c
index 19d0dee..2a7934b 100644
--- a/src/pcm/pcm_plugin.c
+++ b/src/pcm/pcm_plugin.c
@@ -141,11 +141,23 @@ static int snd_pcm_plugin_delay(snd_pcm_t *pcm, snd_pcm_sframes_t *delayp)
 {
 	snd_pcm_plugin_t *plugin = pcm->private_data;
 	snd_pcm_sframes_t sd;
-	int err = snd_pcm_delay(plugin->gen.slave, &sd);
+        snd_pcm_sframes_t slave_avail;
+        snd_pcm_sframes_t plugin_avail;
+        int err = snd_pcm_delay(plugin->gen.slave, &sd);
 	if (err < 0)
 		return err;
+        if (pcm->stream == SND_PCM_STREAM_CAPTURE &&
+	    pcm->access != SND_PCM_ACCESS_RW_INTERLEAVED &&
+	    pcm->access != SND_PCM_ACCESS_RW_NONINTERLEAVED)
+		goto _capture;
 	*delayp = sd;
 	return 0;
+ _capture:
+        slave_avail = snd_pcm_avail_update(plugin->gen.slave);
+        plugin_avail = snd_pcm_avail_update(pcm);
+        assert( plugin_avail >= slave_avail);
+        *delayp = sd + (plugin_avail - slave_avail);
+        return 0;
 }
 
 static int snd_pcm_plugin_prepare(snd_pcm_t *pcm)
-- 
1.7.2.3


[-- Attachment #4: Type: text/plain, Size: 160 bytes --]

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: bug in alsa-lib/snd_pcm_plugin_delay for capture?
  2010-11-22 13:35   ` pl bossart
@ 2010-11-22 14:01     ` Jaroslav Kysela
  2010-11-22 16:27       ` pl bossart
  2010-11-27  7:23       ` Raymond Yau
  0 siblings, 2 replies; 10+ messages in thread
From: Jaroslav Kysela @ 2010-11-22 14:01 UTC (permalink / raw)
  To: pl bossart; +Cc: alsa-devel, wim.taymans

On Mon, 22 Nov 2010, pl bossart wrote:

>>> Since the PulseAudio commit 29acfd0e0413a9bd126782763ee2dcf10357546 I
>>> am seeing errors messages telling me that the information reported by
>>> snd_pcm_avail_delay is not accurate on the capture path, more
>>> specifically the delay is lower than the available samples.
>>
>> Could you test patch bellow?
>>
>> http://git.alsa-project.org/?p=alsa-lib.git;a=commitdiff;h=ba9332e9192814a5431a3a2505d25d74a9232124
>
> Nah, no luck. PulseAudio still reports an error and if I add
> assert(*delayp >= *availp) the code bombs out.
> Here is one way to solve the problem. I don't claim it's the right
> one, but it's compatible with the way the availp value is computed,
> and it makes the error message go away in PulseAudio.

The double avail_update() calling seems too expensive to me. Could you 
test to use snd_pcm_mmap_capture_avail() instead snd_pcm_avail_update() 
calls (to assign both slave_avail and plugin_avail local variables)?

Also, remove the assert. I think that it may be raised when the stream is 
in some xrun state (with xrun checks disabled). The avail_update can 
return the negative values in this specific configuration.

> While I was at it I also removed the .client_frames and .slave_frames
> methods that no one seems to use. This seems to be dead code that
> hasn't been touched in years.

Good catch. This code was used with the really old rate plugin (this 
plugin was recoded completely). I applied this patch.

 					Jaroslav

-----
Jaroslav Kysela <perex@perex.cz>
Linux Kernel Sound Maintainer
ALSA Project, Red Hat, Inc.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: bug in alsa-lib/snd_pcm_plugin_delay for capture?
  2010-11-22 14:01     ` Jaroslav Kysela
@ 2010-11-22 16:27       ` pl bossart
  2010-11-23 10:25         ` Jaroslav Kysela
  2010-11-27  7:23       ` Raymond Yau
  1 sibling, 1 reply; 10+ messages in thread
From: pl bossart @ 2010-11-22 16:27 UTC (permalink / raw)
  To: Jaroslav Kysela; +Cc: alsa-devel, wim.taymans

>>> Could you test patch bellow?
>>>
>>>
>>> http://git.alsa-project.org/?p=alsa-lib.git;a=commitdiff;h=ba9332e9192814a5431a3a2505d25d74a9232124
>>
>> Nah, no luck. PulseAudio still reports an error and if I add
>> assert(*delayp >= *availp) the code bombs out.
>> Here is one way to solve the problem. I don't claim it's the right
>> one, but it's compatible with the way the availp value is computed,
>> and it makes the error message go away in PulseAudio.
>
> The double avail_update() calling seems too expensive to me. Could you test
> to use snd_pcm_mmap_capture_avail() instead snd_pcm_avail_update() calls (to
> assign both slave_avail and plugin_avail local variables)?

Nope, it still does not work if you use mmap_capture_avail. The root
cause of the problem is the while loop in snd_pcm_plugin_avail_update
(copied for reference below), it's called only in the capture case and
the value returned by the xfer variable causes the delay to be
eventually lower than the avail samples.
My fix is indeed expensive but it makes sure the same loop is called
twice, so that these two delay and avail values are consistent. I
don't know how to fix it in a different manner since I don't
understand this while loop in the first place. Maybe we need to
refactor this loop so that it's called in the delay() routine as well?
-Pierre

while (size > 0 && slave_size > 0) {
			snd_pcm_uframes_t frames = size;
			snd_pcm_uframes_t cont = pcm->buffer_size - hw_offset;
			const snd_pcm_channel_area_t *slave_areas;
			snd_pcm_uframes_t slave_offset;
			snd_pcm_uframes_t slave_frames = ULONG_MAX;
			snd_pcm_sframes_t result;
			int err;

			err = snd_pcm_mmap_begin(slave, &slave_areas, &slave_offset, &slave_frames);
			if (err < 0)
				return xfer > 0 ? (snd_pcm_sframes_t)xfer : err;
			if (frames > cont)
				frames = cont;
			frames = (plugin->read)(pcm, areas, hw_offset, frames,
					      slave_areas, slave_offset, &slave_frames);
			snd_atomic_write_begin(&plugin->watom);
			snd_pcm_mmap_hw_forward(pcm, frames);
			result = snd_pcm_mmap_commit(slave, slave_offset, slave_frames);
			snd_atomic_write_end(&plugin->watom);
			if (result > 0 && (snd_pcm_uframes_t)result != slave_frames) {
				snd_pcm_sframes_t res;
				
				res = plugin->undo_read(slave, areas, hw_offset, frames,
slave_frames - result);
				if (res < 0)
					return xfer > 0 ? (snd_pcm_sframes_t)xfer : res;
				frames -= res;
			}
			if (result <= 0)
				return xfer > 0 ? (snd_pcm_sframes_t)xfer : result;
			if (frames == cont)
				hw_offset = 0;
			else
				hw_offset += frames;
			size -= frames;
			slave_size -= slave_frames;
			xfer += frames;
		}
		return (snd_pcm_sframes_t)xfer;

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: bug in alsa-lib/snd_pcm_plugin_delay for capture?
  2010-11-22 16:27       ` pl bossart
@ 2010-11-23 10:25         ` Jaroslav Kysela
  2010-11-23 14:55           ` pl bossart
  0 siblings, 1 reply; 10+ messages in thread
From: Jaroslav Kysela @ 2010-11-23 10:25 UTC (permalink / raw)
  To: pl bossart; +Cc: alsa-devel, wim.taymans

On Mon, 22 Nov 2010, pl bossart wrote:

>>>> Could you test patch bellow?
>>>>
>>>>
>>>> http://git.alsa-project.org/?p=alsa-lib.git;a=commitdiff;h=ba9332e9192814a5431a3a2505d25d74a9232124
>>>
>>> Nah, no luck. PulseAudio still reports an error and if I add
>>> assert(*delayp >= *availp) the code bombs out.
>>> Here is one way to solve the problem. I don't claim it's the right
>>> one, but it's compatible with the way the availp value is computed,
>>> and it makes the error message go away in PulseAudio.
>>
>> The double avail_update() calling seems too expensive to me. Could you test
>> to use snd_pcm_mmap_capture_avail() instead snd_pcm_avail_update() calls (to
>> assign both slave_avail and plugin_avail local variables)?
>
> Nope, it still does not work if you use mmap_capture_avail. The root
> cause of the problem is the while loop in snd_pcm_plugin_avail_update
> (copied for reference below), it's called only in the capture case and
> the value returned by the xfer variable causes the delay to be
> eventually lower than the avail samples.

After rethinking, I believe that the right fix should be to add the 
'snd_pcm_mmap_capture_avail(pcm)' to the sd variable - delay(slave) - for 
the capture direction. You should not bother with avail_update or the 
slave avail values. The avail_update() capture loop just copied samples 
from the producer (slave) to the consumer (pcm). It means that the avail 
becomes lower for the producer (slave) and is increased for the consumer 
(pcm). The delay(slave) already contains the avail(slave) value.

So, the result should be 'delay(slave) + mmap_capture_avail(pcm)'.

 						Jaroslav

-----
Jaroslav Kysela <perex@perex.cz>
Linux Kernel Sound Maintainer
ALSA Project, Red Hat, Inc.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: bug in alsa-lib/snd_pcm_plugin_delay for capture?
  2010-11-23 10:25         ` Jaroslav Kysela
@ 2010-11-23 14:55           ` pl bossart
  2010-11-23 15:02             ` Jaroslav Kysela
  2010-11-25 18:20             ` Wim Taymans
  0 siblings, 2 replies; 10+ messages in thread
From: pl bossart @ 2010-11-23 14:55 UTC (permalink / raw)
  To: Jaroslav Kysela; +Cc: alsa-devel, wim.taymans

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

> After rethinking, I believe that the right fix should be to add the
> 'snd_pcm_mmap_capture_avail(pcm)' to the sd variable - delay(slave) - for
> the capture direction. You should not bother with avail_update or the slave
> avail values. The avail_update() capture loop just copied samples from the
> producer (slave) to the consumer (pcm). It means that the avail becomes
> lower for the producer (slave) and is increased for the consumer (pcm). The
> delay(slave) already contains the avail(slave) value.
>
> So, the result should be 'delay(slave) + mmap_capture_avail(pcm)'.

The attached patch implements your recoomendation and makes error
messages go away in PulseAudio.
It'd good to know if it improves the perfomance of echo cancellation.
Wim, can you try it out?
Thanks,
-Pierre

[-- Attachment #2: 0001-pcm_plugin-fix-delay.patch --]
[-- Type: application/octet-stream, Size: 1217 bytes --]

From 6cb058ed3503f1c61a87fc723787cf992c2b6650 Mon Sep 17 00:00:00 2001
From: Pierre-Louis Bossart <pierre-louis.bossart@intel.com>
Date: Tue, 23 Nov 2010 08:47:08 -0600
Subject: [PATCH] pcm_plugin: fix delay

PulseAudio ALSA modules report errors after calling
snd_pcm_avail_delay(), with a delay lower than the number of samples
available.

Correct delay using Jaroslav's recommendation:
"the result should be 'delay(slave) + mmap_capture_avail(pcm)"

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@intel.com>
---
 src/pcm/pcm_plugin.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/src/pcm/pcm_plugin.c b/src/pcm/pcm_plugin.c
index 19d0dee..d88e117 100644
--- a/src/pcm/pcm_plugin.c
+++ b/src/pcm/pcm_plugin.c
@@ -144,6 +144,12 @@ static int snd_pcm_plugin_delay(snd_pcm_t *pcm, snd_pcm_sframes_t *delayp)
 	int err = snd_pcm_delay(plugin->gen.slave, &sd);
 	if (err < 0)
 		return err;
+        if (pcm->stream == SND_PCM_STREAM_CAPTURE &&
+	    pcm->access != SND_PCM_ACCESS_RW_INTERLEAVED &&
+	    pcm->access != SND_PCM_ACCESS_RW_NONINTERLEAVED) {
+                sd += snd_pcm_mmap_capture_avail(pcm);
+        }        
+
 	*delayp = sd;
 	return 0;
 }
-- 
1.7.2.3


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

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: bug in alsa-lib/snd_pcm_plugin_delay for capture?
  2010-11-23 14:55           ` pl bossart
@ 2010-11-23 15:02             ` Jaroslav Kysela
  2010-11-25 18:20             ` Wim Taymans
  1 sibling, 0 replies; 10+ messages in thread
From: Jaroslav Kysela @ 2010-11-23 15:02 UTC (permalink / raw)
  To: pl bossart; +Cc: alsa-devel, wim.taymans

On Tue, 23 Nov 2010, pl bossart wrote:

>> After rethinking, I believe that the right fix should be to add the
>> 'snd_pcm_mmap_capture_avail(pcm)' to the sd variable - delay(slave) - for
>> the capture direction. You should not bother with avail_update or the slave
>> avail values. The avail_update() capture loop just copied samples from the
>> producer (slave) to the consumer (pcm). It means that the avail becomes
>> lower for the producer (slave) and is increased for the consumer (pcm). The
>> delay(slave) already contains the avail(slave) value.
>>
>> So, the result should be 'delay(slave) + mmap_capture_avail(pcm)'.
>
> The attached patch implements your recoomendation and makes error
> messages go away in PulseAudio.

I applied your patch to the alsa-lib repo. Thank you for discovering and 
testing.

 					Jaroslav

-----
Jaroslav Kysela <perex@perex.cz>
Linux Kernel Sound Maintainer
ALSA Project, Red Hat, Inc.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: bug in alsa-lib/snd_pcm_plugin_delay for capture?
  2010-11-23 14:55           ` pl bossart
  2010-11-23 15:02             ` Jaroslav Kysela
@ 2010-11-25 18:20             ` Wim Taymans
  1 sibling, 0 replies; 10+ messages in thread
From: Wim Taymans @ 2010-11-25 18:20 UTC (permalink / raw)
  To: pl bossart; +Cc: alsa-devel

On Tue, 2010-11-23 at 08:55 -0600, pl bossart wrote:
> > After rethinking, I believe that the right fix should be to add the
> > 'snd_pcm_mmap_capture_avail(pcm)' to the sd variable - delay(slave) - for
> > the capture direction. You should not bother with avail_update or the slave
> > avail values. The avail_update() capture loop just copied samples from the
> > producer (slave) to the consumer (pcm). It means that the avail becomes
> > lower for the producer (slave) and is increased for the consumer (pcm). The
> > delay(slave) already contains the avail(slave) value.
> >
> > So, the result should be 'delay(slave) + mmap_capture_avail(pcm)'.
> 
> The attached patch implements your recoomendation and makes error
> messages go away in PulseAudio.
> It'd good to know if it improves the perfomance of echo cancellation.
> Wim, can you try it out?

I asked Arun to try it, will let you know if it actually improves things
enough to celebrate. The current next plan to improve AEC between
different devices is to first do further smoothing of the rate
estimation in pa_smoother. and then make the sink (or source) do
resampling to compensate (when we manage to get a stable resampling
factor).

Wim

> Thanks,
> -Pierre

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: bug in alsa-lib/snd_pcm_plugin_delay for capture?
  2010-11-22 14:01     ` Jaroslav Kysela
  2010-11-22 16:27       ` pl bossart
@ 2010-11-27  7:23       ` Raymond Yau
  1 sibling, 0 replies; 10+ messages in thread
From: Raymond Yau @ 2010-11-27  7:23 UTC (permalink / raw)
  To: ALSA Development Mailing List

2010/11/22 Jaroslav Kysela <perex@perex.cz>

> On Mon, 22 Nov 2010, pl bossart wrote:
>
> >>> Since the PulseAudio commit 29acfd0e0413a9bd126782763ee2dcf10357546 I
> >>> am seeing errors messages telling me that the information reported by
> >>> snd_pcm_avail_delay is not accurate on the capture path, more
> >>> specifically the delay is lower than the available samples.
> >>
> >> Could you test patch bellow?
> >>
> >>
> http://git.alsa-project.org/?p=alsa-lib.git;a=commitdiff;h=ba9332e9192814a5431a3a2505d25d74a9232124
> >
> > Nah, no luck. PulseAudio still reports an error and if I add
> > assert(*delayp >= *availp) the code bombs out.
> > Here is one way to solve the problem. I don't claim it's the right
> > one, but it's compatible with the way the availp value is computed,
> > and it makes the error message go away in PulseAudio.
>
> The double avail_update() calling seems too expensive to me. Could you
> test to use snd_pcm_mmap_capture_avail() instead snd_pcm_avail_update()
> calls (to assign both slave_avail and plugin_avail local variables)?
>
> Also, remove the assert. I think that it may be raised when the stream is
> in some xrun state (with xrun checks disabled). The avail_update can
> return the negative values in this specific configuration.
>
>
The point is the documentation does not mention that the state still remain
as SND_PCM_STATE_RUNNING even when the distance between the application ptr
and the hw_ptr is more than a buffer size , it only mention the device will
do the endless loop.

The only way to know overrun/underrun is to check the avail is larger than
the buffer size when the application set the stop threshold to boundary
instead of buffer size.

so it is incorrect for PA to say this is a driver bug since it disable the
automatic stop when underrun/overrun occur


e.g. dmix also set stop threshold to boundary but aplay can still get xrun
when using small buffer size with plug:dmix

if you set the boudnary to buffer size instead of boundary in
alsa-time-test.c , alsa-lib will tell you that the program is
underrun/overrun


http://www.alsa-project.org/alsa-doc/alsa-lib/group___p_c_m___s_w___params.html

PCM is automatically stopped in SND_PCM_STATE_XRUN state when available
frames is >= threshold. If the stop threshold is equal to boundary (also
software parameter - sw_param) then automatic stop will be disabled (thus
device will do the endless loop in the ring buffer).

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2010-11-27  7:23 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2010-11-16  7:08 bug in alsa-lib/snd_pcm_plugin_delay for capture? pl bossart
2010-11-18  8:28 ` Jaroslav Kysela
2010-11-22 13:35   ` pl bossart
2010-11-22 14:01     ` Jaroslav Kysela
2010-11-22 16:27       ` pl bossart
2010-11-23 10:25         ` Jaroslav Kysela
2010-11-23 14:55           ` pl bossart
2010-11-23 15:02             ` Jaroslav Kysela
2010-11-25 18:20             ` Wim Taymans
2010-11-27  7:23       ` Raymond Yau

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.