alsa-lib: Bug fixes to namehint, dsnoop and minor changes

alsa-lib: Bug fixes to namehint, dsnoop and minor changes

[Mark Hills]

I've been running these patches in some form for a while. Now tidied up 
for RFC.

It's quite possible I misinterpreted something; the behaviours seem to be 
quite long-standing, hence a first pass for comments.

This has taken me to the _avail() functions, which do not seem to be 
prepared for signed output, yet callers interpret as signed and ultimately 
end up in external API. Their handling of wraparound conditions etc. may 
not be as expected. I've already started to look at these.

-- 
Mark

[RFC PATCH 1/9] control: Fix typos in the namehint example

[Mark Hills]

Ths "namehint" is a list, and there doesn't seem to have been any
history where the separator would be a colon.
---
 src/control/namehint.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/src/control/namehint.c b/src/control/namehint.c
index ecd470f3..d81d3a7e 100644
--- a/src/control/namehint.c
+++ b/src/control/namehint.c
@@ -543,10 +543,10 @@ static int add_software_devices(snd_config_t *config, snd_config_t *rw_config,
  * User-defined hints are gathered from namehint.IFACE tree like:
  *
  * <code>
- * namehint.pcm {<br>
+ * namehint.pcm [<br>
  *   myfile "file:FILE=/tmp/soundwave.raw|Save sound output to /tmp/soundwave.raw"<br>
- *   myplug "plug:front:Do all conversions for front speakers"<br>
- * }
+ *   myplug "plug:front|Do all conversions for front speakers"<br>
+ * ]
  * </code>
  *
  * Note: The device description is separated with '|' char.
-- 
2.17.5

[RFC PATCH 2/9] control: Fix a bug that prevented namehint behaviour

[Mark Hills]

Looks like the documented behaviour was broken in commit 1ba513f9 in
2006, with the introduction of multiple fields.

I've chosen to match the described behaviour. Prior to this patch,
using namehint could be made to work by exploiting the lack of escaping
of the "name", populating the other fields:

  "plug:front|DESCDo all conversions for front speakers"

rather than that which is documented and presumed to be the intention
for asoundrc files:

  "plug:front|Do all conversions for front speakers"

Everything seems to strongly suggest nobody is using this feature; I can
find no working examples through a web search and probably someone
would have hit this bug. It's not documented in configuration, only in
the snd_device_name_hint() call. So it would probably clutter things to
provide compatibility for the old behaviour.

I have found it to be very useful since working in Chromium, where it is
the only way to expose chosen ALSA devices to web applications.

A temporary buffer is required to null-terminate the string.  I see no
use of alloca() in the code, presumably to avoid unbounded stack size.
So memory is allocated on the heap.
---
 src/control/namehint.c | 27 ++++++++++++++++++++++++++-
 1 file changed, 26 insertions(+), 1 deletion(-)

diff --git a/src/control/namehint.c b/src/control/namehint.c
index d81d3a7e..e4f696ad 100644
--- a/src/control/namehint.c
+++ b/src/control/namehint.c
@@ -78,6 +78,31 @@ static int hint_list_add(struct hint_list *list,
 	return 0;
 }
 
+/**
+ * Add a namehint from string given in a user configuration file
+ */
+static int hint_list_add_custom(struct hint_list *list,
+				const char *entry)
+{
+	int err;
+	const char *sep;
+	char *name;
+
+	assert(entry);
+
+	sep = strchr(entry, '|');
+	if (sep == NULL)
+		return hint_list_add(list, entry, NULL);
+
+	name = strndup(entry, sep - entry);
+	if (name == NULL)
+		return -ENOMEM;
+
+	err = hint_list_add(list, name, sep + 1);
+	free(name);
+	return err;
+}
+
 static void zero_handler(const char *file ATTRIBUTE_UNUSED,
 			 int line ATTRIBUTE_UNUSED,
 			 const char *function ATTRIBUTE_UNUSED,
@@ -626,7 +651,7 @@ int snd_device_name_hint(int card, const char *iface, void ***hints)
 			if (snd_config_get_string(snd_config_iterator_entry(i),
 						  &str) < 0)
 				continue;
-			err = hint_list_add(&list, str, NULL);
+			err = hint_list_add_custom(&list, str);
 			if (err < 0)
 				goto __error;
 		}
-- 
2.17.5

[RFC PATCH 3/9] conf: Read a host-specific asoundrc

[Mark Hills]

On systems with a network mounted home directory this is thoroughly
useful to allow for a core set of asoundrc settings, but with different
settings on different hosts.

It's not possibly to implement this in our own asoundrc or local
customisation, as it's too late. The installation file must be modified.

This is similar to ~/.Xdefaults-* on some systems.
---
 src/conf/alsa.conf | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/src/conf/alsa.conf b/src/conf/alsa.conf
index 08370108..39195f49 100644
--- a/src/conf/alsa.conf
+++ b/src/conf/alsa.conf
@@ -24,6 +24,17 @@
 					"/alsa/asoundrc"
 				]
 			}
+			{
+				@func concat
+				strings [
+					"~/.asoundrc-"
+					{
+						@func getenv
+						vars [ HOSTNAME ]
+						default "localhost"
+					}
+				]
+			}
 		]
 		errors false
 	}
-- 
2.17.5

[RFC PATCH 4/9] dsnoop: The delay presented to snd_pcm_status_delay() was incorrect

[Mark Hills]

This was the original bug that caused me to start looking at the
ring buffer functions.

