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;
MIME-Version: 1.0
References: <20210607184616.22051-1-sonnysasaka@chromium.org>
In-Reply-To: <20210607184616.22051-1-sonnysasaka@chromium.org>
From:   Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Date:   Mon, 7 Jun 2021 14:16:18 -0700
Message-ID: <CABBYNZKcr74zYzPMcLo5+-49Fv02Kxoyf11bNPSuSGWThz-eqQ@mail.gmail.com>
Subject: Re: [PATCH BlueZ] Queue SetAbsoluteVolume if there is an in-progress one.
To:     Sonny Sasaka <sonnysasaka@chromium.org>
Cc:     "linux-bluetooth@vger.kernel.org" <linux-bluetooth@vger.kernel.org>,
        Archie Pusaka <apusaka@chromium.org>,
        Miao-chen Chou <mcchou@chromium.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Hi Sonny,

On Mon, Jun 7, 2021 at 1:40 PM Sonny Sasaka <sonnysasaka@chromium.org> wrote:
>
> 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.

You have to explain better what is the situation here, does the upper
layer queue several volume changes in a row and why? If this is coming
from different entities then there is obviously a conflict, but I
think we only allow the volume to changed from the entity that is
handling the A2DP stream.

> 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;

I rather have a volume and volume_pending with the respectively
current volume and volume change in progress, if notification comes
with volume_pending then we are done otherwise we need to send another
command, only the last volume_pending is tracked so we don't have to
queue anything since changes done in between would be override only
the last volume change matters.

>         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);
> +       }

Here it would be something like:

  if (session->volume_pending != -1) {
    if (session->volume_pending != volume)
      avrcp_set_volume(session->dev, session->volume_pending, true);
    else
      session->volume_pending = -1;
  }

Though there is a problem with the above, if for some odd reason the
device don't notify the exact volume we requested this will lead the
an endless loop since the volume would never match.

> +
>         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) {

Let do something like the following:

  if (session->volume_pending != -1 && session->volume_pending != volume) {
    session->volume_pending = value;
    return 0;
   }

> +               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
>


-- 
Luiz Augusto von Dentz