[PATCH 0/3] alsa-lib - improve surround 2.1 support

[PATCH 0/3] alsa-lib - improve surround 2.1 support

[David Henningsson]

So, in order to support 2.1 surround on both 4-channels (e g FL FR LFE LFE,
on internal subwoofer) and 6-channels (e g FL FR RL RR C LFE, on desktops
with 5.1 out), I needed to make the route plugin more chmap aware.

This also included allowing a new syntax for the ttable, which I think is
quite nice. The new syntax also falls back to ALSAs standard chmap in case
the hardware does not support it.

David Henningsson (3):
  route: Allow chmap syntax for slave channels in ttable
  route: Select slave chmap based on ttable information
  conf: Allow 2.1 surround to use different number of channels

 src/conf/pcm/surround21.conf |   7 +-
 src/pcm/pcm_route.c          | 304 ++++++++++++++++++++++++++++++++++++++-----
 2 files changed, 271 insertions(+), 40 deletions(-)

-- 
1.9.0

[PATCH 1/3] route: Allow chmap syntax for slave channels in ttable

[David Henningsson]

Instead of writing e g "0" and "1", one can now write "FL" and "FR" instead.

E g:
	ttable.0.FL 1
	ttable.1.FR 1
	ttable.2.LFE 1

Signed-off-by: David Henningsson <david.henningsson@canonical.com>
---
 src/pcm/pcm_route.c | 22 ++++++++++++++++++++--
 1 file changed, 20 insertions(+), 2 deletions(-)

diff --git a/src/pcm/pcm_route.c b/src/pcm/pcm_route.c
index 2beedf6..ffc283f 100644
--- a/src/pcm/pcm_route.c
+++ b/src/pcm/pcm_route.c
@@ -789,6 +789,24 @@ static void snd_pcm_route_dump(snd_pcm_t *pcm, snd_output_t *out)
 	snd_pcm_dump(route->plug.gen.slave, out);
 }
 