In the API this is documented as:

  "Delay is distance between current application frame position and
   sound frame position. It's positive and less than buffer size in
   normal situation, negative on playback underrun and greater than
   buffer size on capture overrun. "

Because dsnoop was returning the buffer space available to the hardware
the return value was always quite large, and moved in the wrong
direction.

With this patch, dsnoop now gives results which are comparable to using
the "hw" device directly. My test case was with snd-echo3g (Layla3G).
---
 src/pcm/pcm_local.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/pcm/pcm_local.h b/src/pcm/pcm_local.h
index 89d4125b..1fa8e61d 100644
--- a/src/pcm/pcm_local.h
+++ b/src/pcm/pcm_local.h
@@ -589,7 +589,7 @@ static inline snd_pcm_uframes_t snd_pcm_mmap_playback_delay(snd_pcm_t *pcm)
 
 static inline snd_pcm_uframes_t snd_pcm_mmap_capture_delay(snd_pcm_t *pcm)
 {
-	return snd_pcm_mmap_capture_hw_avail(pcm);
+	return snd_pcm_mmap_capture_avail(pcm);
 }
 
 static inline snd_pcm_sframes_t snd_pcm_mmap_delay(snd_pcm_t *pcm)
-- 
2.17.5

[RFC PATCH 5/9] pcm: Annotate the _delay functions based on findings from the previous bug

[Mark Hills]

---
 src/pcm/pcm_local.h | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/src/pcm/pcm_local.h b/src/pcm/pcm_local.h
index 1fa8e61d..cf018fc0 100644
--- a/src/pcm/pcm_local.h
+++ b/src/pcm/pcm_local.h
@@ -582,11 +582,17 @@ static inline snd_pcm_uframes_t snd_pcm_mmap_hw_offset(snd_pcm_t *pcm)
 	return *pcm->hw.ptr % pcm->buffer_size;
 }
 
+/*
+ * \retval number of frames pending from application to hardware
+ */
 static inline snd_pcm_uframes_t snd_pcm_mmap_playback_delay(snd_pcm_t *pcm)
 {
 	return snd_pcm_mmap_playback_hw_avail(pcm);
 }
 
