Hi Archie,
On Thu, Jul 23, 2020 at 12:23 AM Archie Pusaka <apusaka@xxxxxxxxxx> wrote:
>
> From: Archie Pusaka <apusaka@xxxxxxxxxxxx>
>
> 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 we also use signed type, so
> negative values can be used to indicate invalid volume.
>
> Reviewed-by: Michael Sun <michaelfsun@xxxxxxxxxx>
> ---
>
> Changes in v5:
> -Fix coding style and conversion error
>
> Changes in v4:
> -Use int8_t instead of uint8_t
>
> Changes in v3: None
> Changes in v2: None
>
> profiles/audio/avrcp.c | 17 +++++++++-----
> profiles/audio/avrcp.h | 35 ++++++++++++++---------------
> profiles/audio/media.c | 4 ++--
> profiles/audio/transport.c | 45 +++++++++++++++++++-------------------
> profiles/audio/transport.h | 8 +++----
> 5 files changed, 56 insertions(+), 53 deletions(-)
>
> diff --git a/profiles/audio/avrcp.c b/profiles/audio/avrcp.c
> index 1bf85041e..7a06c8353 100644
> --- a/profiles/audio/avrcp.c
> +++ b/profiles/audio/avrcp.c
> @@ -1595,6 +1595,7 @@ static uint8_t avrcp_handle_register_notification(struct avrcp *session,
> struct btd_device *dev = session->dev;
> uint16_t len = ntohs(pdu->params_len);
> uint64_t uid;
> + int8_t volume;
> GList *settings;
>
> /*
> @@ -1659,10 +1660,11 @@ static uint8_t avrcp_handle_register_notification(struct avrcp *session,
> len = 1;
> break;
> case AVRCP_EVENT_VOLUME_CHANGED:
> - pdu->params[1] = media_transport_get_device_volume(dev);
> - if (pdu->params[1] > 127)
> + volume = media_transport_get_device_volume(dev);
> + if (volume < 0)
> goto err;
>
> + pdu->params[1] = volume;
> len = 2;
>
> break;
> @@ -1757,7 +1759,7 @@ static uint8_t avrcp_handle_set_absolute_volume(struct avrcp *session,
> uint8_t transaction)
> {
> uint16_t len = ntohs(pdu->params_len);
> - uint8_t volume;
> + int8_t volume;
>
> if (len != 1)
> goto err;
> @@ -3623,7 +3625,7 @@ static void avrcp_volume_changed(struct avrcp *session,
> struct avrcp_header *pdu)
> {
> struct avrcp_player *player = target_get_player(session);
> - uint8_t volume;
> + int8_t volume;
>
> volume = pdu->params[1] & 0x7F;
>
> @@ -4371,7 +4373,7 @@ static gboolean avrcp_handle_set_volume(struct avctp *conn, uint8_t code,
> struct avrcp *session = user_data;
> struct avrcp_player *player = target_get_player(session);
> struct avrcp_header *pdu = (void *) operands;
> - uint8_t volume;
> + int8_t volume;
>
> if (code == AVC_CTYPE_REJECTED || code == AVC_CTYPE_NOT_IMPLEMENTED ||
> pdu == NULL)
> @@ -4434,13 +4436,16 @@ static int avrcp_event(struct avrcp *session, uint8_t id, const void *data)
> return err;
> }
>
> -int avrcp_set_volume(struct btd_device *dev, uint8_t volume, bool notify)
> +int avrcp_set_volume(struct btd_device *dev, int8_t volume, bool notify)
> {
> struct avrcp_server *server;
> struct avrcp *session;
> uint8_t buf[AVRCP_HEADER_LENGTH + 1];
> struct avrcp_header *pdu = (void *) buf;
>
> + if (volume < 0)
> + return -EINVAL;
> +
> server = find_server(servers, device_get_adapter(dev));
> if (server == NULL)
> return -EINVAL;
> diff --git a/profiles/audio/avrcp.h b/profiles/audio/avrcp.h
> index 86d310c73..a08e7325e 100644
> --- a/profiles/audio/avrcp.h
> +++ b/profiles/audio/avrcp.h
> @@ -83,27 +83,27 @@
> #define AVRCP_EVENT_LAST AVRCP_EVENT_VOLUME_CHANGED
>
> struct avrcp_player_cb {
> - GList *(*list_settings) (void *user_data);
> - const char *(*get_setting) (const char *key, void *user_data);
> - int (*set_setting) (const char *key, const char *value,
> + GList *(*list_settings)(void *user_data);
> + const char *(*get_setting)(const char *key, void *user_data);
> + int (*set_setting)(const char *key, const char *value,
> void *user_data);
> - uint64_t (*get_uid) (void *user_data);
> - const char *(*get_metadata) (const char *key, void *user_data);
> - GList *(*list_metadata) (void *user_data);
> - const char *(*get_status) (void *user_data);
> - uint32_t (*get_position) (void *user_data);
> - uint32_t (*get_duration) (void *user_data);
> - const char *(*get_name) (void *user_data);
> - void (*set_volume) (uint8_t volume, struct btd_device *dev,
> + uint64_t (*get_uid)(void *user_data);
> + const char *(*get_metadata)(const char *key, void *user_data);
> + GList *(*list_metadata)(void *user_data);
> + const char *(*get_status)(void *user_data);
> + uint32_t (*get_position)(void *user_data);
> + uint32_t (*get_duration)(void *user_data);
> + const char *(*get_name)(void *user_data);
> + void (*set_volume)(int8_t volume, struct btd_device *dev,
> void *user_data);
> - bool (*play) (void *user_data);
> - bool (*stop) (void *user_data);
> - bool (*pause) (void *user_data);
> - bool (*next) (void *user_data);
> - bool (*previous) (void *user_data);
> + bool (*play)(void *user_data);
> + bool (*stop)(void *user_data);
> + bool (*pause)(void *user_data);
> + bool (*next)(void *user_data);
> + bool (*previous)(void *user_data);
> };
>
> -int avrcp_set_volume(struct btd_device *dev, uint8_t volume, bool notify);
> +int avrcp_set_volume(struct btd_device *dev, int8_t volume, bool notify);
>
> struct avrcp_player *avrcp_register_player(struct btd_adapter *adapter,
> struct avrcp_player_cb *cb,
> @@ -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/media.c b/profiles/audio/media.c
> index a0173fdd4..d4d58ec86 100644
> --- a/profiles/audio/media.c
> +++ b/profiles/audio/media.c
> @@ -120,7 +120,7 @@ struct media_player {
> char *status;
> uint32_t position;
> uint32_t duration;
> - uint8_t volume;
> + int8_t volume;
> GTimer *timer;
> bool play;
> bool pause;
> @@ -1199,7 +1199,7 @@ static uint32_t get_duration(void *user_data)
> return mp->duration;
> }
>
> -static void set_volume(uint8_t volume, struct btd_device *dev, void *user_data)
> +static void set_volume(int8_t volume, struct btd_device *dev, void *user_data)
> {
> struct media_player *mp = user_data;
>
> diff --git a/profiles/audio/transport.c b/profiles/audio/transport.c
> index 48fabba9b..2ae5118c4 100644
> --- a/profiles/audio/transport.c
> +++ b/profiles/audio/transport.c
> @@ -86,7 +86,7 @@ struct media_owner {
> struct a2dp_transport {
> struct avdtp *session;
> uint16_t delay;
> - uint16_t volume;
> + int8_t volume;
> };
>
> struct media_transport {
> @@ -634,7 +634,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 a2dp->volume >= 0;
> }
>
> static gboolean get_volume(const GDBusPropertyTable *property,
> @@ -642,8 +642,9 @@ static gboolean get_volume(const GDBusPropertyTable *property,
> {
> struct media_transport *transport = data;
> struct a2dp_transport *a2dp = transport->data;
> + uint16_t volume = (uint16_t)a2dp->volume;
>
> - dbus_message_iter_append_basic(iter, DBUS_TYPE_UINT16, &a2dp->volume);
> + dbus_message_iter_append_basic(iter, DBUS_TYPE_UINT16, &volume);
>
> return TRUE;
> }
> @@ -654,27 +655,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;
> + int8_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);
> -
> - if (volume > 127) {
> - g_dbus_pending_property_error(id,
> - ERROR_INTERFACE ".InvalidArguments",
> - "Invalid arguments in method call");
> - return;
> - }
> + dbus_message_iter_get_basic(iter, &arg);
> + if (arg > INT8_MAX)
> + goto error;
>
> g_dbus_pending_property_success(id);
>
> + volume = (int8_t)arg;
> if (a2dp->volume == volume)
> return;
>
> @@ -688,6 +682,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)
> @@ -931,14 +930,14 @@ struct btd_device *media_transport_get_dev(struct media_transport *transport)
> return transport->device;
> }
>
> -uint16_t media_transport_get_volume(struct media_transport *transport)
> +int8_t media_transport_get_volume(struct media_transport *transport)
> {
> struct a2dp_transport *a2dp = transport->data;
> return a2dp->volume;
> }
>
> void media_transport_update_volume(struct media_transport *transport,
> - uint8_t volume)
> + int8_t volume)
> {
> struct a2dp_transport *a2dp = transport->data;
>
> @@ -953,12 +952,12 @@ void media_transport_update_volume(struct media_transport *transport,
> MEDIA_TRANSPORT_INTERFACE, "Volume");
> }
>
> -uint8_t media_transport_get_device_volume(struct btd_device *dev)
> +int8_t media_transport_get_device_volume(struct btd_device *dev)
> {
> GSList *l;
>
> if (dev == NULL)
> - return 128;
> + return -1;
>
> for (l = transports; l; l = l->next) {
> struct media_transport *transport = l->data;
> @@ -974,7 +973,7 @@ uint8_t media_transport_get_device_volume(struct btd_device *dev)
> }
>
> void media_transport_update_device_volume(struct btd_device *dev,
> - uint8_t volume)
> + int8_t volume)
> {
> GSList *l;
>
> diff --git a/profiles/audio/transport.h b/profiles/audio/transport.h
> index ac542bf6c..78024372f 100644
> --- a/profiles/audio/transport.h
> +++ b/profiles/audio/transport.h
> @@ -32,14 +32,14 @@ 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);
> +int8_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,
> - uint8_t volume);
> + int8_t volume);
> void transport_get_properties(struct media_transport *transport,
> DBusMessageIter *iter);
>
> -uint8_t media_transport_get_device_volume(struct btd_device *dev);
> +int8_t media_transport_get_device_volume(struct btd_device *dev);
> void media_transport_update_device_volume(struct btd_device *dev,
> - uint8_t volume);
> + int8_t volume);
> --
> 2.28.0.rc0.105.gf9edc3c819-goog
Applied, thanks.
--
Luiz Augusto von Dentz
