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

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 <[e-mail]>
Reviewed-by: Miao-chen Chou <[e-mail]>

---
 profiles/audio/avrcp.c | 49 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 49 insertions(+)

Comments

[e-mail] 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

Luiz Augusto von Dentz June 7, 2021, 9:16 p.m. UTC | #2

Hi Sonny,

On Mon, Jun 7, 2021 at 1:40 PM Sonny Sasaka <[e-mail]> 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 <[e-mail]>
> Reviewed-by: Miao-chen Chou <[e-mail]>
>
> ---
>  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
>

Marijn Suijten June 7, 2021, 10:46 p.m. UTC | #3

Hi Luiz, Sonny,

On 6/7/21 11:16 PM, Luiz Augusto von Dentz wrote:
> Hi Sonny,
> 
> On Mon, Jun 7, 2021 at 1:40 PM Sonny Sasaka <[e-mail]> 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.


We've been running into a similar issue with PulseAudio.  There is no 
way to track a Set call for the Volume property back to the 
property-change notification, running into both jitter on quick 
successive volume changes and the inability to reflect peer hardware 
volume in case the peer rounds the requested volume to a coarser scale. 
  This rounded value is supposedly returned from SetAbsoluteVolume 
according to AVRCP spec 1.6.2, section 6.13.2:

    The volume level which has actually been set on the TG is returned in
    the response.

I would have proposed a new DBUS function SetAbsoluteVolume that accepts 
volume as sole argument, blocks until the peer replies, and returns the 
actual volume as per its namesake AVRCP command.  This should address 
both issues.

Note that it is also impossible to distinguish Volume property changes 
from an action invoked on the peer (ie. the user pressing physical 
buttons or using a volume slider on headphones) or the result of an 
earlier Set(Volume) call as these to my knowledge all happen async, may 
be intertwined, may involve rounding (on the peer) to make the resulting 
number different, etc.

> 
>> 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 <[e-mail]>
>> Reviewed-by: Miao-chen Chou <[e-mail]>
>>
>> ---
>>   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
> 

- Marijn

Luiz Augusto von Dentz June 7, 2021, 11:47 p.m. UTC | #4

Hi Marijn,

On Mon, Jun 7, 2021 at 3:46 PM Marijn Suijten
<[e-mail]> wrote:
>
> Hi Luiz, Sonny,
>
> On 6/7/21 11:16 PM, Luiz Augusto von Dentz wrote:
> > Hi Sonny,
> >
> > On Mon, Jun 7, 2021 at 1:40 PM Sonny Sasaka <[e-mail]> 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.
>
>
> We've been running into a similar issue with PulseAudio.  There is no
> way to track a Set call for the Volume property back to the
> property-change notification, running into both jitter on quick
> successive volume changes and the inability to reflect peer hardware
> volume in case the peer rounds the requested volume to a coarser scale.
>   This rounded value is supposedly returned from SetAbsoluteVolume
> according to AVRCP spec 1.6.2, section 6.13.2:
>
>     The volume level which has actually been set on the TG is returned in
>     the response.
>
> I would have proposed a new DBUS function SetAbsoluteVolume that accepts
> volume as sole argument, blocks until the peer replies, and returns the
> actual volume as per its namesake AVRCP command.  This should address
> both issues.
>
> Note that it is also impossible to distinguish Volume property changes
> from an action invoked on the peer (ie. the user pressing physical
> buttons or using a volume slider on headphones) or the result of an
> earlier Set(Volume) call as these to my knowledge all happen async, may
> be intertwined, may involve rounding (on the peer) to make the resulting
> number different, etc.

Yep, the volume may actually be rounded like you said, so Im not
really sure how we can queue because we will not be able to verify the
volume has been set properly, we could do a blocking call even in case
of setting as a property we only need to delay the call to
g_dbus_pending_property_success until we actually receive a response,
that said there maybe some impact on the user experience since if you
have the volume implemented with something like a slider it will not
move smoothly anymore, but I guess that is better than have it
flipping back and forth, well except that can still happens because
the volume can be changed on the device in the meantime but I guess
the client wont see those changes until the method returns if it is
blocking waiting for the reply, which means it will only see that the
value flipped to something else later when the signals are dispatched.

