Re: [PATCH v2] Absolute volume control for A2DP transport

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Thanks for the patch! Before I'll review the patch, can you resubmit it
using "git send-email"? The patch doesn't apply in its current form,
because lines are wrapped (there might be other formatting issues as
well). "git send-email" is guaranteed to produce well-formatted
patches.

Instructions here:
https://www.freedesktop.org/wiki/Software/PulseAudio/HowToUseGitSendEmail/

Alternatively, you can also submit the patch as a merge request in
GitLab.

Some comments about the commit message:

* The title (first line) should have a "bluetooth:" prefix. If you look
at the PulseAudio commit history, you see that all commit titles have
some prefix.

* I'd replace the term "absolute volume" with "hardware volume".

* It would be nice have a problem statement, so that someone who isn't
already very familiar with the topic can understand what practical
problem is being fixed.

* Since the patch is rather large, it would be good to have an overview
of what the patch does.

* "Matching disable absolute volume database" isn't a proper sentence,
and it's pretty hard to understand. English isn't your first language,
but please do your best to communicate clearly. I'm guessing that you
meant something like this:

    The patch includes a database of devices with bad volume
    behaviour. Hardware volume control is disabled for those devices.
    The database originates from Android: [insert URL here]

* "Also fixes source volume setting." What was broken in the source
volume setting?

-- Tanu


