On 2015-04-17 22:26, Tanu Kaskinen wrote:
> If two ports have different default volumes, and module-device-restore
> doesn't have an opinion on the port volume, we should change the
> sink/source volume to the new port's default volume when switching
> ports, instead of keeping the volume at whatever it happened to be
> when the old port was active.
>
> This introduces pa_sink/source_port_changed_hook_data, because I
> wanted to call pa_sink/source_set_volume() only once in
> pa_sink/source_set_port(), instead of calling it also in
> module-device-restore's hook callback. The extended hook data allows
> module-device-restore to communicate to the core that the volume
> should be set to a non-default value.
Now that pa_device_port has a default volume, then module-device-restore
should just set it for all ports at the card_new + PA_HOOK_EARLY
callback. There is no need for module-device-restore to do anything at
port_changed anymore; and thus no need to add the
pa_*_port_changed_hook_data struct either, unless I'm missing something.
There's also the point that now volume and mute are handled differently,
which I'm not really happy with.
> ---
> src/modules/dbus/iface-device.c | 4 ++--
> src/modules/module-device-restore.c | 32 ++++++++++++++------------------
> src/pulsecore/sink.c | 8 +++++++-
> src/pulsecore/sink.h | 10 ++++++++++
> src/pulsecore/source.c | 8 +++++++-
> src/pulsecore/source.h | 10 ++++++++++
> 6 files changed, 50 insertions(+), 22 deletions(-)
>
> diff --git a/src/modules/dbus/iface-device.c b/src/modules/dbus/iface-device.c
> index 2c370a8..911890d 100644
> --- a/src/modules/dbus/iface-device.c
> +++ b/src/modules/dbus/iface-device.c
> @@ -1190,8 +1190,8 @@ static pa_hook_result_t port_changed_cb(void *hook_data, void *call_data, void *
> DBusMessage *signal_msg = NULL;
> pa_device_port *new_active_port = NULL;
>
> - if ((d->type == PA_DEVICE_TYPE_SINK && d->sink != call_data) ||
> - (d->type == PA_DEVICE_TYPE_SOURCE && d->source != call_data))
> + if ((d->type == PA_DEVICE_TYPE_SINK && d->sink != ((pa_sink_port_changed_hook_data *) call_data)->sink) ||
> + (d->type == PA_DEVICE_TYPE_SOURCE && d->source != ((pa_source_port_changed_hook_data *) call_data)->source))
> return PA_HOOK_OK;
>
> new_active_port = (d->type == PA_DEVICE_TYPE_SINK) ? d->sink->active_port : d->source->active_port;
> diff --git a/src/modules/module-device-restore.c b/src/modules/module-device-restore.c
> index 8d7b34b..ced2a14 100644
> --- a/src/modules/module-device-restore.c
> +++ b/src/modules/module-device-restore.c
> @@ -798,28 +798,26 @@ static pa_hook_result_t sink_fixate_hook_callback(pa_core *c, pa_sink_new_data *
> return PA_HOOK_OK;
> }
>
> -static pa_hook_result_t sink_port_hook_callback(pa_core *c, pa_sink *sink, struct userdata *u) {
> +static pa_hook_result_t sink_port_hook_callback(pa_core *c, pa_sink_port_changed_hook_data *data, struct userdata *u) {
> + pa_sink *sink;
> char *name;
> struct perportentry *e;
>
> pa_assert(c);
> - pa_assert(sink);
> + pa_assert(data);
> pa_assert(u);
> pa_assert(u->restore_volume || u->restore_muted);
>
> + sink = data->sink;
> name = pa_sprintf_malloc("sink:%s", sink->name);
>
> if ((e = perportentry_read(u, name, (sink->active_port ? sink->active_port->name : NULL)))) {
>
> if (u->restore_volume && e->volume_valid) {
> - pa_cvolume v;
> -
> pa_log_info("Restoring volume for sink %s.", sink->name);
> - v = e->volume;
> - pa_cvolume_remap(&v, &e->channel_map, &sink->channel_map);
> - pa_sink_set_volume(sink, &v, true, false);
> -
> - sink->save_volume = true;
> + data->volume = e->volume;
> + pa_cvolume_remap(&data->volume, &e->channel_map, &sink->channel_map);
> + data->save_volume = true;
> }
>
> if (u->restore_muted && e->muted_valid) {
> @@ -940,28 +938,26 @@ static pa_hook_result_t source_fixate_hook_callback(pa_core *c, pa_source_new_da
> return PA_HOOK_OK;
> }
>
> -static pa_hook_result_t source_port_hook_callback(pa_core *c, pa_source *source, struct userdata *u) {
> +static pa_hook_result_t source_port_hook_callback(pa_core *c, pa_source_port_changed_hook_data *data, struct userdata *u) {
> + pa_source *source;
> char *name;
> struct perportentry *e;
>
> pa_assert(c);
> - pa_assert(source);
> + pa_assert(data);
> pa_assert(u);
> pa_assert(u->restore_volume || u->restore_muted);
>
> + source = data->source;
> name = pa_sprintf_malloc("source:%s", source->name);
>
> if ((e = perportentry_read(u, name, (source->active_port ? source->active_port->name : NULL)))) {
>
> if (u->restore_volume && e->volume_valid) {
> - pa_cvolume v;
> -
> pa_log_info("Restoring volume for source %s.", source->name);
> - v = e->volume;
> - pa_cvolume_remap(&v, &e->channel_map, &source->channel_map);
> - pa_source_set_volume(source, &v, true, false);
> -
> - source->save_volume = true;
> + data->volume = e->volume;
> + pa_cvolume_remap(&data->volume, &e->channel_map, &source->channel_map);
> + data->save_volume = true;
> }
>
> if (u->restore_muted && e->muted_valid) {
> diff --git a/src/pulsecore/sink.c b/src/pulsecore/sink.c
> index 3776a5a..652401d 100644
> --- a/src/pulsecore/sink.c
> +++ b/src/pulsecore/sink.c
> @@ -3310,6 +3310,7 @@ size_t pa_sink_get_max_request(pa_sink *s) {
> int pa_sink_set_port(pa_sink *s, const char *name, bool save) {
> pa_device_port *port;
> int ret;
> + pa_sink_port_changed_hook_data hook_data;
>
> pa_sink_assert_ref(s);
> pa_assert_ctl_context();
> @@ -3350,7 +3351,12 @@ int pa_sink_set_port(pa_sink *s, const char *name, bool save) {
>
> pa_sink_set_latency_offset(s, s->active_port->latency_offset);
>
> - pa_hook_fire(&s->core->hooks[PA_CORE_HOOK_SINK_PORT_CHANGED], s);
> + hook_data.sink = s;
> + pa_cvolume_set(&hook_data.volume, s->sample_spec.channels, port->default_volume);
> + hook_data.save_volume = false;
> + pa_hook_fire(&s->core->hooks[PA_CORE_HOOK_SINK_PORT_CHANGED], &hook_data);
> +
> + pa_sink_set_volume(s, &hook_data.volume, true, hook_data.save_volume);
>
> return 0;
> }
> diff --git a/src/pulsecore/sink.h b/src/pulsecore/sink.h
> index cb32782..4150c5a3 100644
> --- a/src/pulsecore/sink.h
> +++ b/src/pulsecore/sink.h
> @@ -337,6 +337,16 @@ typedef enum pa_sink_message {
> PA_SINK_MESSAGE_MAX
> } pa_sink_message_t;
>
> +typedef struct {
> + pa_sink *sink;
> +
> + /* This will be the new volume for the sink. If the hook callback wants to
> + * set the sink volume, it should do that by setting these variables
> + * instead of calling pa_sink_set_volume(). */
> + pa_cvolume volume;
> + bool save_volume;
> +} pa_sink_port_changed_hook_data;
> +
> typedef struct pa_sink_new_data {
> pa_suspend_cause_t suspend_cause;
>
> diff --git a/src/pulsecore/source.c b/src/pulsecore/source.c
> index b06e7fa..17dd9a1 100644
> --- a/src/pulsecore/source.c
> +++ b/src/pulsecore/source.c
> @@ -2589,6 +2589,7 @@ size_t pa_source_get_max_rewind(pa_source *s) {
> int pa_source_set_port(pa_source *s, const char *name, bool save) {
> pa_device_port *port;
> int ret;
> + pa_source_port_changed_hook_data hook_data;
>
> pa_source_assert_ref(s);
> pa_assert_ctl_context();
> @@ -2627,7 +2628,12 @@ int pa_source_set_port(pa_source *s, const char *name, bool save) {
> s->active_port = port;
> s->save_port = save;
>
> - pa_hook_fire(&s->core->hooks[PA_CORE_HOOK_SOURCE_PORT_CHANGED], s);
> + hook_data.source = s;
> + pa_cvolume_set(&hook_data.volume, s->sample_spec.channels, port->default_volume);
> + hook_data.save_volume = false;
> + pa_hook_fire(&s->core->hooks[PA_CORE_HOOK_SOURCE_PORT_CHANGED], &hook_data);
> +
> + pa_source_set_volume(s, &hook_data.volume, true, hook_data.save_volume);
>
> return 0;
> }
> diff --git a/src/pulsecore/source.h b/src/pulsecore/source.h
> index 7555a9f..b6e9d0e 100644
> --- a/src/pulsecore/source.h
> +++ b/src/pulsecore/source.h
> @@ -274,6 +274,16 @@ typedef enum pa_source_message {
> PA_SOURCE_MESSAGE_MAX
> } pa_source_message_t;
>
> +typedef struct {
> + pa_source *source;
> +
> + /* This will be the new volume for the source. If the hook callback wants
> + * to set the source volume, it should do that by setting these variables
> + * instead of calling pa_source_set_volume(). */
> + pa_cvolume volume;
> + bool save_volume;
> +} pa_source_port_changed_hook_data;
> +
> typedef struct pa_source_new_data {
> pa_suspend_cause_t suspend_cause;
>
>
--
David Henningsson, Canonical Ltd.
https://launchpad.net/~diwic