+/*
+ * \retval number of frames pending from hardware to application
+ */
 static inline snd_pcm_uframes_t snd_pcm_mmap_capture_delay(snd_pcm_t *pcm)
 {
 	return snd_pcm_mmap_capture_avail(pcm);
-- 
2.17.5

[RFC PATCH 6/9] dsnoop: The stop threshold was not implemented correctly

[Mark Hills]

The previous implementation would mean that stop_threshold behaved
erratically. The intent is to detect that the buffer is too full,
and stop.

In practice, I don't think this was a bug in practice for applications
which don't adjust the stop_threshold. The line above catches those cases.
---
 src/pcm/pcm_dsnoop.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/pcm/pcm_dsnoop.c b/src/pcm/pcm_dsnoop.c
index c64df381..790d944c 100644
--- a/src/pcm/pcm_dsnoop.c
+++ b/src/pcm/pcm_dsnoop.c
@@ -165,7 +165,7 @@ static int snd_pcm_dsnoop_sync_ptr(snd_pcm_t *pcm)
 	// printf("sync ptr diff = %li\n", diff);
 	if (pcm->stop_threshold >= pcm->boundary)	/* don't care */
 		return 0;
-	if ((avail = snd_pcm_mmap_capture_hw_avail(pcm)) >= pcm->stop_threshold) {
+	if ((avail = snd_pcm_mmap_capture_avail(pcm)) >= pcm->stop_threshold) {
 		gettimestamp(&dsnoop->trigger_tstamp, pcm->tstamp_type);
 		dsnoop->state = SND_PCM_STATE_XRUN;
 		dsnoop->avail_max = avail;
-- 
2.17.5

[RFC PATCH 7/9] dsnoop: Another bug where the empty, not full, part of the ringbuffer was observed

[Mark Hills]

This looks like a simple mistake dating back to 2003 (commit 7470a5b9)
where code originated from dmix.
---
 src/pcm/pcm_dsnoop.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/pcm/pcm_dsnoop.c b/src/pcm/pcm_dsnoop.c
index 790d944c..3588eb91 100644
--- a/src/pcm/pcm_dsnoop.c
+++ b/src/pcm/pcm_dsnoop.c
@@ -241,7 +241,7 @@ static int snd_pcm_dsnoop_delay(snd_pcm_t *pcm, snd_pcm_sframes_t *delayp)
 		/* Fall through */
 	case SNDRV_PCM_STATE_PREPARED:
 	case SNDRV_PCM_STATE_SUSPENDED:
-		*delayp = snd_pcm_mmap_capture_hw_avail(pcm);
+		*delayp = snd_pcm_mmap_capture_avail(pcm);
 		return 0;
 	case SNDRV_PCM_STATE_XRUN:
 		return -EPIPE;
-- 
2.17.5

[RFC PATCH 8/9] dsnoop: Make use of the (previously unused) function

[Mark Hills]

Match the equivalent funciton for playback. This is on the assumption
that values should be capped at zero, which is what _rewindable()
implements.
---
 src/pcm/pcm_dsnoop.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/pcm/pcm_dsnoop.c b/src/pcm/pcm_dsnoop.c
index 3588eb91..7904314c 100644
--- a/src/pcm/pcm_dsnoop.c
+++ b/src/pcm/pcm_dsnoop.c
@@ -352,7 +352,7 @@ static int snd_pcm_dsnoop_pause(snd_pcm_t *pcm ATTRIBUTE_UNUSED, int enable ATTR
 
 static snd_pcm_sframes_t snd_pcm_dsnoop_rewindable(snd_pcm_t *pcm)
 {
-	return snd_pcm_mmap_capture_hw_avail(pcm);
+	return snd_pcm_mmap_capture_hw_rewindable(pcm);
 }
 
 static snd_pcm_sframes_t snd_pcm_dsnoop_rewind(snd_pcm_t *pcm, snd_pcm_uframes_t frames)
-- 
2.17.5

[RFC PATCH 9/9] pcm: Annotate the _avail functions

[Mark Hills]

I took time to understand these functions in the context of the
rest of the code, which would have been a lot quicker with a comment
like this.
---
 src/pcm/pcm_local.h | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/src/pcm/pcm_local.h b/src/pcm/pcm_local.h
index cf018fc0..aae58ed3 100644
--- a/src/pcm/pcm_local.h
+++ b/src/pcm/pcm_local.h
@@ -480,6 +480,13 @@ static inline int snd_pcm_check_error(snd_pcm_t *pcm, int err)
 	return err;
 }
 
+/**
+ * \retval number of frames available to the application for playback
+ *
+ * This is how far ahead the hardware position in the ring buffer is,
+ * compared to the application position. ie. for playback it's the
+ * number of frames in the empty part of the ring buffer.
+ */
 static inline snd_pcm_uframes_t __snd_pcm_playback_avail(snd_pcm_t *pcm,
 							 const snd_pcm_uframes_t hw_ptr,
 							 const snd_pcm_uframes_t appl_ptr)
@@ -498,6 +505,13 @@ static inline snd_pcm_uframes_t snd_pcm_mmap_playback_avail(snd_pcm_t *pcm)
 	return __snd_pcm_playback_avail(pcm, *pcm->hw.ptr, *pcm->appl.ptr);
 }
 
+/*
+ * \retval number of frames available to the application for capture
+ *
+ * This is how far ahead the hardware position in the ring buffer is
+ * compared to the application position.  ie. for capture, it's the
+ * number of frames in the filled part of the ring buffer.
+ */
 static inline snd_pcm_uframes_t __snd_pcm_capture_avail(snd_pcm_t *pcm,
 							const snd_pcm_uframes_t hw_ptr,
 							const snd_pcm_uframes_t appl_ptr)
@@ -529,11 +543,21 @@ static inline snd_pcm_uframes_t snd_pcm_mmap_avail(snd_pcm_t *pcm)
 	return __snd_pcm_avail(pcm, *pcm->hw.ptr, *pcm->appl.ptr);
 }
 
+/*
+ * \retval number of frames available to the hardware for playback
+ *
+ * ie. the filled part of the ring buffer
+ */
 static inline snd_pcm_sframes_t snd_pcm_mmap_playback_hw_avail(snd_pcm_t *pcm)
 {
 	return pcm->buffer_size - snd_pcm_mmap_playback_avail(pcm);
 }
 
+/*
+ * \retval number of frames available to the hardware for capture
+ *
+ * ie. the empty part of the ring buffer.
+ */
 static inline snd_pcm_sframes_t snd_pcm_mmap_capture_hw_avail(snd_pcm_t *pcm)
 {
 	return pcm->buffer_size - snd_pcm_mmap_capture_avail(pcm);
-- 
2.17.5

Re: alsa-lib: Bug fixes to namehint, dsnoop and minor changes

[Mark Hills]

On Fri, 12 Jun 2020, Mark Hills wrote:

> I've been running these patches in some form for a while. Now tidied up 
> for RFC.
> 
> It's quite possible I misinterpreted something; the behaviours seem to 
> be quite long-standing, hence a first pass for comments.
[...]

Jaroslav, I sent these patches over for RFC. No feedback came, so I'll 
follow this email with them as PATCH with sign-off.

The only changes are a tiny rewording to commit messages.

Please let me know of anything that prevents their inclusion and I'll work 
on it.

-- 
Mark

[PATCH 1/9] control: Fix typos in the namehint example

[Mark Hills]

Ths "namehint" is a list, and there doesn't seem to have been any
history where the separator would be a colon.

Signed-off-by: Mark Hills <mark@xwax.org>
---
 src/control/namehint.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/src/control/namehint.c b/src/control/namehint.c
index ecd470f3..d81d3a7e 100644
--- a/src/control/namehint.c
+++ b/src/control/namehint.c
@@ -543,10 +543,10 @@ static int add_software_devices(snd_config_t *config, snd_config_t *rw_config,
  * User-defined hints are gathered from namehint.IFACE tree like:
  *
  * <code>
- * namehint.pcm {<br>
+ * namehint.pcm [<br>
  *   myfile "file:FILE=/tmp/soundwave.raw|Save sound output to /tmp/soundwave.raw"<br>
- *   myplug "plug:front:Do all conversions for front speakers"<br>
- * }
+ *   myplug "plug:front|Do all conversions for front speakers"<br>
+ * ]
  * </code>
  *
  * Note: The device description is separated with '|' char.
-- 
2.17.5

[PATCH 2/9] control: Fix a bug that prevented namehint behaviour

[Mark Hills]

Looks like the documented behaviour was broken in commit 1ba513f9 in
2006, with the introduction of multiple fields.

I've chosen to match the described behaviour. Prior to this patch,
using namehint could be made to work by exploiting the lack of escaping
of the "name", populating the other fields:

  "plug:front|DESCDo all conversions for front speakers"

rather than that which is documented and presumed to be the intention
for asoundrc files:

  "plug:front|Do all conversions for front speakers"

Everything seems to strongly suggest nobody is using this feature; I can
find no working examples through a web search and probably someone
would have hit this bug. It's not documented in configuration, only in
the snd_device_name_hint() call. So it would probably clutter things to
provide compatibility for the old behaviour.

I have found it to be very useful since working in Chromium, where it is
the only way to expose chosen ALSA devices to web applications.

A temporary buffer is required to null-terminate the string.  I see no
use of alloca() in the code, presumably to avoid unbounded stack size.
So memory is allocated on the heap.

Signed-off-by: Mark Hills <mark@xwax.org>
---
 src/control/namehint.c | 27 ++++++++++++++++++++++++++-
 1 file changed, 26 insertions(+), 1 deletion(-)

diff --git a/src/control/namehint.c b/src/control/namehint.c
index d81d3a7e..e4f696ad 100644
--- a/src/control/namehint.c
+++ b/src/control/namehint.c
@@ -78,6 +78,31 @@ static int hint_list_add(struct hint_list *list,
 	return 0;
 }
 
+/**
+ * Add a namehint from string given in a user configuration file
+ */
+static int hint_list_add_custom(struct hint_list *list,
+				const char *entry)
+{
+	int err;
+	const char *sep;
+	char *name;
+
+	assert(entry);
+
+	sep = strchr(entry, '|');
+	if (sep == NULL)
+		return hint_list_add(list, entry, NULL);
+
+	name = strndup(entry, sep - entry);
+	if (name == NULL)
+		return -ENOMEM;
+
+	err = hint_list_add(list, name, sep + 1);
+	free(name);
+	return err;
+}
+
 static void zero_handler(const char *file ATTRIBUTE_UNUSED,
 			 int line ATTRIBUTE_UNUSED,
 			 const char *function ATTRIBUTE_UNUSED,
@@ -626,7 +651,7 @@ int snd_device_name_hint(int card, const char *iface, void ***hints)
 			if (snd_config_get_string(snd_config_iterator_entry(i),
 						  &str) < 0)
 				continue;
-			err = hint_list_add(&list, str, NULL);
+			err = hint_list_add_custom(&list, str);
 			if (err < 0)
 				goto __error;
 		}
-- 
2.17.5

[PATCH 3/9] conf: Read a host-specific asoundrc

[Mark Hills]

On systems with a network mounted home directory this is thoroughly
useful to allow for a core set of asoundrc settings, but with different
settings on different hosts.

It's not possibly to implement this in our own asoundrc or local
customisation, as it's too late. The installation file must be modified.

This is similar to ~/.Xdefaults-* on some systems.

Signed-off-by: Mark Hills <mark@xwax.org>
---
 src/conf/alsa.conf | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/src/conf/alsa.conf b/src/conf/alsa.conf
index 18427ec6..4dae0e9c 100644
--- a/src/conf/alsa.conf
+++ b/src/conf/alsa.conf
@@ -24,6 +24,17 @@
 					"/alsa/asoundrc"
 				]
 			}
+			{
+				@func concat
+				strings [
+					"~/.asoundrc-"
+					{
+						@func getenv
+						vars [ HOSTNAME ]
+						default "localhost"
+					}
+				]
+			}
 		]
 		errors false
 	}
