[pulseaudio-discuss] [PATCH] card-restore: Don't restore profile on Bluetooth cards by default

[João Paulo Rechi Vita]

Hello Tanu and Luiz,

On Sat, Jul 14, 2018 at 11:57 PM, Tanu Kaskinen <tanuk at iki.fi> wrote:
> On Tue, 2018-07-10 at 13:10 +0300, Luiz Augusto von Dentz wrote:
>> Hi Joao,
>>
>> On Tue, Jul 10, 2018 at 5:23 AM, João Paulo Rechi Vita
>> <jprvita at gmail.com> wrote:
>> > We can provide a better overall user experience with Bluetooth
>> > cards by
>> > always choosing the higher audio quality profile (A2DP) by default
>> > and
>> > updating the profile selection dynamically according to which
>> > streams
>> > are active at a certain moment. The default initial selection has
>> > been
>> > addressed by "85daab272 bluetooth: set better priorities for
>> > profiles"
>> > and the dynamic profile selection is covered by module-bluetooth-
>> > policy.
>> >
>> > In addition, module-card-restore's database entries for Bluetooth
>> > devices
>> > are retained after a device is removed from the system, leading to
>> > the
>> > previously selected profile being restored after a new pairing with
>> > the
>> > same device, with no way for the user to erase this memory and
>> > reset the
>> > default profile except manually fiddling with module-card-restore's
>> > database.
>> >
>> > This commit adds a module argument to have module-card-restore
>> > ignore
>> > Bluetooth profiles and this behavior is set as default.
>>
>> I would have it done differently, just detect if
>> module-bluetooth-policy is loaded instead of having a argument since
>> on platforms not using module-bluetooth-policy it might be useful to
>> restore.
>
> João, are you willing to implement that instead? I'm fine with either
> approach, but Luiz's idea seems a bit better. Note that if you're going
> to implement it, there's the added complication of needing to monitor
> when modules are loaded and unloaded, it's not enough to check it only
> during initialization.
>

I agree with the idea that one could want to have module-card-restore
work on Bluetooth cards when module-bluetooth-policy is not loaded.
But I am not sure tying on module's behavior to whether or not a
different module is loaded or not is a good approach. I think it may
complicate debugging, as when analyzing module-card-restore behavior
you'd have to also think about module-bluetooth-policy as well
(although you need to think about both nowadays when looking at the
bigger picture of Bluetooth cards). I also think is a bit
non-intuitive for users, specially when you think of a user that
disables loading module-bluetooth-policy to avoid its behavior, and
then she gets a different behavior from module-card-restore (which is
she can't disable unless there is also a module argument for that).

In addition, with Luiz' proposal one can't have the current behavior
of having module-bluetooth-policy doing its dynamic profile change
dependent on the active streams AND have module-card-restore set the
initial profile to the saved one, which I believed was Tanu's reason
to have a module argument in the first place. But on his last comment
he says he's fine with either approach, so I probably guessed the
rationale wrongly.

I personally prefer having a static module argument than doing the
dynamic detection.

> One comment about the code below:
>
>> > ---
>> >  src/modules/module-card-restore.c | 20 +++++++++++++++++++-
>> >  1 file changed, 19 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/src/modules/module-card-restore.c
>> > b/src/modules/module-card-restore.c
>> > index 014e75435..10e6f4292 100644
>> > --- a/src/modules/module-card-restore.c
>> > +++ b/src/modules/module-card-restore.c
>> > @@ -48,11 +48,14 @@ PA_MODULE_AUTHOR("Lennart Poettering");
>> >  PA_MODULE_DESCRIPTION("Automatically restore profile of cards");
>> >  PA_MODULE_VERSION(PACKAGE_VERSION);
>> >  PA_MODULE_LOAD_ONCE(true);
>> > +PA_MODULE_USAGE(
>> > +    "restore_bluetooth_profile=<boolean>"
>> > +);
>> >
>> >  #define SAVE_INTERVAL (10 * PA_USEC_PER_SEC)
>> >
>> >  static const char* const valid_modargs[] = {
>> > -    NULL
>> > +    "restore_bluetooth_profile=<boolean>"
>> >  };
>> >
>> >  struct userdata {
>> > @@ -60,6 +63,7 @@ struct userdata {
>> >      pa_module *module;
>> >      pa_time_event *save_time_event;
>> >      pa_database *database;
>> > +    bool restore_bluetooth_profile;
>> >  };
>> >
>> >  #define ENTRY_VERSION 4
>> > @@ -554,6 +558,12 @@ static pa_hook_result_t
>> > card_choose_initial_profile_callback(pa_core *core, pa_c
>> >      if (!(e = entry_read(u, card->name)))
>> >          return PA_HOOK_OK;
>> >
>> > +    if (!u->restore_bluetooth_profile) {
>> > +        const char *s = pa_proplist_gets(card->proplist,
>> > PA_PROP_DEVICE_BUS);
>> > +        if (!s || pa_streq(s, "bluetooth"))
>
> The condition should be
>
>     s && pa_streq(s, "bluetooth")
>
> or
>
>     pa_safe_streq(s, "bluetooth")
>
> because we should skip the restoring only for bluetooth cards, not for
> cards with an unknown bus.
>

Thanks! I'll fix it and send an updated version in case we decide to
stick with this approach.

--
João Paulo Rechi Vita
http://about.me/jprvita