Hi Archie,
On Thu, Jul 9, 2020 at 9:32 PM Archie Pusaka <apusaka@xxxxxxxxxx> wrote:
>
> From: Archie Pusaka <apusaka@xxxxxxxxxxxx>
>
> 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@xxxxxxxxxx>
> ---
>
> 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
