Hi Luiz,
On 8/9/21 8:37 PM, Luiz Augusto von Dentz wrote:
Hi Marijn,
On Sun, Aug 8, 2021 at 7:35 AM Marijn Suijten
<marijn.suijten@xxxxxxxxxxxxxx> wrote:
By setting a2dp->volume early in set_volume() the resulting call to
media_transport_update_volume() when an AVRCP reply is received will
likely see the same volume received (unless the peer rounded it to
another value) and bail on equality, before emitting a DBus property
change. This results in DBus clients not being made aware of the change
unless the peer is an audio source (that receives a notification about
changed volume, instead of a command to _set_ a new volume), where
set_volume() immediately raises the DBus signal.
This issue is observed when playing back audio to headphones through an
AbsoluteVolume-enabled audio server like PulseAudio, which does not
receive the "Volume" change (while the peer does change volume) when
setting this property externally using ie. dbus-send.
Have you confirmed this works with the likes of PulseAudio, afaik
there was some problem when introducing this because the event may
cause a double change on the volume so the server needs to be able to
handle the volume == local volume, anyway if the headset rounds and
the values doesn't match I guess the server will need to be smart
enough to not trigger another volume change otherwise it could cause a
loop where the server request x but the headset rounds to y over and
over.
Thank you for your concerns. I as the author of AVRCP Absolute Volume
support in PulseAudio made sure to insert these equality checks against
the last known Absolute Volume value (separate from user-visible volume
managed by PA), both when Volume is received from the peer:
https://gitlab.freedesktop.org/pulseaudio/pulseaudio/-/blob/1a575bb0a708bc455e977629cb99412551867982/src/modules/bluetooth/bluez5-util.c#L614-621
And when sending a new Volume back:
https://gitlab.freedesktop.org/pulseaudio/pulseaudio/-/blob/1a575bb0a708bc455e977629cb99412551867982/src/modules/bluetooth/bluez5-util.c#L554-557
These together make it impossible to call ".Set" on a value that is
identical to the last value received through the "PropertiesChanged"
signal. It was made this way to prevent round-trips in the first place,
as receiving a Volume change and applying it to PA's sink/source would
also trigger the "volume changed" handler. Even if the peer decides to
reply with Volume-1 for every request, there will still be no round-trip.
If the server never reacts if volume != local volume and instead
just updates the local volume, which is probably the behavior we want,
then I should be safe to apply this change.
That is exactly what happens because PA cannot distinguish between
replies to SetAbsoluteVolume and the ABSOLUTE_VOLUME_CHANGED
notification caused by button presses, when just looking at the Volume
property.
Finally, I don't know what PipeWire does. Someone will have to look
through the codebase and confirm that it performs the same checks, or
loop in one of the authors in cc to confirm. Sonny from CrOS is already
included to make sure this doesn't break anything on their end.
---
profiles/audio/transport.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/profiles/audio/transport.c b/profiles/audio/transport.c
index 8248014ae..d158fc97a 100644
--- a/profiles/audio/transport.c
+++ b/profiles/audio/transport.c
@@ -659,14 +659,14 @@ static void set_volume(const GDBusPropertyTable *property,
if (a2dp->volume == volume)
return;
- a2dp->volume = volume;
-
notify = transport->source_watch ? true : false;
- if (notify)
+ if (notify) {
+ a2dp->volume = volume;
g_dbus_emit_property_changed(btd_get_dbus_connection(),
transport->path,
MEDIA_TRANSPORT_INTERFACE,
"Volume");
+ }
avrcp_set_volume(transport->device, volume, notify);
return;
--
2.32.0
Marijn