-- 
2.17.5

[PATCH 4/9] dsnoop: The delay presented to snd_pcm_status_delay() was incorrect

[Mark Hills]

This was the original bug that caused me to start looking at the
ring buffer functions.

In the API this is documented as:

  "Delay is distance between current application frame position and
   sound frame position. It's positive and less than buffer size in
   normal situation, negative on playback underrun and greater than
   buffer size on capture overrun. "

Because dsnoop was returning the buffer space available to the hardware
the return value was always quite large, and moved in the wrong
direction.

With this patch, dsnoop now gives results which are comparable to using
the "hw" device directly. My test case was with snd-echo3g (Layla3G).

Signed-off-by: Mark Hills <mark@xwax.org>
---
 src/pcm/pcm_local.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/pcm/pcm_local.h b/src/pcm/pcm_local.h
index 89d4125b..1fa8e61d 100644
--- a/src/pcm/pcm_local.h
+++ b/src/pcm/pcm_local.h
@@ -589,7 +589,7 @@ static inline snd_pcm_uframes_t snd_pcm_mmap_playback_delay(snd_pcm_t *pcm)
 
 static inline snd_pcm_uframes_t snd_pcm_mmap_capture_delay(snd_pcm_t *pcm)
 {
-	return snd_pcm_mmap_capture_hw_avail(pcm);
+	return snd_pcm_mmap_capture_avail(pcm);
 }
 
 static inline snd_pcm_sframes_t snd_pcm_mmap_delay(snd_pcm_t *pcm)
-- 
2.17.5

[PATCH 5/9] pcm: Annotate the _delay functions based on findings from the previous bug

[Mark Hills]

Signed-off-by: Mark Hills <mark@xwax.org>
---
 src/pcm/pcm_local.h | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/src/pcm/pcm_local.h b/src/pcm/pcm_local.h
index 1fa8e61d..cf018fc0 100644
--- a/src/pcm/pcm_local.h
+++ b/src/pcm/pcm_local.h
@@ -582,11 +582,17 @@ static inline snd_pcm_uframes_t snd_pcm_mmap_hw_offset(snd_pcm_t *pcm)
 	return *pcm->hw.ptr % pcm->buffer_size;
 }
 
