Thanks for working on this! Sorry for slow review, I hope I'll be much
quicker to comment on subsequent iterations.
On Mon, 2018-11-05 at 09:47 +0800, Hui Wang wrote:
> And don't move the stream in the module-stream-restore anymore,
> And the preferred_sink is only set when user is calling the
> move_to() and the module-stream-restore maintains the saving and
> deleting of preferred_sink.
>
> If the target of move_to() is default_sink, preferred_sink will be
> cleared and the entry->device will be cleared too from database.
Can you split this so that the first patch only changes save_sink to
preferred_sink, without any changes in behaviour? That is, put the
"don't move the stream in the module-stream-restore" and "if the target
of move_to() is default_sink" logic into separate patches.
Also replace tabs with spaces.
> Signed-off-by: Hui Wang <hui.wang@xxxxxxxxxxxxx>
> ---
> src/modules/module-device-manager.c | 2 +-
> src/modules/module-intended-roles.c | 2 +-
> src/modules/module-stream-restore.c | 47 +++++++++++++++++---------
> src/modules/module-switch-on-connect.c | 2 +-
> src/pulsecore/sink-input.c | 25 +++++++++++---
> src/pulsecore/sink-input.h | 7 ++--
> 6 files changed, 59 insertions(+), 26 deletions(-)
>
> diff --git a/src/modules/module-device-manager.c b/src/modules/module-device-manager.c
> index 15fdaaaa2..36e71584e 100644
> --- a/src/modules/module-device-manager.c
> +++ b/src/modules/module-device-manager.c
> @@ -657,7 +657,7 @@ static void route_sink_input(struct userdata *u, pa_sink_input *si) {
> pa_assert(u->do_routing);
>
> /* Don't override user or application routing requests. */
> - if (si->save_sink || si->sink_requested_by_application)
> + if (si->preferred_sink != NULL || si->sink_requested_by_application)
Just checking whether si->preferred_sink is set isn't enough - you need
to also check that the input is currently routed to the preferred sink.
Maybe we should have some helper, since this is a common thing to check
- perhaps pa_sink_input_is_routed_to_preferred_sink(). That's quite a
long name, though, so it's not that much better than just using
pa_safe_streq(si->sink->name, si->preferred_sink). I think the
pa_safe_streq() approach is good enough.
> return;
>
> /* Skip this if it is already in the process of being moved anyway */
> diff --git a/src/modules/module-intended-roles.c b/src/modules/module-intended-roles.c
> index adee51c20..98e4c241f 100644
> --- a/src/modules/module-intended-roles.c
> +++ b/src/modules/module-intended-roles.c
> @@ -175,7 +175,7 @@ static pa_hook_result_t sink_put_hook_callback(pa_core *c, pa_sink *sink, struct
> if (si->sink == sink)
> continue;
>
> - if (si->save_sink)
> + if (si->preferred_sink != NULL)
pa_safe_streq(si->sink->name, si->preferred_sink)
> continue;
>
> /* Skip this if it is already in the process of being moved
> diff --git a/src/modules/module-stream-restore.c b/src/modules/module-stream-restore.c
> index 228e9e447..276957c25 100644
> --- a/src/modules/module-stream-restore.c
> +++ b/src/modules/module-stream-restore.c
> @@ -1311,19 +1311,29 @@ static void subscribe_callback(pa_core *c, pa_subscription_event_type_t t, uint3
> mute_updated = !created_new_entry && (!old->muted_valid || entry->muted != old->muted);
> }
>
> - if (sink_input->save_sink) {
> + if (sink_input->preferred_sink != NULL || !created_new_entry) {
> pa_xfree(entry->device);
> - entry->device = pa_xstrdup(sink_input->sink->name);
> - entry->device_valid = true;
>
> - device_updated = !created_new_entry && (!old->device_valid || !pa_streq(entry->device, old->device));
> - if (sink_input->sink->card) {
> + if (sink_input->preferred_sink != NULL) {
> + entry->device = pa_xstrdup(sink_input->preferred_sink);
> + entry->device_valid = true;
> + } else {
> + entry->device = NULL;
> + entry->device_valid = false;
> + }
> +
> + device_updated = !created_new_entry && (!old->device_valid || !entry->device_valid || !pa_streq(entry->device, old->device));
This sets device_updated to true if both old->device_valid and entry-
>device_valid are false. device_updated should be set to false in that
case.
> +
> + if (entry->device_valid == false) {
> + pa_xfree(entry->card);
> + entry->card = NULL;
> + entry->card_valid = false;
> + } else if (sink_input->sink->card) {
> pa_xfree(entry->card);
> entry->card = pa_xstrdup(sink_input->sink->card->name);
> entry->card_valid = true;
> }
> }
> -
> } else {
> pa_source_output *source_output;
>
> @@ -1641,7 +1651,7 @@ static pa_hook_result_t sink_put_hook_callback(pa_core *c, pa_sink *sink, struct
> if (si->sink == sink)
> continue;
>
> - if (si->save_sink)
> + if (si->preferred_sink != NULL)
> continue;
The purpose of this hook callback is to move streams whose preferred
sink is the one that just got created. Such streams have si-
>preferred_sink set to a non-NULL value, so this if condition is not
good.
With the old code si->save_sink indicated that the stream was already
routed to the preferred sink. To replicate that you would have to check
whether si->sink->name matches si->preferred_sink, but I don't think
that's very useful. This condition can be simply dropped. If the stream
is already correctly routed, then pa_sink_input_move_to() will do
nothing.
>
> /* Skip this if it is already in the process of being moved
> @@ -1665,7 +1675,7 @@ static pa_hook_result_t sink_put_hook_callback(pa_core *c, pa_sink *sink, struct
>
> if ((e = entry_read(u, name))) {
> if (e->device_valid && pa_streq(e->device, sink->name))
> - pa_sink_input_move_to(si, sink, true);
> + si->preferred_sink = pa_xstrdup(sink->name);
I don't understand what this change is trying to achieve. The database
should always be in sync with with the preferred_sink variable of all
sink inputs, so it's not possible that device_valid is true while si-
>preferred_sink is NULL. If the goal is to avoid moving streams in
module-stream-restore, this move has to be done by the core when a new
sink is created, but you didn't add such code (and when that code is
added, it should be in a separate patch). At this point I don't think
any changes are needed here.
>
> entry_free(e);
> }
> @@ -1764,8 +1774,10 @@ static pa_hook_result_t sink_unlink_hook_callback(pa_core *c, pa_sink *sink, str
>
> if ((d = pa_namereg_get(c, e->device, PA_NAMEREG_SINK)) &&
> d != sink &&
> - PA_SINK_IS_LINKED(d->state))
> - pa_sink_input_move_to(si, d, true);
> + PA_SINK_IS_LINKED(d->state)) {
> + pa_xfree(si->preferred_sink);
> + si->preferred_sink = pa_xstrdup(d->name);
> + }
Same as above: the database is always in sync with si->preferred_sink,
so si->preferred_sink doesn't need updating. Also this patch should
keep the pa_sink_input_move_to() call. A later patch should add code to
the core so that streams are rescued to their preferred sink when their
current sink goes away. At that point his whole unlink callback can be
removed. For now this code needs no changing.
> }
>
> entry_free(e);
> @@ -1942,12 +1954,13 @@ static void entry_apply(struct userdata *u, const char *name, struct entry *e) {
>
> if (u->restore_device) {
> if (!e->device_valid) {
> - if (si->save_sink) {
> + if (si->preferred_sink != NULL) {
> pa_log_info("Ensuring device is not saved for stream %s.", name);
> /* If the device is not valid we should make sure the
> save flag is cleared as the user may have specifically
> removed the sink element from the rule. */
> - si->save_sink = false;
> + pa_xfree(si->preferred_sink);
> + si->preferred_sink = NULL;
I think we should add a pa_sink_input_set_preferred_sink() function.
Modifying the pa_sink_input struct outside sink-input.c isn't nice from
encapsulation point of view. pa_sink_input_set_preferred_sink() can
also call pa_subscription_post(), so the pa_subscription_post() call
below can be removed.
pa_sink_input_set_preferred_sink() should also move the stream to the
current default sink, but it's probably best to do that in a later
patch so that the first patch doesn't cause any changes in behaviour.
> /* This is cheating a bit. The sink input itself has not changed
> but the rules governing its routing have, so we fire this event
> such that other routing modules (e.g. module-device-manager)
> @@ -1956,7 +1969,8 @@ static void entry_apply(struct userdata *u, const char *name, struct entry *e) {
> }
> } else if ((s = pa_namereg_get(u->core, e->device, PA_NAMEREG_SINK))) {
> pa_log_info("Restoring device for stream %s.", name);
> - pa_sink_input_move_to(si, s, true);
> + pa_xfree(si->preferred_sink);
> + si->preferred_sink = pa_xstrdup(s->name);
This should also be replaced with a call to
pa_sink_input_set_preferred_sink(), and
pa_sink_input_set_preferred_sink() should move the stream to the new
preferred sink. (In this case the move should be done already in the
first patch, because that matches the old behaviour.)
> }
> }
> }
> @@ -2176,9 +2190,10 @@ static int extension_cb(pa_native_protocol *p, pa_module *m, pa_native_connectio
>
> entry->muted = muted;
> entry->muted_valid = true;
> -
> - entry->device = pa_xstrdup(device);
> - entry->device_valid = device && !!entry->device[0];
> + if (device && !pa_streq(device, m->core->default_sink->name) && !pa_streq(device, m->core->default_source->name)) {
> + entry->device = pa_xstrdup(device);
> + entry->device_valid = device && !!entry->device[0];
> + }
What's the goal here? The client tries to change an entry in the
stream-restore database, why should that change be ignored if the
current default sink happens to be the same as the new device? Maybe
you intended to set entry->device to NULL in this case. But I don't
think that's necessary either - if the client wants to unset the
device, it can just give NULL as the device name. I don't think you
need to change anything here.
By the way, m->core->default_sink can be NULL, so that would have to be
checked if this code was kept.
>
> if (entry->device_valid && !pa_namereg_is_valid_name(entry->device)) {
> entry_free(entry);
> diff --git a/src/modules/module-switch-on-connect.c b/src/modules/module-switch-on-connect.c
> index ebdd6aad0..faa0f289f 100644
> --- a/src/modules/module-switch-on-connect.c
> +++ b/src/modules/module-switch-on-connect.c
> @@ -112,7 +112,7 @@ static pa_hook_result_t sink_put_hook_callback(pa_core *c, pa_sink *sink, void*
> }
>
> PA_IDXSET_FOREACH(i, old_default_sink->inputs, idx) {
> - if (i->save_sink || !PA_SINK_INPUT_IS_LINKED(i->state))
> + if (i->preferred_sink != NULL || !PA_SINK_INPUT_IS_LINKED(i->state))
pa_safe_streq(i->sink->name, i->preferred_sink)
> continue;
>
> if (pa_sink_input_move_to(i, sink, false) < 0)
> diff --git a/src/pulsecore/sink-input.c b/src/pulsecore/sink-input.c
> index 312ec4a97..1031051a7 100644
> --- a/src/pulsecore/sink-input.c
> +++ b/src/pulsecore/sink-input.c
> @@ -190,7 +190,8 @@ bool pa_sink_input_new_data_set_sink(pa_sink_input_new_data *data, pa_sink *s, b
> if (!data->req_formats) {
> /* We're not working with the extended API */
> data->sink = s;
> - data->save_sink = save;
> + if (save)
> + data->preferred_sink = pa_xstrdup(s->name);
> data->sink_requested_by_application = requested_by_application;
> } else {
> /* Extended API: let's see if this sink supports the formats the client can provide */
> @@ -199,7 +200,8 @@ bool pa_sink_input_new_data_set_sink(pa_sink_input_new_data *data, pa_sink *s, b
> if (formats && !pa_idxset_isempty(formats)) {
> /* Sink supports at least one of the requested formats */
> data->sink = s;
> - data->save_sink = save;
> + if (save)
> + data->preferred_sink = pa_xstrdup(s->name);
> data->sink_requested_by_application = requested_by_application;
> if (data->nego_formats)
> pa_idxset_free(data->nego_formats, (pa_free_cb_t) pa_format_info_free);
> @@ -226,7 +228,7 @@ bool pa_sink_input_new_data_set_formats(pa_sink_input_new_data *data, pa_idxset
>
> if (data->sink) {
> /* Trigger format negotiation */
> - return pa_sink_input_new_data_set_sink(data, data->sink, data->save_sink, data->sink_requested_by_application);
> + return pa_sink_input_new_data_set_sink(data, data->sink, false, data->sink_requested_by_application);
Rather than unconditionally setting save to false, we need to check if
data->preferred_sink was set previously.
> }
>
> return true;
> @@ -519,7 +521,7 @@ int pa_sink_input_new(
> pa_cvolume_reset(&i->real_ratio, i->sample_spec.channels);
> i->volume_writable = data->volume_writable;
> i->save_volume = data->save_volume;
> - i->save_sink = data->save_sink;
> + i->preferred_sink = pa_xstrdup(data->preferred_sink);
> i->save_muted = data->save_muted;
>
> i->muted = data->muted;
> @@ -777,6 +779,9 @@ static void sink_input_free(pa_object *o) {
> if (i->volume_factor_sink_items)
> pa_hashmap_free(i->volume_factor_sink_items);
>
> + if (i->preferred_sink)
> + pa_xfree(i->preferred_sink);
> +
> pa_xfree(i->driver);
> pa_xfree(i);
> }
> @@ -1914,7 +1919,17 @@ int pa_sink_input_finish_move(pa_sink_input *i, pa_sink *dest, bool save) {
> i->moving(i, dest);
>
> i->sink = dest;
> - i->save_sink = save;
> +
> + /* save == true, means user is calling the move_to() and want to
> + save the preferred_sinke if the sink is not default_sink */
> + if (save) {
> + pa_xfree(i->preferred_sink);
> + if (dest == dest->core->default_sink)
> + i->preferred_sink = NULL;
> + else
> + i->preferred_sink = pa_xstrdup(dest->name);
> + }
> +
> pa_idxset_put(dest->inputs, pa_sink_input_ref(i), NULL);
>
> PA_HASHMAP_FOREACH(v, i->volume_factor_sink_items, state)
> diff --git a/src/pulsecore/sink-input.h b/src/pulsecore/sink-input.h
> index 9e90291ca..537cec9a1 100644
> --- a/src/pulsecore/sink-input.h
> +++ b/src/pulsecore/sink-input.h
> @@ -123,7 +123,8 @@ struct pa_sink_input {
> * set is worth remembering, i.e. was explicitly chosen by the
> * user and not automatically. module-stream-restore looks for
> * this.*/
> - bool save_sink:1, save_volume:1, save_muted:1;
> + char *preferred_sink;
> + bool save_volume:1, save_muted:1;
The comment above these lines is a bit illogical after this change, so
the comment needs to be updated as well. The preferred_sink deserves an
in-depth explanation about how it's used, so don't bundle that with
save_volume and save_muted.
--
Tanu
https://www.patreon.com/tanuk
https://liberapay.com/tanuk
_______________________________________________
pulseaudio-discuss mailing list
pulseaudio-discuss@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
