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

[Colin Guthrie]

'Twas brillig, and Tanu Kaskinen at 20/07/11 19:18 did gyre and gimble:
> 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.

ACK. Good catch. I remember keeping this in mind and thinking "I must
make sure I get the order right", but clearly I didn't.

>> +
>> +        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.

ACK

>> 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.

Oh, crap. Sorry I mustn't have read the code properly initially and just
made some assumptions. Sorry and thanks for catching.

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

Ahh good point too.

>> 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.

Yup, I just gave up and made module-virtual-* set the flat volume flag
manually... Not IMO the most elegant solution but it'll have to do for
now as I can't really stomach it :D

Cheers

Col



-- 

Colin Guthrie
gmane(at)colin.guthr.ie
http://colin.guthr.ie/

Day Job:
  Tribalogic Limited [http://www.tribalogic.net/]
Open Source:
  Mageia Contributor [http://www.mageia.org/]
  PulseAudio Hacker [http://www.pulseaudio.org/]
  Trac Hacker [http://trac.edgewall.org/]