[pulseaudio-discuss] [PATCH v0 2/2] bluetooth: Request headset audio during profile switch

[Tanu Kaskinen]

On Sun, 2012-12-02 at 10:36 +0100, Mikel Astiz wrote:
> Hi Tanu,
> 
> On Sun, Dec 2, 2012 at 1:10 AM, Tanu Kaskinen <tanuk at iki.fi> wrote:
> > On Thu, 2012-11-29 at 14:28 +0100, Mikel Astiz wrote:
> >> From: Mikel Astiz <mikel.astiz at bmw-carit.de>
> >>
> >> When a headset is having a profile switch, we can either leave the
> >> SCO state unmodified (as it was before this patch) or we can
> >> alternatively request it (as older versions of PA).
> >>
> >> This patch tries to avoid a potential regression in case a module
> >> such as module-suspend-on-idle is not present, due to the provided
> >> resume-on-running policy. Without this patch, and without such a policy,
> >> the sink and sources would stay suspended until the user manually
> >> changed the suspend state.
> >
> > The last sentence is incorrect, the situation is worse than that. If the
> > suspend cause is IDLE, there's no way for the user to unsuspend the
> > device, because the user can only control the USER suspend cause.
> 
> OK, so this is worse than I thought.
> 
> >>
> >> A more sophisticated solution to this problem would be to revert this
> >> patch and implement the resume-on-running policy inside
> >> module-bluetooth-policy.
> >> ---
> >>  src/modules/bluetooth/module-bluetooth-device.c | 5 ++++-
> >>  1 file changed, 4 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/src/modules/bluetooth/module-bluetooth-device.c b/src/modules/bluetooth/module-bluetooth-device.c
> >> index d1c386f..66f00e0 100644
> >> --- a/src/modules/bluetooth/module-bluetooth-device.c
> >> +++ b/src/modules/bluetooth/module-bluetooth-device.c
> >> @@ -2010,7 +2010,10 @@ static int setup_transport(struct userdata *u) {
> >>      u->transport_removed_slot = pa_hook_connect(&t->hooks[PA_BLUETOOTH_TRANSPORT_HOOK_REMOVED], PA_HOOK_NORMAL,
> >>                                                  (pa_hook_cb_t) transport_removed_cb, u);
> >>
> >> -    bt_transport_acquire(u, FALSE);
> >> +    if (u->profile == PROFILE_HSP || u->profile == PROFILE_A2DP)
> >> +        bt_transport_acquire(u, TRUE);
> >> +    else
> >> +        bt_transport_acquire(u, FALSE);
> >>
> >>      bt_transport_config(u);
> >
> > Shouldn't the "data.suspend_cause = PA_SUSPEND_IDLE" assignment be
> > removed from add_sink()? The problem that we're fixing is that if the
> > device gets into the IDLE state, it's hosed if module-suspend-on-idle
> > isn't loaded. Therefore, setting the suspend cause to IDLE makes no
> > sense.
> >
> > I guess the change done in this patch makes sense anyway. What if
> > bt_transport_acquire() fails, shouldn't setup_transport() return an
> > error in that case?
> 
> If bt_transport_acquire() fails during card profile initialization,
> setup_transport() will succeed but the sink/source will be created
> suspended (with PA_SUSPEND_IDLE). So if module-suspend-on-idle is not
> present, it will remain suspended until the user makes some weird
> profile switch (hsp->off->hsp). So this is quite unlikely in practice
> but still bad.
> 
> From my point of view, the situation is the following:
> module-bluetooth-device implements a feature
> ("suspend-headset-sco-if-idle") that relies on module-suspend-on-idle.
> So the three options that I see are:
> 
> (a) Drop the feature completely (in this case PA_SUSPEND_IDLE would
> disappear from module-bluetooth-device).
> (b) Make a workaround by extending module-bluetooth-device such that
> it detects if module-suspend-on-idle has been loaded, and if yes
> support this feature, but not otherwise.
> (c) Move the resume-if-busy policy in module-suspend-on-idle
> (basically the call to pa_{source|sink}_suspend(d, FALSE,
> PA_SUSPEND_IDLE)) to either module-bluetooth-device,
> module-bluetooth-policy or the PA core.
> (d) Modify module-suspend-on-idle such that sink/sources that are
> created in IDLE state are automatically suspended, instead of waiting
> for the timer. Could this be achieved using
> PA_CORE_HOOK_{SOURCE|SINK}_PUT? Any side effect?
> 
> From what you're saying about returning an error in setup_transport(),
> I think you're proposing alternative (a). This sounds fine to me given
> the need to have a stable release asap, but in a longer term I think
> it's a pity not to include the feature, specially because of the
> battery drain that can be avoided if the user is not listening to any
> audio.

Yes, I propose that we drop the feature from 3.0. I agree that it would
be good to have a better solution in the long term.

> So for this longer term, what about option (c) and move the
> resume-if-busy policy to the core? I mean, isn't this part of the
> definition of PA_SUSPEND_IDLE?

That does have some appeal, and maybe we end up doing that, but I'd
propose option (d). The problem with the option (d) is that some
sink/source implementations rely on not being suspended during creation.
The alsa modules are a notable example. The sink parameters (sample rate
etc.) are not known until the device has been successfully opened. For
example, pulseaudio may try opening the device with 44100 Hz sample
rate, but if it fails, pulseaudio will retry with 48000 Hz. If the
device started suspended, the sink sample rate would be unknown until
the first stream connects to the sink.

The alsa modules could tell module-suspend-on-idle via a property that
the alsa devices must not start suspended. If that property is not set,
module-suspend-on-idle can set the device suspended during the device
creation.

-- 
Tanu