Re: [pulseaudio-discuss] [PATCH v2] Absolute volume control for A2DP transport
On Saturday 20 October 2018 22:50:58 EHfive wrote:
> +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}
> + };
It would be better to store database in format suitable for pulseaudio.
Code below converts pulseaudio bt_addr_str to format suitable for
database. Why not not store database format in the same way as
pulseaudio stores bt_addr_str internally? It could simplify that code.
Plus, I think that device blacklist database should not be hardcoded in
pulseaudio source code. But rather be in some text configuration file(s)
which can be changed without recompilation. Similarly like alsa paths
and profile-sets.
> +
> + 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;
> +}
> +
--
Pali Rohár
pali.ro...@gmail.com
signature.asc
Description: PGP signature
___
pulseaudio-discuss mailing list
pulseaudio-discuss@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] [PATCH v2] Absolute volume control for A2DP transport
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 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();
> +
> +pa_assert_se(m = dbus_message_new_method_call(t->owner, t->path,
> "org.freedesktop.DBus.Properties", "Set"));
> +
> +dbus_message_iter_init_append(m, );
> +
> +pa_assert_se(dbus_message_iter_append_basic(, DBUS_TYPE_STRING,
> ));
> +pa_assert_se(dbus_message_iter_append_basic(, DBUS_TYPE_STRING,
> _name));
> +pa_dbus_append_basic_variant(, prop_type, prop_value);
> +
> +r =
> dbus_connection_send_with_reply_and_block(pa_dbus_connection_get(t->device->discovery->connection),
>
> m, -1, );
> +dbus_message_unref(m);
> +m = NULL;
> +if(r) {
> +dbus_message_unref(r);
> +r = NULL;
> +}
> +
> +if(dbus_error_is_set()) {
> +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,
> );
> +}
> +
> 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
