[pulseaudio-discuss] [RFC 1/2] bluetooth: Add MediaTransport volume management

[Tanu Kaskinen]

On Mon, 2012-11-12 at 16:19 +0100, Frédéric Danis wrote:
> With BlueZ 5 volume updates for HFP will go through transport interface.
> 
> Note that this is backward compatible.
> ---
>  src/modules/bluetooth/bluetooth-util.c          |   62 +++++++++++++++----
>  src/modules/bluetooth/bluetooth-util.h          |    4 ++
>  src/modules/bluetooth/module-bluetooth-device.c |   75 +++++++++++++++++++++++
>  3 files changed, 130 insertions(+), 11 deletions(-)
> 
> diff --git a/src/modules/bluetooth/bluetooth-util.c b/src/modules/bluetooth/bluetooth-util.c
> index 272b6ce..c0d5287 100644
> --- a/src/modules/bluetooth/bluetooth-util.c
> +++ b/src/modules/bluetooth/bluetooth-util.c
> @@ -754,21 +754,49 @@ int pa_bluetooth_transport_parse_property(pa_bluetooth_transport *t, DBusMessage
>  
>      dbus_message_iter_recurse(i, &variant_i);
>  
> -    switch (dbus_message_iter_get_arg_type(&variant_i)) {
> +    if (pa_streq(key, "NREC")) {
> +        dbus_bool_t value;
>  
> -        case DBUS_TYPE_BOOLEAN: {
> +        if (dbus_message_iter_get_arg_type(&variant_i) != DBUS_TYPE_BOOLEAN) {
> +            pa_log("Property value not a boolean.");

This is a bit generic. There could be multiple places where this is
printed (this seems to be the only place currently, though). I suggest
"'NREC' property value not a boolean." I know that there are other
places with similarly generic log messages, but I don't think those
messages are worth copying, even in the name of consistency.

Same comment applies to the other new log messages.

> +            return -1;
> +        }
>  
> -            dbus_bool_t value;
> -            dbus_message_iter_get_basic(&variant_i, &value);
> +        dbus_message_iter_get_basic(&variant_i, &value);
>  
> -            if (pa_streq(key, "NREC") && t->nrec != value) {
> -                t->nrec = value;
> -                pa_log_debug("Transport %s: Property 'NREC' changed to %s.", t->path, t->nrec ? "True" : "False");
> -                pa_hook_fire(&t->hooks[PA_BLUETOOTH_TRANSPORT_HOOK_NREC_CHANGED], NULL);
> -            }
> +        if (t->nrec != value) {
> +            t->nrec = value;
> +            pa_log_debug("Transport %s: Property 'NREC' changed to %s.", t->path, t->nrec ? "True" : "False");
> +            pa_hook_fire(&t->hooks[PA_BLUETOOTH_TRANSPORT_HOOK_NREC_CHANGED], NULL);
> +        }
> +    } else if(pa_streq(key,"OutputGain")) {

Spaces missing after "if" and "key,"

> +        uint16_t value;
>  
> -            break;
> -         }
> +        if (dbus_message_iter_get_arg_type(&variant_i) != DBUS_TYPE_UINT16) {
> +            pa_log("Property value not an uint16.");
> +            return -1;
> +        }
> +
> +        dbus_message_iter_get_basic(&variant_i, &value);
> +
> +        if (t->output_gain != value) {
> +            t->output_gain = value;
> +            pa_hook_fire(&t->hooks[PA_BLUETOOTH_TRANSPORT_HOOK_OUTPUT_GAIN_CHANGED], NULL);

I'd like to have a debug log message about the changed value similar to
what the NREC handler prints.

> +        }
> +    } else if(pa_streq(key,"InputGain")) {

More missing spaces.

> +        uint16_t value;
> +
> +        if (dbus_message_iter_get_arg_type(&variant_i) != DBUS_TYPE_UINT16) {
> +            pa_log("Property value not an uint16.");
> +            return -1;
> +        }
> +
> +        dbus_message_iter_get_basic(&variant_i, &value);
> +
> +        if (t->input_gain != value) {
> +            t->input_gain = value;
> +            pa_hook_fire(&t->hooks[PA_BLUETOOTH_TRANSPORT_HOOK_INPUT_GAIN_CHANGED], NULL);

I suggest a debug log message here too.

> +static void send_property_update(struct userdata *u, const char *name, int type, void *data)
> +{
> +    DBusMessage *m;
> +    DBusMessageIter iter;
> +
> +    pa_assert_se(m = dbus_message_new_method_call("org.bluez", u->transport->path, "org.bluez.MediaTransport", "SetProperty"));

I guess this will have to be adapted to Luiz's change that changed the
transport message destination? If so, I can do that. Or, I can also make
my "next" branch public, so you could develop against that if you
prefer. The reason why the branch is not currently public is that I want
to keep rebasing it on master, but if that's not a problem for you, I
can make it public.

> +    dbus_message_iter_init_append(m, &iter);
> +    pa_assert_se(dbus_message_iter_append_basic(&iter, DBUS_TYPE_STRING, &name));
> +    pa_dbus_append_basic_variant(&iter, DBUS_TYPE_UINT16, data);

I think your intention was to use the "type" argument here instead of
hardcoding DBUS_TYPE_UINT16.

> @@ -1468,6 +1486,7 @@ static void source_set_volume_cb(pa_source *s) {
>      pa_volume_t volume;
>      struct userdata *u;
>      char *k;
> +    const char *name = "InputGain";
>  
>      pa_assert(s);
>      pa_assert(s->core);
> @@ -1497,6 +1516,8 @@ static void source_set_volume_cb(pa_source *s) {
>      pa_assert_se(dbus_message_append_args(m, DBUS_TYPE_UINT16, &gain, DBUS_TYPE_INVALID));
>      pa_assert_se(dbus_connection_send(pa_dbus_connection_get(u->connection), m, NULL));
>      dbus_message_unref(m);
> +
> +    send_property_update(u, name, DBUS_TYPE_UINT16, &gain);

Hmm, I would prefer there to be functions in bluetooth-util for setting
transport properties. And preferably one function per property (e.g.
pa_bluetooth_transport_set_input_gain()) rather than a generic "set
property" function. My idea is that bluetooth-util would take care of
the dbus messaging as much as possible.

> @@ -1748,6 +1807,9 @@ static int add_sink(struct userdata *u) {
>          k = pa_sprintf_malloc("bluetooth-device@%p", (void*) u->sink);
>          pa_shared_set(u->core, k, u);
>          pa_xfree(k);
> +
> +        if (u->transport && !u->hsp.output_gain_slot)

There is pa_assert(u->transport) in the beginning of the function, no
need to check it here.

> +            u->hsp.output_gain_slot = pa_hook_connect(&u->transport->hooks[PA_BLUETOOTH_TRANSPORT_HOOK_OUTPUT_GAIN_CHANGED], PA_HOOK_NORMAL, (pa_hook_cb_t) output_gain_changed_cb, u);
>      }
>  
>      return 0;
> @@ -1832,6 +1894,9 @@ static int add_source(struct userdata *u) {
>          k = pa_sprintf_malloc("bluetooth-device@%p", (void*) u->source);
>          pa_shared_set(u->core, k, u);
>          pa_xfree(k);
> +
> +        if (u->transport && !u->hsp.input_gain_slot)

Same comment as above.

-- 
Tanu