Re: [pulseaudio-discuss] [PATCH] Revert "bluetooth: Auto recover if profile is 'off'"
On Mon, 2017-06-19 at 11:43 +0300, Luiz Augusto von Dentz wrote:
> Hi Tanu,
>
> On Tue, Jun 13, 2017 at 9:39 PM, Georg Chini wrote:
> > On 13.06.2017 18:02, Tanu Kaskinen wrote:
> > >
> > > This reverts commit 69c212f8c13794f70899d1fe6baec1b6e3c92032.
> > >
> > > Reasons:
> > >
> > > The original reason for the patch was to work around some issue
> > > regarding the profile not connecting immediately (sorry, I don't really
> > > know the details), but that issue was fixed later by commit 998dfdf4cc,
> > > so the original reason doesn't apply any more.
> > >
> > > Automatically changing the profile when the transport state changes to
> > > PLAYING has traditionally been handled by module-bluetooth-policy, and
> > > as far as I can tell, there's no reason to change that.
> > >
> > > The assertion is unsafe. It's not guaranteed that the profile change
> > > will always succeed (at least pa_thread_mq_init() can fail due to
> > > reaching the maximum file descriptor limit).
> > > ---
> > > src/modules/bluetooth/module-bluez5-device.c | 8 +---
> > > 1 file changed, 1 insertion(+), 7 deletions(-)
> > >
> > > diff --git a/src/modules/bluetooth/module-bluez5-device.c
> > > b/src/modules/bluetooth/module-bluez5-device.c
> > > index 867def742..7e2b0d799 100644
> > > --- a/src/modules/bluetooth/module-bluez5-device.c
> > > +++ b/src/modules/bluetooth/module-bluez5-device.c
> > > @@ -2126,14 +2126,8 @@ static pa_hook_result_t
> > > transport_state_changed_cb(pa_bluetooth_discovery *y, pa
> > > if (t == u->transport && t->state <=
> > > PA_BLUETOOTH_TRANSPORT_STATE_DISCONNECTED)
> > > pa_assert_se(pa_card_set_profile(u->card,
> > > pa_hashmap_get(u->card->profiles, "off"), false) >= 0);
> > > -if (t->device == u->device) {
> > > -/* Auto recover from errors causing the profile to be set to off
> > > */
> > > -if (u->profile == PA_BLUETOOTH_PROFILE_OFF && t->state ==
> > > PA_BLUETOOTH_TRANSPORT_STATE_PLAYING) {
> > > -pa_log_debug("Switching to profile %s",
> > > pa_bluetooth_profile_to_string(t->profile));
> > > -pa_assert_se(pa_card_set_profile(u->card,
> > > pa_hashmap_get(u->card->profiles,
> > > pa_bluetooth_profile_to_string(t->profile)), false) >= 0);
> > > -}
>
> We could at very least put this logic into policy, since in most cases
> leaving the profile set to 'off' probably will cause bad user
> experience.
I'd rather not do that, unless you have a real-world use case that
requires it (so far it seems that you'd like to do that "just in
case").
--
Tanu
https://www.patreon.com/tanuk
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] [PATCH] Revert "bluetooth: Auto recover if profile is 'off'"
Hi Tanu,
On Tue, Jun 13, 2017 at 9:39 PM, Georg Chini wrote:
> On 13.06.2017 18:02, Tanu Kaskinen wrote:
>>
>> This reverts commit 69c212f8c13794f70899d1fe6baec1b6e3c92032.
>>
>> Reasons:
>>
>> The original reason for the patch was to work around some issue
>> regarding the profile not connecting immediately (sorry, I don't really
>> know the details), but that issue was fixed later by commit 998dfdf4cc,
>> so the original reason doesn't apply any more.
>>
>> Automatically changing the profile when the transport state changes to
>> PLAYING has traditionally been handled by module-bluetooth-policy, and
>> as far as I can tell, there's no reason to change that.
>>
>> The assertion is unsafe. It's not guaranteed that the profile change
>> will always succeed (at least pa_thread_mq_init() can fail due to
>> reaching the maximum file descriptor limit).
>> ---
>> src/modules/bluetooth/module-bluez5-device.c | 8 +---
>> 1 file changed, 1 insertion(+), 7 deletions(-)
>>
>> diff --git a/src/modules/bluetooth/module-bluez5-device.c
>> b/src/modules/bluetooth/module-bluez5-device.c
>> index 867def742..7e2b0d799 100644
>> --- a/src/modules/bluetooth/module-bluez5-device.c
>> +++ b/src/modules/bluetooth/module-bluez5-device.c
>> @@ -2126,14 +2126,8 @@ static pa_hook_result_t
>> transport_state_changed_cb(pa_bluetooth_discovery *y, pa
>> if (t == u->transport && t->state <=
>> PA_BLUETOOTH_TRANSPORT_STATE_DISCONNECTED)
>> pa_assert_se(pa_card_set_profile(u->card,
>> pa_hashmap_get(u->card->profiles, "off"), false) >= 0);
>> -if (t->device == u->device) {
>> -/* Auto recover from errors causing the profile to be set to off
>> */
>> -if (u->profile == PA_BLUETOOTH_PROFILE_OFF && t->state ==
>> PA_BLUETOOTH_TRANSPORT_STATE_PLAYING) {
>> -pa_log_debug("Switching to profile %s",
>> pa_bluetooth_profile_to_string(t->profile));
>> -pa_assert_se(pa_card_set_profile(u->card,
>> pa_hashmap_get(u->card->profiles,
>> pa_bluetooth_profile_to_string(t->profile)), false) >= 0);
>> -}
We could at very least put this logic into policy, since in most cases
leaving the profile set to 'off' probably will cause bad user
experience.
>> +if (t->device == u->device)
>> handle_transport_state_change(u, t);
>> -}
>> return PA_HOOK_OK;
>> }
>
>
> Looks good to me.
>
>
> ___
> pulseaudio-discuss mailing list
> pulseaudio-discuss@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
--
Luiz Augusto von Dentz
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] [PATCH] Revert "bluetooth: Auto recover if profile is 'off'"
On 13.06.2017 18:02, Tanu Kaskinen wrote:
This reverts commit 69c212f8c13794f70899d1fe6baec1b6e3c92032.
Reasons:
The original reason for the patch was to work around some issue
regarding the profile not connecting immediately (sorry, I don't really
know the details), but that issue was fixed later by commit 998dfdf4cc,
so the original reason doesn't apply any more.
Automatically changing the profile when the transport state changes to
PLAYING has traditionally been handled by module-bluetooth-policy, and
as far as I can tell, there's no reason to change that.
The assertion is unsafe. It's not guaranteed that the profile change
will always succeed (at least pa_thread_mq_init() can fail due to
reaching the maximum file descriptor limit).
---
src/modules/bluetooth/module-bluez5-device.c | 8 +---
1 file changed, 1 insertion(+), 7 deletions(-)
diff --git a/src/modules/bluetooth/module-bluez5-device.c
b/src/modules/bluetooth/module-bluez5-device.c
index 867def742..7e2b0d799 100644
--- a/src/modules/bluetooth/module-bluez5-device.c
+++ b/src/modules/bluetooth/module-bluez5-device.c
@@ -2126,14 +2126,8 @@ static pa_hook_result_t
transport_state_changed_cb(pa_bluetooth_discovery *y, pa
if (t == u->transport && t->state <=
PA_BLUETOOTH_TRANSPORT_STATE_DISCONNECTED)
pa_assert_se(pa_card_set_profile(u->card, pa_hashmap_get(u->card->profiles,
"off"), false) >= 0);
-if (t->device == u->device) {
-/* Auto recover from errors causing the profile to be set to off */
-if (u->profile == PA_BLUETOOTH_PROFILE_OFF && t->state ==
PA_BLUETOOTH_TRANSPORT_STATE_PLAYING) {
-pa_log_debug("Switching to profile %s",
pa_bluetooth_profile_to_string(t->profile));
-pa_assert_se(pa_card_set_profile(u->card,
pa_hashmap_get(u->card->profiles, pa_bluetooth_profile_to_string(t->profile)), false)
>= 0);
-}
+if (t->device == u->device)
handle_transport_state_change(u, t);
-}
return PA_HOOK_OK;
}
Looks good to me.
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
