[pulseaudio-discuss] [PATCH v3 5/6] alsa-mixer: Add "default-volume" path option

[Tanu Kaskinen]

On Thu, 2015-04-30 at 11:04 +0200, David Henningsson wrote:
> 
> On 2015-04-27 13:34, Tanu Kaskinen wrote:
> > This allows customizing the default volume on a per-path basis.
> >
> > The default volume for analog outputs is set to 30%, because that's
> > less likely to be too loud than the "default default" volume, which
> > is 100%. Digital outputs typically have another volume control down
> > the line, so for those it makes sense to keep the default at 100%.
> > ---
> >   src/modules/alsa/alsa-mixer.c                          | 4 ++++
> >   src/modules/alsa/alsa-mixer.h                          | 1 +
> >   src/modules/alsa/mixer/paths/analog-output.conf.common | 7 +++++++
> >   3 files changed, 12 insertions(+)
> >
> > diff --git a/src/modules/alsa/alsa-mixer.c b/src/modules/alsa/alsa-mixer.c
> > index 2314612..e921f8b 100644
> > --- a/src/modules/alsa/alsa-mixer.c
> > +++ b/src/modules/alsa/alsa-mixer.c
> > @@ -2438,6 +2438,7 @@ pa_alsa_path* pa_alsa_path_new(const char *paths_dir, const char *fname, pa_alsa
> >           { "description",         pa_config_parse_string,            NULL, "General" },
> >           { "mute-during-activation", pa_config_parse_bool,           NULL, "General" },
> >           { "eld-device",          pa_config_parse_int,               NULL, "General" },
> > +        { "default-volume",      pa_config_parse_volume,            NULL, "General" },
> >
> >           /* [Option ...] */
> >           { "priority",            option_parse_priority,             NULL, NULL },
> > @@ -2471,12 +2472,14 @@ pa_alsa_path* pa_alsa_path_new(const char *paths_dir, const char *fname, pa_alsa
> >       p->proplist = pa_proplist_new();
> >       p->direction = direction;
> >       p->eld_device = -1;
> > +    p->default_volume = PA_VOLUME_NORM;
> >
> >       items[0].data = &p->priority;
> >       items[1].data = &p->description_key;
> >       items[2].data = &p->description;
> >       items[3].data = &mute_during_activation;
> >       items[4].data = &p->eld_device;
> > +    items[5].data = &p->default_volume;
> >
> >       if (!paths_dir)
> >           paths_dir = get_default_paths_dir();
> > @@ -4647,6 +4650,7 @@ static pa_device_port* device_port_alsa_init(pa_hashmap *ports, /* card ports */
> >           pa_device_port_new_data_set_name(&port_data, name);
> >           pa_device_port_new_data_set_description(&port_data, description);
> >           pa_device_port_new_data_set_direction(&port_data, path->direction == PA_ALSA_DIRECTION_OUTPUT ? PA_DIRECTION_OUTPUT : PA_DIRECTION_INPUT);
> > +        pa_device_port_new_data_set_default_volume(&port_data, path->default_volume);
> >
> >           p = pa_device_port_new(core, &port_data, sizeof(pa_alsa_port_data));
> >           pa_device_port_new_data_done(&port_data);
> > diff --git a/src/modules/alsa/alsa-mixer.h b/src/modules/alsa/alsa-mixer.h
> > index ec39fab..1141f61 100644
> > --- a/src/modules/alsa/alsa-mixer.h
> > +++ b/src/modules/alsa/alsa-mixer.h
> > @@ -182,6 +182,7 @@ struct pa_alsa_path {
> >       char *description;
> >       unsigned priority;
> >       int eld_device;
> > +    pa_volume_t default_volume;
> >       pa_proplist *proplist;
> >
> >       bool probed:1;
> > diff --git a/src/modules/alsa/mixer/paths/analog-output.conf.common b/src/modules/alsa/mixer/paths/analog-output.conf.common
> > index 17b4527..e461a53 100644
> > --- a/src/modules/alsa/mixer/paths/analog-output.conf.common
> > +++ b/src/modules/alsa/mixer/paths/analog-output.conf.common
> > @@ -66,6 +66,10 @@
> >   ;                                        # cases this can increase such noises. Default: no.
> >   ; eld-device = ...                       # If this is an HDMI port, here's where to specify the device number for the ELD mixer
> >   ;                                        # control. The default is to not make use of ELD information.
> > +; default-volume = ...                   # The default volume for cases where no other source of good default volume is
> > +;                                        # available. Sources of good default volume include policy modules and the current
> > +;                                        # hardware volume, so if the path has hardware volume control available, then this
> > +;                                        # option doesn't have effect. The volume is given as a percentage, e.g. "30%".
> >   ;
> >   ; [Properties]                           # Property list for this path. The list is merged into the port property list.
> >   ; <key> = <value>                        # Each property is defined on its own line.
> > @@ -123,6 +127,9 @@
> >   ; state.plugged = yes | no | unknown   # Normally a plugged jack would mean the port becomes available, and an unplugged means it's
> >   ; state.unplugged = yes | no | unknown # unavailable, but the port status can be overridden by specifying state.plugged and/or state.unplugged.
> >
> > +[General]
> > +default-volume = 30%
> > +
> 
> Acked if you remove this part, which IMO requires some further bikeshedding.

This can't be just removed, doing so would cause a regression. Consider
this scenario: When a laptop is booted for the first time, there's no
saved volume for any ports. Let's assume headphones are not plugged in.
The speaker volume gets initialized based on the default volume alsa
happens to have, which is ok. When headphones are plugged in, the old
way of doing things was to use the same volume for the headphones as
what speakers had, which doesn't really make sense, but it's still
better than applying these patches without setting the analog default
volume to 30%. If the analog default volume is 100%, these patches will
make the initial headphone volume to be 100%, which is not good.

You said that lineout should have 100% default volume, but Alexander
said that lineout is sometimes used with headphones. So, the default
lineout volume will anyway be sometimes non-optimal, and I think it's
worse to have 100% for headphones than 30% for use cases where 100%
would be better, so I think this patch doesn't need changes.

-- 
Tanu