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