+/*
+ * \retval number of frames pending from application to hardware
+ */
 static inline snd_pcm_uframes_t snd_pcm_mmap_playback_delay(snd_pcm_t *pcm)
 {
 	return snd_pcm_mmap_playback_hw_avail(pcm);
 }
 
+/*
+ * \retval number of frames pending from hardware to application
+ */
 static inline snd_pcm_uframes_t snd_pcm_mmap_capture_delay(snd_pcm_t *pcm)
 {
 	return snd_pcm_mmap_capture_avail(pcm);
-- 
2.17.5

[PATCH 6/9] dsnoop: The stop threshold was not implemented correctly

[Mark Hills]

The previous implementation would mean that stop_threshold behaved
erratically. The intent is to detect that the buffer is too full,
and stop.

In practice, I don't think this was a bug in practice for applications
which don't adjust the stop_threshold. The line above catches those cases.

Signed-off-by: Mark Hills <mark@xwax.org>
---
 src/pcm/pcm_dsnoop.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/pcm/pcm_dsnoop.c b/src/pcm/pcm_dsnoop.c
index c64df381..790d944c 100644
--- a/src/pcm/pcm_dsnoop.c
+++ b/src/pcm/pcm_dsnoop.c
@@ -165,7 +165,7 @@ static int snd_pcm_dsnoop_sync_ptr(snd_pcm_t *pcm)
 	// printf("sync ptr diff = %li\n", diff);
 	if (pcm->stop_threshold >= pcm->boundary)	/* don't care */
 		return 0;
-	if ((avail = snd_pcm_mmap_capture_hw_avail(pcm)) >= pcm->stop_threshold) {
+	if ((avail = snd_pcm_mmap_capture_avail(pcm)) >= pcm->stop_threshold) {
 		gettimestamp(&dsnoop->trigger_tstamp, pcm->tstamp_type);
 		dsnoop->state = SND_PCM_STATE_XRUN;
 		dsnoop->avail_max = avail;
-- 
2.17.5

[PATCH 7/9] dsnoop: Another bug where the empty, not full, part of the ringbuffer was observed

[Mark Hills]

This looks like a simple mistake dating back to 2003 (commit 7470a5b9)
where code originated from dmix.

Signed-off-by: Mark Hills <mark@xwax.org>
---
 src/pcm/pcm_dsnoop.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/pcm/pcm_dsnoop.c b/src/pcm/pcm_dsnoop.c
index 790d944c..3588eb91 100644
--- a/src/pcm/pcm_dsnoop.c
+++ b/src/pcm/pcm_dsnoop.c
@@ -241,7 +241,7 @@ static int snd_pcm_dsnoop_delay(snd_pcm_t *pcm, snd_pcm_sframes_t *delayp)
 		/* Fall through */
 	case SNDRV_PCM_STATE_PREPARED:
 	case SNDRV_PCM_STATE_SUSPENDED:
-		*delayp = snd_pcm_mmap_capture_hw_avail(pcm);
+		*delayp = snd_pcm_mmap_capture_avail(pcm);
 		return 0;
 	case SNDRV_PCM_STATE_XRUN:
 		return -EPIPE;
-- 
2.17.5

[PATCH 8/9] dsnoop: Make use of the (previously unused) function

[Mark Hills]

Match the equivalent funciton for playback. This is on the assumption
that values should be capped at zero, which is what _rewindable()
implements.

Signed-off-by: Mark Hills <mark@xwax.org>
---
 src/pcm/pcm_dsnoop.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/pcm/pcm_dsnoop.c b/src/pcm/pcm_dsnoop.c
index 3588eb91..7904314c 100644
--- a/src/pcm/pcm_dsnoop.c
+++ b/src/pcm/pcm_dsnoop.c
@@ -352,7 +352,7 @@ static int snd_pcm_dsnoop_pause(snd_pcm_t *pcm ATTRIBUTE_UNUSED, int enable ATTR
 
 static snd_pcm_sframes_t snd_pcm_dsnoop_rewindable(snd_pcm_t *pcm)
 {
-	return snd_pcm_mmap_capture_hw_avail(pcm);
+	return snd_pcm_mmap_capture_hw_rewindable(pcm);
 }
 
 static snd_pcm_sframes_t snd_pcm_dsnoop_rewind(snd_pcm_t *pcm, snd_pcm_uframes_t frames)
-- 
2.17.5

[PATCH 9/9] pcm: Annotate the _avail functions

[Mark Hills]

I took time to understand these functions in the context of the
rest of the code, which would have been a lot quicker with a comment
like this.

Signed-off-by: Mark Hills <mark@xwax.org>
---
 src/pcm/pcm_local.h | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/src/pcm/pcm_local.h b/src/pcm/pcm_local.h
index cf018fc0..aae58ed3 100644
--- a/src/pcm/pcm_local.h
+++ b/src/pcm/pcm_local.h
@@ -480,6 +480,13 @@ static inline int snd_pcm_check_error(snd_pcm_t *pcm, int err)
 	return err;
 }
 
+/**
+ * \retval number of frames available to the application for playback
+ *
+ * This is how far ahead the hardware position in the ring buffer is,
+ * compared to the application position. ie. for playback it's the
+ * number of frames in the empty part of the ring buffer.
+ */
 static inline snd_pcm_uframes_t __snd_pcm_playback_avail(snd_pcm_t *pcm,
 							 const snd_pcm_uframes_t hw_ptr,
 							 const snd_pcm_uframes_t appl_ptr)
@@ -498,6 +505,13 @@ static inline snd_pcm_uframes_t snd_pcm_mmap_playback_avail(snd_pcm_t *pcm)
 	return __snd_pcm_playback_avail(pcm, *pcm->hw.ptr, *pcm->appl.ptr);
 }
 
+/*
+ * \retval number of frames available to the application for capture
+ *
+ * This is how far ahead the hardware position in the ring buffer is
+ * compared to the application position.  ie. for capture, it's the
+ * number of frames in the filled part of the ring buffer.
+ */
 static inline snd_pcm_uframes_t __snd_pcm_capture_avail(snd_pcm_t *pcm,
 							const snd_pcm_uframes_t hw_ptr,
 							const snd_pcm_uframes_t appl_ptr)
@@ -529,11 +543,21 @@ static inline snd_pcm_uframes_t snd_pcm_mmap_avail(snd_pcm_t *pcm)
 	return __snd_pcm_avail(pcm, *pcm->hw.ptr, *pcm->appl.ptr);
 }
 
+/*
+ * \retval number of frames available to the hardware for playback
+ *
+ * ie. the filled part of the ring buffer
+ */
 static inline snd_pcm_sframes_t snd_pcm_mmap_playback_hw_avail(snd_pcm_t *pcm)
 {
 	return pcm->buffer_size - snd_pcm_mmap_playback_avail(pcm);
 }
 
+/*
+ * \retval number of frames available to the hardware for capture
+ *
+ * ie. the empty part of the ring buffer.
+ */
 static inline snd_pcm_sframes_t snd_pcm_mmap_capture_hw_avail(snd_pcm_t *pcm)
 {
 	return pcm->buffer_size - snd_pcm_mmap_capture_avail(pcm);
-- 
2.17.5

Re: [PATCH 3/9] conf: Read a host-specific asoundrc

[Takashi Iwai]

On Mon, 22 Jun 2020 15:15:09 +0200,
Mark Hills wrote:
> 
> On systems with a network mounted home directory this is thoroughly
> useful to allow for a core set of asoundrc settings, but with different
> settings on different hosts.
> 
> It's not possibly to implement this in our own asoundrc or local
> customisation, as it's too late. The installation file must be modified.
> 
> This is similar to ~/.Xdefaults-* on some systems.
> 
> Signed-off-by: Mark Hills <mark@xwax.org>

This kind of change popping up sometimes in the past, too, and I have
a mixed feeling whether to take such a change globally or not.

In one side, it can work, but OTOH, if you can deal with that detail,
you're certainly able to set up the environment variable easily, too.


thanks,

Takashi

> ---
>  src/conf/alsa.conf | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/src/conf/alsa.conf b/src/conf/alsa.conf
> index 18427ec6..4dae0e9c 100644
> --- a/src/conf/alsa.conf
> +++ b/src/conf/alsa.conf
> @@ -24,6 +24,17 @@
>  					"/alsa/asoundrc"
>  				]
>  			}
> +			{
> +				@func concat
> +				strings [
> +					"~/.asoundrc-"
> +					{
> +						@func getenv
> +						vars [ HOSTNAME ]
> +						default "localhost"
> +					}
> +				]
> +			}
>  		]
>  		errors false
>  	}
> -- 
> 2.17.5
> 

