[PATCH bluez] audio: Don't initialize device volume from media player

[PATCH bluez] audio: Don't initialize device volume from media player

[Myrrh Periwinkle]

Media player objects may be shared between devices. As a result,
a device without support for hardware volume that is connected after one
that does may end up being erroneously considered hardware
volume-capable.

fa7828bdd ("transport: Fix not being able to initialize volume properly")
introduced btd_device_{get,set}_volume that is used as an alternative in
case no media player objects are present. Therefore, we can remove
media_player_get_device_volume and instead use btd_device_get_volume to
determine the initial volume.

---
 profiles/audio/avrcp.c |  2 +-
 profiles/audio/media.c | 33 +--------------------------------
 profiles/audio/media.h |  1 -
 3 files changed, 2 insertions(+), 34 deletions(-)

diff --git a/profiles/audio/avrcp.c b/profiles/audio/avrcp.c
index e2797112fcd580c3fc56793f933e00b1c61e5205..ec07522e6a34eb1dc5f6f413f48f1087a609df9a 100644
--- a/profiles/audio/avrcp.c
+++ b/profiles/audio/avrcp.c
@@ -4284,7 +4284,7 @@ static void target_init(struct avrcp *session)
 		target->player = player;
 		player->sessions = g_slist_prepend(player->sessions, session);
 
-		init_volume = media_player_get_device_volume(session->dev);
+		init_volume = btd_device_get_volume(session->dev);
 		media_transport_update_device_volume(session->dev, init_volume);
 	}
 
diff --git a/profiles/audio/media.c b/profiles/audio/media.c
index 8e62dca17070edbc5101677c6eebd3707492c824..55f1482d1d9ce52e104481bab3ede373f47aee0c 100644
--- a/profiles/audio/media.c
+++ b/profiles/audio/media.c
@@ -499,37 +499,6 @@ struct a2dp_config_data {
 	a2dp_endpoint_config_t cb;
 };
 
