Re: [pulseaudio-discuss] [PATCH v0] bluetooth: Merge headset ports into one
On Mon, 2012-11-26 at 15:06 +0100, Mikel Astiz wrote: > Hi Tanu, > > On Mon, Nov 26, 2012 at 2:52 PM, Tanu Kaskinen wrote: > > module-bluetooth-policy has all information it needs available from > > bluetooth-util, doesn't it? It doesn't necessarily need to even care > > about the port availability, it could just directly follow the profile > > state (connected/playing). > > Actually this kind of information is missing in the bluetooth-util > library. Have a look at how module-bluetooth-device is directly > handling the property changes in filter_cb(). This can be improved of > course (patches coming soon) but I doubt it's worth addressing this > minor detail for 3.0. Fair enough. > > Am I right that the profile switching logic isn't meant for headsets > > anyway? The use case for which the code was written was enabling > > hfgw/a2dp_source when those profiles entered the "playing" state without > > pulseaudio initiating that state change, or do I remember wrong? Maybe > > module-bluetooth-policy could just check that is the port that became > > available hfgw or a2dp_source, that way there would be no risk of > > messing up headset use cases. > > Yes, that could be a possible workaround. Afaik headsets would rarely > resume the audio for a specific profile, at least in desktop > use-cases. > > As you say, we could disable the policy based on the profile to avoid > messing up with headsets, but doesn't this raise the question whether > module-bluetooth-policy should be loaded at all? I don't think the > average user will be doing both gateway and headset roles. I think playing music from a phone via bluetooth to a PC is common enough use case to justify loading module-bluetooth-policy by default. -- Tanu ___ pulseaudio-discuss mailing list pulseaudio-discuss@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] [PATCH v0] bluetooth: Merge headset ports into one
Hi Tanu, On Mon, Nov 26, 2012 at 2:52 PM, Tanu Kaskinen wrote: > On Sun, 2012-11-25 at 12:28 +0100, Mikel Astiz wrote: >> Hi Tanu, >> >> On Fri, Nov 23, 2012 at 8:26 PM, Tanu Kaskinen wrote: >> > On Fri, 2012-11-23 at 13:41 +0100, Mikel Astiz wrote: >> >> From: Mikel Astiz >> >> >> >> Merge the former "hsp-output" and "a2dp-output" ports into one single >> >> port, in order to fix the regression of having several independent >> >> entries in the UI. >> > >> > I'm not sure if this is anything serious, but with this change, I think >> > (I haven't actually tested) module-bluetooth-policy works with headsets >> > that support both profiles as follows, not very smartly: >> > >> > - It never does anything until both profiles enter the "playing" state. >> >> That was not the intention. The patch should set the port to >> PORT_AVAILABLE_YES if *any* of the two profiles is set, and thus the >> port switch could be triggered. > > Ah, so it seems, my mistake. > >> > I'm not sure if this scenario is possible. Can headsets initiate the >> > "playing" state for one profile while the other profile is already >> > "playing" too? >> >> It's very unlikely, I haven't seen that yet. >> >> > >> > - If both profiles enter the "playing" state, then semi-randomly one or >> > the other will be activated. >> >> As stated above, this will happen if any of the two profiles starts >> playing *and* the active profile is "off". But you're right, the >> activated profile will be semi-random, i.e. the policy is not very >> smart. >> >> I can't think of any workaround to this problem once the ports get >> merged, since module-bluetooth-policy is not able to distinguish the >> state of each Bluetooth profile. > > module-bluetooth-policy has all information it needs available from > bluetooth-util, doesn't it? It doesn't necessarily need to even care > about the port availability, it could just directly follow the profile > state (connected/playing). Actually this kind of information is missing in the bluetooth-util library. Have a look at how module-bluetooth-device is directly handling the property changes in filter_cb(). This can be improved of course (patches coming soon) but I doubt it's worth addressing this minor detail for 3.0. > > Am I right that the profile switching logic isn't meant for headsets > anyway? The use case for which the code was written was enabling > hfgw/a2dp_source when those profiles entered the "playing" state without > pulseaudio initiating that state change, or do I remember wrong? Maybe > module-bluetooth-policy could just check that is the port that became > available hfgw or a2dp_source, that way there would be no risk of > messing up headset use cases. Yes, that could be a possible workaround. Afaik headsets would rarely resume the audio for a specific profile, at least in desktop use-cases. As you say, we could disable the policy based on the profile to avoid messing up with headsets, but doesn't this raise the question whether module-bluetooth-policy should be loaded at all? I don't think the average user will be doing both gateway and headset roles. Cheers, Mikel ___ pulseaudio-discuss mailing list pulseaudio-discuss@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] [PATCH v0] bluetooth: Merge headset ports into one
On Sun, 2012-11-25 at 12:28 +0100, Mikel Astiz wrote: > Hi Tanu, > > On Fri, Nov 23, 2012 at 8:26 PM, Tanu Kaskinen wrote: > > On Fri, 2012-11-23 at 13:41 +0100, Mikel Astiz wrote: > >> From: Mikel Astiz > >> > >> Merge the former "hsp-output" and "a2dp-output" ports into one single > >> port, in order to fix the regression of having several independent > >> entries in the UI. > > > > I'm not sure if this is anything serious, but with this change, I think > > (I haven't actually tested) module-bluetooth-policy works with headsets > > that support both profiles as follows, not very smartly: > > > > - It never does anything until both profiles enter the "playing" state. > > That was not the intention. The patch should set the port to > PORT_AVAILABLE_YES if *any* of the two profiles is set, and thus the > port switch could be triggered. Ah, so it seems, my mistake. > > I'm not sure if this scenario is possible. Can headsets initiate the > > "playing" state for one profile while the other profile is already > > "playing" too? > > It's very unlikely, I haven't seen that yet. > > > > > - If both profiles enter the "playing" state, then semi-randomly one or > > the other will be activated. > > As stated above, this will happen if any of the two profiles starts > playing *and* the active profile is "off". But you're right, the > activated profile will be semi-random, i.e. the policy is not very > smart. > > I can't think of any workaround to this problem once the ports get > merged, since module-bluetooth-policy is not able to distinguish the > state of each Bluetooth profile. module-bluetooth-policy has all information it needs available from bluetooth-util, doesn't it? It doesn't necessarily need to even care about the port availability, it could just directly follow the profile state (connected/playing). Am I right that the profile switching logic isn't meant for headsets anyway? The use case for which the code was written was enabling hfgw/a2dp_source when those profiles entered the "playing" state without pulseaudio initiating that state change, or do I remember wrong? Maybe module-bluetooth-policy could just check that is the port that became available hfgw or a2dp_source, that way there would be no risk of messing up headset use cases. -- Tanu ___ pulseaudio-discuss mailing list pulseaudio-discuss@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] [PATCH v0] bluetooth: Merge headset ports into one
Hi Tanu, On Fri, Nov 23, 2012 at 8:26 PM, Tanu Kaskinen wrote: > On Fri, 2012-11-23 at 13:41 +0100, Mikel Astiz wrote: >> From: Mikel Astiz >> >> Merge the former "hsp-output" and "a2dp-output" ports into one single >> port, in order to fix the regression of having several independent >> entries in the UI. > > I'm not sure if this is anything serious, but with this change, I think > (I haven't actually tested) module-bluetooth-policy works with headsets > that support both profiles as follows, not very smartly: > > - It never does anything until both profiles enter the "playing" state. That was not the intention. The patch should set the port to PORT_AVAILABLE_YES if *any* of the two profiles is set, and thus the port switch could be triggered. > I'm not sure if this scenario is possible. Can headsets initiate the > "playing" state for one profile while the other profile is already > "playing" too? It's very unlikely, I haven't seen that yet. > > - If both profiles enter the "playing" state, then semi-randomly one or > the other will be activated. As stated above, this will happen if any of the two profiles starts playing *and* the active profile is "off". But you're right, the activated profile will be semi-random, i.e. the policy is not very smart. I can't think of any workaround to this problem once the ports get merged, since module-bluetooth-policy is not able to distinguish the state of each Bluetooth profile. > To me it looks like this is not serious, so this is not a blocker issue > for accepting the patch. > Cheers, Mikel ___ pulseaudio-discuss mailing list pulseaudio-discuss@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] [PATCH v0] bluetooth: Merge headset ports into one
On Fri, 2012-11-23 at 13:41 +0100, Mikel Astiz wrote: > From: Mikel Astiz > > Merge the former "hsp-output" and "a2dp-output" ports into one single > port, in order to fix the regression of having several independent > entries in the UI. I'm not sure if this is anything serious, but with this change, I think (I haven't actually tested) module-bluetooth-policy works with headsets that support both profiles as follows, not very smartly: - It never does anything until both profiles enter the "playing" state. I'm not sure if this scenario is possible. Can headsets initiate the "playing" state for one profile while the other profile is already "playing" too? - If both profiles enter the "playing" state, then semi-randomly one or the other will be activated. To me it looks like this is not serious, so this is not a blocker issue for accepting the patch. -- Tanu ___ pulseaudio-discuss mailing list pulseaudio-discuss@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] [PATCH v0] bluetooth: Merge headset ports into one
Hi David,
On Fri, Nov 23, 2012 at 2:00 PM, David Henningsson
wrote:
> On 11/23/2012 01:41 PM, Mikel Astiz wrote:
>>
>> From: Mikel Astiz
>>
>> Merge the former "hsp-output" and "a2dp-output" ports into one single
>> port, in order to fix the regression of having several independent
>> entries in the UI.
>> ---
>> I did a fairly limited amount of testing so please review carefully.
>> src/modules/bluetooth/module-bluetooth-device.c | 72
>> ++---
>> 1 file changed, 52 insertions(+), 20 deletions(-)
>>
>> diff --git a/src/modules/bluetooth/module-bluetooth-device.c
>> b/src/modules/bluetooth/module-bluetooth-device.c
>> index dd1bb86..506a479 100644
>> --- a/src/modules/bluetooth/module-bluetooth-device.c
>> +++ b/src/modules/bluetooth/module-bluetooth-device.c
>> @@ -1281,6 +1281,15 @@ static pa_port_available_t
>> audio_state_to_availability(pa_bt_audio_state_t state
>> return PA_PORT_AVAILABLE_UNKNOWN;
>> }
>>
>> +static pa_port_available_t
>> audio_state_to_availability_merged(pa_bt_audio_state_t state1,
>> pa_bt_audio_state_t state2) {
>> +if (state1 < PA_BT_AUDIO_STATE_CONNECTED && state2 <
>> PA_BT_AUDIO_STATE_CONNECTED)
>> +return PA_PORT_AVAILABLE_NO;
>> +else if (state1 >= PA_BT_AUDIO_STATE_PLAYING || state2 >=
>> PA_BT_AUDIO_STATE_PLAYING)
>> +return PA_PORT_AVAILABLE_YES;
>> +else
>> +return PA_PORT_AVAILABLE_UNKNOWN;
>> +}
>
>
> We might need talk about this too. If something is PA_PORT_AVAILABLE_NO, it
> completely disappears from the GUI, so it can't be activated. Just like it
> does not make sense to switch to a pair of headphones which are not
> connected.
>
> Is this expected?
I think yes. If no audio profile is connected, there is no way you can
use the headset from PA. In fact, you don't even know if the device is
physically there.
You need to connect it first using BlueZ, so I think it makes sense
that is disappears from the UI.
>> +
>> /* Run from main thread */
>> static DBusHandlerResult filter_cb(DBusConnection *bus, DBusMessage *m,
>> void *userdata) {
>> DBusError err;
>> @@ -1356,9 +1365,14 @@ static DBusHandlerResult filter_cb(DBusConnection
>> *bus, DBusMessage *m, void *us
>>
>> if (state != PA_BT_AUDIO_STATE_INVALID &&
>> pa_hashmap_get(u->card->profiles, "hsp")) {
>> pa_device_port *port;
>> -pa_port_available_t available =
>> audio_state_to_availability(state);
>> +pa_port_available_t available;
>>
>> -pa_assert_se(port = pa_hashmap_get(u->card->ports,
>> "hsp-output"));
>> +if (pa_hashmap_get(u->card->profiles, "a2dp") == NULL)
>> +available = audio_state_to_availability(state);
>> +else
>> +available = audio_state_to_availability_merged(state,
>> u->device->audio_sink_state);
>> +
>> +pa_assert_se(port = pa_hashmap_get(u->card->ports,
>> "bluetooth-output"));
>> pa_device_port_set_available(port, available);
>>
>> pa_assert_se(port = pa_hashmap_get(u->card->ports,
>> "hsp-input"));
>> @@ -1385,9 +1399,14 @@ static DBusHandlerResult filter_cb(DBusConnection
>> *bus, DBusMessage *m, void *us
>>
>> if (state != PA_BT_AUDIO_STATE_INVALID &&
>> pa_hashmap_get(u->card->profiles, "a2dp")) {
>> pa_device_port *port;
>> -pa_port_available_t available =
>> audio_state_to_availability(state);
>> +pa_port_available_t available;
>> +
>> +if (pa_hashmap_get(u->card->profiles, "hsp") == NULL)
>> +available = audio_state_to_availability(state);
>> +else
>> +available = audio_state_to_availability_merged(state,
>> u->device->headset_state);
>>
>> -pa_assert_se(port = pa_hashmap_get(u->card->ports,
>> "a2dp-output"));
>> +pa_assert_se(port = pa_hashmap_get(u->card->ports,
>> "bluetooth-output"));
>> pa_device_port_set_available(port, available);
>>
>> acquire = (available == PA_PORT_AVAILABLE_YES && u->profile
>> == PROFILE_A2DP);
>> @@ -1614,7 +1633,7 @@ static void connect_ports(struct userdata *u, void
>> *sink_or_source_new_data, pa_
>>
>> switch (u->profile) {
>> case PROFILE_A2DP:
>> -pa_assert_se(port = pa_hashmap_get(u->card->ports,
>> "a2dp-output"));
>> +pa_assert_se(port = pa_hashmap_get(u->card->ports,
>> "bluetooth-output"));
>> pa_assert_se(pa_hashmap_put(data.sink_new_data->ports,
>> port->name, port) >= 0);
>> pa_device_port_ref(port);
>> break;
>> @@ -1627,7 +1646,7 @@ static void connect_ports(struct userdata *u, void
>> *sink_or_source_new_data, pa_
>>
>> case PROFILE_HSP:
>> if (direction == PA_DIRECTION_OUTPUT) {
>> -pa_assert_se(port = pa_hashmap_get(u->card->ports,
>> "hsp-output"));
>> +pa_assert_se(port = pa_hashmap_get(u->card->ports,
>>
Re: [pulseaudio-discuss] [PATCH v0] bluetooth: Merge headset ports into one
On 11/23/2012 01:41 PM, Mikel Astiz wrote:
From: Mikel Astiz
Merge the former "hsp-output" and "a2dp-output" ports into one single
port, in order to fix the regression of having several independent
entries in the UI.
---
I did a fairly limited amount of testing so please review carefully.
src/modules/bluetooth/module-bluetooth-device.c | 72 ++---
1 file changed, 52 insertions(+), 20 deletions(-)
diff --git a/src/modules/bluetooth/module-bluetooth-device.c
b/src/modules/bluetooth/module-bluetooth-device.c
index dd1bb86..506a479 100644
--- a/src/modules/bluetooth/module-bluetooth-device.c
+++ b/src/modules/bluetooth/module-bluetooth-device.c
@@ -1281,6 +1281,15 @@ static pa_port_available_t
audio_state_to_availability(pa_bt_audio_state_t state
return PA_PORT_AVAILABLE_UNKNOWN;
}
+static pa_port_available_t
audio_state_to_availability_merged(pa_bt_audio_state_t state1,
pa_bt_audio_state_t state2) {
+if (state1 < PA_BT_AUDIO_STATE_CONNECTED && state2 <
PA_BT_AUDIO_STATE_CONNECTED)
+return PA_PORT_AVAILABLE_NO;
+else if (state1 >= PA_BT_AUDIO_STATE_PLAYING || state2 >=
PA_BT_AUDIO_STATE_PLAYING)
+return PA_PORT_AVAILABLE_YES;
+else
+return PA_PORT_AVAILABLE_UNKNOWN;
+}
We might need talk about this too. If something is PA_PORT_AVAILABLE_NO,
it completely disappears from the GUI, so it can't be activated. Just
like it does not make sense to switch to a pair of headphones which are
not connected.
Is this expected?
+
/* Run from main thread */
static DBusHandlerResult filter_cb(DBusConnection *bus, DBusMessage *m, void
*userdata) {
DBusError err;
@@ -1356,9 +1365,14 @@ static DBusHandlerResult filter_cb(DBusConnection *bus,
DBusMessage *m, void *us
if (state != PA_BT_AUDIO_STATE_INVALID && pa_hashmap_get(u->card->profiles,
"hsp")) {
pa_device_port *port;
-pa_port_available_t available = audio_state_to_availability(state);
+pa_port_available_t available;
-pa_assert_se(port = pa_hashmap_get(u->card->ports, "hsp-output"));
+if (pa_hashmap_get(u->card->profiles, "a2dp") == NULL)
+available = audio_state_to_availability(state);
+else
+available = audio_state_to_availability_merged(state,
u->device->audio_sink_state);
+
+pa_assert_se(port = pa_hashmap_get(u->card->ports,
"bluetooth-output"));
pa_device_port_set_available(port, available);
pa_assert_se(port = pa_hashmap_get(u->card->ports, "hsp-input"));
@@ -1385,9 +1399,14 @@ static DBusHandlerResult filter_cb(DBusConnection *bus,
DBusMessage *m, void *us
if (state != PA_BT_AUDIO_STATE_INVALID && pa_hashmap_get(u->card->profiles,
"a2dp")) {
pa_device_port *port;
-pa_port_available_t available = audio_state_to_availability(state);
+pa_port_available_t available;
+
+if (pa_hashmap_get(u->card->profiles, "hsp") == NULL)
+available = audio_state_to_availability(state);
+else
+available = audio_state_to_availability_merged(state,
u->device->headset_state);
-pa_assert_se(port = pa_hashmap_get(u->card->ports, "a2dp-output"));
+pa_assert_se(port = pa_hashmap_get(u->card->ports,
"bluetooth-output"));
pa_device_port_set_available(port, available);
acquire = (available == PA_PORT_AVAILABLE_YES && u->profile ==
PROFILE_A2DP);
@@ -1614,7 +1633,7 @@ static void connect_ports(struct userdata *u, void
*sink_or_source_new_data, pa_
switch (u->profile) {
case PROFILE_A2DP:
-pa_assert_se(port = pa_hashmap_get(u->card->ports, "a2dp-output"));
+pa_assert_se(port = pa_hashmap_get(u->card->ports,
"bluetooth-output"));
pa_assert_se(pa_hashmap_put(data.sink_new_data->ports, port->name,
port) >= 0);
pa_device_port_ref(port);
break;
@@ -1627,7 +1646,7 @@ static void connect_ports(struct userdata *u, void
*sink_or_source_new_data, pa_
case PROFILE_HSP:
if (direction == PA_DIRECTION_OUTPUT) {
-pa_assert_se(port = pa_hashmap_get(u->card->ports,
"hsp-output"));
+pa_assert_se(port = pa_hashmap_get(u->card->ports,
"bluetooth-output"));
pa_assert_se(pa_hashmap_put(data.sink_new_data->ports,
port->name, port) >= 0);
} else {
pa_assert_se(port = pa_hashmap_get(u->card->ports,
"hsp-input"));
@@ -2247,13 +2266,20 @@ static void create_ports_for_profile(struct userdata
*u, pa_hashmap *ports, pa_c
switch (*d) {
case PROFILE_A2DP:
-pa_assert_se(port = pa_device_port_new(u->core, "a2dp-output",
_("Bluetooth High Quality (A2DP)"), 0));
-pa_assert_se(pa_hashmap_put(ports, port->name, port) >= 0);
-port->is_output = 1;
-port->is_in
