[Bluez PATCH v1 1/2] audio/transport: change volume to 8bit

[Bluez PATCH v1 1/2] audio/transport: change volume to 8bit

[Archie Pusaka]

From: Archie Pusaka <apusaka@chromium.org>

The valid range of volume is 0 - 127, yet it is stored in 16bit
data type. This patch modifies it so we use 8bit data type to
store volume instead.

Furthermore, this patch introduces helper function and defined
values to check for volume validity, to prevent numbers
scattered all over.

Reviewed-by: Michael Sun <michaelfsun@google.com>
---

 profiles/audio/avrcp.c     |  2 +-
 profiles/audio/avrcp.h     |  1 -
 profiles/audio/transport.c | 46 ++++++++++++++++++++++----------------
 profiles/audio/transport.h |  3 ++-
 4 files changed, 30 insertions(+), 22 deletions(-)

diff --git a/profiles/audio/avrcp.c b/profiles/audio/avrcp.c
index e2428250e..a11b3ef6e 100644
--- a/profiles/audio/avrcp.c
+++ b/profiles/audio/avrcp.c
@@ -1660,7 +1660,7 @@ static uint8_t avrcp_handle_register_notification(struct avrcp *session,
 		break;
 	case AVRCP_EVENT_VOLUME_CHANGED:
 		pdu->params[1] = media_transport_get_device_volume(dev);
-		if (pdu->params[1] > 127)
+		if (!media_transport_volume_valid(pdu->params[1]))
 			goto err;
 
 		len = 2;
diff --git a/profiles/audio/avrcp.h b/profiles/audio/avrcp.h
index 86d310c73..3fd74e18a 100644
--- a/profiles/audio/avrcp.h
+++ b/profiles/audio/avrcp.h
@@ -114,6 +114,5 @@ void avrcp_unregister_player(struct avrcp_player *player);
 void avrcp_player_event(struct avrcp_player *player, uint8_t id,
 							const void *data);
 
-
 size_t avrcp_handle_vendor_reject(uint8_t *code, uint8_t *operands);
 size_t avrcp_browsing_general_reject(uint8_t *operands);
diff --git a/profiles/audio/transport.c b/profiles/audio/transport.c
index 48fabba9b..a32073380 100644
--- a/profiles/audio/transport.c
+++ b/profiles/audio/transport.c
@@ -55,6 +55,8 @@
 
 #define MEDIA_TRANSPORT_INTERFACE "org.bluez.MediaTransport1"
 
+#define UNINITIALIZED_VOLUME_VALUE	128
+
 typedef enum {
 	TRANSPORT_STATE_IDLE,		/* Not acquired and suspended */
 	TRANSPORT_STATE_PENDING,	/* Playing but not acquired */
@@ -86,7 +88,7 @@ struct media_owner {
 struct a2dp_transport {
 	struct avdtp		*session;
 	uint16_t		delay;
-	uint16_t		volume;
+	uint8_t			volume;
 };
 
 struct media_transport {
@@ -634,7 +636,7 @@ static gboolean volume_exists(const GDBusPropertyTable *property, void *data)
 	struct media_transport *transport = data;
 	struct a2dp_transport *a2dp = transport->data;
 
-	return a2dp->volume <= 127;
+	return media_transport_volume_valid(a2dp->volume);
 }
 
 static gboolean get_volume(const GDBusPropertyTable *property,
@@ -654,24 +656,20 @@ static void set_volume(const GDBusPropertyTable *property,
 {
 	struct media_transport *transport = data;
 	struct a2dp_transport *a2dp = transport->data;
-	uint16_t volume;
+	uint16_t arg;
+	uint8_t volume;
 	bool notify;
 
-	if (dbus_message_iter_get_arg_type(iter) != DBUS_TYPE_UINT16) {
-		g_dbus_pending_property_error(id,
-					ERROR_INTERFACE ".InvalidArguments",
-					"Invalid arguments in method call");
-		return;
-	}
+	if (dbus_message_iter_get_arg_type(iter) != DBUS_TYPE_UINT16)
+		goto error;
 
-	dbus_message_iter_get_basic(iter, &volume);
+	dbus_message_iter_get_basic(iter, &arg);
+	if (arg > UINT8_MAX)
+		goto error;
 
-	if (volume > 127) {
-		g_dbus_pending_property_error(id,
-					ERROR_INTERFACE ".InvalidArguments",
-					"Invalid arguments in method call");
-		return;
-	}
+	volume = (uint8_t)arg;
+	if (!media_transport_volume_valid(volume))
+		goto error;
 
 	g_dbus_pending_property_success(id);
 
@@ -688,6 +686,11 @@ static void set_volume(const GDBusPropertyTable *property,
 						"Volume");
 
 	avrcp_set_volume(transport->device, volume, notify);
+	return;
+
+error:
+	g_dbus_pending_property_error(id, ERROR_INTERFACE ".InvalidArguments",
+					"Invalid arguments in method call");
 }
 
 static gboolean endpoint_exists(const GDBusPropertyTable *property, void *data)
@@ -824,7 +827,7 @@ static int media_transport_init_source(struct media_transport *transport)
 	transport->data = a2dp;
 	transport->destroy = destroy_a2dp;
 
-	a2dp->volume = -1;
+	a2dp->volume = UNINITIALIZED_VOLUME_VALUE;
 	transport->sink_watch = sink_add_state_cb(service, sink_state_changed,
 								transport);
 
@@ -931,7 +934,7 @@ struct btd_device *media_transport_get_dev(struct media_transport *transport)
 	return transport->device;
 }
 
-uint16_t media_transport_get_volume(struct media_transport *transport)
+uint8_t media_transport_get_volume(struct media_transport *transport)
 {
 	struct a2dp_transport *a2dp = transport->data;
 	return a2dp->volume;
@@ -958,7 +961,7 @@ uint8_t media_transport_get_device_volume(struct btd_device *dev)
 	GSList *l;
 
 	if (dev == NULL)
-		return 128;
+		return UNINITIALIZED_VOLUME_VALUE;
 
 	for (l = transports; l; l = l->next) {
 		struct media_transport *transport = l->data;
@@ -991,3 +994,8 @@ void media_transport_update_device_volume(struct btd_device *dev,
 			media_transport_update_volume(transport, volume);
 	}
 }
+
+bool media_transport_volume_valid(uint8_t volume)
+{
+	return volume < 128;
+}
diff --git a/profiles/audio/transport.h b/profiles/audio/transport.h
index ac542bf6c..c430515f2 100644
--- a/profiles/audio/transport.h
+++ b/profiles/audio/transport.h
@@ -32,7 +32,7 @@ struct media_transport *media_transport_create(struct btd_device *device,
 void media_transport_destroy(struct media_transport *transport);
 const char *media_transport_get_path(struct media_transport *transport);
 struct btd_device *media_transport_get_dev(struct media_transport *transport);
-uint16_t media_transport_get_volume(struct media_transport *transport);
+uint8_t media_transport_get_volume(struct media_transport *transport);
 void media_transport_update_delay(struct media_transport *transport,
 							uint16_t delay);
 void media_transport_update_volume(struct media_transport *transport,
@@ -43,3 +43,4 @@ void transport_get_properties(struct media_transport *transport,
 uint8_t media_transport_get_device_volume(struct btd_device *dev);
 void media_transport_update_device_volume(struct btd_device *dev,
 								uint8_t volume);
+bool media_transport_volume_valid(uint8_t volume);
-- 
2.27.0.383.g050319c2ae-goog

[Bluez PATCH v1 2/2] audio/transport: store volume for initialization

[Archie Pusaka]

From: Archie Pusaka <apusaka@chromium.org>

Sometimes the response of RegisterNotification for volume change
event came before we create the transport for the corresponding
device. If that happens, we lose the volume information. After the
transport is created, the volume is also potentially stuck to an
uninitialized invalid value. The property Volume of
MediaTransport1 will also be left unaccessible.

This patch stores the value of the volume notification response.
When the transport is initialized, we try to match the device
with the previously stored volume and assign that value instead.

Reviewed-by: Howard Chung <howardchung@google.com>
---

 profiles/audio/media.c     | 17 +----------
 profiles/audio/transport.c | 61 ++++++++++++++++++++++++++++++++++++--
 2 files changed, 60 insertions(+), 18 deletions(-)

diff --git a/profiles/audio/media.c b/profiles/audio/media.c
index 993ecb3b3..be1ca18ee 100644
--- a/profiles/audio/media.c
+++ b/profiles/audio/media.c
@@ -1202,27 +1202,12 @@ 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);
-	}
+	media_transport_update_device_volume(dev, volume);
 }
 
 static bool media_player_send(struct media_player *mp, const char *name)
diff --git a/profiles/audio/transport.c b/profiles/audio/transport.c
index a32073380..2fd04dd42 100644
--- a/profiles/audio/transport.c
+++ b/profiles/audio/transport.c
@@ -56,6 +56,7 @@
 #define MEDIA_TRANSPORT_INTERFACE "org.bluez.MediaTransport1"
 
 #define UNINITIALIZED_VOLUME_VALUE	128
+#define PEND_DEVICE_VOLUME_TIMEOUT	1
 
 typedef enum {
 	TRANSPORT_STATE_IDLE,		/* Not acquired and suspended */
@@ -116,7 +117,13 @@ struct media_transport {
 	void			*data;
 };
 
+struct pending_device_volume {
+	struct btd_device	*device;
+	uint8_t			volume;
+};
+
 static GSList *transports = NULL;
+static GSList *pending_device_volumes;
 
 static const char *state2str(transport_state_t state)
 {
@@ -810,6 +817,52 @@ static void source_state_changed(struct btd_service *service,
 		transport_update_playing(transport, FALSE);
 }
 
+static uint8_t get_pending_device_volume(struct btd_device *dev)
+{
+	GSList *l;
+
+	for (l = pending_device_volumes; l; l = l->next) {
+		struct pending_device_volume *pend = l->data;
+
+		if (pend->device == dev)
+			return pend->volume;
+	}
+
+	return UNINITIALIZED_VOLUME_VALUE;
+}
+
+static gboolean remove_pending_device_volume(gpointer user_data)
+{
+	struct pending_device_volume *pend = user_data;
+
+	pending_device_volumes = g_slist_remove(pending_device_volumes, pend);
+	g_free(pend);
+
+	return FALSE;
+}
+
+static void add_pending_device_volume(struct btd_device *dev, uint8_t volume)
+{
+	GSList *l;
+	struct pending_device_volume *pend;
+
+	for (l = pending_device_volumes; l; l = l->next) {
+		pend = l->data;
+
+		if (pend->device == dev) {
+			pend->volume = volume;
+			return;
+		}
+	}
+
+	pend = g_new0(struct pending_device_volume, 1);
+	pend->device = dev;
+	pend->volume = volume;
+	g_timeout_add_seconds(PEND_DEVICE_VOLUME_TIMEOUT,
+				remove_pending_device_volume, pend);
+	pending_device_volumes = g_slist_append(pending_device_volumes, pend);
+}
+
 static int media_transport_init_source(struct media_transport *transport)
 {
 	struct btd_service *service;
@@ -827,7 +880,7 @@ static int media_transport_init_source(struct media_transport *transport)
 	transport->data = a2dp;
 	transport->destroy = destroy_a2dp;
 
-	a2dp->volume = UNINITIALIZED_VOLUME_VALUE;
+	a2dp->volume = get_pending_device_volume(transport->device);
 	transport->sink_watch = sink_add_state_cb(service, sink_state_changed,
 								transport);
 
@@ -990,9 +1043,13 @@ void media_transport_update_device_volume(struct btd_device *dev,
 			continue;
 
 		/* Volume is A2DP only */
-		if (media_endpoint_get_sep(transport->endpoint))
+		if (media_endpoint_get_sep(transport->endpoint)) {
 			media_transport_update_volume(transport, volume);
+			return;
+		}
 	}
+
+	add_pending_device_volume(dev, volume);
 }
 
 bool media_transport_volume_valid(uint8_t volume)
-- 
2.27.0.383.g050319c2ae-goog

Re: [Bluez PATCH v1 2/2] audio/transport: store volume for initialization

[Luiz Augusto von Dentz]

Hi Archie,

On Thu, Jul 9, 2020 at 9:32 PM Archie Pusaka <apusaka@google.com> wrote:
>
> From: Archie Pusaka <apusaka@chromium.org>
>
> Sometimes the response of RegisterNotification for volume change
> event came before we create the transport for the corresponding
> device. If that happens, we lose the volume information. After the
> transport is created, the volume is also potentially stuck to an
> uninitialized invalid value. The property Volume of
> MediaTransport1 will also be left unaccessible.
>
> This patch stores the value of the volume notification response.
> When the transport is initialized, we try to match the device
> with the previously stored volume and assign that value instead.
>
> Reviewed-by: Howard Chung <howardchung@google.com>
> ---
>
>  profiles/audio/media.c     | 17 +----------
>  profiles/audio/transport.c | 61 ++++++++++++++++++++++++++++++++++++--
>  2 files changed, 60 insertions(+), 18 deletions(-)
>
> diff --git a/profiles/audio/media.c b/profiles/audio/media.c
> index 993ecb3b3..be1ca18ee 100644
> --- a/profiles/audio/media.c
> +++ b/profiles/audio/media.c
> @@ -1202,27 +1202,12 @@ 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);
> -       }
> +       media_transport_update_device_volume(dev, volume);
>  }
>
>  static bool media_player_send(struct media_player *mp, const char *name)
> diff --git a/profiles/audio/transport.c b/profiles/audio/transport.c
> index a32073380..2fd04dd42 100644
> --- a/profiles/audio/transport.c
> +++ b/profiles/audio/transport.c
> @@ -56,6 +56,7 @@
>  #define MEDIA_TRANSPORT_INTERFACE "org.bluez.MediaTransport1"
>
>  #define UNINITIALIZED_VOLUME_VALUE     128
> +#define PEND_DEVICE_VOLUME_TIMEOUT     1
>
>  typedef enum {
>         TRANSPORT_STATE_IDLE,           /* Not acquired and suspended */
> @@ -116,7 +117,13 @@ struct media_transport {
>         void                    *data;
>  };
>
> +struct pending_device_volume {
> +       struct btd_device       *device;
> +       uint8_t                 volume;
> +};
> +
>  static GSList *transports = NULL;
> +static GSList *pending_device_volumes;
>
>  static const char *state2str(transport_state_t state)
>  {
> @@ -810,6 +817,52 @@ static void source_state_changed(struct btd_service *service,
>                 transport_update_playing(transport, FALSE);
>  }
>
> +static uint8_t get_pending_device_volume(struct btd_device *dev)
> +{
> +       GSList *l;
> +
> +       for (l = pending_device_volumes; l; l = l->next) {
> +               struct pending_device_volume *pend = l->data;
> +
> +               if (pend->device == dev)
> +                       return pend->volume;
> +       }
> +
> +       return UNINITIALIZED_VOLUME_VALUE;
> +}
> +
> +static gboolean remove_pending_device_volume(gpointer user_data)
> +{
> +       struct pending_device_volume *pend = user_data;
> +
> +       pending_device_volumes = g_slist_remove(pending_device_volumes, pend);
> +       g_free(pend);
> +
> +       return FALSE;
> +}
> +
> +static void add_pending_device_volume(struct btd_device *dev, uint8_t volume)
> +{
> +       GSList *l;
> +       struct pending_device_volume *pend;
> +
> +       for (l = pending_device_volumes; l; l = l->next) {
> +               pend = l->data;
> +
> +               if (pend->device == dev) {
> +                       pend->volume = volume;
> +                       return;
> +               }
> +       }
> +
> +       pend = g_new0(struct pending_device_volume, 1);
> +       pend->device = dev;
> +       pend->volume = volume;
> +       g_timeout_add_seconds(PEND_DEVICE_VOLUME_TIMEOUT,
> +                               remove_pending_device_volume, pend);
> +       pending_device_volumes = g_slist_append(pending_device_volumes, pend);
> +}
> +
>  static int media_transport_init_source(struct media_transport *transport)
>  {
>         struct btd_service *service;
> @@ -827,7 +880,7 @@ static int media_transport_init_source(struct media_transport *transport)
>         transport->data = a2dp;
>         transport->destroy = destroy_a2dp;
>
> -       a2dp->volume = UNINITIALIZED_VOLUME_VALUE;
> +       a2dp->volume = get_pending_device_volume(transport->device);
>         transport->sink_watch = sink_add_state_cb(service, sink_state_changed,
>                                                                 transport);
>
> @@ -990,9 +1043,13 @@ void media_transport_update_device_volume(struct btd_device *dev,
>                         continue;
>
>                 /* Volume is A2DP only */
> -               if (media_endpoint_get_sep(transport->endpoint))
> +               if (media_endpoint_get_sep(transport->endpoint)) {
>                         media_transport_update_volume(transport, volume);
> +                       return;
> +               }
>         }
> +
> +       add_pending_device_volume(dev, volume);
>  }
>
>  bool media_transport_volume_valid(uint8_t volume)
> --
> 2.27.0.383.g050319c2ae-goog

This sounds a little too complicated, what I would have done it is to
locate the media_player (if there is one), and then pass its stored
volume (mp->volume) to media_transport_create so instead of always
initialize it 127 we actually pass value if it was already fetched and
stored by the media_player instance. Btw, I guess once we merge this
changes we should close the following bug:

https://github.com/bluez/bluez/issues/17

-- 
Luiz Augusto von Dentz