[pulseaudio-discuss] [PATCH v3 1/6] core: Add default volume to ports
Tanu Kaskinen
tanu.kaskinen at linux.intel.com
Thu Apr 30 02:34:08 PDT 2015
On Thu, 2015-04-30 at 10:44 +0200, David Henningsson wrote:
>
> 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.
I'm pretty sure you introduced this duplication just in case some module
does something stupid. That is, there was no documented corner case that
would have required the duplication.
> Anyhow, this move-around does not look immediately related to adding a
> default volume. Could you elaborate?
It is related. I probably should have documented the reason in the
commit message... The initial port decision has to be finalized before
deciding the initial sink volume, because the sink volume depends on the
port default volume.
--
Tanu
More information about the pulseaudio-discuss
mailing list