If you feel like that is acceptable I'm happy to review any patches in
that direction.

> >
> >> 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 <[e-mail]>
> >> Reviewed-by: Miao-chen Chou <[e-mail]>
> >>
> >> ---
> >>   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
> >
>
> - Marijn

Marijn Suijten June 8, 2021, 9:50 a.m. UTC | #5

Hi Luiz,

On 6/8/21 1:47 AM, Luiz Augusto von Dentz wrote:
> Hi Marijn,
> 
[..]
>> We've been running into a similar issue with PulseAudio.  There is no
>> way to track a Set call for the Volume property back to the
>> property-change notification, running into both jitter on quick
>> successive volume changes and the inability to reflect peer hardware
>> volume in case the peer rounds the requested volume to a coarser scale.
>>    This rounded value is supposedly returned from SetAbsoluteVolume
>> according to AVRCP spec 1.6.2, section 6.13.2:
>>
>>      The volume level which has actually been set on the TG is returned in
>>      the response.
>>
>> I would have proposed a new DBUS function SetAbsoluteVolume that accepts
>> volume as sole argument, blocks until the peer replies, and returns the
>> actual volume as per its namesake AVRCP command.  This should address
>> both issues.
>>
>> Note that it is also impossible to distinguish Volume property changes
>> from an action invoked on the peer (ie. the user pressing physical
>> buttons or using a volume slider on headphones) or the result of an
>> earlier Set(Volume) call as these to my knowledge all happen async, may
>> be intertwined, may involve rounding (on the peer) to make the resulting
>> number different, etc.
> 
> Yep, the volume may actually be rounded like you said, so Im not
> really sure how we can queue because we will not be able to verify the
> volume has been set properly, we could do a blocking call even in case
> of setting as a property we only need to delay the call to
> g_dbus_pending_property_success until we actually receive a response,
> that said there maybe some impact on the user experience since if you
> have the volume implemented with something like a slider it will not
> move smoothly anymore, but I guess that is better than have it
> flipping back and forth, well except that can still happens because
> the volume can be changed on the device in the meantime but I guess
> the client wont see those changes until the method returns if it is
> blocking waiting for the reply, which means it will only see that the
> value flipped to something else later when the signals are dispatched.


The main use-case is software volume compensation for devices with 
limited granularity, for which we need to know exactly what is replied 
to AVRCP's SetAbsoluteVolume call.  Delaying 
g_dbus_pending_property_success will only block the call longer but 
won't exactly let us know the resulting value.  We can immediately Get 
the new property after (or try and relate the change notification to 
it), but there's nothing preventing a change on the device intervening. 
  In that case we should discard requested volume (on the host) and 
software compensation, and take/display device volume as-is.
This seems unfortunate as the AVRCP spec provides exactly the 
consistency we're looking for here.

As for user experience it seems acceptable to make such a call block 
until the peer replies, if only for a much more predictable API.  It is 
then up to the caller (ie. PulseAudio) to deal with that by 
disabling/blocking the slider, or scheduling the most recent volume 
update to be sent as soon as a reply is received (the DBus call returns).

> 
> If you feel like that is acceptable I'm happy to review any patches in
> that direction.
> 

[..]

> --
> Luiz Augusto von Dentz
- Marijn

Sonny Sasaka June 8, 2021, 6:37 p.m. UTC | #6

Hi Marijn,

Thanks for your inputs. Having a separate SetAbsoluteVolume API that
blocks (until the peer acknowledges the change) is certainly more
featureful than this patch's approach, since the media player can
decide what to do with pending SetAbsoluteVolume calls and have the
flexibility to handle the situation. However, there is also a value in
the shorter term approach that this patch without any changes in the
media player part and has been tested to solve the janky slider issue
in Chrome OS. I believe that this would also solve PulseAudio's
similar janky slider issue for the short term.

