| Message ID | 20210808143555.100258-1-marijn.suijten@somainline.org (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Luiz Von Dentz |
| Headers | show |
| Series | [BlueZ] audio/transport: Only store volume when also emitting DBus prop change | expand |
This is automated email and please do not reply to this email! Dear submitter, Thank you for submitting the patches to the linux bluetooth mailing list. This is a CI test results with your patch series: PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=528101 ---Test result--- Test Summary: CheckPatch PASS 0.31 seconds GitLint PASS 0.11 seconds Prep - Setup ELL PASS 42.87 seconds Build - Prep PASS 0.09 seconds Build - Configure PASS 7.32 seconds Build - Make PASS 182.65 seconds Make Check PASS 9.08 seconds Make Distcheck PASS 208.43 seconds Build w/ext ELL - Configure PASS 7.70 seconds Build w/ext ELL - Make PASS 175.34 seconds Details ############################## Test: CheckPatch - PASS Desc: Run checkpatch.pl script with rule in .checkpatch.conf ############################## Test: GitLint - PASS Desc: Run gitlint with rule in .gitlint ############################## Test: Prep - Setup ELL - PASS Desc: Clone, build, and install ELL ############################## Test: Build - Prep - PASS Desc: Prepare environment for build ############################## Test: Build - Configure - PASS Desc: Configure the BlueZ source tree ############################## Test: Build - Make - PASS Desc: Build the BlueZ source tree ############################## Test: Make Check - PASS Desc: Run 'make check' ############################## Test: Make Distcheck - PASS Desc: Run distcheck to check the distribution ############################## Test: Build w/ext ELL - Configure - PASS Desc: Configure BlueZ source with '--enable-external-ell' configuration ############################## Test: Build w/ext ELL - Make - PASS Desc: Build BlueZ source with '--enable-external-ell' configuration --- Regards, Linux Bluetooth
Hi Marijn, On Sun, Aug 8, 2021 at 7:35 AM Marijn Suijten <marijn.suijten@somainline.org> 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. 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. > --- > 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 >
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@somainline.org> 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
Hi Marijn, On Mon, Aug 9, 2021 at 1:26 PM Marijn Suijten <marijn.suijten@somainline.org> 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@somainline.org> 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.
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;