> Recently ALSA added the channel mapping API. Here we use it to set
> the right channel map if ALSA gives us several maps to choose from,
> and skip the PCM completely if no map fit.
nitpicking below; what blocks this patch?
> Signed-off-by: David Henningsson <david.henningsson at canonical.com>
> ---
> src/modules/alsa/alsa-sink.c | 6 ++
> src/modules/alsa/alsa-source.c | 6 ++
> src/modules/alsa/alsa-util.c | 142 +++++++++++++++++++++++++++++++++++++++++
> src/modules/alsa/alsa-util.h | 2 +
> 4 files changed, 156 insertions(+)
>
> diff --git a/src/modules/alsa/alsa-sink.c b/src/modules/alsa/alsa-sink.c
> index e10e14e..e101d48 100644
> --- a/src/modules/alsa/alsa-sink.c
> +++ b/src/modules/alsa/alsa-sink.c
> @@ -1111,6 +1111,9 @@ static int unsuspend(struct userdata *u) {
> goto fail;
> }
>
> + if (pa_alsa_set_channel_map(u->pcm_handle, &u->sink->channel_map) < 0)
> + goto fail;
> +
> if (update_sw_params(u) < 0)
> goto fail;
>
> @@ -2243,6 +2246,9 @@ pa_sink *pa_alsa_sink_new(pa_module *m, pa_modargs *ma, const char*driver, pa_ca
> goto fail;
> }
>
> + if (pa_alsa_set_channel_map(u->pcm_handle, &map) < 0)
> + goto fail;
> +
> /* ALSA might tweak the sample spec, so recalculate the frame size */
> frame_size = pa_frame_size(&ss);
>
> diff --git a/src/modules/alsa/alsa-source.c b/src/modules/alsa/alsa-source.c
> index 2e93e0f..41e6601 100644
> --- a/src/modules/alsa/alsa-source.c
> +++ b/src/modules/alsa/alsa-source.c
> @@ -995,6 +995,9 @@ static int unsuspend(struct userdata *u) {
> goto fail;
> }
>
> + if (pa_alsa_set_channel_map(u->pcm_handle, &u->source->channel_map) < 0)
> + goto fail;
> +
> if (update_sw_params(u) < 0)
> goto fail;
>
> @@ -1942,6 +1945,9 @@ pa_source *pa_alsa_source_new(pa_module *m, pa_modargs *ma, const char*driver, p
> goto fail;
> }
>
> + if (pa_alsa_set_channel_map(u->pcm_handle, &map) < 0)
> + goto fail;
> +
> /* ALSA might tweak the sample spec, so recalculate the frame size */
> frame_size = pa_frame_size(&ss);
>
> diff --git a/src/modules/alsa/alsa-util.c b/src/modules/alsa/alsa-util.c
> index 75f5858..0baf54d 100644
> --- a/src/modules/alsa/alsa-util.c
> +++ b/src/modules/alsa/alsa-util.c
> @@ -478,6 +478,143 @@ int pa_alsa_set_sw_params(snd_pcm_t *pcm, snd_pcm_uframes_t avail_min, bool peri
> return 0;
> }
>
> +#if (SND_LIB_VERSION >= ((1<<16)|(0<<8)|27)) /* API additions in 1.0.27 */
> +
> +static const int chmap_channel_ids[PA_CHANNEL_POSITION_MAX] = {
> + [PA_CHANNEL_POSITION_MONO] = SND_CHMAP_MONO,
> +
> + [PA_CHANNEL_POSITION_FRONT_CENTER] = SND_CHMAP_FC,
> + [PA_CHANNEL_POSITION_FRONT_LEFT] = SND_CHMAP_FL,
> + [PA_CHANNEL_POSITION_FRONT_RIGHT] = SND_CHMAP_FR,
> +
> + [PA_CHANNEL_POSITION_REAR_CENTER] = SND_CHMAP_RC,
> + [PA_CHANNEL_POSITION_REAR_LEFT] = SND_CHMAP_RL,
> + [PA_CHANNEL_POSITION_REAR_RIGHT] = SND_CHMAP_RR,
> +
> + [PA_CHANNEL_POSITION_LFE] = SND_CHMAP_LFE,
> +
> + [PA_CHANNEL_POSITION_FRONT_LEFT_OF_CENTER] = SND_CHMAP_FLC,
> + [PA_CHANNEL_POSITION_FRONT_RIGHT_OF_CENTER] = SND_CHMAP_FRC,
> +
> + [PA_CHANNEL_POSITION_SIDE_LEFT] = SND_CHMAP_SL,
> + [PA_CHANNEL_POSITION_SIDE_RIGHT] = SND_CHMAP_SR,
> +
> + [PA_CHANNEL_POSITION_TOP_CENTER] = SND_CHMAP_TC,
> +
> + [PA_CHANNEL_POSITION_TOP_FRONT_CENTER] = SND_CHMAP_TFC,
> + [PA_CHANNEL_POSITION_TOP_FRONT_LEFT] = SND_CHMAP_TFL,
> + [PA_CHANNEL_POSITION_TOP_FRONT_RIGHT] = SND_CHMAP_TFR,
> +
> + [PA_CHANNEL_POSITION_TOP_REAR_CENTER] = SND_CHMAP_TRC,
> + [PA_CHANNEL_POSITION_TOP_REAR_LEFT] = SND_CHMAP_TRL,
> + [PA_CHANNEL_POSITION_TOP_REAR_RIGHT] = SND_CHMAP_TRR
> +};
> +
> +int pa_alsa_set_channel_map(snd_pcm_t *pcm, pa_channel_map *map) {
map could be const
> + snd_pcm_chmap_t *chmap;
> + unsigned int i;
> + int err;
> + char buf[512] = "";
there are no asserts; the internal functions proposed do have asserts...
it should be the other way around :)
init of buf unnecessary I think
> +
> + chmap = alloca(sizeof(unsigned int) * (map->channels + 1));
this seems dangerous; I'd rather have a sizeof(snd_pcm_chmap_t) in there,
and make sure it is zero'd
> + chmap->channels = map->channels;
> + for (i = 0; i < map->channels; i++)
> + chmap->pos[i] = chmap_channel_ids[map->map[i]];
> +
> + if (snd_pcm_chmap_print(chmap, sizeof(buf), buf) < 0)
> + return -1;
> + pa_log_debug("Setting channel map to %s", buf);
> +
> + if ((err = snd_pcm_set_chmap(pcm, chmap)) < 0) {
> + if (err == -ENXIO)
> + return 0; /* snd_pcm_set_chmap returns -ENXIO if device does not support channel maps at all */
> + pa_log_warn("Unable to set channel map: %s\n", pa_alsa_strerror(err));
> + return -1;
> + }
> + return 0;
> +}
> +
> +
> +static int is_chmap_compatible(snd_pcm_chmap_query_t *c, pa_channel_map *map) {
this function does a lot more that checking for compatibility, find better
name?
map const?
> + unsigned int i;
> + bool needsmove = false;
> + char cm[PA_CHANNEL_MAP_SNPRINT_MAX];
> + char buf[512] = "";
> +
> + pa_assert(c);
> + pa_assert(map);
> +
> + pa_channel_map_snprint(cm, sizeof(cm), map);
> + if (snd_pcm_chmap_print(&c->map, sizeof(buf), buf) < 0)
> + return -1;
> +
> + pa_log_debug("Checking ALSA channel map type %s, %s against PA map %s",
> + snd_pcm_chmap_type_name(c->type), buf, cm);
> +
> + if (c->map.channels != map->channels)
> + needsmove = true;
> +
> + for (i = 0; i < map->channels; i++) {
> + unsigned int j;
> + bool found = false;
> + unsigned int m = chmap_channel_ids[map->map[i]];
> + if (c->map.channels > i && m == c->map.pos[i])
> + continue; /* Great, it's where we want it! */
> + for (j = 0; j < c->map.channels; j++)
> + if (m == c->map.pos[j]) {
> + found = true;
> + if (c->type != SND_CHMAP_TYPE_VAR)
> + needsmove = true;
> + break;
> + }
> + if (!found)
> + return -1;
> + }
> +
> + return needsmove ? 1 : 2;
> +}
> +
> +static int query_chmap_for_pcm(snd_pcm_t *pcm, pa_channel_map *map) {
> +
> + snd_pcm_chmap_query_t **i, **avail;
> + int bestscore = 0;
> +
> + avail = snd_pcm_query_chmaps(pcm);
> + if (avail == NULL) {
> + pa_log_debug("This device does not support ALSA channel mapping API.");
> + return 0;
> + }
> +
> + for (i = avail; *i != NULL; i++) {
> + int score = is_chmap_compatible(*i, map);
> + if (score > bestscore)
> + bestscore = score;
> + }
> +
> + snd_pcm_free_chmaps(avail);
> +
> + if (bestscore < 2) {
> + /* TODO: If we find all channels but not in the right order, we can update
> + our own channel map to match. We haven't implemented that yet. */
> + pa_log_debug("Did not find a compatible channel map, skipping.");
> + return -1;
> + }
> +
> + return 0;
> +}
> +
> +#else
> +
> +static int query_chmap_for_pcm(snd_pcm_t *pcm, pa_channel_map *map) {
> + return 0;
> +}
> +
> +int pa_alsa_set_chmap(snd_pcm_t *pcm, pa_channel_map *map) {
> + return 0;
> +}
> +
> +#endif
> +
> snd_pcm_t *pa_alsa_open_by_device_id_auto(
> const char *dev_id,
> char **dev,
> @@ -714,6 +851,11 @@ snd_pcm_t *pa_alsa_open_by_device_string(
> goto fail;
> }
>
> + if (query_chmap_for_pcm(pcm_handle, map) < 0) {
> + snd_pcm_close(pcm_handle);
> + goto fail;
> + }
> +
> if (dev)
> *dev = d;
> else
> diff --git a/src/modules/alsa/alsa-util.h b/src/modules/alsa/alsa-util.h
> index 0e3ae69..2175985 100644
> --- a/src/modules/alsa/alsa-util.h
> +++ b/src/modules/alsa/alsa-util.h
> @@ -108,6 +108,8 @@ snd_pcm_t *pa_alsa_open_by_template(
> bool *use_tsched, /* modified at return */
> bool require_exact_channel_number);
>
> +int pa_alsa_set_channel_map(snd_pcm_t *pcm, pa_channel_map *map);
> +
> void pa_alsa_dump(pa_log_level_t level, snd_pcm_t *pcm);
> void pa_alsa_dump_status(snd_pcm_t *pcm);
>
>
--
Peter Meerwald
+43-664-2444418 (mobile)
