diff mbox series

[BlueZ] Queue SetAbsoluteVolume if there is an in-progress one.

Message ID 20210607184616.22051-1-sonnysasaka@chromium.org
State New
Headers show
Series [BlueZ] Queue SetAbsoluteVolume if there is an in-progress one. | expand

Commit Message

Sonny Sasaka June 7, 2021, 6:46 p.m. UTC 
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(+)

Comments

bluez.test.bot@gmail.com June 7, 2021, 8:15 p.m. UTC | #1 
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=495605

---Test result---

Test Summary:
CheckPatch                    PASS      0.34 seconds
GitLint                       FAIL      0.10 seconds
Prep - Setup ELL              PASS      42.64 seconds
Build - Prep                  PASS      0.10 seconds
Build - Configure             PASS      7.40 seconds
Build - Make                  FAIL      138.65 seconds
Make Check                    FAIL      1.28 seconds
Make Distcheck                PASS      208.78 seconds
Build w/ext ELL - Configure   PASS      8.20 seconds
Build w/ext ELL - Make        FAIL      126.52 seconds

Details
##############################
Test: CheckPatch - PASS
Desc: Run checkpatch.pl script with rule in .checkpatch.conf

##############################
Test: GitLint - FAIL
Desc: Run gitlint with rule in .gitlint
Output:
Queue SetAbsoluteVolume if there is an in-progress one.
1: T3 Title has trailing punctuation (.): "Queue SetAbsoluteVolume if there is an in-progress one."


##############################
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 - FAIL
Desc: Build the BlueZ source tree
Output:
profiles/audio/avrcp.c:4266:6: error: no previous declaration for ‘update_queued_set_volume’ [-Werror=missing-declarations]
 4266 | void update_queued_set_volume(struct avrcp *session, uint8_t volume,
      |      ^~~~~~~~~~~~~~~~~~~~~~~~
profiles/audio/avrcp.c:4276:6: error: no previous declaration for ‘clear_queued_set_volume’ [-Werror=missing-declarations]
 4276 | void clear_queued_set_volume(struct avrcp *session)
      |      ^~~~~~~~~~~~~~~~~~~~~~~
cc1: all warnings being treated as errors
make[1]: *** [Makefile:8615: profiles/audio/bluetoothd-avrcp.o] Error 1
make: *** [Makefile:4134: all] Error 2


##############################
Test: Make Check - FAIL
Desc: Run 'make check'
Output:
profiles/audio/avrcp.c:4266:6: error: no previous declaration for ‘update_queued_set_volume’ [-Werror=missing-declarations]
 4266 | void update_queued_set_volume(struct avrcp *session, uint8_t volume,
      |      ^~~~~~~~~~~~~~~~~~~~~~~~
profiles/audio/avrcp.c:4276:6: error: no previous declaration for ‘clear_queued_set_volume’ [-Werror=missing-declarations]
 4276 | void clear_queued_set_volume(struct avrcp *session)
      |      ^~~~~~~~~~~~~~~~~~~~~~~
cc1: all warnings being treated as errors
make[1]: *** [Makefile:8615: profiles/audio/bluetoothd-avrcp.o] Error 1
make: *** [Makefile:10406: check] Error 2


##############################
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 - FAIL
Desc: Build BlueZ source with '--enable-external-ell' configuration
Output:
profiles/audio/avrcp.c:4266:6: error: no previous declaration for ‘update_queued_set_volume’ [-Werror=missing-declarations]
 4266 | void update_queued_set_volume(struct avrcp *session, uint8_t volume,
      |      ^~~~~~~~~~~~~~~~~~~~~~~~
profiles/audio/avrcp.c:4276:6: error: no previous declaration for ‘clear_queued_set_volume’ [-Werror=missing-declarations]
 4276 | void clear_queued_set_volume(struct avrcp *session)
      |      ^~~~~~~~~~~~~~~~~~~~~~~
cc1: all warnings being treated as errors
make[1]: *** [Makefile:8615: profiles/audio/bluetoothd-avrcp.o] Error 1
make: *** [Makefile:4134: all] Error 2




---
Regards,
Linux Bluetooth
diff mbox series

Patch

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),