[pulseaudio-discuss] [PATCH v3 1/6] core: Add default volume to ports
David Henningsson
david.henningsson at canonical.com
Thu Apr 30 01:44:00 PDT 2015
On 2015-04-27 13:34, Tanu Kaskinen wrote:
> The sink and source default volume have so far been decided using this
> algorithm:
>
> 1. Allow policy modules to decide the default volume.
> 2. If policy modules don't make a decision, use the current hw volume
> as the default volume.
> 3. If no hw volume is available, use 100%.
>
> This patch adds one more step:
>
> 1. Allow policy modules to decide the default volume.
> 2. If policy modules don't make a decision, use the current hw volume
> as the default volume.
> 3. If no hw volume is available, set the default volume based on the
> port default volume.
> 4. If there are no ports, use 100%.
>
> (A sidenote: I think step 2 could and should be removed now that we
> have per-port default volumes, but that change can be done later.)
>
> My motivation for adding per-port default volumes is that I want to
> disable hardware volume for a certain sound card, but doing so would
> result in 100% default volume, which is too high. Therefore, I will
> need to add support for configuring the default volume in the alsa
> mixer configuration files, and that in turn depends on having the
> per-port default volumes in the core. The alsa mixer patches will
> follow.
> ---
> src/pulsecore/device-port.c | 9 ++++++-
> src/pulsecore/device-port.h | 3 +++
> src/pulsecore/sink.c | 64 ++++++++++++++++++++++++++-------------------
> src/pulsecore/sink.h | 7 +++++
> src/pulsecore/source.c | 64 ++++++++++++++++++++++++++-------------------
> src/pulsecore/source.h | 7 +++++
> 6 files changed, 99 insertions(+), 55 deletions(-)
>
> diff --git a/src/pulsecore/device-port.c b/src/pulsecore/device-port.c
> index cfe2a80..227580d 100644
> --- a/src/pulsecore/device-port.c
> +++ b/src/pulsecore/device-port.c
> @@ -29,6 +29,7 @@ pa_device_port_new_data *pa_device_port_new_data_init(pa_device_port_new_data *d
>
> pa_zero(*data);
> data->available = PA_AVAILABLE_UNKNOWN;
> + data->default_volume = PA_VOLUME_NORM;
> return data;
> }
>
> @@ -58,6 +59,12 @@ void pa_device_port_new_data_set_direction(pa_device_port_new_data *data, pa_dir
> data->direction = direction;
> }
>
> +void pa_device_port_new_data_set_default_volume(pa_device_port_new_data *data, pa_volume_t volume) {
> + pa_assert(data);
> +
> + data->default_volume = volume;
> +}
> +
> void pa_device_port_new_data_done(pa_device_port_new_data *data) {
> pa_assert(data);
>
> @@ -124,8 +131,8 @@ pa_device_port *pa_device_port_new(pa_core *c, pa_device_port_new_data *data, si
> p->available = data->available;
> p->profiles = pa_hashmap_new(pa_idxset_string_hash_func, pa_idxset_string_compare_func);
> p->direction = data->direction;
> -
> p->latency_offset = 0;
> + p->default_volume = data->default_volume;
> p->proplist = pa_proplist_new();
>
> return p;
> diff --git a/src/pulsecore/device-port.h b/src/pulsecore/device-port.h
> index f35d07c..8ea49dd 100644
> --- a/src/pulsecore/device-port.h
> +++ b/src/pulsecore/device-port.h
> @@ -51,6 +51,7 @@ struct pa_device_port {
> pa_hashmap *profiles; /* Does not own the profiles */
> pa_direction_t direction;
> int64_t latency_offset;
> + pa_volume_t default_volume;
>
> /* .. followed by some implementation specific data */
> };
> @@ -65,6 +66,7 @@ typedef struct pa_device_port_new_data {
> char *description;
> pa_available_t available;
> pa_direction_t direction;
> + pa_volume_t default_volume;
> } pa_device_port_new_data;
>
> pa_device_port_new_data *pa_device_port_new_data_init(pa_device_port_new_data *data);
> @@ -72,6 +74,7 @@ void pa_device_port_new_data_set_name(pa_device_port_new_data *data, const char
> void pa_device_port_new_data_set_description(pa_device_port_new_data *data, const char *description);
> void pa_device_port_new_data_set_available(pa_device_port_new_data *data, pa_available_t available);
> void pa_device_port_new_data_set_direction(pa_device_port_new_data *data, pa_direction_t direction);
> +void pa_device_port_new_data_set_default_volume(pa_device_port_new_data *data, pa_volume_t volume);
> void pa_device_port_new_data_done(pa_device_port_new_data *data);
>
> pa_device_port *pa_device_port_new(pa_core *c, pa_device_port_new_data *data, size_t extra);
> diff --git a/src/pulsecore/sink.c b/src/pulsecore/sink.c
> index f29a9b7..3776a5a 100644
> --- a/src/pulsecore/sink.c
> +++ b/src/pulsecore/sink.c
> @@ -81,6 +81,7 @@ pa_sink_new_data* pa_sink_new_data_init(pa_sink_new_data *data) {
> pa_zero(*data);
> data->proplist = pa_proplist_new();
> data->ports = pa_hashmap_new_full(pa_idxset_string_hash_func, pa_idxset_string_compare_func, NULL, (pa_free_cb_t) pa_device_port_unref);
> + data->default_volume = PA_VOLUME_NORM;
>
> return data;
> }
> @@ -134,6 +135,12 @@ void pa_sink_new_data_set_port(pa_sink_new_data *data, const char *port) {
> data->active_port = pa_xstrdup(port);
> }
>
> +void pa_sink_new_data_set_default_volume(pa_sink_new_data *data, pa_volume_t volume) {
> + pa_assert(data);
> +
> + data->default_volume = volume;
> +}
> +
> void pa_sink_new_data_done(pa_sink_new_data *data) {
> pa_assert(data);
>
> @@ -172,6 +179,7 @@ pa_sink* pa_sink_new(
>
> pa_sink *s;
> const char *name;
> + pa_device_port *active_port = NULL;
> char st[PA_SAMPLE_SPEC_SNPRINT_MAX], cm[PA_CHANNEL_MAP_SNPRINT_MAX];
> pa_source_new_data source_data;
> const char *dn;
> @@ -211,18 +219,6 @@ pa_sink* pa_sink_new(
> pa_return_null_if_fail(pa_channel_map_valid(&data->channel_map));
> pa_return_null_if_fail(data->channel_map.channels == data->sample_spec.channels);
>
> - /* FIXME: There should probably be a general function for checking whether
> - * the sink volume is allowed to be set, like there is for sink inputs. */
> - pa_assert(!data->volume_is_set || !(flags & PA_SINK_SHARE_VOLUME_WITH_MASTER));
> -
> - if (!data->volume_is_set) {
> - pa_cvolume_reset(&data->volume, data->sample_spec.channels);
> - data->save_volume = false;
> - }
> -
> - pa_return_null_if_fail(pa_cvolume_valid(&data->volume));
> - pa_return_null_if_fail(pa_cvolume_compatible(&data->volume, &data->sample_spec));
> -
> if (!data->muted_is_set)
> data->muted = false;
>
> @@ -233,12 +229,35 @@ pa_sink* pa_sink_new(
> pa_device_init_icon(data->proplist, true);
> pa_device_init_intended_roles(data->proplist);
>
> - if (!data->active_port) {
> - pa_device_port *p = pa_device_port_find_best(data->ports);
> - if (p)
> - pa_sink_new_data_set_port(data, p->name);
> + if (data->active_port) {
> + active_port = pa_hashmap_get(data->ports, data->active_port);
> + if (!active_port)
> + pa_sink_new_data_set_port(data, NULL);
> + }
> +
> + if (!active_port) {
> + active_port = pa_device_port_find_best(data->ports);
> + if (active_port)
> + pa_sink_new_data_set_port(data, active_port->name);
> +
> + data->save_port = false;
> + }
> +
> + if (active_port)
> + pa_sink_new_data_set_default_volume(data, active_port->default_volume);
> +
> + /* FIXME: There should probably be a general function for checking whether
> + * the sink volume is allowed to be set, like there is for sink inputs. */
> + pa_assert(!data->volume_is_set || !(flags & PA_SINK_SHARE_VOLUME_WITH_MASTER));
> +
> + if (!data->volume_is_set) {
> + pa_cvolume_set(&data->volume, data->sample_spec.channels, data->default_volume);
> + data->save_volume = false;
> }
>
> + pa_return_null_if_fail(pa_cvolume_valid(&data->volume));
> + pa_return_null_if_fail(pa_cvolume_compatible(&data->volume, &data->sample_spec));
> +
> if (pa_hook_fire(&core->hooks[PA_CORE_HOOK_SINK_FIXATE], data) < 0) {
> pa_xfree(s);
> pa_namereg_unregister(core, name);
> @@ -297,17 +316,8 @@ pa_sink* pa_sink_new(
> s->ports = data->ports;
> data->ports = NULL;
>
> - s->active_port = NULL;
> - s->save_port = false;
> -
> - if (data->active_port)
> - if ((s->active_port = pa_hashmap_get(s->ports, data->active_port)))
> - s->save_port = data->save_port;
> -
> - /* Hopefully the active port has already been assigned in the previous call
> - to pa_device_port_find_best, but better safe than sorry */
> - if (!s->active_port)
> - s->active_port = pa_device_port_find_best(s->ports);
There seem to be a large move-around here w r t figuring out the active
port. IIRC, we needed to do it twice (both before and after the FIXATE
hook) for some corner case reason, but my memory is vague.
Anyhow, this move-around does not look immediately related to adding a
default volume. Could you elaborate?
> + s->active_port = active_port;
> + s->save_port = data->save_port;
>
> if (s->active_port)
> s->latency_offset = s->active_port->latency_offset;
> diff --git a/src/pulsecore/sink.h b/src/pulsecore/sink.h
> index ed0f9ee..cb32782 100644
> --- a/src/pulsecore/sink.h
> +++ b/src/pulsecore/sink.h
> @@ -356,6 +356,12 @@ typedef struct pa_sink_new_data {
> pa_cvolume volume;
> bool muted:1;
>
> + /* This needs to be set only if the sink has no ports and the default
> + * volume should be something else than PA_VOLUME_NORM. If the sink has any
> + * ports, then the sink default volume will be the default volume of the
> + * activated port. */
> + pa_volume_t default_volume;
> +
> bool sample_spec_is_set:1;
> bool channel_map_is_set:1;
> bool alternate_sample_rate_is_set:1;
> @@ -374,6 +380,7 @@ void pa_sink_new_data_set_name(pa_sink_new_data *data, const char *name);
> void pa_sink_new_data_set_sample_spec(pa_sink_new_data *data, const pa_sample_spec *spec);
> void pa_sink_new_data_set_channel_map(pa_sink_new_data *data, const pa_channel_map *map);
> void pa_sink_new_data_set_alternate_sample_rate(pa_sink_new_data *data, const uint32_t alternate_sample_rate);
> +void pa_sink_new_data_set_default_volume(pa_sink_new_data *data, pa_volume_t volume);
> void pa_sink_new_data_set_volume(pa_sink_new_data *data, const pa_cvolume *volume);
> void pa_sink_new_data_set_muted(pa_sink_new_data *data, bool mute);
> void pa_sink_new_data_set_port(pa_sink_new_data *data, const char *port);
> diff --git a/src/pulsecore/source.c b/src/pulsecore/source.c
> index 97f6cd9..b06e7fa 100644
> --- a/src/pulsecore/source.c
> +++ b/src/pulsecore/source.c
> @@ -72,6 +72,7 @@ pa_source_new_data* pa_source_new_data_init(pa_source_new_data *data) {
> pa_zero(*data);
> data->proplist = pa_proplist_new();
> data->ports = pa_hashmap_new_full(pa_idxset_string_hash_func, pa_idxset_string_compare_func, NULL, (pa_free_cb_t) pa_device_port_unref);
> + data->default_volume = PA_VOLUME_NORM;
>
> return data;
> }
> @@ -125,6 +126,12 @@ void pa_source_new_data_set_port(pa_source_new_data *data, const char *port) {
> data->active_port = pa_xstrdup(port);
> }
>
> +void pa_source_new_data_set_default_volume(pa_source_new_data *data, pa_volume_t volume) {
> + pa_assert(data);
> +
> + data->default_volume = volume;
> +}
> +
> void pa_source_new_data_done(pa_source_new_data *data) {
> pa_assert(data);
>
> @@ -161,6 +168,7 @@ pa_source* pa_source_new(
>
> pa_source *s;
> const char *name;
> + pa_device_port *active_port = NULL;
> char st[PA_SAMPLE_SPEC_SNPRINT_MAX], cm[PA_CHANNEL_MAP_SNPRINT_MAX];
> char *pt;
>
> @@ -198,18 +206,6 @@ pa_source* pa_source_new(
> pa_return_null_if_fail(pa_channel_map_valid(&data->channel_map));
> pa_return_null_if_fail(data->channel_map.channels == data->sample_spec.channels);
>
> - /* FIXME: There should probably be a general function for checking whether
> - * the source volume is allowed to be set, like there is for source outputs. */
> - pa_assert(!data->volume_is_set || !(flags & PA_SOURCE_SHARE_VOLUME_WITH_MASTER));
> -
> - if (!data->volume_is_set) {
> - pa_cvolume_reset(&data->volume, data->sample_spec.channels);
> - data->save_volume = false;
> - }
> -
> - pa_return_null_if_fail(pa_cvolume_valid(&data->volume));
> - pa_return_null_if_fail(pa_cvolume_compatible(&data->volume, &data->sample_spec));
> -
> if (!data->muted_is_set)
> data->muted = false;
>
> @@ -220,12 +216,35 @@ pa_source* pa_source_new(
> pa_device_init_icon(data->proplist, false);
> pa_device_init_intended_roles(data->proplist);
>
> - if (!data->active_port) {
> - pa_device_port *p = pa_device_port_find_best(data->ports);
> - if (p)
> - pa_source_new_data_set_port(data, p->name);
> + if (data->active_port) {
> + active_port = pa_hashmap_get(data->ports, data->active_port);
> + if (!active_port)
> + pa_source_new_data_set_port(data, NULL);
> + }
> +
> + if (!active_port) {
> + active_port = pa_device_port_find_best(data->ports);
> + if (active_port)
> + pa_source_new_data_set_port(data, active_port->name);
> +
> + data->save_port = false;
> + }
> +
> + if (active_port)
> + pa_source_new_data_set_default_volume(data, active_port->default_volume);
> +
> + /* FIXME: There should probably be a general function for checking whether
> + * the source volume is allowed to be set, like there is for source outputs. */
> + pa_assert(!data->volume_is_set || !(flags & PA_SOURCE_SHARE_VOLUME_WITH_MASTER));
> +
> + if (!data->volume_is_set) {
> + pa_cvolume_set(&data->volume, data->sample_spec.channels, data->default_volume);
> + data->save_volume = false;
> }
>
> + pa_return_null_if_fail(pa_cvolume_valid(&data->volume));
> + pa_return_null_if_fail(pa_cvolume_compatible(&data->volume, &data->sample_spec));
> +
> if (pa_hook_fire(&core->hooks[PA_CORE_HOOK_SOURCE_FIXATE], data) < 0) {
> pa_xfree(s);
> pa_namereg_unregister(core, name);
> @@ -285,17 +304,8 @@ pa_source* pa_source_new(
> s->ports = data->ports;
> data->ports = NULL;
>
> - s->active_port = NULL;
> - s->save_port = false;
> -
> - if (data->active_port)
> - if ((s->active_port = pa_hashmap_get(s->ports, data->active_port)))
> - s->save_port = data->save_port;
> -
> - /* Hopefully the active port has already been assigned in the previous call
> - to pa_device_port_find_best, but better safe than sorry */
> - if (!s->active_port)
> - s->active_port = pa_device_port_find_best(s->ports);
> + s->active_port = active_port;
> + s->save_port = data->save_port;
>
> if (s->active_port)
> s->latency_offset = s->active_port->latency_offset;
> diff --git a/src/pulsecore/source.h b/src/pulsecore/source.h
> index e7fc3e2..7555a9f 100644
> --- a/src/pulsecore/source.h
> +++ b/src/pulsecore/source.h
> @@ -293,6 +293,12 @@ typedef struct pa_source_new_data {
> pa_cvolume volume;
> bool muted:1;
>
> + /* This needs to be set only if the source has no ports and the default
> + * volume should be something else than PA_VOLUME_NORM. If the source has
> + * any ports, then the source default volume will be the default volume of
> + * the activated port. */
> + pa_volume_t default_volume;
> +
> bool volume_is_set:1;
> bool muted_is_set:1;
> bool sample_spec_is_set:1;
> @@ -311,6 +317,7 @@ void pa_source_new_data_set_name(pa_source_new_data *data, const char *name);
> void pa_source_new_data_set_sample_spec(pa_source_new_data *data, const pa_sample_spec *spec);
> void pa_source_new_data_set_channel_map(pa_source_new_data *data, const pa_channel_map *map);
> void pa_source_new_data_set_alternate_sample_rate(pa_source_new_data *data, const uint32_t alternate_sample_rate);
> +void pa_source_new_data_set_default_volume(pa_source_new_data *data, pa_volume_t volume);
> void pa_source_new_data_set_volume(pa_source_new_data *data, const pa_cvolume *volume);
> void pa_source_new_data_set_muted(pa_source_new_data *data, bool mute);
> void pa_source_new_data_set_port(pa_source_new_data *data, const char *port);
>
--
David Henningsson, Canonical Ltd.
https://launchpad.net/~diwic
More information about the pulseaudio-discuss
mailing list