[v2,2/2] switch-on-port-available: deactivate direction when the no ports are available for that direction

Submitted by Tanu Kaskinen on Dec. 28, 2017, 3:14 p.m.

Details

Message ID 20171228151414.19167-3-tanuk@iki.fi
State New
Headers show
Series "Improve routing when all ports of a card become unavailable" ( rev: 2 ) in PulseAudio

Not browsing as part of any series.

Commit Message

Tanu Kaskinen Dec. 28, 2017, 3:14 p.m.
This ensures that streams are moved away from unavailable outputs or
inputs. For example, sometimes HDMI is on a dedicated alsa card, and if
all HDMI outputs become unavailable, then the card profile will be set
to "off", and the streams will be moved somewhere else.
---

Changes in v2:
 - Don't call deactivate_direction() unconditionally.


 src/modules/module-switch-on-port-available.c | 40 ++++++++++++++++++++++++++-
 1 file changed, 39 insertions(+), 1 deletion(-)

Patch hide | download patch | download mbox

diff --git a/src/modules/module-switch-on-port-available.c b/src/modules/module-switch-on-port-available.c
index 8fd3c9e5f..c8e3d552b 100644
--- a/src/modules/module-switch-on-port-available.c
+++ b/src/modules/module-switch-on-port-available.c
@@ -80,7 +80,7 @@  static bool profile_good_for_output(pa_card_profile *profile, pa_device_port *po
     if (card->active_profile->max_source_channels != profile->max_source_channels)
         return false;
 
-    if (port == card->preferred_output_port)
+    if (port && port == card->preferred_output_port)
         return true;
 
     PA_IDXSET_FOREACH(sink, card->sinks, idx) {
@@ -251,6 +251,42 @@  static void switch_to_port(pa_device_port *port) {
         pa_sink_set_port(pp.sink, port->name, false);
 }
 
+static void deactivate_direction(pa_card *card, pa_direction_t direction) {
+    pa_card_profile *profile;
+    void *state;
+    pa_card_profile *best_profile = NULL;
+    const char *direction_str;
+
+    PA_HASHMAP_FOREACH(profile, card->profiles, state) {
+        switch (direction) {
+            case PA_DIRECTION_OUTPUT:
+                if (profile->n_sinks != 0)
+                    continue;
+                if (!profile_good_for_output(profile, NULL))
+                    continue;
+                break;
+
+            case PA_DIRECTION_INPUT:
+                if (profile->n_sources != 0)
+                    continue;
+                if (!profile_good_for_input(profile, NULL))
+                    continue;
+                break;
+        }
+
+        if (!best_profile || profile->priority > best_profile->priority)
+            best_profile = profile;
+    }
+
+    direction_str = direction == PA_DIRECTION_OUTPUT ? "output" : "input";
+
+    if (best_profile) {
+        pa_log_debug("Deactivating %s on card %s.", direction_str, card->name);
+        pa_card_set_profile(card, best_profile, false);
+    } else
+        pa_log_debug("Wanted to deactivate %s on card %s, but no suitable profile was found.", direction_str, card->name);
+}
+
 /* Switches away from a port, switching profiles if necessary or preferred */
 static void switch_from_port(pa_device_port *port) {
     struct port_pointers pp = find_port_pointers(port);
@@ -270,6 +306,8 @@  static void switch_from_port(pa_device_port *port) {
 
     if (best_port)
         switch_to_port(best_port);
+    else
+        deactivate_direction(port->card, port->direction);
 }
 
 

Comments

On 28.12.2017 16:14, Tanu Kaskinen wrote:
> This ensures that streams are moved away from unavailable outputs or
> inputs. For example, sometimes HDMI is on a dedicated alsa card, and if
> all HDMI outputs become unavailable, then the card profile will be set
> to "off", and the streams will be moved somewhere else.
> ---

I am not sure if changing the profile is a good idea. There is no
code which restores the profile when the cable gets plugged again.
Can't you fire a hook when a sink or source is suspended due to
PA_SUSPEND_UNAVAILABLE and let module-rescue-stream handle
the case?

Further comment below.
>
> Changes in v2:
>   - Don't call deactivate_direction() unconditionally.
>
>
>   src/modules/module-switch-on-port-available.c | 40 ++++++++++++++++++++++++++-
>   1 file changed, 39 insertions(+), 1 deletion(-)
>
> diff --git a/src/modules/module-switch-on-port-available.c b/src/modules/module-switch-on-port-available.c
> index 8fd3c9e5f..c8e3d552b 100644
> --- a/src/modules/module-switch-on-port-available.c
> +++ b/src/modules/module-switch-on-port-available.c
> @@ -80,7 +80,7 @@ static bool profile_good_for_output(pa_card_profile *profile, pa_device_port *po
>       if (card->active_profile->max_source_channels != profile->max_source_channels)
>           return false;
>   
> -    if (port == card->preferred_output_port)
> +    if (port && port == card->preferred_output_port)
>           return true;
>   
>       PA_IDXSET_FOREACH(sink, card->sinks, idx) {
> @@ -251,6 +251,42 @@ static void switch_to_port(pa_device_port *port) {
>           pa_sink_set_port(pp.sink, port->name, false);
>   }
>   
> +static void deactivate_direction(pa_card *card, pa_direction_t direction) {
> +    pa_card_profile *profile;
> +    void *state;
> +    pa_card_profile *best_profile = NULL;
> +    const char *direction_str;
> +
> +    PA_HASHMAP_FOREACH(profile, card->profiles, state) {
> +        switch (direction) {
> +            case PA_DIRECTION_OUTPUT:
> +                if (profile->n_sinks != 0)
> +                    continue;
> +                if (!profile_good_for_output(profile, NULL))

Should this not be profile_good_for_input()?

> +                    continue;
> +                break;
> +
> +            case PA_DIRECTION_INPUT:
> +                if (profile->n_sources != 0)
> +                    continue;
> +                if (!profile_good_for_input(profile, NULL))

... and this one profile_good_for_output()?

> +                    continue;
> +                break;
> +        }
> +
> +        if (!best_profile || profile->priority > best_profile->priority)
> +            best_profile = profile;
> +    }
> +
> +    direction_str = direction == PA_DIRECTION_OUTPUT ? "output" : "input";
> +
> +    if (best_profile) {
> +        pa_log_debug("Deactivating %s on card %s.", direction_str, card->name);
> +        pa_card_set_profile(card, best_profile, false);
> +    } else
> +        pa_log_debug("Wanted to deactivate %s on card %s, but no suitable profile was found.", direction_str, card->name);
> +}
> +
>   /* Switches away from a port, switching profiles if necessary or preferred */
>   static void switch_from_port(pa_device_port *port) {
>       struct port_pointers pp = find_port_pointers(port);
> @@ -270,6 +306,8 @@ static void switch_from_port(pa_device_port *port) {
>   
>       if (best_port)
>           switch_to_port(best_port);
> +    else
> +        deactivate_direction(port->card, port->direction);
>   }
>   
>
On Fri, 2017-12-29 at 08:25 +0100, Georg Chini wrote:
> On 28.12.2017 16:14, Tanu Kaskinen wrote:
> > This ensures that streams are moved away from unavailable outputs or
> > inputs. For example, sometimes HDMI is on a dedicated alsa card, and if
> > all HDMI outputs become unavailable, then the card profile will be set
> > to "off", and the streams will be moved somewhere else.
> > ---
> 
> I am not sure if changing the profile is a good idea. There is no
> code which restores the profile when the cable gets plugged again.
> Can't you fire a hook when a sink or source is suspended due to
> PA_SUSPEND_UNAVAILABLE and let module-rescue-stream handle
> the case?

When the HDMI port becomes available, module-switch-on-port-available
will change the profile as appropriate.

There's still the problem that once the HDMI sink is available again,
streams won't be moved there automatically (unless module-switch-on-
connect is loaded), but I think that's a lesser problem than keeping
streams connected to a silent sink.

> Further comment below.
> > 
> > Changes in v2:
> >   - Don't call deactivate_direction() unconditionally.
> > 
> > 
> >   src/modules/module-switch-on-port-available.c | 40 ++++++++++++++++++++++++++-
> >   1 file changed, 39 insertions(+), 1 deletion(-)
> > 
> > diff --git a/src/modules/module-switch-on-port-available.c b/src/modules/module-switch-on-port-available.c
> > index 8fd3c9e5f..c8e3d552b 100644
> > --- a/src/modules/module-switch-on-port-available.c
> > +++ b/src/modules/module-switch-on-port-available.c
> > @@ -80,7 +80,7 @@ static bool profile_good_for_output(pa_card_profile *profile, pa_device_port *po
> >       if (card->active_profile->max_source_channels != profile->max_source_channels)
> >           return false;
> >   
> > -    if (port == card->preferred_output_port)
> > +    if (port && port == card->preferred_output_port)
> >           return true;
> >   
> >       PA_IDXSET_FOREACH(sink, card->sinks, idx) {
> > @@ -251,6 +251,42 @@ static void switch_to_port(pa_device_port *port) {
> >           pa_sink_set_port(pp.sink, port->name, false);
> >   }
> >   
> > +static void deactivate_direction(pa_card *card, pa_direction_t direction) {
> > +    pa_card_profile *profile;
> > +    void *state;
> > +    pa_card_profile *best_profile = NULL;
> > +    const char *direction_str;
> > +
> > +    PA_HASHMAP_FOREACH(profile, card->profiles, state) {
> > +        switch (direction) {
> > +            case PA_DIRECTION_OUTPUT:
> > +                if (profile->n_sinks != 0)
> > +                    continue;
> > +                if (!profile_good_for_output(profile, NULL))
> 
> Should this not be profile_good_for_input()?

No. profile_good_for_output() is called when modifying the output
configuration. The function is used to check that the input side
doesn't change. I'm occasionally confused about the function naming
too, so it would be good to make it more clear...
On 29.12.2017 13:49, Tanu Kaskinen wrote:
> On Fri, 2017-12-29 at 08:25 +0100, Georg Chini wrote:
>> On 28.12.2017 16:14, Tanu Kaskinen wrote:
>>> This ensures that streams are moved away from unavailable outputs or
>>> inputs. For example, sometimes HDMI is on a dedicated alsa card, and if
>>> all HDMI outputs become unavailable, then the card profile will be set
>>> to "off", and the streams will be moved somewhere else.
>>> ---
>> I am not sure if changing the profile is a good idea. There is no
>> code which restores the profile when the cable gets plugged again.
>> Can't you fire a hook when a sink or source is suspended due to
>> PA_SUSPEND_UNAVAILABLE and let module-rescue-stream handle
>> the case?
> When the HDMI port becomes available, module-switch-on-port-available
> will change the profile as appropriate.

OK, I would have expected that jack state changes are ignored
once the card is switched to "off" but I see this is not the case.

>
> There's still the problem that once the HDMI sink is available again,
> streams won't be moved there automatically (unless module-switch-on-
> connect is loaded), but I think that's a lesser problem than keeping
> streams connected to a silent sink.
>
>> Further comment below.
>>> Changes in v2:
>>>    - Don't call deactivate_direction() unconditionally.
>>>
>>>
>>>    src/modules/module-switch-on-port-available.c | 40 ++++++++++++++++++++++++++-
>>>    1 file changed, 39 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/src/modules/module-switch-on-port-available.c b/src/modules/module-switch-on-port-available.c
>>> index 8fd3c9e5f..c8e3d552b 100644
>>> --- a/src/modules/module-switch-on-port-available.c
>>> +++ b/src/modules/module-switch-on-port-available.c
>>> @@ -80,7 +80,7 @@ static bool profile_good_for_output(pa_card_profile *profile, pa_device_port *po
>>>        if (card->active_profile->max_source_channels != profile->max_source_channels)
>>>            return false;
>>>    
>>> -    if (port == card->preferred_output_port)
>>> +    if (port && port == card->preferred_output_port)
>>>            return true;
>>>    
>>>        PA_IDXSET_FOREACH(sink, card->sinks, idx) {
>>> @@ -251,6 +251,42 @@ static void switch_to_port(pa_device_port *port) {
>>>            pa_sink_set_port(pp.sink, port->name, false);
>>>    }
>>>    
>>> +static void deactivate_direction(pa_card *card, pa_direction_t direction) {
>>> +    pa_card_profile *profile;
>>> +    void *state;
>>> +    pa_card_profile *best_profile = NULL;
>>> +    const char *direction_str;
>>> +
>>> +    PA_HASHMAP_FOREACH(profile, card->profiles, state) {
>>> +        switch (direction) {
>>> +            case PA_DIRECTION_OUTPUT:
>>> +                if (profile->n_sinks != 0)
>>> +                    continue;
>>> +                if (!profile_good_for_output(profile, NULL))
>> Should this not be profile_good_for_input()?
> No. profile_good_for_output() is called when modifying the output
> configuration. The function is used to check that the input side
> doesn't change. I'm occasionally confused about the function naming
> too, so it would be good to make it more clear...
>
I see, thanks.

One other question: Is it safe to pass NULL as port to profile_good_for_*()?
There is a comparison in the function which uses port->priority without
checking if port is NULL.
On 29.12.2017 21:28, Georg Chini wrote:
> On 29.12.2017 13:49, Tanu Kaskinen wrote:
>> On Fri, 2017-12-29 at 08:25 +0100, Georg Chini wrote:
>>> On 28.12.2017 16:14, Tanu Kaskinen wrote:
>>>> This ensures that streams are moved away from unavailable outputs or
>>>> inputs. For example, sometimes HDMI is on a dedicated alsa card, 
>>>> and if
>>>> all HDMI outputs become unavailable, then the card profile will be set
>>>> to "off", and the streams will be moved somewhere else.
>>>> ---
>>> I am not sure if changing the profile is a good idea. There is no
>>> code which restores the profile when the cable gets plugged again.
>>> Can't you fire a hook when a sink or source is suspended due to
>>> PA_SUSPEND_UNAVAILABLE and let module-rescue-stream handle
>>> the case?
>> When the HDMI port becomes available, module-switch-on-port-available
>> will change the profile as appropriate.
>
> OK, I would have expected that jack state changes are ignored
> once the card is switched to "off" but I see this is not the case.
>
>>
>> There's still the problem that once the HDMI sink is available again,
>> streams won't be moved there automatically (unless module-switch-on-
>> connect is loaded), but I think that's a lesser problem than keeping
>> streams connected to a silent sink.

Would not using a hook and let module-rescue-stream handle the
suspend/unsuspend solve that problem as well? You would then also
have to modify the default sink selection to take the suspend state
into account.
On Fri, 2017-12-29 at 21:28 +0100, Georg Chini wrote:
> On 29.12.2017 13:49, Tanu Kaskinen wrote:
> > On Fri, 2017-12-29 at 08:25 +0100, Georg Chini wrote:
> > > On 28.12.2017 16:14, Tanu Kaskinen wrote:
> > > > +static void deactivate_direction(pa_card *card, pa_direction_t direction) {
> > > > +    pa_card_profile *profile;
> > > > +    void *state;
> > > > +    pa_card_profile *best_profile = NULL;
> > > > +    const char *direction_str;
> > > > +
> > > > +    PA_HASHMAP_FOREACH(profile, card->profiles, state) {
> > > > +        switch (direction) {
> > > > +            case PA_DIRECTION_OUTPUT:
> > > > +                if (profile->n_sinks != 0)
> > > > +                    continue;
> > > > +                if (!profile_good_for_output(profile, NULL))
> > > 
> > > Should this not be profile_good_for_input()?
> > 
> > No. profile_good_for_output() is called when modifying the output
> > configuration. The function is used to check that the input side
> > doesn't change. I'm occasionally confused about the function naming
> > too, so it would be good to make it more clear...
> > 
> 
> I see, thanks.
> 
> One other question: Is it safe to pass NULL as port to profile_good_for_*()?
> There is a comparison in the function which uses port->priority without
> checking if port is NULL.

It should be safe, because the port pointer is dereferenced only if the
current active sink port is available, and deactivate_direction()
passes NULL only when the sink port is unavailable. However, maybe it's
best to add an extra check just in case.
On Sat, 2017-12-30 at 13:09 +0100, Georg Chini wrote:
> On 29.12.2017 21:28, Georg Chini wrote:
> > On 29.12.2017 13:49, Tanu Kaskinen wrote:
> > > There's still the problem that once the HDMI sink is available again,
> > > streams won't be moved there automatically (unless module-switch-on-
> > > connect is loaded), but I think that's a lesser problem than keeping
> > > streams connected to a silent sink.
> 
> Would not using a hook and let module-rescue-stream handle the
> suspend/unsuspend solve that problem as well? You would then also
> have to modify the default sink selection to take the suspend state
> into account.

What logic would module-rescue-stream use in the unsuspend case? It's
not obvious to me how it should determine when to move streams to an
unsuspended sink and which streams to move.

I don't think the default sink selection logic needs any changes. Sinks
whose active port is unavailable are already avoided, and when the
active port becomes available again, the default sink selection is re-
evaluated, so the HDMI sink will likely become the default sink again
if it was the default sink previously.

For what it's worth, I think the real fix is to make it more explicit
when streams are "default-routed" and when their routing has been
overridden by the user. Then all default-routed streams could be easily
moved whenever the default sink changes. I think this needs UI changes
as well, because otherwise it will be confusing. UIs that provide per-
stream routing control should show when a stream has been manually
routed, and it should be possible to return the stream to the "default-
routed" group. I don't expect to work on this any time soon, so this
only slightly relevant to the present discussion.
On 30.12.2017 13:55, Tanu Kaskinen wrote:
> On Sat, 2017-12-30 at 13:09 +0100, Georg Chini wrote:
>> On 29.12.2017 21:28, Georg Chini wrote:
>>> On 29.12.2017 13:49, Tanu Kaskinen wrote:
>>>> There's still the problem that once the HDMI sink is available again,
>>>> streams won't be moved there automatically (unless module-switch-on-
>>>> connect is loaded), but I think that's a lesser problem than keeping
>>>> streams connected to a silent sink.
>> Would not using a hook and let module-rescue-stream handle the
>> suspend/unsuspend solve that problem as well? You would then also
>> have to modify the default sink selection to take the suspend state
>> into account.
> What logic would module-rescue-stream use in the unsuspend case? It's
> not obvious to me how it should determine when to move streams to an
> unsuspended sink and which streams to move.

The same logic that module-switch-on-connect applies with the
difference that the unsuspended sink does not become the default
automatically. So if the "normal" default sink selection determines
that the unsuspended sink should be the default, then move the
streams from the current default to the new one, else do nothing.

>
> I don't think the default sink selection logic needs any changes. Sinks
> whose active port is unavailable are already avoided, and when the
> active port becomes available again, the default sink selection is re-
> evaluated, so the HDMI sink will likely become the default sink again
> if it was the default sink previously.

Yes, right, I did not take into account that the algorithm checks
the port state as well.
On 30.12.2017 13:55, Tanu Kaskinen wrote:
> On Sat, 2017-12-30 at 13:09 +0100, Georg Chini wrote:
>> On 29.12.2017 21:28, Georg Chini wrote:
>>> On 29.12.2017 13:49, Tanu Kaskinen wrote:
>>>> There's still the problem that once the HDMI sink is available again,
>>>> streams won't be moved there automatically (unless module-switch-on-
>>>> connect is loaded), but I think that's a lesser problem than keeping
>>>> streams connected to a silent sink.
>>
>>
>> For what it's worth, I think the real fix is to make it more explicit
>> when streams are "default-routed" and when their routing has been
>> overridden by the user. Then all default-routed streams could be easily
>> moved whenever the default sink changes. I think this needs UI changes
>> as well, because otherwise it will be confusing. UIs that provide per-
>> stream routing control should show when a stream has been manually
>> routed, and it should be possible to return the stream to the "default-
>> routed" group. I don't expect to work on this any time soon, so this
>> only slightly relevant to the present discussion.
>>
I still think that it would be sufficient to consider all streams on
the default sink as default-routed. Even if a user moves a stream
manually to the default sink, the purpose is probably that the
stream plays on the default sink and not that it plays on that
specific hardware. From a user perspective I would not expect
that some streams stay on the old sink when the default changes,
on the contrary I would find it rather annoying.

Anything not playing on the default sink is user-routed. This avoids
the necessity of a per stream extra flag and simplifies the stream
move logic.

But as you said above it's not relevant at the moment.
On Sat, 2017-12-30 at 14:27 +0100, Georg Chini wrote:
> On 30.12.2017 13:55, Tanu Kaskinen wrote:
> > On Sat, 2017-12-30 at 13:09 +0100, Georg Chini wrote:
> > > On 29.12.2017 21:28, Georg Chini wrote:
> > > > On 29.12.2017 13:49, Tanu Kaskinen wrote:
> > > > > There's still the problem that once the HDMI sink is available again,
> > > > > streams won't be moved there automatically (unless module-switch-on-
> > > > > connect is loaded), but I think that's a lesser problem than keeping
> > > > > streams connected to a silent sink.
> > > 
> > > Would not using a hook and let module-rescue-stream handle the
> > > suspend/unsuspend solve that problem as well? You would then also
> > > have to modify the default sink selection to take the suspend state
> > > into account.
> > 
> > What logic would module-rescue-stream use in the unsuspend case? It's
> > not obvious to me how it should determine when to move streams to an
> > unsuspended sink and which streams to move.
> 
> The same logic that module-switch-on-connect applies with the
> difference that the unsuspended sink does not become the default
> automatically. So if the "normal" default sink selection determines
> that the unsuspended sink should be the default, then move the
> streams from the current default to the new one, else do nothing.

So module-rescue-stream should move all streams from the old default
sink to the new default sink? Or are there some exceptions? Can it even
ignore the unsuspend event and only care about default sink changes?

module-switch-on-connect doesn't move streams that have save_sink set
on them. I get the impression that you don't want that exception.
Instead, if a stream ever becomes routed to the default sink (either by
moving to the default sink, or the default sink changing to the
stream's sink), then that should make the stream "default-routed".

How is module-stream-restore supposed to behave? If a stream becomes
"default-routed", it would seem appropriate to erase the "manual
override" from the stream-restore database, but that doesn't work in
this scenario:

 - sink1 is the default sink
 - sink2 is some other sink
 - stream is manually routed to sink2
 - sink2 disappears and stream moves to sink1 -> stream becomes
   "default-routed"
 - sink2 appears again

The stream should quite clearly be moved to sink2, but if the routing
information is removed from the stream-restore database, no move will
happen.

On the other hand, if the stream-restore database is left alone, that
doesn't work in this scenario:

 - sink1 is the default sink
 - sink2 is some other sink
 - stream is manually routed to sink2
 - sink1 disappears -> sink2 becomes the default sink
 - sink1 appears again -> sink1 becomes the default sink -> stream
   moves to sink1, because it was previously routed to the default sink
 - stream disappears (maybe because application is restarted)
 - stream appears again, now module-stream-restore will route it to
   sink2 again

So before restarting the application the stream was routed to sink1,
and after restarting the application the stream is routed to sink2.
That doesn't make sense.
On 30.12.2017 16:12, Tanu Kaskinen wrote:
> On Sat, 2017-12-30 at 14:27 +0100, Georg Chini wrote:
>> On 30.12.2017 13:55, Tanu Kaskinen wrote:
>>> On Sat, 2017-12-30 at 13:09 +0100, Georg Chini wrote:
>>>> On 29.12.2017 21:28, Georg Chini wrote:
>>>>> On 29.12.2017 13:49, Tanu Kaskinen wrote:
>>>>>> There's still the problem that once the HDMI sink is available again,
>>>>>> streams won't be moved there automatically (unless module-switch-on-
>>>>>> connect is loaded), but I think that's a lesser problem than keeping
>>>>>> streams connected to a silent sink.
>>>> Would not using a hook and let module-rescue-stream handle the
>>>> suspend/unsuspend solve that problem as well? You would then also
>>>> have to modify the default sink selection to take the suspend state
>>>> into account.
>>> What logic would module-rescue-stream use in the unsuspend case? It's
>>> not obvious to me how it should determine when to move streams to an
>>> unsuspended sink and which streams to move.
>> The same logic that module-switch-on-connect applies with the
>> difference that the unsuspended sink does not become the default
>> automatically. So if the "normal" default sink selection determines
>> that the unsuspended sink should be the default, then move the
>> streams from the current default to the new one, else do nothing.
> So module-rescue-stream should move all streams from the old default
> sink to the new default sink? Or are there some exceptions? Can it even
> ignore the unsuspend event and only care about default sink changes?

Yes, in theory module-rescue-stream could just care about default
sink changes in addition to the appearance and disappearance of
sinks. This would also simplify module-switch-on-connect or make
it even obsolete.
The drawback are those cards with flapping jack detection. At the
moment we can hide this driver or card problem simply by not using
module-switch-on-port-available.
Therefore currently I would use the suspend/unsuspend events to
avoid mixing several unrelated problems.

>
> module-switch-on-connect doesn't move streams that have save_sink set
> on them. I get the impression that you don't want that exception.
> Instead, if a stream ever becomes routed to the default sink (either by
> moving to the default sink, or the default sink changing to the
> stream's sink), then that should make the stream "default-routed".

In general you are right, I don't want exceptions. See also my
other mail. You are however also correct that there are some
situations I did not think about. So exceptions are necessary
whenever a stream is moved forcefully to the default sink.

To take care of those situations, the save_sink flag should be
used differently. It should only be set on those streams that
have forcefully been moved from a non-default-sink to the
default sink. Two possible ways this can happen:

1) The stream is moved to the sink: The original sink disappears
      and the stream is rescued to the default sink.
2) The sink is moved to the stream: The default sink disappears
      and the default moves to a sink where some streams are
      already playing.

If the default sink is chosen manually, all save_sink flags should
be reset.
A manual stream move to some sink would reset the flag.

This way only streams on the default sink could have the save_sink
flag set which would always clearly indicate if a stream is playing on
the default sink on purpose.

>
> How is module-stream-restore supposed to behave? If a stream becomes
> "default-routed", it would seem appropriate to erase the "manual
> override" from the stream-restore database, but that doesn't work in
> this scenario:
>
>   - sink1 is the default sink
>   - sink2 is some other sink
>   - stream is manually routed to sink2
>   - sink2 disappears and stream moves to sink1 -> stream becomes
>     "default-routed"

The save_sink flag is set on the stream

>   - sink2 appears again

Module-stream-restore can detect that the stream should be moved
back to sink2 because of the save_sink flag.

>
> The stream should quite clearly be moved to sink2, but if the routing
> information is removed from the stream-restore database, no move will
> happen.
>
> On the other hand, if the stream-restore database is left alone, that
> doesn't work in this scenario:
>
>   - sink1 is the default sink
>   - sink2 is some other sink
>   - stream is manually routed to sink2
>   - sink1 disappears -> sink2 becomes the default sink

The stream gets the save_sink flag set.

>   - sink1 appears again -> sink1 becomes the default sink -> stream
>     moves to sink1, because it was previously routed to the default sink

Module-stream-restore can detect that the stream should not be
moved because of the save_sink flag.

>   - stream disappears (maybe because application is restarted)
>   - stream appears again, now module-stream-restore will route it to
>     sink2 again
>
> So before restarting the application the stream was routed to sink1,
> and after restarting the application the stream is routed to sink2.
> That doesn't make sense.
>
Maybe save_sink could contain the name of the origin sink instead
of just being a flag, that would make routing easier.
On Sat, 2017-12-30 at 18:08 +0100, Georg Chini wrote:
> On 30.12.2017 16:12, Tanu Kaskinen wrote:
> > On Sat, 2017-12-30 at 14:27 +0100, Georg Chini wrote:
> > > On 30.12.2017 13:55, Tanu Kaskinen wrote:
> > > > On Sat, 2017-12-30 at 13:09 +0100, Georg Chini wrote:
> > > > > On 29.12.2017 21:28, Georg Chini wrote:
> > > > > > On 29.12.2017 13:49, Tanu Kaskinen wrote:
> > > > > > > There's still the problem that once the HDMI sink is available again,
> > > > > > > streams won't be moved there automatically (unless module-switch-on-
> > > > > > > connect is loaded), but I think that's a lesser problem than keeping
> > > > > > > streams connected to a silent sink.
> > > > > 
> > > > > Would not using a hook and let module-rescue-stream handle the
> > > > > suspend/unsuspend solve that problem as well? You would then also
> > > > > have to modify the default sink selection to take the suspend state
> > > > > into account.
> > > > 
> > > > What logic would module-rescue-stream use in the unsuspend case? It's
> > > > not obvious to me how it should determine when to move streams to an
> > > > unsuspended sink and which streams to move.
> > > 
> > > The same logic that module-switch-on-connect applies with the
> > > difference that the unsuspended sink does not become the default
> > > automatically. So if the "normal" default sink selection determines
> > > that the unsuspended sink should be the default, then move the
> > > streams from the current default to the new one, else do nothing.
> > 
> > So module-rescue-stream should move all streams from the old default
> > sink to the new default sink? Or are there some exceptions? Can it even
> > ignore the unsuspend event and only care about default sink changes?
> 
> Yes, in theory module-rescue-stream could just care about default
> sink changes in addition to the appearance and disappearance of
> sinks. This would also simplify module-switch-on-connect or make
> it even obsolete.
> The drawback are those cards with flapping jack detection. At the
> moment we can hide this driver or card problem simply by not using
> module-switch-on-port-available.
> Therefore currently I would use the suspend/unsuspend events to
> avoid mixing several unrelated problems.

Good point about faulty jack detection. We shouldn't add too much
automation that can't be disabled when jack detection is broken.

> > module-switch-on-connect doesn't move streams that have save_sink set
> > on them. I get the impression that you don't want that exception.
> > Instead, if a stream ever becomes routed to the default sink (either by
> > moving to the default sink, or the default sink changing to the
> > stream's sink), then that should make the stream "default-routed".
> 
> In general you are right, I don't want exceptions. See also my
> other mail. You are however also correct that there are some
> situations I did not think about. So exceptions are necessary
> whenever a stream is moved forcefully to the default sink.
> 
> To take care of those situations, the save_sink flag should be
> used differently. It should only be set on those streams that
> have forcefully been moved from a non-default-sink to the
> default sink. Two possible ways this can happen:
> 
> 1) The stream is moved to the sink: The original sink disappears
>       and the stream is rescued to the default sink.
> 2) The sink is moved to the stream: The default sink disappears
>       and the default moves to a sink where some streams are
>       already playing.
> 
> If the default sink is chosen manually, all save_sink flags should
> be reset.
> A manual stream move to some sink would reset the flag.
> 
> This way only streams on the default sink could have the save_sink
> flag set which would always clearly indicate if a stream is playing on
> the default sink on purpose.
> 
> > 
> > How is module-stream-restore supposed to behave? If a stream becomes
> > "default-routed", it would seem appropriate to erase the "manual
> > override" from the stream-restore database, but that doesn't work in
> > this scenario:
> > 
> >   - sink1 is the default sink
> >   - sink2 is some other sink
> >   - stream is manually routed to sink2
> >   - sink2 disappears and stream moves to sink1 -> stream becomes
> >     "default-routed"
> 
> The save_sink flag is set on the stream
> 
> >   - sink2 appears again
> 
> Module-stream-restore can detect that the stream should be moved
> back to sink2 because of the save_sink flag.
> 
> > 
> > The stream should quite clearly be moved to sink2, but if the routing
> > information is removed from the stream-restore database, no move will
> > happen.
> > 
> > On the other hand, if the stream-restore database is left alone, that
> > doesn't work in this scenario:
> > 
> >   - sink1 is the default sink
> >   - sink2 is some other sink
> >   - stream is manually routed to sink2
> >   - sink1 disappears -> sink2 becomes the default sink
> 
> The stream gets the save_sink flag set.
> 
> >   - sink1 appears again -> sink1 becomes the default sink -> stream
> >     moves to sink1, because it was previously routed to the default sink
> 
> Module-stream-restore can detect that the stream should not be
> moved because of the save_sink flag.
> 
> >   - stream disappears (maybe because application is restarted)
> >   - stream appears again, now module-stream-restore will route it to
> >     sink2 again
> > 
> > So before restarting the application the stream was routed to sink1,
> > and after restarting the application the stream is routed to sink2.
> > That doesn't make sense.
> > 
> 
> Maybe save_sink could contain the name of the origin sink instead
> of just being a flag, that would make routing easier.

Your proposal sounds good, if I understand it correctly. I don't think
module-rescue-streams needs or should be involved, however. Here's my
proposal:

 - The "save_sink" flag should be replaced with "users_routing_choice",
which is a sink name (i.e. string).
 - The routing choice is set when the user moves a stream to a non-
default sink and unset when the user moves a stream to the default
sink.
 - When the default sink changes, streams connected to the old default
sink should be moved unless their routing choice is set to the old
default sink.

The moving can be handled automatically by the core. module-stream-
restore doesn't have to be concerned with moving streams any more, it
just needs to save the routing choices in the database and restore them
when new streams appear. module-switch-on-connect doesn't have to move
streams either, it only needs to set the default sink.

To deal with broken jack detection, module-udev-detect should first get
an "enable_jack_detection" option that can be set to false to disable
any automatic routing based on jack detection. (I'd like to have finer-
grained control for disabling jack detection for individual ports
eventually, but I prefer to start with a simple all-or-nothing option
to allow quick progress on the automatic routing improvements).
On 03.01.2018 14:51, Tanu Kaskinen wrote:
> On Sat, 2017-12-30 at 18:08 +0100, Georg Chini wrote:
>> On 30.12.2017 16:12, Tanu Kaskinen wrote:
>>> On Sat, 2017-12-30 at 14:27 +0100, Georg Chini wrote:
>>>> On 30.12.2017 13:55, Tanu Kaskinen wrote:
>>>>> On Sat, 2017-12-30 at 13:09 +0100, Georg Chini wrote:
>>>>>> On 29.12.2017 21:28, Georg Chini wrote:
>>>>>>> On 29.12.2017 13:49, Tanu Kaskinen wrote:
>>>>>>>> There's still the problem that once the HDMI sink is available again,
>>>>>>>> streams won't be moved there automatically (unless module-switch-on-
>>>>>>>> connect is loaded), but I think that's a lesser problem than keeping
>>>>>>>> streams connected to a silent sink.
>>>>>> Would not using a hook and let module-rescue-stream handle the
>>>>>> suspend/unsuspend solve that problem as well? You would then also
>>>>>> have to modify the default sink selection to take the suspend state
>>>>>> into account.
>>>>> What logic would module-rescue-stream use in the unsuspend case? It's
>>>>> not obvious to me how it should determine when to move streams to an
>>>>> unsuspended sink and which streams to move.
>>>> The same logic that module-switch-on-connect applies with the
>>>> difference that the unsuspended sink does not become the default
>>>> automatically. So if the "normal" default sink selection determines
>>>> that the unsuspended sink should be the default, then move the
>>>> streams from the current default to the new one, else do nothing.
>>> So module-rescue-stream should move all streams from the old default
>>> sink to the new default sink? Or are there some exceptions? Can it even
>>> ignore the unsuspend event and only care about default sink changes?
>> Yes, in theory module-rescue-stream could just care about default
>> sink changes in addition to the appearance and disappearance of
>> sinks. This would also simplify module-switch-on-connect or make
>> it even obsolete.
>> The drawback are those cards with flapping jack detection. At the
>> moment we can hide this driver or card problem simply by not using
>> module-switch-on-port-available.
>> Therefore currently I would use the suspend/unsuspend events to
>> avoid mixing several unrelated problems.
> Good point about faulty jack detection. We shouldn't add too much
> automation that can't be disabled when jack detection is broken.
>
>>> module-switch-on-connect doesn't move streams that have save_sink set
>>> on them. I get the impression that you don't want that exception.
>>> Instead, if a stream ever becomes routed to the default sink (either by
>>> moving to the default sink, or the default sink changing to the
>>> stream's sink), then that should make the stream "default-routed".
>> In general you are right, I don't want exceptions. See also my
>> other mail. You are however also correct that there are some
>> situations I did not think about. So exceptions are necessary
>> whenever a stream is moved forcefully to the default sink.
>>
>> To take care of those situations, the save_sink flag should be
>> used differently. It should only be set on those streams that
>> have forcefully been moved from a non-default-sink to the
>> default sink. Two possible ways this can happen:
>>
>> 1) The stream is moved to the sink: The original sink disappears
>>        and the stream is rescued to the default sink.
>> 2) The sink is moved to the stream: The default sink disappears
>>        and the default moves to a sink where some streams are
>>        already playing.
>>
>> If the default sink is chosen manually, all save_sink flags should
>> be reset.
>> A manual stream move to some sink would reset the flag.
>>
>> This way only streams on the default sink could have the save_sink
>> flag set which would always clearly indicate if a stream is playing on
>> the default sink on purpose.
>>
>>
> Your proposal sounds good, if I understand it correctly. I don't think
> module-rescue-streams needs or should be involved, however. Here's my
> proposal:
>
>   - The "save_sink" flag should be replaced with "users_routing_choice",
> which is a sink name (i.e. string).
>   - The routing choice is set when the user moves a stream to a non-
> default sink and unset when the user moves a stream to the default
> sink.
>   - When the default sink changes, streams connected to the old default
> sink should be moved unless their routing choice is set to the old
> default sink.

Well, my proposal was slightly different from yours. While you
want to set the routing choice on any stream not playing on the
default sink, I wanted to set it on any stream that is forcefully
moved to the default sink.
With regard to routing, the end result however is the same in
both cases. The only difference is that the streams on the
non-default sinks also have the users choice set, which makes
things easier when it comes to saving/restoring.
So I am fine with your proposal.

One addition: When the user manually sets the default sink,
the users choice should be reset for the streams currently
playing on the new default sink.

One question: When a stream is created and the sink it is
intended to play on is not available, it is put to the default
sink instead. Should we keep the users original choice then
or remove it because the stream was this time created on
the default sink? (I would tend to remove the choice in that
case.)
> The moving can be handled automatically by the core. module-stream-
> restore doesn't have to be concerned with moving streams any more, it
> just needs to save the routing choices in the database and restore them
> when new streams appear. module-switch-on-connect doesn't have to move
> streams either, it only needs to set the default sink.

This sounds perfect and will make the modules much simpler.

>
> To deal with broken jack detection, module-udev-detect should first get
> an "enable_jack_detection" option that can be set to false to disable
> any automatic routing based on jack detection. (I'd like to have finer-
> grained control for disabling jack detection for individual ports
> eventually, but I prefer to start with a simple all-or-nothing option
> to allow quick progress on the automatic routing improvements).
>
Would that not be a use case for the message subsystem? In addition
to the global flag, each port could have a handler, so that you could
disable/enable jack detection on a per port basis on the fly.
On 03.01.2018 14:51, Tanu Kaskinen wrote:
> On Sat, 2017-12-30 at 18:08 +0100, Georg Chini wrote:
>> On 30.12.2017 16:12, Tanu Kaskinen wrote:
>>> On Sat, 2017-12-30 at 14:27 +0100, Georg Chini wrote:
>>>> On 30.12.2017 13:55, Tanu Kaskinen wrote:
>>>>> On Sat, 2017-12-30 at 13:09 +0100, Georg Chini wrote:
>>>>>> On 29.12.2017 21:28, Georg Chini wrote:
>>>>>>> On 29.12.2017 13:49, Tanu Kaskinen wrote:
>>>>>>>> There's still the problem that once the HDMI sink is available again,
>>>>>>>> streams won't be moved there automatically (unless module-switch-on-
>>>>>>>> connect is loaded), but I think that's a lesser problem than keeping
>>>>>>>> streams connected to a silent sink.
>>>>>> Would not using a hook and let module-rescue-stream handle the
>>>>>> suspend/unsuspend solve that problem as well? You would then also
>>>>>> have to modify the default sink selection to take the suspend state
>>>>>> into account.
>>>>> What logic would module-rescue-stream use in the unsuspend case? It's
>>>>> not obvious to me how it should determine when to move streams to an
>>>>> unsuspended sink and which streams to move.
>>>> The same logic that module-switch-on-connect applies with the
>>>> difference that the unsuspended sink does not become the default
>>>> automatically. So if the "normal" default sink selection determines
>>>> that the unsuspended sink should be the default, then move the
>>>> streams from the current default to the new one, else do nothing.
>>> So module-rescue-stream should move all streams from the old default
>>> sink to the new default sink? Or are there some exceptions? Can it even
>>> ignore the unsuspend event and only care about default sink changes?
>> Yes, in theory module-rescue-stream could just care about default
>> sink changes in addition to the appearance and disappearance of
>> sinks. This would also simplify module-switch-on-connect or make
>> it even obsolete.
>> The drawback are those cards with flapping jack detection. At the
>> moment we can hide this driver or card problem simply by not using
>> module-switch-on-port-available.
>> Therefore currently I would use the suspend/unsuspend events to
>> avoid mixing several unrelated problems.
> Good point about faulty jack detection. We shouldn't add too much
> automation that can't be disabled when jack detection is broken.
>
>>> module-switch-on-connect doesn't move streams that have save_sink set
>>> on them. I get the impression that you don't want that exception.
>>> Instead, if a stream ever becomes routed to the default sink (either by
>>> moving to the default sink, or the default sink changing to the
>>> stream's sink), then that should make the stream "default-routed".
>> In general you are right, I don't want exceptions. See also my
>> other mail. You are however also correct that there are some
>> situations I did not think about. So exceptions are necessary
>> whenever a stream is moved forcefully to the default sink.
>>
>> To take care of those situations, the save_sink flag should be
>> used differently. It should only be set on those streams that
>> have forcefully been moved from a non-default-sink to the
>> default sink. Two possible ways this can happen:
>>
>> 1) The stream is moved to the sink: The original sink disappears
>>        and the stream is rescued to the default sink.
>> 2) The sink is moved to the stream: The default sink disappears
>>        and the default moves to a sink where some streams are
>>        already playing.
>>
>> If the default sink is chosen manually, all save_sink flags should
>> be reset.
>> A manual stream move to some sink would reset the flag.
>>
>> This way only streams on the default sink could have the save_sink
>> flag set which would always clearly indicate if a stream is playing on
>> the default sink on purpose.
>>
>>
> Your proposal sounds good, if I understand it correctly. I don't think
> module-rescue-streams needs or should be involved, however.

Don't we still need module-rescue-stream when a non-default HDMI
sink is forcefully suspended? Or do you want to move the rescue
functionality to the core?

> Here's my proposal:
>
>   - The "save_sink" flag should be replaced with "users_routing_choice",
> which is a sink name (i.e. string).
>   - The routing choice is set when the user moves a stream to a non-
> default sink and unset when the user moves a stream to the default
> sink.
>   - When the default sink changes, streams connected to the old default
> sink should be moved unless their routing choice is set to the old
> default sink.
>
> The moving can be handled automatically by the core. module-stream-
> restore doesn't have to be concerned with moving streams any more, it
> just needs to save the routing choices in the database and restore them
> when new streams appear. module-switch-on-connect doesn't have to move
> streams either, it only needs to set the default sink.
>
On Wed, 2018-01-03 at 17:34 +0100, Georg Chini wrote:
> On 03.01.2018 14:51, Tanu Kaskinen wrote:
> > Your proposal sounds good, if I understand it correctly. I don't think
> > module-rescue-streams needs or should be involved, however. Here's my
> > proposal:
> > 
> >   - The "save_sink" flag should be replaced with "users_routing_choice",
> > which is a sink name (i.e. string).
> >   - The routing choice is set when the user moves a stream to a non-
> > default sink and unset when the user moves a stream to the default
> > sink.
> >   - When the default sink changes, streams connected to the old default
> > sink should be moved unless their routing choice is set to the old
> > default sink.
> 
> Well, my proposal was slightly different from yours. While you
> want to set the routing choice on any stream not playing on the
> default sink, I wanted to set it on any stream that is forcefully
> moved to the default sink.
> With regard to routing, the end result however is the same in
> both cases. The only difference is that the streams on the
> non-default sinks also have the users choice set, which makes
> things easier when it comes to saving/restoring.
> So I am fine with your proposal.
> 
> One addition: When the user manually sets the default sink,
> the users choice should be reset for the streams currently
> playing on the new default sink.

I agree.

> One question: When a stream is created and the sink it is
> intended to play on is not available, it is put to the default
> sink instead. Should we keep the users original choice then
> or remove it because the stream was this time created on
> the default sink? (I would tend to remove the choice in that
> case.)

I don't think restarting an application should have effect on how it's
routed, so I think we should keep the original choice.

> > To deal with broken jack detection, module-udev-detect should first get
> > an "enable_jack_detection" option that can be set to false to disable
> > any automatic routing based on jack detection. (I'd like to have finer-
> > grained control for disabling jack detection for individual ports
> > eventually, but I prefer to start with a simple all-or-nothing option
> > to allow quick progress on the automatic routing improvements).
> > 
> 
> Would that not be a use case for the message subsystem? In addition
> to the global flag, each port could have a handler, so that you could
> disable/enable jack detection on a per port basis on the fly.

Yes, if I'm going to work on this, I plan using the message subsystem.
But if I'm going to work on this, I will first work on improving how
configuration files are handled, because the choice of disabling jack
detection for a port should be saved on disk, and I want to use a
configuration file for that in a particular way that isn't currently
supported.
On Thu, 2018-01-04 at 08:52 +0100, Georg Chini wrote:
> On 03.01.2018 14:51, Tanu Kaskinen wrote:
> > Your proposal sounds good, if I understand it correctly. I don't think
> > module-rescue-streams needs or should be involved, however.
> 
> Don't we still need module-rescue-stream when a non-default HDMI
> sink is forcefully suspended? Or do you want to move the rescue
> functionality to the core?

I didn't think of this case. Moving rescuing to the core would make
sense to me, but for now I think it's fine to handle this in module-
rescue-streams. I think the rescue trigger should be a sink becoming
unavailable rather than suspended, though. If we were to use suspending
as the trigger, we'd anyway only do the move when the suspend cause is
PA_SUSPEND_UNAVAILABLE, and I don't see why we shouldn't rescue streams
that are left on unavailable sinks that are not suspended.
On Thu, 2018-01-04 at 15:35 +0200, Tanu Kaskinen wrote:
> On Thu, 2018-01-04 at 08:52 +0100, Georg Chini wrote:
> > On 03.01.2018 14:51, Tanu Kaskinen wrote:
> > > Your proposal sounds good, if I understand it correctly. I don't think
> > > module-rescue-streams needs or should be involved, however.
> > 
> > Don't we still need module-rescue-stream when a non-default HDMI
> > sink is forcefully suspended? Or do you want to move the rescue
> > functionality to the core?
> 
> I didn't think of this case. Moving rescuing to the core would make
> sense to me, but for now I think it's fine to handle this in module-
> rescue-streams. I think the rescue trigger should be a sink becoming
> unavailable rather than suspended, though. If we were to use suspending
> as the trigger, we'd anyway only do the move when the suspend cause is
> PA_SUSPEND_UNAVAILABLE, and I don't see why we shouldn't rescue streams
> that are left on unavailable sinks that are not suspended.

Hmm... when the sink becomes available again, the rescued streams
should be moved back to it. I think this functionality is not a good
fit for module-rescue-streams. I think I'll put this stuff in the core
instead, if you're ok with that.
On 04.01.2018 14:49, Tanu Kaskinen wrote:
> On Thu, 2018-01-04 at 15:35 +0200, Tanu Kaskinen wrote:
>> On Thu, 2018-01-04 at 08:52 +0100, Georg Chini wrote:
>>> On 03.01.2018 14:51, Tanu Kaskinen wrote:
>>>> Your proposal sounds good, if I understand it correctly. I don't think
>>>> module-rescue-streams needs or should be involved, however.
>>> Don't we still need module-rescue-stream when a non-default HDMI
>>> sink is forcefully suspended? Or do you want to move the rescue
>>> functionality to the core?
>> I didn't think of this case. Moving rescuing to the core would make
>> sense to me, but for now I think it's fine to handle this in module-
>> rescue-streams. I think the rescue trigger should be a sink becoming
>> unavailable rather than suspended, though. If we were to use suspending
>> as the trigger, we'd anyway only do the move when the suspend cause is
>> PA_SUSPEND_UNAVAILABLE, and I don't see why we shouldn't rescue streams
>> that are left on unavailable sinks that are not suspended.
> Hmm... when the sink becomes available again, the rescued streams
> should be moved back to it. I think this functionality is not a good
> fit for module-rescue-streams. I think I'll put this stuff in the core
> instead, if you're ok with that.
>
Yes, I am fine with that. Basically it means that when a new sink
becomes available, you have to check the default sink if there is
any stream playing on it where the users choice is the just
appearing sink.
On 04.01.2018 14:35, Tanu Kaskinen wrote:
> On Thu, 2018-01-04 at 08:52 +0100, Georg Chini wrote:
>> On 03.01.2018 14:51, Tanu Kaskinen wrote:
>>> Your proposal sounds good, if I understand it correctly. I don't think
>>> module-rescue-streams needs or should be involved, however.
>> Don't we still need module-rescue-stream when a non-default HDMI
>> sink is forcefully suspended? Or do you want to move the rescue
>> functionality to the core?
> I didn't think of this case. Moving rescuing to the core would make
> sense to me, but for now I think it's fine to handle this in module-
> rescue-streams. I think the rescue trigger should be a sink becoming
> unavailable rather than suspended, though. If we were to use suspending
> as the trigger, we'd anyway only do the move when the suspend cause is
> PA_SUSPEND_UNAVAILABLE, and I don't see why we shouldn't rescue streams
> that are left on unavailable sinks that are not suspended.
>
I don't understand the argument. module-rescue-stream currently
rescues any stream if the underlying sink becomes unavailable, so
streams that are left on unavailable sinks that are not suspended
are already rescued.

PA_SUSPEND_UNAVAILABLE would only be an additional trigger for
the case that the sink is not removed. Your current patches change
the state of the sink to unavailable without removing it.

I do not want to rely on another module like switch-on-port-available
to remove the sink. I see three possible ways to ensure that the
streams are properly rescued:

- Change your patches so that the sink will be removed instead of suspended.
- Or add PA_SUSPEND_UNAVAILABLE as additional trigger to 
module-rescue-stream.
- Or let the core rescue the streams in the suspend case.
On 04.01.2018 14:27, Tanu Kaskinen wrote:
> On Wed, 2018-01-03 at 17:34 +0100, Georg Chini wrote:
>> On 03.01.2018 14:51, Tanu Kaskinen wrote:
>>> Your proposal sounds good, if I understand it correctly. I don't think
>>> module-rescue-streams needs or should be involved, however. Here's my
>>> proposal:
>>>
>>>    - The "save_sink" flag should be replaced with "users_routing_choice",
>>> which is a sink name (i.e. string).
>>>    - The routing choice is set when the user moves a stream to a non-
>>> default sink and unset when the user moves a stream to the default
>>> sink.
>>>    - When the default sink changes, streams connected to the old default
>>> sink should be moved unless their routing choice is set to the old
>>> default sink.
>>
>> One question: When a stream is created and the sink it is
>> intended to play on is not available, it is put to the default
>> sink instead. Should we keep the users original choice then
>> or remove it because the stream was this time created on
>> the default sink? (I would tend to remove the choice in that
>> case.)
> I don't think restarting an application should have effect on how it's
> routed, so I think we should keep the original choice.

I am not sure if you understood what I mean. Let me make an
example. Consider a stream that is manually moved to some
bluetooth sink, let's say because I want to watch a video using my
bluetooth headset. Next time I watch a video, the BT headset is
not connected and the video is played back on the default device.
The BT headset choice will however be remembered as long as
I do not manually move the stream.
If I now (a couple of weeks and many videos later) connect my
headset and watch a video, the stream will automatically move
to the BT headset. I am not sure if this should be the expected
behavior, but I am OK with keeping it that way.

>
>>> To deal with broken jack detection, module-udev-detect should first get
>>> an "enable_jack_detection" option that can be set to false to disable
>>> any automatic routing based on jack detection. (I'd like to have finer-
>>> grained control for disabling jack detection for individual ports
>>> eventually, but I prefer to start with a simple all-or-nothing option
>>> to allow quick progress on the automatic routing improvements).
>>>
>> Would that not be a use case for the message subsystem? In addition
>> to the global flag, each port could have a handler, so that you could
>> disable/enable jack detection on a per port basis on the fly.
> Yes, if I'm going to work on this, I plan using the message subsystem.
> But if I'm going to work on this, I will first work on improving how
> configuration files are handled, because the choice of disabling jack
> detection for a port should be saved on disk, and I want to use a
> configuration file for that in a particular way that isn't currently
> supported.
>
Can't we start with a CLI command? I think it is rather important that
we can enable/disable jack detection on a per port basis. I have seen
quite a few reports of flapping ports recently and somehow it seems
a waste to completely disable jack detection because of a single port.

The configuration will be rather static anyway and if we have a CLI
command, the user can put it in default.pa (at least temporarily
until your configuration file improvements are done).

If you agree, I would even volunteer to implement the per port control
once you have implemented the other routing changes (and my
messaging patches are pushed).
On Thu, 2018-01-04 at 20:54 +0100, Georg Chini wrote:
> On 04.01.2018 14:35, Tanu Kaskinen wrote:
> > On Thu, 2018-01-04 at 08:52 +0100, Georg Chini wrote:
> > > On 03.01.2018 14:51, Tanu Kaskinen wrote:
> > > > Your proposal sounds good, if I understand it correctly. I don't think
> > > > module-rescue-streams needs or should be involved, however.
> > > 
> > > Don't we still need module-rescue-stream when a non-default HDMI
> > > sink is forcefully suspended? Or do you want to move the rescue
> > > functionality to the core?
> > 
> > I didn't think of this case. Moving rescuing to the core would make
> > sense to me, but for now I think it's fine to handle this in module-
> > rescue-streams. I think the rescue trigger should be a sink becoming
> > unavailable rather than suspended, though. If we were to use suspending
> > as the trigger, we'd anyway only do the move when the suspend cause is
> > PA_SUSPEND_UNAVAILABLE, and I don't see why we shouldn't rescue streams
> > that are left on unavailable sinks that are not suspended.
> > 
> 
> I don't understand the argument. module-rescue-stream currently
> rescues any stream if the underlying sink becomes unavailable, so
> streams that are left on unavailable sinks that are not suspended
> are already rescued.
> 
> PA_SUSPEND_UNAVAILABLE would only be an additional trigger for
> the case that the sink is not removed. Your current patches change
> the state of the sink to unavailable without removing it.
> 
> I do not want to rely on another module like switch-on-port-available
> to remove the sink. I see three possible ways to ensure that the
> streams are properly rescued:
> 
> - Change your patches so that the sink will be removed instead of suspended.
> - Or add PA_SUSPEND_UNAVAILABLE as additional trigger to 
> module-rescue-stream.
> - Or let the core rescue the streams in the suspend case.

My plan is to let the core rescue streams that are left on a sink whose
active port becomes unavailable. It doesn't matter whether the sink
suspends itself or not.
On Sat, 2018-01-06 at 15:55 +0100, Georg Chini wrote:
> On 04.01.2018 14:27, Tanu Kaskinen wrote:
> > On Wed, 2018-01-03 at 17:34 +0100, Georg Chini wrote:
> > > On 03.01.2018 14:51, Tanu Kaskinen wrote:
> > > > Your proposal sounds good, if I understand it correctly. I don't think
> > > > module-rescue-streams needs or should be involved, however. Here's my
> > > > proposal:
> > > > 
> > > >    - The "save_sink" flag should be replaced with "users_routing_choice",
> > > > which is a sink name (i.e. string).
> > > >    - The routing choice is set when the user moves a stream to a non-
> > > > default sink and unset when the user moves a stream to the default
> > > > sink.
> > > >    - When the default sink changes, streams connected to the old default
> > > > sink should be moved unless their routing choice is set to the old
> > > > default sink.
> > > 
> > > One question: When a stream is created and the sink it is
> > > intended to play on is not available, it is put to the default
> > > sink instead. Should we keep the users original choice then
> > > or remove it because the stream was this time created on
> > > the default sink? (I would tend to remove the choice in that
> > > case.)
> > 
> > I don't think restarting an application should have effect on how it's
> > routed, so I think we should keep the original choice.
> 
> I am not sure if you understood what I mean. Let me make an
> example. Consider a stream that is manually moved to some
> bluetooth sink, let's say because I want to watch a video using my
> bluetooth headset. Next time I watch a video, the BT headset is
> not connected and the video is played back on the default device.
> The BT headset choice will however be remembered as long as
> I do not manually move the stream.
> If I now (a couple of weeks and many videos later) connect my
> headset and watch a video, the stream will automatically move
> to the BT headset. I am not sure if this should be the expected
> behavior, but I am OK with keeping it that way.

My point was that the routing is different in these two cases, and I
don't think it should be different:

case 1:

 - you move the video player to the BT headset
 - you disconnect the BT headset
 - you restart the video player
 - you connect the BT headset -> the video player stays on the default
device

case 2:

 - you move the video player to the BT headset
 - you disconnect the BT headset
 - you connect the BT headset -> the video player is moved to the
headset

The only difference is that the video player got restarted in case 1,
and I don't think that should have effect on the routing.

> > > > To deal with broken jack detection, module-udev-detect should first get
> > > > an "enable_jack_detection" option that can be set to false to disable
> > > > any automatic routing based on jack detection. (I'd like to have finer-
> > > > grained control for disabling jack detection for individual ports
> > > > eventually, but I prefer to start with a simple all-or-nothing option
> > > > to allow quick progress on the automatic routing improvements).
> > > > 
> > > 
> > > Would that not be a use case for the message subsystem? In addition
> > > to the global flag, each port could have a handler, so that you could
> > > disable/enable jack detection on a per port basis on the fly.
> > 
> > Yes, if I'm going to work on this, I plan using the message subsystem.
> > But if I'm going to work on this, I will first work on improving how
> > configuration files are handled, because the choice of disabling jack
> > detection for a port should be saved on disk, and I want to use a
> > configuration file for that in a particular way that isn't currently
> > supported.
> > 
> 
> Can't we start with a CLI command? I think it is rather important that
> we can enable/disable jack detection on a per port basis. I have seen
> quite a few reports of flapping ports recently and somehow it seems
> a waste to completely disable jack detection because of a single port.
> 
> The configuration will be rather static anyway and if we have a CLI
> command, the user can put it in default.pa (at least temporarily
> until your configuration file improvements are done).
> 
> If you agree, I would even volunteer to implement the per port control
> once you have implemented the other routing changes (and my
> messaging patches are pushed).

I'm OK with a CLI command as a temporary solution.
On 07.01.2018 20:32, Tanu Kaskinen wrote:
> On Thu, 2018-01-04 at 20:54 +0100, Georg Chini wrote:
>> On 04.01.2018 14:35, Tanu Kaskinen wrote:
>>> On Thu, 2018-01-04 at 08:52 +0100, Georg Chini wrote:
>>>> On 03.01.2018 14:51, Tanu Kaskinen wrote:
>>>>> Your proposal sounds good, if I understand it correctly. I don't think
>>>>> module-rescue-streams needs or should be involved, however.
>>>> Don't we still need module-rescue-stream when a non-default HDMI
>>>> sink is forcefully suspended? Or do you want to move the rescue
>>>> functionality to the core?
>>> I didn't think of this case. Moving rescuing to the core would make
>>> sense to me, but for now I think it's fine to handle this in module-
>>> rescue-streams. I think the rescue trigger should be a sink becoming
>>> unavailable rather than suspended, though. If we were to use suspending
>>> as the trigger, we'd anyway only do the move when the suspend cause is
>>> PA_SUSPEND_UNAVAILABLE, and I don't see why we shouldn't rescue streams
>>> that are left on unavailable sinks that are not suspended.
>>>
>> I don't understand the argument. module-rescue-stream currently
>> rescues any stream if the underlying sink becomes unavailable, so
>> streams that are left on unavailable sinks that are not suspended
>> are already rescued.
>>
>> PA_SUSPEND_UNAVAILABLE would only be an additional trigger for
>> the case that the sink is not removed. Your current patches change
>> the state of the sink to unavailable without removing it.
>>
>> I do not want to rely on another module like switch-on-port-available
>> to remove the sink. I see three possible ways to ensure that the
>> streams are properly rescued:
>>
>> - Change your patches so that the sink will be removed instead of suspended.
>> - Or add PA_SUSPEND_UNAVAILABLE as additional trigger to
>> module-rescue-stream.
>> - Or let the core rescue the streams in the suspend case.
> My plan is to let the core rescue streams that are left on a sink whose
> active port becomes unavailable. It doesn't matter whether the sink
> suspends itself or not.
>
I wonder if in that case it would not make sense to put the whole
rescue logic into the core (and maybe add a flag to disable it). But
that can be done in a second step.