On Sat, 2018-10-20 at 22:50 +0800, EHfive wrote:
> Patch v1:
> 
> https://lists.freedesktop.org/archives/pulseaudio-discuss/2018-October/030538.html
> 
> ValdikSS <iam@xxxxxxxxxxxxxxx> requests
> 
> https://lists.freedesktop.org/archives/pulseaudio-discuss/2018-October/030566.html
> 
> 
> Matching disable absolute volume database.
> 
> Also fixes source volume setting.
> 
> 
>   src/modules/bluetooth/bluez5-util.c          |  65 
> +++++++++++++++++++++++++++++++++++++++++
>   src/modules/bluetooth/bluez5-util.h          |   4 +++
>   src/modules/bluetooth/module-bluez5-device.c | 203 
> +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>   3 files changed, 272 insertions(+)
> 
> ========================================
> 
> diff --git a/src/modules/bluetooth/bluez5-util.c 
> b/src/modules/bluetooth/bluez5-util.c
> index 2d83373..72cd05a 100644
> --- a/src/modules/bluetooth/bluez5-util.c
> +++ b/src/modules/bluetooth/bluez5-util.c
> @@ -348,6 +348,50 @@ void 
> pa_bluetooth_transport_free(pa_bluetooth_transport *t) {
>       pa_xfree(t);
>   }
> 
> +static int bluez5_transport_set_property(pa_bluetooth_transport *t, 
> const char *prop_name, int prop_type, void *prop_value){
> +    DBusMessage *m, *r;
> +    DBusError err;
> +    DBusMessageIter i;
> +    const char * interface = BLUEZ_MEDIA_TRANSPORT_INTERFACE;
> +
> +    pa_log_debug("Setting property, Owner: %s; Path: %s; Property: 
> %s",t->owner, t->path, prop_name);
> +
> +    pa_assert(t);
> +    pa_assert(t->device);
> +    pa_assert(t->device->discovery);
> +
> +    dbus_error_init(&err);
> +
> +    pa_assert_se(m = dbus_message_new_method_call(t->owner, t->path, 
> "org.freedesktop.DBus.Properties", "Set"));
> +
> +    dbus_message_iter_init_append(m, &i);
> +
> +    pa_assert_se(dbus_message_iter_append_basic(&i, DBUS_TYPE_STRING, 
> &interface));
> +    pa_assert_se(dbus_message_iter_append_basic(&i, DBUS_TYPE_STRING, 
> &prop_name));
> +    pa_dbus_append_basic_variant(&i, prop_type, prop_value);
> +
> +    r = 
> dbus_connection_send_with_reply_and_block(pa_dbus_connection_get(t->device->discovery->connection), 
> m, -1, &err);
> +    dbus_message_unref(m);
> +    m = NULL;
> +    if(r) {
> +        dbus_message_unref(r);
> +        r = NULL;
> +    }
> +
> +    if(dbus_error_is_set(&err)) {
> +        pa_log_debug("Failed to set property \"%s.%s\"", interface, 
> prop_name);
> +        return -1;
> +    }
> +
> +    return 0;
> +}
> +
> +static int bluez5_transport_set_volume(pa_bluetooth_transport *t, 
> uint16_t volume){
> +    if(t->a2dp_gain == volume)
> +        return 0;
> +    return bluez5_transport_set_property(t, "Volume", DBUS_TYPE_UINT16, 
> &volume);
> +}
> +
>   static int bluez5_transport_acquire_cb(pa_bluetooth_transport *t, bool 
> optional, size_t *imtu, size_t *omtu) {
>       DBusMessage *m, *r;
>       DBusError err;
> @@ -441,6 +485,14 @@ bool 
> pa_bluetooth_device_any_transport_connected(const pa_bluetooth_device *d) {
>       return false;
>   }
> 
> +void pa_transport_set_a2dp_gain(pa_bluetooth_transport *t, uint16_t 
> a2dp_gain){
> +    if(t->a2dp_gain == a2dp_gain)
> +        return;
> +    t->a2dp_gain = a2dp_gain;
> + pa_hook_fire(pa_bluetooth_discovery_hook(t->device->discovery, 
> PA_BLUETOOTH_HOOK_TRANSPORT_A2DP_GAIN_CHANGED), t);
> +}
> +
> +
>   static int transport_state_from_string(const char* value, 
> pa_bluetooth_transport_state_t *state) {
>       pa_assert(value);
>       pa_assert(state);
> @@ -483,6 +535,18 @@ static void 
> parse_transport_property(pa_bluetooth_transport *t, DBusMessageIter
>                   pa_bluetooth_transport_set_state(t, state);
>               }
> 
> +            break;
> +        }
> +        case DBUS_TYPE_UINT16: {
> +
> +            uint16_t value;
> +            dbus_message_iter_get_basic(&variant_i, &value);
> +
> +            if (pa_streq(key, "Volume")) {
> +                pa_log_debug("Transport Volume Changed to %u ", value);
> +                pa_transport_set_a2dp_gain(t, value);
> +            }
> +
>               break;
>           }
>       }
> @@ -1468,6 +1532,7 @@ static DBusMessage 
> *endpoint_set_configuration(DBusConnection *conn, DBusMessage
>       t = pa_bluetooth_transport_new(d, sender, path, p, config, size);
>       t->acquire = bluez5_transport_acquire_cb;
>       t->release = bluez5_transport_release_cb;
> +    t->set_volume = bluez5_transport_set_volume;
>       pa_bluetooth_transport_put(t);
> 
>       pa_log_debug("Transport %s available for profile %s", t->path, 
> pa_bluetooth_profile_to_string(t->profile));
> diff --git a/src/modules/bluetooth/bluez5-util.h 
> b/src/modules/bluetooth/bluez5-util.h
> index ad30708..5b8149d 100644
> --- a/src/modules/bluetooth/bluez5-util.h
> +++ b/src/modules/bluetooth/bluez5-util.h
> @@ -47,6 +47,7 @@ typedef enum pa_bluetooth_hook {
>       PA_BLUETOOTH_HOOK_TRANSPORT_STATE_CHANGED,            /* Call 
> data: pa_bluetooth_transport */
>       PA_BLUETOOTH_HOOK_TRANSPORT_MICROPHONE_GAIN_CHANGED,  /* Call 
> data: pa_bluetooth_transport */
>       PA_BLUETOOTH_HOOK_TRANSPORT_SPEAKER_GAIN_CHANGED,     /* Call 
> data: pa_bluetooth_transport */
> +    PA_BLUETOOTH_HOOK_TRANSPORT_A2DP_GAIN_CHANGED,        /* Call data: 
> pa_bluetooth_transport */
>       PA_BLUETOOTH_HOOK_MAX
>   } pa_bluetooth_hook_t;
> 
> @@ -70,6 +71,7 @@ typedef void 
> (*pa_bluetooth_transport_release_cb)(pa_bluetooth_transport *t);
>   typedef void 
> (*pa_bluetooth_transport_destroy_cb)(pa_bluetooth_transport *t);
>   typedef void 
> (*pa_bluetooth_transport_set_speaker_gain_cb)(pa_bluetooth_transport *t, 
> uint16_t gain);
>   typedef void 
> (*pa_bluetooth_transport_set_microphone_gain_cb)(pa_bluetooth_transport 
> *t, uint16_t gain);
> +typedef int 
> (*pa_bluetooth_transport_set_volume_cb)(pa_bluetooth_transport *t, 
> uint16_t volume);
> 
>   struct pa_bluetooth_transport {
>       pa_bluetooth_device *device;
> @@ -84,6 +86,7 @@ struct pa_bluetooth_transport {
> 
>       uint16_t microphone_gain;
>       uint16_t speaker_gain;
> +    uint16_t a2dp_gain;
> 
>       pa_bluetooth_transport_state_t state;
> 
> @@ -92,6 +95,7 @@ struct pa_bluetooth_transport {
>       pa_bluetooth_transport_destroy_cb destroy;
>       pa_bluetooth_transport_set_speaker_gain_cb set_speaker_gain;
>       pa_bluetooth_transport_set_microphone_gain_cb set_microphone_gain;
> +    pa_bluetooth_transport_set_volume_cb set_volume;
>       void *userdata;
>   };
> 
> diff --git a/src/modules/bluetooth/module-bluez5-device.c 
> b/src/modules/bluetooth/module-bluez5-device.c
> index 351ad12..4b4c9b2 100644
> --- a/src/modules/bluetooth/module-bluez5-device.c
> +++ b/src/modules/bluetooth/module-bluez5-device.c
> @@ -27,6 +27,8 @@
>   #include <arpa/inet.h>
>   #include <sbc/sbc.h>
> 
> +#include <bluetooth/bluetooth.h>
> +
>   #include <pulse/rtclock.h>
>   #include <pulse/timeval.h>
>   #include <pulse/utf8.h>
> @@ -64,6 +66,7 @@ PA_MODULE_USAGE("path=<device object path>"
>   #define BITPOOL_DEC_LIMIT 32
>   #define BITPOOL_DEC_STEP 5
>   #define HSP_MAX_GAIN 15
> +#define BLUEZ_MAX_GAIN 127
> 
>   static const char* const valid_modargs[] = {
>       "path",
> @@ -113,6 +116,7 @@ struct userdata {
>       pa_hook_slot *transport_state_changed_slot;
>       pa_hook_slot *transport_speaker_gain_changed_slot;
>       pa_hook_slot *transport_microphone_gain_changed_slot;
> +    pa_hook_slot *transport_a2dp_gain_changed_slot;
> 
>       pa_bluetooth_discovery *discovery;
>       pa_bluetooth_device *device;
> @@ -1056,6 +1060,37 @@ static void source_set_volume_cb(pa_source *s) {
>       u->transport->set_microphone_gain(u->transport, gain);
>   }
> 
> +static void source_set_a2dp_volume_cb(pa_source *s) {
> +    uint16_t gain;
> +    pa_volume_t volume;
> +    struct userdata *u;
> +
> +    pa_assert(s);
> +    pa_assert(s->core);
> +
> +    u = s->userdata;
> +
> +    pa_assert(u);
> +    pa_assert(u->source == s);
> +
> +    if (u->transport->set_volume == NULL)
> +        return;
> +
> +    gain = (uint16_t) ((pa_cvolume_max(&s->real_volume) * 
> BLUEZ_MAX_GAIN) / PA_VOLUME_NORM);
> +
> +    pa_log_debug("Real Volume Gain:%u", gain);
> +
> +    if (gain > BLUEZ_MAX_GAIN)
> +        gain = BLUEZ_MAX_GAIN;
> +
> +    volume = (pa_volume_t) (gain * PA_VOLUME_NORM / BLUEZ_MAX_GAIN);
> +
> +    pa_cvolume_set(&s->real_volume, u->sample_spec.channels, volume);
> +    pa_cvolume_set(&s->soft_volume, u->sample_spec.channels, volume);
> +
> +    u->transport->set_volume(u->transport, gain);
> +}
> +
>   /* Run from main thread */
>   static int add_source(struct userdata *u) {
>       pa_source_new_data data;
> @@ -1109,6 +1144,9 @@ static int add_source(struct userdata *u) {
>       if (u->profile == PA_BLUETOOTH_PROFILE_HEADSET_HEAD_UNIT || 
> u->profile == PA_BLUETOOTH_PROFILE_HEADSET_AUDIO_GATEWAY) {
>           pa_source_set_set_volume_callback(u->source, 
> source_set_volume_cb);
>           u->source->n_volume_steps = 16;
> +    } else if (u->profile == PA_BLUETOOTH_PROFILE_A2DP_SOURCE) {
> +        pa_source_set_set_volume_callback(u->source, 
> source_set_a2dp_volume_cb);
> +        u->source->n_volume_steps = 1;
>       }
>       return 0;
>   }
> @@ -1230,6 +1268,110 @@ static void sink_set_volume_cb(pa_sink *s) {
>       u->transport->set_speaker_gain(u->transport, gain);
>   }
> 
> +static void sink_set_a2dp_volume_cb(pa_sink *s) {
> +    uint16_t gain;
> +    pa_volume_t volume;
> +    struct userdata *u;
> +
> +    pa_assert(s);
> +    pa_assert(s->core);
> +
> +    u = s->userdata;
> +
> +    pa_assert(u);
> +    pa_assert(u->sink == s);
> +
> +    if (u->transport->set_volume == NULL)
> +        return;
> +
> +    gain = (uint16_t) ((pa_cvolume_max(&s->real_volume) * 
> BLUEZ_MAX_GAIN) / PA_VOLUME_NORM);
> +
> +    pa_log_debug("Real Volume Gain:%u", gain);
> +
> +    if (gain > BLUEZ_MAX_GAIN)
> +        gain = BLUEZ_MAX_GAIN;
> +
> +    volume = (pa_volume_t) (gain * PA_VOLUME_NORM / BLUEZ_MAX_GAIN);
> +
> +    pa_cvolume_set(&s->real_volume, u->sample_spec.channels, volume);
> +
> +    if(u->transport->set_volume(u->transport, gain) < 0) {
> +        pa_cvolume_set(&s->soft_volume, u->sample_spec.channels, 
> PA_VOLUME_NORM);
> +        pa_sink_set_set_volume_callback(s, NULL);
> +        u->transport->a2dp_gain = 0xFFu;
> +    }
> +}
> +
> +static bool disable_absolute_volume_match(const char * bt_addr_str) {
> +    bdaddr_t bt_addr;
> +    int i;
> +    static bdaddr_t affected_bt_addr_cache;
> +    static bool effected_bt_addr_cached = false;
> +
> +    static const struct {
> +        bdaddr_t addr;
> +        size_t length;
> +    }  database[] = {
> +
> +            // Ausdom M05 - unacceptably loud volume
> +            {{{0xa0, 0xe9, 0xdb, 0, 0, 0}}, 3},
> +            // iKross IKBT83B HS - unacceptably loud volume
> +            {{{0x00, 0x14, 0x02, 0, 0, 0}}, 3},
> +            // JayBird BlueBuds X - low granularity on volume control
> +            {{{0x44, 0x5e, 0xf3, 0, 0, 0}}, 3},
> +            {{{0xd4, 0x9c, 0x28, 0, 0, 0}}, 3},
> +            // LG Tone HBS-730 - unacceptably loud volume
> +            {{{0x00, 0x18, 0x6b, 0, 0, 0}}, 3},
> +            {{{0xb8, 0xad, 0x3e, 0, 0, 0}}, 3},
> +            // LG Tone HV-800 - unacceptably loud volume
> +            {{{0xa0, 0xe9, 0xdb, 0, 0, 0}}, 3},
> +            // Motorola Roadster
> +            {{{0x00, 0x24, 0x1C, 0, 0, 0}}, 3},
> +            // Mpow Cheetah - unacceptably loud volume
> +            {{{0x00, 0x11, 0xb1, 0, 0, 0}}, 3},
> +            // SOL REPUBLIC Tracks Air - unable to adjust volume back 
> off from max
> +            {{{0xa4, 0x15, 0x66, 0, 0, 0}}, 3},
> +            // Swage Rokitboost HS - unacceptably loud volume
> +            {{{0x00, 0x14, 0xf1, 0, 0, 0}}, 3},
> +            // VW Car Kit - not enough granularity with volume
> +            {{{0x00, 0x26, 0x7e, 0, 0, 0}}, 3},
> +            {{{0x90, 0x03, 0xb7, 0, 0, 0}}, 3},
> +            // deepblue2 - cannot change smoothly the volume, 0x b/37834035
> +            {{{0x0c, 0xa6, 0x94, 0, 0, 0}}, 3}
> +    };
> +
> +    for (i = 0; i < 6; ++i, bt_addr_str += 3)
> +        bt_addr.b[i] = (uint8_t) strtol(bt_addr_str, NULL, 16);
> +
> +    if (effected_bt_addr_cached){
> +        int j;
> +        bool flag = true;
> +        for (j = 0; j < 6; ++j)
> +            if (affected_bt_addr_cache.b[j] != bt_addr.b[j]){
> +                flag = false;
> +                break;
> +            }
> +        if(flag)
> +            return true;
> +    }
> +
> +    for(i = 0; i < PA_ELEMENTSOF(database); ++i){
> +        int j;
> +        bool flag = true;
> +        for(j = 0; j < database[i].length; ++j)
> +            if(database[i].addr.b[j] != bt_addr.b[j]){
> +                flag = false;
> +                break;
> +            }
> +        if(flag){
> +            affected_bt_addr_cache = bt_addr;
> +            effected_bt_addr_cached = true;
> +            return true;
> +        }
> +    }
> +    return false;
> +}
> +
>   /* Run from main thread */
>   static int add_sink(struct userdata *u) {
>       pa_sink_new_data data;
> @@ -1284,6 +1426,11 @@ static int add_sink(struct userdata *u) {
>       if (u->profile == PA_BLUETOOTH_PROFILE_HEADSET_HEAD_UNIT || 
> u->profile == PA_BLUETOOTH_PROFILE_HEADSET_AUDIO_GATEWAY) {
>           pa_sink_set_set_volume_callback(u->sink, sink_set_volume_cb);
>           u->sink->n_volume_steps = 16;
> +    } else if (u->profile == PA_BLUETOOTH_PROFILE_A2DP_SINK) {
> + if(disable_absolute_volume_match(u->device->address))
> +            return 0;
> +        pa_sink_set_set_volume_callback(u->sink, sink_set_a2dp_volume_cb);
> +        u->sink->n_volume_steps = 1;
>       }
>       return 0;
>   }
> @@ -2340,6 +2487,56 @@ static pa_hook_result_t 
> transport_microphone_gain_changed_cb(pa_bluetooth_discov
>       return PA_HOOK_OK;
>   }
> 
> +static pa_hook_result_t 
> transport_a2dp_gain_changed_cb(pa_bluetooth_discovery *y, 
> pa_bluetooth_transport *t, struct userdata *u) {
> +    pa_volume_t volume;
> +    pa_cvolume v;
> +    uint16_t gain;
> +
> +    pa_assert(t);
> +    pa_assert(u);
> +
> +    if (t != u->transport)
> +        return PA_HOOK_OK;
> +
> +    if(disable_absolute_volume_match(u->device->address))
> +        return PA_HOOK_OK;
> +
> +    gain = t->a2dp_gain;
> +    volume = (pa_volume_t) (gain * PA_VOLUME_NORM / BLUEZ_MAX_GAIN);
> +
> +    pa_cvolume_set(&v, u->sample_spec.channels, volume);
> +
> +    if(u->profile == PA_BLUETOOTH_PROFILE_A2DP_SINK){
> +        pa_assert(u->sink);
> +
> +        if(!u->sink->set_volume){
> +            pa_cvolume_set(&u->sink->soft_volume, 
> u->sample_spec.channels, PA_VOLUME_NORM);
> +            pa_sink_set_set_volume_callback(u->sink, 
> sink_set_a2dp_volume_cb);
> +        }
> +
> +        /* Mute when gain equal 0 or 1 */
> +        if (gain <= 1)
> +            pa_sink_mute_changed(u->sink, true);
> +        else if(u->sink->muted)
> +            pa_sink_mute_changed(u->sink, false);
> +
> +        pa_sink_volume_changed(u->sink, &v);
> +    } else if(u->profile == PA_BLUETOOTH_PROFILE_A2DP_SOURCE){
> +        pa_assert(u->source);
> +
> +        /* Mute when gain equal 0 or 1 */
> +        if (gain <= 1)
> +            pa_source_mute_changed(u->source, true);
> +        else if(u->source->muted)
> +            pa_source_mute_changed(u->source, false);
> +
> +        pa_source_set_volume(u->source, &v, true, true);
> +    }
> +
> +
> +    return PA_HOOK_OK;
> +}
> +
>   /* Run from main thread context */
>   static int device_process_msg(pa_msgobject *obj, int code, void *data, 
> int64_t offset, pa_memchunk *chunk) {
>       struct bluetooth_msg *m = BLUETOOTH_MSG(obj);
> @@ -2430,6 +2627,9 @@ int pa__init(pa_module* m) {
>       u->transport_microphone_gain_changed_slot =
> pa_hook_connect(pa_bluetooth_discovery_hook(u->discovery, 
> PA_BLUETOOTH_HOOK_TRANSPORT_MICROPHONE_GAIN_CHANGED), PA_HOOK_NORMAL, 
> (pa_hook_cb_t) transport_microphone_gain_changed_cb, u);
> 
> +    u->transport_a2dp_gain_changed_slot =
> + pa_hook_connect(pa_bluetooth_discovery_hook(u->discovery, 
> PA_BLUETOOTH_HOOK_TRANSPORT_A2DP_GAIN_CHANGED), PA_HOOK_NORMAL, 
> (pa_hook_cb_t) transport_a2dp_gain_changed_cb, u);
> +
>       if (add_card(u) < 0)
>           goto fail;
> 
> @@ -2491,6 +2691,9 @@ void pa__done(pa_module *m) {
>       if (u->transport_microphone_gain_changed_slot)
> pa_hook_slot_free(u->transport_microphone_gain_changed_slot);
> 
> +    if (u->transport_a2dp_gain_changed_slot)
> + pa_hook_slot_free(u->transport_a2dp_gain_changed_slot);
> +
>       if (u->sbc_info.buffer)
>           pa_xfree(u->sbc_info.buffer);
> 
> 
> _______________________________________________
> pulseaudio-discuss mailing list
> pulseaudio-discuss@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss

_______________________________________________
pulseaudio-discuss mailing list
pulseaudio-discuss@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss




[Index of Archives]     [Linux Audio Users]     [AMD Graphics]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux