On Tue, 2016-05-03 at 18:22 +0530, a...@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
_______________________________________________
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
