[pulseaudio-discuss] [PATCH v5 5/7] bluetooth: Auto recover if profile is 'off'

[Tanu Kaskinen]

On Mon, 2017-05-08 at 11:26 +0300, Luiz Augusto von Dentz wrote:
> Hi Tanu,
> 
> On Sat, May 6, 2017 at 7:20 PM, Tanu Kaskinen <tanuk at iki.fi> wrote:
> > On Fri, 2017-05-05 at 16:41 +0300, Luiz Augusto von Dentz wrote:
> > > Hi Tanu,
> > > 
> > > On Thu, May 4, 2017 at 3:29 PM, Tanu Kaskinen <tanuk at iki.fi> wrote:
> > > > On Thu, 2017-05-04 at 12:58 +0300, Luiz Augusto von Dentz wrote:
> > > > > From: Luiz Augusto von Dentz <luiz.von.dentz at intel.com>
> > > > > 
> > > > > This means something went wrong, which in case of ofono backend it is
> > > > > probably due to the profile not connecting immediately, but it can be
> > > > > safely restored in that case the transport is playing which means the
> > > > > profile has recovered connectivity.
> > > > > ---
> > > > >  src/modules/bluetooth/module-bluez5-device.c | 8 +++++++-
> > > > >  1 file changed, 7 insertions(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/src/modules/bluetooth/module-bluez5-device.c b/src/modules/bluetooth/module-bluez5-device.c
> > > > > index a96da17..2f0ec97 100644
> > > > > --- a/src/modules/bluetooth/module-bluez5-device.c
> > > > > +++ b/src/modules/bluetooth/module-bluez5-device.c
> > > > > @@ -2060,8 +2060,14 @@ 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)
> > > > > +    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);
> > > > 
> > > > Regarding the assertion, how do you know that the card profile switch
> > > > will always succeed? Is there no IO involved? If you somehow know that
> > > > it will always succeed, there should be a comment explaining how you
> > > > know that.
> > > 
> > > The playing state should guarantee the fd is available, so there
> > > shouldn't be any problem in this regard.
> > 
> > At least start_thread() can fail, and that will make the profile change
> > fail.
> > 
> > > > I know there are already assertions when switching to the off profile,
> > > > but that's a special profile - activating it should never fail.
> > > > 
> > > > This change seems dubious in another way too. So far we've kept all
> > > > automatic profile switch code out of module-bluez5-device (apart from
> > > > switches to "off" when things fail). Automatic switching is handled by
> > > > module-bluetooth-policy instead. If I understood correctly, this is a
> > > > fix for a situation where we already tried to switch to a profile, but
> > > > it failed first. The code is lacking any checks that would do the
> > > > profile change only if there really was a prior profile switch attempt.
> > > 
> > > It is exactly to counter the profiles switching 'off' which is done in
> > > the device itself, we could of course move this to policy as well
> > > though. Note that in most cases it is better to switch to a working
> > > profile than 'off' since that for sure won't render anything.
> > 
> > If we try to switch to some profile and it fails and as a result the
> > profile gets set to "off", then I think it's fine to fix that problem
> > in module-bluez5-device, but as I said, there's nothing in this patch
> > that would ensure that this is done *only* when this situation occurs.
> > If there was no problem earlier, then module-bluez5-device shouldn't
> > automatically change its profile, that kind of stuff belongs to module-
> > bluetooth-policy.
> 
> Profiles can be connected from both ends, lets say the policy attempt
> to switch profile, lets say from A2DP to HFP, and for some reason it
> fails, the result would be "off" profile but then at some point the
> remote device decides to play some audio, not A2DP is playing but
> because the current profile is 'off' nothing is heard.

module-bluetooth-policy already handles this case.

> > Georg told me that this patch isn't needed anyway, after the "bluez5-
> > device: Correctly handle suspend/resume cyle for audio gateway role of
> > ofono backend" patch. If that's true, then can we just revert this?
> 
> It is not needed for regular cases, though what I just said above is
> still valid,

See the above comment.

> also note that in both A2DP and HFP the device may refuse
> to resume

Are you talking specifically about the case where pulseaudio acts as an
A2DP source or HFP audio gateway? In these cases switching to off seems
like the right approach. We don't know when the remote will again allow
streaming, so the user has to manually try again.

If resuming fails, can the remote even initiate streaming without
pulseaudio doing anything? If the remote can initiate streaming, then
maybe module-bluetooth-policy should do something, although I don't see
what the use case would be.

> or it can sometimes have a different policy, disabling
> inband ringtone for example can prevent SCO to connect until the call
> is actually in place but once it has been answer it would resume SCO
> but at that point the profile would be set to 'off'.

Can you explain this "disable inband ringtone" case in more detail?
What role (GW or HF) does pulseaudio have? What steps are in the
process?

-- 
Tanu

https://www.patreon.com/tanuk