Received-SPF: pass (google.com: domain of linux-bluetooth-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   Sonny Sasaka <sonnysasaka@chromium.org>
To:     linux-bluetooth@vger.kernel.org
Cc:     Sonny Sasaka <sonnysasaka@chromium.org>,
        Archie Pusaka <apusaka@chromium.org>,
        Miao-chen Chou <mcchou@chromium.org>
Subject: [PATCH BlueZ] Queue SetAbsoluteVolume if there is an in-progress one.
Date:   Mon,  7 Jun 2021 11:46:16 -0700
Message-Id: <20210607184616.22051-1-sonnysasaka@chromium.org>
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Precedence: bulk

SetAbsoluteVolume command may receive late response for Target Device
that have high latency processing. In that case we may send the next
SetAbsoluteVolume commands before the previous SetAbsoluteVolume
response is received. This causes the media transport volume to jitter.

The solution in this patch is to not send any SetAbsoluteVolume command
if there is an in-progress one. Instead we should queue this command to
be executed after the in-progress one receives the response.

Reviewed-by: Archie Pusaka <apusaka@chromium.org>
Reviewed-by: Miao-chen Chou <mcchou@chromium.org>

---
 profiles/audio/avrcp.c | 49 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 49 insertions(+)

diff --git a/profiles/audio/avrcp.c b/profiles/audio/avrcp.c
index ccf34b220..c6946dc46 100644
--- a/profiles/audio/avrcp.c
+++ b/profiles/audio/avrcp.c
@@ -256,6 +256,11 @@ struct avrcp_data {
 	GSList *players;
 };
 
+struct set_volume_command {
+	uint8_t volume;
+	bool notify;
+};
+
 struct avrcp {
 	struct avrcp_server *server;
 	struct avctp *conn;
@@ -275,6 +280,12 @@ struct avrcp {
 	uint8_t transaction;
 	uint8_t transaction_events[AVRCP_EVENT_LAST + 1];
 	struct pending_pdu *pending_pdu;
+	// Whether there is a SetAbsoluteVolume command that is still waiting
+	// for response.
+	bool is_set_volume_in_progress;
+	// If this is non-null, then we need to issue SetAbsoluteVolume
+	// after the current in-progress SetAbsoluteVolume receives response.
+	struct set_volume_command *queued_set_volume;
 };
 
 struct passthrough_handler {
@@ -4252,6 +4263,24 @@ static void target_destroy(struct avrcp *session)
 	g_free(target);
 }
 
+void update_queued_set_volume(struct avrcp *session, uint8_t volume,
+				bool notify)
+{
+	if (!session->queued_set_volume)
+		session->queued_set_volume = g_new0(struct set_volume_command,
+							1);
+	session->queued_set_volume->volume = volume;
+	session->queued_set_volume->notify = notify;
+}
+
+void clear_queued_set_volume(struct avrcp *session)
+{
+	if (!session->queued_set_volume)
+		return;
+	g_free(session->queued_set_volume);
+	session->queued_set_volume = NULL;
+}
+
 static void session_destroy(struct avrcp *session, int err)
 {
 	struct avrcp_server *server = session->server;
@@ -4295,6 +4324,8 @@ static void session_destroy(struct avrcp *session, int err)
 	if (session->browsing_id > 0)
 		avctp_unregister_browsing_pdu_handler(session->browsing_id);
 
+	clear_queued_set_volume(session);
+
 	g_free(session);
 }
 
@@ -4486,6 +4517,8 @@ static gboolean avrcp_handle_set_volume(struct avctp *conn, uint8_t code,
 	struct avrcp_header *pdu = (void *) operands;
 	int8_t volume;
 
+	session->is_set_volume_in_progress = false;
+
 	if (code == AVC_CTYPE_REJECTED || code == AVC_CTYPE_NOT_IMPLEMENTED ||
 								pdu == NULL)
 		return FALSE;
@@ -4495,6 +4528,13 @@ 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);
 
+	if (session->queued_set_volume) {
+		avrcp_set_volume(session->dev,
+					session->queued_set_volume->volume,
+					session->queued_set_volume->notify);
+		clear_queued_set_volume(session);
+	}
+
 	if (player != NULL)
 		player->cb->set_volume(volume, session->dev, player->user_data);
 
@@ -4570,6 +4610,14 @@ int avrcp_set_volume(struct btd_device *dev, int8_t volume, bool notify)
 	if (session == NULL)
 		return -ENOTCONN;
 
+	// If there is an in-progress SetAbsoluteVolume, just update the
+	// queued_set_volume. Once the in-progress SetAbsoluteVolume receives
+	// response, it will send the queued SetAbsoluteVolume command.
+	if (session->is_set_volume_in_progress) {
+		update_queued_set_volume(session, volume, notify);
+		return 0;
+	}
+
 	if (notify) {
 		if (!session->target)
 			return -ENOTSUP;
@@ -4589,6 +4637,7 @@ int avrcp_set_volume(struct btd_device *dev, int8_t volume, bool notify)
 	pdu->params[0] = volume;
 	pdu->params_len = htons(1);
 
+	session->is_set_volume_in_progress = TRUE;
 	return avctp_send_vendordep_req(session->conn,
 					AVC_CTYPE_CONTROL, AVC_SUBUNIT_PANEL,
 					buf, sizeof(buf),
-- 
2.31.0