+static int safe_chmapchannel(const char *id, long *channel)
+{
+	int err;
+	int ch;
+	err = safe_strtol(id, channel);
+	if (err >= 0)
+		return err;
+
+	ch = (int) snd_pcm_chmap_from_string(id);
+	if (ch == -1)
+		return -EINVAL;
+
+	/* For now, assume standard channel mapping */
+	*channel = ch - SND_CHMAP_FL;
+	return 0;
+}
+
+
 static const snd_pcm_ops_t snd_pcm_route_ops = {
 	.close = snd_pcm_route_close,
 	.info = snd_pcm_generic_info,
@@ -983,7 +1001,7 @@ int snd_pcm_route_determine_ttable(snd_config_t *tt,
 			const char *id;
 			if (snd_config_get_id(jnode, &id) < 0)
 				continue;
-			err = safe_strtol(id, &schannel);
+			err = safe_chmapchannel(id, &schannel);
 			if (err < 0) {
 				SNDERR("Invalid slave channel: %s", id);
 				return -EINVAL;
@@ -1046,7 +1064,7 @@ int snd_pcm_route_load_ttable(snd_config_t *tt, snd_pcm_route_ttable_entry_t *tt
 			const char *id;
 			if (snd_config_get_id(jnode, &id) < 0)
 				continue;
-			err = safe_strtol(id, &schannel);
+			err = safe_chmapchannel(id, &schannel);
 			if (err < 0 || 
 			    schannel < 0 || (unsigned int) schannel > tt_ssize || 
 			    (schannels > 0 && schannel >= schannels)) {
-- 
1.9.0

Re: [PATCH 1/3] route: Allow chmap syntax for slave channels in ttable

[Takashi Iwai]

At Fri, 21 Feb 2014 16:24:20 +0100,
David Henningsson wrote:
> 
> Instead of writing e g "0" and "1", one can now write "FL" and "FR" instead.
> 
> E g:
> 	ttable.0.FL 1
> 	ttable.1.FR 1
> 	ttable.2.LFE 1
> 
> Signed-off-by: David Henningsson <david.henningsson@canonical.com>
> ---
>  src/pcm/pcm_route.c | 22 ++++++++++++++++++++--
>  1 file changed, 20 insertions(+), 2 deletions(-)
> 
> diff --git a/src/pcm/pcm_route.c b/src/pcm/pcm_route.c
> index 2beedf6..ffc283f 100644
> --- a/src/pcm/pcm_route.c
> +++ b/src/pcm/pcm_route.c
> @@ -789,6 +789,24 @@ static void snd_pcm_route_dump(snd_pcm_t *pcm, snd_output_t *out)
>  	snd_pcm_dump(route->plug.gen.slave, out);
>  }
>  
> +static int safe_chmapchannel(const char *id, long *channel)

You need no safe_ prefix for this function.  safe_strtol() is just
because strtol() already exists in an unsafe way.

Besides that, I find it also OK to just return the channel (>= 0) or a
negative error.  But it's no big matter in which form is.


Takashi

> +{
> +	int err;
> +	int ch;
> +	err = safe_strtol(id, channel);
> +	if (err >= 0)
> +		return err;
> +
> +	ch = (int) snd_pcm_chmap_from_string(id);
> +	if (ch == -1)
> +		return -EINVAL;
> +
> +	/* For now, assume standard channel mapping */
> +	*channel = ch - SND_CHMAP_FL;
> +	return 0;
> +}
> +
> +
>  static const snd_pcm_ops_t snd_pcm_route_ops = {
>  	.close = snd_pcm_route_close,
>  	.info = snd_pcm_generic_info,
> @@ -983,7 +1001,7 @@ int snd_pcm_route_determine_ttable(snd_config_t *tt,
>  			const char *id;
>  			if (snd_config_get_id(jnode, &id) < 0)
>  				continue;
> -			err = safe_strtol(id, &schannel);
> +			err = safe_chmapchannel(id, &schannel);
>  			if (err < 0) {
>  				SNDERR("Invalid slave channel: %s", id);
>  				return -EINVAL;
> @@ -1046,7 +1064,7 @@ int snd_pcm_route_load_ttable(snd_config_t *tt, snd_pcm_route_ttable_entry_t *tt
>  			const char *id;
>  			if (snd_config_get_id(jnode, &id) < 0)
>  				continue;
> -			err = safe_strtol(id, &schannel);
> +			err = safe_chmapchannel(id, &schannel);
>  			if (err < 0 || 
>  			    schannel < 0 || (unsigned int) schannel > tt_ssize || 
>  			    (schannels > 0 && schannel >= schannels)) {
> -- 
> 1.9.0
> 

[PATCH 2/3] route: Select slave chmap based on ttable information

[David Henningsson]

It means we need to initialize this order:

 1) Read the ttable to figure out which channels are present
 2) Open slave pcm and find a matching chmap
 3) Determine size of ttable (this can now depend on the chmap)
 4) Read ttable coefficients
 5) At prepare time, select the matching chmap

Signed-off-by: David Henningsson <david.henningsson@canonical.com>
---
 src/pcm/pcm_route.c | 300 ++++++++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 257 insertions(+), 43 deletions(-)

diff --git a/src/pcm/pcm_route.c b/src/pcm/pcm_route.c
index ffc283f..355da1e 100644
--- a/src/pcm/pcm_route.c
+++ b/src/pcm/pcm_route.c
@@ -103,6 +103,7 @@ typedef struct {
 	snd_pcm_format_t sformat;
 	int schannels;
 	snd_pcm_route_params_t params;
+	snd_pcm_chmap_t *chmap;
 } snd_pcm_route_t;
 
 #endif /* DOC_HIDDEN */
@@ -518,6 +519,7 @@ static int snd_pcm_route_close(snd_pcm_t *pcm)
 		}
 		free(params->dsts);
 	}
+	free(route->chmap);
 	return snd_pcm_generic_close(pcm);
 }
 
@@ -789,21 +791,168 @@ static void snd_pcm_route_dump(snd_pcm_t *pcm, snd_output_t *out)
 	snd_pcm_dump(route->plug.gen.slave, out);
 }
 