Re: [PATCH 1/9] control: Fix typos in the namehint example

[Takashi Iwai]

On Mon, 22 Jun 2020 15:15:07 +0200,
Mark Hills wrote:
> 
> Ths "namehint" is a list, and there doesn't seem to have been any
> history where the separator would be a colon.
> 
> Signed-off-by: Mark Hills <mark@xwax.org>

Thanks, applied.


Takashi

Re: [PATCH 2/9] control: Fix a bug that prevented namehint behaviour

[Takashi Iwai]

On Mon, 22 Jun 2020 15:15:08 +0200,
Mark Hills wrote:
> 
> Looks like the documented behaviour was broken in commit 1ba513f9 in
> 2006, with the introduction of multiple fields.
> 
> I've chosen to match the described behaviour. Prior to this patch,
> using namehint could be made to work by exploiting the lack of escaping
> of the "name", populating the other fields:
> 
>   "plug:front|DESCDo all conversions for front speakers"
> 
> rather than that which is documented and presumed to be the intention
> for asoundrc files:
> 
>   "plug:front|Do all conversions for front speakers"
> 
> Everything seems to strongly suggest nobody is using this feature; I can
> find no working examples through a web search and probably someone
> would have hit this bug. It's not documented in configuration, only in
> the snd_device_name_hint() call. So it would probably clutter things to
> provide compatibility for the old behaviour.
> 
> I have found it to be very useful since working in Chromium, where it is
> the only way to expose chosen ALSA devices to web applications.
> 
> A temporary buffer is required to null-terminate the string.  I see no
> use of alloca() in the code, presumably to avoid unbounded stack size.
> So memory is allocated on the heap.
> 
> Signed-off-by: Mark Hills <mark@xwax.org>

Thanks, applied.


Takashi

Re: [PATCH 4/9] dsnoop: The delay presented to snd_pcm_status_delay() was incorrect

[Takashi Iwai]

On Mon, 22 Jun 2020 15:15:10 +0200,
Mark Hills wrote:
> 
> This was the original bug that caused me to start looking at the
> ring buffer functions.
> 
> In the API this is documented as:
> 
>   "Delay is distance between current application frame position and
>    sound frame position. It's positive and less than buffer size in
>    normal situation, negative on playback underrun and greater than
>    buffer size on capture overrun. "
> 
> Because dsnoop was returning the buffer space available to the hardware
> the return value was always quite large, and moved in the wrong
> direction.
> 
> With this patch, dsnoop now gives results which are comparable to using
> the "hw" device directly. My test case was with snd-echo3g (Layla3G).
> 
> Signed-off-by: Mark Hills <mark@xwax.org>

