[pulseaudio-discuss] [PATCH 3/3] alsa-ucm: Support Playback/CaptureVolume

[Tanu Kaskinen]

On Tue, 2016-05-03 at 18:22 +0530, arun at accosted.net wrote:
> @@ -2479,7 +2515,8 @@ static void userdata_free(struct userdata *u) {
>      if (u->mixer_fdl)
>          pa_alsa_fdlist_free(u->mixer_fdl);
>  
> -    if (u->mixer_path && !u->mixer_path_set)
> +    /* Only free the mixer_path if the sink owns it */

This same comment could be added to alsa-source.c.

>  static int source_set_port_ucm_cb(pa_source *s, pa_device_port *p) {
>      struct userdata *u = s->userdata;
> +    pa_alsa_ucm_port_data *data;
> +
> +    data = PA_DEVICE_PORT_DATA(p);
>  
>      pa_assert(u);
>      pa_assert(p);
>      pa_assert(u->ucm_context);
> +    pa_assert(u->mixer_handle);
> +
> +    u->mixer_path = data->path;
> +    mixer_volume_init(u);
> +
> +    if (u->mixer_path) {
> +        pa_alsa_path_select(u->mixer_path, NULL, u->mixer_handle, s->muted);
> +
> +        if (s->set_mute)
> +            s->set_mute(s);
> +        if (s->flags & PA_SINK_DEFERRED_VOLUME) {

Should be PA_SOURCE_DEFERRED_VOLUME.

> @@ -297,6 +289,17 @@ static int ucm_get_device_property(
>              else
>                  pa_log_debug("UCM playback priority %s for device %s error", value, device_name);
>          }
> +
> +        /* Volume control element */
> +        if (!device->playback_volume)
> +            device->playback_volume = pa_hashmap_new_full(pa_idxset_string_hash_func, pa_idxset_string_compare_func, pa_xfree,
> +                                                          pa_xfree);

I think it would make more sense to create this hashmap when creating
the device object (in ucm_get_devices).

> @@ -318,6 +321,17 @@ static int ucm_get_device_property(
>              else
>                  pa_log_debug("UCM capture priority %s for device %s error", value, device_name);
>          }
> +
> +        /* Volume control element */
> +        if (!device->capture_volume)
> +            device->capture_volume = pa_hashmap_new_full(pa_idxset_string_hash_func, pa_idxset_string_compare_func, pa_xfree,
> +                                                         pa_xfree);

Same as above.

> +static void probe_volumes(pa_hashmap *hash, snd_pcm_t *pcm_handle, bool ignore_dB) {
> +    pa_device_port *port;
> +    pa_alsa_path *path;
> +    pa_alsa_ucm_port_data *data;
> +    snd_mixer_t *mixer_handle;
> +    const char *profile;
> +    void *state, *state2;
> +
> +    if (!(mixer_handle = pa_alsa_open_mixer_for_pcm(pcm_handle, NULL))) {
> +        pa_log_error("Failed to find a working mixer device.");
> +        goto fail;
> +    }
> +
> +    PA_HASHMAP_FOREACH(port, hash, state) {
> +        data = PA_DEVICE_PORT_DATA(port);
> +
> +        PA_HASHMAP_FOREACH_KV(profile, path, data->paths, state2) {
> +            if (pa_alsa_path_probe(path, mixer_handle, ignore_dB) < 0) {
> +                pa_log_warn("Could not probe path: %s, using s/w volume", data->path->name);
> +                pa_hashmap_remove(data->paths, profile);
> +            } else
> +                pa_log_debug("Set up h/w volume using '%s' for %s:%s", path->name, profile, port->name);
> +        }
> +    }
> +
> +    snd_mixer_close(mixer_handle);
> +
> +    return;
> +
> +fail:
> +    /* We could not probe the paths we created. Free them and revert to software volumes. */
> +    PA_HASHMAP_FOREACH(port, hash, state) {
> +        data = PA_DEVICE_PORT_DATA(port);
> +        pa_hashmap_remove_all(data->paths);
> +
> +        data->paths = NULL;

You only removed the contents of data->paths, you didn't free the
hashmap, so setting data->paths to NULL here leaks the hashmap. I think
it's better to keep the hashmap alive but empty, so this assignment can
just be dropped.

> +        data->path = NULL;

This is unnecessary too, because data->path is set when activating a
profile, and volumes are probed before the initial profile has been
chosen. data->path is already NULL at this point.

> @@ -710,7 +764,10 @@ static void ucm_add_port_combination(
>      char *name, *desc;
>      const char *dev_name;
>      const char *direction;
> +    const char *profile, *volume;

I think "volume" can be a bit confusing name (I tend associate it with
something that contains an actual volume value). I'd prefer "element"
or "volume_element". Or "element_name". Or something like that. Since
the UCM variable name is "PlaybackVolume" or "CaptureVolume", I
understand that "volume" is justifiable from that point of view,
though. I don't mind if you prefer to keep the variable name as is.

> @@ -139,6 +142,11 @@ struct pa_alsa_ucm_device {
>      unsigned playback_channels;
>      unsigned capture_channels;
>  
> +    /* These may be different per verb, so we store this as a hashmap of verb -> volume_control. We might eventually want to
> +     * make this a hashmap of verb -> per-verb-device-properties-struct. */
> +    pa_hashmap *playback_volume;
> +    pa_hashmap *capture_volume;

"playback_volumes"/"capture_volumes" would make more sense to me, since
these variables are containers that can hold multiple values. Bonus
points for including "elements" or "element_names" in the variable
names (that can get a bit long, though)...

-- 
Tanu