-int8_t media_player_get_device_volume(struct btd_device *device)
-{
-#ifdef HAVE_AVRCP
-	struct avrcp_player *target_player;
-	struct media_adapter *adapter;
-	GSList *l;
-
-	if (!device)
-		return -1;
-
-	target_player = avrcp_get_target_player_by_device(device);
-	if (!target_player)
-		goto done;
-
-	adapter = find_adapter(device);
-	if (!adapter)
-		goto done;
-
-	for (l = adapter->players; l; l = l->next) {
-		struct media_player *mp = l->data;
-
-		if (mp->player == target_player)
-			return mp->volume;
-	}
-
-done:
-#endif /* HAVE_AVRCP */
-	/* If media_player doesn't exists use device_volume */
-	return btd_device_get_volume(device);
-}
-
 static gboolean set_configuration(struct media_endpoint *endpoint,
 					uint8_t *configuration, size_t size,
 					media_endpoint_cb_t cb,
@@ -556,7 +525,7 @@ static gboolean set_configuration(struct media_endpoint *endpoint,
 	if (transport == NULL)
 		return FALSE;
 
-	init_volume = media_player_get_device_volume(device);
+	init_volume = btd_device_get_volume(device);
 	media_transport_update_volume(transport, init_volume);
 
 	msg = dbus_message_new_method_call(endpoint->sender, endpoint->path,
diff --git a/profiles/audio/media.h b/profiles/audio/media.h
index 2b2e8e1572874d5f71abb28fdd5b92fa2d9efe83..d3954abd6de505a69cab3fcffc217d236a52d3e5 100644
--- a/profiles/audio/media.h
+++ b/profiles/audio/media.h
@@ -23,6 +23,5 @@ uint8_t media_endpoint_get_codec(struct media_endpoint *endpoint);
 struct btd_adapter *media_endpoint_get_btd_adapter(
 					struct media_endpoint *endpoint);
 bool media_endpoint_is_broadcast(struct media_endpoint *endpoint);
-int8_t media_player_get_device_volume(struct btd_device *device);
 
 const struct media_endpoint *media_endpoint_get_asha(void);

---
base-commit: 2c0c323d08357a4ff3065fcd49fee0c83b5835cd
change-id: 20250805-audio-no-reuse-media-player-volume-fbc2983a287a

Best regards,
-- 
Myrrh Periwinkle <myrrhperiwinkle@qtmlabs.xyz>

RE: [bluez] audio: Don't initialize device volume from media player

[bluez.test.bot]

[-- Attachment #1: Type: text/plain, Size: 1261 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=988417

---Test result---

Test Summary:
CheckPatch                    PENDING   0.29 seconds
GitLint                       PENDING   0.28 seconds
BuildEll                      PASS      20.01 seconds
BluezMake                     PASS      2701.18 seconds
MakeCheck                     PASS      20.14 seconds
MakeDistcheck                 PASS      184.68 seconds
CheckValgrind                 PASS      234.98 seconds
CheckSmatch                   PASS      304.73 seconds
bluezmakeextell               PASS      128.95 seconds
IncrementalBuild              PENDING   0.24 seconds
ScanBuild                     PASS      911.45 seconds

Details
##############################
Test: CheckPatch - PENDING
Desc: Run checkpatch.pl script
Output:

##############################
Test: GitLint - PENDING
Desc: Run gitlint
Output:

##############################
Test: IncrementalBuild - PENDING
Desc: Incremental build with the patches in the series
Output:



---
Regards,
Linux Bluetooth

Re: [PATCH bluez] audio: Don't initialize device volume from media player

[Luiz Augusto von Dentz]

Hi Myrrh,

On Tue, Aug 5, 2025 at 6:29 AM Myrrh Periwinkle
<myrrhperiwinkle@qtmlabs.xyz> wrote:
>
> Media player objects may be shared between devices. As a result,
> a device without support for hardware volume that is connected after one
> that does may end up being erroneously considered hardware
> volume-capable.

Don't quite follow, avrcp_player is per device, not sure how it can be
shared between devices?

> fa7828bdd ("transport: Fix not being able to initialize volume properly")
> introduced btd_device_{get,set}_volume that is used as an alternative in
> case no media player objects are present. Therefore, we can remove
> media_player_get_device_volume and instead use btd_device_get_volume to
> determine the initial volume.

Don't follow you here, why shouldn;t we use the media player if we have one?

> ---
>  profiles/audio/avrcp.c |  2 +-
>  profiles/audio/media.c | 33 +--------------------------------
>  profiles/audio/media.h |  1 -
>  3 files changed, 2 insertions(+), 34 deletions(-)
>
> diff --git a/profiles/audio/avrcp.c b/profiles/audio/avrcp.c
> index e2797112fcd580c3fc56793f933e00b1c61e5205..ec07522e6a34eb1dc5f6f413f48f1087a609df9a 100644
> --- a/profiles/audio/avrcp.c
> +++ b/profiles/audio/avrcp.c
> @@ -4284,7 +4284,7 @@ static void target_init(struct avrcp *session)
>                 target->player = player;
>                 player->sessions = g_slist_prepend(player->sessions, session);
>
> -               init_volume = media_player_get_device_volume(session->dev);
> +               init_volume = btd_device_get_volume(session->dev);
>                 media_transport_update_device_volume(session->dev, init_volume);
>         }
>
> diff --git a/profiles/audio/media.c b/profiles/audio/media.c
> index 8e62dca17070edbc5101677c6eebd3707492c824..55f1482d1d9ce52e104481bab3ede373f47aee0c 100644
> --- a/profiles/audio/media.c
> +++ b/profiles/audio/media.c
> @@ -499,37 +499,6 @@ struct a2dp_config_data {
>         a2dp_endpoint_config_t cb;
>  };
>
> -int8_t media_player_get_device_volume(struct btd_device *device)
> -{
> -#ifdef HAVE_AVRCP
> -       struct avrcp_player *target_player;
> -       struct media_adapter *adapter;
> -       GSList *l;
> -
> -       if (!device)
> -               return -1;
> -
> -       target_player = avrcp_get_target_player_by_device(device);
> -       if (!target_player)
> -               goto done;
> -
> -       adapter = find_adapter(device);
> -       if (!adapter)
> -               goto done;
> -
> -       for (l = adapter->players; l; l = l->next) {
> -               struct media_player *mp = l->data;
> -
> -               if (mp->player == target_player)
> -                       return mp->volume;
> -       }
> -
> -done:
> -#endif /* HAVE_AVRCP */
> -       /* If media_player doesn't exists use device_volume */
> -       return btd_device_get_volume(device);
> -}
> -
>  static gboolean set_configuration(struct media_endpoint *endpoint,
>                                         uint8_t *configuration, size_t size,
>                                         media_endpoint_cb_t cb,
> @@ -556,7 +525,7 @@ static gboolean set_configuration(struct media_endpoint *endpoint,
>         if (transport == NULL)
>                 return FALSE;
>
> -       init_volume = media_player_get_device_volume(device);
> +       init_volume = btd_device_get_volume(device);
>         media_transport_update_volume(transport, init_volume);
>
>         msg = dbus_message_new_method_call(endpoint->sender, endpoint->path,
> diff --git a/profiles/audio/media.h b/profiles/audio/media.h
> index 2b2e8e1572874d5f71abb28fdd5b92fa2d9efe83..d3954abd6de505a69cab3fcffc217d236a52d3e5 100644
> --- a/profiles/audio/media.h
> +++ b/profiles/audio/media.h
> @@ -23,6 +23,5 @@ uint8_t media_endpoint_get_codec(struct media_endpoint *endpoint);
>  struct btd_adapter *media_endpoint_get_btd_adapter(
>                                         struct media_endpoint *endpoint);
>  bool media_endpoint_is_broadcast(struct media_endpoint *endpoint);
> -int8_t media_player_get_device_volume(struct btd_device *device);
>
>  const struct media_endpoint *media_endpoint_get_asha(void);
>
> ---
> base-commit: 2c0c323d08357a4ff3065fcd49fee0c83b5835cd
> change-id: 20250805-audio-no-reuse-media-player-volume-fbc2983a287a
>
> Best regards,
> --
> Myrrh Periwinkle <myrrhperiwinkle@qtmlabs.xyz>
>
>


-- 
Luiz Augusto von Dentz

Re: [PATCH bluez] audio: Don't initialize device volume from media player

[Myrrh Periwinkle]

Resent because I forgot to use "Reply All"

On 8/5/25 22:24, Luiz Augusto von Dentz wrote:
> Hi Myrrh,
>
> On Tue, Aug 5, 2025 at 6:29 AM Myrrh Periwinkle
> <myrrhperiwinkle@qtmlabs.xyz> wrote:
>> Media player objects may be shared between devices. As a result,
>> a device without support for hardware volume that is connected after one
>> that does may end up being erroneously considered hardware
>> volume-capable.
> Don't quite follow, avrcp_player is per device, not sure how it can be
> shared between devices?
>
>> fa7828bdd ("transport: Fix not being able to initialize volume properly")
>> introduced btd_device_{get,set}_volume that is used as an alternative in
>> case no media player objects are present. Therefore, we can remove
>> media_player_get_device_volume and instead use btd_device_get_volume to
>> determine the initial volume.
> Don't follow you here, why shouldn;t we use the media player if we have one?
>
>> ---
>>   profiles/audio/avrcp.c |  2 +-
>>   profiles/audio/media.c | 33 +--------------------------------
>>   profiles/audio/media.h |  1 -
>>   3 files changed, 2 insertions(+), 34 deletions(-)
>>
>> diff --git a/profiles/audio/avrcp.c b/profiles/audio/avrcp.c
>> index e2797112fcd580c3fc56793f933e00b1c61e5205..ec07522e6a34eb1dc5f6f413f48f1087a609df9a 100644
>> --- a/profiles/audio/avrcp.c
>> +++ b/profiles/audio/avrcp.c
>> @@ -4284,7 +4284,7 @@ static void target_init(struct avrcp *session)
>>                  target->player = player;
>>                  player->sessions = g_slist_prepend(player->sessions, session);
>>
>> -               init_volume = media_player_get_device_volume(session->dev);
>> +               init_volume = btd_device_get_volume(session->dev);
>>                  media_transport_update_device_volume(session->dev, init_volume);
>>          }
>>
>> diff --git a/profiles/audio/media.c b/profiles/audio/media.c
>> index 8e62dca17070edbc5101677c6eebd3707492c824..55f1482d1d9ce52e104481bab3ede373f47aee0c 100644
>> --- a/profiles/audio/media.c
>> +++ b/profiles/audio/media.c
>> @@ -499,37 +499,6 @@ struct a2dp_config_data {
>>          a2dp_endpoint_config_t cb;
>>   };
>>
>> -int8_t media_player_get_device_volume(struct btd_device *device)
>> -{
>> -#ifdef HAVE_AVRCP
>> -       struct avrcp_player *target_player;
>> -       struct media_adapter *adapter;
>> -       GSList *l;
>> -
>> -       if (!device)
>> -               return -1;
>> -
>> -       target_player = avrcp_get_target_player_by_device(device);
>> -       if (!target_player)
>> -               goto done;
>> -
>> -       adapter = find_adapter(device);
>> -       if (!adapter)
>> -               goto done;
>> -
>> -       for (l = adapter->players; l; l = l->next) {
>> -               struct media_player *mp = l->data;
>> -
>> -               if (mp->player == target_player)
>> -                       return mp->volume;
The `avrcp_player` object indeed is not shared between devices, but the 
volume is acquired from (and set for) the associated `media_player` 
object which is not tied to a specific device, and corresponds to client 
`org.mpris.MediaPlayer2.Player` objects.
>> -       }
>> -
>> -done:
>> -#endif /* HAVE_AVRCP */
>> -       /* If media_player doesn't exists use device_volume */
>> -       return btd_device_get_volume(device);
>> -}
>> -
>>   static gboolean set_configuration(struct media_endpoint *endpoint,
>>                                          uint8_t *configuration, size_t size,
>>                                          media_endpoint_cb_t cb,
>> @@ -556,7 +525,7 @@ static gboolean set_configuration(struct media_endpoint *endpoint,
>>          if (transport == NULL)
>>                  return FALSE;
>>
>> -       init_volume = media_player_get_device_volume(device);
>> +       init_volume = btd_device_get_volume(device);
>>          media_transport_update_volume(transport, init_volume);
>>
>>          msg = dbus_message_new_method_call(endpoint->sender, endpoint->path,
>> diff --git a/profiles/audio/media.h b/profiles/audio/media.h
>> index 2b2e8e1572874d5f71abb28fdd5b92fa2d9efe83..d3954abd6de505a69cab3fcffc217d236a52d3e5 100644
>> --- a/profiles/audio/media.h
>> +++ b/profiles/audio/media.h
>> @@ -23,6 +23,5 @@ uint8_t media_endpoint_get_codec(struct media_endpoint *endpoint);
>>   struct btd_adapter *media_endpoint_get_btd_adapter(
>>                                          struct media_endpoint *endpoint);
>>   bool media_endpoint_is_broadcast(struct media_endpoint *endpoint);
>> -int8_t media_player_get_device_volume(struct btd_device *device);
>>
>>   const struct media_endpoint *media_endpoint_get_asha(void);
>>
>> ---
>> base-commit: 2c0c323d08357a4ff3065fcd49fee0c83b5835cd
>> change-id: 20250805-audio-no-reuse-media-player-volume-fbc2983a287a
>>
>> Best regards,
>> --
>> Myrrh Periwinkle<myrrhperiwinkle@qtmlabs.xyz>
>>
>>

Re: [PATCH bluez] audio: Don't initialize device volume from media player

[Luiz Augusto von Dentz]

Hi Myrrh,

On Tue, Aug 5, 2025 at 11:34 AM Myrrh Periwinkle
<myrrhperiwinkle@qtmlabs.xyz> wrote:
>
> Resent because I forgot to use "Reply All"
>
> On 8/5/25 22:24, Luiz Augusto von Dentz wrote:
> > Hi Myrrh,
> >
> > On Tue, Aug 5, 2025 at 6:29 AM Myrrh Periwinkle
> > <myrrhperiwinkle@qtmlabs.xyz> wrote:
> >> Media player objects may be shared between devices. As a result,
> >> a device without support for hardware volume that is connected after one
> >> that does may end up being erroneously considered hardware
> >> volume-capable.
> > Don't quite follow, avrcp_player is per device, not sure how it can be
> > shared between devices?
> >
> >> fa7828bdd ("transport: Fix not being able to initialize volume properly")
> >> introduced btd_device_{get,set}_volume that is used as an alternative in
> >> case no media player objects are present. Therefore, we can remove
> >> media_player_get_device_volume and instead use btd_device_get_volume to
> >> determine the initial volume.
> > Don't follow you here, why shouldn;t we use the media player if we have one?
> >
> >> ---
> >>   profiles/audio/avrcp.c |  2 +-
> >>   profiles/audio/media.c | 33 +--------------------------------
> >>   profiles/audio/media.h |  1 -
> >>   3 files changed, 2 insertions(+), 34 deletions(-)
> >>
> >> diff --git a/profiles/audio/avrcp.c b/profiles/audio/avrcp.c
> >> index e2797112fcd580c3fc56793f933e00b1c61e5205..ec07522e6a34eb1dc5f6f413f48f1087a609df9a 100644
> >> --- a/profiles/audio/avrcp.c
> >> +++ b/profiles/audio/avrcp.c
> >> @@ -4284,7 +4284,7 @@ static void target_init(struct avrcp *session)
> >>                  target->player = player;
> >>                  player->sessions = g_slist_prepend(player->sessions, session);
> >>
> >> -               init_volume = media_player_get_device_volume(session->dev);
> >> +               init_volume = btd_device_get_volume(session->dev);
> >>                  media_transport_update_device_volume(session->dev, init_volume);
> >>          }
> >>
> >> diff --git a/profiles/audio/media.c b/profiles/audio/media.c
> >> index 8e62dca17070edbc5101677c6eebd3707492c824..55f1482d1d9ce52e104481bab3ede373f47aee0c 100644
> >> --- a/profiles/audio/media.c
> >> +++ b/profiles/audio/media.c
> >> @@ -499,37 +499,6 @@ struct a2dp_config_data {
> >>          a2dp_endpoint_config_t cb;
> >>   };
> >>
> >> -int8_t media_player_get_device_volume(struct btd_device *device)
> >> -{
> >> -#ifdef HAVE_AVRCP
> >> -       struct avrcp_player *target_player;
> >> -       struct media_adapter *adapter;
> >> -       GSList *l;
> >> -
> >> -       if (!device)
> >> -               return -1;
> >> -
> >> -       target_player = avrcp_get_target_player_by_device(device);
> >> -       if (!target_player)
> >> -               goto done;
> >> -
> >> -       adapter = find_adapter(device);
> >> -       if (!adapter)
> >> -               goto done;
> >> -
> >> -       for (l = adapter->players; l; l = l->next) {
> >> -               struct media_player *mp = l->data;
> >> -
> >> -               if (mp->player == target_player)
> >> -                       return mp->volume;
> The `avrcp_player` object indeed is not shared between devices, but the
> volume is acquired from (and set for) the associated `media_player`
> object which is not tied to a specific device, and corresponds to client
> `org.mpris.MediaPlayer2.Player` objects.

Ok, so what is the problem with that? If there are 2 headsets that
wants to control the same player registered via mpris-proxy(?) that
should be allowed, or the problem is that each device should control a
dedicated instance of the volume? I suspect the volume store in
mp->volume is only used as initial volume since the actual volume is
stored in the Transport.Volume, the reason why we can't use the
Transport directly is that it is created on demand while there is a
stream so if the stream is not ready at time a volume has been set we
need to store it elsewhere, that said perhaps that should be store per
device not per media_player which seem to be registered per adapter in
media.c but then we should probably remove the volume field altogether
to avoid any confusion in the future.

> >> -       }
> >> -
> >> -done:
> >> -#endif /* HAVE_AVRCP */
> >> -       /* If media_player doesn't exists use device_volume */
> >> -       return btd_device_get_volume(device);
> >> -}
> >> -
> >>   static gboolean set_configuration(struct media_endpoint *endpoint,
> >>                                          uint8_t *configuration, size_t size,
> >>                                          media_endpoint_cb_t cb,
> >> @@ -556,7 +525,7 @@ static gboolean set_configuration(struct media_endpoint *endpoint,
> >>          if (transport == NULL)
> >>                  return FALSE;
> >>
> >> -       init_volume = media_player_get_device_volume(device);
> >> +       init_volume = btd_device_get_volume(device);
> >>          media_transport_update_volume(transport, init_volume);
> >>
> >>          msg = dbus_message_new_method_call(endpoint->sender, endpoint->path,
> >> diff --git a/profiles/audio/media.h b/profiles/audio/media.h
> >> index 2b2e8e1572874d5f71abb28fdd5b92fa2d9efe83..d3954abd6de505a69cab3fcffc217d236a52d3e5 100644
> >> --- a/profiles/audio/media.h
> >> +++ b/profiles/audio/media.h
> >> @@ -23,6 +23,5 @@ uint8_t media_endpoint_get_codec(struct media_endpoint *endpoint);
> >>   struct btd_adapter *media_endpoint_get_btd_adapter(
> >>                                          struct media_endpoint *endpoint);
> >>   bool media_endpoint_is_broadcast(struct media_endpoint *endpoint);
> >> -int8_t media_player_get_device_volume(struct btd_device *device);
> >>
> >>   const struct media_endpoint *media_endpoint_get_asha(void);
> >>
> >> ---
> >> base-commit: 2c0c323d08357a4ff3065fcd49fee0c83b5835cd
> >> change-id: 20250805-audio-no-reuse-media-player-volume-fbc2983a287a
> >>
> >> Best regards,
> >> --
> >> Myrrh Periwinkle<myrrhperiwinkle@qtmlabs.xyz>
> >>
> >>



-- 
Luiz Augusto von Dentz

Re: [PATCH bluez] audio: Don't initialize device volume from media player

[Myrrh Periwinkle]

Hi,

On 8/5/25 23:01, Luiz Augusto von Dentz wrote:
> Hi Myrrh,
>
> On Tue, Aug 5, 2025 at 11:34 AM Myrrh Periwinkle
> <myrrhperiwinkle@qtmlabs.xyz> wrote:
>> Resent because I forgot to use "Reply All"
>>
>> On 8/5/25 22:24, Luiz Augusto von Dentz wrote:
>>> Hi Myrrh,
>>>
>>> On Tue, Aug 5, 2025 at 6:29 AM Myrrh Periwinkle
>>> <myrrhperiwinkle@qtmlabs.xyz> wrote:
>>>> Media player objects may be shared between devices. As a result,
>>>> a device without support for hardware volume that is connected after one
>>>> that does may end up being erroneously considered hardware
>>>> volume-capable.
>>> Don't quite follow, avrcp_player is per device, not sure how it can be
>>> shared between devices?
>>>
>>>> fa7828bdd ("transport: Fix not being able to initialize volume properly")
>>>> introduced btd_device_{get,set}_volume that is used as an alternative in
>>>> case no media player objects are present. Therefore, we can remove
>>>> media_player_get_device_volume and instead use btd_device_get_volume to
>>>> determine the initial volume.
>>> Don't follow you here, why shouldn;t we use the media player if we have one?
>>>
>>>> ---
>>>>    profiles/audio/avrcp.c |  2 +-
>>>>    profiles/audio/media.c | 33 +--------------------------------
>>>>    profiles/audio/media.h |  1 -
>>>>    3 files changed, 2 insertions(+), 34 deletions(-)
>>>>
>>>> diff --git a/profiles/audio/avrcp.c b/profiles/audio/avrcp.c
>>>> index e2797112fcd580c3fc56793f933e00b1c61e5205..ec07522e6a34eb1dc5f6f413f48f1087a609df9a 100644
>>>> --- a/profiles/audio/avrcp.c
>>>> +++ b/profiles/audio/avrcp.c
>>>> @@ -4284,7 +4284,7 @@ static void target_init(struct avrcp *session)
>>>>                   target->player = player;
>>>>                   player->sessions = g_slist_prepend(player->sessions, session);
>>>>
>>>> -               init_volume = media_player_get_device_volume(session->dev);
>>>> +               init_volume = btd_device_get_volume(session->dev);
>>>>                   media_transport_update_device_volume(session->dev, init_volume);
>>>>           }
>>>>
>>>> diff --git a/profiles/audio/media.c b/profiles/audio/media.c
>>>> index 8e62dca17070edbc5101677c6eebd3707492c824..55f1482d1d9ce52e104481bab3ede373f47aee0c 100644
>>>> --- a/profiles/audio/media.c
>>>> +++ b/profiles/audio/media.c
>>>> @@ -499,37 +499,6 @@ struct a2dp_config_data {
>>>>           a2dp_endpoint_config_t cb;
>>>>    };
>>>>
>>>> -int8_t media_player_get_device_volume(struct btd_device *device)
>>>> -{
>>>> -#ifdef HAVE_AVRCP
>>>> -       struct avrcp_player *target_player;
>>>> -       struct media_adapter *adapter;
>>>> -       GSList *l;
>>>> -
>>>> -       if (!device)
>>>> -               return -1;
>>>> -
>>>> -       target_player = avrcp_get_target_player_by_device(device);
>>>> -       if (!target_player)
>>>> -               goto done;
>>>> -
>>>> -       adapter = find_adapter(device);
>>>> -       if (!adapter)
>>>> -               goto done;
>>>> -
>>>> -       for (l = adapter->players; l; l = l->next) {
>>>> -               struct media_player *mp = l->data;
>>>> -
>>>> -               if (mp->player == target_player)
>>>> -                       return mp->volume;
>> The `avrcp_player` object indeed is not shared between devices, but the
>> volume is acquired from (and set for) the associated `media_player`
>> object which is not tied to a specific device, and corresponds to client
>> `org.mpris.MediaPlayer2.Player` objects.
> Ok, so what is the problem with that? If there are 2 headsets that
> wants to control the same player registered via mpris-proxy(?) that
> should be allowed, or the problem is that each device should control a
> dedicated instance of the volume? I suspect the volume store in
> mp->volume is only used as initial volume since the actual volume is

I searched the codebase for all usages of `mp->volume` and seems like 
this is solely used as a temporary storage for the initial volume. This 
usage was first introduced in 4b6153b0501cf18812cb869c2320c41e51f81adc 
and the field itself was introduced in 
a282fff97dabbd3a814765a868e3367cb3dc1ce3 (with unclear purposes).

> stored in the Transport.Volume, the reason why we can't use the
> Transport directly is that it is created on demand while there is a
> stream so if the stream is not ready at time a volume has been set we
> need to store it elsewhere, that said perhaps that should be store per
> device not per media_player which seem to be registered per adapter in
> media.c but then we should probably remove the volume field altogether
> to avoid any confusion in the future.
>
>>>> -       }
>>>> -
>>>> -done:
>>>> -#endif /* HAVE_AVRCP */
>>>> -       /* If media_player doesn't exists use device_volume */
>>>> -       return btd_device_get_volume(device);
>>>> -}
>>>> -
>>>>    static gboolean set_configuration(struct media_endpoint *endpoint,
>>>>                                           uint8_t *configuration, size_t size,
>>>>                                           media_endpoint_cb_t cb,
>>>> @@ -556,7 +525,7 @@ static gboolean set_configuration(struct media_endpoint *endpoint,
>>>>           if (transport == NULL)
>>>>                   return FALSE;
>>>>
>>>> -       init_volume = media_player_get_device_volume(device);
>>>> +       init_volume = btd_device_get_volume(device);
>>>>           media_transport_update_volume(transport, init_volume);
>>>>
>>>>           msg = dbus_message_new_method_call(endpoint->sender, endpoint->path,
>>>> diff --git a/profiles/audio/media.h b/profiles/audio/media.h
>>>> index 2b2e8e1572874d5f71abb28fdd5b92fa2d9efe83..d3954abd6de505a69cab3fcffc217d236a52d3e5 100644
>>>> --- a/profiles/audio/media.h
>>>> +++ b/profiles/audio/media.h
>>>> @@ -23,6 +23,5 @@ uint8_t media_endpoint_get_codec(struct media_endpoint *endpoint);
>>>>    struct btd_adapter *media_endpoint_get_btd_adapter(
>>>>                                           struct media_endpoint *endpoint);
>>>>    bool media_endpoint_is_broadcast(struct media_endpoint *endpoint);
>>>> -int8_t media_player_get_device_volume(struct btd_device *device);
>>>>
>>>>    const struct media_endpoint *media_endpoint_get_asha(void);
>>>>
>>>> ---
>>>> base-commit: 2c0c323d08357a4ff3065fcd49fee0c83b5835cd
>>>> change-id: 20250805-audio-no-reuse-media-player-volume-fbc2983a287a
>>>>
>>>> Best regards,
>>>> --
>>>> Myrrh Periwinkle<myrrhperiwinkle@qtmlabs.xyz>
>>>>
>>>>
-Myrrh

Re: [PATCH bluez] audio: Don't initialize device volume from media player

[Myrrh Periwinkle]

On 8/5/25 23:08, Myrrh Periwinkle wrote:
> Hi,
>
> On 8/5/25 23:01, Luiz Augusto von Dentz wrote:
>> Hi Myrrh,
>>
>> On Tue, Aug 5, 2025 at 11:34 AM Myrrh Periwinkle
>> <myrrhperiwinkle@qtmlabs.xyz> wrote:
>>> Resent because I forgot to use "Reply All"
>>>
>>> On 8/5/25 22:24, Luiz Augusto von Dentz wrote:
>>>> Hi Myrrh,
>>>>
>>>> On Tue, Aug 5, 2025 at 6:29 AM Myrrh Periwinkle
>>>> <myrrhperiwinkle@qtmlabs.xyz> wrote:
>>>>> Media player objects may be shared between devices. As a result,
>>>>> a device without support for hardware volume that is connected 
>>>>> after one
>>>>> that does may end up being erroneously considered hardware
>>>>> volume-capable.
>>>> Don't quite follow, avrcp_player is per device, not sure how it can be
>>>> shared between devices?
>>>>
>>>>> fa7828bdd ("transport: Fix not being able to initialize volume 
>>>>> properly")
>>>>> introduced btd_device_{get,set}_volume that is used as an 
>>>>> alternative in
>>>>> case no media player objects are present. Therefore, we can remove
>>>>> media_player_get_device_volume and instead use 
>>>>> btd_device_get_volume to
>>>>> determine the initial volume.
>>>> Don't follow you here, why shouldn;t we use the media player if we 
>>>> have one?
>>>>
>>>>> ---
>>>>>    profiles/audio/avrcp.c |  2 +-
>>>>>    profiles/audio/media.c | 33 +--------------------------------
>>>>>    profiles/audio/media.h |  1 -
>>>>>    3 files changed, 2 insertions(+), 34 deletions(-)
>>>>>
>>>>> diff --git a/profiles/audio/avrcp.c b/profiles/audio/avrcp.c
>>>>> index 
>>>>> e2797112fcd580c3fc56793f933e00b1c61e5205..ec07522e6a34eb1dc5f6f413f48f1087a609df9a 
>>>>> 100644
>>>>> --- a/profiles/audio/avrcp.c
>>>>> +++ b/profiles/audio/avrcp.c
>>>>> @@ -4284,7 +4284,7 @@ static void target_init(struct avrcp *session)
>>>>>                   target->player = player;
>>>>>                   player->sessions = 
>>>>> g_slist_prepend(player->sessions, session);
>>>>>
>>>>> -               init_volume = 
>>>>> media_player_get_device_volume(session->dev);
>>>>> +               init_volume = btd_device_get_volume(session->dev);
>>>>> media_transport_update_device_volume(session->dev, init_volume);
>>>>>           }
>>>>>
>>>>> diff --git a/profiles/audio/media.c b/profiles/audio/media.c
>>>>> index 
>>>>> 8e62dca17070edbc5101677c6eebd3707492c824..55f1482d1d9ce52e104481bab3ede373f47aee0c 
>>>>> 100644
>>>>> --- a/profiles/audio/media.c
>>>>> +++ b/profiles/audio/media.c
>>>>> @@ -499,37 +499,6 @@ struct a2dp_config_data {
>>>>>           a2dp_endpoint_config_t cb;
>>>>>    };
>>>>>
>>>>> -int8_t media_player_get_device_volume(struct btd_device *device)
>>>>> -{
>>>>> -#ifdef HAVE_AVRCP
>>>>> -       struct avrcp_player *target_player;
>>>>> -       struct media_adapter *adapter;
>>>>> -       GSList *l;
>>>>> -
>>>>> -       if (!device)
>>>>> -               return -1;
>>>>> -
>>>>> -       target_player = avrcp_get_target_player_by_device(device);
>>>>> -       if (!target_player)
>>>>> -               goto done;
>>>>> -
>>>>> -       adapter = find_adapter(device);
>>>>> -       if (!adapter)
>>>>> -               goto done;
>>>>> -
>>>>> -       for (l = adapter->players; l; l = l->next) {
>>>>> -               struct media_player *mp = l->data;
>>>>> -
>>>>> -               if (mp->player == target_player)
>>>>> -                       return mp->volume;
>>> The `avrcp_player` object indeed is not shared between devices, but the
>>> volume is acquired from (and set for) the associated `media_player`
>>> object which is not tied to a specific device, and corresponds to 
>>> client
>>> `org.mpris.MediaPlayer2.Player` objects.
>> Ok, so what is the problem with that? If there are 2 headsets that
>> wants to control the same player registered via mpris-proxy(?) that
>> should be allowed, or the problem is that each device should control a
>> dedicated instance of the volume? I suspect the volume store in
>> mp->volume is only used as initial volume since the actual volume is
>
> I searched the codebase for all usages of `mp->volume` and seems like 
> this is solely used as a temporary storage for the initial volume. 
> This usage was first introduced in 
> 4b6153b0501cf18812cb869c2320c41e51f81adc and the field itself was 
> introduced in a282fff97dabbd3a814765a868e3367cb3dc1ce3 (with unclear 
> purposes).
>
>> stored in the Transport.Volume, the reason why we can't use the
>> Transport directly is that it is created on demand while there is a
>> stream so if the stream is not ready at time a volume has been set we
>> need to store it elsewhere, that said perhaps that should be store per
>> device not per media_player which seem to be registered per adapter in
>> media.c but then we should probably remove the volume field altogether
>> to avoid any confusion in the future.

For some additional context: This issue only started happening when I 
started using mpris-proxy. Without mpris-proxy the device without 
hardware volume support is always recognized as such. What I believe is 
happening is that the volume for the first device with hardware volume 
support is set on the media_player object which is then used as the 
initial volume for the second device that doesn't have hardware volume.

I will soon send a v2 that removes the media_player.volume field 
entirely since it doesn't appear to be used for anything else.

>>
>>>>> -       }
>>>>> -
>>>>> -done:
>>>>> -#endif /* HAVE_AVRCP */
>>>>> -       /* If media_player doesn't exists use device_volume */
>>>>> -       return btd_device_get_volume(device);
>>>>> -}
>>>>> -
>>>>>    static gboolean set_configuration(struct media_endpoint *endpoint,
>>>>>                                           uint8_t *configuration, 
>>>>> size_t size,
>>>>> media_endpoint_cb_t cb,
>>>>> @@ -556,7 +525,7 @@ static gboolean set_configuration(struct 
>>>>> media_endpoint *endpoint,
>>>>>           if (transport == NULL)
>>>>>                   return FALSE;
>>>>>
>>>>> -       init_volume = media_player_get_device_volume(device);
>>>>> +       init_volume = btd_device_get_volume(device);
>>>>>           media_transport_update_volume(transport, init_volume);
>>>>>
>>>>>           msg = dbus_message_new_method_call(endpoint->sender, 
>>>>> endpoint->path,
>>>>> diff --git a/profiles/audio/media.h b/profiles/audio/media.h
>>>>> index 
>>>>> 2b2e8e1572874d5f71abb28fdd5b92fa2d9efe83..d3954abd6de505a69cab3fcffc217d236a52d3e5 
>>>>> 100644
>>>>> --- a/profiles/audio/media.h
>>>>> +++ b/profiles/audio/media.h
>>>>> @@ -23,6 +23,5 @@ uint8_t media_endpoint_get_codec(struct 
>>>>> media_endpoint *endpoint);
>>>>>    struct btd_adapter *media_endpoint_get_btd_adapter(
>>>>>                                           struct media_endpoint 
>>>>> *endpoint);
>>>>>    bool media_endpoint_is_broadcast(struct media_endpoint *endpoint);
>>>>> -int8_t media_player_get_device_volume(struct btd_device *device);
>>>>>
>>>>>    const struct media_endpoint *media_endpoint_get_asha(void);
>>>>>
>>>>> ---
>>>>> base-commit: 2c0c323d08357a4ff3065fcd49fee0c83b5835cd
>>>>> change-id: 20250805-audio-no-reuse-media-player-volume-fbc2983a287a
>>>>>
>>>>> Best regards,
>>>>> -- 
>>>>> Myrrh Periwinkle<myrrhperiwinkle@qtmlabs.xyz>
>>>>>
>>>>>
> -Myrrh

-Myrrh

Re: [PATCH bluez] audio: Don't initialize device volume from media player

[patchwork-bot+bluetooth]

Hello:

This patch was applied to bluetooth/bluez.git (master)
by Luiz Augusto von Dentz <luiz.von.dentz@intel.com>:

On Tue, 05 Aug 2025 17:21:28 +0700 you wrote:
> Media player objects may be shared between devices. As a result,
> a device without support for hardware volume that is connected after one
> that does may end up being erroneously considered hardware
> volume-capable.
> 
> fa7828bdd ("transport: Fix not being able to initialize volume properly")
> introduced btd_device_{get,set}_volume that is used as an alternative in
> case no media player objects are present. Therefore, we can remove
> media_player_get_device_volume and instead use btd_device_get_volume to
> determine the initial volume.
> 
> [...]

Here is the summary with links:
  - [bluez] audio: Don't initialize device volume from media player
    https://git.kernel.org/pub/scm/bluetooth/bluez.git/?id=5db6d2fef66f

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html