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

[Luiz Augusto von Dentz <luiz.dentz@gmail.com>]

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