'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/]