If Marijn and Luiz are okay with the shorter term approach first, I
will continue updating this patch according to Luiz's comments, but
otherwise I will abandon this patch in favor of the separate
SetAbsoluteVolume API that Marijn will provide.

On Tue, Jun 8, 2021 at 2:50 AM Marijn Suijten
<[e-mail]> wrote:
>
> Hi Luiz,
>
> On 6/8/21 1:47 AM, Luiz Augusto von Dentz wrote:
> > Hi Marijn,
> >
> [..]
> >> We've been running into a similar issue with PulseAudio.  There is no
> >> way to track a Set call for the Volume property back to the
> >> property-change notification, running into both jitter on quick
> >> successive volume changes and the inability to reflect peer hardware
> >> volume in case the peer rounds the requested volume to a coarser scale.
> >>    This rounded value is supposedly returned from SetAbsoluteVolume
> >> according to AVRCP spec 1.6.2, section 6.13.2:
> >>
> >>      The volume level which has actually been set on the TG is returned in
> >>      the response.
> >>
> >> I would have proposed a new DBUS function SetAbsoluteVolume that accepts
> >> volume as sole argument, blocks until the peer replies, and returns the
> >> actual volume as per its namesake AVRCP command.  This should address
> >> both issues.
> >>
> >> Note that it is also impossible to distinguish Volume property changes
> >> from an action invoked on the peer (ie. the user pressing physical
> >> buttons or using a volume slider on headphones) or the result of an
> >> earlier Set(Volume) call as these to my knowledge all happen async, may
> >> be intertwined, may involve rounding (on the peer) to make the resulting
> >> number different, etc.
> >
> > Yep, the volume may actually be rounded like you said, so Im not
> > really sure how we can queue because we will not be able to verify the
> > volume has been set properly, we could do a blocking call even in case
> > of setting as a property we only need to delay the call to
> > g_dbus_pending_property_success until we actually receive a response,
> > that said there maybe some impact on the user experience since if you
> > have the volume implemented with something like a slider it will not
> > move smoothly anymore, but I guess that is better than have it
> > flipping back and forth, well except that can still happens because
> > the volume can be changed on the device in the meantime but I guess
> > the client wont see those changes until the method returns if it is
> > blocking waiting for the reply, which means it will only see that the
> > value flipped to something else later when the signals are dispatched.
>
>
> The main use-case is software volume compensation for devices with
> limited granularity, for which we need to know exactly what is replied
> to AVRCP's SetAbsoluteVolume call.  Delaying
> g_dbus_pending_property_success will only block the call longer but
> won't exactly let us know the resulting value.  We can immediately Get
> the new property after (or try and relate the change notification to
> it), but there's nothing preventing a change on the device intervening.
>   In that case we should discard requested volume (on the host) and
> software compensation, and take/display device volume as-is.
> This seems unfortunate as the AVRCP spec provides exactly the
> consistency we're looking for here.
>
> As for user experience it seems acceptable to make such a call block
> until the peer replies, if only for a much more predictable API.  It is
> then up to the caller (ie. PulseAudio) to deal with that by
> disabling/blocking the slider, or scheduling the most recent volume
> update to be sent as soon as a reply is received (the DBus call returns).
>
> >
> > If you feel like that is acceptable I'm happy to review any patches in
> > that direction.
> >
>
> [..]
>
> > --
> > Luiz Augusto von Dentz
> - Marijn

Marijn Suijten Aug. 8, 2021, 10:15 a.m. UTC | #7

Hi Sonny,

