[pulseaudio-discuss,3/3] sink, source: copy priority from active port

Submitted by Tanu Kaskinen on Sept. 8, 2016, 11:06 a.m.

Details

Message ID 20160908110629.14402-4-tanuk@iki.fi
State New
Headers show
Series "Improve default device policy" ( rev: 1 ) in PulseAudio

Not browsing as part of any series.

Commit Message

Tanu Kaskinen Sept. 8, 2016, 11:06 a.m.
It was reported in bug 93006 that on hardware that has HDMI and analog
outputs on different cards, the HDMI sink is chosen by default even when
headphones are plugged in. The headphone port has higher priority than
the HDMI port, but that is not reflected in the sink priorities. This
patch changes that - the sink/source priority is now the same as the
active port priority, unless the sink/source doesn't have any ports, in
which case the old priority logic is retained.

BugLink: https://bugs.freedesktop.org/show_bug.cgi?id=93006
---
 src/pulsecore/sink.c   | 18 ++++++++++++------
 src/pulsecore/source.c | 19 +++++++++++++------
 2 files changed, 25 insertions(+), 12 deletions(-)

Patch hide | download patch | download mbox

diff --git a/src/pulsecore/sink.c b/src/pulsecore/sink.c
index 9b6e46b..aba11aa 100644
--- a/src/pulsecore/sink.c
+++ b/src/pulsecore/sink.c
@@ -251,7 +251,6 @@  pa_sink* pa_sink_new(
     s->core = core;
     s->state = PA_SINK_INIT;
     s->flags = flags;
-    s->priority = 0;
     s->suspend_cause = data->suspend_cause;
     pa_sink_set_mixer_dirty(s, false);
     s->name = pa_xstrdup(name);
@@ -259,9 +258,6 @@  pa_sink* pa_sink_new(
     s->driver = pa_xstrdup(pa_path_get_filename(data->driver));
     s->module = data->module;
     s->card = data->card;
-
-    s->priority = pa_device_init_priority(s->proplist);
-
     s->sample_spec = data->sample_spec;
     s->channel_map = data->channel_map;
     s->default_sample_rate = s->sample_spec.rate;
@@ -309,10 +305,13 @@  pa_sink* pa_sink_new(
     if (!s->active_port)
         s->active_port = pa_device_port_find_best(s->ports);
 
-    if (s->active_port)
+    if (s->active_port) {
+        s->priority = s->active_port->priority;
         s->port_latency_offset = s->active_port->latency_offset;
-    else
+    } else {
+        s->priority = pa_device_init_priority(s->proplist);
         s->port_latency_offset = 0;
+    }
 
     s->save_volume = data->save_volume;
     s->save_muted = data->save_muted;
@@ -3375,6 +3374,13 @@  int pa_sink_set_port(pa_sink *s, const char *name, bool save) {
     s->active_port = port;
     s->save_port = save;
 
+    if (port->priority != s->priority) {
+        pa_log_info("%s: priority: %u -> %u", s->name, s->priority, port->priority);
+        s->priority = port->priority;
+
+        pa_core_update_default_sink(s->core);
+    }
+
     pa_sink_set_port_latency_offset(s, s->active_port->latency_offset);
 
     pa_hook_fire(&s->core->hooks[PA_CORE_HOOK_SINK_PORT_CHANGED], s);
diff --git a/src/pulsecore/source.c b/src/pulsecore/source.c
index 6292e28..5dae761 100644
--- a/src/pulsecore/source.c
+++ b/src/pulsecore/source.c
@@ -238,7 +238,6 @@  pa_source* pa_source_new(
     s->core = core;
     s->state = PA_SOURCE_INIT;
     s->flags = flags;
-    s->priority = 0;
     s->suspend_cause = data->suspend_cause;
     pa_source_set_mixer_dirty(s, false);
     s->name = pa_xstrdup(name);
@@ -246,9 +245,6 @@  pa_source* pa_source_new(
     s->driver = pa_xstrdup(pa_path_get_filename(data->driver));
     s->module = data->module;
     s->card = data->card;
-
-    s->priority = pa_device_init_priority(s->proplist);
-
     s->sample_spec = data->sample_spec;
     s->channel_map = data->channel_map;
     s->default_sample_rate = s->sample_spec.rate;
@@ -297,10 +293,13 @@  pa_source* pa_source_new(
     if (!s->active_port)
         s->active_port = pa_device_port_find_best(s->ports);
 
-    if (s->active_port)
+    if (s->active_port) {
+        s->priority = s->active_port->priority;
         s->port_latency_offset = s->active_port->latency_offset;
-    else
+    } else {
+        s->priority = pa_device_init_priority(s->proplist);
         s->port_latency_offset = 0;
+    }
 
     s->save_volume = data->save_volume;
     s->save_muted = data->save_muted;
@@ -2644,6 +2643,14 @@  int pa_source_set_port(pa_source *s, const char *name, bool save) {
     s->active_port = port;
     s->save_port = save;
 
+    if (port->priority != s->priority) {
+        pa_log_info("%s: priority: %u -> %u", s->name, s->priority, port->priority);
+        s->priority = port->priority;
+
+        /* The source priority may affect the default source choice. */
+        pa_core_update_default_source(s->core);
+    }
+
     pa_source_set_port_latency_offset(s, s->active_port->latency_offset);
 
     pa_hook_fire(&s->core->hooks[PA_CORE_HOOK_SOURCE_PORT_CHANGED], s);

Comments

On Thu, 8 Sep 2016, at 04:36 PM, Tanu Kaskinen wrote:
> It was reported in bug 93006 that on hardware that has HDMI and analog
> outputs on different cards, the HDMI sink is chosen by default even when
> headphones are plugged in. The headphone port has higher priority than
> the HDMI port, but that is not reflected in the sink priorities. This
> patch changes that - the sink/source priority is now the same as the
> active port priority, unless the sink/source doesn't have any ports, in
> which case the old priority logic is retained.
> 
> BugLink: https://bugs.freedesktop.org/show_bug.cgi?id=93006
> ---

I don't agree with this approach -- it conflates the priority values of
two different objects, which IMO is wrong.

I think we should adjust the sink priorities independently to make sure
we get the default setup that we want.

-- Arun
On Mon, 2017-01-30 at 14:54 +0530, Arun Raghavan wrote:
> On Thu, 8 Sep 2016, at 04:36 PM, Tanu Kaskinen wrote:
> > It was reported in bug 93006 that on hardware that has HDMI and analog
> > outputs on different cards, the HDMI sink is chosen by default even when
> > headphones are plugged in. The headphone port has higher priority than
> > the HDMI port, but that is not reflected in the sink priorities. This
> > patch changes that - the sink/source priority is now the same as the
> > active port priority, unless the sink/source doesn't have any ports, in
> > which case the old priority logic is retained.
> > 
> > BugLink: https://bugs.freedesktop.org/show_bug.cgi?id=93006
> > ---

As discussed in IRC, the analog sink should already have a higher
priority than the HDMI sink, so this patch shouldn't be needed to solve
the original problem. I don't know why HDMI was being preferred over
the analog sink. I still think the change is good, however.

> I don't agree with this approach -- it conflates the priority values of
> two different objects, which IMO is wrong.
> 
> I think we should adjust the sink priorities independently to make sure
> we get the default setup that we want.

Let's say there are two sinks, sink1 and sink2. Their active ports are
port1 and port2, respectively. Can you think of any situation where
you'd want to have higher priority on sink1 than sink2, if the priority
of port2 is higher than the priority of port1?

Here's a more contrived example:

sink1 has ports port1 and port3. Port1's priority is 1 and port3's
priority is 3.

sink2 has port2. port2's priority is 2.

When sink1's active port is port3, I'd want PulseAudio to use sink1,
because it has a better port than sink2. When port3 is unavailable and
sink1's active port is port1, I'd want PulseAudio to use sink2 by
default. If the sink priorities don't change when the port changes,
sink1 will always have a higher priority than sink2 or vice versa, so
the default sink will unavoidably be wrong some of the time.

Note that sink priorities are not used for anything else than choosing
the default sink. Clients don't use the sink priorities either, because
the sink priorities are not exported to the client API.
On Tue, 31 Jan 2017, at 08:01 PM, Tanu Kaskinen wrote:
> On Mon, 2017-01-30 at 14:54 +0530, Arun Raghavan wrote:
> > On Thu, 8 Sep 2016, at 04:36 PM, Tanu Kaskinen wrote:
> > > It was reported in bug 93006 that on hardware that has HDMI and analog
> > > outputs on different cards, the HDMI sink is chosen by default even when
> > > headphones are plugged in. The headphone port has higher priority than
> > > the HDMI port, but that is not reflected in the sink priorities. This
> > > patch changes that - the sink/source priority is now the same as the
> > > active port priority, unless the sink/source doesn't have any ports, in
> > > which case the old priority logic is retained.
> > > 
> > > BugLink: https://bugs.freedesktop.org/show_bug.cgi?id=93006
> > > ---
> 
> As discussed in IRC, the analog sink should already have a higher
> priority than the HDMI sink, so this patch shouldn't be needed to solve
> the original problem. I don't know why HDMI was being preferred over
> the analog sink. I still think the change is good, however.
> 
> > I don't agree with this approach -- it conflates the priority values of
> > two different objects, which IMO is wrong.
> > 
> > I think we should adjust the sink priorities independently to make sure
> > we get the default setup that we want.
> 
> Let's say there are two sinks, sink1 and sink2. Their active ports are
> port1 and port2, respectively. Can you think of any situation where
> you'd want to have higher priority on sink1 than sink2, if the priority
> of port2 is higher than the priority of port1?
> 
> Here's a more contrived example:
> 
> sink1 has ports port1 and port3. Port1's priority is 1 and port3's
> priority is 3.
> 
> sink2 has port2. port2's priority is 2.
> 
> When sink1's active port is port3, I'd want PulseAudio to use sink1,
> because it has a better port than sink2. When port3 is unavailable and
> sink1's active port is port1, I'd want PulseAudio to use sink2 by
> default. If the sink priorities don't change when the port changes,
> sink1 will always have a higher priority than sink2 or vice versa, so
> the default sink will unavoidably be wrong some of the time.
> 
> Note that sink priorities are not used for anything else than choosing
> the default sink. Clients don't use the sink priorities either, because
> the sink priorities are not exported to the client API.

This rationale seems to suggest dropping the sink priority altogether
and just using the port priority by default (with a default, low
priority for sinks without any ports)?

I realise this might be more work (quick grep suggests that most if it
is around the alsa module bits), but it feels like not having the basic
concept behind these  clearly defined will make this harder to reason
about in the future.

-- Arun
On Thu, 2017-02-02 at 13:02 +0530, Arun Raghavan wrote:
> On Tue, 31 Jan 2017, at 08:01 PM, Tanu Kaskinen wrote:
> > On Mon, 2017-01-30 at 14:54 +0530, Arun Raghavan wrote:
> > > I don't agree with this approach -- it conflates the priority values of
> > > two different objects, which IMO is wrong.
> > > 
> > > I think we should adjust the sink priorities independently to make sure
> > > we get the default setup that we want.
> > 
> > Let's say there are two sinks, sink1 and sink2. Their active ports are
> > port1 and port2, respectively. Can you think of any situation where
> > you'd want to have higher priority on sink1 than sink2, if the priority
> > of port2 is higher than the priority of port1?
> > 
> > Here's a more contrived example:
> > 
> > sink1 has ports port1 and port3. Port1's priority is 1 and port3's
> > priority is 3.
> > 
> > sink2 has port2. port2's priority is 2.
> > 
> > When sink1's active port is port3, I'd want PulseAudio to use sink1,
> > because it has a better port than sink2. When port3 is unavailable and
> > sink1's active port is port1, I'd want PulseAudio to use sink2 by
> > default. If the sink priorities don't change when the port changes,
> > sink1 will always have a higher priority than sink2 or vice versa, so
> > the default sink will unavoidably be wrong some of the time.
> > 
> > Note that sink priorities are not used for anything else than choosing
> > the default sink. Clients don't use the sink priorities either, because
> > the sink priorities are not exported to the client API.
> 
> This rationale seems to suggest dropping the sink priority altogether
> and just using the port priority by default (with a default, low
> priority for sinks without any ports)?
> 
> I realise this might be more work (quick grep suggests that most if it
> is around the alsa module bits), but it feels like not having the basic
> concept behind these  clearly defined will make this harder to reason
> about in the future.

The basic concept, as I see it, is that there are "outputs", and
portless sinks and ports are both instances of outputs. The priority is
a property of an output. Unfortunately, the code doesn't actually have
a concept of output that would cover both types, so we need to attach
the priority property to two types.

Dropping the sink priority should be quite easy, though. I can do that
if you really want - it will be easy enough to bring it back, if that
turns out to be necessary. The reason why we might want to bring it
back is that maybe we want to have different priorities with different
sinks. For example, a filter sink should probably have slightly higher
priority than the master sink. Currently we don't have such logic, so
there won't be any regressions caused by removing the sink priority
concept.