Hi,
> >> 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?
> Good question.
> I don't think I ever got 2.1-on-5.1 to work, i e, on desktops with three
> 3.5 mm stereo line outputs, surround 2.1 should be available (and output
> three channels silent).
>
> Second, there are all variants of bass speakers now, some have it on the
> left side, others on the right, and then some on both. Since this patch
> can potentially block non-matching outputs, I'm not 100% sure it won't
> cause a regression compared to only applying the 2.1 patch.
we'll never find out if the patch is not tried
I think some concrete example(s) would be good to illustrate the purpose
of the patch series and what is meant by 'best'
ciao, p.
> >> 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 :)
>
> The coding style document doesn't say anything about internal and
> external functions when it comes to asserts.
> But in general, I tend to use asserts when I want pulseaudio to crash,
> which is approximately never. :-)
right; an assert() is part of the documentation for me, which makes them
more needed for functions that for part of an external interface
> > 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
>
> If you look at the definition of snd_pcm_chmap_t, you'll find that
> alloca(sizeof(snd_pcm_chmap_t)) is not a good idea.
> But maybe something like alloca(sizeof(snd_pcm_chmap_t) +
> sizeof(chmap->pos[0]) * map->channels) would be more future proof.
this is what I meant
> >
> >> + 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)
