Re: [pulseaudio-discuss] [PATCH 1/4] change bool save_sink to char *preferred_sink
On 2018/12/23 上午3:06, Tanu Kaskinen wrote:
On Sat, 2018-12-15 at 16:47 +0800, Hui Wang wrote:
On 2018/12/12 下午9:39, Tanu Kaskinen wrote:
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.
OK, got it, will addressed all comments.
Signed-off-by: Hui Wang
[...]
}
}
@@ -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.
Actually I didn't change this part first, but I remember the stream bond
did not work as expected, after changed as above, it worked as expected.
Supposing sink0 is hdmi, and is playing a music over sink0, sink1 is
speaker, after I unplugged the hdmi cable, the music is switched to
speaker, but music->preferred_sink is still sink0-hdmi, after I plug
the hdmi cable again, the music should be switched back to sink0-hdmi.
I remember after I unplugged the cable, a user-space app call
extension_cb to set the music->preferred_sink to be sink1-speaker
(default_sink), then the music can't be switched back to hdmi anymore.
I will test it again, if it is really not needed, I will drop these code.
I would guess that it's gnome-control-center that sets the device to
speakers when you select speakers as the output. gnome-control-center
updates the routing in all stream-restore entries. Once these patches
are merged (and once a new release is made), gnome-control-center can
be fixed to not mess with the stream-restore database any more. I
believe the current gnome-control-center behaviour is a workaround for
the fact that PulseAudio hasn't so far automatically moved streams when
the default sink changes.
Yes, it is gnome-control-center. OK, will drop this part.
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] [PATCH 1/4] change bool save_sink to char *preferred_sink
On Sat, 2018-12-15 at 16:47 +0800, Hui Wang wrote:
> On 2018/12/12 下午9:39, Tanu Kaskinen wrote:
> > 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.
> OK, got it, will addressed all comments.
> > > Signed-off-by: Hui Wang
> [...]
> > > }
> > > }
> > > @@ -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.
>
> Actually I didn't change this part first, but I remember the stream bond
> did not work as expected, after changed as above, it worked as expected.
>
> Supposing sink0 is hdmi, and is playing a music over sink0, sink1 is
> speaker, after I unplugged the hdmi cable, the music is switched to
> speaker, but music->preferred_sink is still sink0-hdmi, after I plug
> the hdmi cable again, the music should be switched back to sink0-hdmi.
> I remember after I unplugged the cable, a user-space app call
> extension_cb to set the music->preferred_sink to be sink1-speaker
> (default_sink), then the music can't be switched back to hdmi anymore.
>
> I will test it again, if it is really not needed, I will drop these code.
I would guess that it's gnome-control-center that sets the device to
speakers when you select speakers as the output. gnome-control-center
updates the routing in all stream-restore entries. Once these patches
are merged (and once a new release is made), gnome-control-center can
be fixed to not mess with the stream-restore database any more. I
believe the current gnome-control-center behaviour is a workaround for
the fact that PulseAudio hasn't so far automatically moved streams when
the default sink changes.
--
Tanu
https://www.patreon.com/tanuk
https://liberapay.com/tanuk
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] [PATCH 1/4] change bool save_sink to char *preferred_sink
On 2018/12/12 下午9:39, Tanu Kaskinen wrote:
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.
OK, got it, will addressed all comments.
Signed-off-by: Hui Wang
[...]
}
}
@@ -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.
Actually I didn't change this part first, but I remember the stream bond
did not work as expected, after changed as above, it worked as expected.
Supposing sink0 is hdmi, and is playing a music over sink0, sink1 is
speaker, after I unplugged the hdmi cable, the music is switched to
speaker, but music->preferred_sink is still sink0-hdmi, after I plug
the hdmi cable again, the music should be switched back to sink0-hdmi.
I remember after I unplugged the cable, a user-space app call
extension_cb to set the music->preferred_sink to be sink1-speaker
(default_sink), then the music can't be switched back to hdmi anymore.
I will test it again, if it is really not needed, I will drop these code.
Thanks,
Hui.
if (entry->device_valid && !pa_namereg_is_valid_name(entry->device)) {
entry_free(entry);
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] [PATCH 1/4] change bool save_sink to char *preferred_sink
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
> ---
> 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 15fd2..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
[pulseaudio-discuss] [PATCH 1/4] change bool save_sink to char *preferred_sink
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.
Signed-off-by: Hui Wang
---
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 15fd2..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)
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)
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));
+
+ 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;
/* 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);
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);
+ }
}
entry_free(e);
@@ -1942,12 +1954,13 @@ static void entry_apply(struct userdata *u, const char
*name, struct entry *e) {
