Hi Marijn, On Mon, Aug 9, 2021 at 1:26 PM Marijn Suijten <marijn.suijten@xxxxxxxxxxxxxx> wrote: > > 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 Applied, thanks. -- Luiz Augusto von Dentz
