Received-SPF: pass (google.com: best guess record for domain of linux-bluetooth-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
From:   Marijn Suijten <marijns95@gmail.com>
To:     linux-bluetooth@vger.kernel.org
Cc:     luiz.von.dentz@intel.com, Marijn Suijten <marijns95@gmail.com>
Subject: [PATCH] audio: avrcp: Always update transport volume regardless of player.
Date:   Sat, 18 Jan 2020 16:31:38 +0100
Message-Id: <20200118153138.240216-1-marijns95@gmail.com>
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Sender: linux-bluetooth-owner@vger.kernel.org
Precedence: bulk

From: Marijn Suijten <marijns95@gmail.com>

`Volume` is a special property that not only exists on players but also
on the transport (see org.bluez.MediaTransport1). A player is not
attached when the controller does not support FEATURE_CATEGORY_1, which
is common on headphones without media browsing capabilities.

On such audio devices (headphones, in-ears and the like) Absolute Volume
is not available unless an external player is registered
(org.bluez.Media1.RegisterPlayer) _and_ the device sends a volume event
back after that to set a2dp->volume in transport.c to a valid value
(causing volume_exists to finally return true).

The mail thread https://marc.info/?l=linux-bluetooth&m=145337574806153
which denotes the same issue has a solution to at least request
capabilities from the controller, but any notifications received on
AVRCP_EVENT_VOLUME_CHANGED that is subsequently registered will be
ignored by avrcp_volume_changed unless a player is present.

This issue is not addressed by adding a fake player but instead dealing
with the fact that volume is "special" and available on the transport
regardless of the existence of a player. This can be seen in
avrcp_get_capabilities_resp as well which requires a player to register
any event except AVRCP_EVENT_VOLUME_CHANGED.

Updating the transport in avrcp_volume_changed and
avrcp_handle_set_volume leaves set_volume in media.c in an awkward state
since the call here becomes superfluous, which has thus been removed.
The volume member of the media_player is never used which seems a result
of updating from org.bluez.MediaPlayer1 to org.mpris.MediaPlayer2.Player
in 15e421737ccc4696ed567edcc24d178aedb47854, where the volume property:
https://specifications.freedesktop.org/mpris-spec/2.2/Player_Interface.html#Property:Volume
is left out.

Signed-off-by: Marijn Suijten <marijns95@gmail.com>
---
Hi,

I would have submitted a patch to 3-way synchronize the volume between
an AVRCP-connected device, the transport and the mpris player, but an
attempt thus far has shown my inability to properly and fully understand
the structure. Most notably it is not obvious to me how registered
applications work on the org.bluez.Media1 interface.

I assume the following scenarios need to be dealt with:

- The device running bluez plays back media to a bluetooth device
  (headphones, a (car) speaker, and the like). mpris-proxy is used to
  register an exposed mpris player to org.bluez.Media1.RegisterPlayer,
  and this should allow the player application to show and control the
  volume. Any change in volume should be sent to the AVRCP remote using
  avrcp_set_volume(notify=false) and reflected in the transport, which
  happens automatically when the remote confirms the volume using
  avrcp_handle_set_volume.

- The device running bluez receives an audio stream from a remote device
  (and usually plays that back on the attached speakers). In this case
  media information from the remote device is exposed on
  org.bluez.MediaPlayer1, and this is where my understanding stops: it
  looks like applications can register themselves on
  org.bluez.Media1.RegisterApplication, and in turn have bluez add or
  notify their ObjectManager with a new mpris player?

Finally, I mentioned in the patch that updating the transport volume in
set_volume on the media_player has now become superfluous. Or at least
that's what I assume, since there is always only a single transport
associated with an endpoint. set_volume iterates endpoints whereas
media_transport_update_device_volume iterates transports, both of which
should result in the same being updated (are there such cases with
multiple a2dp transports?), correct?

Thanks in advance for considering this patch, looking forward to your
replies/clarifications.

- Marijn Suijten

 profiles/audio/avrcp.c | 12 ++++++++----
 profiles/audio/media.c | 16 ----------------
 2 files changed, 8 insertions(+), 20 deletions(-)

diff --git a/profiles/audio/avrcp.c b/profiles/audio/avrcp.c
index 6b3f685d2..7fb8af608 100644
--- a/profiles/audio/avrcp.c
+++ b/profiles/audio/avrcp.c
@@ -3548,12 +3548,13 @@ static void avrcp_volume_changed(struct avrcp *session,
 	struct avrcp_player *player = target_get_player(session);
 	uint8_t volume;
 
-	if (!player)
-		return;
-
 	volume = pdu->params[1] & 0x7F;
 
-	player->cb->set_volume(volume, session->dev, player->user_data);
+	/* Always update the transport volume, which is separate from the player */
+	media_transport_update_device_volume(session->dev, volume);
+
+	if (player)
+		player->cb->set_volume(volume, session->dev, player->user_data);
 }
 
 static void avrcp_status_changed(struct avrcp *session,
@@ -4296,6 +4297,9 @@ static gboolean avrcp_handle_set_volume(struct avctp *conn, uint8_t code,
 
 	volume = pdu->params[0] & 0x7F;
 
+	/* Always update the transport volume, which is separate from the player */
+	media_transport_update_device_volume(session->dev, volume);
+
 	if (player != NULL)
 		player->cb->set_volume(volume, session->dev, player->user_data);
 
diff --git a/profiles/audio/media.c b/profiles/audio/media.c
index 993ecb3b3..a0173fdd4 100644
--- a/profiles/audio/media.c
+++ b/profiles/audio/media.c
@@ -1202,27 +1202,11 @@ static uint32_t get_duration(void *user_data)
 static void set_volume(uint8_t volume, struct btd_device *dev, void *user_data)
 {
 	struct media_player *mp = user_data;
-	GSList *l;
 
 	if (mp->volume == volume)
 		return;
 
 	mp->volume = volume;
-
-	for (l = mp->adapter->endpoints; l; l = l->next) {
-		struct media_endpoint *endpoint = l->data;
-		struct media_transport *transport;
-
-		/* Volume is A2DP only */
-		if (endpoint->sep == NULL)
-			continue;
-
-		transport = find_device_transport(endpoint, dev);
-		if (transport == NULL)
-			continue;
-
-		media_transport_update_volume(transport, volume);
-	}
 }
 
 static bool media_player_send(struct media_player *mp, const char *name)
-- 
2.25.0