On 6/8/21 8:37 PM, Sonny Sasaka wrote:
> Hi Marijn,
> 
> Thanks for your inputs. Having a separate SetAbsoluteVolume API that
> blocks (until the peer acknowledges the change) is certainly more
> featureful than this patch's approach, since the media player can
> decide what to do with pending SetAbsoluteVolume calls and have the
> flexibility to handle the situation. However, there is also a value in
> the shorter term approach that this patch without any changes in the
> media player part and has been tested to solve the janky slider issue
> in Chrome OS. I believe that this would also solve PulseAudio's
> similar janky slider issue for the short term.
> 
> If Marijn and Luiz are okay with the shorter term approach first, I
> will continue updating this patch according to Luiz's comments, but
> otherwise I will abandon this patch in favor of the separate
> SetAbsoluteVolume API that Marijn will provide.


With no acknowledgement from Luiz yet I've gone ahead and added an 
experimental `uint16 SetVolume(uint16)` call to "MediaTransport1" to 
start testing this behaviour. You can find the commits here:

https://github.com/MarijnS95/bluez/commits/master

This has only been tested with dbus-send, the PA changes still have to 
be written.  Given the original review on Sonny's patches we might want 
to replace the allocation with `pending` members on the session instead, 
so that we can possibly do some debouncing if multiple .Set calls 
arrive.  Seems like we need three members then:

volume: 	current known volume between BlueZ and the peer.
pending_volume: an active AVRCP SetAbsoluteVolume call is in progress
		with this value.  Though this could also be a non-null
		DBusMessage *volume_reply; as we don't need the
		requested volume anymore.
next_volume:	a client already queued a new value through .Set, while
		SetAbsoluteVolume (pending_volume != -1) is still in
		flight.

While putting this together I noticed that manually calling `.Set 
"MediaTransport1" "Volume" XX` doesn't notify other applications.  What 
happens is that `a2dp->volume` is set to the actual volume (in 
set_volume), and no "PropertiesChanged" is emitted unless we're in 
"notify" mode ("we are the headphones").  Then, as soon as the peer 
replies (avrcp_handle_set_volume, media_transport_update_device_volume, 
media_transport_update_volume) we see that `a2dp->volume == volume` and 
never emit "PropertiesChanged" either, leaving all other clients unaware 
of the change.

This seems simply addressed by not setting `a2dp->volume` set_volume() 
and instead rely on that to happen in avrcp_handle_set_volume, just like 
I'm doing in the handler for this new SetVolume function.
That might already solve your problem in CrOS, by simply waiting for a 
property change notification before sending the next volume change.  We 
however won't be able to distinguish it from a button-press on the 
headphones by the user, but I'm not entirely sure anymore if that's 
still needed.

Marijn

PS: Since these messages seem to come through, Luiz have you received my 
patch to address AVRCP Absolute Volume missing when connected to Android 
phones?

Sonny Sasaka Aug. 9, 2021, 5:46 p.m. UTC | #8

Hi Marijn,

Thank you for following up. Chrome OS has temporarily adopted my patch
to resolve the issue without changing the audio client. We will pick
up your patch at the next uprev.


