Hi Pauli,
On Sun, Oct 10, 2021 at 10:20 AM Pauli Virtanen <pav@xxxxxx> wrote:
>
> Some devices may send AVRCP VolumeChanged notification before AVDTP
> SetConfiguration occurs, and not send another until a hardware button on
> the device is pressed. If a media_player is registered to BlueZ, the
> volume from the event is stored on the player, and used as init_volume
> for new transports. However, if no media_player is registered,
> transports are created with volume missing.
>
> If that occurs, the DBus "Volume" attribute on transports will be
> missing until a hardware button is pressed. Consequently, applications
> cannot get or set volume, even though it is actually possible.
>
> Address this by keeping track of the last device volume set in AVRCP
> session. If no media_player is registered, use that as the init_volume
> for new transports. This has a similar effect as if a dummy media
> player was registered.
>
> This fixes AVRCP absolute volume not being available on some headphones
> on Pipewire & Pulseaudio until HW button press.
> ---
> profiles/audio/avrcp.c | 23 +++++++++++++++++++++++
> profiles/audio/avrcp.h | 1 +
> profiles/audio/media.c | 3 +++
> 3 files changed, 27 insertions(+)
>
> diff --git a/profiles/audio/avrcp.c b/profiles/audio/avrcp.c
> index 7c280203c..0df416d2c 100644
> --- a/profiles/audio/avrcp.c
> +++ b/profiles/audio/avrcp.c
> @@ -276,6 +276,8 @@ struct avrcp {
> uint8_t transaction;
> uint8_t transaction_events[AVRCP_EVENT_LAST + 1];
> struct pending_pdu *pending_pdu;
> +
> + int8_t last_device_volume;
We can probably keep this short and just call it volume.
> };
>
> struct passthrough_handler {
> @@ -1759,6 +1761,7 @@ static uint8_t avrcp_handle_set_absolute_volume(struct avrcp *session,
> volume = pdu->params[0] & 0x7F;
>
> media_transport_update_device_volume(session->dev, volume);
> + session->last_device_volume = volume;
>
> return AVC_CTYPE_ACCEPTED;
>
> @@ -3731,6 +3734,7 @@ static void avrcp_volume_changed(struct avrcp *session,
>
> /* Always attempt to update the transport volume */
> media_transport_update_device_volume(session->dev, volume);
> + session->last_device_volume = volume;
>
> if (player)
> player->cb->set_volume(volume, session->dev, player->user_data);
> @@ -4145,6 +4149,7 @@ static void target_init(struct avrcp *session)
>
> init_volume = media_player_get_device_volume(session->dev);
> media_transport_update_device_volume(session->dev, init_volume);
> + session->last_device_volume = init_volume;
> }
>
> session->supported_events |= (1 << AVRCP_EVENT_STATUS_CHANGED) |
> @@ -4308,6 +4313,7 @@ static struct avrcp *session_create(struct avrcp_server *server,
> session->server = server;
> session->conn = avctp_connect(device);
> session->dev = device;
> + session->last_device_volume = -1;
>
> server->sessions = g_slist_append(server->sessions, session);
>
> @@ -4497,6 +4503,7 @@ static gboolean avrcp_handle_set_volume(struct avctp *conn, uint8_t code,
>
> /* Always attempt to update the transport volume */
> media_transport_update_device_volume(session->dev, volume);
> + session->last_device_volume = volume;
So if I understand this right we are going to cache the volume here
since media_transport_update_device_volume may not have a transport
yet? If that is the case we probably need to be checking if there is a
transport or not beforehand instead of doing this blindly.
>
> if (player != NULL)
> player->cb->set_volume(volume, session->dev, player->user_data);
> @@ -4598,6 +4605,22 @@ int avrcp_set_volume(struct btd_device *dev, int8_t volume, bool notify)
> avrcp_handle_set_volume, session);
> }
>
> +int8_t avrcp_get_last_volume(struct btd_device *dev)
> +{
> + struct avrcp_server *server;
> + struct avrcp *session;
> +
> + server = find_server(servers, device_get_adapter(dev));
> + if (server == NULL)
> + return -1;
> +
> + session = find_session(server->sessions, dev);
> + if (session == NULL)
> + return -1;
> +
> + return session->last_device_volume;
> +}
> +
> struct avrcp_player *avrcp_get_target_player_by_device(struct btd_device *dev)
> {
> struct avrcp_server *server;
> diff --git a/profiles/audio/avrcp.h b/profiles/audio/avrcp.h
> index dcc580e37..952f0eea9 100644
> --- a/profiles/audio/avrcp.h
> +++ b/profiles/audio/avrcp.h
> @@ -91,6 +91,7 @@ struct avrcp_player_cb {
> };
>
> int avrcp_set_volume(struct btd_device *dev, int8_t volume, bool notify);
> +int8_t avrcp_get_last_volume(struct btd_device *dev);
Let's have it as avrcp_get_volume so it is symmetric to avrcp_set_volume.
> struct avrcp_player *avrcp_register_player(struct btd_adapter *adapter,
> struct avrcp_player_cb *cb,
> diff --git a/profiles/audio/media.c b/profiles/audio/media.c
> index 521902ed8..a37378393 100644
> --- a/profiles/audio/media.c
> +++ b/profiles/audio/media.c
> @@ -494,6 +494,9 @@ static gboolean set_configuration(struct media_endpoint *endpoint,
> return FALSE;
>
> init_volume = media_player_get_device_volume(device);
> + if (init_volume < 0)
> + init_volume = avrcp_get_last_volume(device);
I wonder if we shouldn't be better to move the call to
avrcp_get_volume inside media_player_get_device_volume so it does the
fallback automatically if there is no volume set.
> media_transport_update_volume(transport, init_volume);
>
> msg = dbus_message_new_method_call(endpoint->sender, endpoint->path,
> --
> 2.31.1
>
--
Luiz Augusto von Dentz