-static int safe_chmapchannel(const char *id, long *channel)
+static int safe_chmapchannel(const char *id, snd_pcm_chmap_t *chmap,
+			     long *channel, int channel_size)
 {
-	int err;
 	int ch;
-	err = safe_strtol(id, channel);
-	if (err >= 0)
-		return err;
+	if (safe_strtol(id, channel) >= 0)
+		return 1;
 
 	ch = (int) snd_pcm_chmap_from_string(id);
 	if (ch == -1)
 		return -EINVAL;
 
-	/* For now, assume standard channel mapping */
-	*channel = ch - SND_CHMAP_FL;
+	if (chmap) {
+		int i, r = 0;
+		/* Start with highest channel to simplify implementation of
+		   determine ttable size */
+		for (i = chmap->channels - 1; i >= 0; i--) {
+			if ((int) chmap->pos[i] != ch)
+				continue;
+			if (r >= channel_size)
+				continue;
+			channel[r++] = i;
+		}
+		return r;
+	}
+	else {
+		/* Assume ALSA standard channel mapping */
+		*channel = ch - SND_CHMAP_FL;
+		return 1;
+	}
+}
+
+#define MAX_CHMAP_CHANNELS 256
+
+static int determine_chmap(snd_config_t *tt, snd_pcm_chmap_t **tt_chmap)
+{
+	snd_config_iterator_t i, inext;
+	snd_pcm_chmap_t *chmap;
+
+	assert(tt && tt_chmap);
+	chmap = malloc(sizeof(snd_pcm_chmap_t) +
+		       MAX_CHMAP_CHANNELS * sizeof(unsigned int));
+
+	chmap->channels = 0;
+	snd_config_for_each(i, inext, tt) {
+		const char *id;
+		snd_config_iterator_t j, jnext;
+		snd_config_t *in = snd_config_iterator_entry(i);
+
+		if (!snd_config_get_id(in, &id) < 0)
+			continue;
+		if (snd_config_get_type(in) != SND_CONFIG_TYPE_COMPOUND)
+			goto err;
+		snd_config_for_each(j, jnext, in) {
+			int ch, k, found;
+			long schannel;
+			snd_config_t *jnode = snd_config_iterator_entry(j);
+			if (snd_config_get_id(jnode, &id) < 0)
+				continue;
+			if (safe_strtol(id, &schannel) >= 0)
+				continue;
+			ch = (int) snd_pcm_chmap_from_string(id);
+			if (ch == -1)
+				goto err;
+
+			found = 0;
+			for (k = 0; k < (int) chmap->channels; k++)
+				if (ch == (int) chmap->pos[k]) {
+					found = 1;
+					break;
+				}
+			if (found)
+				continue;
+
+			if (chmap->channels >= MAX_CHMAP_CHANNELS) {
+				SNDERR("Too many channels in ttable chmap");
+				goto err;
+			}
+			chmap->pos[chmap->channels++] = ch;
+		}
+	}
+
+
+	*tt_chmap = chmap;
 	return 0;
+
+err:
+	*tt_chmap = NULL;
+	free(chmap);
+	return -EINVAL;
+}
+
+static int find_matching_chmap(snd_pcm_t *spcm, snd_pcm_chmap_t *tt_chmap,
+			       snd_pcm_chmap_t **found_chmap, int *schannels)
+{
+	snd_pcm_chmap_query_t** chmaps = snd_pcm_query_chmaps(spcm);
+	int i;
+
+	*found_chmap = NULL;
+
+	if (chmaps == NULL)
+		return 0; /* chmap API not supported for this slave */
+
+	for (i = 0; chmaps[i]; i++) {
+		unsigned int j, k;
+		int match = 1;
+		snd_pcm_chmap_t *c = &chmaps[i]->map;
+		if (*schannels >= 0 && (int) c->channels != *schannels)
+			continue;
+
+		for (j = 0; j < tt_chmap->channels; j++) {
+			int found = 0;
+			unsigned int ch = tt_chmap->pos[j];
+			for (k = 0; k < c->channels; k++)
+				if (c->pos[k] == ch) {
+					found = 1;
+					break;
+				}
+			if (!found) {
+				match = 0;
+				break;
+			}
+		}
+
+		if (match) {
+			int size = sizeof(snd_pcm_chmap_t) + c->channels * sizeof(unsigned int);
+			*found_chmap = malloc(size);
+			memcpy(*found_chmap, c, size);
+			*schannels = c->channels;
+			break;
+		}
+	}
+
+	snd_pcm_free_chmaps(chmaps);
+
+	if (*found_chmap == NULL) {
+		SNDERR("Found no matching channel map");
+		return -EINVAL;
+	}
+	return 0;
+}
+
+static int route_chmap_init(snd_pcm_t *pcm)
+{
+	int set_map = 0;
+	snd_pcm_chmap_t *current;
+	snd_pcm_route_t *route = pcm->private_data;
+	if (!route->chmap)
+		return 0;
+	if (snd_pcm_state(pcm) != SND_PCM_STATE_PREPARED)
+		return 0;
+
+	current = snd_pcm_get_chmap(route->plug.gen.slave);
+	if (!current)
+		return -ENOSYS;
+	if (current->channels != route->chmap->channels)
+		set_map = 1;
+	else set_map = memcmp(current->pos, route->chmap->pos, current->channels);
+	free(current);
+	if (!set_map)
+		return 0;
+
+	return snd_pcm_set_chmap(route->plug.gen.slave, route->chmap);
 }
 
 
