[PATCH BlueZ] audio/transport: Only store volume when also emitting DBus prop change

[PATCH BlueZ] audio/transport: Only store volume when also emitting DBus prop change

[Marijn Suijten]

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.
---
 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

RE: [BlueZ] audio/transport: Only store volume when also emitting DBus prop change

[bluez.test.bot]

[-- Attachment #1: Type: text/plain, Size: 1953 bytes --]

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

Re: [PATCH BlueZ] audio/transport: Only store volume when also emitting DBus prop change

[Luiz Augusto von Dentz]

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
>


-- 
Luiz Augusto von Dentz

Re: [PATCH BlueZ] audio/transport: Only store volume when also emitting DBus prop change

[Marijn Suijten]

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

Re: [PATCH BlueZ] audio/transport: Only store volume when also emitting DBus prop change

[Luiz Augusto von Dentz]

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.

-- 
Luiz Augusto von Dentz