Thanks, applied.


Takashi

Re: [PATCH 5/9] pcm: Annotate the _delay functions based on findings from the previous bug

[Takashi Iwai]

On Mon, 22 Jun 2020 15:15:11 +0200,
Mark Hills wrote:
> 
> Signed-off-by: Mark Hills <mark@xwax.org>

Thanks, applied.


Takashi

Re: [PATCH 6/9] dsnoop: The stop threshold was not implemented correctly

[Takashi Iwai]

On Mon, 22 Jun 2020 15:15:12 +0200,
Mark Hills wrote:
> 
> The previous implementation would mean that stop_threshold behaved
> erratically. The intent is to detect that the buffer is too full,
> and stop.
> 
> In practice, I don't think this was a bug in practice for applications
> which don't adjust the stop_threshold. The line above catches those cases.
> 
> Signed-off-by: Mark Hills <mark@xwax.org>

Thanks, applied.


Takashi

Re: [PATCH 7/9] dsnoop: Another bug where the empty, not full, part of the ringbuffer was observed

[Takashi Iwai]

On Mon, 22 Jun 2020 15:15:13 +0200,
Mark Hills wrote:
> 
> This looks like a simple mistake dating back to 2003 (commit 7470a5b9)
> where code originated from dmix.
> 
> Signed-off-by: Mark Hills <mark@xwax.org>

Thanks, applied.


Takashi

Re: [PATCH 8/9] dsnoop: Make use of the (previously unused) function

[Takashi Iwai]

On Mon, 22 Jun 2020 15:15:14 +0200,
Mark Hills wrote:
> 
> Match the equivalent funciton for playback. This is on the assumption
> that values should be capped at zero, which is what _rewindable()
> implements.
> 
> Signed-off-by: Mark Hills <mark@xwax.org>

Thanks, applied.


Takashi

Re: [PATCH 9/9] pcm: Annotate the _avail functions

[Takashi Iwai]

On Mon, 22 Jun 2020 15:15:15 +0200,
Mark Hills wrote:
> 
> I took time to understand these functions in the context of the
> rest of the code, which would have been a lot quicker with a comment
> like this.
> 
> Signed-off-by: Mark Hills <mark@xwax.org>

Thanks, applied.


Takashi

Re: [PATCH 3/9] conf: Read a host-specific asoundrc

[Mark Hills]

On Tue, 23 Jun 2020, Takashi Iwai wrote:

> On Mon, 22 Jun 2020 15:15:09 +0200,
> Mark Hills wrote:
> > 
> > On systems with a network mounted home directory this is thoroughly
> > useful to allow for a core set of asoundrc settings, but with different
> > settings on different hosts.
> > 
> > It's not possibly to implement this in our own asoundrc or local
> > customisation, as it's too late. The installation file must be modified.
> > 
> > This is similar to ~/.Xdefaults-* on some systems.
> > 
> > Signed-off-by: Mark Hills <mark@xwax.org>
> 
> This kind of change popping up sometimes in the past, too, and I have
> a mixed feeling whether to take such a change globally or not.
> 
> In one side, it can work, but OTOH, if you can deal with that detail,
> you're certainly able to set up the environment variable easily, too.

I'm happy for a concern to be raised.

Can you clarify, which environment variable?

I wasn't aware it was possible to override asoundrc with an environment 
variable, until I looked up just now and found ALSA_CONFIG_PATH in the 
code.

Though I do have vague recollection of trying this some years ago. I think 
perhaps I ad issues using it to read other config files. I will re-visit 
when I have time.

FYI To solve my development work, I have:

  ~/.asoundrc      --  my personal settings
  ~/.asoundrc-xxx  --  host specific settings

and I must read both, in that order.

-- 
Mark

Re: [PATCH 3/9] conf: Read a host-specific asoundrc

[Takashi Iwai]

On Tue, 23 Jun 2020 13:18:49 +0200,
Mark Hills wrote:
> 
> On Tue, 23 Jun 2020, Takashi Iwai wrote:
> 
> > On Mon, 22 Jun 2020 15:15:09 +0200,
> > Mark Hills wrote:
> > > 
> > > On systems with a network mounted home directory this is thoroughly
> > > useful to allow for a core set of asoundrc settings, but with different
> > > settings on different hosts.
> > > 
> > > It's not possibly to implement this in our own asoundrc or local
> > > customisation, as it's too late. The installation file must be modified.
> > > 
> > > This is similar to ~/.Xdefaults-* on some systems.
> > > 
> > > Signed-off-by: Mark Hills <mark@xwax.org>
> > 
> > This kind of change popping up sometimes in the past, too, and I have
> > a mixed feeling whether to take such a change globally or not.
> > 
> > In one side, it can work, but OTOH, if you can deal with that detail,
> > you're certainly able to set up the environment variable easily, too.
> 
> I'm happy for a concern to be raised.
> 
> Can you clarify, which environment variable?
> 
> I wasn't aware it was possible to override asoundrc with an environment 
> variable, until I looked up just now and found ALSA_CONFIG_PATH in the 
> code.

Hrm, you're right, I thought we had a simple override, but it doesn't
look so.

OK, then it makes sense to take your patch.  Or rather better to allow
an own environment variable (e.g. $ASOUNDRC) instead?  It's more
flexible.


thanks,

Takashi