@@ -939,6 +1088,7 @@ int snd_pcm_route_open(snd_pcm_t **pcmp, const char *name,
 	route->plug.undo_write = snd_pcm_plugin_undo_write_generic;
 	route->plug.gen.slave = slave;
 	route->plug.gen.close_slave = close_slave;
+	route->plug.init = route_chmap_init;
 
 	err = snd_pcm_new(&pcm, SND_PCM_TYPE_ROUTE, name, slave->stream, slave->mode);
 	if (err < 0) {
@@ -963,16 +1113,10 @@ int snd_pcm_route_open(snd_pcm_t **pcmp, const char *name,
 	return 0;
 }
 
-/**
- * \brief Determine route matrix sizes
- * \param tt Configuration root describing route matrix
- * \param tt_csize Returned client size in elements
- * \param tt_ssize Returned slave size in elements
- * \retval zero on success otherwise a negative error code
- */
-int snd_pcm_route_determine_ttable(snd_config_t *tt,
-				   unsigned int *tt_csize,
-				   unsigned int *tt_ssize)
+static int _snd_pcm_route_determine_ttable(snd_config_t *tt,
+					   unsigned int *tt_csize,
+					   unsigned int *tt_ssize,
+					   snd_pcm_chmap_t *chmap)
 {
 	snd_config_iterator_t i, inext;
 	long csize = 0, ssize = 0;
@@ -1001,7 +1145,7 @@ int snd_pcm_route_determine_ttable(snd_config_t *tt,
 			const char *id;
 			if (snd_config_get_id(jnode, &id) < 0)
 				continue;
-			err = safe_chmapchannel(id, &schannel);
+			err = safe_chmapchannel(id, chmap, &schannel, 1);
 			if (err < 0) {
 				SNDERR("Invalid slave channel: %s", id);
 				return -EINVAL;
@@ -1020,6 +1164,20 @@ int snd_pcm_route_determine_ttable(snd_config_t *tt,
 }
 
 /**
+ * \brief Determine route matrix sizes
+ * \param tt Configuration root describing route matrix
+ * \param tt_csize Returned client size in elements
+ * \param tt_ssize Returned slave size in elements
+ * \retval zero on success otherwise a negative error code
+ */
+int snd_pcm_route_determine_ttable(snd_config_t *tt,
+				   unsigned int *tt_csize,
+				   unsigned int *tt_ssize)
+{
+	return _snd_pcm_route_determine_ttable(tt, tt_csize, tt_ssize, NULL);
+}
+
+/**
  * \brief Load route matrix
  * \param tt Configuration root describing route matrix
  * \param ttable Returned route matrix
@@ -1030,10 +1188,10 @@ int snd_pcm_route_determine_ttable(snd_config_t *tt,
  * \param schannels Slave channels
  * \retval zero on success otherwise a negative error code
  */
-int snd_pcm_route_load_ttable(snd_config_t *tt, snd_pcm_route_ttable_entry_t *ttable,
-			      unsigned int tt_csize, unsigned int tt_ssize,
-			      unsigned int *tt_cused, unsigned int *tt_sused,
-			      int schannels)
+static int _snd_pcm_route_load_ttable(snd_config_t *tt, snd_pcm_route_ttable_entry_t *ttable,
+				      unsigned int tt_csize, unsigned int tt_ssize,
+				      unsigned int *tt_cused, unsigned int *tt_sused,
+				      int schannels, snd_pcm_chmap_t *chmap)
 {
 	int cused = -1;
 	int sused = -1;
@@ -1060,17 +1218,18 @@ int snd_pcm_route_load_ttable(snd_config_t *tt, snd_pcm_route_ttable_entry_t *tt
 		snd_config_for_each(j, jnext, in) {
 			snd_config_t *jnode = snd_config_iterator_entry(j);
 			double value;
-			long schannel;
+			int ss;
+			long *scha = alloca(tt_ssize * sizeof(long));
 			const char *id;
 			if (snd_config_get_id(jnode, &id) < 0)
 				continue;
-			err = safe_chmapchannel(id, &schannel);
-			if (err < 0 || 
-			    schannel < 0 || (unsigned int) schannel > tt_ssize || 
-			    (schannels > 0 && schannel >= schannels)) {
+
+			ss = safe_chmapchannel(id, chmap, scha, tt_ssize);
+			if (ss < 0) {
 				SNDERR("Invalid slave channel: %s", id);
 				return -EINVAL;
 			}
+
 			err = snd_config_get_real(jnode, &value);
 			if (err < 0) {
 				long v;
@@ -1081,9 +1240,18 @@ int snd_pcm_route_load_ttable(snd_config_t *tt, snd_pcm_route_ttable_entry_t *tt
 				}
 				value = v;
 			}
-			ttable[cchannel * tt_ssize + schannel] = value;
-			if (schannel > sused)
-				sused = schannel;
+
+			for (k = 0; (int) k < ss; k++) {
+				long schannel = scha[k];
+				if (schannel < 0 || (unsigned int) schannel > tt_ssize ||
+				    (schannels > 0 && schannel >= schannels)) {
+					SNDERR("Invalid slave channel: %s", id);
+					return -EINVAL;
+				}
+				ttable[cchannel * tt_ssize + schannel] = value;
+				if (schannel > sused)
+					sused = schannel;
+			}
 		}
 		if (cchannel > cused)
 			cused = cchannel;
@@ -1093,6 +1261,26 @@ int snd_pcm_route_load_ttable(snd_config_t *tt, snd_pcm_route_ttable_entry_t *tt
 	return 0;
 }
 
+/**
+ * \brief Load route matrix
+ * \param tt Configuration root describing route matrix
+ * \param ttable Returned route matrix
+ * \param tt_csize Client size in elements
+ * \param tt_ssize Slave size in elements
+ * \param tt_cused Used client elements
+ * \param tt_sused Used slave elements
+ * \param schannels Slave channels
+ * \retval zero on success otherwise a negative error code
+ */
+int snd_pcm_route_load_ttable(snd_config_t *tt, snd_pcm_route_ttable_entry_t *ttable,
+			      unsigned int tt_csize, unsigned int tt_ssize,
+			      unsigned int *tt_cused, unsigned int *tt_sused,
+			      int schannels)
+{
+	return _snd_pcm_route_load_ttable(tt, ttable, tt_csize, tt_ssize,
+					  tt_cused, tt_sused, schannels, NULL);
+}
+
 /*! \page pcm_plugins
 
 \section pcm_plugins_route Plugin: Route & Volume
@@ -1100,6 +1288,9 @@ int snd_pcm_route_load_ttable(snd_config_t *tt, snd_pcm_route_ttable_entry_t *tt
 This plugin converts channels and applies volume during the conversion.
 The format and rate must match for both of them.
 
+SCHANNEL can be a channel name instead of a number (e g FL, LFE).
+If so, a matching channel map will be selected for the slave.
+
 \code
 pcm.name {
         type route              # Route & Volume conversion PCM
@@ -1150,6 +1341,7 @@ int _snd_pcm_route_open(snd_pcm_t **pcmp, const char *name,
 	int err;
 	snd_pcm_t *spcm;
 	snd_config_t *slave = NULL, *sconf;
+	snd_pcm_chmap_t *tt_chmap, *chmap;
 	snd_pcm_format_t sformat = SND_PCM_FORMAT_UNKNOWN;
 	int schannels = -1;
 	snd_config_t *tt = NULL;
@@ -1198,37 +1390,59 @@ int _snd_pcm_route_open(snd_pcm_t **pcmp, const char *name,
 		return -EINVAL;
 	}
 
-	err = snd_pcm_route_determine_ttable(tt, &csize, &ssize);
+	err = determine_chmap(tt, &tt_chmap);
 	if (err < 0) {
-		snd_config_delete(sconf);
+		free(ttable);
 		return err;
 	}
-	ttable = malloc(csize * ssize * sizeof(snd_pcm_route_ttable_entry_t));
-	if (ttable == NULL) {
-		snd_config_delete(sconf);
-		return -ENOMEM;
-	}
-	err = snd_pcm_route_load_ttable(tt, ttable, csize, ssize,
-					&cused, &sused, schannels);
+
+	err = snd_pcm_open_slave(&spcm, root, sconf, stream, mode, conf);
+	snd_config_delete(sconf);
 	if (err < 0) {
+		free(tt_chmap);
 		free(ttable);
-		snd_config_delete(sconf);
 		return err;
 	}
 
-	err = snd_pcm_open_slave(&spcm, root, sconf, stream, mode, conf);
-	snd_config_delete(sconf);
+	if (tt_chmap) {
+		err = find_matching_chmap(spcm, tt_chmap, &chmap, &schannels);
+		free(tt_chmap);
+		if (err < 0)
+			return err;
+	}
+
+	err = _snd_pcm_route_determine_ttable(tt, &csize, &ssize, chmap);
 	if (err < 0) {
+		free(chmap);
+		snd_pcm_close(spcm);
+		return err;
+	}
+	ttable = malloc(csize * ssize * sizeof(snd_pcm_route_ttable_entry_t));
+	if (ttable == NULL) {
+		free(chmap);
+		snd_pcm_close(spcm);
+		return -ENOMEM;
+	}
+	err = _snd_pcm_route_load_ttable(tt, ttable, csize, ssize,
+					&cused, &sused, schannels, chmap);
+	if (err < 0) {
+		free(chmap);
 		free(ttable);
+		snd_pcm_close(spcm);
 		return err;
 	}
+
 	err = snd_pcm_route_open(pcmp, name, sformat, schannels,
 				 ttable, ssize,
 				 cused, sused,
 				 spcm, 1);
 	free(ttable);
-	if (err < 0)
+	if (err < 0) {
+		free(chmap);
 		snd_pcm_close(spcm);
+	}
+	((snd_pcm_route_t*) (*pcmp)->private_data)->chmap = chmap;
+
 	return err;
 }
 #ifndef DOC_HIDDEN
-- 
1.9.0

Re: [PATCH 2/3] route: Select slave chmap based on ttable information

[Takashi Iwai]

At Fri, 21 Feb 2014 16:24:21 +0100,
David Henningsson wrote:
> 
> It means we need to initialize this order:
> 
>  1) Read the ttable to figure out which channels are present
>  2) Open slave pcm and find a matching chmap
>  3) Determine size of ttable (this can now depend on the chmap)
>  4) Read ttable coefficients
>  5) At prepare time, select the matching chmap
> 
> Signed-off-by: David Henningsson <david.henningsson@canonical.com>
> ---
>  src/pcm/pcm_route.c | 300 ++++++++++++++++++++++++++++++++++++++++++++--------
>  1 file changed, 257 insertions(+), 43 deletions(-)
> 
> diff --git a/src/pcm/pcm_route.c b/src/pcm/pcm_route.c
> index ffc283f..355da1e 100644
> --- a/src/pcm/pcm_route.c
> +++ b/src/pcm/pcm_route.c
> @@ -103,6 +103,7 @@ typedef struct {
>  	snd_pcm_format_t sformat;
>  	int schannels;
>  	snd_pcm_route_params_t params;
> +	snd_pcm_chmap_t *chmap;
>  } snd_pcm_route_t;
>  
>  #endif /* DOC_HIDDEN */
> @@ -518,6 +519,7 @@ static int snd_pcm_route_close(snd_pcm_t *pcm)
>  		}
>  		free(params->dsts);
>  	}
> +	free(route->chmap);
>  	return snd_pcm_generic_close(pcm);
>  }
>  
> @@ -789,21 +791,168 @@ static void snd_pcm_route_dump(snd_pcm_t *pcm, snd_output_t *out)
>  	snd_pcm_dump(route->plug.gen.slave, out);
>  }
>  
> -static int safe_chmapchannel(const char *id, long *channel)
> +static int safe_chmapchannel(const char *id, snd_pcm_chmap_t *chmap,
> +			     long *channel, int channel_size)

A brief description of the function would be helpful, as the code
becomes no longer trivial by this patch.


>  {
> -	int err;
>  	int ch;
> -	err = safe_strtol(id, channel);
> -	if (err >= 0)
> -		return err;
> +	if (safe_strtol(id, channel) >= 0)
> +		return 1;
>  
>  	ch = (int) snd_pcm_chmap_from_string(id);
>  	if (ch == -1)
>  		return -EINVAL;
>  
> -	/* For now, assume standard channel mapping */
> -	*channel = ch - SND_CHMAP_FL;
> +	if (chmap) {
> +		int i, r = 0;
> +		/* Start with highest channel to simplify implementation of
> +		   determine ttable size */
> +		for (i = chmap->channels - 1; i >= 0; i--) {
> +			if ((int) chmap->pos[i] != ch)
> +				continue;
> +			if (r >= channel_size)
> +				continue;
> +			channel[r++] = i;
> +		}
> +		return r;
> +	}
> +	else {
> +		/* Assume ALSA standard channel mapping */
> +		*channel = ch - SND_CHMAP_FL;
> +		return 1;
> +	}
> +}
> +
> +#define MAX_CHMAP_CHANNELS 256
> +
> +static int determine_chmap(snd_config_t *tt, snd_pcm_chmap_t **tt_chmap)
> +{
> +	snd_config_iterator_t i, inext;
> +	snd_pcm_chmap_t *chmap;
> +
> +	assert(tt && tt_chmap);
> +	chmap = malloc(sizeof(snd_pcm_chmap_t) +
> +		       MAX_CHMAP_CHANNELS * sizeof(unsigned int));
> +
> +	chmap->channels = 0;
> +	snd_config_for_each(i, inext, tt) {
> +		const char *id;
> +		snd_config_iterator_t j, jnext;
> +		snd_config_t *in = snd_config_iterator_entry(i);
> +
> +		if (!snd_config_get_id(in, &id) < 0)
> +			continue;
> +		if (snd_config_get_type(in) != SND_CONFIG_TYPE_COMPOUND)
> +			goto err;
> +		snd_config_for_each(j, jnext, in) {
> +			int ch, k, found;
> +			long schannel;
> +			snd_config_t *jnode = snd_config_iterator_entry(j);
> +			if (snd_config_get_id(jnode, &id) < 0)
> +				continue;
> +			if (safe_strtol(id, &schannel) >= 0)
> +				continue;
> +			ch = (int) snd_pcm_chmap_from_string(id);
> +			if (ch == -1)
> +				goto err;
> +
> +			found = 0;
> +			for (k = 0; k < (int) chmap->channels; k++)
> +				if (ch == (int) chmap->pos[k]) {
> +					found = 1;
> +					break;
> +				}
> +			if (found)
> +				continue;
> +
> +			if (chmap->channels >= MAX_CHMAP_CHANNELS) {
> +				SNDERR("Too many channels in ttable chmap");
> +				goto err;
> +			}
> +			chmap->pos[chmap->channels++] = ch;
> +		}
> +	}
> +
> +
> +	*tt_chmap = chmap;
>  	return 0;
> +
> +err:
> +	*tt_chmap = NULL;
> +	free(chmap);
> +	return -EINVAL;
> +}
> +
> +static int find_matching_chmap(snd_pcm_t *spcm, snd_pcm_chmap_t *tt_chmap,
> +			       snd_pcm_chmap_t **found_chmap, int *schannels)
> +{
> +	snd_pcm_chmap_query_t** chmaps = snd_pcm_query_chmaps(spcm);
> +	int i;
> +
> +	*found_chmap = NULL;
> +
> +	if (chmaps == NULL)
> +		return 0; /* chmap API not supported for this slave */
> +
> +	for (i = 0; chmaps[i]; i++) {
> +		unsigned int j, k;
> +		int match = 1;
> +		snd_pcm_chmap_t *c = &chmaps[i]->map;
> +		if (*schannels >= 0 && (int) c->channels != *schannels)
> +			continue;
> +
> +		for (j = 0; j < tt_chmap->channels; j++) {
> +			int found = 0;
> +			unsigned int ch = tt_chmap->pos[j];
> +			for (k = 0; k < c->channels; k++)
> +				if (c->pos[k] == ch) {
> +					found = 1;
> +					break;
> +				}
> +			if (!found) {
> +				match = 0;
> +				break;
> +			}
> +		}
> +
> +		if (match) {
> +			int size = sizeof(snd_pcm_chmap_t) + c->channels * sizeof(unsigned int);
> +			*found_chmap = malloc(size);

NULL check is missing here.


> +			memcpy(*found_chmap, c, size);
> +			*schannels = c->channels;
> +			break;
> +		}
> +	}
> +
> +	snd_pcm_free_chmaps(chmaps);
> +
> +	if (*found_chmap == NULL) {
> +		SNDERR("Found no matching channel map");
> +		return -EINVAL;
> +	}
> +	return 0;
> +}
> +
> +static int route_chmap_init(snd_pcm_t *pcm)
> +{
> +	int set_map = 0;
> +	snd_pcm_chmap_t *current;
> +	snd_pcm_route_t *route = pcm->private_data;
> +	if (!route->chmap)
> +		return 0;
> +	if (snd_pcm_state(pcm) != SND_PCM_STATE_PREPARED)
> +		return 0;
> +
> +	current = snd_pcm_get_chmap(route->plug.gen.slave);
> +	if (!current)
> +		return -ENOSYS;
> +	if (current->channels != route->chmap->channels)
> +		set_map = 1;
> +	else set_map = memcmp(current->pos, route->chmap->pos, current->channels);

Please fix the indentation.


Takashi

[PATCH 3/3] conf: Allow 2.1 surround to use different number of channels

[David Henningsson]

This way, cards that support LFE on four channels (e g laptop with
internal subwoofer) can do that, and other cards on a six channel setup
can use that as well.

Well, note that there is still a reference to "pcm.surround51" left here.
In practice, for HDA Intel sound cards this does not matter as both
surround51 and surround40 reference the same definition.
(And that's the only card I currently know of that actually does
surround2.1 over four channels.)

Signed-off-by: David Henningsson <david.henningsson@canonical.com>
---
 src/conf/pcm/surround21.conf | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/src/conf/pcm/surround21.conf b/src/conf/pcm/surround21.conf
index be29020..7f4676b 100644
--- a/src/conf/pcm/surround21.conf
+++ b/src/conf/pcm/surround21.conf
@@ -51,10 +51,9 @@ pcm.!surround21 {
 			]
 		}
 	}
-	slave.channels 6
-	ttable.0.0 1
-	ttable.1.1 1
-	ttable.2.5 1
+	ttable.0.FL 1
+	ttable.1.FR 1
+	ttable.2.LFE 1
 	hint {
 		description "2.1 Surround output to Front and Subwoofer speakers"
 		device $DEV
-- 
1.9.0

Re: [PATCH 0/3] alsa-lib - improve surround 2.1 support

[Takashi Iwai]

At Fri, 21 Feb 2014 16:24:19 +0100,
David Henningsson wrote:
> 
> So, in order to support 2.1 surround on both 4-channels (e g FL FR LFE LFE,
> on internal subwoofer) and 6-channels (e g FL FR RL RR C LFE, on desktops
> with 5.1 out), I needed to make the route plugin more chmap aware.
> 
> This also included allowing a new syntax for the ttable, which I think is
> quite nice. The new syntax also falls back to ALSAs standard chmap in case
> the hardware does not support it.

The idea looks great, a nice improvement.
Spotted a few trivial things I posted individually, but in general
it's in a good shape.


thanks,

Takashi


> 
> David Henningsson (3):
>   route: Allow chmap syntax for slave channels in ttable
>   route: Select slave chmap based on ttable information
>   conf: Allow 2.1 surround to use different number of channels
> 
>  src/conf/pcm/surround21.conf |   7 +-
>  src/pcm/pcm_route.c          | 304 ++++++++++++++++++++++++++++++++++++++-----
>  2 files changed, 271 insertions(+), 40 deletions(-)
> 
> -- 
> 1.9.0
>