On Sun, Aug 8, 2021 at 3:15 AM Marijn Suijten
<[e-mail]> wrote:
>
> Hi Sonny,
>
> On 6/8/21 8:37 PM, Sonny Sasaka wrote:
> > Hi Marijn,
> >
> > Thanks for your inputs. Having a separate SetAbsoluteVolume API that
> > blocks (until the peer acknowledges the change) is certainly more
> > featureful than this patch's approach, since the media player can
> > decide what to do with pending SetAbsoluteVolume calls and have the
> > flexibility to handle the situation. However, there is also a value in
> > the shorter term approach that this patch without any changes in the
> > media player part and has been tested to solve the janky slider issue
> > in Chrome OS. I believe that this would also solve PulseAudio's
> > similar janky slider issue for the short term.
> >
> > If Marijn and Luiz are okay with the shorter term approach first, I
> > will continue updating this patch according to Luiz's comments, but
> > otherwise I will abandon this patch in favor of the separate
> > SetAbsoluteVolume API that Marijn will provide.
>
>
> With no acknowledgement from Luiz yet I've gone ahead and added an
> experimental `uint16 SetVolume(uint16)` call to "MediaTransport1" to
> start testing this behaviour. You can find the commits here:
>
> https://github.com/MarijnS95/bluez/commits/master
>
> This has only been tested with dbus-send, the PA changes still have to
> be written.  Given the original review on Sonny's patches we might want
> to replace the allocation with `pending` members on the session instead,
> so that we can possibly do some debouncing if multiple .Set calls
> arrive.  Seems like we need three members then:
>
> volume:         current known volume between BlueZ and the peer.
> pending_volume: an active AVRCP SetAbsoluteVolume call is in progress
>                 with this value.  Though this could also be a non-null
>                 DBusMessage *volume_reply; as we don't need the
>                 requested volume anymore.
> next_volume:    a client already queued a new value through .Set, while
>                 SetAbsoluteVolume (pending_volume != -1) is still in
>                 flight.
>
> While putting this together I noticed that manually calling `.Set
> "MediaTransport1" "Volume" XX` doesn't notify other applications.  What
> happens is that `a2dp->volume` is set to the actual volume (in
> set_volume), and no "PropertiesChanged" is emitted unless we're in
> "notify" mode ("we are the headphones").  Then, as soon as the peer
> replies (avrcp_handle_set_volume, media_transport_update_device_volume,
> media_transport_update_volume) we see that `a2dp->volume == volume` and
> never emit "PropertiesChanged" either, leaving all other clients unaware
> of the change.
>
> This seems simply addressed by not setting `a2dp->volume` set_volume()
> and instead rely on that to happen in avrcp_handle_set_volume, just like
> I'm doing in the handler for this new SetVolume function.
> That might already solve your problem in CrOS, by simply waiting for a
> property change notification before sending the next volume change.  We
> however won't be able to distinguish it from a button-press on the
> headphones by the user, but I'm not entirely sure anymore if that's
> still needed.
>
> Marijn
>
> PS: Since these messages seem to come through, Luiz have you received my
> patch to address AVRCP Absolute Volume missing when connected to Android
> phones?

Marijn Suijten Aug. 9, 2021, 8:31 p.m. UTC | #9

Hi Sonny,

On 8/9/21 7:46 PM, Sonny Sasaka wrote:
> Hi Marijn,
> 
> Thank you for following up. Chrome OS has temporarily adopted my patch
> to resolve the issue without changing the audio client. We will pick
> up your patch at the next uprev.

Note that these patches are still draft in search for initial feedback 
on the approach, even if the implementation itself is trivial.  Does 
this mean you won't be able to test this locally until the next uprev? 
Either way I will have to confirm their usefulness in PulseAudio too 
before actually committing to this, so there's no hurry.

Marijn

Sonny Sasaka Aug. 9, 2021, 8:49 p.m. UTC | #10

Hi Marijn,

On Mon, Aug 9, 2021 at 1:31 PM Marijn Suijten
<[e-mail]> wrote:
>
> Hi Sonny,
>
> On 8/9/21 7:46 PM, Sonny Sasaka wrote:
> > Hi Marijn,
> >
> > Thank you for following up. Chrome OS has temporarily adopted my patch
> > to resolve the issue without changing the audio client. We will pick
> > up your patch at the next uprev.
>
> Note that these patches are still draft in search for initial feedback
> on the approach, even if the implementation itself is trivial.  Does
> this mean you won't be able to test this locally until the next uprev?
> Either way I will have to confirm their usefulness in PulseAudio too
> before actually committing to this, so there's no hurry.
Yes, Chrome OS is currently not working on solving this issue since
there is already a local fix. We will adopt the BlueZ fix after your
patch gets merged and is available in BlueZ future version when we
rebase with upstream in the next uprev.

>
> Marijn

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