On 12/30/2013 11:30 AM, Peter Meerwald wrote:
>
>> 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.
>> 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. :-)
> 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.
>
>> + 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);
>>
>>
>
--
David Henningsson, Canonical Ltd.
https://launchpad.net/~diwic
