[PATCH 1/2] alsa: Use alsa-lib's channel mapping API to select the best channel map

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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)


[Index of Archives]     [Linux Audio Users]     [AMD Graphics]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux