[pulseaudio-discuss] [PATCH 03/11] alsa: Reinitialise the mixer on port change.

[Tanu Kaskinen]

On Mon, 2011-07-18 at 11:36 +0100, Colin Guthrie wrote:
> This allows us to flip from software to hardware volume control as the port's
> mixer path dictates.
> ---
>  src/modules/alsa/alsa-sink.c                 |  114 +++++++++++++++-----------
>  src/modules/alsa/alsa-source.c               |  113 +++++++++++++++-----------
>  src/modules/echo-cancel/module-echo-cancel.c |    4 +-
>  src/modules/module-equalizer-sink.c          |    2 +-
>  src/modules/module-ladspa-sink.c             |    2 +-
>  src/modules/module-virtual-sink.c            |    5 +-
>  src/modules/module-virtual-source.c          |    5 +-
>  src/pulsecore/sink.c                         |   87 ++++++++++++++++++--
>  src/pulsecore/sink.h                         |    2 +
>  src/pulsecore/source.c                       |   87 ++++++++++++++++++--
>  src/pulsecore/source.h                       |    2 +
>  11 files changed, 305 insertions(+), 118 deletions(-)
> 
> diff --git a/src/modules/alsa/alsa-sink.c b/src/modules/alsa/alsa-sink.c
> index 0dd1840..cbf75b4 100644
> --- a/src/modules/alsa/alsa-sink.c
> +++ b/src/modules/alsa/alsa-sink.c
> @@ -130,7 +130,7 @@ struct userdata {
>      char *device_name;  /* name of the PCM device */
>      char *control_device; /* name of the control device */
>  
> -    pa_bool_t use_mmap:1, use_tsched:1;
> +    pa_bool_t use_mmap:1, use_tsched:1, sync_volume:1;
>  
>      pa_bool_t first, after_rewind;
>  
> @@ -1372,6 +1372,54 @@ static void sink_set_mute_cb(pa_sink *s) {
>      pa_alsa_path_set_mute(u->mixer_path, u->mixer_handle, s->muted);
>  }
>  
> +static void mixer_volume_init(struct userdata *u) {
> +    pa_assert(u);
> +
> +    if (!u->mixer_path->has_volume) {
> +        pa_sink_set_get_volume_callback(u->sink, NULL);
> +        pa_sink_set_set_volume_callback(u->sink, NULL);
> +        pa_sink_set_write_volume_callback(u->sink, NULL);

I think this can crash due to the assertion in
pa_sink_set_set_volume_callback(). The write_volume callback should be
set to NULL before setting the set_volume callback to NULL.

Same for source.

> +
> +        pa_log_info("Driver does not support hardware volume control, falling back to software volume control.");
> +    } else {
> +        pa_sink_set_get_volume_callback(u->sink, sink_get_volume_cb);
> +        pa_sink_set_set_volume_callback(u->sink, sink_set_volume_cb);
> +        pa_sink_set_write_volume_callback(u->sink, NULL);

I'd rather set the write_volume callback only once, after we know what
we should set it to. The reason is that this can probably eventually
trigger a dbus signal, and if the callback is set again a few lines
later, there will be another signal.

Same for source.

> diff --git a/src/modules/module-virtual-sink.c b/src/modules/module-virtual-sink.c
> index a880df4..5aa6e4f 100644
> --- a/src/modules/module-virtual-sink.c
> +++ b/src/modules/module-virtual-sink.c
> @@ -554,8 +554,7 @@ int pa__init(pa_module*m) {
>      }
>  
>      u->sink = pa_sink_new(m->core, &sink_data, (master->flags & (PA_SINK_LATENCY|PA_SINK_DYNAMIC_LATENCY))
> -                                               | (use_volume_sharing ? PA_SINK_SHARE_VOLUME_WITH_MASTER : 0)
> -                                               | (force_flat_volume ? PA_SINK_FLAT_VOLUME : 0));
> +                                               | (use_volume_sharing ? PA_SINK_SHARE_VOLUME_WITH_MASTER : 0));
>      pa_sink_new_data_done(&sink_data);
>  
>      if (!u->sink) {
> @@ -569,6 +568,8 @@ int pa__init(pa_module*m) {
>      u->sink->request_rewind = sink_request_rewind_cb;
>      pa_sink_set_set_volume_callback(u->sink, use_volume_sharing ? NULL : sink_set_volume_cb);
>      pa_sink_set_set_mute_callback(u->sink, sink_set_mute_cb);
> +    if (force_flat_volume)
> +        pa_sink_enable_flat_volume(u->sink, TRUE);

Given the implementation of pa_sink_enable_flat_volume, I don't think
this works. pa_sink_enable_flat_volume only enables flat volume if it's
enabled in the daemon configuration, and the point of forcing flat
volume is to enable flat volume even if it's disabled in the daemon
configuration.

Also, if volume sharing is not used, then decibel volume should be
explicitly enabled because the set_volume callback is set.

Same for virtual source.

> diff --git a/src/pulsecore/sink.h b/src/pulsecore/sink.h
> index c725e88..e6367bc 100644
> --- a/src/pulsecore/sink.h
> +++ b/src/pulsecore/sink.h
> @@ -347,6 +347,8 @@ void pa_sink_set_set_volume_callback(pa_sink *s, pa_sink_cb_t cb);
>  void pa_sink_set_write_volume_callback(pa_sink *s, pa_sink_cb_t cb);
>  void pa_sink_set_get_mute_callback(pa_sink *s, pa_sink_cb_t cb);
>  void pa_sink_set_set_mute_callback(pa_sink *s, pa_sink_cb_t cb);
> +void pa_sink_enable_decibel_volume(pa_sink *s, pa_bool_t enable);
> +void pa_sink_enable_flat_volume(pa_sink *s, pa_bool_t enable);

Just a reminder: depending on how you solve the flat volume forcing
problem, pa_sink_enable_flat_volume() may become unused outside sink.c.
If that happens, I'd move the function to sink.c and make it static.

Same for source.

-- 
Tanu