[pulseaudio-discuss] [PATCH 2/2] alsa: Add support for channel reconfiguration

[Tanu Kaskinen]

On Sun, 2018-05-20 at 11:58 +0530, Arun Raghavan wrote:
> This is needed for supporting high-bitrate formats in passthrough mode.
> The alsa-source reconfiguration path is untested at the moment as I do
> not have hardware that can support multiple channel configurations for
> capture.
> 
> This patch is not complete, because setting a channel count that results
> in a frame size that does not exactly divide the h/w buffer size results
> in a failure during unsuspend() -- the buffer and period count that we
> get are necessarily lower than the total buffer / period size.

Is there a reason why you don't reconfigure the buffer size?

> ---
>  src/modules/alsa/alsa-sink.c   | 43 ++++++++++++++++++++++++++++------
>  src/modules/alsa/alsa-source.c | 43 ++++++++++++++++++++++++++++------
>  src/modules/alsa/alsa-util.c   | 38 ++++++++++++++++++++++++++++++
>  src/modules/alsa/alsa-util.h   |  1 +
>  4 files changed, 111 insertions(+), 14 deletions(-)
> 
> diff --git a/src/modules/alsa/alsa-sink.c b/src/modules/alsa/alsa-sink.c
> index ed9e0a51c..56d4feafd 100644
> --- a/src/modules/alsa/alsa-sink.c
> +++ b/src/modules/alsa/alsa-sink.c
> @@ -112,6 +112,7 @@ struct userdata {
>      pa_cvolume hardware_volume;
>  
>      unsigned int *rates;
> +    unsigned int *channels;
>  
>      size_t
>          frame_size,
> @@ -1670,30 +1671,49 @@ static bool sink_set_formats(pa_sink *s, pa_idxset *formats) {
>      return true;
>  }
>  
> -static int sink_reconfigure_cb(pa_sink *s, pa_sample_spec *spec, bool passthrough) {
> +static int sink_reconfigure_cb(pa_sink *s, pa_sample_spec *spec, pa_channel_map *map, bool passthrough) {
>      struct userdata *u = s->userdata;
>      int i;
> -    bool supported = false;
> -
> -    /* FIXME: we only update rate for now */
> +    bool supported_rate = false, supported_channels = false;
>  
>      pa_assert(u);
>  
>      for (i = 0; u->rates[i]; i++) {
>          if (u->rates[i] == spec->rate) {
> -            supported = true;
> +            supported_rate = true;
>              break;
>          }
>      }
>  
> -    if (!supported) {
> +    if (!supported_rate) {
>          pa_log_info("Sink does not support sample rate of %d Hz", spec->rate);
>          return -1;
>      }
>  
> +    for (i = 0; u->channels[i]; i++) {
> +        if (u->channels[i] == spec->channels) {
> +            supported_channels = true;
> +            break;
> +        }
> +    }
> +
> +    if (!supported_channels) {
> +        pa_log_info("Sink does not support channels: %d", spec->channels);
> +        return -1;
> +    }
> +
>      if (!PA_SINK_IS_OPENED(s->state)) {
> -        pa_log_info("Updating rate for device %s, new rate is %d", u->device_name, spec->rate);
> +        pa_log_info("Updating rate and channels for device %s, %d Hz, %d channels", u->device_name, spec->rate, spec->channels);
> +
>          u->sink->sample_spec.rate = spec->rate;
> +        u->sink->sample_spec.channels = spec->channels;
> +        if (map)
> +            u->sink->channel_map = *map;
> +        else
> +            pa_channel_map_init_auto(&u->sink->channel_map, u->sink->sample_spec.channels, PA_CHANNEL_MAP_ALSA);

You should use pa_channel_map_init_extend(), because
pa_channel_map_init_auto() can fail. (Same for the source.)

> +unsigned int *pa_alsa_get_supported_channels(snd_pcm_t *pcm, unsigned int fallback_channels) {
> +    snd_pcm_hw_params_t *hwparams;
> +    unsigned int min, max;
> +    unsigned int *channels;
> +    unsigned int n, i;
> +    int ret;
> +
> +    snd_pcm_hw_params_alloca(&hwparams);
> +
> +    if ((ret = snd_pcm_hw_params_any(pcm, hwparams)) < 0) {
> +        pa_log_debug("snd_pcm_hw_params_any() failed: %s", pa_alsa_strerror(ret));
> +        return NULL;
> +    }
> +
> +    if (snd_pcm_hw_params_get_channels_min(hwparams, &min) < 0 ||
> +        snd_pcm_hw_params_get_channels_max(hwparams, &max) < 0 ||
> +        min - max + 1 <= 0) {
> +
> +        pa_log_debug("Could not probe channel range");
> +        return NULL;
> +    }
> +
> +    channels = pa_xnew0(unsigned int, max - min + 1);

The intention seems to be that the last array element is always 0 (it
would be good to document this, by the way), but here you don't reserve
space for it. If max and min are the same, then the array has length of
1, which isn't enough for the terminating 0.

-- 
Tanu

https://liberapay.com/tanuk
https://www.patreon.com/tanuk