diff mbox series

[BlueZ,v3,2/3] audio/avrcp: Only allow absolute volume call/event on category-2 peers

Message ID 20230311003826.454858-3-marijn.suijten@somainline.org
State New
Headers show
Series audio/avrcp: Determine Absolute Volume support from feature category 2 | expand

Commit Message

Marijn Suijten March 11, 2023, 12:38 a.m. UTC
Restrict the use of SetAbsoluteVolume and EVENT_VOLUME_CHANGED to peers
with at least AVRCP version 1.4 and AVRCP_FEATURE_CATEGORY_2 on their
respective target or controller profiles.
---
 profiles/audio/avrcp.c | 39 ++++++++++++++++++++++++++++++++++-----
 1 file changed, 34 insertions(+), 5 deletions(-)

Comments

Luiz Augusto von Dentz March 14, 2023, 11:26 p.m. UTC | #1
Hi Marijn,

On Fri, Mar 10, 2023 at 4:39 PM Marijn Suijten
<marijn.suijten@somainline.org> wrote:
>
> Restrict the use of SetAbsoluteVolume and EVENT_VOLUME_CHANGED to peers
> with at least AVRCP version 1.4 and AVRCP_FEATURE_CATEGORY_2 on their
> respective target or controller profiles.

Hmm, couldn't this actually make things even worse since we now are
checking the category as well? I know this is by the spec but as we
already are experiencing some stacks tend to deviate a lot from the
spec, perhaps it would have been better to introduce another config
option e.g. VolumeCategory = true so people can roll back to the old
behavior if their devices don't work well with this changes.

> ---
>  profiles/audio/avrcp.c | 39 ++++++++++++++++++++++++++++++++++-----
>  1 file changed, 34 insertions(+), 5 deletions(-)
>
> diff --git a/profiles/audio/avrcp.c b/profiles/audio/avrcp.c
> index 5e6322916..c16f9cfef 100644
> --- a/profiles/audio/avrcp.c
> +++ b/profiles/audio/avrcp.c
> @@ -1757,6 +1757,16 @@ static uint8_t avrcp_handle_set_absolute_volume(struct avrcp *session,
>         if (len != 1)
>                 goto err;
>
> +       /**
> +        * The controller on the remote end is only allowed to call SetAbsoluteVolume
> +        * on our target if it's at least version 1.4 and a category-2 device.
> +        */
> +       if (!session->target || session->target->version < 0x0104 ||
> +                       !(session->target->features & AVRCP_FEATURE_CATEGORY_2)) {
> +               error("Remote SetAbsoluteVolume rejected from non-category-2 peer");
> +               goto err;
> +       }
> +
>         volume = pdu->params[0] & 0x7F;
>
>         media_transport_update_device_volume(session->dev, volume);
> @@ -3728,6 +3738,16 @@ static void avrcp_volume_changed(struct avrcp *session,
>         struct avrcp_player *player = target_get_player(session);
>         int8_t volume;
>
> +       /**
> +        * The target on the remote end is only allowed to reply to EVENT_VOLUME_CHANGED
> +        * on our controller if it's at least version 1.4 and a category-2 device.
> +        */
> +       if (!session->controller || session->controller->version < 0x0104 ||
> +                       !(session->controller->features & AVRCP_FEATURE_CATEGORY_2)) {
> +               error("Remote EVENT_VOLUME_CHANGED rejected from non-category-2 peer");
> +               return;
> +       }
> +
>         volume = pdu->params[1] & 0x7F;
>
>         /* Always attempt to update the transport volume */
> @@ -3981,7 +4001,7 @@ static gboolean avrcp_get_capabilities_resp(struct avctp *conn, uint8_t code,
>                 case AVRCP_EVENT_ADDRESSED_PLAYER_CHANGED:
>                 case AVRCP_EVENT_UIDS_CHANGED:
>                 case AVRCP_EVENT_AVAILABLE_PLAYERS_CHANGED:
> -                       /* These events above requires a player */
> +                       /* These events above require a player */
>                         if (!session->controller ||
>                                                 !session->controller->player)
>                                 break;
> @@ -4154,10 +4174,13 @@ static void target_init(struct avrcp *session)
>         if (target->version < 0x0104)
>                 return;
>
> +       if (target->features & AVRCP_FEATURE_CATEGORY_2)
> +               session->supported_events |=
> +                               (1 << AVRCP_EVENT_VOLUME_CHANGED);
> +
>         session->supported_events |=
>                                 (1 << AVRCP_EVENT_ADDRESSED_PLAYER_CHANGED) |
> -                               (1 << AVRCP_EVENT_AVAILABLE_PLAYERS_CHANGED) |
> -                               (1 << AVRCP_EVENT_VOLUME_CHANGED);
> +                               (1 << AVRCP_EVENT_AVAILABLE_PLAYERS_CHANGED);
>
>         /* Only check capabilities if controller is not supported */
>         if (session->controller == NULL)
> @@ -4572,8 +4595,11 @@ int avrcp_set_volume(struct btd_device *dev, int8_t volume, bool notify)
>                 return -ENOTCONN;
>
>         if (notify) {
> -               if (!session->target)
> +               if (!session->target || session->target->version < 0x0104 ||
> +                               !(session->target->features & AVRCP_FEATURE_CATEGORY_2)) {
> +                       error("Can't send EVENT_VOLUME_CHANGED to non-category-2 peer");
>                         return -ENOTSUP;
> +               }
>                 return avrcp_event(session, AVRCP_EVENT_VOLUME_CHANGED,
>                                                                 &volume);
>         }
> @@ -4583,8 +4609,11 @@ int avrcp_set_volume(struct btd_device *dev, int8_t volume, bool notify)
>                                                 AVRCP_EVENT_VOLUME_CHANGED))
>                         return -ENOTSUP;
>         } else {
> -               if (!session->controller || session->controller->version < 0x0104)
> +               if (!session->controller || session->controller->version < 0x0104 ||
> +                               !(session->controller->features & AVRCP_FEATURE_CATEGORY_2)) {
> +                       error("Can't send SetAbsoluteVolume to non-category-2 peer");
>                         return -ENOTSUP;
> +               }
>         }
>
>         memset(buf, 0, sizeof(buf));
> --
> 2.39.2
>
diff mbox series