> Though I do have vague recollection of trying this some years ago. I think 
> perhaps I ad issues using it to read other config files. I will re-visit 
> when I have time.
> 
> FYI To solve my development work, I have:
> 
>   ~/.asoundrc      --  my personal settings
>   ~/.asoundrc-xxx  --  host specific settings
> 
> and I must read both, in that order.
> 
> -- 
> Mark
> 

Re: [PATCH 3/9] conf: Read a host-specific asoundrc

[Mark Hills]

On Tue, 23 Jun 2020, Takashi Iwai wrote:

> On Tue, 23 Jun 2020 13:18:49 +0200,
> Mark Hills wrote:
> > 
> > On Tue, 23 Jun 2020, Takashi Iwai wrote:
> > 
> > > On Mon, 22 Jun 2020 15:15:09 +0200,
> > > Mark Hills wrote:
> > > > 
> > > > On systems with a network mounted home directory this is thoroughly
> > > > useful to allow for a core set of asoundrc settings, but with different
> > > > settings on different hosts.
> > > > 
> > > > It's not possibly to implement this in our own asoundrc or local
> > > > customisation, as it's too late. The installation file must be modified.
> > > > 
> > > > This is similar to ~/.Xdefaults-* on some systems.
> > > > 
> > > > Signed-off-by: Mark Hills <mark@xwax.org>
> > > 
> > > This kind of change popping up sometimes in the past, too, and I have
> > > a mixed feeling whether to take such a change globally or not.
> > > 
> > > In one side, it can work, but OTOH, if you can deal with that detail,
> > > you're certainly able to set up the environment variable easily, too.
> > 
> > I'm happy for a concern to be raised.
> > 
> > Can you clarify, which environment variable?
> > 
> > I wasn't aware it was possible to override asoundrc with an environment 
> > variable, until I looked up just now and found ALSA_CONFIG_PATH in the 
> > code.
> 
> Hrm, you're right, I thought we had a simple override, but it doesn't
> look so.
> 
> OK, then it makes sense to take your patch.  Or rather better to allow
> an own environment variable (e.g. $ASOUNDRC) instead?  It's more
> flexible.

Why not let me experiment, I'll check ALSA_CONFIG_PATH, and then see what 
patch I can come up with, at least for my own use case.

Something like $ASOUNDRC will depend on whether we want it to augment, or 
fully replace the configuration.

I did do some experiments some time ago with fully replacing ALSA 
configuration; I find the defaults (eg. "Surround speakers" etc.) to be 
not a good match for my setup. I found it quite difficult to reason about 
the variable-driven design of the default asoundrc files and for me it 
seemed to cause more problems that it was solving. But then I think I 
discovered I could hack it with 'namehint' and moved on.

But in general something which allows policy to be passed to shell profile 
scripts is often not a bad thing.

-- 
Mark

Re: [PATCH 3/9] conf: Read a host-specific asoundrc

[Takashi Iwai]

On Tue, 23 Jun 2020 13:42:12 +0200,
Mark Hills wrote:
> 
> On Tue, 23 Jun 2020, Takashi Iwai wrote:
> 
> > On Tue, 23 Jun 2020 13:18:49 +0200,
> > Mark Hills wrote:
> > > 
> > > On Tue, 23 Jun 2020, Takashi Iwai wrote:
> > > 
> > > > On Mon, 22 Jun 2020 15:15:09 +0200,
> > > > Mark Hills wrote:
> > > > > 
> > > > > On systems with a network mounted home directory this is thoroughly
> > > > > useful to allow for a core set of asoundrc settings, but with different
> > > > > settings on different hosts.
> > > > > 
> > > > > It's not possibly to implement this in our own asoundrc or local
> > > > > customisation, as it's too late. The installation file must be modified.
> > > > > 
> > > > > This is similar to ~/.Xdefaults-* on some systems.
> > > > > 
> > > > > Signed-off-by: Mark Hills <mark@xwax.org>
> > > > 
> > > > This kind of change popping up sometimes in the past, too, and I have
> > > > a mixed feeling whether to take such a change globally or not.
> > > > 
> > > > In one side, it can work, but OTOH, if you can deal with that detail,
> > > > you're certainly able to set up the environment variable easily, too.
> > > 
> > > I'm happy for a concern to be raised.
> > > 
> > > Can you clarify, which environment variable?
> > > 
> > > I wasn't aware it was possible to override asoundrc with an environment 
> > > variable, until I looked up just now and found ALSA_CONFIG_PATH in the 
> > > code.
> > 
> > Hrm, you're right, I thought we had a simple override, but it doesn't
> > look so.
> > 
> > OK, then it makes sense to take your patch.  Or rather better to allow
> > an own environment variable (e.g. $ASOUNDRC) instead?  It's more
> > flexible.
> 
> Why not let me experiment, I'll check ALSA_CONFIG_PATH, and then see what 
> patch I can come up with, at least for my own use case.

Sure, I'm looking forward seeing a good outcome :)

> Something like $ASOUNDRC will depend on whether we want it to augment, or 
> fully replace the configuration.
> 
> I did do some experiments some time ago with fully replacing ALSA 
> configuration; I find the defaults (eg. "Surround speakers" etc.) to be 
> not a good match for my setup. I found it quite difficult to reason about 
> the variable-driven design of the default asoundrc files and for me it 
> seemed to cause more problems that it was solving. But then I think I 
> discovered I could hack it with 'namehint' and moved on.
> 
> But in general something which allows policy to be passed to shell profile 
> scripts is often not a bad thing.

Yes, it makes one's life sometimes easier...


Takashi