Patch

diff --git a/profiles/audio/avrcp.c b/profiles/audio/avrcp.c
index 5e6322916..c16f9cfef 100644
--- a/profiles/audio/avrcp.c
+++ b/profiles/audio/avrcp.c
@@ -1757,6 +1757,16 @@  static uint8_t avrcp_handle_set_absolute_volume(struct avrcp *session,
 	if (len != 1)
 		goto err;
 
+	/**
+	 * The controller on the remote end is only allowed to call SetAbsoluteVolume
+	 * on our target if it's at least version 1.4 and a category-2 device.
+	 */
+	if (!session->target || session->target->version < 0x0104 ||
+			!(session->target->features & AVRCP_FEATURE_CATEGORY_2)) {
+		error("Remote SetAbsoluteVolume rejected from non-category-2 peer");
+		goto err;
+	}
+
 	volume = pdu->params[0] & 0x7F;
 
 	media_transport_update_device_volume(session->dev, volume);
@@ -3728,6 +3738,16 @@  static void avrcp_volume_changed(struct avrcp *session,
 	struct avrcp_player *player = target_get_player(session);
 	int8_t volume;
 
+	/**
+	 * The target on the remote end is only allowed to reply to EVENT_VOLUME_CHANGED
+	 * on our controller if it's at least version 1.4 and a category-2 device.
+	 */
+	if (!session->controller || session->controller->version < 0x0104 ||
+			!(session->controller->features & AVRCP_FEATURE_CATEGORY_2)) {
+		error("Remote EVENT_VOLUME_CHANGED rejected from non-category-2 peer");
+		return;
+	}
+
 	volume = pdu->params[1] & 0x7F;
 
 	/* Always attempt to update the transport volume */
@@ -3981,7 +4001,7 @@  static gboolean avrcp_get_capabilities_resp(struct avctp *conn, uint8_t code,
 		case AVRCP_EVENT_ADDRESSED_PLAYER_CHANGED:
 		case AVRCP_EVENT_UIDS_CHANGED:
 		case AVRCP_EVENT_AVAILABLE_PLAYERS_CHANGED:
-			/* These events above requires a player */
+			/* These events above require a player */
 			if (!session->controller ||
 						!session->controller->player)
 				break;
@@ -4154,10 +4174,13 @@  static void target_init(struct avrcp *session)
 	if (target->version < 0x0104)
 		return;
 
+	if (target->features & AVRCP_FEATURE_CATEGORY_2)
+		session->supported_events |=
+				(1 << AVRCP_EVENT_VOLUME_CHANGED);
+
 	session->supported_events |=
 				(1 << AVRCP_EVENT_ADDRESSED_PLAYER_CHANGED) |
-				(1 << AVRCP_EVENT_AVAILABLE_PLAYERS_CHANGED) |
-				(1 << AVRCP_EVENT_VOLUME_CHANGED);
+				(1 << AVRCP_EVENT_AVAILABLE_PLAYERS_CHANGED);
 
 	/* Only check capabilities if controller is not supported */
 	if (session->controller == NULL)
@@ -4572,8 +4595,11 @@  int avrcp_set_volume(struct btd_device *dev, int8_t volume, bool notify)
 		return -ENOTCONN;
 
 	if (notify) {
-		if (!session->target)
+		if (!session->target || session->target->version < 0x0104 ||
+				!(session->target->features & AVRCP_FEATURE_CATEGORY_2)) {
+			error("Can't send EVENT_VOLUME_CHANGED to non-category-2 peer");
 			return -ENOTSUP;
+		}
 		return avrcp_event(session, AVRCP_EVENT_VOLUME_CHANGED,
 								&volume);
 	}
@@ -4583,8 +4609,11 @@  int avrcp_set_volume(struct btd_device *dev, int8_t volume, bool notify)
 						AVRCP_EVENT_VOLUME_CHANGED))
 			return -ENOTSUP;
 	} else {
-		if (!session->controller || session->controller->version < 0x0104)
+		if (!session->controller || session->controller->version < 0x0104 ||
+				!(session->controller->features & AVRCP_FEATURE_CATEGORY_2)) {
+			error("Can't send SetAbsoluteVolume to non-category-2 peer");
 			return -ENOTSUP;
+		}
 	}
 
 	memset(buf, 0, sizeof(buf));